-
Notifications
You must be signed in to change notification settings - Fork 156
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: requested character too large for encoding in chr function #552
Conversation
…256, also added test case
@andygrove this is ready for review. |
Co-authored-by: Andy Grove <andygrove73@gmail.com>
Co-authored-by: Andy Grove <andygrove73@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @vaibhawvipul
None => { | ||
exec_err!("requested character too large for encoding.") | ||
.map(|integer| { | ||
if integer < 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andygrove can we use unlikely
(requires nightly) to indicate a less likely branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we are moving to stable?
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala
Outdated
Show resolved
Hide resolved
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala
Outdated
Show resolved
Hide resolved
Co-authored-by: Andy Grove <andygrove73@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vaibhawvipul. I think this addresses the feedback from @viirya so I will plan on merging this soon if there is no other feedback.
Thanks @vaibhawvipul |
…he#552) * adding support for negative integers and modulo checks for int above 256, also added test case * fix test case to avoid cast integer overflow error * Update core/src/execution/datafusion/expressions/scalar_funcs/chr.rs Co-authored-by: Andy Grove <andygrove73@gmail.com> * returning negative ints as empty string earlier * adding scalar test and removing unreachable code * Update core/src/execution/datafusion/expressions/scalar_funcs/chr.rs Co-authored-by: Andy Grove <andygrove73@gmail.com> * Update spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala Co-authored-by: Andy Grove <andygrove73@gmail.com> * add a test case where integer & 0xFF == 0 --------- Co-authored-by: Andy Grove <andygrove73@gmail.com>
Which issue does this PR close?
Closes #480 .
Rationale for this change
improves compatibility with spark
What changes are included in this PR?
How are these changes tested?
Test cases added, all the existing tests pass.