Toby1009 commented on code in PR #25815:
URL: https://github.com/apache/datafusion/pull/25815#discussion_r4121564541
##########
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 @viirya, this addresses my concern. LGTM!
--
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]
