fix date_bin overflows scaling extreme Timestamp(Second) source#22315
fix date_bin overflows scaling extreme Timestamp(Second) source#22315xiedeyantu wants to merge 4 commits into
Conversation
|
|
||
| stride_fn(stride, scaled, origin) | ||
| .map(|binned| binned / scale) | ||
| .map_err(|e| { |
There was a problem hiding this comment.
ArrowError::ComputeError("Execution error: DATE_BIN source timestamp...") gets wrapped a second time into DataFusionError::Execution(...) which i htink will produce the triple nested message.
| res.err().unwrap().strip_backtrace(), | ||
| "This feature is not implemented: DATE_BIN only supports literal values for the origin argument, not arrays" | ||
| ); | ||
|
|
There was a problem hiding this comment.
Could you add simple test for the array path?
just like:
let input = TimestampSecondArray::from(vec![Some(i64::MAX)]);
let args = vec![
ColumnarValue::Scalar(ScalarValue::IntervalMonthDayNano(...)),
ColumnarValue::Array(Arc::new(input)),
ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(Some(0), None)),
];
let res = invoke_date_bin_with_args(args, 1, return_field);
assert!(res.err().unwrap().strip_backtrace().contains(
"DATE_BIN source timestamp 9223372036854775807 cannot be represented in nanoseconds"
));related to first comment because this might help clear up what you expect the result to look like
|
@nathanb9 Thanks for your review. All comments have been revised according to your suggestions. Please confirm if this is what you expected. |
nathanb9
left a comment
There was a problem hiding this comment.
I think this is better way to handle it too @xiedeyantu Thanks for fixing quickly.
FYI im not a committer so youll need someone else as well.
Thanks all the same. |
Which issue does this PR close?
Rationale for this change
date_bincould panic during planning or constant evaluation when scaling a non-nanosecond source timestamp to nanoseconds overflowed. This change makes that path return a regular error instead of panicking.What changes are included in this PR?
date_bin.NULLbehavior for unrelated out-of-rangedate_bincases.Are these changes tested?
Yes.
Verified with:
cargo test -p datafusion-functions test_date_bin --libcargo test -p datafusion-sqllogictest --test sqllogictests date_bin_errorsAre there any user-facing changes?
Yes. Queries that previously could panic now return a normal error:
Execution error: DATE_BIN source timestamp ... cannot be represented in nanoseconds