-
Notifications
You must be signed in to change notification settings - Fork 155
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
feat: Add support for CreateNamedStruct #620
Conversation
b232326
to
5cf3264
Compare
.setCreateNamedStruct(structBuilder) | ||
.build()) | ||
} else { | ||
withInfo(expr, struct.valExprs: _*) |
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.
I think we need to specify an info string here otherwise the user will not see the reason for falling back to Spark (perhaps @parthchandra can confirm).
withInfo(expr, struct.valExprs: _*) | |
withInfo(expr, "unsupported arguments for CreateNamedStruct", struct.valExprs: _*) |
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.
Based on this comment here
datafusion-comet/spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala
Lines 1085 to 1086 in 7d6ad4b
* Information text. Optional, may be null or empty. If not provided, then only information | |
* from child nodes will be included. |
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.
I think we need to specify an info string here otherwise the user will not see the reason for falling back to Spark (perhaps @parthchandra can confirm).
The user will get the reason from the child nodes. We could add the additional message as @andygrove has suggested, but it is not strictly necessary.
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.
Resolved, by adding the message. I think there is some argument to made around this making it harder to figure out the root cause since there is more information in the info log.
7d6ad4b
to
9463dde
Compare
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala
Outdated
Show resolved
Hide resolved
9463dde
to
350e8d7
Compare
350e8d7
to
9a71e40
Compare
9a71e40
to
45d913c
Compare
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.
LGTM. Thanks @eejbyfeldt
Thanks @eejbyfeldt |
* Add support for CreateNamedStruct * Exclude HashJoins using struct keys as this is currently unsupported in datafusion * Add CreateNamedStruct to docs * Add message
Which issue does this PR close?
Closes #619.
Rationale for this change
Add support for one more expression.
What changes are included in this PR?
Adds a custom
PhysicalExpr
that implements the behavior of the CreateNamedStruct in spark.How are these changes tested?
New unit tests in the CometExpressionSuite.