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

[ahkcs]

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 so the DSL-pushdown and 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 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 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.

Behavior notes (intentional trade-offs)

• Boundary: 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: 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/.

Related

Addresses the aggregate half of #5164. Stacked on #5604 (reuses its ArithmeticException → 4xx routing); the diff will shrink once #5604 merges.

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]

PR Reviewer Guide 🔍

(Review updated until commit f01ed11)

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 narrow method compares a double magnitude against TWO_POW_63 (0x1p63) to detect overflow. However, when value is a BigDecimal that exactly equals Long.MAX_VALUE (2^63 - 1), converting it to double via doubleValue() rounds to exactly 2^63 due to IEEE 754 precision limits. The condition magnitude > TWO_POW_63 then evaluates to false (since 2^63 is not strictly greater than 2^63), so the method proceeds to the BigDecimal branch. There, decimal.min(BigDecimal.valueOf(Long.MAX_VALUE)) clamps the value to Long.MAX_VALUE, which is correct. But if the input BigDecimal is slightly above Long.MAX_VALUE (e.g., 2^63 - 0.5), it still rounds to 2^63 as a double, passes the overflow check, and gets clamped to Long.MAX_VALUE instead of raising an error. This means a sum that genuinely exceeds the BIGINT range by a fractional amount (but rounds to exactly 2^63 in double) will saturate rather than error, which may be intentional per the PR description but could silently hide a small overflow if the input is a BigDecimal with a fractional part. public static long narrow(Object value) { 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) { // Exact for the in-memory (DECIMAL) path; clamps a value that rounds to exactly 2^63. return decimal .min(BigDecimal.valueOf(Long.MAX_VALUE)) .max(BigDecimal.valueOf(Long.MIN_VALUE)) .longValue(); } // double path (pushed-down sum): saturating narrow (JLS 5.1.3) clamps 2^63 to Long.MAX_VALUE. return (long) magnitude; } Possible Issue In castBigintSumResults, the method checks if aggExprList.size() != aggRexList.size() and returns aggRexList unchanged if they differ. However, if the sizes match but the lists are semantically misaligned (e.g., due to an upstream bug or unexpected plan structure), the loop pairs aggExprList.get(i) with aggRexList.get(i) without verifying they correspond to the same aggregate. If a mismatch occurs, the narrowing UDF could be applied to the wrong aggregate result (e.g., wrapping a non-SUM result or missing an actual SUM). The code assumes the lists are always aligned, but no assertion or comment documents this invariant. private static List<RexNode> castBigintSumResults( CalcitePlanContext context, List<UnresolvedExpression> aggExprList, List<RexNode> aggRexList) { if (aggExprList.size() != aggRexList.size()) { return aggRexList; } List<RexNode> result = new ArrayList<>(aggRexList.size()); for (int i = 0; i < aggRexList.size(); i++) { RexNode aggRex = aggRexList.get(i); AggregateFunction aggFunc = extractAggregateFunction(aggExprList.get(i)); if (aggFunc != null && BuiltinFunctionName.ofAggregation(aggFunc.getFuncName()) .filter(name -> name == BuiltinFunctionName.SUM) .isPresent() && isWidenedBigintSumType(aggRex.getType())) { RexNode narrowed = context.rexBuilder.makeCall(PPLBuiltinOperators.CHECKED_LONG_NARROW, aggRex); // Wrapping in the narrowing UDF drops the aggregate's output name; restore it so downstream // projection and the result schema keep the original "sum(...)" column name. result.add( aggRex instanceof RexInputRef ref ? context.relBuilder.alias( narrowed, context.relBuilder.peek().getRowType().getFieldNames().get(ref.getIndex())) : narrowed); } else { result.add(aggRex); } } return result; }

[github-actions]

Persistent review updated to latest commit f01ed11

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

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