Toby1009 commented on code in PR #25815:
URL: https://github.com/apache/datafusion/pull/25815#discussion_r4119121075
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -273,17 +273,13 @@ impl ScalarUDFImpl for DateBinFunc {
let date_value = &input[1];
let reference = input.get(2);
- // Scaling these representations to nanoseconds can overflow and turn
- // otherwise valid input rows into NULL. Unknown ranges use the Null
- // type and can hide one of these representations. The generated NULLs
- // need not have the same placement as the source ordering.
- let scale_can_overflow = matches!(
- date_value.range.data_type(),
- Null | Timestamp(Second | Millisecond | Microsecond, _) |
Time64(Microsecond)
- );
-
- if !scale_can_overflow
- && step.sort_properties == SortProperties::Singleton
+ // DATE_BIN preserves the order of its second argument. Values whose
+ // nanosecond form overflows i64 are binned in i128, so a non-null
+ // input only becomes NULL when its bin cannot be represented: a bin
+ // starting before the minimum value of the type, or a month bin
+ // outside the range of `DateTime<Utc>`. These extremes are accepted
+ // rather than giving up the ordering for all inputs.
+ if step.sort_properties == SortProperties::Singleton
Review Comment:
Could we handle negative month strides before restoring ordering propagation
here? They are currently accepted and can be non-monotonic even for ordinary
dates, without overflow or generated NULLs.
I reproduced this on `756662df`:
```sql
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;
```
The final sort is removed, and the result is `2023-02-28`, then
`2023-01-31`, violating the ascending `ORDER BY`.
The underlying negative-month behavior predates this PR: when `bin_time >
source_date`, subtracting a negative `stride_months` moves the candidate
forward. The regression here is exposing that behavior to sort elimination for
coarse-precision inputs. I also verified that restoring only the previous
ordering guard, while keeping the new i128 arithmetic, restores the correct
ordering of this query's output.
Could we reject unsupported negative month strides, or avoid propagating
ordering for them until their semantics are made monotonic? This would preserve
the optimization for ordinary positive strides.
For focused coverage, I'd suggest:
- Add the query above as an end-to-end regression in `timestamps.slt` if
negative strides remain supported. If they are rejected instead, add scalar and
column-input error cases in `date_bin_errors.slt`.
- Add the same query with `INTERVAL '1 month'` as a positive control,
checking the result (`2022-12-31`, `2023-01-31`) and that the final sort can
still be removed. I checked this positive case on the current head 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
