kosiew commented on code in PR #25315:
URL: https://github.com/apache/datafusion/pull/25315#discussion_r4134638943
##########
datafusion/functions/src/datetime/date_bin.rs:
##########
@@ -466,6 +466,69 @@ fn date_bin_timestamp_value<T: ArrowTimestampType>(
.map(|binned| binned / scale)
}
+/// Fast path for the common nanosecond interval `date_bin` cases
+///
+/// Time-based interval strides (`INTERVAL '1 hour'`, `'1 day'`, `'1 minute'`,
+/// etc.) all carry `months == 0` and are represented as
+/// [`Interval::Nanoseconds`], which is the overwhelmingly common case for
+/// time-series bucketing. Month/year strides (`INTERVAL '1 month'`) use the
+/// general path and never reach this function.
+///
+/// This is a vectorized fast path: it computes every bin with a single
+/// infallible pass of plain integer arithmetic
+/// (`bin = origin + floor((value * scale - origin) / stride) * stride`),
+/// avoiding the per-value function-pointer dispatch and checked
+/// arithmetic / `Result` plumbing of the general path. It is only used when
+/// the whole array is bounded away from the `i64` extremes (checked with a
+/// cheap min/max scan and a margin covering the shifting by `origin` and
+/// `stride`); otherwise `None` falls back to the general path.
+fn try_fast_date_bin_nanos_stride<T: ArrowTimestampType>(
+ array: &PrimitiveArray<T>,
+ origin: i64,
+ stride: i64,
+) -> Option<PrimitiveArray<T>> {
+ if stride <= 0 {
+ return None;
+ }
+
+ let scale = timestamp_scale::<T>();
+
+ // Whole-array safety check: min/max scan.
+ let (min_value, max_value) = array
+ .values()
+ .iter()
+ .fold((i64::MAX, i64::MIN), |(mn, mx), v| (mn.min(*v), mx.max(*v)));
+ let scaled_max = max_value.checked_mul(scale)?;
+ let scaled_min = min_value.checked_mul(scale)?;
+
+ // One binning can shift the value by at most `2 * |origin| + |stride|`
+ // from the scaled input, so require every scaled value to stay within
+ // that margin of the `i64` range. If not, fall back (return `None`) and
+ // let the caller use the general path unchanged.
+ let margin = origin
+ .unsigned_abs()
+ .saturating_mul(2)
+ .saturating_add(stride.unsigned_abs())
+ .saturating_add(1) as i64;
Review Comment:
The `as i64` cast can turn a saturated unsigned margin into `-1`, which lets
unsafe inputs enter the fast path and produce a value instead of the NULL
returned by the existing checked path. Please fall back when the margin exceeds
`i64::MAX` and add a regression test, for example with timestamp `[0]`, `origin
= i64::MIN`, and stride `1`.
--
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]
