Detect long (BIGINT) arithmetic overflow instead of silently wrapping (#5164)#5604
Detect long (BIGINT) arithmetic overflow instead of silently wrapping (#5164)#5604ahkcs wants to merge 2 commits into
Conversation
PR Reviewer Guide 🔍(Review updated until commit 447b958)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 447b958 Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit 2d6e9a1
Suggestions up to commit 2d2ef9f
Suggestions up to commit 7e0a74d
|
7e0a74d to
2d2ef9f
Compare
|
Persistent review updated to latest commit 2d2ef9f |
2d2ef9f to
2d6e9a1
Compare
|
Persistent review updated to latest commit 2d6e9a1 |
2d6e9a1 to
fc2e0f2
Compare
|
Persistent review updated to latest commit fc2e0f2 |
PPL integer arithmetic (`+`, `-`, `*` and the named add/subtract/multiply
functions) inferred `SMALLINT op SMALLINT -> SMALLINT` (and likewise for
TINYINT) because the operators were registered with Calcite's stock
`SqlStdOperatorTable.PLUS/MINUS/MULTIPLY`, whose return-type inference
(`ReturnTypes.NULLABLE_SUM` / `PRODUCT_NULLABLE`) falls through to
`LEAST_RESTRICTIVE` for non-decimal integers.
As a result the product/sum of two narrow-integer columns overflowed the
inferred type on every backend, just differently:
- DataFusion / analytics-engine: silently wraps the i16 result, so e.g.
`eval area = ResolutionWidth * ResolutionHeight | where area > 2000000`
returned 0 rows instead of the matching row.
- Calcite Enumerable engine: throws `ArithmeticException: value out of
range`.
- v2 legacy engine: `ExprShortValue` narrows via `shortValue()`, wrapping.
Fix in the SQL-plugin lowering so all backends are corrected at once: widen
the operands (byte/short -> INTEGER, any int/long -> BIGINT) before applying
the operator. Casting the operands rather than only relabelling the result
type is required, otherwise DataFusion still computes the narrow multiply and
wraps before any outer cast. Non-integral operands (float/double/decimal/
datetime/mixed) are left untouched and defer to Calcite's default inference.
The string-concat `ADD` variant and the DATETIME-DATETIME `SUBTRACT` variant
are unchanged.
mvindex's internal array-index arithmetic now uses the raw Calcite
PLUS/MINUS operators so array indices stay INTEGER for ITEM/ARRAY_SLICE
codegen (the widened result would otherwise be rejected as a long index).
Note: this changes user-visible result column types for integer arithmetic
(int-operand expressions now report bigint), which is the intended trade-off
for overflow-safe, backend-consistent results.
Adds CalcitePPLBuiltinFunctionIT coverage for the short->int and int->bigint
widening tiers and updates the affected logical-plan / Spark-SQL snapshots.
Signed-off-by: Kai Huang <ahkcs@amazon.com>
PPL/SQL long arithmetic (+, -, *) in eval/SELECT expressions silently wrapped on overflow in the Calcite engine (e.g. long_field * 999...9 wrapped to a negative value with HTTP 200). SqlStdOperatorTable.PLUS/MINUS/MULTIPLY generate plain Java +/-/* in the Enumerable code path, which wrap. Rewrite long +/-/* to their overflow-checked variants (CHECKED_PLUS / CHECKED_MINUS / CHECKED_MULTIPLY), which generate Math.addExact etc. and throw ArithmeticException on overflow. The exception is caught in QueryService and surfaced as a 4xx client error instead of wrapping or falling back to V2. Scoped to BIGINT operands only: narrower integer arithmetic (byte/short/int) is widened to a type that cannot overflow before this rewrite runs (PPLFuncImpTable promotes byte/short to INT and any int/long to BIGINT for +/-/*), so long — which has no wider integer type — is the sole remaining overflow case on the Calcite engine. Float/double/decimal follow IEEE 754 / decimal semantics and have no CHECKED_* runtime, so they are left untouched. The rewrite runs after the analytics-engine fork, so only the Calcite path is affected (the DataFusion backend handles its own overflow). The rewrite runs before pushdown so both coordinator-executed and pushed-down (script) arithmetic are checked; PPLAggregateConvertRule, OpenSearchRelOptUtil, and ExtendedRelJson recognize the CHECKED_* kinds so aggregate/sort pushdown and script serialization keep working. Adds a REST yaml test (issues/5164.yml) and updates the affected explain fixtures. Resolves opensearch-project#5164 Signed-off-by: Kai Huang <ahkcs@amazon.com>
fc2e0f2 to
447b958
Compare
|
Persistent review updated to latest commit 447b958 |
Description
long(BIGINT) arithmetic (+,-,*) in PPL/SQLeval/SELECTexpressions silently wrapped on overflow in the Calcite engine — e.g.long_field * 9999999999at the top of the 64-bit range returned a negative value with HTTP 200.SqlStdOperatorTable.PLUS/MINUS/MULTIPLYgenerate plain Java+/-/*in the Enumerable code path, which wrap.This rewrites
long+/-/*to their overflow-checked variants (CHECKED_PLUS/CHECKED_MINUS/CHECKED_MULTIPLY), which generateMath.addExact/subtractExact/multiplyExactand throwArithmeticExceptionon overflow. The exception is caught inQueryServiceand surfaced as a 4xx client error (NonFallbackCalciteException) instead of wrapping or falling back to the V2 engine. (The V2 engine already usesMath.*Exact.)Scope: BIGINT-only
The checked rewrite is gated to
long(BIGINT) operands. Narrower integer arithmetic (byte/short/int) is widened to a type that cannot overflow before this rewrite runs —PPLFuncImpTablepromotesbyte/short→intand anyint/longoperand →longfor+/-/*(#5603) — solong, which has no wider integer type to widen into, is the only remaining overflow case on the Calcite engine.float/double/decimalfollow IEEE 754 / decimal semantics and have noCHECKED_*runtime, so they are left untouched.The rewrite runs after the analytics-engine fork in
convertToCalcitePlan, so only the Calcite/Enumerable path is affected. The DataFusion analytics backend handles its ownlongoverflow (checked arrow kernels, opensearch-project/OpenSearch#22378) — the two engines now converge on erroring forlongoverflow.It runs before pushdown so both coordinator-executed and pushed-down (script) arithmetic are checked.
PPLAggregateConvertRule,OpenSearchRelOptUtil, andExtendedRelJsonrecognize theCHECKED_*kinds so aggregate-pushdown, sort-expression pushdown, and script serialization keep working for pushed-down checked arithmetic.Stacked on #5603
This PR is stacked on #5603 (operand widening) and depends on it: without the widening underneath,
short/intoverflow would slip through un-checked on the Calcite path. The first commit here is #5603's widening; it drops out of this diff once #5603 merges. Together the two PRs close integer-arithmetic overflow on the Calcite engine (#5603 widens the narrow tiers; this errors thelongtier), matching the analytics-engine behavior.Testing
issues/5164.yml(REST)int/shortoverflow → widened correct value (200);longoverflow (+/-/*) → 4xx; normal arithmetic unaffectedCalciteExplainIT(pushdown)CHECKED_*in pushed scriptsCalciteNoPushdownIT(CalciteExplainIT)Related Issues
Resolves #5164. Stacked on #5603. Analytics-engine counterpart: opensearch-project/OpenSearch#22378.
Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.