Query range fields by doc values when they are expected to be more efficient than points #24823

martijnvg · 2017-05-22T08:47:13Z

Enable doc values for range fields by default.
Store ranges in a binary format that support multi field fields.
Added BinaryDocValuesRangeQuery that can query ranges that have been encoded into a binary doc values field.
Wrap range queries on a range field in IndexOrDocValuesQuery query.

jpountz

It looks good overall. My main concern is that given how the encoding works, we have to deserialize the ranges at query time back to two Comparable instances. I think we should design it in such a way that queries can work directly on the encoded representation?

jpountz · 2017-05-23T07:55:48Z

core/src/main/java/org/apache/lucene/queries/BinaryDocValuesRangeQuery.java

+                    @Override
+                    public boolean matches() throws IOException {
+                        BytesRef encodedRanges = values.binaryValue();
+                        NormalizedRange[] ranges = rangeType.decodeRanges(encodedRanges);


Could we work directly on the encoded bytes instead of decoding?

jpountz · 2017-05-23T07:57:12Z

core/src/main/java/org/apache/lucene/queries/BinaryDocValuesRangeQuery.java

+
+    @Override
+    public int hashCode() {
+        return classHash() + Objects.hash(fieldName, queryType, expectedRange);


nitpick: I'd prefer return Objects.hash(getClass(), fieldName, queryType, expectedRange); which is supposed to be less subject to collisions.

jpountz · 2017-05-23T08:02:29Z

core/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java

+            public BytesRef encodeRanges(Set<Range> ranges) throws IOException {
+                final byte[] encoded = new byte[ByteUtils.MAX_BYTES_VLONG + (16 * 2) * ranges.size()];
+                ByteArrayDataOutput out = new ByteArrayDataOutput(encoded);
+                out.writeVInt(ranges.size());


since we use a fixed length, maybe we do not need to encode the number of values?

oh I see it is to be consistent with other types which use vlong

martijnvg · 2017-05-23T13:45:19Z

I think we should design it in such a way that queries can work directly on the encoded representation?

The idea here is that we can save some space by encoding the ranges as two vlongs. Downside is that we can't work on the encoded representation. Do you think that the vlong encoding isn't worth the effort here? Personally I'm ok with changing the encoding here, so that we don't to encode at query time, but just wondering if we should make a tradeoff between space or query speed.

jpountz · 2017-05-23T15:51:41Z

You convinced me that we need to perform some sort of compression for numeric values. However I think we should use an encoding that doesn't require the query to know whether the field stores longs or ip addresses?

jpountz

I left some comments.

jpountz · 2017-05-31T07:39:44Z

core/src/main/java/org/apache/lucene/queries/BinaryDocValuesRangeQuery.java

+        }
+        if (includeTo) {
+            to = rangeType.nextUp(to);
+        }


This looks a bit backward to me, we would usually rather do

if (includeFrom == false) { from = rangeType.nextUp(from); }

Do I miss something?

jpountz · 2017-05-31T07:42:09Z

core/src/main/java/org/apache/lucene/queries/BinaryDocValuesRangeQuery.java

+            ByteArrayDataInput in = new ByteArrayDataInput(encodedRanges.bytes, encodedRanges.offset, encodedRanges.length);
+            int numRanges = in.readVInt();
+            for (int i = 0; i < numRanges; i++) {
+                int length = in.readVInt();


the first bytes are not a vint when we encode eg. longs? So I don't think it works?

In RangeFieldMapper.RangeType#encodeLongRanges(...) each encoded value is preceded by a vint.

jpountz · 2017-05-31T07:42:51Z

core/src/main/java/org/apache/lucene/queries/BinaryDocValuesRangeQuery.java

+                length = in.readVInt();
+                bytes = new byte[length];
+                in.readBytes(bytes, 0, length);
+                BytesRef otherTo = new BytesRef(bytes);


We should try to avoid ByteArrayDataInput and BytesRef allocations since this method may be called in a tight loop.

I can remove the ByteRef instance creation and use just plain byte[].
The reason I used ByteArrayDataInput is because of its readVInt() and readBytes() methods. Are there static alternatives that I can use? Otherwise maybe it is sufficient enough if I keep a spare instance around that I keep reusing?

jpountz · 2017-05-31T07:43:25Z

core/src/main/java/org/apache/lucene/queries/BinaryDocValuesRangeQuery.java

+                        // does not disjoint AND not within:
+                        result =  (from.compareTo(otherTo) > 0 || to.compareTo(otherFrom) < 0) == false &&
+                            (from.compareTo(otherFrom) <= 0 && to.compareTo(otherTo) >= 0) == false;
+                        break;


should we put a method on the queryType enum instead of this switch?

jpountz · 2017-05-31T07:45:33Z

core/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java

+            }
+            return encoded;
+        }
+


I'm wondering that putting these helpers into a dedicated class would make things easier to read?

jpountz · 2017-05-31T07:48:19Z

core/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java

+                if (sign == 1) {
+                    encoded[encoded.length - 1 - b] = (byte) (l >>> (8 * b));
+                } else if (sign == 0) {
+                    encoded[encoded.length - 1 - b] = (byte) (0xFF - (l >>> (8 * b)));


I think we miss a mask here: encoded[encoded.length - 1 - b] = (byte) (0xFF - ((l >>> (8 * b)) & 0xFF));

jpountz · 2017-05-31T07:54:45Z

core/src/test/java/org/apache/lucene/queries/LongRandomBinaryDocValuesRangeQueryTests.java

+        }
+        long max = Long.MAX_VALUE / 2;
+        return (max + max) * random().nextLong() - max;
+    }


