-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 Spark array_append function #12043
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for meta-velox canceled.
|
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.
Thanks for your contribution. We can simply register this Presto function to Spark if their semantics are exactly the same. If any change on the implementation is needed for the reuse, we could move it to the 'velox/functions/lib' folder and make the necessary changes. Please also add documentation for the new registered Spark function.
4263b6c
to
dbdc5ab
Compare
@rui-mo Updated as your suggestion, can you review again? |
dbdc5ab
to
5ec415e
Compare
5ec415e
to
f404b3f
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.
Thanks!
@@ -15,6 +15,12 @@ Array Functions | |||
|
|||
SELECT array(1, 2, 3); -- [1,2,3] | |||
|
|||
.. spark:function:: array_append(array(E), value) -> array(E) | |||
|
|||
Add the element at the end of the array passed as first argument. :: |
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.
Perhaps also document the behavior when input array or value is null.
FOLLY_ALWAYS_INLINE bool callNullable( | ||
out_type<Array<Generic<T1>>>& out, | ||
const arg_type<Array<Generic<T1>>>* array, | ||
const arg_type<Generic<T1>>* element) { |
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.
Do we have test for element being null?
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: it would be nice to include Spark's implementation to the PR description.
Add Spark array_append function