ESQL: Introduce per agg filter by costin · Pull Request #113735 · elastic/elasticsearch

costin · 2024-09-28T06:00:16Z

Add support for aggregation scoped filters that work dynamically on the
data in each group.

| STATS
    success = COUNT(*) WHERE 200 <= code AND code < 300,
   redirect = COUNT(*) WHERE 300 <= code AND code < 400,
 client_err = COUNT(*) WHERE 400 <= code AND code < 500,
 server_err = COUNT(*) WHERE 500 <= code AND code < 600,
total_count = COUNT(*)

Implementation wise, the base AggregateFunction has been extended to
allow a filter to be passed on. This is required to incorporate the
filter as part of the aggregate equality/identify which would fail with
the filter as an external component.

As part of the process, the serialization for the existing aggregations
had to be fixed so AggregateFunction implementations so that it
delegates to their parent first.

costin · 2024-09-28T06:01:37Z

x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4

+    : (qualifiedName ASSIGN)? booleanExpression
    ;

 fromCommand
    : FROM indexPattern (COMMA indexPattern)* metadata?
    ;

 indexPattern
-    : clusterString COLON indexString
-    | indexString
+    : (clusterString COLON)? indexString


Small improvements to the grammar that don't change the parsing.

costin · 2024-09-28T06:02:08Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java

+        // unwrap filtered expression
+        if (e instanceof FilteredExpression fe) {
+            e = fe.delegate();
+            // TODO add verification for filter clause
+        }


costin · 2024-09-28T06:03:15Z

.../main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/AggregateFunction.java

+        this(
+            Source.readFrom((PlanStreamInput) in),
+            in.readNamedWriteable(Expression.class),
+            in.getTransportVersion().onOrAfter(TransportVersions.ESQL_PER_AGGREGATE_FILTER)
+                ? in.readNamedWriteable(Expression.class)
+                : Literal.TRUE,
+            in.getTransportVersion().onOrAfter(TransportVersions.ESQL_PER_AGGREGATE_FILTER)
+                ? in.readNamedWriteableCollectionAsList(Expression.class)
+                : emptyList()
+        );


Uniform serialization across all aggregate functions - we should had that since the beginning to avoid having each subclass add its own repetitive serialization.
/cc @nik9000

I think it's easier to make the filter wrap the aggregation rather than inside each one. I think also that serializing a list in case aggs use it is probably not a great choice either. It's fine, but feels like something we'd do when we need it.

costin · 2024-09-28T06:04:11Z

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

