viirya commented on code in PR #25815:
URL: https://github.com/apache/datafusion/pull/25815#discussion_r4158051532
##########
datafusion/sqllogictest/test_files/datetime/timestamps.slt:
##########
@@ -1792,9 +1817,112 @@ FROM (
)
ORDER BY b ASC NULLS LAST
----
+1653-02-10T06:13:20
1970-01-01T00:00:00
-NULL
-NULL
+2286-11-20T17:46:40
+
+# A negative month stride can move a bin past its source, so date_bin keeps
Review Comment:
Thanks, that's a fair point; an error would also match PostgreSQL. I'd like
to settle this in #25856 so the discussion isn't lost once this PR merges. I'll
add both positions there.
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -475,11 +562,31 @@ fn date_bin_timestamp_value<T: ArrowTimestampType>(
value: i64,
origin: i64,
stride: i64,
- stride_fn: BinFunction,
+ stride_fn: BinFunctions,
) -> Option<i64> {
let scale = timestamp_scale::<T>();
- scale_and_bin_to_nanos(value, scale, origin, stride, stride_fn)
- .map(|binned| binned / scale)
+ match scale_and_bin_to_nanos(value, scale, origin, stride,
stride_fn.narrow) {
+ Some(binned) => Some(binned / scale),
+ None => {
+ date_bin_timestamp_value_wide(value, scale, origin, stride,
stride_fn.wide)
+ }
+ }
+}
+
+// Slow path for values whose i64 nanosecond computation overflows. Binning in
+// i128 means that only a result outside the source type's range becomes NULL,
+// instead of any value outside the i64 nanosecond range.
Review Comment:
Interesting idea, and it would keep the output monotonic. For fixed strides
the only remaining `NULL`s are bins starting before the type's minimum value,
so clamping them to `i64::MIN` keeps every result in order and never after its
source. Month strides also hit chrono's range at both ends, though (#25855),
and clamping the high end to `i64::MIN` would break the ordering, so it
wouldn't cover them yet. Once #25855 does the month arithmetic without chrono,
only the low end remains, and with clamping `date_bin` would never return
`NULL` for a non-null input. Since that changes results and needs documenting,
I'd rather do it together with #25855 than in this PR. Would that work for you?
--
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]
