-
Notifications
You must be signed in to change notification settings - Fork 25.7k
ES|QL: Fix Fork field reference tracking #131723
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
9f24a18
594d76a
69fff05
b64ec91
666382c
15480b6
96951c7
8a98b45
bb7082d
a20fb5b
b159bd7
3b7f546
f9f26d4
917b273
cb412e6
c5564f0
7d67986
2479bfa
3ebe919
482fabe
300f169
6378f76
f337b02
82f189d
199392f
b7a7854
5dd7462
98aa887
7bc4a87
b017321
d14bf24
2931d83
82672be
94da34b
9606c04
3f55419
00a885a
4313222
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2080,7 +2080,7 @@ public void testForkFieldsWithKeepAfterFork() { | |
| (WHERE d > 1000 AND e == "aaa" | EVAL c = a + 200) | ||
| | WHERE x > y | ||
| | KEEP a, b, c, d, x | ||
| """, ALL_FIELDS); | ||
| """, Set.of("a", "x", "y", "d", "e", "e.*", "d.*", "y.*", "x.*", "a.*")); | ||
| } | ||
|
|
||
| public void testForkFieldsWithKeepBeforeFork() { | ||
|
|
@@ -2092,7 +2092,7 @@ public void testForkFieldsWithKeepBeforeFork() { | |
| | FORK (WHERE c > 1 AND a < 10000 | EVAL d = a + 500) | ||
| (WHERE d > 1000 AND e == "aaa" | EVAL c = a + 200) | ||
| | WHERE x > y | ||
| """, ALL_FIELDS); | ||
| """, Set.of("x", "y", "a", "d", "e", "b", "c", "e.*", "d.*", "y.*", "x.*", "a.*", "c.*", "b.*")); | ||
| } | ||
|
|
||
| public void testForkFieldsWithNoProjection() { | ||
|
|
@@ -2118,17 +2118,41 @@ public void testForkFieldsWithStatsInOneBranch() { | |
| } | ||
|
|
||
| public void testForkFieldsWithEnrichAndLookupJoins() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add some more tests? it would be great if you can test this case and see that the following 2 queries have the same references: Then I would also add some more tests where the FORK branches themselves use KEEP and DROP (with wildcard field patterns too). When I added these tests the FORK branches did not support commands like KEEP and DROP, but now they do.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for clarifying! Again, for future reference, the original issue is a great place to record those particulars. |
||
| assertFieldNames(""" | ||
| FROM test | ||
| | KEEP a, b, abc, def, z, xyz | ||
| | ENRICH enrich_policy ON abc | ||
| | EVAL b = a + 100 | ||
| | LOOKUP JOIN my_lookup_index ON def | ||
| | FORK (WHERE c > 1 AND a < 10000 | EVAL d = a + 500) | ||
| (STATS x = count(*), y=min(z)) | ||
| | LOOKUP JOIN my_lookup_index ON xyz | ||
| | WHERE x > y OR _fork == "fork1" | ||
| """, ALL_FIELDS); | ||
| assertFieldNames( | ||
| """ | ||
| FROM test | ||
| | KEEP a, b, abc, def, z, xyz | ||
| | ENRICH enrich_policy ON abc | ||
| | EVAL b = a + 100 | ||
| | LOOKUP JOIN my_lookup_index ON def | ||
| | FORK (WHERE c > 1 AND a < 10000 | EVAL d = a + 500) | ||
| (STATS x = count(*), y=min(z)) | ||
| | LOOKUP JOIN my_lookup_index ON xyz | ||
| | WHERE x > y OR _fork == "fork1" | ||
| """, | ||
| Set.of( | ||
| "x", | ||
| "y", | ||
| "_fork", | ||
| "a", | ||
| "c", | ||
| "abc", | ||
| "b", | ||
| "def", | ||
| "z", | ||
| "xyz", | ||
| "def.*", | ||
| "_fork.*", | ||
| "y.*", | ||
| "x.*", | ||
| "xyz.*", | ||
| "z.*", | ||
| "abc.*", | ||
| "a.*", | ||
| "c.*", | ||
| "b.*" | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| public void testForkWithStatsInAllBranches() { | ||
|
|
@@ -2140,7 +2164,7 @@ public void testForkWithStatsInAllBranches() { | |
| (EVAL z = a * b | STATS m = max(z)) | ||
| (STATS x = count(*), y=min(z)) | ||
| | WHERE x > y | ||
| """, ALL_FIELDS); | ||
| """, Set.of("c", "a", "z", "z.*", "a.*", "c.*")); | ||
| } | ||
|
|
||
| public void testForkWithStatsAndWhere() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these look like the changes from #128193 - these are not going to work.
please check what happens later in the
fieldNamesmethod. For example here where we remove field references - this has the potential to work incorrectly for FORK:elasticsearch/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java
Lines 909 to 929 in 3ecfed1
As an example, I tested the following queries locally:
They are pretty much the same - just the order of the FORK branches is different.
The first one breaks with:
the second one works.
the first one breaks because most likely we remove the reference to
last_nameas part of the logic from the snippet I shared earlier.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing those. For the future, please record such failing cases in the original github issue. This will be helpful for folks working on it to understand the context better.