ESQL: Keep DROP attributes when resolving field names

[kanoshiou]

For the following query:

from test 
| eval first_name = 1 
| drop first_name 
| drop *name 
| keep gender

During field name resolution, the attribute *name is removed because the alias first_name, defined in eval first_name = 1, matches the regex pattern. However, this behavior is incorrect, as other fields matching the regex pattern may also exist.

Closes #126418

[elasticsearchmachine]

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

[astefan]

@kanoshiou thank you for looking into this issue and preparing a PR.
I am not sure this is the best solution, though. We already have a test that is similar to the one that is failing, but that one is passing - IndexResolverFieldNamesTests.testProjectDropWithQuotedAndUnquotedPatternAndKeepOthers:

from test
            | drop `l`*
            | keep first_name, salary

How is this different from the query that you use as a test in this PR (I am looking for a deep analysis explanation)?

Also, in this PR you are manipulating the content of keepCommandRefsBuilder which has a slightly different purpose (and name) than what you are adding to it with your change:

                    if (p instanceof Keep || p instanceof Drop) {
                        keepCommandRefsBuilder.add(ua);
                    }

This specific change (which doesn't seem quite right and fit) combined with the fact that there is an already very similar passing test leads me to question this solution as being the best (I am not arguing it's not fixing the issue, though).

[kanoshiou]

Thank you for reviewing @astefan !

For query:

from employees
| eval full_name = 12
| drop full_name
| drop *name
| keep emp_no

The significant difference compared to the query you mentioned earlier is the alias created by eval full_name = 12.

When processing the plan:

Drop[[?*name]]
\_Drop[[?full_name]]
  \_Eval[[12[INTEGER] AS full_name]]
    \_UnresolvedRelation[employees]

The attribute ?*name in referencesBuilder would be removed by the code below, as the alias full_name matches the pattern. As a result, an EsRelation containing only emp_no is produced: \_EsRelation[employees][emp_no{f}#58]

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

Lines 658 to 665 in 1a8dadc

	p.forEachExpressionDown(Alias.class, alias -> {
	// do not remove the UnresolvedAttribute that has the same name as its alias, ie "rename id AS id"
	// or the UnresolvedAttributes that are used in Functions that have aliases "STATS id = MAX(id)"
	if (fieldNames.contains(alias.name())) {
	return;
	}
	referencesBuilder.removeIf(attr -> matchByName(attr, alias.name(), keepCommandRefsBuilder.contains(attr)));
	});

you are manipulating the content of keepCommandRefsBuilder which has a slightly different purpose (and name) than what you are adding to it with your change

You are correct that adding a drop attribute in keepCommandRefsBuilder may not be ideal. However, I haven’t yet thought of a more elegant solution. Perhaps we could create a separate keepDropRefsBuilder and check if the attribute exists within it when removing aliases.

[astefan]

buildkite test this

[astefan]

@kanoshiou your solution LGTM.
I've made some very small adjustments to it after a more thorough look at the code, but in essence is the same solution as your original one. I have also added two more unit tests.

[astefan]

LGTM

[kanoshiou]

Thank you, @astefan! I think you should ping buildkite again to run the CI tests, as the new commit 6d80d58 has caused a block.

[astefan]

buildkite test this

[astefan]

buildkite test this

[astefan]

buildkite test this

[astefan]

buildkite test this

[astefan]

buildkite test this

[elasticsearchmachine]

💔 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 127009

[alex-spies]

@astefan , since this was backported to 9.0, shouldn't we also backport to 8.18?

[astefan]

@alex-spies done.