fix: reject date_bin strides that are not positive

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<i64> {
-    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:?}"
-        );
-    }
 }

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'));

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

docs/source/library-user-guide/upgrading/56.0.0.md

@@ -611,6 +611,37 @@ 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.
+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 differ from before where the negative
+stride returned a bin later than the timestamp:
+
+```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<Item = (&str, ArrayRef)>`

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: