mhilton commented on code in PR #25815:
URL: https://github.com/apache/datafusion/pull/25815#discussion_r4123693561
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -383,6 +418,29 @@ fn compute_distance(time_diff: i64, stride: i64) ->
Result<i64> {
}
}
+// `date_bin_nanos_interval` in i128, which cannot overflow for an i64 source
+// scaled to nanoseconds.
+fn date_bin_nanos_interval_wide(
+ stride_nanos: i64,
+ source: i128,
+ origin: i64,
+) -> Option<i128> {
+ let origin = i128::from(origin);
+ let time_delta = compute_distance_wide(source - origin,
i128::from(stride_nanos));
+ Some(origin + time_delta)
+}
+
+// `compute_distance` in i128. `stride` is non-zero, and `time_diff` is far
+// from i128::MIN, so none of these operations can overflow.
+fn compute_distance_wide(time_diff: i128, stride: i128) -> i128 {
+ let time_delta = time_diff - time_diff % stride;
+ if time_diff < 0 && stride > 1 && time_delta != time_diff {
Review Comment:
Maybe a comment saying why we're doing this (`%` rounds towards 0 for
negative numbers, therefore later).
Or you could use
[`rem_euclid`](https://doc.rust-lang.org/stable/std/primitive.i128.html#method.rem_euclid)
above.
##########
datafusion/sqllogictest/test_files/date_bin_errors.slt:
##########
@@ -93,25 +94,27 @@ select date_bin(
----
NULL
-# Source timestamp scaling to nanoseconds overflows: should return NULL, not
panic
-query P
-select date_bin(
+# Source timestamp scaling to nanoseconds overflows, so the bin is computed in
+# i128 instead (previously NULL, and before that a panic). The result cannot be
+# displayed as a date, so show its raw value.
+query I
+select arrow_cast(date_bin(
interval '1 nanosecond',
arrow_cast(9223372036854775807, 'Timestamp(Second, None)'),
timestamp '1970-01-01 00:00:00'
-);
+), 'Int64');
----
-NULL
+9223372036854775807
Review Comment:
Presumably so that one can see the result as a numeric value rather than
some date centuries in the future.
##########
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:
What does a negative interval even mean for date binning? I don't think it
would move where the bins are, should it mean that a date is rounded up rather
than down in which case it should hold for negative nanoseconds too. I
personally would advocate for taking the `abs` value of any interval used in
this context.
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -490,10 +598,26 @@ fn date_bin_time_value(
scale: i64,
origin: i64,
stride: i64,
- stride_fn: BinFunction,
+ stride_fn: BinFunctions,
) -> Option<i64> {
- scale_and_bin_to_nanos(value, scale, origin, stride, stride_fn)
- .map(|binned| (binned % NANOSECONDS_IN_DAY) / scale)
+ match scale_and_bin_to_nanos(value, scale, origin, stride,
stride_fn.narrow) {
+ Some(binned) => Some((binned % NANOSECONDS_IN_DAY) / scale),
+ None => date_bin_time_value_wide(value, scale, origin, stride,
stride_fn.wide),
+ }
+}
+
+// Slow path of `date_bin_time_value`, like `date_bin_timestamp_value_wide`.
+#[cold]
+#[inline(never)]
+fn date_bin_time_value_wide(
Review Comment:
My reading of the arrow spec suggests that this code will never be used.
Time values are the number of `s`/`ms`/`us`/`ns` since midnight that should
never overflow. I suppose someone could get funky with the origin but I suspect
we will see little use of this function.
##########
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:
To @alamb's point I'd like to see an example which inputs move things to the
point of needing a sort.
--
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]
