[BugFix] Detect BIGINT overflow in SUM and fix wrong AVG on the Calcite engine by ahkcs · Pull Request #5612 · opensearch-project/sql

ahkcs · 2026-07-07T21:43:12Z

Description

SUM/AVG over a BIGINT (long) column near 2^63 silently produced wrong results on the Calcite engine:

SUM(long) - the enumerable accumulator is a plain long, so a running sum past 2^63 wrapped to a negative value (e.g. SUM(UserID) returned -5594372458244005145 with HTTP 200).
AVG(long) - AVG reduces to SUM(field)/COUNT(field); the intermediate long sum wrapped the same way, so AVG returned a wrong (often negative) result.

Fix

Applied in the SQL-plugin lowering while preserving the pushdown and in-memory execution paths:

SUM(long) - the aggregate argument (a bare BIGINT column) is summed in DECIMAL so it cannot wrap, and the result is narrowed back to BIGINT via a new CHECKED_LONG_NARROW UDF. Narrowing raises a client error (4xx) when the sum clearly overflowed 2^63, and otherwise saturates. The output type stays bigint.
AVG(long) - the bare BIGINT argument is averaged in DOUBLE, which avoids an intermediate long wrap. The DOUBLE output type is unchanged.

Only bare BIGINT column references are widened; expressions such as sum(field + 1) are left untouched so PPLAggregateConvertRule can still rewrite them to pushdown-friendly form. Narrower integer sums (byte/short/int) already widen to a BIGINT accumulator and cannot overflow, so they are unchanged.

Design

#5604 handles scalar arithmetic because Calcite exposes each operation as a RexCall:

RexCall(PLUS, a, b)
    -> CHECKED_PLUS
    -> Math.addExact(a, b)
    -> BIGINT result or ArithmeticException -> 4xx

SUM is an AggregateCall; its internal additions are not visible PLUS nodes, so the #5604 rewrite cannot intercept them. #5612 instead widens the accumulator and checks the final result:

SUM(BIGINT field)
    -> SUM(CAST(field AS DECIMAL))
    -> no pushdown: exact DECIMAL accumulator
       pushdown: OpenSearch native double accumulator
    -> CHECKED_LONG_NARROW
    -> BIGINT result or ArithmeticException -> 4xx

The DECIMAL path is exact and order-independent. Keeping the standard SUM operator also preserves existing aggregate optimization and pushdown rules. #5612 reuses #5604's ArithmeticException-to-4xx routing, but not its scalar-expression rewrite.

CHECKED_PLUS receives two long operands and performs the addition, so it can use Math.addExact(a, b). CHECKED_LONG_NARROW receives one already-computed DECIMAL/double aggregate result and only converts it back to long; there is no addition left for Math.addExact to perform. Using checked addition would require a separate custom CHECKED_SUM accumulator. It also would not recover precision already lost by OpenSearch's pushed-down double sum.

For AVG, the plan becomes AVG(CAST(field AS DOUBLE)), preventing Calcite's internal SUM/COUNT reduction from first overflowing a long.

Behavior notes (intentional trade-offs)

Pushdown precision: OpenSearch computes pushed-down sums in double. Large in-range sums can therefore lose low-order precision before narrowing; for example, 2^62 + 1 becomes 2^62. Without pushdown, the DECIMAL accumulator returns the exact result.
Boundary: (double) Long.MAX_VALUE rounds to exactly 2^63, indistinguishable from a small overflow. To avoid erroring on a legitimate near-Long.MAX_VALUE sum, only magnitudes strictly beyond 2^63 are treated as overflow.
AVG pushdown: casting the AVG argument to DOUBLE keeps native avg pushdown in most cases, but an avg with a preceding sort can fall back to an in-memory sum/count reduction instead of a pushed-down native avg. The result stays correct; only that narrow case loses aggregate pushdown.

Before / After

Query	Before	After
`stats sum(long_field)` (overflow)	wrong negative value, HTTP 200	HTTP 400
`stats avg(long_field)` (overflow)	wrong negative value	correct `double`
`stats sum(long_field)` (in range)	correct	correct (`bigint`)
`stats sum(Long.MAX)`	correct	correct (no false error)

Testing

CalcitePPLAggregationIT.testSumAvgLongOverflow - overflow -> 4xx, avg correct, in-range and single-Long.MAX sums; runs on both the pushdown path and the no-pushdown path (via CalciteNoPushdownIT).
REST yaml test issues/5164_agg.yml.
Regenerated the affected explain fixtures under expectedOutput/calcite/ and expectedOutput/calcite_no_pushdown/.

Check List

