Repository navigation
fix: handle out of range errors in DATE_BIN instead of panicking - #20221
Conversation
neilconway
left a comment
neilconway
left a comment
There was a problem hiding this comment.
Thanks for looking at this! Can we add some test cases?
Added test cases for both out-of-range scenarios in the latest commit. |
| stride: i64, | ||
| stride_fn: fn(i64, i64, i64) -> i64, | ||
| ) -> impl Fn(i64) -> i64 { | ||
| stride_fn: fn(i64, i64, i64) -> Result<i64>, |
There was a problem hiding this comment.
nit: Use BinFunction here?
There was a problem hiding this comment.
Updated to use BinFunction alias. Thanks for the review.
|
@alamb @avantgardnerio When you get a chance, could you take a look at this PR? All feedback has been addressed, checks are passing, and it's ready for merge. Thanks! |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @mishop-15
Can you please add some .slt tests that show the problem? I think that will be less verbose than using unit tests and will make sure everything is hooked up correctly
While adding .slt tests, can you also please include a few other tests such as
select date_bin(interval '1 month', to_timestamp_millis(-1040292460), timestamp '1984-01-07 00:00:00');
-- large strides to force overflows
select date_bin(
interval '1637426858 months',
timestamp '1969-12-31 00:00:00',
timestamp '1984-01-07 00:00:00'
);
select date_bin(
interval '1637426858 months',
to_timestamp_millis(-1040292000),
timestamp '1984-01-07 00:00:00'
) as b;
select date_bin(
interval '1637426858 months',
to_timestamp_millis(-2208988800000), -- 1900-01-01
timestamp '1984-01-07 00:00:00'
) as b;| let array: PrimitiveArray<Time64MicrosecondType> = | ||
| array.unary(apply_stride_fn); | ||
| ColumnarValue::Array(Arc::new(array)) | ||
| let result: PrimitiveArray<Time64MicrosecondType> = array |
There was a problem hiding this comment.
How about using try_unary? https://docs.rs/arrow/latest/arrow/array/struct.PrimitiveArray.html#method.try_unary
I think that will result in a smaller diff and be faster as well
The same comment applies to the other methods in this function
There was a problem hiding this comment.
Switched to try_unary, thanks for pointing that out!
| const NANOS_PER_MILLI: i64 = 1_000_000; | ||
| const NANOS_PER_SEC: i64 = NANOSECONDS; | ||
|
|
||
| type BinFunction = fn(i64, i64, i64) -> Result<i64>; |
There was a problem hiding this comment.
Can you please add comments here aout what BinFunction is and what it represents (e.g. what are the aguments and what does it return)?
There was a problem hiding this comment.
Added comments explaining the arguments and return value.
|
Thank you @mishop-15 and @mhilton and @neilconway |
…che#20221) ## Which issue does this PR close? Closes apache#20219 ## Rationale for this change The DATE_BIN function was panicking when datetime operations went out of range instead of returning proper errors. The two specific cases were: 1. Month subtraction going out of range causing `DateTime - Months` panic 2. `timestamp_nanos_opt()` returning None and then unwrapping ## What changes are included in this PR? - Changed `date_bin_months_interval` and `to_utc_date_time` to return `Result` instead of panicking - Replaced `origin_date - Months` and `origin_date + Months` with `checked_sub_months` and `checked_add_months` - Replaced `.unwrap()` calls with proper `match` statements and error handling - Updated all callers throughout the file to handle `Result` types ## Are these changes tested? Tested manually with the exact queries from the issue that were panicking: ```sql select DATE_BIN('1637426858', TO_TIMESTAMP_MILLIS(1040292460), TIMESTAMP '1984-01-07 00:00:00'); select DATE_BIN('1637426858', TO_TIMESTAMP_MILLIS(-1040292460), TIMESTAMP '1984-01-07 00:00:00'); ``` Both queries now return NULL instead of panicking. All existing unit tests pass. ## Are there any user-facing changes? Yes - queries with DATE_BIN that would previously panic now return NULL when datetime operations go out of range.
Which issue does this PR close?
Closes #20219
Rationale for this change
The DATE_BIN function was panicking when datetime operations went out of range instead of returning proper errors. The two specific cases were:
DateTime - Monthspanictimestamp_nanos_opt()returning None and then unwrappingWhat changes are included in this PR?
date_bin_months_intervalandto_utc_date_timeto returnResultinstead of panickingorigin_date - Monthsandorigin_date + Monthswithchecked_sub_monthsandchecked_add_months.unwrap()calls with propermatchstatements and error handlingResulttypesAre these changes tested?
Tested manually with the exact queries from the issue that were panicking:
Both queries now return NULL instead of panicking. All existing unit tests pass.
Are there any user-facing changes?
Yes - queries with DATE_BIN that would previously panic now return NULL when datetime operations go out of range.