fix: unify checked arithmetic and overflow semantics#25846
Conversation
Unify overflow checking across SQL arithmetic execution kernels to match MySQL 8.4 strict-mode behavior (ERROR 1690): - Merge 24 per-type integer overflow helpers into 6 generic checked operations; fix the signed-subtraction check that missed the v1 - MinInt wraparound (e.g. 0 - (-9223372036854775808)). - Add float32/float64 overflow checks for -, *, / (finite operands producing Inf now raise out-of-range, matching + which already had the check). - DIV: float operands now truncate through a checked float64 quotient; any quotient with magnitude >= 2^63 raises out-of-range, including MinInt64 DIV 1/-1, since MySQL computes DIV on unsigned magnitudes (verified against MySQL 8.4.8). - Map decimal ErrInvalidInput to ErrOutOfRange at the SQL arithmetic execution boundary. - specialTemplateForDivFunction now supports error-returning kernels. - Add characterization tests for all 8 integer type boundaries, float overflow, DIV/MOD boundaries, and div/mod template branches. Division-by-zero semantics (SQL-mode controlled NULL vs error) are unchanged and remain distinct from overflow errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- specialTemplateForDivFunction's vector/const branch only checked p1.WithAnyNullValue() and ignored rsAnyNull, so rows masked by selectList (e.g. short-circuited by CASE/IF) were still evaluated; with error-returning kernels this raised spurious overflow errors (e.g. a masked 1e308 / 1e-308 row). Add the rsAnyNull check, matching the const/vector branch, the basic case, and the mod template. Add regression tests covering divFn vector/const, integerDivFn vector/const, and divFn vector/vector with a masked overflow row (verified to fail without the fix). - Regenerate func_div_by_zero.result via mo-tester: MinInt64 DIV 1 now raises ERROR 1690 per MySQL 8.4 unsigned-magnitude DIV semantics instead of returning MinInt64. Audited the rest of test/distributed: uint64_div_overflow's overflow cases are already behind @bvt:issue and MaxUint64 DIV 1 already expects an error; no other case pins the old behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
integerDivSigned/integerDivUnsigned iterated over every row regardless of selectList, so rows short-circuited by CASE/IF were still evaluated: the MinInt64 DIV +/-1 check, the uint64 quotient > MaxInt64 check, and the strict-mode division-by-zero check could all raise errors for rows that must not be evaluated (the div-by-zero case predates this PR). Extract maskUnselectedRows to mark masked rows NULL up front (returning early when all rows are masked) and skip them in the per-row loops, matching the behavior of the template kernels. The template kernels are unaffected: opBinaryFixedFixedToFixedWithErrorCheck, the mod template, and decimalBatchArith (whose kernels skip rsnull rows) all already handle selectList. Add regression tests driving the real integer kernels: masked MinInt64 DIV 1 (vector/const), MinInt64 DIV -1 (vector/vector), MaxUint64 DIV 1 (vector/vector), and a masked zero divisor under forced strict mode; all verified to fail without the fix. Clarify that the earlier float64 DIV case covers the float branch only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Regenerate TimeToInt.result and decimal.result via mo-tester: decimal arithmetic overflow now raises ERROR 1690 (out of range) instead of the internal "invalid input" error, per the ErrInvalidInput -> ErrOutOfRange mapping at the SQL arithmetic execution boundary. This fixes the 9 e2e BVT failures in CI (1 in TimeToInt.sql, 8 in decimal.test). The regenerated files also refresh checkpoint metadata lines (display widths) to match current server output; those lines are not compared by mo-tester. Both files verified locally with mo-tester at 100%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
There is still one blocking select-list correctness gap in the Decimal128 addition path.
P1 - Preserve mask-derived NULLs after Decimal128 addition (pkg/sql/plan/function/arithmetic.go:240)
decimalBatchArith now correctly adds unselected rows to the result NULL bitmap, but the Decimal128-result special case in plusFn unconditionally calls result.GetResultVector().GetNulls().Reset() whenever both input vectors contain no NULLs. That condition says nothing about select-list eligibility, so it erases the NULLs just established for masked CASE/IF rows.
I reproduced this at the current head bc96aa17c9: two non-NULL Decimal128 vectors, FunctionSelectList{AnyNull: true, SelectList: []bool{true, false}}, and a two-row result. plusFn returns nil, but result.GetNulls().Contains(1) is false; row 1 is exposed as a valid value although it was not selected.
Please remove the caller-side bitmap reset or make it preserve all result NULL sources, and add Decimal128 + partial-mask coverage (including vector/vector and const/vector or vector/const). The underlying invariant is that result validity is the union of input NULLs and execution-ineligible rows; it cannot be reconstructed from input NULL state alone.
The latest head does close the previously reported masked-overflow and stale-tail issues, and the current CI is green. This remaining case is absent from the matrix: the addition test uses Decimal64, while Decimal128 is covered only for /, DIV, and MOD.
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
No blocking correctness regression found. One non-blocking performance regression remains; focused tests were blocked by missing CGo headers.
P2 - Avoid unconditional null-bitmap scan for decimal batches (pkg/sql/plan/function/baseTemplate.go:427)
decimalBatchArith calls hasEvaluableRows even when there are no nulls or select-list masks. This adds an O(length) bitmap scan before the decimal kernel, which already scans the batch, for decimal +, -, *, /, DIV, and MOD. Add an empty-null-bitmap fast path or make the scan conditional.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Re-reviewed current head. Decimal128 checked arithmetic now preserves the output NULL mask for vector/vector, constant/vector, and vector/constant paths. I found no remaining blocker.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Re-reviewed current head. Decimal128 checked arithmetic now preserves the output NULL mask for vector/vector, constant/vector, and vector/constant paths. I found no remaining blocker.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed current head e847902 from first principles. The previously blocking Decimal128 mask-loss issue is fixed, and row eligibility is now consistently enforced before overflow/division-by-zero checks across integer, float, and decimal arithmetic. The test matrix covers type families, all const/vector shapes, NULL and selection masks, strict/non-strict modes, stale select-list tails, and result reuse. I also verified that hasEvaluableRows does not add an O(n) cost on the normal no-NULL/no-mask decimal path: Nulls.IsEmpty is an O(1) bitmap-count check. A synthetic merge with latest main was conflict-free; go build, go vet, the full pkg/sql/plan/function test suite, focused race tests, and CI all pass. No blocking correctness, performance, concurrency, ownership, or unbounded-growth issue remains.
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
No blocking or non-blocking regression found. Reviewed arithmetic semantics, overflow boundaries, null/select-list reuse, decimal error mapping, type resolution, and updated BVT baselines. git diff --check passes. Focused Go tests could not compile because this checkout lacks required MatrixOne CGo artifacts/headers.
Merge Queue Status
This pull request spent 3 hours 56 minutes 49 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #25846 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Tick the box to put this pull request back in the merge queue (same as
|
6 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #25754
What this PR does / why we need it:
Unifies checked arithmetic and overflow semantics across the SQL arithmetic execution kernels to match MySQL 8.4 strict-mode behavior (ERROR 1690), as required by the subtask of #25705.
Integer arithmetic
addInt8WithOverflowCheck...mulUint64WithOverflowCheck) into 6 generic checked operations constrained bysignedInt/unsignedInt. Error messages (type name and(%d op %d)format) are byte-for-byte identical to the old helpers.v1 > 0and therefore missed thev1 - MinIntwraparound (e.g.0 - (-9223372036854775808)silently wrapped; MySQL raises ERROR 1690).Float arithmetic
-,*,/: finite operands producing Inf now raise out-of-range, consistent with+which already had the check.DIV
int64(v1/v2)silently truncated, soSELECT 1e308 DIV 1returned garbage instead of erroring.MinInt64 DIV 1andMinInt64 DIV -1now raise out-of-range. MySQL computes DIV on unsigned magnitudes, so any quotient with magnitude 2^63 errors even when the signed result would fit (verified against MySQL 8.4.8).Decimal
ErrInvalidInputfrom decimal kernels toErrOutOfRangeat the SQL arithmetic execution boundary.Infrastructure
specialTemplateForDivFunctionnow accepts error-returning kernels.Tests
All boundary assertions were validated against a local MySQL 8.4.8 instance:
arithmetic_int_overflow_test.go: boundary matrix for all 8 integer types (+, -, *), including0 - MinIntandMinInt * -1.arithmetic_float_overflow_test.go: float overflow helpers and decimal error mapping.arithmetic_overflow_boundary_test.go: DIV int64/uint64/float boundaries, MOD boundaries (MinInt64 MOD -1 = 0, matching MySQL), and div/mod template const/vector/null branch coverage.Known deviations documented in test comments (out of scope here):
MaxUint64 DIV 1: MySQL returns an UNSIGNED BIGINT; MO's DIV result type is fixed at signed int64, so it errors. Result type inference belongs to [Subtask]: add numeric resolver and prepared expression context #25751.🤖 Generated with Claude Code