-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-47430][SQL] Rework group by map type to fix bind reference exception #47545
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
Conversation
|
cc @cloud-fan @stevomitric thank you |
|
cc @nebojsa-db |
To fix which issue? |
|
@HyukjinKwon to fix the issue memtioned in pr description.. for example: I add this case in description to make it clear. |
|
Could you please ensure that the PR title does not sound like it's for refactoring if it's a bugfix? |
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala
Outdated
Show resolved
Hide resolved
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.
is it a simple rename? not sure why git diff doesn't detect it
a575672 to
a443354
Compare
...catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala
Outdated
Show resolved
Hide resolved
.../main/scala/org/apache/spark/sql/catalyst/optimizer/InsertMapSortInGroupingExpressions.scala
Outdated
Show resolved
Hide resolved
.../main/scala/org/apache/spark/sql/catalyst/optimizer/InsertMapSortInGroupingExpressions.scala
Outdated
Show resolved
Hide resolved
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.
why rename? It's indeed inserting MapSort
| exprToMapSort.getOrElseUpdate( | ||
| expr.canonicalized, Alias(inserted, "_groupingmapsort")()) | ||
| .toAttribute |
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.
| exprToMapSort.getOrElseUpdate( | |
| expr.canonicalized, Alias(inserted, "_groupingmapsort")()) | |
| .toAttribute | |
| exprToMapSort.getOrElseUpdate( | |
| expr.canonicalized, | |
| Alias(inserted, "_groupingmapsort")() | |
| ).toAttribute |
| ReplaceExpressions, | ||
| RewriteNonCorrelatedExists, | ||
| PullOutGroupingExpressions, | ||
| InsertMapSortInGroupingExpressions, |
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.
let's add some comments to explain the rule order reasoning.
|
thank you all, merged to master |
What changes were proposed in this pull request?
This pr reworks the group by map type to fix issues:
MapSortand we didi not transform the plan with new outputMapSortrule should be put beforePullOutGroupingExpressionsto avoid complex expr existing in grouping keysWhy are the changes needed?
To fix issues.
for example:
Does this PR introduce any user-facing change?
no, not released
How was this patch tested?
improve the tests to add more cases
Was this patch authored or co-authored using generative AI tooling?
no