-
Notifications
You must be signed in to change notification settings - Fork 25k
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
ESQL: Ask for correct field names if only LOOKUP/ENRICH fields are kept #120372
base: main
Are you sure you want to change the base?
Conversation
Hi @ivancea, I've created a changelog YAML for you. |
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.
I agree that making fieldNames
more accurate requires quite some work and would also increase the blast radius of the fix. That's because I believe fieldNames
would have to go down the parsed plan, and whenever we encounter a LOOKUP we'd need to perform a field caps call before moving further down.
We had discussions about the required precision of fieldNames
not too long ago between @astefan , @craigtaverner and myself. At the time, we agreed that asking for too many fields was generally okay. (Although it leads to larger than required field caps requests/responses.)
In that light, I believe the proposed fix here is very nice and contained. But I believe @astefan should have a good hard look at this.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/session/EsqlSession.java
Show resolved
Hide resolved
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.
If we decide to go forward with this approach, it'll require a couple nice unit tests for the change to fieldNames
- and we should try and be as creative as possible with these.
The two test classes are xpack/esql/session/IndexResolverFieldNamesTests.java
and /xpack/esql/qa/rest/RequestIndexFilteringTestCase.java
- you can also compare with the tests introduced in Craig's PR here.
Fixes #120272
This is just a draft as the solution is quite bruteforce-y.
Some context on the bug
The issue occurs as follows, top-down (Read linked issue for context on the bug):
FROM index | EVAL x = 1 | LOOKUP l ON x | KEEP lookup_field
returns no rows. Exactly the same with ENRICHPotential solution
Givens:
Which means we will probably have to reorganize the
EsqlSession.analyzedPlan()
code to correctly trace every field.Current solution
This PR's solution is a simple, lazy one. It works well apparently, and I suppose it wasn't done before to try to keep the fieldNames "exact". But since enrich&lookup, it wasn't exact anymore, and we were asking for enrich/lookup index fields indistinctly.
Consider this a temporary solution until analyzedPlan() and fieldNames() get a refactor