Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

leoluan2009
Copy link
Contributor

Add Spark array_append function

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 9, 2025
Copy link

netlify bot commented Jan 9, 2025

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit f404b3f
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6790812c564f0400085f3758

Copy link
Collaborator

@rui-mo rui-mo left a 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.

@leoluan2009
Copy link
Contributor Author

@rui-mo Updated as your suggestion, can you review again?

Copy link
Collaborator

@rui-mo rui-mo left a 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. ::
Copy link
Collaborator

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) {
Copy link
Collaborator

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?

Copy link
Collaborator

@rui-mo rui-mo left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants