ESQL: Avoid unintended attribute removal

[kanoshiou]

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, commands like lookup join and enrich can introduce fields with the same names as user-defined fields. To accommodate this, we ensure that aliases are not removed from the attributes list when a subsequent JOIN occurs.

For the query:

from message_types 
| eval type = 1 
| lookup join message_types_lookup on message 
| drop  message 
| grok type "%{WORD:b}" 
| stats x = max(b) 
| keep x

The plan grok type "%{WORD:b} passes the removal check and initiates alias removal down the tree. However, the removal process should halt at lookup join due to the reason mentioned above. The issue arises because the original code allows progression to eval type = 1, which inadvertently removes the type field. As a result, the field type ends up being of type integer defined by eval type = 1, rather than preserving the keyword type field type obtained from message_types_lookup.

Closes #127468

[elasticsearchmachine]

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

[astefan]

buildkite test this

[kanoshiou]

I'm really confused. I can get a result of 4325287503714500000 when I run the query on my local machine, but the test says it should be 4325287503714500302. It seems the test result is right, I will update the branch.

[astefan]

@kanoshiou I really appreciate you provided this PR and you tried to understand where the fix needs to go.

Unfortunately, this is a trivial fix that doesn't quite catch the true reason for the initial failure.

The essence of the failure is the fact that lookup join can override the type user-provided field with its own type field. The type from eval is of type integer while the type that comes from the lookup join is of type keyword. Because the list of field names didn't contain type the Analyzer couldn't learn about that type that exists in message_types_lookup and the only thing it knew about its type is that it comes from eval.

lookup join (like enrich and maybe inlinestats) are special in the sense that they add some columns to the result, so some of the rules we have in EsqlSession.fieldNames need to be adjusted to account for these special characteristics. Because lookup join can add a field of the same name as type coming from an eval before the said lookup join command, we have a special check in fieldNames that forbids the removal of those Aliases.

Your proposed solution does this:

• places the input references of grok in a new variable, instead of the referenceBuilder that was used before
• the special check above for lookup join is not applied to referenceBuilder for grok because the input reference is not there anymore
• you add to referenceBuilder the content of the grok-special variable

This is a bypass of the lookup join special removal check.

Instead, let the grok references in referenceBuilder where they are and don't let the special lookup join check remove it. The issue is with the lookup join check, not with grok, so adjust that one. I have the feeling that check is applicable in other cases, only that we didn't catch those yet.

[kanoshiou]

@astefan Thank you for your review and for taking the time to provide such a detailed explanation of the issue's root cause. Your insights have been incredibly helpful, and I truly appreciate your guidance on my code, it has given me a much deeper understanding. I’ve learned a lot from your feedback!

I have reverted the bypass and implemented a modification that I believe addresses the issue. If there are any areas that need improvement or further adjustments, please let me know, I’d be happy to refine it further.

[astefan]

@kanoshiou I have a simpler suggestion for the change in EsqlSession. Instead of the original code in main

if (canRemoveAliases[0] && couldOverrideAliases(p)) {

we can do:

if (canRemoveAliases[0] && p.anyMatch(c -> couldOverrideAliases(c))) {

It will slightly change some of the existent tests in IndexResolverFieldNamesTests in terms of which eval generated field name will be put in the list of fieldNames, but I think that's acceptable.

Please, try it out and let me know your thoughts.

[astefan]

buildkite test this

[kanoshiou]

Thank you for your review and suggestions, @astefan!

I believe the statement p.anyMatch(c -> couldOverrideAliases(c)) will always return true since there must be an UnresolvedRelation at the end. If we disregard the UnresolvedRelation case, I believe your code essentially avoids alias removal whenever there is a lookup join or enrich operation in the tree. However, this contradicts the original intent of minimizing the number of attributes required from field_caps at pre-analysis time.

[astefan]

I believe the statement p.anyMatch(c -> couldOverrideAliases(c)) will always return true since there must be an UnresolvedRelation at the end

Indeed, I forgot about this part. UnresolvedRelation needs to also be accounted for in couldOverrideAliases method.

I believe your code essentially avoids alias removal whenever there is a lookup join or enrich operation in the tree

Yes

However, this contradicts the original intent of minimizing the number of attributes required from field_caps at pre-analysis time.

Yes, it's a fine balance. Being too aggressive in minimizing the list of fields could lead to some of the issues we've had recently. Having more fields in that list, but still avoiding to ask for all the fields (*) which is what we had before, is still an improvement.

[kanoshiou]

Thank you for your review @astefan! Your suggestion has been applied in ee9de1c.

[astefan]

buildkite test this

[astefan]

LGTM

[astefan]

@kanoshiou please, look into the conflicts. Most likely, they are coming from the freshly merged PR.

[kanoshiou]

@astefan conflicts resolved.

[astefan]

buildkite test this

[elasticsearchmachine]

💚 Backport successful

Status	Branch	Result
✅	8.19
✅	9.0

[kanoshiou]

Thank you @astefan!