[SPARK-47895][SQL] group by alias should be idempotent #50461
Closed
Conversation
This file contains hidden or 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
reviewed
Mar 31, 2025
...yst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveReferencesInAggregate.scala
Outdated
Show resolved
Hide resolved
Contributor
|
thanks, merging to master/4.0! |
cloud-fan
pushed a commit
that referenced
this pull request
Mar 31, 2025
### What changes were proposed in this pull request? This is a followup of #43797 . GROUP BY ALIAS has the same bug and this PR applies the same fix to GROUP BY ALIAS ### Why are the changes needed? For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. ### Does this PR introduce _any_ user-facing change? Yes, we will fix the error. ### How was this patch tested? Test added. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #50461 from mihailom-db/mihailom-db/fixGroupBy. Authored-by: Mihailo Milosevic <mihailo.milosevic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 02db872) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
cloud-fan
pushed a commit
that referenced
this pull request
Apr 14, 2025
### What changes were proposed in this pull request? This PR reverts #50461 because it introduces a correctness issue by replacing an `Alias` with incorrect literal. A followup will be made with a different way to fix this issue. ### Why are the changes needed? In the below example, alias `abc` is replaced with `2` resulting in 2 rows instead of correct 3. Before erronous PR:  After erronous PR:  ### Does this PR introduce _any_ user-facing change? User now sees an error message instead of an incorrect result. ### How was this patch tested? Added a test case to check for this behavior in the future ### Was this patch authored or co-authored using generative AI tooling? No Closes #50567 from mihailotim-db/mihailotim-db/revert_group_by. Authored-by: Mihailo Timotic <mihailo.timotic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
cloud-fan
pushed a commit
that referenced
this pull request
Apr 14, 2025
### What changes were proposed in this pull request? This PR reverts #50461 because it introduces a correctness issue by replacing an `Alias` with incorrect literal. A followup will be made with a different way to fix this issue. ### Why are the changes needed? In the below example, alias `abc` is replaced with `2` resulting in 2 rows instead of correct 3. Before erronous PR:  After erronous PR:  ### Does this PR introduce _any_ user-facing change? User now sees an error message instead of an incorrect result. ### How was this patch tested? Added a test case to check for this behavior in the future ### Was this patch authored or co-authored using generative AI tooling? No Closes #50567 from mihailotim-db/mihailotim-db/revert_group_by. Authored-by: Mihailo Timotic <mihailo.timotic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 8718eba) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
cloud-fan
pushed a commit
that referenced
this pull request
Apr 18, 2025
…sis to avoid issue with group by ordinal ### What changes were proposed in this pull request? This is a followup to #43797 and #50461 where hacks were introduced in order to solve the issue of repeated analysis on plans that have a group by ordinal. The latter PR caused a regression so the hack needs to be removed. This PR proposed a move of `UnresolvedOrdinal` construction before Analyzer runs. ### Why are the changes needed? We are reverting a hack introduced in the previous PRs to improve the behavior of group by ordinal and additionally fix the issue that #50461 was trying to solve. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. ### Was this patch authored or co-authored using generative AI tooling? No Closes #50606 from mihailotim-db/mihailotim-db/new_group_by_ordinal. Authored-by: Mihailo Timotic <mihailo.timotic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Member
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
To @mihailom-db , I just want to give you the context in order to prevent future accident like the following.
02db87282da [SPARK-47895][SQL] group by alias should be idempotent
b5bb75ca240 [SPARK-47895][SQL] group by all should be idempotent
ASF JIRA IDs have Fixed Versions field. Even for the direct follow-up PRs, we cannot reuse JIRA IDs when the original patch is released already. It's because the original PR and the follow-up PR cannot have the same Fixed Versions field. Please make it sure that ASF JIRA ID is different from any internal customer ticket systems.
So, please be careful about JIRA IDs next time.
zifeif2
pushed a commit
to zifeif2/spark
that referenced
this pull request
Nov 14, 2025
### What changes were proposed in this pull request? This is a followup of apache#43797 . GROUP BY ALIAS has the same bug and this PR applies the same fix to GROUP BY ALIAS ### Why are the changes needed? For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable. ### Does this PR introduce _any_ user-facing change? Yes, we will fix the error. ### How was this patch tested? Test added. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#50461 from mihailom-db/mihailom-db/fixGroupBy. Authored-by: Mihailo Milosevic <mihailo.milosevic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 9cc8b18) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
zifeif2
pushed a commit
to zifeif2/spark
that referenced
this pull request
Nov 14, 2025
### What changes were proposed in this pull request? This PR reverts apache#50461 because it introduces a correctness issue by replacing an `Alias` with incorrect literal. A followup will be made with a different way to fix this issue. ### Why are the changes needed? In the below example, alias `abc` is replaced with `2` resulting in 2 rows instead of correct 3. Before erronous PR:  After erronous PR:  ### Does this PR introduce _any_ user-facing change? User now sees an error message instead of an incorrect result. ### How was this patch tested? Added a test case to check for this behavior in the future ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#50567 from mihailotim-db/mihailotim-db/revert_group_by. Authored-by: Mihailo Timotic <mihailo.timotic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit f970fe2) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 #43797 . GROUP BY ALIAS has the same bug and this PR applies the same fix to GROUP BY ALIAS
Why are the changes needed?
For advanced users or Spark plugins, they may manipulate the logical plans directly. We need to make the framework more reliable.
Does this PR introduce any user-facing change?
Yes, we will fix the error.
How was this patch tested?
Test added.
Was this patch authored or co-authored using generative AI tooling?
No.