Skip to content

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

Merged
elasticsearchmachine merged 10 commits intoelastic:9.1from
kanoshiou:backport/9.1/pr-139175
Jan 23, 2026
Merged

[9.1] ESQL: Prevent circular alias references in DeduplicateAggs (#139175)#139617
elasticsearchmachine merged 10 commits intoelastic:9.1from
kanoshiou:backport/9.1/pr-139175

Conversation

@kanoshiou
Copy link
Copy Markdown
Contributor

@kanoshiou kanoshiou commented Dec 16, 2025

Backport

This will backport the following commits from main to 9.1:

Questions ?

Please refer to the Backport tool documentation

@kanoshiou
ESQL: Prevent circular alias references in DeduplicateAggs (elastic…
903d47c 
…#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
@kanoshiou kanoshiou mentioned this pull request Dec 16, 2025
Merged
@elasticsearchmachine elasticsearchmachine added v9.1.10 external-contributor Pull request authored by a developer outside the Elasticsearch team needs:triage Requires assignment of a team area label labels Dec 16, 2025
@kanoshiou
Copy link
Copy Markdown
Contributor Author

kanoshiou commented Dec 16, 2025

@bpintea I had a few hiccups during the backport, but hopefully we’re good now.

@bpintea bpintea self-assigned this Dec 16, 2025
@bpintea bpintea added >bug backport :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels Dec 16, 2025
@bpintea
Copy link
Copy Markdown
Contributor

bpintea commented Dec 16, 2025

buildkite test this

@bpintea bpintea added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 16, 2025
@kanoshiou
Copy link
Copy Markdown
Contributor Author

kanoshiou commented Dec 16, 2025

I’ve verified that the query below reproduces the issue on this patch if the code in Enrich is reverted, and I’m now updating the test.

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

But something strange happened:

in the multi-cluster tests, the response is 8 | 8, while on a single node it’s 4 | 4.

@kanoshiou
Copy link
Copy Markdown
Contributor Author

kanoshiou commented Dec 18, 2025

Hi @bpintea,

Does this CI failure look familiar to you? I was wondering if you might have some insight.

@bpintea
Copy link
Copy Markdown
Contributor

bpintea commented Dec 19, 2025

Hey @kanoshiou,

Does this CI failure look familiar to you? I was wondering if you might have some insight.

Unfortunately not. :)
I'd have to dig a bit to see what's going on.

@kanoshiou
Merge remote-tracking branch 'origin/9.1' into backport/9.1/pr-139175
5b3f3bf 
# Conflicts:
#	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
@alex-spies alex-spies mentioned this pull request Jan 23, 2026
Closed
@alex-spies
Copy link
Copy Markdown
Contributor

alex-spies commented Jan 23, 2026

@elasticsearchmachine test this please

@alex-spies
Fix tests
b0696b5 
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.
@alex-spies
Copy link
Copy Markdown
Contributor

alex-spies commented Jan 23, 2026

@elasticsearchmachine test this please

@alex-spies
Revert "Fix tests"
3fc8f68 
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.
@alex-spies alex-spies self-assigned this Jan 23, 2026
@alex-spies
Copy link
Copy Markdown
Contributor

alex-spies commented Jan 23, 2026

I’ve verified that the query below reproduces the issue on this patch if the code in Enrich is reverted, and I’m now updating the test.

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

But something strange happened:

in the multi-cluster tests, the response is 8 | 8, while on a single node it’s 4 | 4.

This happens because the multi-cluster tests (randomly?) turn a FROM languages into a FROM languages, *:languages. Because the languages index is used for ENRICH, this index exists both on the local and the remote clusters. So the data is literally duplicated, and having 8 in the result is actually correct for the multi-cluster tests, specifically.

Except that the spec test has to work for both multi- and single-cluster tests :)

I'll add a copy of the languages index that isn't also used for ENRICH and thus indexed twice.

@kanoshiou
Copy link
Copy Markdown
Contributor Author

kanoshiou commented Jan 23, 2026

Thank you for your detailed explanation, @alex-spies! I'm just a bit puzzled by the inconsistent test behavior between branches.

@alex-spies
Copy link
Copy Markdown
Contributor

alex-spies commented Jan 23, 2026

@elasticsearchmachine test this please

@alex-spies
Copy link
Copy Markdown
Contributor

alex-spies commented Jan 23, 2026 •
edited
Loading

I'm just a bit puzzled by the inconsistent test behavior between branches.

9.1 didn't have lookup joins in CCS, so CCS-tests just didn't allow lookup indices. That's why you didn't see this fail on 9.2, for instance.

@alex-spies
Copy link
Copy Markdown
Contributor

alex-spies commented Jan 23, 2026

@elasticsearchmachine test this please

@alex-spies
Copy link
Copy Markdown
Contributor

alex-spies commented Jan 23, 2026

@elasticsearchmachine test this please

@elasticsearchmachine elasticsearchmachine merged commit 9c0d758 into elastic:9.1 Jan 23, 2026
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@bpintea bpintea

@alex-spies alex-spies

Labels

:Analytics/ES|QL AKA ESQL auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) backport >bug external-contributor Pull request authored by a developer outside the Elasticsearch team v9.1.11

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kanoshiou @bpintea @alex-spies @elasticsearchmachine