viirya commented on code in PR #25692:
URL: https://github.com/apache/datafusion/pull/25692#discussion_r4106758963
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -269,14 +269,22 @@ impl ScalarUDFImpl for DateBinFunc {
}
fn output_ordering(&self, input: &[ExprProperties]) ->
Result<SortProperties> {
- // The DATE_BIN function preserves the order of its second argument.
let step = &input[0];
let date_value = &input[1];
let reference = input.get(2);
- if step.sort_properties.eq(&SortProperties::Singleton)
+ // Scaling these representations to nanoseconds can overflow and turn
Review Comment:
Yes, the upper boundary is 2262-04-11, and there is also a lower boundary at
1677-09-21. The repro values of +/-10,000,000,000 seconds correspond to roughly
1653 and 2286. These are valid `Timestamp(Second)` values, but `date_bin`
currently maps per-row scaling failures to `NULL`.
This means sorted input can become `[NULL, valid value, NULL]`, which no
longer satisfies the original `NULLS FIRST/LAST` ordering. Propagating that
ordering can therefore produce incorrect query results.
I agree the guard is conservative: column ranges are currently unbounded in
`ExprProperties`, so DataFusion cannot distinguish ordinary dates from values
outside the nanosecond range and may add a sort for all coarse-precision
timestamps.
Avoiding that cost safely would require a larger change, such as computing
`date_bin` in the source precision using wider arithmetic, or propagating
proven input bounds. My inclination is to keep the correctness guard here and
handle source-precision computation as a follow-up.
--
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]
