ESQL: Prevent circular alias references in `DeduplicateAggs` by kanoshiou · Pull Request #139175 · elastic/elasticsearch

kanoshiou · 2025-12-07T16:40:39Z

Summary

Fixes circular reference detection in alias chains during DeduplicateAggs optimizations.

Problem

The PushDownEnrich optimization can create circular references when pushing Enrich past Project operations, causing ql_illegal_argument_exception: Potential cycle detected errors when DeduplicateAggs.

Example query:

FROM languages_lookup 
| RENAME language_name AS message  
| ENRICH languages_policy ON language_code 
| STATS a = count(to_lower(language_name)), b = count(language_name)

Changes

Skip aliases that would create circular references by checking if alias.toAttribute() equals the resolved alias.child().

Closes #138346
Closes #139541

elasticsearchmachine · 2025-12-10T13:42:40Z

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

bpintea

Thanks @kanoshiou for the fix!

Didn't look too deep at it yet, but we'd need unit tests too. This should also make it easier to understand how the defect occurs and how the fix patches it. 🙏

bpintea · 2025-12-10T14:10:28Z

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

+;
+
+
+circularRefComplexEnrich


Could this test be "minimised" i.e. reduced to a minimal reproducer? This is not a must, however, if the other tests do actually that; but it would be, if there are multiple "minimum" scenarios that trigger the issue.

Also, could we make it "human" friendly. Those random identifiers are a pain to track.

Finally, would it be OK to capitalise the language keywords, to stay consistent? I'm not sure if that's always the case with these csv-spec tests, but it makes it again easier to parse the query when reviewing.

In another PR, I was asked to include the original generated query in the CSV tests, so I have consistently followed that requirement. I assumed it was mandatory when addressing generative CSV test failures. I was also asked to include the original issue in the comment above the new capability.

bpintea · 2025-12-10T14:11:30Z

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

