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

Add Spark raise_error function #10110

Closed

Conversation

gaoyangxiaozhu
Copy link
Contributor

@gaoyangxiaozhu gaoyangxiaozhu commented Jun 7, 2024

This PR adds Spark raise_error function. It is re-used in Spark assert_true
function, so after this PR assert_true can also be supported.

Spark document: https://spark.apache.org/docs/latest/api/sql/#raise_error
Spark code: https://github.com/apache/spark/blob/branch-3.5/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala#L85C1-L92C1
Issue: apache/incubator-gluten#5991

@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 Jun 7, 2024
Copy link

netlify bot commented Jun 7, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit bc6b380
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/667e97e90817a60008bc9ed3

@gaoyangxiaozhu
Copy link
Contributor Author

@rui-mo / @PHILO-HE help review

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/misc.rst Outdated Show resolved Hide resolved
velox/docs/functions/spark/misc.rst Outdated Show resolved Hide resolved
velox/functions/sparksql/RaiseError.h Outdated Show resolved Hide resolved
velox/functions/sparksql/RaiseError.h Outdated Show resolved Hide resolved
velox/functions/sparksql/RaiseError.h Outdated Show resolved Hide resolved
velox/functions/sparksql/tests/RaiseErrorTest.cpp Outdated Show resolved Hide resolved
@gaoyangxiaozhu gaoyangxiaozhu requested a review from rui-mo June 12, 2024 07:00
@gaoyangxiaozhu
Copy link
Contributor Author

ping @rui-mo

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/misc.rst Outdated Show resolved Hide resolved
velox/functions/sparksql/RaiseError.h Outdated Show resolved Hide resolved
velox/docs/functions/spark/misc.rst Outdated Show resolved Hide resolved
velox/docs/functions/spark/misc.rst Outdated Show resolved Hide resolved
@gaoyangxiaozhu
Copy link
Contributor Author

I need @rui-mo for some input, @PHILO-HE please ignore my review refresh request before i update the code

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/functions/sparksql/RaiseError.h Outdated Show resolved Hide resolved
@gaoyangxiaozhu gaoyangxiaozhu requested a review from rui-mo June 17, 2024 06:31
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.

Could you run the fuzzer test for this function to make sure it works? Thanks.

velox/functions/sparksql/RaiseError.h Outdated Show resolved Hide resolved
velox/functions/sparksql/RaiseError.h Outdated Show resolved Hide resolved
@gaoyangxiaozhu
Copy link
Contributor Author

Could you run the fuzzer test for this function to make sure it works? Thanks.

Could you run the fuzzer test for this function to make sure it works? Thanks.

Could you run the fuzzer test for this function to make sure it works? Thanks.

I have verified locally end to end with gluten, @rui-mo what do you means for run fuzzer test for this function , does i need add a new fuzzer test or something , any reference ?

@gaoyangxiaozhu gaoyangxiaozhu requested a review from rui-mo June 17, 2024 12:12
@rui-mo
Copy link
Collaborator

rui-mo commented Jun 17, 2024

@gaoyangxiaozhu The command is like below, and you could use --only raise_error to test this function only.
https://github.com/facebookincubator/velox/blob/main/.github/workflows/scheduled.yml#L508-L512

@gaoyangxiaozhu
Copy link
Contributor Author

--only raise_error

got! so it should be fine , a example local fuzzer run

image

@gaoyangxiaozhu
Copy link
Contributor Author

@rui-mo any new comments ?

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/misc.rst Outdated Show resolved Hide resolved
velox/docs/functions/spark/misc.rst Outdated Show resolved Hide resolved
velox/functions/sparksql/tests/RaiseErrorTest.cpp Outdated Show resolved Hide resolved
velox/functions/sparksql/RaiseError.h Outdated Show resolved Hide resolved
@gaoyangxiaozhu gaoyangxiaozhu requested a review from rui-mo June 19, 2024 10:37
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 you.

velox/docs/functions/spark/misc.rst Outdated Show resolved Hide resolved
velox/expression/fuzzer/SparkExpressionFuzzerTest.cpp Outdated Show resolved Hide resolved
velox/functions/sparksql/RaiseError.h Outdated Show resolved Hide resolved
@gaoyangxiaozhu gaoyangxiaozhu requested a review from rui-mo June 19, 2024 13:46
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.

velox/functions/sparksql/tests/RaiseErrorTest.cpp Outdated Show resolved Hide resolved
@rui-mo
Copy link
Collaborator

rui-mo commented Jun 20, 2024

@gaoyangxiaozhu Could you try the fuzzer test again to make sure it works? Thanks.

@gaoyangxiaozhu
Copy link
Contributor Author

@gaoyangxiaozhu Could you try the fuzzer test again to make sure it works? Thanks.
done.

image

@gaoyangxiaozhu gaoyangxiaozhu requested a review from rui-mo June 20, 2024 12:19
@gaoyangxiaozhu
Copy link
Contributor Author

ping @rui-mo / @PHILO-HE for follow up if has any new comments ?

@rui-mo
Copy link
Collaborator

rui-mo commented Jun 25, 2024

@gaoyangxiaozhu I assume we need to rebase to #10274 and returns status in raise_error function after it gets merged. Thanks.

@gaoyangxiaozhu
Copy link
Contributor Author

@gaoyangxiaozhu I assume we need to rebase to #10274 and returns status in raise_error function after it gets merged. Thanks.

sure, so could you help sign the PR or we need people from meta to review and help sign ?

@rui-mo
Copy link
Collaborator

rui-mo commented Jun 26, 2024

@gaoyangxiaozhu Will continue to review this PR after the code is rebased to #10274. Thanks.

@rui-mo
Copy link
Collaborator

rui-mo commented Jun 28, 2024

@gaoyangxiaozhu #10274 has been merged. Could you rebase to it and update the code? Thanks.

@gaoyangxiaozhu
Copy link
Contributor Author

@gaoyangxiaozhu #10274 has been merged. Could you rebase to it and update the code? Thanks.

Done, and also local test with fuzze test. should be fine. @rui-mo

Copy link
Contributor

@PHILO-HE PHILO-HE left a comment

Choose a reason for hiding this comment

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

I have no other comment.

@gaoyangxiaozhu gaoyangxiaozhu requested a review from rui-mo June 28, 2024 11:01
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.

@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 Jul 1, 2024
@rui-mo rui-mo changed the title Spark raise_error function support Add Spark raise_error function Jul 1, 2024
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@bikramSingh91 merged this pull request in 4e39b06.

Copy link

Conbench analyzed the 1 benchmark run on commit 4e39b06c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

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.

4 participants