viirya commented on code in PR #25692:
URL: https://github.com/apache/datafusion/pull/25692#discussion_r4116123363
##########
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:
Fair point. The guard is conservative, and it doesn't fully close the hole
either: nanosecond inputs can still become NULL near `i64::MIN` (e.g.
`compute_distance`'s `time_delta - stride` for negative `time_diff`), or with
an explicit origin. So we pay the re-sort without getting the full guarantee.
I think the better fix is to stop `date_bin` from producing NULL for valid
input at all. For s/ms/us inputs we can compute the bin with i128 (or in source
precision) instead of scaling to nanoseconds first. Then `date_bin` is monotone
without per-row NULLs, and we can go back to propagating ordering for every
unit, not only nanoseconds. I filed #25812 to track this and will work on it
next. If it won't land before the next release, I'm happy to revert the guard
in the meantime so this doesn't ship as a regression.
--
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]
