Optimize ST_EXTENT_AGG for geo_shape and cartesian_shape #119889

craigtaverner · 2025-01-09T17:21:20Z

Support for ST_EXTENT_AGG was added in #118829, and then partially optimized in #118829. This optimization worked only for cartesian_shape fields, and worked by extracting the Extent from the doc-values and re-encoding it as a WKB BBOX geometry. This does not work for geo_shape, where we need to retain all 6 integers stored in the doc-values, in order to perform the datelline choice only at reduce time during the final phase of the aggregation.

Since both geo_shape and cartesian_shape perform the aggregations using integers, and the original Extent values in the doc-values are integers, this PR expands the previous optimization by:

Saving all Extent values into a multi-valued field in an IntBlock for both cartesian_shape and geo_shape
Simplifying the logic around merging intermediate states for all cases (geo/cartesian and grouped and non-grouped aggs)
Widening test cases for testing more combinations of aggregations and types, and fixing a few bugs found

… and geo shapes

…oshape

elasticsearchmachine · 2025-01-09T17:22:16Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2025-01-09T17:22:16Z

Hi @craigtaverner, I've created a changelog YAML for you.

This reverts commit 898d1ba.

…uster issues Currently the fact that this index is also used for ENRICH means the test infrastructure duplicates the index across the cluster, which leads to incorrect results in COUNT stats (double the rows). We avoid this by using airports index only if we want to get counts, and ENRICH against airport boundaries to get the geo_shapes.

These versions cannot use COUNT() due to the issue with indexes that are also used in enrich policies being duplicated across multi-cluster IT environments.

…hange in meaning Of some int values in the block, particularly those that mean unset or infinity.

We had many different parameter orderings, but want to stardize on just two: * For cartesian we use Rectangle class order (minX, maxX, maxY, minY) * For geo we use Extent class order (top, bottom, negLeft, negRight, posLeft, posRight) Since the PR already changed the meaning of the invalid/infinite values of the intermediate state integers, it was already not compatible with the previous cluster versions. We disabled mixed-cluster testing to prevent errors as a result of that. This leaves us the opportunity to make further changes that are mixed-cluster incompatible, hence the decision to perform this consistency update now.

GalLalouche · 2025-01-12T10:45:00Z

libs/geo/src/main/java/org/elasticsearch/geometry/utils/SpatialEnvelopeVisitor.java

@@ -116,6 +116,8 @@ public interface PointVisitor {
        boolean isValid();

        Rectangle getResult();
+
+        void reset();


Consider adding a line of documentation on what this is supposed to do.

GalLalouche · 2025-01-12T10:48:41Z

libs/geo/src/main/java/org/elasticsearch/geometry/utils/SpatialEnvelopeVisitor.java

-            } else if (Double.isInfinite(minNegX)) {
-                return new Rectangle(minPosX, maxPosX, maxY, minY);
+            assert Double.isFinite(top);
+            // Due to this data coming through Extent (and aggs that use the same approach), which saves values as integers,


I'm not sure I understand this comment. For one thing, why does the data coming in as integers mean we must use the below equation? For another, we don't actually do it! We just check for infinity.

The comment is meant to explain why the code works. We could change the code to be more like the comment, but I wanted to minimize code changes, while simultaneously increasing clarity by adding the comment. The key point here is when looking for infinities we have to look at posRight and negLeft.

I'll delete the comment and change the code to instead do that explicitly.

GalLalouche · 2025-01-12T10:53:39Z

server/src/main/java/org/elasticsearch/index/mapper/AbstractShapeGeometryFieldMapper.java

-                        );
-                        builder.appendBytesRef(new BytesRef(WellKnownBinary.toWKB(rectangle, ByteOrder.LITTLE_ENDIAN)));
+                        // We store the 6 values as a single multi-valued field, in the same order as the fields in the Extent class
+                        // This requires that consumers also know the meaning of the values, which they can learn from the Extent class


Not sure I understood the second line here. Perhaps rephrase?

I cannot think of a clearer way of saying this. Can you make a suggestion?

