Skip to content

Fix ResolveRefs may not resolve a resolvable aggregation with grouping #118473

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

Closed

Conversation

jackpan123
Copy link
Contributor

Closes #116781
When the plan first enters the execution of resolveAggregate method, we can observe that c as an UnresolvedAttribute object has a value for the unresolvedMsg attribute which is not null. However, the customMessage attribute is false. But In the potentialCandidatesIfNoMatchesFound method, calling withUnresolvedMessage to regenerate the UnresolvedAttribute object sets the customMessage attribute just based on whether unresolvedMessage != null. This behavior may be a little confusion.

Thus, relying solely on the customMessage attribute of UnresolvedAttribute for some judgments is insufficient and may lead to errors. To address this, I have added additional checks to ensure that the maybeResolveAttribute method and potentialCandidatesIfNoMatchesFound behavior as we expected.

@elasticsearchmachine elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team needs:triage Requires assignment of a team area label v9.0.0 labels Dec 11, 2024
@jackpan123
Copy link
Contributor Author

@elasticsearchmachine generate changelog

@benwtrent benwtrent added :Analytics/ES|QL AKA ESQL and removed needs:triage Requires assignment of a team area label labels Dec 12, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Dec 12, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@alex-spies alex-spies self-requested a review January 20, 2025 14:49
@alex-spies alex-spies self-assigned this Jan 24, 2025
@wchaparro
Copy link
Member

@alex-spies should we take a look again on this one?

@fang-xing-esql fang-xing-esql self-requested a review April 29, 2025 14:16
@alex-spies
Copy link
Contributor

Thanks a lot @jackpan123 and I'm terribly sorry it took so long to get back to you and review your submission.

Unfortunately, the change proposed here has a pretty large blast radius; while the change is small in terms of changed lines, it changes maybeResolveAttribute. This method is used all over in the reference resolution step, whereas the scope of the bug is limited to very specific edge cases when resolving aggregates.

Running the VerifierTests on this PR shows 4 failures where we return wrong error messages to users, and in one case even the analyzer doesn't terminate anymore. For this reason, I think we should attempt a different approach.

I'm closing this - sorry @jackpan123 .

In the meantime, @fang-xing-esql is working on a more contained fix right now that is limited to the resolution of aggs, only.

@alex-spies alex-spies closed this Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL 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
5 participants