-
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: null character not permitted in chr function #513
Conversation
@andygrove this is ready for review. |
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala
Outdated
Show resolved
Hide resolved
val table = "test0" | ||
withTable(table) { | ||
sql(s"create table $table(c9 int, c4 int) using parquet") | ||
sql(s"insert into $table values(0, 0), (66, null), (null, 70), (null, null)") |
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'd like to see a wider range of values being tested here such as large numbers and negative numbers
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.
Actually, ignore that. We have a separate issue related to large/negative numbers: #480
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala
Outdated
Show resolved
Hide resolved
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. I will plan on merging this early next week if there is no other feedback.
Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>
.iter() | ||
.map(|integer: Option<i64>| { | ||
integer | ||
.map(|integer| match core::char::from_u32(integer as u32) { |
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.
Spark Chr
expression has the behavior:
- If input < 0, return empty char.
- If input % 0xFF == 0, return
'\u0000'
. - Otherwise, return
input & 0xFF
as char.
Does core::char::from_u32
follow all these behaviors?
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.
Seems this is for the issue of null char, it is okay to fix it in follow up.
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.
Yes i am tracking that, #480 is the one I will work on next. That one handles the cases you mentioned above!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #513 +/- ##
============================================
+ Coverage 34.05% 34.37% +0.31%
+ Complexity 859 808 -51
============================================
Files 116 106 -10
Lines 38679 38548 -131
Branches 8567 8573 +6
============================================
+ Hits 13173 13250 +77
+ Misses 22745 22544 -201
+ Partials 2761 2754 -7 ☔ View full report in Codecov by Sentry. |
* adding chr null test * removing comments * repro bug * adding chr scalarfunc compatible with spark * remove ANSI configs, as it is not needed for chr * removing redundant test * Update core/src/execution/datafusion/expressions/scalar_funcs/chr.rs Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com> * adding support for scalar values * fix clippy errs * simplifying the scalar/array handling fn * fix fmt errors --------- Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>
Which issue does this PR close?
Closes #479 .
Rationale for this change
Improves compatibility with apache spark.
What changes are included in this PR?
Spark compatible implementation of chr scala func.
How are these changes tested?
New test case added to check null character. All test case pass.