ESQL: Prevent overflow on SUM using multiple aggregators #116170

ivancea · 2024-11-04T12:41:30Z

This PR does some things:

Implement warnExceptions for SumLongAggregator (This change can be partially seen at ESQL: Prevent overflow on SUM aggregation #111639)
Add cluster features to ESQL config, and use them when choosing aggregators
Add a second OverflowingSumLongAggregator aggregator to be used depending on the features

To do

Cluster features don't work with CCS, so a different solution will be needed

# Conflicts: # x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/AbstractFallibleArrayState.java

# Conflicts: # x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/SumLongGroupingAggregatorFunction.java

# Conflicts: # x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/SumLongAggregatorFunction.java # x-pack/plugin/esql/compute/src/test/java/org/elasticsearch/compute/data/BlockSerializationTests.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/TestCaseSupplier.java

# Conflicts: # x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/SumLongAggregatorFunction.java

elasticsearchmachine · 2024-11-04T12:41:54Z

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

ivancea · 2024-11-04T13:12:10Z

...e/gen/src/main/java/org/elasticsearch/compute/gen/AggregatorFunctionSupplierImplementer.java

-                    + "warningsLineNumber, warningsColumnNumber, warningsSourceText)"
+                "var warnings = $T.createWarnings(driverContext.warningsMode(), "
+                    + "warningsLineNumber, warningsColumnNumber, warningsSourceText)",
+                WARNINGS


This was a bug in the PR implementing warnExceptions on aggs, which wasn't being used anywhere yet

ivancea · 2024-11-04T13:14:39Z

...ompute/src/main/java/org/elasticsearch/compute/aggregation/OverflowingSumLongAggregator.java

We could also go with SumV1LongAggregator or something like that

ivancea · 2024-11-04T13:16:05Z

...lugin/esql/compute/src/test/java/org/elasticsearch/compute/data/BlockSerializationTests.java

        DriverContext driverCtx = driverContext();
        Page page = new Page(blockFactory.newLongArrayVector(new long[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 }, 10).asBlock());
-        var function = SumLongAggregatorFunction.create(driverCtx, List.of(0));
+        var function = MaxLongAggregatorFunction.create(driverCtx, List.of(0));


As the agg doesn't matter here, I changed it to another one. There are other similar fixes around tests

ivancea · 2024-11-04T13:19:13Z

...g/elasticsearch/xpack/esql/expression/function/aggregate/ConfigurationAggregateFunction.java

Following the same inheritance pattern as with Scalar functions

ivancea · 2024-11-04T13:20:16Z

...lugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Sum.java

+        DataType type = field().dataType();
+        if (type == DataType.LONG) {
+            // Old aggregator without overflow handling
+            if (configuration().clusterHasFeature(EsqlFeatures.FN_SUM_OVERFLOW_HANDLING) == false) {


This is the core of this PR: Choosing an aggregator based on a feature

ivancea · 2024-11-04T13:23:59Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/AggregateMapper.java

    }

    private static List<NamedExpression> computeEntryForAgg(Expression aggregate, boolean grouping) {
+        if (aggregate instanceof ToIntermediateState intermediateStateGetter) {


An interface like this looked interesting to make custom logics. It could be applied to other edge cases we have around AggregateMapper.

This doesn't remove the reflection-based logic we use here, it's just an easier-to-use override

ivancea · 2024-11-04T13:31:06Z

.../plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/TransportEsqlQueryAction.java

            request.tables(),
-            System.nanoTime()
+            System.nanoTime(),
+            Configuration.calculateActiveClusterFeatures(featureService, clusterService)


This is where we inject the features into the Configuration object of the query

ivancea · 2024-11-04T14:00:32Z

...g/elasticsearch/xpack/esql/expression/function/AbstractConfigurationAggregationTestCase.java

A clone of the AbstractConfigurationScalarFunctionTestCase

nik9000 · 2024-11-04T15:16:17Z

...ompute/src/main/java/org/elasticsearch/compute/aggregation/OverflowingSumLongAggregator.java

+ * </p>
+ * <p>
+ *     Can't be removed, as the new aggregator's layout is different.
+ * </p>


It can be removed once we no longer need wire compatibility with versions before this PR.

nik9000 · 2024-11-04T15:16:43Z

...lugin/esql/compute/src/test/java/org/elasticsearch/compute/data/BlockSerializationTests.java

        DriverContext driverCtx = driverContext();
        Page page = new Page(blockFactory.newLongArrayVector(new long[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 }, 10).asBlock());
-        var function = SumLongAggregatorFunction.create(driverCtx, List.of(0));
+        var function = MaxLongAggregatorFunction.create(driverCtx, List.of(0));


nik9000 · 2024-11-04T15:22:03Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats_sum.csv-spec

+required_capability: fn_sum_overflow_handling
+FROM employees
+| LIMIT 2
+| EVAL x = languages - languages + 9223372036854775807


Should we multiply languages by a big number? That feels less likely to get optimized away one day.

Uhm yeah, I'll remove that cheap trick

nik9000 · 2024-11-04T15:25:32Z

...lugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Sum.java

-    public Sum(Source source, Expression field, Expression filter) {
-        super(source, field, filter, emptyList());
+    public Sum(Source source, Expression field, Expression filter, Configuration configuration) {
+        super(source, field, filter, emptyList(), configuration);


I was thinking you'd read the value from the configuration in the ctor rather than keep the configuration around. Heck - in this public ctor you could just pass the boolean and read if from the configuration in the one above. That way you could call this one in tests if you wanted.

I was planning to do that, but as scalar functions already have this configuration stored, I thought it was better to keep the same mechanism (?)
Edit: Your next comment digs deeper in the solution, so maybe ignore this comment for now

nik9000 · 2024-11-04T15:35:01Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/Configuration.java

+            this.activeEsqlFeatures = in.readCollectionAsImmutableSet(StreamInput::readString);
+        } else {
+            this.activeEsqlFeatures = Set.of();
+        }


I wonder if we could avoid serializing this. Like, maybe it doesn't belong in the configuration - maybe it's another parameter that we add to FunctionDefinition.Builder or something.

I feel like we'd be better off reading from the Set<NodeFeatures> that are active in the ctor of the Function and setting a boolean. That feels like it'd be easier to debug and test and the functions could have that boolean in their descriptions so even in profile we could see what it decided to do. I feel like there aren't going to be that many of these functions with behavior changes so the extra booleans here and there are ok. And the "you can see what it chose" behavior is nicer than serializing all of these strings.

Also! I kind of like the idea of choosing at function resolution time more than copying the list of features, saving it, and choosing at function translation time. It amounts to the same thing but it feels better in my heart.

ivancea · 2025-04-07T13:30:31Z

Closing as we're not actively working on this, and there's a dependency on CCS features

ivancea and others added 30 commits August 5, 2024 15:09

Remove unused empty constructor

7a872cf

Added fallible single states

87db6ac

Added fallible array state

17e9ea2

Added custom warning object to aggregator function

318d5cd

Completed single aggregator implementation

4937082

Updated SumTests and format

7935330

Update docs/changelog/111639.yaml

35f7002

Fix tests compilation

0fb3040

Add warn exceptions in grouping aggregator

0d33140

Update docs/changelog/111639.yaml

523a583

Early returns on non-grouping, and todos for missing catch

1b09dd3

Add try-catch in intermediate state

b1fdc25

Release failed array, and fixed warnings on grouping tests

16b1cf8

Fixed benchmark compilation

d6d44c7

Merge branch 'main' into sum-warns

31cb9d1

Extend from AbstractArrayState, and extract annotations method

10ae7b2

Removed obsolete test

e95cc8d

Merge branch 'main' into sum-warns

55ac3b8

Merge branch 'main' into sum-warns

5530b3a

# Conflicts: # x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/AbstractFallibleArrayState.java

Merge branch 'main' into sum-warns

c76bb62

# Conflicts: # x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/SumLongGroupingAggregatorFunction.java

Test warnings for long overflow

753af10

Merge branch 'main' into sum-warns

7b2e1c7

Merge branch 'main' into sum-warns

678d183

Merge branch 'main' into sum-warns

086ee5a

# Conflicts: # x-pack/plugin/esql/compute/src/main/generated/org/elasticsearch/compute/aggregation/SumLongAggregatorFunction.java

Added CSV test failing in mixed cluster

d71eea2

Merge branch 'main' into sum-warns

5dd8762

Merge branch 'main' into sum-warns

c4a0aef

Merge branch 'main' into sum-warns

ce9d7c3

Merge branch 'main' into sum-warns-cluster-feature

66ab25f

ivancea added the v8.17.0 label Nov 4, 2024

ivancea and others added 2 commits November 4, 2024 13:41

Update docs/changelog/116170.yaml

0978eeb

Remove old branch changelog entry

423549d

ivancea commented Nov 4, 2024

View reviewed changes

Add "final" again to AggregateFunction#writeTo

fb98bec

ivancea commented Nov 4, 2024

View reviewed changes

ivancea added 2 commits November 4, 2024 14:52

Fixed serialization of functions with configuration

abcd246

Format

b9d66b4

ivancea commented Nov 4, 2024

View reviewed changes

ivancea requested review from nik9000 and not-napoleon November 4, 2024 14:01

nik9000 reviewed Nov 4, 2024

View reviewed changes

ivancea added 3 commits November 4, 2024 17:43

Fixed PhysicalOptimizerTests not using same config for everything

134a0d4

Format

20d9adf

Merge branch 'main' into sum-warns-cluster-feature

cde0ed7

elasticsearchmachine added v8.18.0 and removed v8.17.0 labels Nov 20, 2024

elasticsearchmachine added v8.19.0 v9.1.0 and removed v8.18.0 v9.0.0 labels Jan 30, 2025

ivancea closed this Apr 7, 2025

alex-spies mentioned this pull request Jul 11, 2025

ESQL: Make query planning/optimization aware of the version of nodes/clusters #131108

Closed

ESQL: Prevent overflow on SUM using multiple aggregators #116170

ESQL: Prevent overflow on SUM using multiple aggregators #116170

Uh oh!

Conversation

ivancea commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To do

Uh oh!

elasticsearchmachine commented Nov 4, 2024

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!

ivancea Nov 4, 2024 • 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!

ivancea commented Apr 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ivancea commented Nov 4, 2024 •

edited

Loading

ivancea Nov 4, 2024 •

edited

Loading