- Notifications
You must be signed in to change notification settings - Fork 25.7k
ESQL: Preserve single aggregate when all attributes are pruned #126397
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: Preserve single aggregate when all attributes are pruned #126397
Conversation
|
|
| Pinging @elastic/es-analytical-engine (Team:Analytics) |
| buildkite test this |
| buildkite test this |
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.
Thank you for catching this bug @kanoshiou!
I wonder if you have considered the option to limit the scope of change inside the PruneColumns rule, if it is the rule that creates an EmptyAttribute that causes trouble in NamedExpressions.mergeOutputExpressions?
Changing NamedExpressions.mergeOutputExpressions to skip EmptyAttribute fixes the bug, however this method is kind of a low level API that is called by parser, the output() method of LogicalPlans and another rule which might be irrelevant to this bug, it looks to me that the scope of impact of this change might be quite larger than we had expected.
An alternative way that I can think of is limiting the change to the PruneColumns itself, when creating a LocalRelation, avoiding using EmptyAttribute, so that it won't cause NamedExpressions.mergeOutputExpressions to add a null field to the output.
| Thank you for your review @fang-xing-esql ! Your concern about I've updated the branch. Could you please review it again? |
fang-xing-esql left a comment
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.
Thank you @kanoshiou! LGTM
| p = new LocalRelation( | ||
| aggregate.source(), | ||
| List.of(new EmptyAttribute(aggregate.source())), | ||
| List.of(Expressions.attribute(aggregate.aggregates().getFirst())), |
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 was thinking about something like List.of(aggregate.output().get(0)), your way does a similar job too.
| buildkite test this |
EmptyAttribute in merging output expressions| buildkite test this |
💚 Backport successful
|
In
PruneColumns,Aggregatemight be optimized to produce aLocalRelationoutput with anEmptyAttribute. However, the subsequent eval execution does not filter out thisEmptyAttribute, which could lead to an additionalnullcolumn with an empty name appearing in the result.Closes #126392