ESQL: Fix count optimization with pushable union types #127225

alex-spies · 2025-04-23T10:12:30Z

Fix #127200

elasticsearchmachine · 2025-04-23T10:13:27Z

Hi @alex-spies, I've created a changelog YAML for you.

elasticsearchmachine · 2025-04-25T16:49:29Z

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

…able-union-types

Ensure that the code path with pushdown is actually covered.

craigtaverner

Interesting! I did not know the bug was about Lucene pushdown of STATS.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

craigtaverner · 2025-04-28T10:26:26Z

.../org/elasticsearch/xpack/esql/optimizer/rules/logical/local/ReplaceMissingFieldWithNull.java

            // This means that an EsRelation[field1, field2, field3] where field1 and field 3 are missing will be replaced by
            // Project[field1, field2, field3] <- keeps the ordering intact
            // \_Eval[field1 = null, field3 = null]
-            // \_EsRelation[field2]
+            // \_EsRelation[field1, field2, field3]


If the field is missing, would it be in the EsRelation at all?

Yes! Because we're in the local optimizer, the field does exist overall, and thus is put into the EsRelation after the field caps call on the coordinator. But on the local node it's missing! (Or in the search stats that this optimization run uses, to be more precise) This optimizer rule applies exclusively to such fields.

craigtaverner · 2025-04-28T10:27:51Z

...main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushStatsToSource.java

