[SPARK-51820][SQL] Move UnresolvedOrdinal construction before analysis to avoid issue with group by ordinal
#50606
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
mihailotim-db
force-pushed
the
mihailotim-db/new_group_by_ordinal
branch
5 times, most recently
from
da5bdea to
d0a8187
Compare
mihailotim-db
commented
Apr 16, 2025
Contributor
Author
There was a problem hiding this comment.
I am removing some test cases from AggregateResolverSuite because we need to support the new behavior in single-pass analyzer as well, but that might bloat this PR too much. I would like to do that in a followup where I will revert this change
Contributor
There was a problem hiding this comment.
Can you copy these test contents to the Jira so we don't forget?
mihailotim-db
force-pushed
the
mihailotim-db/new_group_by_ordinal
branch
from
d0a8187 to
daa2aa3
Compare
mihailotim-db
changed the title
UnresolvedOrdinal construction before analysis to avoid issue with group by ordinal
mihailotim-db
force-pushed
the
mihailotim-db/new_group_by_ordinal
branch
from
daa2aa3 to
56bbcb1
Compare
dtenedor
reviewed
Apr 16, 2025
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/classic/Dataset.scala
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Can you copy these test contents to the Jira so we don't forget?
dtenedor
reviewed
Apr 16, 2025
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
Outdated
Show resolved
Hide resolved
Contributor
|
shall we also handle Spark Connect queries in |
mihailotim-db
force-pushed
the
mihailotim-db/new_group_by_ordinal
branch
from
56bbcb1 to
33ec241
Compare
mihailotim-db
force-pushed
the
mihailotim-db/new_group_by_ordinal
branch
from
33ec241 to
464d2ab
Compare
Contributor
Author
Sure, done! |
cloud-fan
approved these changes
Apr 17, 2025
vladimirg-db
approved these changes
Apr 17, 2025
sql/core/src/test/scala/org/apache/spark/sql/analysis/resolver/AggregateResolverSuite.scala
Outdated
Show resolved
Hide resolved
...yst/src/main/scala/org/apache/spark/sql/catalyst/analysis/SubstituteUnresolvedOrdinals.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
Outdated
Show resolved
Hide resolved
...test/scala/org/apache/spark/sql/catalyst/analysis/GroupByOrdinalsRepeatedAnalysisSuite.scala
Outdated
Show resolved
Hide resolved
Contributor
|
Just one thing we need to check - if the view is persisted with The view must keep its confs. |
Contributor
Author
Confirmed the correct behavior in the shell. With conf off, the query should error out with |
mihailotim-db
force-pushed
the
mihailotim-db/new_group_by_ordinal
branch
from
464d2ab to
62729a8
Compare
mihailotim-db
force-pushed
the
mihailotim-db/new_group_by_ordinal
branch
from
62729a8 to
e62c9d6
Compare
dtenedor
approved these changes
Apr 17, 2025
Contributor
|
thanks, merging to master! |
cloud-fan
pushed a commit
that referenced
this pull request
Apr 23, 2025
### What changes were proposed in this pull request? This is a followup to #50606 to fix the origin and context of newly created `UnresolvedOrdinal`. ### Why are the changes needed? Origin and context of `UnresolvedOrdinal` should be the same as for the original `Literal`. ### 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 #50676 from mihailotim-db/mihailotim-db/fix_origin. 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 29, 2025
…`order` by ordinal approach ### What changes were proposed in this pull request? This is a followup to #50606, to address remaining issues in fixed-point analyzer and parser. This PR does the following: 1. Set correct origin in `SparkConnect` and `Dataframe` API 2. Handle remaining cases for order by ordinal in `Dataframe` API 3. Add a test suite for Dataframe use cases since we are missing those ### Why are the changes needed? To address current issue with `UnresolvedOrdinal` ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added a new test suite and existing tests. ### Was this patch authored or co-authored using generative AI tooling? No Closes #50699 from mihailotim-db/mihailotim-db/fix_ordinal_followup. 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
May 20, 2025
…hen appending grouping to aggregate expressions ### What changes were proposed in this pull request? Don't add `UnresolvedOrdinal` when appending grouping to aggregate expressions in Spark Connect. ### Why are the changes needed? Change is needed to fix a regression caused by #50606 where `UnresolvedOrdinal` would end up in aggregate expression and propagate all the way to `CheckAnalysis` where it would throw an error ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added a test case for the correct behavior. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #50958 from mihailotim-db/mihailotim-db/fix_unresolved_ordinal. Authored-by: Mihailo Timotic <mihailo.timotic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
yhuang-db
pushed a commit
to yhuang-db/spark
that referenced
this pull request
Jun 9, 2025
…`order` by ordinal approach ### What changes were proposed in this pull request? This is a followup to apache#50606, to address remaining issues in fixed-point analyzer and parser. This PR does the following: 1. Set correct origin in `SparkConnect` and `Dataframe` API 2. Handle remaining cases for order by ordinal in `Dataframe` API 3. Add a test suite for Dataframe use cases since we are missing those ### Why are the changes needed? To address current issue with `UnresolvedOrdinal` ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added a new test suite and existing tests. ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#50699 from mihailotim-db/mihailotim-db/fix_ordinal_followup. Authored-by: Mihailo Timotic <mihailo.timotic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
yhuang-db
pushed a commit
to yhuang-db/spark
that referenced
this pull request
Jun 9, 2025
…hen appending grouping to aggregate expressions ### What changes were proposed in this pull request? Don't add `UnresolvedOrdinal` when appending grouping to aggregate expressions in Spark Connect. ### Why are the changes needed? Change is needed to fix a regression caused by apache#50606 where `UnresolvedOrdinal` would end up in aggregate expression and propagate all the way to `CheckAnalysis` where it would throw an error ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added a test case for the correct behavior. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#50958 from mihailotim-db/mihailotim-db/fix_unresolved_ordinal. Authored-by: Mihailo Timotic <mihailo.timotic@databricks.com> 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
4 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 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
UnresolvedOrdinalconstruction 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