+        /**
+         * Fix for circular reference in alias chains during PushDownEnrich and aggregate deduplication.
+         * Prevents "Potential cycle detected" errors when aliases reference each other.
+         * https://github.com/elastic/elasticsearch/issues/XXXXX


Did you intend to add the PR number here? Or really an issue?
In any case, the GH link is optional, git will contain the history (but up to you).

Sorry, this was AI generated and I didn’t review it carefully. This situation will not happen again.

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

kanoshiou · 2025-12-10T15:44:56Z

Thank you for your review, @bpintea! Could you please advise on the types of tests that should be added? Would LogicalPlan optimizer tests be suitable, or is there any existing reference or guideline I should consult?

bpintea · 2025-12-11T10:36:27Z

Ah, sorry, that wasn't clear.

Would LogicalPlan optimizer tests be suitable

Yes, the patch updates a logical plan optimiser rule, so some tests in the LogicalPlanOptimizerTests suite should do.

ncordon · 2025-12-12T09:21:37Z

buildkite test this please

ncordon · 2025-12-12T11:11:52Z

...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

+        assertThat(agg2.name(), is("b"));
+        var count2 = as(agg2.child(), Count.class);
+        var field2 = as(count2.field(), ReferenceAttribute.class);
+        assertThat(field2.name(), is("$$language_name$temp_name$23"));


This check will fail. If you execute the full suite you see a different id than if you only execute the particular test.

It's best to just check the name starts with "$$language_name$temp_name$"

Thanks for your review! I have updated this test.

ncordon

The PR looks good from my viewpoint, thanks a lot for addressing this @kanoshiou! The only think I would maybe tweak would be what we are testing exactly.

@bpintea opinions?

ncordon · 2025-12-12T11:46:58Z

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

+FROM firewall_logs*,languages_lookup 
+| RENAME language_name AS message 
+| LOOKUP JOIN message_types_lookup ON message 
+| DISSECT message "%{a}"


Why do we need tests with DISECT?

I think we should just have one test with RENAME after the ENRICH and with RENAME before the ENRICH instead

This actually comes from #138346 (comment), and I didn't look into it in depth. I can remove this test if you’d like, since I believe the query below is the minimal reproducer.

FROM languages_lookup | RENAME language_name AS message | ENRICH languages_policy ON language_code | STATS a = count(to_lower(language_name)), b = count(language_name)

I'm inclined to leave only that query if you don't mind 😄

I will remove circularRefComplexEnrich and circularRefEnrichDissect once the CI finishes 😄

Actually I think it might be good to test all possible combinations of aliasing ENRICH can introduce:

FROM languages_lookup | RENAME language_name AS message | ENRICH languages_policy ON language_code | STATS a = count(to_lower(language_name)), b = count(message)

FROM languages_lookup | ENRICH languages_policy ON language_code | RENAME language_name AS message | STATS a = count(to_lower(message)), b = count(message)

FROM languages_lookup | ENRICH languages_policy ON language_code WITH message=language_name | RENAME language_name AS message | STATS a = count(to_lower(message)), b = count(message)

FROM languages_lookup | ENRICH languages_policy ON language_code WITH message=language_name | STATS a = count(to_lower(language_name)), b = count(message)

Do you mind adding those to the pr? After that I'm happy to approve and merge

I'm glad to add these tests. Thanks for providing them! 😊

ncordon · 2025-12-12T11:48:41Z

buildkite test this please

# Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

kanoshiou · 2025-12-16T01:49:44Z

Hi @alex-spies @bpintea,

I have updated the branch. Regarding the issue, I see two possible solutions here:

Update the nameId of ReferenceAttribute when modifying the generated ENRICH names. This is a safe change that fundamentally resolves potential circular references in the future. The only downside is that the new nameIds are often semantically meaningless.

--- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Enrich.java
+++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Enrich.java
@@ -234,7 +234,7 @@ public class Enrich extends UnaryPlan
             if (enrichField.name().equals(newName)) {
                 newEnrichFields.add(enrichField);
             } else if (enrichField instanceof ReferenceAttribute ra) {
-                newEnrichFields.add(new Alias(ra.source(), newName, ra, new NameId(), ra.synthetic()));
+                newEnrichFields.add(new Alias(ra.source(), newName, ra.withId(new NameId()), new NameId(), ra.synthetic()));
             } else if (enrichField instanceof Alias a) {
                 newEnrichFields.add(new Alias(a.source(), newName, a.child(), new NameId(), a.synthetic()));
             } else {

Stick with the current patch. This limits the scope to PushDownEnrich. However, we cannot guarantee that similar bugs won't arise in other optimizer rules.

Which one do you think is better? I'd like to know your preference.

alex-spies

Update the nameId of ReferenceAttribute when modifying the generated ENRICH names. This is a safe change that fundamentally resolves potential circular references in the future. The only downside is that the new nameIds are often semantically meaningless.

I think this is the best change we can make.

It's a little unfortunate that Enrich doesn't just always have aliases for its output fields. ... | EVAL 2*x will still generate an alias behind the scenes, whereas ... | ENRICH policy ON field WITH field, foo = otherfield will only create an alias for otherfield.

However, Enrich's internal fields that say which physical fields we should fetch from the enrich policy don't matter to the plan - downstream nodes can only reference the output fields. Arguably, the enrich policy fields could be represented without the use of attributes or aliases altogether.

That is to say, the name ids of the original attributes are always semantically meaningless IMHO - they're only needed in the case without aliases. But ENRICH WITH field is equivalent to ENRICH WITH field = field; and turning an un-aliased enrich field into an aliased one with a new name id for the physical enrich policy field - that's essentially the same as re-writing ENRICH WITH field into ENRICH WITH field = field (+ renaming the left hand side of the alias to deal with the name conflict).

alex-spies · 2025-12-16T11:25:20Z

...n/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownUtils.java

+            UnaryPlan finalGeneratingPlan = generatingPlanWithRenamedAttributes;
+            if (finalGeneratingPlan instanceof Enrich enrich && renameGeneratedAttributeTo.isEmpty() == false) {
+                finalGeneratingPlan = (UnaryPlan) enrich.transformExpressionsOnly(Alias.class, alias -> {
+                    if (alias.child() instanceof ReferenceAttribute ra && renameGeneratedAttributeTo.containsKey(ra.name())) {
+                        return alias.replaceChild(ra.withId(new NameId()));
+                    }
+                    return alias;
+                });
+            }


I think we could contain the change more inside Enrich.java, by assigning a new name id when calling withGeneratedNames here.

If that still passes tests, we should probably also add a comment saying "it's safe to generate new name ids when assigning new names - the original attribute representing the field in the enrich index cannot be directly referenced in downstream plans, anyway".

kanoshiou · 2025-12-16T12:06:31Z

Thank you for the review, @alex-spies!

I’ve updated the branch, and it should be ready for testing now.

bpintea · 2025-12-16T12:58:17Z

buildkite test this please

bpintea

Thanks for this contribution, @kanoshiou!

bpintea · 2025-12-16T14:08:35Z

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

It'd have been great to have one test with ENRICH + SORT (besides RENAME), but this can come in a next PR (you may add one yourself, @kanoshiou, or I'll make a mental note to do it myself in some other PR).

Thanks for your review @bpintea! I will add this test in my next PR ✍️

elasticsearchmachine · 2025-12-16T14:30:35Z

💔 Backport failed

Status	Branch	Result
❌	9.1	Commit could not be cherrypicked due to conflicts
❌	9.2	Commit could not be cherrypicked due to conflicts

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

kanoshiou · 2025-12-16T15:03:27Z

Hi @bpintea,

If you don’t have the time, I’m happy to take care of the manual backport myself.

…#139175) Fixes circular reference detection in alias chains during `DeduplicateAggs` optimizations. The `PushDownEnrich` optimization can create circular references when pushing `Enrich` past `Project` operations, causing `ql_illegal_argument_exception: Potential cycle detected` errors when `DeduplicateAggs`. Example query: ```esql FROM languages_lookup | RENAME language_name AS message | ENRICH languages_policy ON language_code | STATS a = count(to_lower(language_name)), b = count(language_name) ``` Skip aliases that would create circular references by checking if `alias.toAttribute()` equals the resolved `alias.child()`. Closes elastic#138346 Closes elastic#139541 (cherry picked from commit 2289e17)

bpintea · 2025-12-16T15:11:25Z

Hi @bpintea,
If you don’t have the time, I’m happy to take care of the manual backport myself.

Oh, that'd be great, thank you! I've meanwhile opened #139614 for 9.2, feel free to go on with the 9.1 backport please.

…#139175) ## Summary Fixes circular reference detection in alias chains during `DeduplicateAggs` optimizations. ## Problem The `PushDownEnrich` optimization can create circular references when pushing `Enrich` past `Project` operations, causing `ql_illegal_argument_exception: Potential cycle detected` errors when `DeduplicateAggs`. Example query: ```esql FROM languages_lookup | RENAME language_name AS message | ENRICH languages_policy ON language_code | STATS a = count(to_lower(language_name)), b = count(language_name) ``` ## Changes Skip aliases that would create circular references by checking if `alias.toAttribute()` equals the resolved `alias.child()`. Closes elastic#138346 Closes elastic#139541 (cherry picked from commit 2289e17) # Conflicts: # x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

bpintea · 2025-12-16T15:12:35Z

I've meanwhile opened #139614 for 9.2

I'll close it, I see you've opened yourself #139615

kanoshiou · 2025-12-16T15:31:27Z

💚 All backports created successfully

Status	Branch	Result
✅	9.1

Questions ?

Please refer to the Backport tool documentation

…#139175) ## Summary Fixes circular reference detection in alias chains during `DeduplicateAggs` optimizations. ## Problem The `PushDownEnrich` optimization can create circular references when pushing `Enrich` past `Project` operations, causing `ql_illegal_argument_exception: Potential cycle detected` errors when `DeduplicateAggs`. Example query: ```esql FROM languages_lookup | RENAME language_name AS message | ENRICH languages_policy ON language_code | STATS a = count(to_lower(language_name)), b = count(language_name) ``` ## Changes Skip aliases that would create circular references by checking if `alias.toAttribute()` equals the resolved `alias.child()`. Closes elastic#138346 Closes elastic#139541 (cherry picked from commit 2289e17) # Conflicts: # x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java # x-pack/plugin/esql/qa/testFixtures/src/main/resources/enrich.csv-spec # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

#139615) ## Summary Fixes circular reference detection in alias chains during `DeduplicateAggs` optimizations. ## Problem The `PushDownEnrich` optimization can create circular references when pushing `Enrich` past `Project` operations, causing `ql_illegal_argument_exception: Potential cycle detected` errors when `DeduplicateAggs`. Example query: ```esql FROM languages_lookup | RENAME language_name AS message | ENRICH languages_policy ON language_code | STATS a = count(to_lower(language_name)), b = count(language_name) ``` ## Changes Skip aliases that would create circular references by checking if `alias.toAttribute()` equals the resolved `alias.child()`. Closes #138346 Closes #139541 (cherry picked from commit 2289e17) # Conflicts: # x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

…139175) (#139617) * ESQL: Prevent circular alias references in `DeduplicateAggs` (#139175) ## Summary Fixes circular reference detection in alias chains during `DeduplicateAggs` optimizations. ## Problem The `PushDownEnrich` optimization can create circular references when pushing `Enrich` past `Project` operations, causing `ql_illegal_argument_exception: Potential cycle detected` errors when `DeduplicateAggs`. Example query: ```esql FROM languages_lookup | RENAME language_name AS message | ENRICH languages_policy ON language_code | STATS a = count(to_lower(language_name)), b = count(language_name) ``` ## Changes Skip aliases that would create circular references by checking if `alias.toAttribute()` equals the resolved `alias.child()`. Closes #138346 Closes #139541 (cherry picked from commit 2289e17) # Conflicts: # x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java # x-pack/plugin/esql/qa/testFixtures/src/main/resources/enrich.csv-spec # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java * unmute GenerativeIT * Fix tests Lookup indices are not loaded in multi-cluster tests in 9.1. Instead of languages_lookup we can also just use languages: same index, just without the lookup mode set. * Revert "Fix tests" This reverts commit b0696b5. Turns out, we cannot use the languages index because this one is also used for enrich, and the multi cluster tests index this two times, on the remote AND on the local cluster. * Use index that isn't duplicated in multi-cluster * Fix more tests... --------- Co-authored-by: Alexander Spies <alexander.spies@elastic.co>

kanoshiou added 2 commits December 8, 2025 00:32

fix(esql): Prevent circular alias references in aggregate deduplication

7920f40

Update generative test

28a93ba

elasticsearchmachine added v9.3.0 external-contributor Pull request authored by a developer outside the Elasticsearch team needs:triage Requires assignment of a team area label labels Dec 7, 2025

Update docs/changelog/139175.yaml

bdc7147

idegtiarenko added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL labels Dec 10, 2025

elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Dec 10, 2025

bpintea self-assigned this Dec 10, 2025

bpintea reviewed Dec 10, 2025

View reviewed changes

Merge branch 'refs/heads/main' into fix-enrich-alias-cycle

8ef1cff

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

capitalise the language keywords

5294f0b

ncordon self-requested a review December 11, 2025 08:53

ncordon self-assigned this Dec 11, 2025

ncordon added the >bug label Dec 11, 2025

kanoshiou added 3 commits December 11, 2025 23:01

Update LogicalPlanOptimizerTests

e047a4a

Merge branch 'refs/heads/main' into fix-enrich-alias-cycle

739b85b

Update LogicalPlanOptimizerTests

adc108f

ncordon reviewed Dec 12, 2025

View reviewed changes

kanoshiou added 2 commits December 12, 2025 19:39

Merge branch 'refs/heads/main' into fix-enrich-alias-cycle

a22ea60

Update test

3279329

ncordon reviewed Dec 12, 2025

View reviewed changes

kanoshiou added 2 commits December 12, 2025 20:53

Merge remote-tracking branch 'origin/main' into fix-enrich-alias-cycle

3128105

Update tests

09d378c

alex-spies mentioned this pull request Dec 15, 2025

[CI] GenerativeIT test failing #139541

Closed

kanoshiou added 2 commits December 16, 2025 09:24

Merge branch 'refs/heads/main' into fix-enrich-alias-cycle

635b25b

# Conflicts: # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

Update field name id in enrich

d9e639e

Update docs/changelog/139175.yaml

cf037e0

alex-spies reviewed Dec 16, 2025

View reviewed changes

kanoshiou added 2 commits December 16, 2025 19:55

Merge branch 'refs/heads/main' into fix-enrich-alias-cycle

08c4053

Assign a new nameId

22517d2

bpintea approved these changes Dec 16, 2025

View reviewed changes

bpintea added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) v9.2.4 auto-backport Automatically create backport pull requests when merged v9.1.10 labels Dec 16, 2025

elasticsearchmachine merged commit 2289e17 into elastic:main Dec 16, 2025
37 checks passed

elasticsearchmachine added the backport pending label Dec 16, 2025

kanoshiou mentioned this pull request Dec 16, 2025

[9.2] ESQL: Prevent circular alias references in DeduplicateAggs (#139175) #139615

Merged

kanoshiou mentioned this pull request Dec 16, 2025

[9.1] ESQL: Prevent circular alias references in DeduplicateAggs (#139175) #139617

Merged

kanoshiou mentioned this pull request Dec 24, 2025

ESQL: Fix multi-value constant propagation after STATS #139442

Merged

		;


		circularRefComplexEnrich

Conversation

kanoshiou commented Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Changes

Uh oh!

elasticsearchmachine commented Dec 10, 2025

Uh oh!

bpintea 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!

kanoshiou commented Dec 10, 2025

Uh oh!

bpintea commented Dec 11, 2025

Uh oh!

ncordon commented Dec 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncordon 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!

ncordon commented Dec 12, 2025

Uh oh!

kanoshiou commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kanoshiou commented Dec 16, 2025

Uh oh!

bpintea commented Dec 16, 2025

Uh oh!

bpintea 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!

Uh oh!

elasticsearchmachine commented Dec 16, 2025

💔 Backport failed

Uh oh!

kanoshiou commented Dec 16, 2025

Uh oh!

bpintea commented Dec 16, 2025

Uh oh!

bpintea commented Dec 16, 2025

Uh oh!

kanoshiou commented Dec 16, 2025

💚 All backports created successfully

Questions ?

Uh oh!

kanoshiou commented Dec 7, 2025 •

edited

Loading

kanoshiou commented Dec 16, 2025 •

edited

Loading