@@ -94,15 +94,19 @@ private Tuple<List<Attribute>, List<EsStatsQueryExec.Stat>> pushableStats(
                            // check if regular field
                            else {
                                if (target instanceof FieldAttribute fa) {
-                                    var fName = fa.name();
+                                    var fName = fa.fieldName();


Wow! Is this the fix? I imagine this could have impacts in many places, so could this fix other bugs we've not noticed?

Yep, this is the fix. Simple oversight to use the correct field name - in the past, the field attribute name and the field name were the same, but union types had to break with this pattern.

I do not see other situations that this may fix, too, because the specific stats-pushdown optimization only triggers in a narrow slice of queries, anyway.

What I'm wondering is if we could make this mistake again and if we could prevent it.
Also, should we insert filters here using #fieldName()?

elasticsearch/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/local/InferNonNullAggConstraint.java

Line 58 in 9e0a5af

if (field.foldable() == false && field instanceof FieldAttribute fa && stats.isIndexed(fa.name())) {

craigtaverner · 2025-04-28T10:31:45Z

...main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushStatsToSource.java

                                        query = QueryBuilders.existsQuery(fieldName);
                                    }
                                }
                            }
                            if (fieldName != null) {
                                if (count.hasFilter()) {
+                                    // Note: currently, it seems like we never actually perform stats pushdown if we reach here.


It seems the question of what exactly gets pushed down and why is subtle. I wonder if we want some documentation on this?

When we touch this rule again, I think we should add unit tests that demonstrate exactly what is pushed down and how.

I also found that union type filters don't seem to be pushed to Lucene, yet - maybe we could improve the documentation as part of that work?

If you agree, I can open up an issue for this.

// Note: currently, it seems like we never actually perform stats pushdown if we reach here.

We evolved to this. Before the aggs filter were extracted into an upstream filter and pushed down, this code was "alive".

Yep yep, and if we manage to evolve the optimization to multiple stats, this code could come alive again.

There's an argument to be made that it should be deleted as long as it's dead - but properly double checking that this code path really is never ever used atm was beyond the scope that I could allocate to this bug fix.

So leaving a comment was the next best thing for the time being, I think :)

…able-union-types

When pushing down STATS count(field::type) to Lucene for a union-typed field, use the correct field name in the Lucene query and not the synthetic attribute name $$field$converted_to$type.

elasticsearchmachine · 2025-04-28T11:52:13Z

💚 Backport successful

Status	Branch	Result
✅	8.18
✅	8.19
✅	9.0
✅	8.17

When pushing down STATS count(field::type) to Lucene for a union-typed field, use the correct field name in the Lucene query and not the synthetic attribute name $$field$converted_to$type.

bpintea · 2025-04-28T10:55:06Z

.../org/elasticsearch/xpack/esql/optimizer/rules/logical/local/ReplaceMissingFieldWithNull.java

+            // For any missing field, place an Eval right after the EsRelation to assign null values to that attribute (using the same name
+            // id!), thus avoiding that InsertFieldExtrations inserts a field extraction later.


bpintea · 2025-04-28T11:14:03Z

...main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushStatsToSource.java

                                        query = QueryBuilders.existsQuery(fieldName);
                                    }
                                }
                            }
                            if (fieldName != null) {
                                if (count.hasFilter()) {
+                                    // Note: currently, it seems like we never actually perform stats pushdown if we reach here.


// Note: currently, it seems like we never actually perform stats pushdown if we reach here.

We evolved to this. Before the aggs filter were extracted into an upstream filter and pushed down, this code was "alive".

bpintea · 2025-04-28T11:53:19Z

...main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/PushStatsToSource.java

@@ -94,15 +94,19 @@ private Tuple<List<Attribute>, List<EsStatsQueryExec.Stat>> pushableStats(
                            // check if regular field
                            else {
                                if (target instanceof FieldAttribute fa) {
-                                    var fName = fa.name();
+                                    var fName = fa.fieldName();


What I'm wondering is if we could make this mistake again and if we could prevent it.
Also, should we insert filters here using #fieldName()?

elasticsearch/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/local/InferNonNullAggConstraint.java

Line 58 in 9e0a5af

if (field.foldable() == false && field instanceof FieldAttribute fa && stats.isIndexed(fa.name())) {

When pushing down STATS count(field::type) to Lucene for a union-typed field, use the correct field name in the Lucene query and not the synthetic attribute name $$field$converted_to$type.

…7460) When pushing down STATS count(field::type) to Lucene for a union-typed field, use the correct field name in the Lucene query and not the synthetic attribute name $$field$converted_to$type.

…7462) When pushing down STATS count(field::type) to Lucene for a union-typed field, use the correct field name in the Lucene query and not the synthetic attribute name $$field$converted_to$type.

bpintea · 2025-04-28T13:03:51Z

Oh, too late.

Also, should we insert filters here using #fieldName()?

I will follow up on this.

…7461) When pushing down STATS count(field::type) to Lucene for a union-typed field, use the correct field name in the Lucene query and not the synthetic attribute name $$field$converted_to$type.

…7459) When pushing down STATS count(field::type) to Lucene for a union-typed field, use the correct field name in the Lucene query and not the synthetic attribute name $$field$converted_to$type.

alex-spies · 2025-04-29T08:58:51Z

@bpintea , your comment #127225 (comment) is an excellent find. Dang, we missed another usage of FieldName#name where we really meant #fieldName.

Thank you for following up on this! Maybe let's also add a javadoc comment to NamedExpression#name, or more specifically FieldAttribute#name to warn against using the attribute name (which can be synthetic).

To remove the sharp edge, we could consider refactoring methods using the actual field name to use an EsField object (each FieldAttribute has one!) rather than a String to ensure, at compile time, that we never hand them a bogus name accidentally. More specifically, SearchStats' methods should maybe not use String everywhere, but maybe EsField or a dedicated String wrapper record that we can call FielName or so, just to leverage the compiler.

alex-spies · 2025-04-29T17:06:59Z

@bpintea , your comment #127225 (comment) is an excellent find. Dang, we missed another usage of FieldName#name where we really meant #fieldName.

Thank you for following up on this! Maybe let's also add a javadoc comment to NamedExpression#name, or more specifically FieldAttribute#name to warn against using the attribute name (which can be synthetic).

To remove the sharp edge, we could consider refactoring methods using the actual field name to use an EsField object (each FieldAttribute has one!) rather than a String to ensure, at compile time, that we never hand them a bogus name accidentally. More specifically, SearchStats' methods should maybe not use String everywhere, but maybe EsField or a dedicated String wrapper record that we can call FielName or so, just to leverage the compiler.

I think it's worth tracking this in an issue. Opened #127521

elasticsearchmachine added the v9.1.0 label Apr 23, 2025

alex-spies added >bug :Analytics/ES|QL AKA ESQL v9.0.0 v8.18.1 v8.19.0 v9.0.1 and removed v9.0.0 labels Apr 23, 2025

alex-spies added 2 commits April 25, 2025 18:25

Fix count optimization with pushable union types

7e46eae

Update docs/changelog/127225.yaml

c90cacf

alex-spies force-pushed the fix-count-with-pushable-union-types branch from 8e4867a to c90cacf Compare April 25, 2025 16:26

alex-spies added 2 commits April 25, 2025 18:42

Add capability and tests

3cc50a8

Improve comments

50c8a2a

alex-spies added the auto-backport Automatically create backport pull requests when merged label Apr 25, 2025

alex-spies marked this pull request as ready for review April 25, 2025 16:49

alex-spies requested a review from craigtaverner April 25, 2025 16:49

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 25, 2025

idegtiarenko approved these changes Apr 28, 2025

View reviewed changes

alex-spies added the v8.17.6 label Apr 28, 2025

alex-spies added 2 commits April 28, 2025 09:39

Merge remote-tracking branch 'upstream/main' into fix-count-with-push…

becc26c

…able-union-types

Add more tests

e8251f6

Ensure that the code path with pushdown is actually covered.

craigtaverner approved these changes Apr 28, 2025

View reviewed changes

alex-spies added 2 commits April 28, 2025 12:40

Move capability to better location

a7e7faa

Merge remote-tracking branch 'upstream/main' into fix-count-with-push…

a08b148

…able-union-types

alex-spies merged commit 9e0a5af into elastic:main Apr 28, 2025
17 checks passed

alex-spies deleted the fix-count-with-pushable-union-types branch April 28, 2025 11:50

This was referenced Apr 28, 2025

[8.18] ESQL: Fix count optimization with pushable union types (#127225) #127459

Merged

[8.19] ESQL: Fix count optimization with pushable union types (#127225) #127460

Merged

This was referenced Apr 28, 2025

[9.0] ESQL: Fix count optimization with pushable union types (#127225) #127461

Merged

[8.17] ESQL: Fix count optimization with pushable union types (#127225) #127462

Merged

bpintea reviewed Apr 28, 2025

View reviewed changes

alex-spies mentioned this pull request Apr 29, 2025

ESQL: InferNonNullAggConstraint using wrong field name for union types #127521

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Fix count optimization with pushable union types #127225

ESQL: Fix count optimization with pushable union types #127225

alex-spies commented Apr 23, 2025

elasticsearchmachine commented Apr 23, 2025

elasticsearchmachine commented Apr 25, 2025

craigtaverner left a comment

craigtaverner Apr 28, 2025

alex-spies Apr 28, 2025

craigtaverner Apr 28, 2025

alex-spies Apr 28, 2025

bpintea Apr 28, 2025

craigtaverner Apr 28, 2025

alex-spies Apr 28, 2025

bpintea Apr 28, 2025

alex-spies Apr 29, 2025

elasticsearchmachine commented Apr 28, 2025

bpintea Apr 28, 2025

bpintea Apr 28, 2025

bpintea Apr 28, 2025

bpintea commented Apr 28, 2025

alex-spies commented Apr 29, 2025

alex-spies commented Apr 29, 2025

		// For any missing field, place an Eval right after the EsRelation to assign null values to that attribute (using the same name
		// id!), thus avoiding that InsertFieldExtrations inserts a field extraction later.

ESQL: Fix count optimization with pushable union types #127225

ESQL: Fix count optimization with pushable union types #127225

Conversation

alex-spies commented Apr 23, 2025

elasticsearchmachine commented Apr 23, 2025

elasticsearchmachine commented Apr 25, 2025

craigtaverner 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

elasticsearchmachine commented Apr 28, 2025

💚 Backport successful

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea commented Apr 28, 2025

alex-spies commented Apr 29, 2025

alex-spies commented Apr 29, 2025