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

fix: Returns null for invalid inputs in Spark make_date function #11950

Closed

Conversation

zhli1142015
Copy link
Contributor

@zhli1142015 zhli1142015 commented Dec 24, 2024

Spark make_date function used to throw error for invalid inputs. This PR
changes to return null to conform to Spark ANSI off mode.

@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 Dec 24, 2024
@zhli1142015
Copy link
Contributor Author

cc @rui-mo and @liujiayi771 , thanks

Copy link

netlify bot commented Dec 24, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 9a481b5
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67874270dfe9540008b1af5e

velox/docs/functions/spark/datetime.rst Outdated Show resolved Hide resolved
velox/functions/sparksql/DateTimeFunctions.h Outdated Show resolved Hide resolved
@zhli1142015 zhli1142015 requested a review from rui-mo December 24, 2024 11:21
@rui-mo rui-mo changed the title fix: Returns null if make_date Spark function has invalid inputs fix: Returns null for invalid inputs in Spark make_date function Dec 27, 2024
@rui-mo rui-mo added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Dec 27, 2024
@liujiayi771
Copy link
Contributor

@zhli1142015 LGTM, thanks.

@zhli1142015 zhli1142015 force-pushed the make_date_null branch 4 times, most recently from 8dc1f24 to fd6b765 Compare January 9, 2025 02:32
@facebook-github-bot
Copy link
Contributor

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@kagamiori
Copy link
Contributor

Hi @zhli1142015, could you please rebase the PR onto the latest main? It's needed for merging. Thanks!

@zhli1142015
Copy link
Contributor Author

Hi @zhli1142015, could you please rebase the PR onto the latest main? It's needed for merging. Thanks!

Thank you for your help with the merge. The PR has been rebased.

@facebook-github-bot
Copy link
Contributor

@kagamiori has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@kagamiori merged this pull request in 1231ca4.

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. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants