Parse nanoseconds for intervals #4186
Conversation
Jefffrey
force-pushed
the
3204_micro_nano_second_parsing
branch
from
6acff67
to
a4c0d4f
Compare
| ); | ||
| test_expression!( | ||
| "interval '0.49999 day'", | ||
| "0 years 0 mons 0 days 11 hours 59 mins 59.136 secs" | ||
| ); | ||
| test_expression!( | ||
| "interval '0.49999999999 day'", | ||
| "0 years 0 mons 0 days 12 hours 0 mins 0.00 secs" | ||
| "0 years 0 mons 0 days 11 hours 59 mins 59.999999136 secs" |
Co-authored-by: Ruihang Xia <waynestxia@gmail.com>
There was a problem hiding this comment.
thank you @Jefffrey
there's a bug while displaying IntervalMonthDayNano
❯ select interval '0.0001 second';
+----------------------------------------------------+
| IntervalMonthDayNano("100000") |
+----------------------------------------------------+
| 0 years 0 mons 0 days 0 hours 0 mins 0.100000 secs |
+----------------------------------------------------+
1 row in set. Query took 0.002 seconds.i checked the underline value it's correct (month: 0, day: 0, nanos: 100000)
i think this is the display issue
| |month_part: f64, mut day_part: f64, mut nanos_part: f64| -> (i64, i64, f64) { | ||
| // Convert fractional month to days, It's not supported by Arrow types, but anyway | ||
| day_part += (month_part - (month_part as i32) as f32) * 30_f32; | ||
| day_part += (month_part - (month_part as i64) as f64) * 30_f64; | ||
|
|
||
| // Convert fractional days to hours | ||
| milles_part += (day_part - ((day_part as i32) as f32)) | ||
| * 24_f32 | ||
| nanos_part += (day_part - ((day_part as i64) as f64)) | ||
| * 24_f64 | ||
| * SECONDS_PER_HOUR | ||
| * MILLIS_PER_SECOND; | ||
| * NANOS_PER_SECOND; | ||
|
|
||
| (month_part as i32, day_part as i32, milles_part) | ||
| (month_part as i64, day_part as i64, nanos_part) |
Issue with arrow-rs, has already been addressed: apache/arrow-rs#3093 |
|
while we have leading zeros after decimal, it displays incorrectly ❯ select interval '0.01 second';
+------------------------------------------------+
| IntervalDayTime("10") |
+------------------------------------------------+
| 0 years 0 mons 0 days 0 hours 0 mins 0.10 secs |
+------------------------------------------------+
1 row in set. Query took 0.000 seconds.
❯ select interval '0.001 second';
+------------------------------------------------+
| IntervalDayTime("1") |
+------------------------------------------------+
| 0 years 0 mons 0 days 0 hours 0 mins 0.01 secs |
+------------------------------------------------+
1 row in set. Query took 0.000 seconds. |
| ); | ||
| test_expression!( | ||
| "interval '0.49999 day'", | ||
| "0 years 0 mons 0 days 11 hours 59 mins 59.136 secs" | ||
| ); | ||
| test_expression!( | ||
| "interval '0.49999999999 day'", | ||
| "0 years 0 mons 0 days 12 hours 0 mins 0.00 secs" | ||
| "0 years 0 mons 0 days 11 hours 59 mins 59.999999136 secs" | ||
| ); | ||
| test_expression!( |
There was a problem hiding this comment.
i think we can add a test case for this (which will fail in the current version) and wait for the update to arrow 27 #4199
| test_expression!( | |
| test_expression!( | |
| "interval '0.00000000001 day'", | |
| "0 years 0 mons 0 days 0 hours 0 mins 0.000000864 secs" | |
| ); | |
| test_expression!( |
There was a problem hiding this comment.
i just found that the arrow-rs fix isn't included in 27.0.0
@tustvold do you have any suggestion for this? should we just merge this first and add some test cases after arrow 28.0.0 merged?
There was a problem hiding this comment.
We can also add test cases that are marked as #[should_fail]
|
Thanks @Jefffrey for the fix and @waitingkuo and @waynexia for the reviews! |
|
Benchmark runs are scheduled for baseline = d91ce06 and contender = 8bfc0ea. 8bfc0ea is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3204.
Rationale for this change
Change parsing granularity of intervals from milliseconds to nanoseconds, to support passing fractions of milliseconds/seconds (down to nanoseconds) as
IntervalMonthDayNanosupports granularity down to nanoseconds.What changes are included in this PR?
Accumulating with nanoseconds instead of milliseconds, changing to f64/i128 where necessary to account for shift in range of values.
Are these changes tested?
Added some unit tests, and fixed others as there is now more precision for seconds fractions.
Are there any user-facing changes?