Add early termination support for min/max aggregations #33375

jimczi · 2018-09-04T12:31:29Z

This commit adds the support to early terminate the collection of a leaf
in the min/max aggregator. If the query matches all documents the min and max value
for a numeric field can be retrieved efficiently in the points reader.
This change applies this optimization when possible.

This commit adds the support to early terminate the collection of a leaf in the min/max aggregator. If the query matches all documents the min and max value for a numeric field can be retrieved efficiently in the points reader. This change applies this optimization when possible.

elasticmachine · 2018-09-04T12:31:31Z

Pinging @elastic/es-search-aggs

colings86 · 2018-09-04T12:41:04Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/max/MaxAggregator.java

            SearchContext context,
            Aggregator parent, List<PipelineAggregator> pipelineAggregators,
-            Map<String, Object> metaData) throws IOException {
+            Map<String, Object> metaData, CheckedFunction<LeafReader, Number, IOException> maxFunc) throws IOException {


I think we should call this something other than maxFunc as it confuses what its actually doing (getting the max for the segment) from the normal running of the agg. Maybe calling it segmentMaxFunction would be clearer?

colings86 · 2018-09-04T12:42:32Z

...er/src/main/java/org/elasticsearch/search/aggregations/metrics/max/MaxAggregatorFactory.java

+     * @param fieldName The name of the field.
+     * @param converter The point value converter.
+     */
+    public static CheckedFunction<LeafReader, Number, IOException> createShortcutMax(String fieldName, Function<byte[], Number> converter) {


This feels weird to me here. I wonder if we should instead have this method in the aggregator class and pass in a boolean parameter that determines whether we use it?

We need the converter so we could pass it to the aggregator directly and if it's not null we can try to extract the min/max value from points ?

++ I like that better. I'm not a fan of the Aggregators calling back out to the factory so if we can help that not happen then I'm on board 😄

can it be pkg-private?

This is used in tests that are in a different package (metrics). We could probably merge all metrics.* in a single package instead of having one package per metrics agg ?

jpountz

I left some questions.

jpountz · 2018-09-04T15:59:23Z

...er/src/main/java/org/elasticsearch/search/aggregations/metrics/max/MaxAggregatorFactory.java

+                        }
+                    }
+                });
+            } catch (CollectionTerminatedException e) {}


why do we catch CollectionTerminatedException here?

this is not needed, thanks

jpountz · 2018-09-04T16:02:01Z

...er/src/main/java/org/elasticsearch/search/aggregations/metrics/max/MaxAggregatorFactory.java

