You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "xiedeyantu (via GitHub)" <gi...@apache.org> on 2026/05/17 16:19:16 UTC
[PR] fix date_bin overflows scaling extreme Timestamp(Second) source [datafusion]
xiedeyantu opened a new pull request, #22315:
URL: https://github.com/apache/datafusion/pull/22315
## Which issue does this PR close?
- Closes #22211.
## Rationale for this change
`date_bin` could 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?
- Added checked overflow handling for source timestamp scaling in `date_bin`.
- Return an error for out-of-range source timestamp conversion instead of panicking.
- Preserved existing `NULL` behavior for unrelated out-of-range `date_bin` cases.
- Added Rust unit test and sqllogictest coverage for the overflow case.
## Are these changes tested?
Yes.
Verified with:
- `cargo test -p datafusion-functions test_date_bin --lib`
- `cargo test -p datafusion-sqllogictest --test sqllogictests date_bin_errors`
## Are 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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix date_bin overflows scaling extreme Timestamp(Second) source [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey merged PR #22315:
URL: https://github.com/apache/datafusion/pull/22315
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix date_bin overflows scaling extreme Timestamp(Second) source [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #22315:
URL: https://github.com/apache/datafusion/pull/22315#issuecomment-4621062674
thanks @xiedeyantu & @nathanb9
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix date_bin overflows scaling extreme Timestamp(Second) source [datafusion]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #22315:
URL: https://github.com/apache/datafusion/pull/22315#issuecomment-4544441900
@Jefffrey I tried to fix it, but I'm not sure if it's what you want. Could you take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix date_bin overflows scaling extreme Timestamp(Second) source [datafusion]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #22315:
URL: https://github.com/apache/datafusion/pull/22315#issuecomment-4515791075
@Dandandan Could you help me take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix date_bin overflows scaling extreme Timestamp(Second) source [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #22315:
URL: https://github.com/apache/datafusion/pull/22315#discussion_r3329719174
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -710,20 +765,26 @@ fn date_bin_impl(
T: ArrowTimestampType,
{
let array = as_primitive_array::<T>(array)?;
- let scale = match T::UNIT {
- Nanosecond => 1,
- Microsecond => NANOS_PER_MICRO,
- Millisecond => NANOS_PER_MILLI,
- Second => NANOSECONDS,
- };
-
- let result: PrimitiveArray<T> = array.try_unary(|val| {
- stride_fn(stride, val * scale, origin)
- .map(|binned| binned / scale)
- .map_err(|e| {
- arrow::error::ArrowError::ComputeError(e.to_string())
- })
- })?;
+ let scale = timestamp_scale::<T>();
+
+ let values = array
+ .iter()
+ .map(|val| match val {
+ Some(val) => {
+ let scaled = val.checked_mul(scale).ok_or_else(|| {
+ DataFusionError::Execution(
+ timestamp_scale_overflow_message(val),
+ )
+ })?;
+ Ok(stride_fn(stride, scaled, origin)
+ .ok()
Review Comment:
Thanks @Jefffrey I will wait for this to get merged as I am doing some cleanup too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix date_bin overflows scaling extreme Timestamp(Second) source [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #22315:
URL: https://github.com/apache/datafusion/pull/22315#discussion_r3303749711
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -575,53 +578,85 @@ fn date_bin_impl(
return exec_err!("DATE_BIN stride must be non-zero");
}
- fn stride_map_fn<T: ArrowTimestampType>(
- origin: i64,
- stride: i64,
- stride_fn: BinFunction,
- ) -> impl Fn(i64) -> Result<i64> {
- let scale = match T::UNIT {
+ fn timestamp_scale<T: ArrowTimestampType>() -> i64 {
+ match T::UNIT {
Nanosecond => 1,
Microsecond => NANOS_PER_MICRO,
Millisecond => NANOS_PER_MILLI,
Second => NANOSECONDS,
- };
- move |x: i64| match stride_fn(stride, x * scale, origin) {
- Ok(result) => Ok(result / scale),
- Err(e) => Err(e),
}
}
+ fn scale_timestamp_to_nanos(x: i64, scale: i64) -> Result<i64> {
Review Comment:
Maybe its better to inline this (and just have the error message as the abstraction) since forcing a DataFusion error here leads to some quirks like needing to convert to arrow (before converting back to datafusion error)
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -575,53 +578,85 @@ fn date_bin_impl(
return exec_err!("DATE_BIN stride must be non-zero");
}
- fn stride_map_fn<T: ArrowTimestampType>(
- origin: i64,
- stride: i64,
- stride_fn: BinFunction,
- ) -> impl Fn(i64) -> Result<i64> {
- let scale = match T::UNIT {
+ fn timestamp_scale<T: ArrowTimestampType>() -> i64 {
+ match T::UNIT {
Nanosecond => 1,
Microsecond => NANOS_PER_MICRO,
Millisecond => NANOS_PER_MILLI,
Second => NANOSECONDS,
- };
- move |x: i64| match stride_fn(stride, x * scale, origin) {
- Ok(result) => Ok(result / scale),
- Err(e) => Err(e),
}
}
+ fn scale_timestamp_to_nanos(x: i64, scale: i64) -> Result<i64> {
+ x.checked_mul(scale).ok_or_else(|| {
+ DataFusionError::Execution(format!(
+ "DATE_BIN source timestamp {x} cannot be represented in nanoseconds"
+ ))
+ })
+ }
+
Ok(match array {
ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(v, tz_opt)) => {
- let apply_stride_fn =
- stride_map_fn::<TimestampNanosecondType>(origin, stride, stride_fn);
+ let scale = timestamp_scale::<TimestampNanosecondType>();
ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
- v.and_then(|val| apply_stride_fn(val).ok()),
+ match *v {
+ Some(val) => {
+ let scaled = scale_timestamp_to_nanos(val, scale)?;
+ match stride_fn(stride, scaled, origin) {
+ Ok(result) => Some(result / scale),
+ Err(_) => None,
+ }
Review Comment:
I find it a little confusing we surface error for the overflow, but other potential errors are masked via null values 🤔
I raised an issue since I see we are already a bit inconsistent with this:
- https://github.com/apache/datafusion/issues/22528
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -1066,6 +1101,54 @@ mod tests {
res.err().unwrap().strip_backtrace(),
"This feature is not implemented: DATE_BIN only supports literal values for the origin argument, not arrays"
);
+
+ // source: overflow while scaling to nanoseconds
+ args = vec![
+ ColumnarValue::Scalar(ScalarValue::IntervalMonthDayNano(Some(
+ IntervalMonthDayNano {
+ months: 0,
+ days: 0,
+ nanoseconds: 1,
+ },
+ ))),
+ ColumnarValue::Scalar(ScalarValue::TimestampSecond(Some(i64::MAX), None)),
+ ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(Some(0), None)),
+ ];
+ let res = invoke_date_bin_with_args(args, 1, return_field);
+ assert_eq!(
+ res.err().unwrap().strip_backtrace(),
+ "Execution error: DATE_BIN source timestamp 9223372036854775807 cannot be represented in nanoseconds"
+ );
+
+ // source: overflow while scaling to nanoseconds (array path)
+ let input = Arc::new(
+ vec![Some(i64::MAX)]
+ .into_iter()
+ .collect::<TimestampSecondArray>(),
+ );
+ let return_field_second = &Arc::new(Field::new(
+ "f",
+ DataType::Timestamp(TimeUnit::Second, None),
+ true,
+ ));
+ args = vec![
+ ColumnarValue::Scalar(ScalarValue::IntervalMonthDayNano(Some(
+ IntervalMonthDayNano {
+ months: 0,
Review Comment:
Can we have these as slts only? Or is it easier to represent via rust? It seems we overlap with the introduced slt case anyway
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix date_bin overflows scaling extreme Timestamp(Second) source [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #22315:
URL: https://github.com/apache/datafusion/pull/22315#discussion_r3327912717
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -594,53 +597,97 @@ fn date_bin_impl(
return exec_err!("DATE_BIN stride must be non-zero");
}
- fn stride_map_fn<T: ArrowTimestampType>(
- origin: i64,
- stride: i64,
- stride_fn: BinFunction,
- ) -> impl Fn(i64) -> Result<i64> {
- let scale = match T::UNIT {
+ fn timestamp_scale<T: ArrowTimestampType>() -> i64 {
+ match T::UNIT {
Nanosecond => 1,
Microsecond => NANOS_PER_MICRO,
Millisecond => NANOS_PER_MILLI,
Second => NANOSECONDS,
- };
- move |x: i64| match stride_fn(stride, x * scale, origin) {
- Ok(result) => Ok(result / scale),
- Err(e) => Err(e),
}
}
+ fn timestamp_scale_overflow_message(x: i64) -> String {
Review Comment:
since we're not doing the mapping b/w datafusion & arrow errors anymore (since we're not using try_unary now) we might as well have this function just return the datafusion error for simplicity, instead of only the message
(my previous comment was alluding to the fact that we might be better off constructing an arrow & datafusion version of the errors instead of doing only datafusion error and mapping to back to arrow (to then map it to datafusion again) but since we're not doing this anymore, we can just have this be datafusion error only)
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -710,20 +765,26 @@ fn date_bin_impl(
T: ArrowTimestampType,
{
let array = as_primitive_array::<T>(array)?;
- let scale = match T::UNIT {
- Nanosecond => 1,
- Microsecond => NANOS_PER_MICRO,
- Millisecond => NANOS_PER_MILLI,
- Second => NANOSECONDS,
- };
-
- let result: PrimitiveArray<T> = array.try_unary(|val| {
- stride_fn(stride, val * scale, origin)
- .map(|binned| binned / scale)
- .map_err(|e| {
- arrow::error::ArrowError::ComputeError(e.to_string())
- })
- })?;
+ let scale = timestamp_scale::<T>();
+
+ let values = array
+ .iter()
+ .map(|val| match val {
+ Some(val) => {
+ let scaled = val.checked_mul(scale).ok_or_else(|| {
+ DataFusionError::Execution(
+ timestamp_scale_overflow_message(val),
+ )
+ })?;
+ Ok(stride_fn(stride, scaled, origin)
+ .ok()
Review Comment:
cc @kumarUjjawal I think this will overlap with your PR
- https://github.com/apache/datafusion/pull/22610
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -699,6 +746,14 @@ fn date_bin_impl(
ColumnarValue::Scalar(ScalarValue::Time64Microsecond(result))
}
ColumnarValue::Array(array) => {
+ fn df_to_arrow(e: DataFusionError) -> ArrowError {
Review Comment:
I think it's preferably to remove this new `df_to_arrow` function since we're only using one arm of it (mapping to compute error)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix date_bin overflows scaling extreme Timestamp(Second) source [datafusion]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #22315:
URL: https://github.com/apache/datafusion/pull/22315#issuecomment-4476944750
@nathanb9 Thanks for your review. All comments have been revised according to your suggestions. Please confirm if this is what you expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix date_bin overflows scaling extreme Timestamp(Second) source [datafusion]
Posted by "nathanb9 (via GitHub)" <gi...@apache.org>.
nathanb9 commented on code in PR #22315:
URL: https://github.com/apache/datafusion/pull/22315#discussion_r3255727517
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -691,20 +723,24 @@ fn date_bin_impl(
T: ArrowTimestampType,
{
let array = as_primitive_array::<T>(array)?;
- let scale = match T::UNIT {
- Nanosecond => 1,
- Microsecond => NANOS_PER_MICRO,
- Millisecond => NANOS_PER_MILLI,
- Second => NANOSECONDS,
- };
-
- let result: PrimitiveArray<T> = array.try_unary(|val| {
- stride_fn(stride, val * scale, origin)
- .map(|binned| binned / scale)
- .map_err(|e| {
- arrow::error::ArrowError::ComputeError(e.to_string())
- })
- })?;
+ let scale = timestamp_scale::<T>();
+
+ let result: PrimitiveArray<T> = array
+ .try_unary(|val| {
+ let scaled =
+ scale_timestamp_to_nanos(val, scale).map_err(|e| {
+ arrow::error::ArrowError::ComputeError(e.to_string())
+ })?;
+
+ stride_fn(stride, scaled, origin)
+ .map(|binned| binned / scale)
+ .map_err(|e| {
Review 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.
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -1066,6 +1102,24 @@ mod tests {
res.err().unwrap().strip_backtrace(),
"This feature is not implemented: DATE_BIN only supports literal values for the origin argument, not arrays"
);
+
Review Comment:
Could you add simple test for the array path?
just like:
```rust
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix date_bin overflows scaling extreme Timestamp(Second) source [datafusion]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #22315:
URL: https://github.com/apache/datafusion/pull/22315#issuecomment-4483018443
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix date_bin overflows scaling extreme Timestamp(Second) source [datafusion]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #22315:
URL: https://github.com/apache/datafusion/pull/22315#discussion_r3303812605
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -575,53 +578,85 @@ fn date_bin_impl(
return exec_err!("DATE_BIN stride must be non-zero");
}
- fn stride_map_fn<T: ArrowTimestampType>(
- origin: i64,
- stride: i64,
- stride_fn: BinFunction,
- ) -> impl Fn(i64) -> Result<i64> {
- let scale = match T::UNIT {
+ fn timestamp_scale<T: ArrowTimestampType>() -> i64 {
+ match T::UNIT {
Nanosecond => 1,
Microsecond => NANOS_PER_MICRO,
Millisecond => NANOS_PER_MILLI,
Second => NANOSECONDS,
- };
- move |x: i64| match stride_fn(stride, x * scale, origin) {
- Ok(result) => Ok(result / scale),
- Err(e) => Err(e),
}
}
+ fn scale_timestamp_to_nanos(x: i64, scale: i64) -> Result<i64> {
+ x.checked_mul(scale).ok_or_else(|| {
+ DataFusionError::Execution(format!(
+ "DATE_BIN source timestamp {x} cannot be represented in nanoseconds"
+ ))
+ })
+ }
+
Ok(match array {
ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(v, tz_opt)) => {
- let apply_stride_fn =
- stride_map_fn::<TimestampNanosecondType>(origin, stride, stride_fn);
+ let scale = timestamp_scale::<TimestampNanosecondType>();
ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(
- v.and_then(|val| apply_stride_fn(val).ok()),
+ match *v {
+ Some(val) => {
+ let scaled = scale_timestamp_to_nanos(val, scale)?;
+ match stride_fn(stride, scaled, origin) {
+ Ok(result) => Some(result / scale),
+ Err(_) => None,
+ }
Review Comment:
Should I fix these two issues in this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org