fix: null character not permitted in chr function #513
Conversation
vaibhawvipul
changed the title
|
@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.
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.
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.
LGTM. Thanks @vaibhawvipul. I will plan on merging this early next week if there is no other feedback.
kazuyukitanimura
changed the title
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.
Spark Chr expression has the behavior:
- If input < 0, return empty char.
- If input % 0xFF == 0, return
'\u0000'. - Otherwise, return
input & 0xFFas char.
Does core::char::from_u32 follow all these behaviors?
There was a problem hiding this comment.
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.
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.