-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-33503][SQL] Refactor SortOrder class to allow multiple childrens #30430
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
[SPARK-33503][SQL] Refactor SortOrder class to allow multiple childrens #30430
Conversation
|
@cloud-fan @maropu I have made the changes as suggested in #30302 (comment) . |
|
ok to test |
|
@prakharjain09 Thanks for working on this. btw, could you assign a new jira ID to this? |
| child: Expression, | ||
| direction: SortDirection, | ||
| nullOrdering: NullOrdering, | ||
| sameOrderExpressions: Set[Expression]) |
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.
We need to use Set for this variable?
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.
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.
We can use ExpressionSet maybe at those 2 places and make it a Seq in constructor here. any thoughts?
Yea, I think it is okay to just deduplicate it before storing it in the class as you said, and then make it Seq.
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/SortOrder.scala
Show resolved
Hide resolved
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Test build #131386 has finished for PR 30430 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #131426 has finished for PR 30430 at commit
|
sql/core/src/main/scala/org/apache/spark/sql/execution/joins/SortMergeJoinExec.scala
Outdated
Show resolved
Hide resolved
|
Test build #131785 has finished for PR 30430 at commit
|
|
Test build #131885 has finished for PR 30430 at commit
|
|
Test build #131886 has finished for PR 30430 at commit
|
sql/core/src/test/scala/org/apache/spark/sql/execution/PlannerSuite.scala
Outdated
Show resolved
Hide resolved
|
The GA tests passed, so I merged to master. Thanks! |
What changes were proposed in this pull request?
This is a followup of #30302 . As part of this PR, sameOrderExpressions set is made part of children of SortOrder node - so that they don't need any special handling as done in #30302 .
Why are the changes needed?
sameOrderExpressions should get same treatment as child. So making them part of children helps in transforming them easily.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing UTs