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

@leoluan2009 leoluan2009 commented Jan 9, 2025

@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 e15fbeb
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67ade59cec243c0008c6940c

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!

velox/docs/functions/spark/array.rst Outdated Show resolved Hide resolved
velox/functions/sparksql/ArrayAppend.h Show resolved Hide resolved
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.

@leoluan2009
Copy link
Contributor Author

@rui-mo updated, can you review again? thanks

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.

velox/docs/functions/spark/array.rst Outdated Show resolved Hide resolved
velox/docs/functions/spark/array.rst Outdated Show resolved Hide resolved
velox/functions/sparksql/ArrayAppend.h Show resolved Hide resolved
velox/docs/functions/spark/array.rst Outdated Show resolved Hide resolved
velox/docs/functions/spark/array.rst Outdated Show resolved Hide resolved
velox/docs/functions/spark/array.rst Outdated Show resolved Hide resolved
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. Looks good overall.

@@ -15,6 +15,15 @@ Array Functions

SELECT array(1, 2, 3); -- [1,2,3]

.. spark:function:: array_append(array(E), element) -> 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.

nit: array passed as first argument -> input array

.. spark:function:: array_append(array(E), element) -> array(E)
Add the ``element`` at the end of the ``array`` passed as first argument.
Type of element should be same to type of the elements of the ``array``.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use reference to element.

.. spark:function:: array_append(array(E), element) -> array(E)
Add the ``element`` at the end of the ``array`` passed as first argument.
Type of element should be same to type of the elements of the ``array``.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: same -> the same
type of the elements of the array -> the type of elements in the array

Add the ``element`` at the end of the ``array`` passed as first argument.
Type of element should be same to type of the elements of the ``array``.
NULL element is also appended into the array. But if the ``array`` passed is NULL output is NULL. ::
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if the array passed is NULL output is NULL.

Comment suggestion: Returns NULL when the input ``array`` is NULL.

{7, 8, 9, 33},
{10, 20, std::nullopt, std::nullopt},
});
testExpression(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you add a test when the input array and element are both null?

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