Skip to content

[codex] Fix time interval arithmetic wrapping#22280

Open
Sean-Kenneth-Doherty wants to merge 2 commits into
apache:mainfrom
Sean-Kenneth-Doherty:codex/time-interval-wrap
Open

[codex] Fix time interval arithmetic wrapping#22280
Sean-Kenneth-Doherty wants to merge 2 commits into
apache:mainfrom
Sean-Kenneth-Doherty:codex/time-interval-wrap

Conversation

@Sean-Kenneth-Doherty
Copy link
Copy Markdown

@Sean-Kenneth-Doherty Sean-Kenneth-Doherty commented May 16, 2026

Summary

  • make time +/- interval and interval + time return a time value instead of an interval
  • evaluate time/interval arithmetic with 24-hour wrapping across Time32 and Time64 units
  • apply interval nanoseconds before reducing back to the output time unit, so negative sub-second intervals wrap correctly for lower-precision time values
  • update arith_time_interval.slt with PostgreSQL-compatible wrapping coverage, including the reported 23:30 + 2 hours case and a Time32(Second) sub-second regression

Fixes #22265.

Rationale for this change

time +/- interval should stay in the 24-hour time-of-day domain. DataFusion currently routes these expressions through Interval(MonthDayNano), which can produce interval values such as 25 hours 30 mins instead of wrapping to 01:30:00.

What changes are included in this PR?

  • Adds explicit coercion signatures for time + interval, interval + time, and time - interval so the result type remains the original time type.
  • Adds physical expression handling for Time32/Time64 plus Interval(MonthDayNano), wrapping arithmetic modulo one day.
  • Keeps whole-day-or-larger interval fields as no-ops for time-of-day wrapping, while applying the nanosecond field before narrowing back to the target time unit.
  • Updates SQLLogic coverage for wrapping and result type behavior.

Are these changes tested?

  • cargo test --profile=ci --test sqllogictests -- arith_time_interval.slt
  • cargo fmt --all --check
  • git diff --check origin/main...HEAD
  • cargo clippy --all-targets --all-features -- -D warnings

Are there any user-facing changes?

Yes. time +/- interval and interval + time now return wrapped time values instead of interval values.

AI-assisted contribution note

I used OpenAI Codex while developing and validating this PR. I reviewed the implementation end-to-end. The main assumption to call out is that for time-of-day arithmetic, interval month/day fields are whole-day-or-larger components and therefore have no effect after modulo-24-hour wrapping; the nanosecond field is applied at full precision before reducing to the specific Time32/Time64 unit.

@github-actions github-actions Bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels May 16, 2026
@Sean-Kenneth-Doherty Sean-Kenneth-Doherty marked this pull request as ready for review May 16, 2026 22:04
@Sean-Kenneth-Doherty
Copy link
Copy Markdown
Author

Validation completed before moving this out of draft:

  • CARGO_TARGET_DIR=/home/sean/.cache/codex-cargo-targets/datafusion-time-interval RUSTFLAGS='-Cdebuginfo=0' cargo test -p datafusion-sqllogictest --test sqllogictests -- arith_time_interval
  • cargo fmt --all --check
  • git diff --check
  • TMPDIR=/home/sean/.cache/codex-tmp CARGO_TARGET_DIR=/home/sean/.cache/codex-cargo-targets/datafusion-time-interval RUSTFLAGS='-Cdebuginfo=0' cargo clippy -p datafusion-physical-expr -p datafusion-expr-common --all-targets --all-features -- -D warnings

Note: the first clippy attempt failed because native C compilation scratch files hit a local /tmp disk quota. Rerunning the same command with TMPDIR on the home filesystem completed successfully.

@Sean-Kenneth-Doherty
Copy link
Copy Markdown
Author

Follow-up pushed after final local review: the arithmetic now wraps in nanoseconds before reducing back to the time unit, which covers negative sub-second intervals for lower-precision time values.

Added regression:

  • arrow_cast('00:00:00', 'Time32(Second)') - interval '500 milliseconds' -> 23:59:59

Reran validation after the follow-up:

  • TMPDIR=/home/sean/.cache/codex-tmp CARGO_TARGET_DIR=/home/sean/.cache/codex-cargo-targets/datafusion-time-interval RUSTFLAGS='-Cdebuginfo=0' cargo test -p datafusion-sqllogictest --test sqllogictests -- arith_time_interval
  • cargo fmt --all --check
  • git diff --check
  • TMPDIR=/home/sean/.cache/codex-tmp CARGO_TARGET_DIR=/home/sean/.cache/codex-cargo-targets/datafusion-time-interval RUSTFLAGS='-Cdebuginfo=0' cargo clippy -p datafusion-physical-expr -p datafusion-expr-common --all-targets --all-features -- -D warnings

@Sean-Kenneth-Doherty
Copy link
Copy Markdown
Author

The PR body is updated with the latest follow-up and validation. Only the labeler workflow has run so far; could a committer please trigger the full CI when convenient?

Latest local validation:

  • cargo test --profile=ci --test sqllogictests -- arith_time_interval.slt
  • cargo fmt --all --check
  • git diff --check origin/main...HEAD
  • cargo clippy --all-targets --all-features -- -D warnings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PostgreSQL compatibility: time + interval should wrap within the 24-hour time domain

1 participant