Decode functions for range field binary encoded doc values #41206
Conversation
not-napoleon
added
>enhancement
:Search Foundations/Mapping
|
Pinging @elastic/es-search |
| static List<RangeFieldMapper.Range> decodeRanges(BytesRef encodedRanges, RangeFieldMapper.RangeType rangeType, | ||
| TriFunction<byte[], Integer, Integer, Object> decodeBytes) { | ||
|
|
||
| BinaryDocValuesRangeQuery.LengthType lengthType = rangeType.lengthType; |
There was a problem hiding this comment.
Nit: It seems a bit weird for a utility class to be using something from a query class rather than the other way around. Maybe we should consider either moving BinaryDocValuesRangeQuery.LengthType to be an inner class of this BinaryRangeUtil class or make it a standalone class on its own?
There was a problem hiding this comment.
Or even make it an inner class of RangeFieldMapper?
There was a problem hiding this comment.
I kind of feel like we already have too many inner classes crammed into RangeFieldMapper, but moving LengthType to BinaryRangeUtil makes a lot of sense to me. Pushed a change set doing that :)
server/src/test/java/org/elasticsearch/index/mapper/BinaryRangeUtilTests.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/BinaryRangeUtilTests.java
Show resolved
Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/BinaryRangeUtilTests.java
Show resolved
Hide resolved
| import java.util.Comparator; | ||
| import java.util.List; | ||
| import java.util.Set; | ||
|
|
||
| enum BinaryRangeUtil { | ||
| public enum BinaryRangeUtil { |
There was a problem hiding this comment.
Can we extract LengthType in his own file and leave this class package protected ? The encoding should remain internal.
There was a problem hiding this comment.
So, I feel really bad about making LengthType a top level enum; it's very much an implementation detail of the range encoding. The more I thought about it, the more I came to feel it really should be part of RangeType, and my only objection to putting it there in the first place was that RangeFieldMapper is already 1000 lines and defines half a dozen classes. So I made RangeType a top level enum and put LengthType under that. RangeType needs to be public anyway, so there's no increased API surface with this arrangement.
This seems like the most natural refactoring to me, since LengthType is a direct function of RangeType, but I'm open to rolling that back and just making LengthType a top level if you feel strongly that's the right way to do this. Thanks for the feedback!
There was a problem hiding this comment.
ok fine with me, thanks for explaining
This PR is part of the range aggregation work from #34644. Decoding functions will be necessary for aggregations to operate on the numeric values of the ranges.
The decode logic for IP, Float, and Double values is straightforward as the classes we use for encoding already provide a decoder. Longs are a custom encoding, and I hand-rolled the bit manipulation to do the decoding. Reviewer, please pay the most attention to this. I believe my logic to be sound and my test cases are passing, but it is the part of this work I am least confidant in. Thanks.
In addition to the decode logic, I did some small refactoring:
RangeTypeenum; I moved it toBinaryRangeUtil, where all the other encoding logic is.equals(andhashcode) function forRange, to make life easier for writing tests.LengthType#readLengthmethod public, and madeLengthTypea field ofRangeTypesince those two concepts are fundamentally linked.Also note, I expect a small conflict with #41160. I'll fix once that merges.