fix: Remove wrong calculation for Murmur3Hash for float with null input #245
Conversation
advancedxy
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #245 +/- ##
=========================================
Coverage 33.58% 33.58%
Complexity 780 780
=========================================
Files 107 107
Lines 37212 37212
Branches 8161 8161
=========================================
+ Hits 12496 12497 +1
+ Misses 22076 22075 -1
Partials 2640 2640 ☔ View full report in Codecov by Sentry. |
| // this as well | ||
| let expected = vec![3286402344, 2486176763, 142593372, 885025535, 2395000894]; | ||
| assert_eq!(hashes, expected); | ||
| test_primitive!( |
There was a problem hiding this comment.
string is not primitive. Maybe rename the macro as test_hashes.
There was a problem hiding this comment.
Hmm, I can rename the macro to test_hashes since we have not add tests for complex types yet.
However, I think we may have different view about the definition of primitive type. Per my understanding, types other than complex types(Array, Map, Struct and its nested ones) are considered primitive type. Spark's parser also consider String as a primitive type, see https://github.com/apache/spark/blob/4d9dbb35aacb6bd8ca1e5a6dff5076034b5a042b/sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala#L61.
There was a problem hiding this comment.
Yea, for that I mean in arrow-rs, string is not primitive.
advancedxy
changed the title
|
Merged. Thanks. |
|
Thanks for the reviewing. |
…ut (apache#245) * fix: Remove extra & wrong calculation for Murmur3Hash with float with null input * chore: address comments * address comments
3 participants
Which issue does this PR close?
Closes #.
Rationale for this change
While developing #226, I noticed the rust implementation of Murmur3Hash doesn't handle float with null input correctly.
This PR fixes that correctness issue.
What changes are included in this PR?
hash_array_primitive_floatmacroHow are these changes tested?
Updated test