viirya commented on code in PR #25815:
URL: https://github.com/apache/datafusion/pull/25815#discussion_r4129068286
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -273,16 +273,24 @@ 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)
- );
+ // 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.
+ //
+ // A negative month stride can move a bin past its source (see
+ // `bin_months`), so its output is not monotonic, even for ordinary
+ // dates. Only propagate the ordering for strides known not to be one.
Review Comment:
Applied in bac3c017e, thanks.
##########
datafusion/sqllogictest/test_files/datetime/timestamps.slt:
##########
@@ -1728,7 +1728,7 @@ physical_plan
02)--SortExec: TopK(fetch=2), expr=[column1@0 ASC NULLS LAST],
preserve_partitioning=[false]
03)----DataSourceExec: partitions=1, partition_sizes=[1]
-# date_bin remains conservatively unordered because per-row failures become
NULL
+# Typed bounds allow date_trunc to propagate ordering through timezone-free
date_bin
Review Comment:
The remaining `SortExec` is the `TopK(fetch=2)` for the subquery's `ORDER BY
ts LIMIT 2`, which produces the ordered input in this test; the outer
`SortExec` on `truncated` is the one that gets removed. Agreed that an `ORDERED
BY` external table would make these tests clearer, and happy to do that as a
follow-up 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: [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]
