viirya commented on code in PR #25815:
URL: https://github.com/apache/datafusion/pull/25815#discussion_r4129067393
##########
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:
Here's the example: it's the query right below this comment (reworded in
bac3c017e). With origin 2023-01-31 and `INTERVAL '-1 month'`, sorted input
2023-01-01, 2023-01-31 becomes 2023-02-28, 2023-01-31. 2023-01-31 bins to
itself, but for 2023-01-01 the first candidate (2023-01-31) is after the
source, and the "move back one bin" step in `bin_months` subtracts the negative
stride, which moves it forward a month instead. So the bins don't all move the
same way, and the output is out of order.
@mhilton I agree: the bins `origin + k * stride` are the same set for
`stride` and `-stride`, so taking the absolute value is the natural reading,
and it would make negative strides monotonic and never after the source (rules
1 and 4 in your proposal). Negative fixed strides currently round toward the
origin, though, so that would change their results too. I opened #25856 for it
rather than doing it here; this PR only stops propagating ordering for negative
month strides.
##########
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:
Added a comment. I kept the same form as the i64 `compute_distance` rather
than `rem_euclid`: the two paths have to agree, and they differ for negative
strides (e.g. stride -3, diff -4 gives -3 here but -6 with `rem_euclid`). If we
take the absolute value of the stride in #25856, both can switch to
`rem_euclid`.
##########
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:
Right, valid TIME values can't overflow. Only out-of-spec
`Time64(Microsecond)` values of more than ~106,752 days reach it (e.g.
`i64::MAX` after a reinterpret cast, as in
`test_date_bin_time64_micro_overflow_handling`). Smaller out-of-spec values
already got a value from the i64 path, reduced to a day; with this path the
larger ones get one computed the same way instead of `NULL`. It's `#[cold]`, so
it costs nothing otherwise. Happy to drop it if you'd rather keep `NULL` there.
##########
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:
The result there is `i64::MAX` seconds, far outside chrono's range, so the
runner can't format it as a date at all ("Failed to convert ... to datetime");
the cast was only there to print it. In bac3c017e both queries use
10,000,000,000 seconds (2286-11-20) instead, which still overflows when scaled
to nanoseconds but displays fine, so the cast is gone. The `i64::MAX` cases
stay covered by `test_date_bin_beyond_nanosecond_range`.
##########
datafusion/sqllogictest/test_files/date_bin_errors.slt:
##########
@@ -73,15 +73,16 @@ select date_bin(
----
NULL
-# Extreme timestamp overflow: source - origin overflows i64 (should return
NULL, not panic)
+# Extreme timestamp overflow: source - origin overflows i64, so the bin is
+# computed in i128 instead (previously NULL, and before that a panic)
Review Comment:
Done in bac3c017e, trimmed it to just the overflow.
##########
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
Review Comment:
Done in bac3c017e, trimmed it to just the overflow.
--
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]