+        return NodeInfo.create(this, Avg::new, field(), filter());
    }

    @Override
    public Avg replaceChildren(List<Expression> newChildren) {
-        return new Avg(source(), newChildren.get(0));
+        return new Avg(source(), newChildren.get(0), newChildren.get(1));
+    }
+
+    @Override
+    public Avg withFilter(Expression filter) {
+        return new Avg(source(), field(), filter);


The side effect is all aggs had to be modified to take into account the parent filter.

Could we wrap them with something like we do at runtime? It feels like that might work.

Not sure what you mean. My initial approach was to wrap the agg and filter in separate class (FilteredAggregate with two children - aggregate and filter/expression ) however this messed up the optimizer since the rules picked the aggregate functions but had no idea there was a filter associated with it.
This is problematic for scenario where the same agg is defined with different filter:
STATS c = count(). c_f = count() WHERE a > 1
since the where filter is disjoined from the count, the rules replaced the second count with the first one but that is incorrect since they have different execution paths.
Furthermore at compute time the filter and actual agg gets fused anyway so the two are a pair anyway.

They do get fused anyway, but it's just a lot more convenient if every agg doesn't have to worry about being filtered. If filter is a member here we have to think about it all over the place. It's enough that the fused filter at compute time has to add all the tests for it, I'd love to avoid that bubbling all the way out. OTOH, if that's how it has to be, I'm not deep enough to object.

I wonder if this means each aggregation evaluates its own filter? If so, from performance perspective, one of the advantages of separating filter from aggregation is that, if the same filter applies to different aggregations, it makes it easier to recognize that it is the same filter and can be evaluated once for all aggregations that share it, in stead of evaluating each filter within each aggregation separately. However I couldn’t figure out how the current infrastructure supports it yet. At first look at this, it reminds me of the (common)table expressions and union all subqueries in SQL, but we don’t have this infrastructure in ES|QL yet, and it could be a much broader scope.

This approach doesn't prevent optimizing the filter evaluation.
Each aggregation ends up having its own filter because count(*) is different than count(*) where a > 10. Initially they were decoupled however the rules considered them the same since the filter was sitting outside the aggregation.
When dealing with the same filter, we can assemble a different pipeline so the mask or masked block is reused across multiple aggregations.

costin · 2024-09-28T06:05:03Z

.../org/elasticsearch/xpack/esql/optimizer/rules/logical/ReplaceStatsAggExpressionWithEval.java

+        for (int i = 0, s = aggs.size(); i < s; i++) {
+            NamedExpression agg = aggs.get(i);
+


This needs to be rolled back.

...l/src/main/java/org/elasticsearch/xpack/esql/planner/AbstractPhysicalOperationProviders.java

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

costin

Draft PR to check the CI.
Was based on an old commit to avoid some of the volatility in main.
Will rebase and polish after a few CI rounds.

elasticsearchmachine · 2024-10-01T01:57:37Z

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

elasticsearchmachine · 2024-10-01T01:57:38Z

Pinging @elastic/kibana-esql (ES|QL-ui)

Add support for aggregation scoped filters that work dynamically on the data in each group. | STATS success = COUNT(*) WHERE 200 <= code AND code < 300, redirect = COUNT(*) WHERE 300 <= code AND code < 400, client_err = COUNT(*) WHERE 400 <= code AND code < 500, server_err = COUNT(*) WHERE 500 <= code AND code < 600, total_count = COUNT(*) Implementation wise, the base AggregateFunction has been extended to allow a filter to be passed on. This is required to incorporate the filter as part of the aggregate equality/identify which would fail with the filter as an external component. As part of the process, the serialization for the existing aggregations had to be fixed so AggregateFunction implementations so that it delegates to their parent first.

costin

Addressed TBDs, add verifier code thus promoted from draft and rebased on latest main.

elasticsearchmachine · 2024-10-01T02:36:48Z

Hi @costin, I've created a changelog YAML for you. Note that since this PR is labelled release highlight, you need to update the changelog YAML to fill out the extended information sections.

elasticsearchmachine · 2024-10-15T07:36:03Z

💔 Backport failed

The backport operation could not be completed due to the following error:

An unexpected error occurred when attempting to backport this PR.

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

Add support for aggregation scoped filters that work dynamically on the data in each group. | STATS success = COUNT(*) WHERE 200 <= code AND code < 300, redirect = COUNT(*) WHERE 300 <= code AND code < 400, client_err = COUNT(*) WHERE 400 <= code AND code < 500, server_err = COUNT(*) WHERE 500 <= code AND code < 600, total_count = COUNT(*) Implementation wise, the base AggregateFunction has been extended to allow a filter to be passed on. This is required to incorporate the filter as part of the aggregate equality/identify which would fail with the filter as an external component. As part of the process, the serialization for the existing aggregations had to be fixed so AggregateFunction implementations so that it delegates to their parent first.

Add support for aggregation scoped filters that work dynamically on the data in each group. | STATS success = COUNT(*) WHERE 200 <= code AND code < 300, redirect = COUNT(*) WHERE 300 <= code AND code < 400, client_err = COUNT(*) WHERE 400 <= code AND code < 500, server_err = COUNT(*) WHERE 500 <= code AND code < 600, total_count = COUNT(*) Implementation wise, the base AggregateFunction has been extended to allow a filter to be passed on. This is required to incorporate the filter as part of the aggregate equality/identify which would fail with the filter as an external component. As part of the process, the serialization for the existing aggregations had to be fixed so AggregateFunction implementations so that it delegates to their parent first. (cherry picked from commit d102659)

Remove dev flag left in grammar for agg filtering Related to elastic#113735

* ESQL: Introduce per agg filter (#113735) Add support for aggregation scoped filters that work dynamically on the data in each group. | STATS success = COUNT(*) WHERE 200 <= code AND code < 300, redirect = COUNT(*) WHERE 300 <= code AND code < 400, client_err = COUNT(*) WHERE 400 <= code AND code < 500, server_err = COUNT(*) WHERE 500 <= code AND code < 600, total_count = COUNT(*) Implementation wise, the base AggregateFunction has been extended to allow a filter to be passed on. This is required to incorporate the filter as part of the aggregate equality/identify which would fail with the filter as an external component. As part of the process, the serialization for the existing aggregations had to be fixed so AggregateFunction implementations so that it delegates to their parent first. (cherry picked from commit d102659) * Update docs/changelog/114842.yaml * Delete docs/changelog/114842.yaml

Remove dev flag left in grammar for agg filtering Related to #113735

Remove dev flag left in grammar for agg filtering Related to elastic#113735 (cherry picked from commit 5e59ab5)

Remove dev flag left in grammar for agg filtering Related to #113735 (cherry picked from commit 5e59ab5)

bpintea · 2024-10-24T12:09:32Z

From above:

we can prevent queries where a filter is specified through the verifier.

@costin, I guess this hasn't been implemented right? I've opened #115522, but do we want a fix before addressing that?

Add support for aggregation scoped filters that work dynamically on the data in each group. | STATS success = COUNT(*) WHERE 200 <= code AND code < 300, redirect = COUNT(*) WHERE 300 <= code AND code < 400, client_err = COUNT(*) WHERE 400 <= code AND code < 500, server_err = COUNT(*) WHERE 500 <= code AND code < 600, total_count = COUNT(*) Implementation wise, the base AggregateFunction has been extended to allow a filter to be passed on. This is required to incorporate the filter as part of the aggregate equality/identify which would fail with the filter as an external component. As part of the process, the serialization for the existing aggregations had to be fixed so AggregateFunction implementations so that it delegates to their parent first.

Remove dev flag left in grammar for agg filtering Related to elastic#113735

When using a WHERE clause within the STATS of a spatial aggregation, and there are more than one aggregation function, the results are incorrect, and reflect the values of the first aggregating function. With some investigation by Cursor it turns out this is a side effect of the fact that the replaceChildren function was not capturing the filter (and not the window or field-extract preference either). This was probably due to the first implementation of the filter in #113735 back in October 2024, which appears to have not updated this method in the SpatialCentroid class, although it did update that method in other aggregations. Then SpatialExtents, inspired by SpatialCentroid code, inherited this flaw. Since it is rare to perform queries like this, and our test suite did not include any, we did not notice this until working on a more advanced feature, extracting a combination of both centroid and bounds from shape doc-values, which requires both centroid and shape aggregations in the same query.

When using a WHERE clause within the STATS of a spatial aggregation, and there are more than one aggregation function, the results are incorrect, and reflect the values of the first aggregating function. With some investigation by Cursor it turns out this is a side effect of the fact that the replaceChildren function was not capturing the filter (and not the window or field-extract preference either). This was probably due to the first implementation of the filter in elastic#113735 back in October 2024, which appears to have not updated this method in the SpatialCentroid class, although it did update that method in other aggregations. Then SpatialExtents, inspired by SpatialCentroid code, inherited this flaw. Since it is rare to perform queries like this, and our test suite did not include any, we did not notice this until working on a more advanced feature, extracting a combination of both centroid and bounds from shape doc-values, which requires both centroid and shape aggregations in the same query.

) When using a WHERE clause within the STATS of a spatial aggregation, and there are more than one aggregation function, the results are incorrect, and reflect the values of the first aggregating function. With some investigation by Cursor it turns out this is a side effect of the fact that the replaceChildren function was not capturing the filter (and not the window or field-extract preference either). This was probably due to the first implementation of the filter in #113735 back in October 2024, which appears to have not updated this method in the SpatialCentroid class, although it did update that method in other aggregations. Then SpatialExtents, inspired by SpatialCentroid code, inherited this flaw. Since it is rare to perform queries like this, and our test suite did not include any, we did not notice this until working on a more advanced feature, extracting a combination of both centroid and bounds from shape doc-values, which requires both centroid and shape aggregations in the same query.

elastic#142385) When using a WHERE clause within the STATS of a spatial aggregation, and there are more than one aggregation function, the results are incorrect, and reflect the values of the first aggregating function. With some investigation by Cursor it turns out this is a side effect of the fact that the replaceChildren function was not capturing the filter (and not the window or field-extract preference either). This was probably due to the first implementation of the filter in elastic#113735 back in October 2024, which appears to have not updated this method in the SpatialCentroid class, although it did update that method in other aggregations. Then SpatialExtents, inspired by SpatialCentroid code, inherited this flaw. Since it is rare to perform queries like this, and our test suite did not include any, we did not notice this until working on a more advanced feature, extracting a combination of both centroid and bounds from shape doc-values, which requires both centroid and shape aggregations in the same query.

) (#142487) When using a WHERE clause within the STATS of a spatial aggregation, and there are more than one aggregation function, the results are incorrect, and reflect the values of the first aggregating function. With some investigation by Cursor it turns out this is a side effect of the fact that the replaceChildren function was not capturing the filter (and not the window or field-extract preference either). This was probably due to the first implementation of the filter in #113735 back in October 2024, which appears to have not updated this method in the SpatialCentroid class, although it did update that method in other aggregations. Then SpatialExtents, inspired by SpatialCentroid code, inherited this flaw. Since it is rare to perform queries like this, and our test suite did not include any, we did not notice this until working on a more advanced feature, extracting a combination of both centroid and bounds from shape doc-values, which requires both centroid and shape aggregations in the same query.

elastic#142385) (elastic#142487) When using a WHERE clause within the STATS of a spatial aggregation, and there are more than one aggregation function, the results are incorrect, and reflect the values of the first aggregating function. With some investigation by Cursor it turns out this is a side effect of the fact that the replaceChildren function was not capturing the filter (and not the window or field-extract preference either). This was probably due to the first implementation of the filter in elastic#113735 back in October 2024, which appears to have not updated this method in the SpatialCentroid class, although it did update that method in other aggregations. Then SpatialExtents, inspired by SpatialCentroid code, inherited this flaw. Since it is rare to perform queries like this, and our test suite did not include any, we did not notice this until working on a more advanced feature, extracting a combination of both centroid and bounds from shape doc-values, which requires both centroid and shape aggregations in the same query.

) (#142487) (#142533) When using a WHERE clause within the STATS of a spatial aggregation, and there are more than one aggregation function, the results are incorrect, and reflect the values of the first aggregating function. With some investigation by Cursor it turns out this is a side effect of the fact that the replaceChildren function was not capturing the filter (and not the window or field-extract preference either). This was probably due to the first implementation of the filter in #113735 back in October 2024, which appears to have not updated this method in the SpatialCentroid class, although it did update that method in other aggregations. Then SpatialExtents, inspired by SpatialCentroid code, inherited this flaw. Since it is rare to perform queries like this, and our test suite did not include any, we did not notice this until working on a more advanced feature, extracting a combination of both centroid and bounds from shape doc-values, which requires both centroid and shape aggregations in the same query.

costin added :Analytics/ES|QL AKA ESQL ES|QL-ui Impacts ES|QL UI labels Sep 28, 2024

costin requested review from alex-spies, astefan, bpintea, fang-xing-esql and nik9000 September 28, 2024 06:00

elasticsearchmachine added the v9.0.0 label Sep 28, 2024

costin commented Sep 28, 2024

View reviewed changes

...l/src/main/java/org/elasticsearch/xpack/esql/planner/AbstractPhysicalOperationProviders.java Show resolved Hide resolved

costin commented Sep 28, 2024

View reviewed changes

x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec Show resolved Hide resolved

costin commented Sep 28, 2024

View reviewed changes

costin force-pushed the esql/per-agg-filtering branch from 60af98d to d2db8db Compare October 1, 2024 01:36

costin added v8.16.0 release highlight labels Oct 1, 2024

costin force-pushed the esql/per-agg-filtering branch from d2db8db to 8a201ec Compare October 1, 2024 01:55

costin marked this pull request as ready for review October 1, 2024 01:57

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 1, 2024

costin force-pushed the esql/per-agg-filtering branch from 8a201ec to 839dbbf Compare October 1, 2024 02:31

costin commented Oct 1, 2024

View reviewed changes

costin added the >feature label Oct 1, 2024

Update docs/changelog/113735.yaml

586dabe

elasticsearchmachine added the backport pending label Oct 15, 2024

This was referenced Oct 15, 2024

ESQL: Introduce per agg filter (#113735) #114842

Merged

ESQL: deprecated private constructor affect EsqlNodeSubclass tests #114845

Closed

costin added a commit to costin/elasticsearch that referenced this pull request Oct 15, 2024

ESQL: Fix grammar changes around per agg filtering

b1367aa

Remove dev flag left in grammar for agg filtering Related to elastic#113735

costin mentioned this pull request Oct 15, 2024

ESQL: Fix grammar changes around per agg filtering #114848

Merged

costin added a commit that referenced this pull request Oct 15, 2024

ESQL: Fix grammar changes around per agg filtering (#114848)

5e59ab5

Remove dev flag left in grammar for agg filtering Related to #113735

costin added a commit to costin/elasticsearch that referenced this pull request Oct 15, 2024

ESQL: Fix grammar changes around per agg filtering (elastic#114848)

516d424

Remove dev flag left in grammar for agg filtering Related to elastic#113735 (cherry picked from commit 5e59ab5)

costin mentioned this pull request Oct 15, 2024

ESQL: Fix grammar changes around per agg filtering (#114848) #114853

Merged

costin removed the backport pending label Oct 15, 2024

elasticsearchmachine pushed a commit that referenced this pull request Oct 15, 2024

ESQL: Fix grammar changes around per agg filtering (#114848) (#114853)

8c475a2

Remove dev flag left in grammar for agg filtering Related to #113735 (cherry picked from commit 5e59ab5)

This was referenced Oct 18, 2024

ESQL: Docs: per agg filtering #115083

Closed

ESQL: prevent usage of BUCKET in agg filters #115521

Closed

ESQL: unfolded counting fails with filtered agg #115522

Closed

georgewallace pushed a commit to georgewallace/elasticsearch that referenced this pull request Oct 25, 2024

ESQL: Fix grammar changes around per agg filtering (elastic#114848)

eb934e9

Remove dev flag left in grammar for agg filtering Related to elastic#113735

alex-spies mentioned this pull request Mar 13, 2025

ESQL: INLINESTATS with per-agg WHERE #124741

Closed

craigtaverner mentioned this pull request Feb 11, 2026

Fix bug with multiple spatial aggs filtering in ES|QL #142332

Merged

craigtaverner mentioned this pull request Feb 13, 2026

[9.2] Fix bug with multiple spatial aggs filtering in ES|QL (#142332) #142487

Merged

		for (int i = 0, s = aggs.size(); i < s; i++) {
		NamedExpression agg = aggs.get(i);

Conversation

costin commented Sep 28, 2024 • edited by nik9000 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

fang-xing-esql Oct 3, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Oct 1, 2024

Uh oh!

elasticsearchmachine commented Oct 1, 2024

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Oct 1, 2024

Uh oh!

elasticsearchmachine commented Oct 15, 2024

💔 Backport failed

Uh oh!

bpintea commented Oct 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

costin commented Sep 28, 2024 •

edited by nik9000

Loading

fang-xing-esql Oct 3, 2024 •

edited

Loading