+
+                    @Override
+                    public PointValues.Relation compare(byte[] minPackedValue, byte[] maxPackedValue) {
+                        if (FutureArrays.compareUnsigned(maxValue, 0, numBytes, maxPackedValue, 0, numBytes) == 0) {


Arrays#equals? :)

jpountz · 2018-09-04T16:02:52Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/max/MaxAggregator.java

    public LeafBucketCollector getLeafCollector(LeafReaderContext ctx,
            final LeafBucketCollector sub) throws IOException {
        if (valuesSource == null) {
            return LeafBucketCollector.NO_OP_COLLECTOR;


should we early terminate in this case too?

if we can (no parent agg), yes

jpountz · 2018-09-04T16:04:51Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/max/MaxAggregator.java

+            double max = maxes.get(0);
+            max = Math.max(max, segMax.doubleValue());
+            maxes.set(0, max);
+            throw new CollectionTerminatedException();


how do we know it's ok to throw this exception and that we won't terminate parent aggregators too?

currently this is checked in the factory when we create the function which will return null if the aggregator has a parent.

jpountz · 2018-09-04T16:05:22Z

...er/src/main/java/org/elasticsearch/search/aggregations/metrics/max/MaxAggregatorFactory.java

+     * @param fieldName The name of the field.
+     * @param converter The point value converter.
+     */
+    public static CheckedFunction<LeafReader, Number, IOException> createShortcutMax(String fieldName, Function<byte[], Number> converter) {


can it be pkg-private?

jpountz · 2018-09-04T16:08:10Z

...er/src/main/java/org/elasticsearch/search/aggregations/metrics/min/MinAggregatorFactory.java

+            if (fieldType == null || fieldType.indexOptions() == IndexOptions.NONE) {
+                return null;
+            }
+            Function<byte[], Number> converter = null;


should we also verify that the script is null?

yes good catch

jpountz

I like it, just left some minor comments. Could we also have test for the case that a field has doc values but is not indexed, so that its min/max values can't be computed?

jpountz · 2018-09-07T12:02:59Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/max/MaxAggregator.java

+
+            @Override
+            public PointValues.Relation compare(byte[] minPackedValue, byte[] maxPackedValue) {
+                if (FutureArrays.compareUnsigned(maxValue, 0, numBytes, maxPackedValue, 0, numBytes) == 0) {


Arrays#equals?

It's a java9 method, at least the one that takes a start and end offset, and I am not sure we can use the java8 Arrays#equals(byte[], byte[]) since the min and max can have a length greater than numBytes ?

I switched to FutureArrays#equals which is equivalent

jpountz · 2018-09-07T12:04:25Z

server/src/test/java/org/elasticsearch/search/aggregations/metrics/MaxAggregatorTests.java

+
+            @Override
+            public PointValues.Relation compare(byte[] minPackedValue, byte[] maxPackedValue) {
+                if (FutureArrays.compareUnsigned(maxPackedValue, 0,  numBytes, maxValue, 0, numBytes) == 0) {


Arrays#equals?

I switched to FutureArrays#equals which is equivalent

jpountz · 2018-09-07T12:07:15Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/min/MinAggregator.java

+                    if (liveDocs.get(docID)) {
+                        result[0] = converter.apply(packedValue);
+                        // this is the first leaf with a live doc so the value is the minimum for this segment.
+                        throw new CollectionTerminatedException();


I'd rather use a similar mechanism as max and avoid throwing an exception here.

For the max we can only check leaves that contain the max value of the segment but for min we can start from the first leaf and stop whenever we reach a non-deleted document. If we apply the same logic than max to avoid the exception we'd be able to find the minimum value only on leaves that contain the real minimum value regardless of deletions. I am fine with changing the logic here but just wanted to outline the implications.

jpountz · 2018-09-07T12:11:18Z

You can ignore my comment about testing, I just saw there was one already.

colings86

I left two minor comments but LGTM

server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java

server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java

This commit adds the support to early terminate the collection of a leaf in the min/max aggregator. If the query matches all documents the min and max value for a numeric field can be retrieved efficiently in the points reader. This change applies this optimization when possible.

jimczi added >enhancement :Analytics/Aggregations Aggregations v7.0.0 v6.5.0 labels Sep 4, 2018

colings86 reviewed Sep 4, 2018

View reviewed changes

jpountz reviewed Sep 4, 2018

View reviewed changes

apply feedback

c19d7a8

jpountz reviewed Sep 7, 2018

View reviewed changes

jimczi added 4 commits September 12, 2018 10:00

Merge branch 'master' into early_terminate_min_max

07a0a75

fixed redundant public modifier for java11

9350f6d

fix test compilation

c954fa0

fix IT

a29edad

colings86 approved these changes Oct 3, 2018

View reviewed changes

server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java Show resolved Hide resolved

server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java Show resolved Hide resolved

jimczi added 2 commits October 3, 2018 11:25

Merge branch 'master' into early_terminate_min_max

5664f55

address review comments

295a0f4

jimczi merged commit ee21067 into elastic:master Oct 3, 2018

jimczi deleted the early_terminate_min_max branch October 3, 2018 16:33

$@polyfractal$ polyfractal mentioned this pull request Oct 18, 2018

Optimize min/max aggs to use index stats when applicable #14835

Closed

Mpdreamz mentioned this pull request Dec 13, 2018

[meta] 6.5.0 Release elastic/elasticsearch-net#3457

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Add early termination support for min/max aggregations #33375

Add early termination support for min/max aggregations #33375

Uh oh!

Conversation

jimczi commented Sep 4, 2018

Uh oh!

elasticmachine commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpountz commented Sep 7, 2018

Uh oh!

colings86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!