Repository navigation
fix: prevent date_trunc array underflow - #25131
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25131 +/- ##
==========================================
+ Coverage 81.80% 81.91% +0.10%
==========================================
Files 1130 1132 +2
Lines 417754 420651 +2897
Branches 417754 420651 +2897
==========================================
+ Hits 341754 344569 +2815
+ Misses 55875 55762 -113
- Partials 20125 20320 +195 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gene-bordegaray
left a comment
gene-bordegaray
left a comment
There was a problem hiding this comment.
the logic is sound I would be curios to see perf implications. can you tun the date_trunc_minute_1000 bench before and after.
the try_unary functin says it can't be vectorized so maybe some prerequisite check and then do a more optimized pass
|
@gene-bordegaray yup, massive regression (~50%), investigating alternative implementations |
jayzhan211
left a comment
jayzhan211
left a comment
There was a problem hiding this comment.
Thanks @benbellick , here is a possible perf issue
| ) | ||
| .with_timezone_opt(tz_opt); | ||
| let array: PrimitiveArray<T> = array | ||
| .try_unary(|value| { |
There was a problem hiding this comment.
Perf: this de-vectorizes the fast path it's fixing.
try_unary turns the straight-line map into a per-element Result with an early return. Arrow says as much in the docs for the adjacent try_unary_mut: "LLVM is currently unable to effectively vectorize fallible operations". This block is the fast path added in #23542 precisely to avoid per-value work, and there's an in-repo bench for it:
cargo bench -p datafusion-functions --bench datetime_expressions -- date_trunc_minute_1000
PR vs HEAD~1: 706 ns → 829 ns, +16.4% (p = 0.00).
Underflow is only possible for values within one unit of i64::MIN, so it can be detected with a branchless flag in the same pass, keeping the fallible kernel for the rare array that trips it:
if let Some(unit) = unit {
let unit = unit.get();
- let array: PrimitiveArray<T> = array
- .try_unary(|value| {
- value.checked_sub(value.rem_euclid(unit)).ok_or_else(|| {
- exec_datafusion_err!(
- "Timestamp {value} out of range after truncating to {granularity}"
- )
- })
- })?
- .with_timezone_opt(tz_opt);
+ // `value - value.rem_euclid(unit)` can only underflow within one `unit`
+ // of `i64::MIN`. Detect that with a branchless flag so the common case
+ // stays a single vectorizable pass, and only then take the fallible path.
+ let bound = i64::MIN + unit;
+ let mut maybe_underflow = false;
+ let values: Vec<i64> = array
+ .values()
+ .iter()
+ .map(|v| {
+ maybe_underflow |= *v < bound;
+ v.wrapping_sub(v.rem_euclid(unit))
+ })
+ .collect();
+ let array: PrimitiveArray<T> = if !maybe_underflow {
+ PrimitiveArray::<T>::new(values.into(), array.nulls().cloned())
+ } else {
+ array.try_unary(|value| {
+ value.checked_sub(value.rem_euclid(unit)).ok_or_else(|| {
+ exec_datafusion_err!(
+ "Timestamp {value} out of range after truncating to {granularity}"
+ )
+ })
+ })?
+ }
+ .with_timezone_opt(tz_opt);
Ok(Arc::new(array))Measured 651 ns on the same bench — 8.2% faster than main — with test_date_trunc_fine_granularity_underflow and the rest of the date_trunc tests passing, and cargo clippy --all-targets --all-features / cargo fmt clean. (Null slots can hold arbitrary values, so a tripped fl; that's fine — the fallback try_unary skips nulls and produces the right answer eitherway.)
There was a problem hiding this comment.
ya this should work thank @jayzhan211 ❤️
There was a problem hiding this comment.
Yup I had something similarish to this locally, this looks better though. I'll push a fix! thanks 🚀
There was a problem hiding this comment.
pushed a fix. locally I found that this current version is slightly faster (about 1.3% on the benchmark).
| } | ||
|
|
||
| #[test] | ||
| fn test_date_trunc_fine_granularity_underflow() { |
There was a problem hiding this comment.
actually going to delete this test, it feels a bit redundant now with the below.
|
cc: @jayzhan211 and @gabotechs |
|
Thanks @gabotechs @gene-bordegaray ! |
…26607) ## Which issue does this PR close? Closes apache#26605. ## Rationale for this change `date_bin` returns NULL for unrepresentable timestamp bins. Return an error instead, matching [PostgreSQL](https://www.db-fiddle.com/f/h3qSHUSRprbzkX9GBkSuY2/0). I'm not sure whether DataFusion has a specific policy on returning NULL versus an error here, but the PostgreSQL precedent seems clear. Returning NULL because we cannot compute a bin for a non-null timestamp silently hides a computation failure rather than representing missing input. This prepares `date_bin` for avoiding aggregate repartitioning in [apache#25345](apache#25345), similar to the `date_trunc` optimization in [apache#26449](apache#26449). It follows the error behavior established for `date_trunc` in [apache#25131](apache#25131). ## What changes are included in this PR? Make scalar and array timestamp-binning failures return `DATE_BIN timestamp out of range`. Preserve NULL inputs and successful wider-arithmetic results. ## What is the testing strategy for this PR? Unit tests and SQLLogicTests cover overflow errors, scalar/array consistency, NULL preservation, and representable results across timestamp precisions. ## Are there any user-facing changes? Previously NULL-producing timestamp-binning failures now fail the query. --- This PR was developed with AI assistance. All changes have been reviewed, and I take full responsibility for this contribution.
Which issue does this PR close?
Rationale for this change
date_trunccan panic in debug builds or return an incorrect timestamp in release builds when its array fast path truncates a timestamp neari64::MIN.What changes are included in this PR?
Use checked subtraction in the fine-granularity array path and return an execution error when the truncated timestamp cannot be represented.
What is the testing strategy for this PR?
Added
test_date_trunc_fine_granularity_underflow, which passesi64::MINthrough the nanosecond timestamp array path and verifies thatdate_truncreturns an execution error.The original SQL reproduction was also verified with the locally compiled DataFusion CLI.
Are there any user-facing changes?
Invalid extreme timestamp truncations now return an execution error instead of panicking or returning a wrapped timestamp.
Note: This PR was developed with AI assistance. All changes have been reviewed, and I take full responsibility for this contribution.