From 3c7f86f73bbabd2e3d1ca8481d6cb93c81d00ed1 Mon Sep 17 00:00:00 2001 From: Liang-Chi Hsieh Date: Mon, 5 Oct 2026 19:46:43 -0700 Subject: [PATCH 1/2] fix: reject date_bin strides that are not positive Negative strides had no defined semantics: a negative fixed stride rounds toward the origin, and a negative month stride is not monotonic. Like PostgreSQL, `date_bin` now returns an error for a stride that is not greater than zero, which also covers the zero stride. With only positive strides, `output_ordering` no longer special-cases negative month strides (added in #25815), and `compute_distance` / `compute_distance_wide` round down with `rem_euclid`. Closes #25856. --- datafusion/functions/src/datetime/date_bin.rs | 117 +++++++----------- .../test_files/date_bin_errors.slt | 54 ++++++++ .../test_files/datetime/timestamps.slt | 68 +--------- .../library-user-guide/upgrading/56.0.0.md | 25 ++++ .../source/user-guide/sql/scalar_functions.md | 2 +- 5 files changed, 125 insertions(+), 141 deletions(-) diff --git a/datafusion/functions/src/datetime/date_bin.rs b/datafusion/functions/src/datetime/date_bin.rs index 1e04c6c05754b..d702af392c7c3 100644 --- a/datafusion/functions/src/datetime/date_bin.rs +++ b/datafusion/functions/src/datetime/date_bin.rs @@ -89,7 +89,10 @@ FROM VALUES (TIME '02:18:18'), (TIME '19:00:03') t(time); +----------+ 2 row(s) fetched. ```"#, - argument(name = "interval", description = "Bin interval."), + argument( + name = "interval", + description = "Bin interval. Must be greater than zero." + ), argument( name = "expression", description = "Time expression to operate on. Can be a constant, column, or function." @@ -273,19 +276,7 @@ impl ScalarUDFImpl for DateBinFunc { let reference = input.get(2); // DATE_BIN preserves the order of its second argument. - // - // A negative month stride can move a bin past its source (see - // `bin_months`), so its output is not monotonic, even for ordinary - // dates. See https://github.com/apache/datafusion/issues/25856 - let monotonic_stride = - matches!(step.range.lower(), ScalarValue::IntervalDayTime(Some(_))) - || matches!( - step.range.lower(), - ScalarValue::IntervalMonthDayNano(Some(v)) if v.months >= 0 - ); - - if monotonic_stride - && step.sort_properties == SortProperties::Singleton + if step.sort_properties == SortProperties::Singleton && reference .map(|r| r.sort_properties == SortProperties::Singleton) .unwrap_or(true) @@ -381,30 +372,16 @@ fn date_bin_nanos_interval(stride_nanos: i64, source: i64, origin: i64) -> Resul }) } -// distance from origin to bin +// distance from origin to bin, rounded down to a multiple of the positive +// `stride`, so a source before the origin falls into the previous bin fn compute_distance(time_diff: i64, stride: i64) -> Result { - let remainder = time_diff.checked_rem(stride).ok_or_else(|| { - ArrowError::InvalidArgumentError(format!( - "date_bin compute_distance time_diff {time_diff} % stride {stride} overflows i64" - )) - })?; - let time_delta = time_diff.checked_sub(remainder).ok_or_else(|| { + let remainder = time_diff.rem_euclid(stride); + time_diff.checked_sub(remainder).ok_or_else(|| { ArrowError::InvalidArgumentError(format!( "date_bin compute_distance time_diff {time_diff} - remainder {remainder} overflows i64" )) - })?; - - if time_diff < 0 && stride > 1 && time_delta != time_diff { - // The origin is later than the source timestamp, round down to the previous bin - time_delta.checked_sub(stride).ok_or_else(|| { - ArrowError::InvalidArgumentError(format!( - "date_bin compute_distance time_delta {time_delta} - stride {stride} overflows i64" - )) - .into() - }) - } else { - Ok(time_delta) - } + .into() + }) } // `date_bin_nanos_interval` in i128, which cannot overflow for an i64 source @@ -419,19 +396,10 @@ fn date_bin_nanos_interval_wide( Some(origin + time_delta) } -// `compute_distance` in i128. `stride` is non-zero, and `time_diff` is far +// `compute_distance` in i128. `stride` is positive, and `time_diff` is far // from i128::MIN, so none of these operations can overflow. fn compute_distance_wide(time_diff: i128, stride: i128) -> i128 { - let time_delta = time_diff - time_diff % stride; - // `%` rounds toward zero, so a negative `time_diff` between two bins is - // rounded up to the later bin; move back one bin. This must match - // `compute_distance`, so it does not use `rem_euclid`, which rounds - // negative strides differently. - if time_diff < 0 && stride > 1 && time_delta != time_diff { - time_delta - stride - } else { - time_delta - } + time_diff - time_diff.rem_euclid(stride) } // Shift `origin_date` by `month_delta` months, mapping an out-of-range result to @@ -726,13 +694,14 @@ fn date_bin_impl( let (stride, stride_fn) = stride.bin_fn(); - // Return error if stride is 0 - if stride == 0 { - return exec_err!("DATE_BIN stride must be non-zero"); + // Like PostgreSQL, only positive strides are supported. The binning + // functions rely on this to round down to the bin start. + if stride <= 0 { + return exec_err!("DATE_BIN stride must be greater than zero"); } // A TIME source requires a TIME origin. This shared-input check is ordered - // after stride/origin parsing and the zero-stride check so error ordering is + // after stride/origin parsing and the stride check so error ordering is // unchanged, and replaces the per-arm guards in the TIME branches below. if !is_time { match array.data_type() { @@ -1014,7 +983,7 @@ mod tests { } #[test] - fn output_ordering_requires_monotonic_stride() { + fn output_ordering_requires_constant_stride() { use arrow::compute::SortOptions; use datafusion_expr::sort_properties::{ExprProperties, SortProperties}; @@ -1035,25 +1004,19 @@ mod tests { ordering(literal(ScalarValue::new_interval_dt(0, 1000))), ordered ); - assert_eq!( - ordering(literal(ScalarValue::new_interval_dt(-1, 0))), - ordered - ); assert_eq!( ordering(literal(ScalarValue::new_interval_mdn(1, 0, 0))), ordered ); + // Strides that are not positive are rejected at execution, so a + // constant stride preserves the order whatever its value. assert_eq!( - ordering(literal(ScalarValue::new_interval_mdn(0, -1, 0))), + ordering(ExprProperties::new_unknown().with_order(SortProperties::Singleton)), ordered ); + // A stride that is not constant does not. assert_eq!( - ordering(literal(ScalarValue::new_interval_mdn(-1, 0, 0))), - SortProperties::Unordered - ); - // A constant stride whose value is unknown may be a negative month. - assert_eq!( - ordering(ExprProperties::new_unknown().with_order(SortProperties::Singleton)), + ordering(ExprProperties::new_unknown().with_order(ordered)), SortProperties::Unordered ); } @@ -1158,9 +1121,26 @@ mod tests { let res = invoke_date_bin_with_args(args, 1, return_field); assert_eq!( res.err().unwrap().strip_backtrace(), - "Execution error: DATE_BIN stride must be non-zero" + "Execution error: DATE_BIN stride must be greater than zero" ); + // A negative day-time stride, which SQL interval literals cannot produce + for stride in [ + ScalarValue::new_interval_dt(-1, 0), + ScalarValue::new_interval_dt(1, -86_400_001), + ] { + args = vec![ + ColumnarValue::Scalar(stride), + ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(Some(1), None)), + ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(Some(1), None)), + ]; + let res = invoke_date_bin_with_args(args, 1, return_field); + assert_eq!( + res.err().unwrap().strip_backtrace(), + "Execution error: DATE_BIN stride must be greater than zero" + ); + } + // stride: overflow of day-time interval args = vec![ ColumnarValue::Scalar(ScalarValue::IntervalDayTime(Some( @@ -1827,17 +1807,4 @@ mod tests { time64_msg, ); } - - #[test] - fn test_date_bin_compute_distance_rem_overflow() { - // Regression for #22215: `time_diff % stride` panics with "attempt to - // calculate the remainder with overflow" when `time_diff == i64::MIN` - // and `stride == -1`. Now it must return a normal Err that the scalar - // pipeline maps to NULL. - let result = date_bin_nanos_interval(-1, i64::MIN, 0); - assert!( - result.is_err(), - "expected Err for time_diff=i64::MIN, stride=-1, got {result:?}" - ); - } } diff --git a/datafusion/sqllogictest/test_files/date_bin_errors.slt b/datafusion/sqllogictest/test_files/date_bin_errors.slt index cb26023c460d9..68536ee455e4d 100644 --- a/datafusion/sqllogictest/test_files/date_bin_errors.slt +++ b/datafusion/sqllogictest/test_files/date_bin_errors.slt @@ -115,3 +115,57 @@ from ( ) as t(ts); ---- 2286-11-20T17:46:40 + +# Strides must be greater than zero, like in PostgreSQL. A negative stride has +# no well-defined bins: a negative month stride used to bin 2023-01-01 forward +# to 2023-02-28 with origin 2023-01-31. +query error DataFusion error: Execution error: DATE_BIN stride must be greater than zero +select date_bin(interval '-1 month', timestamp '2023-01-01 00:00:00', timestamp '2023-01-31 00:00:00'); + +query error DataFusion error: Execution error: DATE_BIN stride must be greater than zero +select date_bin(interval '-1 year', timestamp '2023-01-01 00:00:00'); + +query error DataFusion error: Execution error: DATE_BIN stride must be greater than zero +select date_bin(interval '-15 minutes', timestamp '2023-01-01 18:18:18'); + +query error DataFusion error: Execution error: DATE_BIN stride must be greater than zero +select date_bin(interval '-1 day', timestamp '2023-01-01 18:18:18'); + +# The parts of the interval add up to a negative stride. +query error DataFusion error: Execution error: DATE_BIN stride must be greater than zero +select date_bin(interval '1 day -25 hours', timestamp '2023-01-01 18:18:18'); + +# A stride of -1 ns with a source at i64::MIN used to overflow the remainder +# (#22215) +query error DataFusion error: Execution error: DATE_BIN stride must be greater than zero +select date_bin( + interval '-1 nanosecond', + arrow_cast(-9223372036854775808, 'Timestamp(Nanosecond, None)') +); + +# Column inputs, for second and nanosecond precision +query error DataFusion error: Execution error: DATE_BIN stride must be greater than zero +select date_bin(interval '-1 month', ts, timestamp '2023-01-31 00:00:00') +from ( + select arrow_cast(column1, 'Timestamp(Second)') as ts + from (values (timestamp '2023-01-01 00:00:00'), (timestamp '2023-01-31 00:00:00')) +); + +query error DataFusion error: Execution error: DATE_BIN stride must be greater than zero +select date_bin(interval '-1 month', column1, timestamp '2023-01-31 00:00:00') +from (values (timestamp '2023-01-01 00:00:00'), (timestamp '2023-01-31 00:00:00')); + +query error DataFusion error: Execution error: DATE_BIN stride must be greater than zero +select date_bin(interval '-15 minutes', column1) +from (values (timestamp '2023-01-01 18:18:18'), (timestamp '2023-01-03 19:00:03')); + +# TIME inputs +query error DataFusion error: Execution error: DATE_BIN stride must be greater than zero +select date_bin(interval '-15 minutes', time '14:38:50', time '00:00:00'); + +query error DataFusion error: Execution error: DATE_BIN stride must be greater than zero +select date_bin(interval '-1 month', time '14:38:50', time '00:00:00'); + +query error DataFusion error: Execution error: DATE_BIN stride must be greater than zero +select date_bin(interval '-15 minutes', column1, time '00:00:00') +from (values (time '14:38:50'), (time '19:00:03')); diff --git a/datafusion/sqllogictest/test_files/datetime/timestamps.slt b/datafusion/sqllogictest/test_files/datetime/timestamps.slt index 71118ddcabb73..ee6bedfeba5dd 100644 --- a/datafusion/sqllogictest/test_files/datetime/timestamps.slt +++ b/datafusion/sqllogictest/test_files/datetime/timestamps.slt @@ -823,13 +823,13 @@ query error SELECT DATE_BIN(INTERVAL '0 second', 25, TIMESTAMP '1970-01-01T00:00:00Z') # not support interval 0 -statement error Execution error: DATE_BIN stride must be non-zero +statement error Execution error: DATE_BIN stride must be greater than zero SELECT DATE_BIN(INTERVAL '0 second', TIMESTAMP '2022-08-03 14:38:50.000000006Z', TIMESTAMP '1970-01-01T00:00:00Z') -statement error Execution error: DATE_BIN stride must be non-zero +statement error Execution error: DATE_BIN stride must be greater than zero SELECT DATE_BIN(INTERVAL '0 month', TIMESTAMP '2022-08-03 14:38:50.000000006Z') -statement error Execution error: DATE_BIN stride must be non-zero +statement error Execution error: DATE_BIN stride must be greater than zero SELECT DATE_BIN(INTERVAL '0' minute, time) AS time, count(val) @@ -1821,68 +1821,6 @@ ORDER BY b ASC NULLS LAST 1970-01-01T00:00:00 2286-11-20T17:46:40 -# A negative month stride does not shift every bin by the same amount: here -# 2023-01-01 bins forward to 2023-02-28, while 2023-01-31 bins to itself. The -# output is not sorted, so date_bin keeps the final sort for it. -query TT -EXPLAIN SELECT date_bin(INTERVAL '-1 month', ts, TIMESTAMP '2023-01-31 00:00:00') AS b -FROM ( - SELECT arrow_cast(column1, 'Timestamp(Second)') AS ts - FROM (VALUES - (TIMESTAMP '2023-01-01 00:00:00'), - (TIMESTAMP '2023-01-31 00:00:00') - ) - ORDER BY ts - LIMIT 2 -) -ORDER BY b ----- -logical_plan -01)Sort: b ASC NULLS LAST -02)--Projection: date_bin(IntervalMonthDayNano("IntervalMonthDayNano { months: -1, days: 0, nanoseconds: 0 }"), ts, TimestampNanosecond(1675123200000000000, None)) AS b -03)----Sort: ts ASC NULLS LAST, fetch=2 -04)------Projection: CAST(column1 AS Timestamp(s)) AS ts -05)--------Values: (TimestampNanosecond(1672531200000000000, None) AS Utf8("2023-01-01 00:00:00")), (TimestampNanosecond(1675123200000000000, None) AS Utf8("2023-01-31 00:00:00")) -physical_plan -01)SortExec: expr=[b@0 ASC NULLS LAST], preserve_partitioning=[false] -02)--ProjectionExec: expr=[date_bin(IntervalMonthDayNano { months: -1, days: 0, nanoseconds: 0 }, ts@0, 1675123200000000000) as b] -03)----SortExec: TopK(fetch=2), expr=[ts@0 ASC NULLS LAST], preserve_partitioning=[false] -04)------ProjectionExec: expr=[CAST(column1@0 AS Timestamp(s)) as ts] -05)--------DataSourceExec: partitions=1, partition_sizes=[1] - -query P -SELECT date_bin(INTERVAL '-1 month', ts, TIMESTAMP '2023-01-31 00:00:00') AS b -FROM ( - SELECT arrow_cast(column1, 'Timestamp(Second)') AS ts - FROM (VALUES - (TIMESTAMP '2023-01-01 00:00:00'), - (TIMESTAMP '2023-01-31 00:00:00') - ) - ORDER BY ts - LIMIT 2 -) -ORDER BY b ----- -2023-01-31T00:00:00 -2023-02-28T00:00:00 - -# Same for nanosecond timestamps. -query P -SELECT date_bin(INTERVAL '-1 month', ts, TIMESTAMP '2023-01-31 00:00:00') AS b -FROM ( - SELECT column1 AS ts - FROM (VALUES - (TIMESTAMP '2023-01-01 00:00:00'), - (TIMESTAMP '2023-01-31 00:00:00') - ) - ORDER BY ts - LIMIT 2 -) -ORDER BY b ----- -2023-01-31T00:00:00 -2023-02-28T00:00:00 - # A positive month stride is monotonic, so the final sort is removed. query TT EXPLAIN SELECT date_bin(INTERVAL '1 month', ts, TIMESTAMP '2023-01-31 00:00:00') AS b diff --git a/docs/source/library-user-guide/upgrading/56.0.0.md b/docs/source/library-user-guide/upgrading/56.0.0.md index 6d616a90a8575..ad75b102e4bb9 100644 --- a/docs/source/library-user-guide/upgrading/56.0.0.md +++ b/docs/source/library-user-guide/upgrading/56.0.0.md @@ -611,6 +611,31 @@ should treat an empty list as the absent-key case instead. See [issue #24981](https://github.com/apache/datafusion/issues/24981) and [issue #24983](https://github.com/apache/datafusion/issues/24983) for details. +### `date_bin` rejects strides that are not positive + +`date_bin` now returns an error when its stride is not greater than zero, +matching PostgreSQL. A zero stride was already an error; its message is now +`DATE_BIN stride must be greater than zero`. Previously, a negative stride was +accepted but its bins were not well defined: a negative month stride could +place a timestamp in a bin after it, and the output was not sorted even when +the input was. + +A positive stride already bins timestamps before the origin, so use the +absolute value of the stride. The results can differ from before where a +negative stride placed a timestamp in a later bin, such as for timestamps +before the origin: + +```sql +-- Before +SELECT date_bin(INTERVAL '-1 month', ts, TIMESTAMP '2023-01-31 00:00:00') FROM t; + +-- After +SELECT date_bin(INTERVAL '1 month', ts, TIMESTAMP '2023-01-31 00:00:00') FROM t; +``` + +See [issue #25856](https://github.com/apache/datafusion/issues/25856) for +details. + ### `DataFrame::from_columns` accepts `IntoIterator` `DataFrame::from_columns` now accepts any `IntoIterator` diff --git a/docs/source/user-guide/sql/scalar_functions.md b/docs/source/user-guide/sql/scalar_functions.md index 1ba1e8c820005..05590330b5ec1 100644 --- a/docs/source/user-guide/sql/scalar_functions.md +++ b/docs/source/user-guide/sql/scalar_functions.md @@ -2478,7 +2478,7 @@ date_bin(interval, expression[, origin_timestamp]) #### Arguments -- **interval**: Bin interval. +- **interval**: Bin interval. Must be greater than zero. - **expression**: Time expression to operate on. Can be a constant, column, or function. - **origin_timestamp**: Optional. Starting point used to determine bin boundaries. If not specified defaults 1970-01-01T00:00:00Z (the UNIX epoch in UTC). The following intervals are supported: From 7ed727ac819b6a57f314becad3aad9e34581385c Mon Sep 17 00:00:00 2001 From: Liang-Chi Hsieh Date: Mon, 5 Oct 2026 22:20:29 -0700 Subject: [PATCH 2/2] docs: clarify previous negative date_bin stride results in upgrade guide --- .../library-user-guide/upgrading/56.0.0.md | 24 ++++++++++++------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/docs/source/library-user-guide/upgrading/56.0.0.md b/docs/source/library-user-guide/upgrading/56.0.0.md index ad75b102e4bb9..fa71d9e087310 100644 --- a/docs/source/library-user-guide/upgrading/56.0.0.md +++ b/docs/source/library-user-guide/upgrading/56.0.0.md @@ -613,17 +613,23 @@ See [issue #24981](https://github.com/apache/datafusion/issues/24981) and ### `date_bin` rejects strides that are not positive -`date_bin` now returns an error when its stride is not greater than zero, -matching PostgreSQL. A zero stride was already an error; its message is now -`DATE_BIN stride must be greater than zero`. Previously, a negative stride was -accepted but its bins were not well defined: a negative month stride could -place a timestamp in a bin after it, and the output was not sorted even when -the input was. +`date_bin` now returns an error when its stride is not greater than zero. +This matches PostgreSQL for fixed strides (PostgreSQL does not support month +strides at all), and DataFusion applies the same rule to month strides. A zero +stride was already an error; its message is now +`DATE_BIN stride must be greater than zero`. + +Previously, a negative stride was accepted but its bins were not well defined. +It returned the same bin as the positive stride only when the timestamp was at +or after the origin, and for month strides only when the timestamp's day of +month was not earlier than the origin's. Otherwise it returned a bin later than +the timestamp. For example, with origin `2023-01-31`, `INTERVAL '-1 month'` +binned `2023-03-15` to `2023-04-30`, and `2023-01-01` to `2023-02-28`. The +output was also not sorted even when the input was. A positive stride already bins timestamps before the origin, so use the -absolute value of the stride. The results can differ from before where a -negative stride placed a timestamp in a later bin, such as for timestamps -before the origin: +absolute value of the stride. The results differ from before where the negative +stride returned a bin later than the timestamp: ```sql -- Before