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 Spark expression fuzzer test on deny_precision_loss functions #11526

Closed
wants to merge 1 commit into from

Conversation

rui-mo
Copy link
Collaborator

@rui-mo rui-mo commented Nov 13, 2024

Adds 'allowPrecisionLoss' parameter for decimal add/subtract/multiply/divide
argument generators. When false, the generators are used for
deny_precision_loss arithmetic functions.

Fixes: #11522.

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

netlify bot commented Nov 13, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 2d9c50f
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67355ab50bb1810008013717

@rui-mo
Copy link
Collaborator Author

rui-mo commented Nov 13, 2024

@kgpai @Yuhta Would you like to take a look on this fuzzer fix? Thanks.

@rui-mo
Copy link
Collaborator Author

rui-mo commented Nov 14, 2024

cc: @mbasmanova Thanks.


namespace facebook::velox::functions::sparksql::fuzzer {

class MultiplyDenyPrecisionLossArgGenerator
Copy link
Contributor

Choose a reason for hiding this comment

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

These names are very long. Would it make sense to fold this into MultiplyArgGenerator and add a boolean parameter to the ctor for denyPrecisionLoss?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your review. It makes sense to me. Updated this PR to add an 'allowPrecisionLoss' parameter for each generator.

Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

@rui-mo Thank you for the quick fix.

@mbasmanova mbasmanova 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 Nov 14, 2024
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@Yuhta merged this pull request in 5ac572b.

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.

Spark fuzzer failure: Cannot generate argument types for divide_deny_precision_loss
3 participants