-
Notifications
You must be signed in to change notification settings - Fork 307
fix: Sort on single struct should fallback to Spark #811
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2501,6 +2501,13 @@ object QueryPlanSerde extends Logging with ShimQueryPlanSerde with CometExprShim | |
|
|
||
| case SortExec(sortOrder, _, child, _) | ||
| if isCometOperatorEnabled(op.conf, CometConf.OPERATOR_SORT) => | ||
| // TODO: Remove this constraint when we upgrade to new arrow-rs including | ||
| // https://github.com/apache/arrow-rs/pull/6225 | ||
| if (child.output.length == 1 && child.output.head.dataType.isInstanceOf[StructType]) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we add support for other types, do we need to update this to make it recursive so that we check for Map or Array containing struct?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me add more data types here according to arrow-rs. |
||
| withInfo(op, "Sort on single struct column is not supported") | ||
| return None | ||
| } | ||
|
|
||
| val sortOrders = sortOrder.map(exprToProto(_, child.output)) | ||
|
|
||
| if (sortOrders.forall(_.isDefined) && childOp.nonEmpty) { | ||
|
|
||
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.
nit: Shall we use ` instead of '
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.
This is not changed by this PR. I think there is previous PR changing it, but didn't update the document.
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.
The document is updated automatically when
make releaselocally.