nikolas-sapa commented on issue #26671:
URL: https://github.com/apache/datafusion/issues/26671#issuecomment-6424993031
Hi — I dug into this and opened PR #26742 with a fix. Thanks for the clear
repro!
**Root cause.** Comparison coercion unifies any `Duration` with any
`Interval` as `Interval(MonthDayNano)`, and arrow's duration→interval cast puts
the *entire* duration into the nanoseconds field (`IntervalMonthDayNano::new(0,
0, v)` in arrow-cast 60.0.0 `cast/mod.rs:516-568`). But
`IntervalMonthDayNano`'s derived `Ord` is lexicographic **months → days →
nanoseconds** (arrow-buffer `interval.rs:46-77` — its docs warn the ordering is
"potentially surprising"). So for `(ts1 - ts2) <= INTERVAL '3 days'`, the
duration becomes `{months: 0, days: 0, nanos: 10d}` and the interval `{months:
0, days: 3, nanos: 0}` — the days field decides `0 < 3` before nanoseconds are
ever compared, and the predicate folds to `true`. Any interval with a non-zero
months or days field mis-decides the same way; pure-nanosecond intervals
(`INTERVAL '1 hour'`) happened to compare correctly because the comparison
degenerated to a pure nanoseconds compare. Both the constant-folded and the
runtime path
are affected.
**Proposed semantics** (in the PR): mixed Duration-vs-Interval comparisons
coerce both sides to `Duration(Nanosecond)` and compare exact elapsed time,
following arrow's own convertibility rules:
- arrow-cast only supports `Duration <-> Interval(MonthDayNano)` casts at
all (`cast/mod.rs:336-337`), so `Interval(YearMonth)`/`Interval(DayTime)` mixed
comparisons are rejected at planning (`Cannot infer common argument type for
comparison operation`).
- `cast_month_day_nano_to_duration` succeeds exactly when `months == 0 &&
days == 0` and otherwise fails loudly (`Cannot convert interval containing
non-zero months or days to duration`). With DataFusion's default `safe: false`
cast options, pure-nanosecond intervals keep comparing correctly via exact
nanoseconds, while days/months intervals error — at plan time for literals, at
execution time for columns — instead of silently returning wrong results.
- Duration-vs-Duration, Interval-vs-Interval, and UNION/CASE type
unification are unchanged.
**One judgment call I'd appreciate maintainer feedback on:** for intervals
with a **days** component, an alternative would be to normalize days as exact
24h (`days * 86_400e9 + nanos`) and reject only months (arrow-arith does add
interval days as exact 24h, `arrow-array` `types.rs:1095-1105`). I went with
erroring instead, because arrow-cast deliberately refuses interval→duration for
non-zero days, and hand-rolling the conversion would make `d <= i` disagree
with `CAST(i AS Duration(...))` / `arrow_cast(i, 'Duration(Second)')`. If you'd
rather keep `d <= INTERVAL '3 days'` working via exact-24h normalization,
that's a small change to the coercion and I'm happy to do it.
(Prepared with AI assistance; analysis and fix verified locally — unit tests
plus sqllogictests for both the constant-folded and runtime paths, all green.)
--
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]