Is it the word 'Consumer'? I consider this code the producer (produces an int[6] array). Any consumer that should read that array needs to know what each of the 6 values means. For example, it needs to know that a[1] means the bottom latitude (minimum y value).

Perhaps it could be rephrased as: "See that class for the meaning of these values".

I see now that you were asking for less verbocity, since my two comment lines overlap quite a bit. I've deleted the second line, as I agree it is excessive.

GalLalouche · 2025-01-12T11:04:51Z

...ugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/AggregatorImplementer.java

@@ -497,6 +511,23 @@ private void combineRawInputForBytesRef(MethodSpec.Builder builder, String block
        builder.addStatement("$T.combine(state, $L.getBytesRef(i, scratch))", declarationType, blockVariable);
    }

+    private void startWarningsBlock(MethodSpec.Builder builder) {


Since any piece of code which starts with startWarningBlock should always end with endWarningsBlock, I think this should be enforced, e.g., by passing in a lambda to a template which wraps it with those two functions.

That is possible, but note that this is not new code and the previous version of this did not do that either.

If this is not enforced, and someone makes a mistake, the generated code will not compile, so we immediately get a failure. I think that is sufficient, and probably the reason this was not done before.

GalLalouche · 2025-01-12T11:11:33Z

...ugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/AggregatorImplementer.java

+
+    private String valueTypeString() {
+        String valueTypeString = TypeName.get(valueTypeMirror()).toString();
+        if (valuesIsArray) {


Consider using ternary expression instead.

GalLalouche · 2025-01-12T11:31:54Z

...rg/elasticsearch/xpack/esql/optimizer/rules/physical/local/SpatialShapeBoundsExtraction.java

@@ -49,9 +50,20 @@
 public class SpatialShapeBoundsExtraction extends ParameterizedOptimizerRule<AggregateExec, LocalPhysicalOptimizerContext> {
    @Override
    protected PhysicalPlan rule(AggregateExec aggregate, LocalPhysicalOptimizerContext ctx) {
-        var foundAttributes = new HashSet<Attribute>();
+        var foundAttributes = findSpatialShapeBoundsAttributes(aggregate, ctx);


Don't use var here, as the type isn't clear from the name or expression.

GalLalouche · 2025-01-12T11:35:42Z

...rg/elasticsearch/xpack/esql/optimizer/rules/physical/local/SpatialShapeBoundsExtraction.java

+        return foundAttributes;
+    }
+
+    private PhysicalPlan transformFieldExtractExec(FieldExtractExec fieldExtractExec, Set<Attribute> foundAttributes) {


Can be static.

GalLalouche · 2025-01-12T11:35:53Z

...rg/elasticsearch/xpack/esql/optimizer/rules/physical/local/SpatialShapeBoundsExtraction.java

+        return fieldExtractExec.withBoundsAttributes(boundsAttributes);
+    }
+
+    private PhysicalPlan transformAggregateExec(AggregateExec agg, Set<Attribute> foundAttributes) {


Can be static.

GalLalouche · 2025-01-12T11:36:11Z

...rg/elasticsearch/xpack/esql/optimizer/rules/physical/local/SpatialShapeBoundsExtraction.java


-        return aggregate.transformDown(UnaryExec.class, exec -> {
+    private Set<Attribute> findSpatialShapeBoundsAttributes(AggregateExec aggregate, LocalPhysicalOptimizerContext ctx) {


Can be static.

GalLalouche · 2025-01-12T11:45:16Z

.../esql/src/test/java/org/elasticsearch/xpack/esql/planner/TestPhysicalOperationProviders.java

+                for (int c = 0; c < docIndices.getPositionCount(); c++) {
+                    int doc = docIndices.getInt(c);
+                    int count = bytesRefBlock.getValueCount(doc);
+                    int i = bytesRefBlock.getFirstValueIndex(doc);


Move this nearer to where it is used?

I wanted it close to the previous line, so we get the key information we need from the bytesRefBlock in one place.

I found a way to move it and make it look better.

…r/elasticsearch into st_extent_optimize_geoshape

We left the bug as a TODO for the next PR, so at least the primary optimization can get merged for tech-preview in 8.18.

…r/elasticsearch into st_extent_optimize_geoshape

This is to increase the chance that the same failure seen for geo_shape in serverless would also occur with cartesian_shape in serverless.

elasticsearchmachine · 2025-01-16T18:45:11Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 119889

) Support for `ST_EXTENT_AGG` was added in elastic#118829, and then partially optimized in elastic#118829. This optimization worked only for cartesian_shape fields, and worked by extracting the Extent from the doc-values and re-encoding it as a WKB `BBOX` geometry. This does not work for geo_shape, where we need to retain all 6 integers stored in the doc-values, in order to perform the datelline choice only at reduce time during the final phase of the aggregation. Since both geo_shape and cartesian_shape perform the aggregations using integers, and the original Extent values in the doc-values are integers, this PR expands the previous optimization by: * Saving all Extent values into a multi-valued field in an IntBlock for both cartesian_shape and geo_shape * Simplifying the logic around merging intermediate states for all cases (geo/cartesian and grouped and non-grouped aggs) * Widening test cases for testing more combinations of aggregations and types, and fixing a few bugs found * Enhancing cartesian extent to convert from 6 ints to 4 ints at block loading time (for efficiency) * Fixing bugs in both cartesian and geo extents for generating intermediate state with missing groups (flaky tests in serverless) * Moved the int order to always match Rectangle for 4-int and Extent for 6-int cases (improved internal consistency) Since the PR already changed the meaning of the invalid/infinite values of the intermediate state integers, it was already not compatible with the previous cluster versions. We disabled mixed-cluster testing to prevent errors as a result of that. This leaves us the opportunity to make further changes that are mixed-cluster incompatible, hence the decision to perform this consistency update now.

…) (#122276) * Optimize ST_EXTENT_AGG for geo_shape and cartesian_shape (#119889) Support for `ST_EXTENT_AGG` was added in #118829, and then partially optimized in #118829. This optimization worked only for cartesian_shape fields, and worked by extracting the Extent from the doc-values and re-encoding it as a WKB `BBOX` geometry. This does not work for geo_shape, where we need to retain all 6 integers stored in the doc-values, in order to perform the datelline choice only at reduce time during the final phase of the aggregation. Since both geo_shape and cartesian_shape perform the aggregations using integers, and the original Extent values in the doc-values are integers, this PR expands the previous optimization by: * Saving all Extent values into a multi-valued field in an IntBlock for both cartesian_shape and geo_shape * Simplifying the logic around merging intermediate states for all cases (geo/cartesian and grouped and non-grouped aggs) * Widening test cases for testing more combinations of aggregations and types, and fixing a few bugs found * Enhancing cartesian extent to convert from 6 ints to 4 ints at block loading time (for efficiency) * Fixing bugs in both cartesian and geo extents for generating intermediate state with missing groups (flaky tests in serverless) * Moved the int order to always match Rectangle for 4-int and Extent for 6-int cases (improved internal consistency) Since the PR already changed the meaning of the invalid/infinite values of the intermediate state integers, it was already not compatible with the previous cluster versions. We disabled mixed-cluster testing to prevent errors as a result of that. This leaves us the opportunity to make further changes that are mixed-cluster incompatible, hence the decision to perform this consistency update now. * Regenerate generated files

…ic#119889) (elastic#122276) * Optimize ST_EXTENT_AGG for geo_shape and cartesian_shape (elastic#119889) Support for `ST_EXTENT_AGG` was added in elastic#118829, and then partially optimized in elastic#118829. This optimization worked only for cartesian_shape fields, and worked by extracting the Extent from the doc-values and re-encoding it as a WKB `BBOX` geometry. This does not work for geo_shape, where we need to retain all 6 integers stored in the doc-values, in order to perform the datelline choice only at reduce time during the final phase of the aggregation. Since both geo_shape and cartesian_shape perform the aggregations using integers, and the original Extent values in the doc-values are integers, this PR expands the previous optimization by: * Saving all Extent values into a multi-valued field in an IntBlock for both cartesian_shape and geo_shape * Simplifying the logic around merging intermediate states for all cases (geo/cartesian and grouped and non-grouped aggs) * Widening test cases for testing more combinations of aggregations and types, and fixing a few bugs found * Enhancing cartesian extent to convert from 6 ints to 4 ints at block loading time (for efficiency) * Fixing bugs in both cartesian and geo extents for generating intermediate state with missing groups (flaky tests in serverless) * Moved the int order to always match Rectangle for 4-int and Extent for 6-int cases (improved internal consistency) Since the PR already changed the meaning of the invalid/infinite values of the intermediate state integers, it was already not compatible with the previous cluster versions. We disabled mixed-cluster testing to prevent errors as a result of that. This leaves us the opportunity to make further changes that are mixed-cluster incompatible, hence the decision to perform this consistency update now. * Regenerate generated files

craigtaverner · 2025-02-12T17:23:04Z

Backports done manually:

8.x (8.19) in [8.x] Optimize ST_EXTENT_AGG for geo_shape and cartesian_shape (#119889) #122276
8.18.1 in [8.18] Optimize ST_EXTENT_AGG for geo_shape and cartesian_shape (#119889) #122420

…) (#122276) (#122420) * Optimize ST_EXTENT_AGG for geo_shape and cartesian_shape (#119889) Support for `ST_EXTENT_AGG` was added in #118829, and then partially optimized in #118829. This optimization worked only for cartesian_shape fields, and worked by extracting the Extent from the doc-values and re-encoding it as a WKB `BBOX` geometry. This does not work for geo_shape, where we need to retain all 6 integers stored in the doc-values, in order to perform the datelline choice only at reduce time during the final phase of the aggregation. Since both geo_shape and cartesian_shape perform the aggregations using integers, and the original Extent values in the doc-values are integers, this PR expands the previous optimization by: * Saving all Extent values into a multi-valued field in an IntBlock for both cartesian_shape and geo_shape * Simplifying the logic around merging intermediate states for all cases (geo/cartesian and grouped and non-grouped aggs) * Widening test cases for testing more combinations of aggregations and types, and fixing a few bugs found * Enhancing cartesian extent to convert from 6 ints to 4 ints at block loading time (for efficiency) * Fixing bugs in both cartesian and geo extents for generating intermediate state with missing groups (flaky tests in serverless) * Moved the int order to always match Rectangle for 4-int and Extent for 6-int cases (improved internal consistency) Since the PR already changed the meaning of the invalid/infinite values of the intermediate state integers, it was already not compatible with the previous cluster versions. We disabled mixed-cluster testing to prevent errors as a result of that. This leaves us the opportunity to make further changes that are mixed-cluster incompatible, hence the decision to perform this consistency update now. * Regenerate generated files

craigtaverner added 2 commits January 9, 2025 17:50

Support extracting extent from doc values as int[] for both cartesian…

ec4f4e7

… and geo shapes

Merge remote-tracking branch 'origin/main' into st_extent_optimize_ge…

daedc53

…oshape

Merge branch 'main' into st_extent_optimize_geoshape

7fdef3e

craigtaverner added 2 commits January 9, 2025 18:22

Update docs/changelog/119889.yaml

5c79f04

Fix changelog

f7f9403

craigtaverner requested a review from GalLalouche January 9, 2025 17:24

craigtaverner added 9 commits January 10, 2025 11:19

Added capability to block mixed-cluster tests which fail

898d1ba

Merge branch 'main' into st_extent_optimize_geoshape

2ade43c

Revert "Added capability to block mixed-cluster tests which fail"

a32e45c

This reverts commit 898d1ba.

Add back non-ENRICH versions to be sure we have multi-shape aggs

ad3f846

These versions cannot use COUNT() due to the issue with indexes that are also used in enrich policies being duplicated across multi-cluster IT environments.

Merge branch 'main' into st_extent_optimize_geoshape

6a7e517

Bring back capability because the mixed-cluster tests failed on the c…

49e912a

…hange in meaning Of some int values in the block, particularly those that mean unset or infinity.

Merge branch 'main' into st_extent_optimize_geoshape

f877c9e

GalLalouche requested changes Jan 12, 2025

View reviewed changes

craigtaverner added 5 commits January 13, 2025 11:04

Merge branch 'main' into st_extent_optimize_geoshape

a503549

Merge branch 'main' into st_extent_optimize_geoshape

5284203

Code review updates

9c45f72

Merge branch 'st_extent_optimize_geoshape' of github.com:craigtaverne…

acd58ea

…r/elasticsearch into st_extent_optimize_geoshape

Merge branch 'main' into st_extent_optimize_geoshape

44d0392

Merge branch 'main' into st_extent_optimize_geoshape

eaa5b21

GalLalouche approved these changes Jan 15, 2025

View reviewed changes

craigtaverner added 13 commits January 15, 2025 17:45

Merge branch 'main' into st_extent_optimize_geoshape

229aacb

Merge branch 'main' into st_extent_optimize_geoshape

7379c8f

Updated SpatialGeometryFieldMapperTests and identified bug

9fb92b1

We left the bug as a TODO for the next PR, so at least the primary optimization can get merged for tech-preview in 8.18.

Merge branch 'main' into st_extent_optimize_geoshape

86f350f

Merge branch 'st_extent_optimize_geoshape' of github.com:craigtaverne…

e13bf12

…r/elasticsearch into st_extent_optimize_geoshape

Merge branch 'main' into st_extent_optimize_geoshape

736285d

Support missing groupIds in intermediate state creation

3332224

Merge branch 'st_extent_optimize_geoshape' of github.com:craigtaverne…

394cc19

…r/elasticsearch into st_extent_optimize_geoshape

Merge branch 'main' into st_extent_optimize_geoshape

a66c67b

Added more cartesian tests with larger result sets

f0b7ab1

This is to increase the chance that the same failure seen for geo_shape in serverless would also occur with cartesian_shape in serverless.

Merge branch 'main' into st_extent_optimize_geoshape

1100bf3

Merge branch 'main' into st_extent_optimize_geoshape

77d639b

Merge branch 'main' into st_extent_optimize_geoshape

ffb590f

craigtaverner merged commit 40c34cd into elastic:main Jan 16, 2025
16 checks passed

elasticsearchmachine added the backport pending label Jan 16, 2025

idegtiarenko mentioned this pull request Feb 6, 2025

Refactor and simplify aggregation implementers #121818

Merged

ivancea mentioned this pull request Feb 10, 2025

ESQL: Remove AggregateMapper reflection, and delegate intermediate state to suppliers #122023

Merged

craigtaverner mentioned this pull request Feb 11, 2025

[8.x] Optimize ST_EXTENT_AGG for geo_shape and cartesian_shape (#119889) #122276

Merged

craigtaverner mentioned this pull request Feb 12, 2025

[8.18] Optimize ST_EXTENT_AGG for geo_shape and cartesian_shape (#119889) #122420

Merged

craigtaverner removed backport pending v8.18.0 labels Feb 12, 2025


		return aggregate.transformDown(UnaryExec.class, exec -> {
		private Set<Attribute> findSpatialShapeBoundsAttributes(AggregateExec aggregate, LocalPhysicalOptimizerContext ctx) {

Optimize ST_EXTENT_AGG for geo_shape and cartesian_shape #119889

Optimize ST_EXTENT_AGG for geo_shape and cartesian_shape #119889

Uh oh!

Conversation

craigtaverner commented Jan 9, 2025

Uh oh!

elasticsearchmachine commented Jan 9, 2025

Uh oh!

elasticsearchmachine commented Jan 9, 2025

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!

GalLalouche Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Uh oh!

elasticsearchmachine commented Jan 16, 2025

💔 Backport failed

Uh oh!

craigtaverner commented Feb 12, 2025

Uh oh!

Uh oh!

GalLalouche Jan 13, 2025 •

edited

Loading