New functionality includes testing.
New functionality has been documented.
Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2026-07-07T21:45:01Z

PR Reviewer Guide 🔍

(Review updated until commit `9eec01c`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Possible Issue The aggregator checks `valuesSource.isFloatingPoint()` and `valuesSource.isBigInteger()` to reject non-integral types, but does not verify that the field is actually signed. If an unsigned long field is passed, the checked addition will misinterpret high unsigned values as negative signed values, producing incorrect overflow detection. This occurs when the field's actual value exceeds Long.MAX_VALUE but is stored as an unsigned long in Lucene. if (valuesSource != null && (valuesSource.isFloatingPoint() \|\| valuesSource.isBigInteger())) { throw new IllegalArgumentException( "checked_long_sum requires a signed integral numeric field"); } Possible Issue The `widenBigintColumnToDouble` method casts a BIGINT `RexInputRef` to DOUBLE to prevent intermediate long overflow in AVG. However, a double cannot represent all 64-bit integers exactly (only up to 2^53). For BIGINT values above 2^53, the cast loses precision, so the average may be slightly inaccurate. This is a known trade-off but should be documented or reconsidered if exact averages are required for large integers. private static RexNode widenBigintColumnToDouble(RexNode field, CalcitePlanContext ctx) { if (field instanceof RexInputRef && field.getType().getSqlTypeName() == SqlTypeName.BIGINT) { RelDataType doubleType = TYPE_FACTORY.createTypeWithNullability( TYPE_FACTORY.createSqlType(SqlTypeName.DOUBLE), field.getType().isNullable()); return ctx.rexBuilder.makeCast(doubleType, field); } return field; } Possible Issue The `createCheckedLongSumAggregation` method throws `AggregateAnalyzerException` if the argument is not a direct `RexInputRef`, preventing pushdown for expressions like `sum(field + 0)`. However, the error message says "CHECKED_LONG_SUM pushdown requires one direct field reference" without explaining why or suggesting an alternative. If the intent is to allow only bare columns, the restriction is correct, but the error could be clearer about the fallback behavior (in-memory execution). private static Pair<AggregationBuilder, MetricParser> createCheckedLongSumAggregation( List<Pair<RexNode, String>> args, String aggName, AggregateBuilderHelper helper) { if (args.size() != 1 \|\| !(args.getFirst().getKey() instanceof RexInputRef)) { throw new AggregateAnalyzerException( "CHECKED_LONG_SUM pushdown requires one direct field reference"); } String field = helper.inferNamedField(args.getFirst().getKey()).getReferenceForTermQuery(); CheckedLongSumAggregationBuilder builder = new CheckedLongSumAggregationBuilder(aggName).field(field); return Pair.of(builder, new CheckedLongSumParser(aggName)); }

github-actions · 2026-07-07T22:24:49Z

Persistent review updated to latest commit f01ed11

github-actions · 2026-07-07T22:25:44Z

PR Code Suggestions ✨

Latest suggestions up to 9eec01c

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Validate input before resource allocation The validation logic should occur before any resource allocation (like `sums` and `seen` arrays). Move this check to the beginning of the constructor to avoid potential resource leaks if validation fails after arrays are initialized. opensearch/src/main/java/org/opensearch/sql/opensearch/aggregation/CheckedLongSumAggregator.java [44-51] if (valuesSource != null && (valuesSource.isFloatingPoint() \|\| valuesSource.isBigInteger())) { throw new IllegalArgumentException( "checked_long_sum requires a signed integral numeric field"); } +if (valuesSource != null) { + sums = context.bigArrays().newLongArray(1, true); + seen = context.bigArrays().newLongArray(1, true); +} Suggestion importance[1-10]: 7 __ Why: Moving validation before resource allocation prevents potential resource leaks if validation fails. The `sums` and `seen` arrays are allocated at lines 49-50, but validation occurs at line 44. If validation fails after allocation, resources may not be properly released.	Medium
	Add negative bucket index validation The bounds check `bucket < seen.size()` may not be sufficient. If `bucket` is negative, it could cause issues. Add a check to ensure `bucket >= 0` before accessing the array to prevent potential index-out-of-bounds errors. opensearch/src/main/java/org/opensearch/sql/opensearch/aggregation/CheckedLongSumAggregator.java [119-121] private boolean hasValue(long bucket) { - return valuesSource != null && bucket < seen.size() && seen.get(bucket) != 0; + return valuesSource != null && bucket >= 0 && bucket < seen.size() && seen.get(bucket) != 0; } Suggestion importance[1-10]: 6 __ Why: Adding a check for `bucket >= 0` improves defensive programming by preventing potential negative index access. However, in the OpenSearch aggregation framework, bucket ordinals are typically non-negative by design, making this a minor improvement.	Low
	Clarify intentional long-to-double conversion The method returns `double` but retrieves a `long` value from `sums`. This could cause precision loss for large long values. Consider whether this conversion is intentional or if the return type should be `long` to preserve exact values. opensearch/src/main/java/org/opensearch/sql/opensearch/aggregation/CheckedLongSumAggregator.java [89-91] public double metric(long owningBucketOrd) { - return hasValue(owningBucketOrd) ? sums.get(owningBucketOrd) : Double.NaN; + return hasValue(owningBucketOrd) ? (double) sums.get(owningBucketOrd) : Double.NaN; } Suggestion importance[1-10]: 3 __ Why: The explicit cast to `double` is already implicit in the return type. While adding an explicit cast improves clarity, the current code is functionally correct and the conversion is intentional for the `metric` interface contract.	Low

Previous suggestions

Suggestions up to commit 0a077e0

Category	Suggestion	Impact
Possible issue	Add null value handling The method doesn't handle `null` values, which could cause a `NullPointerException` when calling `doubleValue()` on a null `Number`. Add a null check at the beginning of the method to return a default value or throw a more descriptive exception. core/src/main/java/org/opensearch/sql/expression/function/udf/CheckedLongNarrowFunction.java [55-69] public static long narrow(Object value) { + if (value == null) { + throw new IllegalArgumentException("Cannot narrow null value to BIGINT"); + } double magnitude = ((Number) value).doubleValue(); if (magnitude > TWO_POW_63 \|\| magnitude < -TWO_POW_63) { throw new ArithmeticException("BIGINT overflow in SUM"); } if (value instanceof BigDecimal decimal) { return decimal .min(BigDecimal.valueOf(Long.MAX_VALUE)) .max(BigDecimal.valueOf(Long.MIN_VALUE)) .longValue(); } return (long) magnitude; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the `narrow` method doesn't handle `null` values, which could cause a `NullPointerException`. Adding a null check improves robustness, though the actual impact depends on whether nulls can reach this method in practice (the UDF uses `NullPolicy.ANY` which may handle nulls before reaching this method).	Medium

Suggestions up to commit 31bbdf2

Category	Suggestion	Impact
Possible issue	Add null value handling The `narrow` method doesn't handle `null` values, which could cause a `NullPointerException` when calling `doubleValue()` on a null `Number`. Add a null check at the beginning of the method to return a default value or throw a more descriptive exception. core/src/main/java/org/opensearch/sql/expression/function/udf/CheckedLongNarrowFunction.java [55-69] public static long narrow(Object value) { + if (value == null) { + throw new IllegalArgumentException("Cannot narrow null value to BIGINT"); + } double magnitude = ((Number) value).doubleValue(); if (magnitude > TWO_POW_63 \|\| magnitude < -TWO_POW_63) { throw new ArithmeticException("BIGINT overflow in SUM"); } if (value instanceof BigDecimal decimal) { return decimal .min(BigDecimal.valueOf(Long.MAX_VALUE)) .max(BigDecimal.valueOf(Long.MIN_VALUE)) .longValue(); } return (long) magnitude; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the `narrow` method lacks null handling, which could cause a `NullPointerException`. However, the score is moderate because the context of how this method is called (within aggregate operations) may already guarantee non-null values, making this a defensive programming improvement rather than a critical bug fix.	Medium

Suggestions up to commit 4d2c6fc

Category	Suggestion	Impact
Possible issue	Fix asymmetric overflow detection The overflow check uses `magnitude > TWO_POW_63` which will never trigger for `Long.MIN_VALUE` (-2^63) since its magnitude is exactly 2^63, not greater. This asymmetry means negative overflows beyond -2^63 won't be detected. Use `Math.abs(magnitude) > TWO_POW_63` or separate checks for positive/negative bounds. core/src/main/java/org/opensearch/sql/expression/function/udf/CheckedLongNarrowFunction.java [55-69] public static long narrow(Object value) { double magnitude = ((Number) value).doubleValue(); - if (magnitude > TWO_POW_63 \|\| magnitude < -TWO_POW_63) { + if (Math.abs(magnitude) > TWO_POW_63) { throw new ArithmeticException("BIGINT overflow in SUM"); } if (value instanceof BigDecimal decimal) { return decimal .min(BigDecimal.valueOf(Long.MAX_VALUE)) .max(BigDecimal.valueOf(Long.MIN_VALUE)) .longValue(); } return (long) magnitude; } Suggestion importance[1-10]: 2 __ Why: The suggestion misunderstands the overflow check logic. The condition `magnitude > TWO_POW_63 \|\| magnitude < -TWO_POW_63` correctly checks both positive and negative bounds since `magnitude` is a signed `double`. The value `-TWO_POW_63` is exactly `-2^63`, and values less than this (more negative) will be caught. The check is symmetric and correct as written.	Low

Suggestions up to commit f01ed11

Category	Suggestion	Impact
General	Strengthen type matching to avoid false positives The method assumes that any DECIMAL with scale 0 and max precision is a widened BIGINT sum, but this could incorrectly match user-defined DECIMAL columns that happen to have the same characteristics. Consider adding additional context checks (e.g., verifying the aggregate function is SUM) to avoid false positives. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1870-1874] private static boolean isWidenedBigintSumType(RelDataType type) { + // Only match the specific DECIMAL type created by SUM(bigint) widening return type.getSqlTypeName() == SqlTypeName.DECIMAL && type.getScale() == 0 - && type.getPrecision() == OpenSearchTypeSystem.MAX_PRECISION; + && type.getPrecision() == OpenSearchTypeSystem.MAX_PRECISION + && !type.isNullable(); // Widened types preserve nullability; add more checks if needed } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about potential false positives, but the proposed solution (checking `!type.isNullable()`) is incorrect because widened types do preserve nullability (as the comment in the suggestion itself notes). The method is called only within `castBigintSumResults`, which already verifies the aggregate function is `SUM`, providing sufficient context. The concern is minor and the fix is not appropriate.	Low

penghuo · 2026-07-14T17:29:10Z

+### Overflow behavior
+
+Integer and long arithmetic operations (`+`, `-`, `*`) in `eval` expressions detect overflow and return an error instead of silently wrapping. For example, `eval x = int_field + 1` where `int_field` is `2147483647` (integer max) returns an error rather than `-2147483648`. Floating-point (`float`, `double`) arithmetic follows IEEE 754 and does not produce overflow errors.
+


call-out alg difference between pushdown enabled vs disabled?

Two docs indexed: v = 4611686018427387904 (2^62) and v = 1. True sum = 4611686018427387905 (fits BIGINT exactly).

The no-pushdown PPL path (via widenBigintColumnToDecimal + CHECKED_LONG_NARROW) returns the exact bigint. The pushdown path silently loses precision because OpenSearch computes the sum in double, and 2^62 + 1 isn't representable

Added this distinction to the overflow documentation.

github-actions · 2026-07-16T18:17:57Z

Persistent review updated to latest commit 4d2c6fc

github-actions · 2026-07-16T18:21:42Z

Persistent review updated to latest commit 31bbdf2

SUM and AVG over a BIGINT (long) column near 2^63 silently produced wrong results on the Calcite engine: - SUM(long): the enumerable accumulator is a plain long, so a running sum past 2^63 wrapped to a negative value (e.g. SUM(UserID) returned -5594372458244005145 instead of erroring). - AVG(long): AVG reduces to SUM(field)/COUNT(field); the intermediate long SUM wrapped the same way, so AVG returned a wrong (often negative) result. Fix, applied in the SQL-plugin lowering so both the DSL-pushdown and the in-memory (no-pushdown) paths behave identically: - SUM(long): the aggregate argument (a bare BIGINT column) is summed in DECIMAL so it cannot wrap, and the result is narrowed back to BIGINT with CHECKED_LONG_NARROW. Narrowing errors (HTTP 4xx) when the sum clearly overflowed 2^63, and otherwise saturates. The output type stays bigint. - AVG(long): the bare BIGINT argument is averaged in DOUBLE, which holds the true average without an intermediate long wrap. The DOUBLE output type is unchanged. Only bare BIGINT column references are widened; expressions such as sum(field + 1) are left untouched so PPLAggregateConvertRule can still rewrite them to pushdown-friendly form. Narrower integer sums (byte/short/int) already widen to a BIGINT accumulator and cannot overflow, so they are unchanged. Boundary note: OpenSearch computes pushed-down sums in double, and (double) Long.MAX_VALUE rounds to exactly 2^63, indistinguishable from a small overflow. To avoid erroring on a legitimate near-Long.MAX_VALUE sum, only magnitudes strictly beyond 2^63 are treated as overflow; a genuine overflow lands far outside that boundary, and the in-memory path detects it exactly. AVG pushdown note: casting the AVG argument to DOUBLE keeps native avg pushdown in most cases, but an AVG with a preceding sort can fall back to an in-memory sum/count reduction instead of a pushed-down native avg. The result stays correct; only that narrow case loses aggregate pushdown. Adds CalcitePPLAggregationIT coverage (overflow -> 4xx, avg correct, in-range and Long.MAX sums), a REST yaml test (issues/5164_agg.yml), and regenerates the affected explain fixtures. Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-07-16T20:21:53Z

Persistent review updated to latest commit 0a077e0

penghuo · 2026-07-17T16:38:33Z

+     * are left as-is so {@link org.opensearch.sql.calcite.plan.rule.PPLAggregateConvertRule} can
+     * still rewrite them to pushdown-friendly {@code sum(field) OP literal} form.
+     */
+    void registerSumOperator() {


#5604 use Math.addExact to detect overflow. Why Sum choose another approach?

#5604 handles scalar arithmetic such as a + b. Calcite represents that operation as a visible RexCall(PLUS, a, b), so we can replace it with CHECKED_PLUS, which executes using Math.addExact.

SUM is represented as an AggregateCall; its repeated additions happen internally inside Calcite’s aggregate accumulator, so there are no individual PLUS nodes for #5604’s rewrite to replace. Using Math.addExact would require a custom aggregate implementation, would not apply to native OpenSearch pushdown, and could report an order-dependent intermediate overflow even when the final mathematical sum fits.

Therefore, the no-pushdown path accumulates BIGINT values in a wider DECIMAL, then checks and narrows the final result back to BIGINT. Keeping the standard SUM operator also preserves the existing pushdown optimizations.

Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-07-20T23:02:39Z

Persistent review updated to latest commit 9eec01c

ahkcs requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners July 7, 2026 21:43

ahkcs added the bugFix label Jul 7, 2026

ahkcs force-pushed the fix/sum-avg-bigint-overflow branch from 634d9b6 to f01ed11 Compare July 7, 2026 22:22

Swiddis approved these changes Jul 10, 2026

View reviewed changes

penghuo reviewed Jul 14, 2026

View reviewed changes

ahkcs force-pushed the fix/sum-avg-bigint-overflow branch from f01ed11 to 4d2c6fc Compare July 16, 2026 18:16

ahkcs force-pushed the fix/sum-avg-bigint-overflow branch from 4d2c6fc to 31bbdf2 Compare July 16, 2026 18:20

ahkcs force-pushed the fix/sum-avg-bigint-overflow branch from 31bbdf2 to 0a077e0 Compare July 16, 2026 20:20

penghuo reviewed Jul 17, 2026

View reviewed changes

Use checked long accumulator for BIGINT SUM

9eec01c

Signed-off-by: Kai Huang <ahkcs@amazon.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[BugFix] Detect BIGINT overflow in SUM and fix wrong AVG on the Calcite engine#5612

[BugFix] Detect BIGINT overflow in SUM and fix wrong AVG on the Calcite engine#5612
ahkcs wants to merge 2 commits into
opensearch-project:mainfrom
ahkcs:fix/sum-avg-bigint-overflow

ahkcs commented Jul 7, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 7, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 7, 2026

Uh oh!

github-actions Bot commented Jul 7, 2026 •

edited

Loading

Uh oh!

penghuo Jul 14, 2026

Uh oh!

ahkcs Jul 17, 2026

Uh oh!

github-actions Bot commented Jul 16, 2026

Uh oh!

github-actions Bot commented Jul 16, 2026

Uh oh!

github-actions Bot commented Jul 16, 2026

Uh oh!

penghuo Jul 17, 2026

Uh oh!

ahkcs Jul 17, 2026

Uh oh!

github-actions Bot commented Jul 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		### Overflow behavior

		Integer and long arithmetic operations (`+`, `-`, `*`) in `eval` expressions detect overflow and return an error instead of silently wrapping. For example, `eval x = int_field + 1` where `int_field` is `2147483647` (integer max) returns an error rather than `-2147483648`. Floating-point (`float`, `double`) arithmetic follows IEEE 754 and does not produce overflow errors.

Uh oh!

Conversation

ahkcs commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Fix

Design

Behavior notes (intentional trade-offs)

Before / After

Testing

Related

Check List

Uh oh!

github-actions Bot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 9eec01c)

Uh oh!

github-actions Bot commented Jul 7, 2026

Uh oh!

github-actions Bot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

penghuo Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

ahkcs Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 16, 2026

Uh oh!

github-actions Bot commented Jul 16, 2026

Uh oh!

github-actions Bot commented Jul 16, 2026

Uh oh!

penghuo Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

ahkcs Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ahkcs commented Jul 7, 2026 •

edited

Loading

github-actions Bot commented Jul 7, 2026 •

edited

Loading

(Review updated until commit `9eec01c`)

github-actions Bot commented Jul 7, 2026 •

edited

Loading