Update IntervalMonthDayNanoType::make_value() to conform to specifications
#2235
Conversation
|
@alamb I could use a second set of eyes to make sure it is correct this time. Sincere apologies for merging non-conforming code. |
Codecov Report
@@ Coverage Diff @@
## master #2235 +/- ##
=======================================
Coverage 82.31% 82.31%
=======================================
Files 240 241 +1
Lines 62445 62455 +10
=======================================
+ Hits 51400 51410 +10
Misses 11045 11045
Help us with your feedback. Take ten seconds to tell us how you rate us. |
There was a problem hiding this comment.
Thanks @avantgardnerio -- the pictures are 😍
Can you please add some unit tests (maybe showing that values make it through roundtrip)?
LIke
let value = IntervalMonthDayNanoType::make_value(1, 2, 3);
assert_eq!(IntervalMonthDayNanoType::to_parts(value), (1,2,3));I think that type of test would would have caught this bug in the first place as well
| @@ -232,6 +232,16 @@ impl IntervalDayTimeType { | |||
| days: i32, | |||
| millis: i32, | |||
| ) -> <IntervalDayTimeType as ArrowPrimitiveType>::Native { | |||
| /* | |||
| let months = (i >> 96) as i32; | ||
| let days = (i >> 64) as i32; | ||
| let nanos = i as i64; |
There was a problem hiding this comment.
Re-reading this now with your great pictures, I was wondering if we had to mask off the remaining bits in the other fields -- otherwise the days will also include the months
However, after some thought, I think the truncation done via as i32 and as i64 should be good enough
I did, and they indeed caught another bug. I was failing to mask the nanos during |
Co-authored-by: Liang-Chi Hsieh <[email protected]>
|
Benchmark runs are scheduled for baseline = 2c09ba4 and contender = d4f038a. d4f038a is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
This implementation was aligned with https://github.com/cube-js/arrow-datafusion/blob/370f91fda4a7e9387dded4edaf2425c447e76fbd/datafusion/physical-expr/src/expressions/binary_distinct.rs#L306-L353 Can drop this after rebase on commit d4f038a " Update IntervalMonthDayNanoType::make_value() to conform to specifications (apache#2235)", first released in 20.0.0
This implementation was aligned with https://github.com/cube-js/arrow-datafusion/blob/370f91fda4a7e9387dded4edaf2425c447e76fbd/datafusion/physical-expr/src/expressions/binary_distinct.rs#L306-L353 Can drop this after rebase on commit d4f038a " Update IntervalMonthDayNanoType::make_value() to conform to specifications (apache#2235)", first released in 20.0.0
This implementation was aligned with https://github.com/cube-js/arrow-datafusion/blob/370f91fda4a7e9387dded4edaf2425c447e76fbd/datafusion/physical-expr/src/expressions/binary_distinct.rs#L306-L353 Can drop this after rebase on commit d4f038a " Update IntervalMonthDayNanoType::make_value() to conform to specifications (apache#2235)", first released in 20.0.0
Which issue does this PR close?
Closes #2234.
Rationale for this change
Fix bug that will cause users to generate incorrect interval values.
What changes are included in this PR?
Revised implementation of
IntervalMonthDayNanoType::make_value()and better inline documentation.Are there any user-facing changes?
Erroneous durations will be fixed.