[SPARK-41635][SQL] Fix group by all error reporting #39509
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cloud-fan
commented
Jan 11, 2023
|
|
||
| -- make sure we report the right error when there's an attribute in correlated subquery | ||
| -- that is, to report UNRESOLVED_ALL_IN_GROUP_BY, rather than some random subquery error. | ||
| select coutnry, (select count(*) from data d1 where d1.country = d2.country), count(id) from data d2 group by all; |
There was a problem hiding this comment.
coutnry is an non-existing column and we should report normal missing column error.
| @@ -93,8 +93,9 @@ object ResolveGroupByAll extends Rule[LogicalPlan] { | |||
| * end of analysis, so we can tell users that we fail to infer the grouping columns. | |||
| */ | |||
| def checkAnalysis(operator: LogicalPlan): Unit = operator match { | |||
| case a: Aggregate if matchToken(a) => | |||
| if (a.aggregateExpressions.exists(_.exists(_.isInstanceOf[Attribute]))) { | |||
| case a: Aggregate if a.aggregateExpressions.forall(_.resolved) && matchToken(a) => | |||
There was a problem hiding this comment.
Nit: there are some codes duplicated with the apply method. We can consider simple refactors.
There was a problem hiding this comment.
The check code here is a partial version of the apply code. I can't think of a simple refactor that can reduce more code duplication. Let's think about this later.
There was a problem hiding this comment.
You are right about the check here. For the grouping expression inference, I think we can refactor it as #39523
gengliangwang
approved these changes
Jan 11, 2023
|
thanks for review, merging to master! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This is a followup of #39134 . It fixes a bug in the error reporting: when detecting the failed GROUP BY ALL case, we should use the same code that was used to resolve GROUP BY ALL. This bug was hidden because the test has a typo.
Why are the changes needed?
bug fix
Does this PR introduce any user-facing change?
yes, better errors
How was this patch tested?
new tests