ESQL: Fix alias removal in regex extraction with `JOIN` by kanoshiou · Pull Request #127687 · elastic/elasticsearch

kanoshiou · 2025-05-04T16:46:00Z

Because we aim to minimize the number of attributes required from field_caps at pre-analysis time, a removal check helps eliminate fields defined by users (such as eval). However, this removal logic does not apply to ReferenceAttribute produced by regex extraction commands grok and dissect, even though these fields are also user-defined.

Closes #127467

…emoval

elasticsearchmachine · 2025-05-05T14:03:12Z

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

astefan · 2025-05-05T14:03:14Z

buildkite test this

astefan

The solution is not optimal. Maybe it fixes the problem, but the canRemoveAliases references in multiple places makes the code hard to follow and reason about.

Instead, look at the code that is tied to lookup join and enrich where, after seeing this issue with dissect, it is clear that this code - p.forEachExpressionDown(Alias.class, alias -> { shouldn't only apply to Alias (which comes from eval commands), but also to whatever comes out from dissect (meaning, any user-defined columns).

Rather than using canRemoveAliases[0] in RegexExtract check, look into adjusting the code I mentioned above.

…emoval

kanoshiou · 2025-05-08T16:07:35Z

Thank you for your review and detailed guidance @astefan! Your keen eye for code goes far beyond mine, I still have a long journey ahead in learning and improving.

I've updated the branch, please feel free to review it again at your convenience. If there are any further adjustments needed, I'm more than happy to make them.

astefan · 2025-05-09T09:27:47Z

buildkite test this

astefan · 2025-05-09T12:34:25Z

buildkite test this

astefan · 2025-05-09T14:08:10Z

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

                AttributeSet planRefs = p.references();
                Set<String> fieldNames = planRefs.names();
-                p.forEachExpressionDown(Alias.class, alias -> {
+                p.forEachExpressionDown(NamedExpression.class, expression -> {


Just a naming preference towards NamedExpression - ne that's used in other places in code.

Suggested change

p.forEachExpressionDown(NamedExpression.class, expression -> {

p.forEachExpressionDown(NamedExpression.class, ne-> {

astefan · 2025-05-09T14:11:41Z

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

 ;
+
+
+joinMaskingRegex


I am not a big fan of having such test queries here. At least, add a comment with the link to the original bug report.

astefan · 2025-05-09T14:13:40Z

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

+joinMaskingDissect
+required_capability: join_lookup_v12
+required_capability: fix_join_masking_regex_extract
+from sample_data


Add this test and the one below in IndexResolverFieldNamesTests as well.

I think you missed this request here.

Oh, sorry! I misunderstood what you meant. I was actually thinking about adding comments of links to the original issue.

Added in d56383b

astefan · 2025-05-09T14:21:58Z

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

                    }
-                    referencesBuilder.removeIf(attr -> matchByName(attr, alias.name(), keepCommandRefsBuilder.contains(attr)));
+                    referencesBuilder.removeIf(
+                        attr -> matchByName(attr, expression.name(), (expression instanceof Alias) && keepCommandRefsBuilder.contains(attr))


Why this check here? (expression instanceof Alias)

I didn't give it much thought since skipIfPattern was set to false for RegexExtract. After considering it briefly, I think removing this check wouldn't hurt too much.

…emoval

astefan · 2025-05-12T21:12:17Z

buildkite test this

astefan · 2025-05-12T23:58:40Z

buildkite test this

…emoval # 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/lookup-join.csv-spec # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java

astefan

LGTM

astefan · 2025-05-20T10:08:50Z

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

+        /**
+         * During resolution (pre-analysis) we have to consider that joins can override regex extracted values
+         * see <a href="https://github.com/elastic/elasticsearch/issues/127467"> ES|QL: pruning of JOINs leads to missing fields #127467 </a>
+         */
+        FIX_JOIN_MASKING_REGEX_EXTRACT,
+


Please, add this capability at the end of the list of capabilities. It will be slightly easier for me to manually backport the PR, if the automatic backport fails.

astefan · 2025-05-20T10:09:17Z

When you get the chance, please integrate new changes from main and I'll start a new CI round of tests. Thank you.

…emoval

kanoshiou · 2025-05-20T10:13:36Z

Thanks @astefan. The branch has been updated.

astefan · 2025-05-20T10:14:55Z

buildkite test this

astefan · 2025-05-20T10:33:03Z

buildkite test this

elasticsearchmachine · 2025-05-20T12:08:00Z

💔 Backport failed

Status	Branch	Result
❌	8.19	Commit could not be cherrypicked due to conflicts
❌	9.0	Commit could not be cherrypicked due to conflicts

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

kanoshiou · 2025-05-20T12:10:00Z

Thank you @astefan!

* Disallow removal of regex extracted fields --------- Co-authored-by: Andrei Stefan <astefan@users.noreply.github.com> Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> (cherry picked from commit 557f1f1)

…28202) * Disallow removal of regex extracted fields --------- (cherry picked from commit 557f1f1) Co-authored-by: kanoshiou <73424326+kanoshiou@users.noreply.github.com> Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

…28204) * ESQL: Fix alias removal in regex extraction with `JOIN` (#127687) * Disallow removal of regex extracted fields --------- Co-authored-by: Andrei Stefan <astefan@users.noreply.github.com> Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> (cherry picked from commit 557f1f1) * Checkstyle --------- Co-authored-by: kanoshiou <73424326+kanoshiou@users.noreply.github.com> Co-authored-by: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co>

…) (elastic#128204) * Disallow removal of regex extracted fields

…28204) (#130566) * Disallow removal of regex extracted fields

Disallow removal of regex extracted fields

f33b994

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

kanoshiou added 3 commits May 5, 2025 00:48

Update docs/changelog/127687.yaml

b42cf9f

Update tests

0655294

Merge branch 'refs/heads/main' into fix-join-extracted-regex-fields-r…

10b7991

…emoval

astefan self-requested a review May 5, 2025 07:44

astefan self-assigned this May 5, 2025

astefan added >bug :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels May 5, 2025

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

astefan requested changes May 5, 2025

View reviewed changes

astefan requested a review from luigidellaquila May 5, 2025 17:41

kanoshiou added 2 commits May 8, 2025 23:40

Merge branch 'refs/heads/main' into fix-join-extracted-regex-fields-r…

3439f1f

…emoval

Also remove ReferenceAttribute

ed1074e

astefan self-requested a review May 9, 2025 09:28

Merge branch 'main' into fix-join-extracted-regex-fields-removal

3ab5965

astefan reviewed May 9, 2025

View reviewed changes

kanoshiou and others added 4 commits May 10, 2025 14:09

Merge branch 'refs/heads/main' into fix-join-extracted-regex-fields-r…

cbe265d

…emoval

Comments

6c700a8

Add capability

044d3fa

Merge branch 'main' into fix-join-extracted-regex-fields-removal

31c2b2c

kanoshiou added 2 commits May 13, 2025 09:15

Update IndexResolverFieldNamesTests

d56383b

astefan self-requested a review May 20, 2025 10:04

astefan approved these changes May 20, 2025

View reviewed changes

astefan reviewed May 20, 2025

View reviewed changes

astefan added v8.19.0 v9.0.2 auto-backport Automatically create backport pull requests when merged labels May 20, 2025

kanoshiou added 2 commits May 20, 2025 18:12

Merge branch 'refs/heads/main' into fix-join-extracted-regex-fields-r…

3af81d5

…emoval

Move capability to the bottom

ba02e1d

astefan merged commit 557f1f1 into elastic:main May 20, 2025
19 checks passed

elasticsearchmachine added the backport pending label May 20, 2025

astefan mentioned this pull request May 20, 2025

ESQL: Fix alias removal in regex extraction with JOIN (#127687) #128202

Merged

astefan mentioned this pull request May 20, 2025

ESQL: Fix alias removal in regex extraction with JOIN (#127687) #128204

Merged

astefan removed the backport pending label May 21, 2025

astefan added a commit to astefan/elasticsearch that referenced this pull request Jul 3, 2025

ESQL: Fix alias removal in regex extraction with JOIN (elastic#127687…

8360151

…) (elastic#128204) * Disallow removal of regex extracted fields

astefan mentioned this pull request Jul 3, 2025

ESQL: Fix alias removal in regex extraction with JOIN (#127687) (#128204) #130566

Merged

elasticsearchmachine pushed a commit that referenced this pull request Jul 3, 2025

ESQL: Fix alias removal in regex extraction with JOIN (#127687) (#1…

acae036

…28204) (#130566) * Disallow removal of regex extracted fields

astefan added the v8.18.4 label Jul 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Fix alias removal in regex extraction with `JOIN`#127687

ESQL: Fix alias removal in regex extraction with `JOIN`#127687
astefan merged 16 commits intoelastic:mainfrom
kanoshiou:fix-join-extracted-regex-fields-removal

kanoshiou commented May 4, 2025 •

edited

Loading

elasticsearchmachine commented May 5, 2025

astefan commented May 5, 2025

astefan left a comment •

edited

Loading

kanoshiou commented May 8, 2025

astefan commented May 9, 2025

astefan commented May 9, 2025

astefan May 9, 2025

astefan May 9, 2025

astefan May 9, 2025

astefan May 12, 2025

kanoshiou May 13, 2025

kanoshiou May 13, 2025

astefan May 9, 2025

kanoshiou May 10, 2025 •

edited

Loading

astefan commented May 12, 2025

astefan commented May 12, 2025

astefan left a comment

astefan May 20, 2025

astefan commented May 20, 2025

kanoshiou commented May 20, 2025

astefan commented May 20, 2025

astefan commented May 20, 2025

Uh oh!

elasticsearchmachine commented May 20, 2025

kanoshiou commented May 20, 2025

Labels

3 participants

	p.forEachExpressionDown(NamedExpression.class, expression -> {
	p.forEachExpressionDown(NamedExpression.class, ne-> {

		;


		joinMaskingRegex

Conversation

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

Uh oh!

elasticsearchmachine commented May 5, 2025

astefan commented May 5, 2025

astefan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

kanoshiou commented May 8, 2025

astefan commented May 9, 2025

astefan commented May 9, 2025

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

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

Uh oh!

Choose a reason for hiding this comment

astefan commented May 12, 2025

astefan commented May 12, 2025

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan commented May 20, 2025

kanoshiou commented May 20, 2025

astefan commented May 20, 2025

astefan commented May 20, 2025

Uh oh!

elasticsearchmachine commented May 20, 2025

💔 Backport failed

kanoshiou commented May 20, 2025

Labels

3 participants

kanoshiou commented May 4, 2025 •

edited

Loading

astefan left a comment •

edited

Loading

kanoshiou May 10, 2025 •

edited

Loading