fix(plan): support INT + INTERVAL / INT - INTERVAL arithmetic - #24441
Conversation
The binder's INTERVAL rewrite for + and - operators only covered date/datetime/timestamp/varchar types, leaving INT columns (common for YYYYMMDD-format date storage) to fall through to operator dispatch which has no INT-INTERVAL overload. Add T_int32 and T_int64 rewrite branches that route INT-INTERVAL expressions to date_sub/date_add, plus new overloads with DateIntSub and DateIntAdd implementations that convert INT<->Date to preserve INT return type (YYYYMMDD format) for type-compatible BETWEEN usage. Fixes matrixorigin#24412. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ithmetic Merge upstream/main into fix/int-interval-arithmetic
|
@aunjgr All three review points from your May 18 review have been addressed in follow-up commits:
PTAL when you have a moment, thanks. |
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Review: INT and INTERVAL arithmetic
Request Changes
The prior correctness fixes look mostly addressed, but the BVT coverage is still missing the critical reversed-order paths.
-
Reversed INTERVAL + INT coverage is only constant-only. The implementation has distinct binder branches for reversed INTERVAL + int32/int64, but there is no non-constant reversed test such as INTERVAL 7 DAY + date_id or INTERVAL 7 DAY + max(date_id). The INT-left runtime path is covered, but the reversed non-constant binder path is not.
-
Reversed sub-day rejection is untested. The regression coverage checks INT + INTERVAL and INT - INTERVAL sub-day cases, but not the combined prior failure mode: INTERVAL 1 HOUR + CAST(20260515 AS INT). Since reversed-order support uses separate guarded binder logic, please add a negative BVT proving reversed sub-day arithmetic errors instead of truncating.
Please add BVT coverage for reversed-order column/aggregate execution and reversed sub-day rejection before merging.
Add non-constant reversed-order tests (column and aggregate) and reversed sub-day rejection tests requested by reviewer. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@aunjgr Added the missing BVT coverage:
Both paths are now covered in . |
|
@aunjgr Added the missing BVT coverage from your May 20 review:
PTAL thanks. |
Merge Queue Status
This pull request spent 1 hour 1 minute 22 seconds in the queue, including 1 hour 58 seconds running CI. Required conditions to merge
|
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #24412
What this PR does / why we need it:
Fix
INT - INTERVALandINT + INTERVALarithmetic not supported when date columns are stored as INT (YYYYMMDD format).The binder's INTERVAL rewrite for
+and-operators only covered date/datetime/timestamp/varchar types. INT columns storing dates in YYYYMMDD format would fall through to operator dispatch which has no INT-INTERVAL overload, producing "invalid argument operator -, bad value [INT INTERVAL]".Changes
base_binder.go— AddT_int32andT_int64branches to+and-operator INTERVAL rewrite chains, routing them todate_add/date_sub.func_binary.go— AddDateIntSubandDateIntAddexecution functions withintToDate/dateToInthelpers that convert between YYYYMMDD-format INT and internaltypes.Date. Return type stays INT to preserve type compatibility for BETWEEN and other comparisons.list_builtIn.go— RegisterT_int32overloads forDATE_SUBandDATE_ADDreturningT_int32.Before / After
🤖 Generated with Claude Code