viirya commented on code in PR #25815:
URL: https://github.com/apache/datafusion/pull/25815#discussion_r4119539128
##########
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:
Thanks, confirmed. For a negative month stride, the `bin_time > source_date`
fallback in `bin_months` subtracts a negative stride and moves the bin forward,
so the output isn't monotonic even for ordinary dates. This predates both PRs:
before #25692 the same query removed the final sort and returned the wrong
order for every precision, and on current `main` it still does for
`Timestamp(Nanosecond)`, since #25692 only guarded s/ms/us. So this PR
re-exposed it for s/ms/us rather than introducing it.
17d6f9436 only propagates ordering when the stride is known not to be a
negative month (`IntervalDayTime`, or `IntervalMonthDayNano` with `months >=
0`); results are unchanged. I added your query for both second and nanosecond
precision, plus the positive-stride control with its `EXPLAIN`, to
`timestamps.slt`, and a unit test for `output_ordering`.
I kept negative month strides supported rather than rejecting them here,
since that would turn currently-working queries into errors. Their semantics (a
bin after its source) look wrong anyway, so I'd like to handle that separately.
--
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]