We had similar random number generation in eg. TestIntRangeFieldQueries but this proved inefficient at finding bugs (eg. it makes it very unlikely to get twice the same value in the same test) so we changed it in https://issues.apache.org/jira/browse/LUCENE-7847. Maybe we should do something similar here (as well as for doubles, ip addresses, etc.)

jpountz · 2017-05-31T07:59:56Z

core/src/main/java/org/apache/lucene/queries/BinaryDocValuesRangeQuery.java

+    private final QueryType queryType;
+    private final NormalizedRange expectedRange;
+
+    public BinaryDocValuesRangeQuery(String fieldName, RangeFieldMapper.RangeType rangeType, QueryType queryType, Object from, Object to,


Can we avoid passing the range type? I think it would be more robust if it does not need to know about the type of values that are stored and works directly on the encoded data?

martijnvg · 2017-05-31T15:13:53Z

@jpountz Thanks for reviewing. I've updated the PR.

jpountz

It looks good overall, I left some comments. I'm wondering whether we should sort the ranges before encoding them as it might allow for optimizations in the multi-valued case eg. if the max value of the query is less than the min value of the first encoded range.

It would be nice to have some benchmarks as well in order to make sure that this actually performs better than point-based ranges when the range does not drive iteration (it might make more sense to do it after LUCENE-7828 is merged). I can help with that.

jpountz · 2017-06-02T07:38:26Z

core/src/main/java/org/apache/lucene/queries/BinaryDocValuesRangeQuery.java

+                        int numRanges = in.readVInt();
+                        for (int i = 0; i < numRanges; i++) {
+                            otherFrom.length = in.readVInt();
+                            in.readBytes(otherFrom.bytes, 0, otherFrom.length);


maybe we could avoid the copy by doing this instead:

otherFrom.bytes = encodedRanges.bytes; otherFrom.offset = in.position(); in.skip(otherFrom.length);

and similarly for the to value?

jpountz · 2017-06-02T07:58:49Z

core/src/main/java/org/elasticsearch/index/mapper/BinaryRangeUtil.java

+        return new BytesRef(encoded, 0, out.getPosition());
+    }
+
+    static byte[] encode(long l) {


Maybe add javadocs saying that this is variable-length encoding that preserves ordering?

Also I think we need some unit tests for that method in order to ensure it actually preserves ordering?

jpountz · 2017-06-02T08:09:48Z

core/src/main/java/org/elasticsearch/index/mapper/BinaryRangeUtil.java

+            l = Double.doubleToRawLongBits(d);
+            sign = 1; // means positive
+        }
+        return encode(l, sign);


doubles should probably use a different encoding, but we can work on it as a follow-up

Also I think we should specialize floats too

martijnvg · 2017-06-07T12:20:56Z

@jpountz I've updated the pr.

I'm wondering whether we should sort the ranges before encoding them as it might allow for optimizations in the multi-valued case eg. if the max value of the query is less than the min value of the first encoded range.

I've added the sorting, but I think it will only make sense when there multiple adjacent ranges and when ranges are overlapping it wouldn't help that much?

It would be nice to have some benchmarks as well in order to make sure that this actually performs better than point-based ranges when the range does not drive iteration (it might make more sense to do it after LUCENE-7828 is merged). I can help with that.

Thanks! I guess we will need a dataset that when benchmarked with a specific query forces the range query to be executed in random access mode.

jpountz · 2017-06-07T14:07:42Z

I've added the sorting, but I think it will only make sense when there multiple adjacent ranges and when ranges are overlapping it wouldn't help that much?

Right I don't think this is a game changer for queries in general. However it might be the case for aggregations? For instance, say you want to run an histogram aggregation on a range field, it would be easier with sorted ranges, since you can easily track which buckets have already been incremented and which buckets haven't?

jpountz · 2017-06-08T08:09:53Z

I merged LUCENE-7828 yesterday so that we can better benchmark this change, but upgrading to a new Lucene 7 snapshot is currently blocked on #25028 since the postings highlighter has been removed from Lucene master. It should hopefully be merged in the next few days.

martijnvg · 2017-07-05T08:56:57Z

(Updated the benchmark to index the docs into arbitrary order and captured the 50th precentile instead of the 99th because it is less noisy)

This are the 50th percentile latency results between master (baseline) and this pr (contender) of the new benchmark added to rally:

Metric	Operation	Baseline	Contender	Diff	Unit
50th percentile latency	range_field_big_range	43.802	43.7695	-0.03246	ms
50th percentile latency	range_field_small_range	20.4401	14.685	-5.75507	ms
50th percentile latency	range_field_conjunction_big_range_small_term_query	53.0726	19.2134	-33.8592	ms
50th percentile latency	range_field_conjunction_small_range_small_term_query	26.9517	18.5648	-8.38684	ms
50th percentile latency	range_field_conjunction_small_range_big_term_query	75.8826	67.3323	-8.55028	ms
50th percentile latency	range_field_conjunction_big_range_big_term_query	20651	17353	-3298.01	ms
50th percentile latency	range_field_disjunction_small_range_small_term_query	49.0655	39.0006	-10.0649	ms
50th percentile latency	range_field_disjunction_big_range_small_term_query	7185.2	86.6476	-7098.55	ms

jpountz

I think we need to do 2 things before merging:

back compat in the mapper so that the pre-6.0 mappers know they do not have doc values
sort ranges by from then to

jpountz · 2017-07-11T12:27:36Z

core/src/main/java/org/apache/lucene/queries/BinaryDocValuesRangeQuery.java

+
+    @Override
+    public String toString(String field) {
+        return "BinaryDocValuesRangeQuery(fieldName=" + field + ")";


let's put from and to in the toSTring() ?

jpountz · 2017-07-11T12:40:12Z

core/src/main/java/org/elasticsearch/index/mapper/BinaryRangeUtil.java

+            long r2From = ((Number) r2.from).longValue();
+            long r2To = ((Number) r2.from).longValue();
+            long middle2 = r2From + ((r2To - r2From) / 2);
+            return Long.compare(middle1, middle2);


I'd rather sort by from then to, feels like something that is easier to rely on.

jpountz · 2017-07-11T12:54:21Z

core/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java

@@ -194,7 +201,7 @@ public RangeFieldType(RangeType type) {
            super();
            this.rangeType = Objects.requireNonNull(type);
            setTokenized(false);
-            setHasDocValues(false);
+            setHasDocValues(true);


I think we need to handle backcompat in the parser to make sure we use false as a default value if the index was created before 6.0?

martijnvg · 2017-07-11T14:57:37Z

@jpountz I've updated the pr.

…ficient than points. * Enable doc values for range fields by default. * Store ranges in a binary format that support multi field fields. * Added BinaryDocValuesRangeQuery that can query ranges that have been encoded into a binary doc values field. * Wrap range queries on a range field in IndexOrDocValuesQuery query. Closes elastic#24314

martijnvg added :Search/Search Search-related issues that do not fall into other categories >enhancement review v6.0.0 labels May 22, 2017

jpountz reviewed May 23, 2017

View reviewed changes

martijnvg force-pushed the dv_ranges branch from 835613c to 0bee2d3 Compare May 26, 2017 15:15

jpountz reviewed May 31, 2017

View reviewed changes

jpountz reviewed Jun 2, 2017

View reviewed changes

colings86 mentioned this pull request Jun 26, 2017

range_histogram and date_range_histogram aggregations to help analyse "session" duration type data #23182

Closed

martijnvg force-pushed the dv_ranges branch 2 times, most recently from c8f6f44 to 6aaab01 Compare July 4, 2017 07:51

jpountz requested changes Jul 11, 2017

View reviewed changes

martijnvg force-pushed the dv_ranges branch from 6aaab01 to 731b9e2 Compare July 11, 2017 14:57

jpountz approved these changes Jul 12, 2017

View reviewed changes

martijnvg force-pushed the dv_ranges branch from 731b9e2 to 996f200 Compare July 12, 2017 07:55

martijnvg force-pushed the dv_ranges branch from 996f200 to 0a25558 Compare July 12, 2017 11:04

martijnvg merged commit 0a25558 into elastic:master Jul 12, 2017

colings86 added v6.0.0-beta1 and removed v6.0.0 labels Jul 31, 2017

$@polyfractal$ polyfractal mentioned this pull request Mar 21, 2018

Histogram aggregation on range fields using lte/lt or gte/gt #23414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query range fields by doc values when they are expected to be more efficient than points #24823

Query range fields by doc values when they are expected to be more efficient than points #24823

martijnvg commented May 22, 2017

jpountz left a comment

jpountz May 23, 2017

jpountz May 23, 2017

jpountz May 23, 2017

jpountz May 23, 2017

martijnvg commented May 23, 2017

jpountz commented May 23, 2017

jpountz left a comment

jpountz May 31, 2017

jpountz May 31, 2017

martijnvg May 31, 2017

jpountz May 31, 2017

martijnvg May 31, 2017

jpountz May 31, 2017

jpountz May 31, 2017

jpountz May 31, 2017

jpountz May 31, 2017

jpountz May 31, 2017

martijnvg commented May 31, 2017

jpountz left a comment

jpountz Jun 2, 2017

jpountz Jun 2, 2017

jpountz Jun 2, 2017

jpountz Jun 2, 2017

jpountz Jun 2, 2017

martijnvg commented Jun 7, 2017

jpountz commented Jun 7, 2017

jpountz commented Jun 8, 2017

martijnvg commented Jul 5, 2017 •

edited

Loading

jpountz left a comment

jpountz Jul 11, 2017

jpountz Jul 11, 2017

jpountz Jul 11, 2017

martijnvg commented Jul 11, 2017

Query range fields by doc values when they are expected to be more efficient than points #24823

Query range fields by doc values when they are expected to be more efficient than points #24823

Conversation

martijnvg commented May 22, 2017

jpountz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented May 23, 2017

jpountz commented May 23, 2017

jpountz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented May 31, 2017

jpountz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Jun 7, 2017

jpountz commented Jun 7, 2017

jpountz commented Jun 8, 2017

martijnvg commented Jul 5, 2017 • edited Loading

jpountz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Jul 11, 2017

martijnvg commented Jul 5, 2017 •

edited

Loading