Skip to content

ESQL: Keep DROP attributes when resolving field names #127009

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

kanoshiou
Copy link
Contributor

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 elasticsearchmachine added needs:triage Requires assignment of a team area label v9.1.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Apr 17, 2025
@kanoshiou kanoshiou changed the title Keep drop attributes when resolving field names ESQL: KeepDROP attributes when resolving field names Apr 17, 2025
@kanoshiou kanoshiou changed the title ESQL: KeepDROP attributes when resolving field names ESQL: Keep DROP attributes when resolving field names Apr 17, 2025
…ng-field-names

# Conflicts:
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
@alex-spies alex-spies added :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels Apr 22, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 22, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@astefan
Copy link
Contributor

astefan commented Apr 23, 2025

@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).

@astefan astefan added the >bug label Apr 23, 2025
@kanoshiou
Copy link
Contributor Author

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]

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.

…ng-field-names

# Conflicts:
#	x-pack/plugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/generative/GenerativeRestTest.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL >bug external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ES|QL: no matches for pattern
4 participants