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

[ahkcs]

Description

SUM/AVG over integral columns near the BIGINT boundary could silently return wrong results on the Calcite engine:

• Local SUM could wrap its running long accumulator.
• Local AVG(BIGINT) could overflow its intermediate long sum before division.
• Native OpenSearch aggregation uses double, so pushed-down sums can lose low-order precision for large values.

Fix

• Integral SUM inputs (TINYINT, SMALLINT, INTEGER, and BIGINT) use CHECKED_LONG_SUM.
• With pushdown disabled, CheckedLongSumAggFunction calls Math.addExact during every accumulation step.
• CHECKED_LONG_SUM reports SqlKind.SUM, preserving SUM planner rewrites and native pushdown.
• Pushdown uses OpenSearch's existing native sum(field) or scripted sum(expression) aggregation. No new OpenSearch aggregation is registered.
• The pushed result is range-checked and narrowed to BIGINT; backend precision already lost in double cannot be recovered.
• Bare AVG(BIGINT) uses a reflective double sum + long count aggregate locally and reports SqlKind.AVG, allowing pushdown to remain native avg(field) without a cast-generated script.
• FLOAT/DOUBLE arithmetic retains standard IEEE 754 behavior.

Architecture

PPL: stats sum(value), avg(value)
                 |
                 v
        Type-aware operator selection
                 |
       +---------+----------+
       |                    |
 pushdown disabled     pushdown enabled
       |                    |
       v                    v
 Calcite Enumerable     Planner sees standard kinds
 execution             CHECKED_LONG_SUM -> SqlKind.SUM
                       BIGINT_AVG      -> SqlKind.AVG
       |                    |
       |                    v
       |             OpenSearch AggregateAnalyzer
       |                    |
       |             +------+----------------+
       |             |                       |
       |        SUM(field)              SUM(expression)
       |        native field sum        native scripted sum
       |             |
       |        AVG(field)
       |        native field avg
       |
       +-- integral SUM
       |   Math.addExact(runningSum, value)
       |   -> exact long or overflow error
       |
       +-- bare BIGINT AVG
           double sum + long count
           -> sum / count

This separates execution implementation from planner identity: Calcite invokes the reflective checked aggregates locally, while SqlKind.SUM/AVG lets planner rules retain native OpenSearch pushdown.

Precision behavior

The two execution paths intentionally have different precision guarantees:

No pushdown:
  long accumulation with Math.addExact
  -> exact while every intermediate result fits in BIGINT

Pushdown:
  native OpenSearch double accumulation
  -> preserves existing DSL performance and semantics
  -> can lose low-order bits for large integers

Example:

Values: 2^62 and 1
True sum: 4611686018427387905

No pushdown: 4611686018427387905
Pushdown:    may return 4611686018427387904

At that magnitude, adjacent long values cannot all be represented by double. CheckedLongSumParser can detect a non-finite or out-of-BIGINT-range result, but it cannot reconstruct bits already rounded by the backend.

AVG returns DOUBLE on both paths, so it follows double precision semantics. The local BIGINT-specific implementation prevents long overflow; it does not provide arbitrary-precision averaging.

Overflow semantics

Local overflow is checked during accumulation, not only against the final mathematical result:

Values: Long.MAX_VALUE, 1, -1

0 + Long.MAX_VALUE = Long.MAX_VALUE
Long.MAX_VALUE + 1 = overflow -> error

The later -1 is not processed. A sequence whose final mathematical sum fits can therefore fail when an intermediate running sum exceeds the BIGINT range. This fail-fast behavior is intentional and follows Math.addExact accumulation semantics.

Before / After

Query/path	Before	After
Local integral SUM overflow	wrapped/wrong value	overflow error
Local integral SUM in range	could wrap during accumulation	exact BIGINT
Pushed integral SUM	native double semantics	unchanged native double semantics
Local AVG(BIGINT) near long overflow	wrong result	valid DOUBLE result
Pushed AVG(BIGINT)	cast-generated script in an intermediate revision	native avg(field)
FLOAT/DOUBLE SUM	IEEE 754	unchanged

Testing

• Covered exact local sums, Long.MAX_VALUE, intermediate overflow, null handling, and all integral input types.
• Covered local BIGINT AVG without intermediate long overflow.
• Covered native field AVG, native field SUM, and scripted SUM request generation.
• Verified pushdown plans retain AGGREGATION-> and bare field AVG no longer creates a script.
• Updated Calcite, no-pushdown, Big5, ClickBench, and explain fixtures.
• Verified the focused unit suites and all 265 CalciteExplainIT tests with pushdown disabled.

Related

Addresses the aggregate half of #5164 and builds on #5604's checked scalar arithmetic.

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 481819a)

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 narrowing conversion from double to long saturates at Long.MAX_VALUE when the double equals 2^63 (0x1p63), which is ambiguous because Long.MAX_VALUE itself rounds to 2^63 in double. The check at line 42 rejects values strictly greater than 2^63, but a native sum that produces exactly 2^63 (e.g., from rounding) will pass the check and saturate to Long.MAX_VALUE, potentially masking an overflow. This occurs when the true sum is slightly above Long.MAX_VALUE but rounds to 2^63 in double precision. static long narrow(double value) { if (!Double.isFinite(value) || value > TWO_POW_63 || value < -TWO_POW_63) { throw new ArithmeticException("BIGINT overflow in SUM"); } return (long) value; Possible Issue The AVG(BIGINT) handler casts the field to DOUBLE only when it is a RexInputRef. If the field is a more complex expression (e.g., a CAST or arithmetic operation that still has BIGINT type), the cast is skipped and the standard avg is used, which may still produce an intermediate long sum that overflows. This inconsistency means overflow protection is incomplete. if (field instanceof RexInputRef && field.getType().getSqlTypeName() == SqlTypeName.BIGINT) { return ctx.relBuilder .aggregateCall(PPLBuiltinOperators.BIGINT_AVG, field) .distinct(distinct); } return ctx.relBuilder.avg(distinct, null, field); },

[github-actions]

Persistent review updated to latest commit f01ed11

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 481819a

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Reject ambiguous 2^63 boundary value The boundary check value > TWO_POW_63 allows value == TWO_POW_63 (which equals 2^63) to pass through. However, 2^63 exceeds Long.MAX_VALUE (which is 2^63 - 1). When cast to long, Java's narrowing conversion saturates it to Long.MAX_VALUE, masking the ambiguity mentioned in the class comment. Consider using value >= TWO_POW_63 to reject this ambiguous boundary case explicitly. opensearch/src/main/java/org/opensearch/sql/opensearch/response/agg/CheckedLongSumParser.java [41-46] static long narrow(double value) { - if (!Double.isFinite(value) || value > TWO_POW_63 || value < -TWO_POW_63) { + if (!Double.isFinite(value) || value >= TWO_POW_63 || value < -TWO_POW_63) { throw new ArithmeticException("BIGINT overflow in SUM"); } return (long) value; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that 2^63 exceeds Long.MAX_VALUE and should be rejected. However, the class comment explicitly states that "A double at the positive BIGINT boundary is also ambiguous because Long.MAX_VALUE rounds to 2^63; Java's narrowing conversion saturates that value to Long.MAX_VALUE." This indicates the current behavior is intentional, treating the ambiguous boundary case by saturating to Long.MAX_VALUE rather than throwing an error. The suggestion would change this documented behavior.	Medium
General	Clarify overflow detection limitations The documentation states that overflow detection may fail when a small overflow rounds to a BIGINT boundary, but this creates ambiguity about when users can rely on overflow errors. Clarify whether this is a known limitation or if there are specific value ranges where overflow detection is guaranteed to work correctly. docs/user/ppl/functions/expressions.md [20] -- With `plugins.calcite.pushdown.enabled=true` (the default), OpenSearch uses its native double-based `sum`, then the result is checked and narrowed to `BIGINT`. Large in-range sums can lose low-order precision. A small overflow that rounds to a signed `BIGINT` boundary can also be indistinguishable from an in-range sum and may saturate at the boundary instead of returning an error. +- With `plugins.calcite.pushdown.enabled=true` (the default), OpenSearch uses its native double-based `sum`, then the result is checked and narrowed to `BIGINT`. Large in-range sums can lose low-order precision. Note: A small overflow that rounds exactly to a signed `BIGINT` boundary (e.g., `Long.MIN_VALUE` or `Long.MAX_VALUE`) may saturate at the boundary instead of returning an error due to floating-point rounding limitations. Suggestion importance[1-10]: 6 __ Why: The suggestion improves clarity by explicitly mentioning Long.MIN_VALUE and Long.MAX_VALUE as boundary cases and explaining the floating-point rounding limitation. However, the improvement is modest as the original text already conveys the core issue. The 'improved_code' adds helpful specificity but doesn't fundamentally change the meaning.	Low
	Verify integral type check covers casts The checkedLongSum decision is based solely on field.getType().getSqlTypeName(), which may not account for expressions that produce integral results from non-integral inputs (e.g., CAST(double_field AS BIGINT)). Verify that the type check correctly identifies all cases requiring overflow protection, or document the limitation. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [1617-1631] void registerSumOperator() { SqlOperandTypeChecker innerTypeChecker = extractTypeCheckerFromUDF(SqlStdOperatorTable.SUM); PPLTypeChecker typeChecker = wrapSqlOperandTypeChecker(innerTypeChecker, SUM.name(), true); AggHandler handler = (distinct, field, argList, ctx) -> { List<RexNode> newArgList = argList.stream().map(PlanUtils::derefMapCall).collect(Collectors.toList()); + // Check if the field's SQL type is integral; note this may not cover all cast scenarios boolean checkedLongSum = isIntegral(field.getType().getSqlTypeName()); SqlAggFunction sumOperator = checkedLongSum ? PPLBuiltinOperators.CHECKED_LONG_SUM : SqlStdOperatorTable.SUM; return UserDefinedFunctionUtils.makeAggregateCall( sumOperator, List.of(field), newArgList, ctx.relBuilder); }; register(SUM, handler, typeChecker); } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about whether the type check correctly identifies all cases requiring overflow protection. However, it only adds a comment without providing a concrete solution or demonstrating that the current implementation is incorrect. The concern is speculative and would require verification through testing or code analysis.	Low
	Clarify intentional precision loss in sum The sum field is a double, and adding a long value directly may lose precision for large values near Long.MAX_VALUE. While this is acceptable for averaging, consider documenting this behavior or using a more explicit cast to clarify the intentional precision trade-off. core/src/main/java/org/opensearch/sql/calcite/udf/udaf/BigintAvgAggFunction.java [15-20] public static Accumulator add(Accumulator accumulator, Long value) { if (value != null) { - accumulator.sum += value; + accumulator.sum += (double) value; accumulator.count++; } return accumulator; } Suggestion importance[1-10]: 3 __ Why: The explicit cast to double is redundant since sum is already a double field and Java automatically widens long to double in the addition. While the suggestion aims to clarify intent, it adds no functional value and the existing code is already clear in its behavior.	Low

---

Previous suggestions

Suggestions up to commit 2166002

Category	Suggestion	Impact
Possible issue	Tighten overflow boundary check The boundary check value > TWO_POW_63 allows values slightly above Long.MAX_VALUE (e.g., 2^63) to pass through, which then saturate to Long.MAX_VALUE during the cast. This creates ambiguity: a legitimate sum of exactly Long.MAX_VALUE becomes indistinguishable from an overflowed sum that rounded to 2^63. Consider using a stricter threshold or documenting this edge case behavior explicitly. opensearch/src/main/java/org/opensearch/sql/opensearch/response/agg/CheckedLongSumParser.java [42-46] static long narrow(double value) { - if (!Double.isFinite(value) || value > TWO_POW_63 || value < -TWO_POW_63) { + if (!Double.isFinite(value) || value >= TWO_POW_63 || value < -TWO_POW_63) { throw new ArithmeticException("BIGINT overflow in SUM"); } return (long) value; } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a subtle edge case where value > TWO_POW_63 allows 2^63 to pass through and saturate to Long.MAX_VALUE. However, the PR explicitly documents this ambiguity in the class javadoc and test (saturatesAmbiguousPositiveBoundary), indicating it's an intentional design choice. The suggestion to use >= would reject legitimate Long.MAX_VALUE sums, which the current implementation correctly accepts.	Low
General	Clarify boundary saturation behavior The documentation states that with pushdown enabled, "a small overflow that rounds to a signed BIGINT boundary can also be indistinguishable from an in-range sum and may saturate at the boundary instead of returning an error." This behavior could silently produce incorrect results. Consider clarifying whether this is intended behavior or if additional validation should be added to detect and report such edge cases. docs/user/ppl/functions/expressions.md [18-23] The accumulator used by `stats sum(integral_field)` depends on whether Calcite pushdown is enabled: -- With `plugins.calcite.pushdown.enabled=true` (the default), OpenSearch uses its native double-based `sum`, then the result is checked and narrowed to `BIGINT`. Large in-range sums can lose low-order precision. A small overflow that rounds to a signed `BIGINT` boundary can also be indistinguishable from an in-range sum and may saturate at the boundary instead of returning an error. +- With `plugins.calcite.pushdown.enabled=true` (the default), OpenSearch uses its native double-based `sum`, then the result is checked and narrowed to `BIGINT`. Large in-range sums can lose low-order precision. Note: A small overflow that rounds to a signed `BIGINT` boundary can be indistinguishable from an in-range sum and may saturate at the boundary instead of returning an error. Users should be aware of this limitation when working with values near the `BIGINT` boundaries. - With `plugins.calcite.pushdown.enabled=false`, Calcite uses an exact `BIGINT` accumulator and `Math.addExact` for every addition. It returns an error as soon as the running sum exceeds the `BIGINT` range. For example, summing `4611686018427387904` (`2^62`) and `1` returns the exact `4611686018427387905` without pushdown. With pushdown enabled, the double accumulator cannot represent the low-order `1`, so the result is `4611686018427387904`. Suggestion importance[1-10]: 3 __ Why: The suggestion adds a "Note:" to emphasize the boundary saturation limitation, but the improved_code is nearly identical to the existing_code with only minor wording changes. The original documentation already clearly explains this edge case behavior. The suggestion provides marginal improvement in clarity but doesn't address a significant issue or add substantial value.	Low

Suggestions up to commit aab660c

Category	Suggestion	Impact
General	Clarify overflow error handling behavior The documentation states that with pushdown enabled, "a small overflow that rounds to a signed BIGINT boundary can also be indistinguishable from an in-range sum and may saturate at the boundary instead of returning an error." This creates ambiguity about error handling behavior. Clarify whether the system returns an error or saturates when overflow occurs with pushdown enabled, as both behaviors are mentioned. docs/user/ppl/functions/expressions.md [18-23] The accumulator used by `stats sum(integral_field)` depends on whether Calcite pushdown is enabled: -- With `plugins.calcite.pushdown.enabled=true` (the default), OpenSearch uses its native double-based `sum`, then the result is checked and narrowed to `BIGINT`. Large in-range sums can lose low-order precision. A small overflow that rounds to a signed `BIGINT` boundary can also be indistinguishable from an in-range sum and may saturate at the boundary instead of returning an error. +- With `plugins.calcite.pushdown.enabled=true` (the default), OpenSearch uses its native double-based `sum`, then the result is checked and narrowed to `BIGINT`. Large in-range sums can lose low-order precision. When overflow occurs, the system attempts to detect it after narrowing, but small overflows that round to values near the `BIGINT` boundary may not be reliably detected. - With `plugins.calcite.pushdown.enabled=false`, Calcite uses an exact `BIGINT` accumulator and `Math.addExact` for every addition. It returns an error as soon as the running sum exceeds the `BIGINT` range. For example, summing `4611686018427387904` (`2^62`) and `1` returns the exact `4611686018427387905` without pushdown. With pushdown enabled, the double accumulator cannot represent the low-order `1`, so the result is `4611686018427387904`. Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies ambiguity in the documentation regarding overflow behavior with pushdown enabled. The improved text clarifies that overflow detection may not be reliable for small overflows near the BIGINT boundary, which is more accurate than the original wording that mentioned both saturation and error returns.	Medium
	Document BIGINT expression limitation The method widenBigintColumnToDouble only widens bare BIGINT column references but not BIGINT expressions. Consider documenting this limitation or extending the logic to handle BIGINT expressions (e.g., arithmetic operations on BIGINT columns) to ensure consistent AVG behavior across all BIGINT inputs. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [1645-1653] +/** + * If {@code field} is a bare BIGINT column reference, cast it to DOUBLE so that AVG's reduced + * intermediate SUM accumulates in double rather than a long that wraps near 2^63. Any other + * expression (including BIGINT expressions) is returned unchanged. + * + * Note: This only handles direct column references. BIGINT expressions may still overflow. + */ 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; } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that widenBigintColumnToDouble only handles direct column references. Adding documentation about this limitation is helpful for maintainability, though the impact is moderate since the current behavior is intentional.	Low
	Allow fallback for complex expressions The error message states "CHECKED_LONG_SUM pushdown requires one direct field reference", but this restriction may be too strict. If the argument is a valid expression (not just a direct field), consider falling back to non-pushdown execution instead of throwing an exception, ensuring broader query compatibility. opensearch/src/main/java/org/opensearch/sql/opensearch/request/AggregateAnalyzer.java [662-665] if (args.size() != 1 || !(args.getFirst().getKey() instanceof RexInputRef)) { - throw new AggregateAnalyzerException( - "CHECKED_LONG_SUM pushdown requires one direct field reference"); + // Fall back to non-pushdown execution for complex expressions + return null; } Suggestion importance[1-10]: 3 __ Why: The suggestion proposes returning null instead of throwing an exception for non-direct field references. While this could improve compatibility, the current behavior is intentional per the comment at line 663. The suggestion has merit but would require broader changes to the fallback mechanism.	Low

Suggestions up to commit aab660c

Category	Suggestion	Impact
General	Document widening limitation for expressions The method widenBigintColumnToDouble only widens bare BIGINT column references but not BIGINT expressions. Consider documenting this limitation more explicitly in the method's Javadoc, or verify that complex BIGINT expressions (e.g., arithmetic operations) are intentionally excluded from widening to avoid unexpected behavior. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [1645-1653] +/** + * If {@code field} is a bare BIGINT column reference, cast it to DOUBLE so that AVG's reduced + * intermediate SUM accumulates in double rather than a long that wraps near 2^63. Any other + * expression (including BIGINT expressions) is returned unchanged to preserve existing behavior. + */ 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; } Suggestion importance[1-10]: 5 __ Why: The suggestion to enhance documentation is valid and helpful. The method intentionally only widens bare RexInputRef nodes, and making this explicit in the Javadoc improves code clarity. However, this is a documentation improvement rather than a functional fix.	Low
	Verify exception handling for pushdown failure The method throws AggregateAnalyzerException when pushdown requirements are not met, but this exception might not be caught properly in all execution paths. Verify that this exception is handled gracefully and that the query falls back to enumerable execution rather than failing completely. opensearch/src/main/java/org/opensearch/sql/opensearch/request/AggregateAnalyzer.java [660-670] private static Pair<AggregationBuilder, MetricParser> createCheckedLongSumAggregation( - List<Pair<RexNode, String>> args, String aggName, AggregateBuilderHelper helper) { + List<Pair<RexNode, String>> args, String aggName, AggregateAnalyzerHelper helper) { if (args.size() != 1 || !(args.getFirst().getKey() instanceof RexInputRef)) { throw new AggregateAnalyzerException( "CHECKED_LONG_SUM pushdown requires one direct field reference"); } - ... + + return Pair.of( + helper.build(args.getFirst().getKey(), AggregationBuilders.sum(aggName)), + new CheckedLongSumParser(aggName)); } Suggestion importance[1-10]: 4 __ Why: The suggestion to verify exception handling is reasonable, but the improved_code is identical to the existing_code, providing no actual improvement. The concern about fallback behavior is valid but would require investigation beyond this code snippet.	Low
	Clarify overflow behavior with pushdown The documentation states that with pushdown enabled, "a small overflow that rounds to a signed BIGINT boundary can also be indistinguishable from an in-range sum and may saturate at the boundary instead of returning an error." This creates ambiguity about the actual behavior. Clarify whether the system returns an error or saturates at the boundary when overflow occurs with pushdown enabled. docs/user/ppl/functions/expressions.md [18-23] The accumulator used by `stats sum(integral_field)` depends on whether Calcite pushdown is enabled: -- With `plugins.calcite.pushdown.enabled=true` (the default), OpenSearch uses its native double-based `sum`, then the result is checked and narrowed to `BIGINT`. Large in-range sums can lose low-order precision. A small overflow that rounds to a signed `BIGINT` boundary can also be indistinguishable from an in-range sum and may saturate at the boundary instead of returning an error. +- With `plugins.calcite.pushdown.enabled=true` (the default), OpenSearch uses its native double-based `sum`, then the result is checked and narrowed to `BIGINT`. Large in-range sums can lose low-order precision. When overflow occurs, the system returns an error if the overflow is detectable after the double-to-BIGINT conversion. - With `plugins.calcite.pushdown.enabled=false`, Calcite uses an exact `BIGINT` accumulator and `Math.addExact` for every addition. It returns an error as soon as the running sum exceeds the `BIGINT` range. For example, summing `4611686018427387904` (`2^62`) and `1` returns the exact `4611686018427387905` without pushdown. With pushdown enabled, the double accumulator cannot represent the low-order `1`, so the result is `4611686018427387904`. Suggestion importance[1-10]: 4 __ Why: The suggestion addresses a legitimate ambiguity in the documentation about overflow behavior with pushdown enabled. However, the improved_code doesn't fully resolve the ambiguity - it still doesn't clearly explain what happens when "overflow is detectable after the double-to-BIGINT conversion" versus when it's not detectable. The original text's mention of saturation at the boundary is removed without fully explaining the actual behavior, making this only a partial improvement.	Low
	Reject ambiguous positive boundary value The boundary check value > TWO_POW_63 allows exactly 2^63 (which is Long.MAX_VALUE + 1) to pass through, then saturates it to Long.MAX_VALUE via Java's narrowing cast. This creates an ambiguous case where both Long.MAX_VALUE and 2^63 map to the same result. Consider rejecting value >= TWO_POW_63 to avoid this ambiguity. opensearch/src/main/java/org/opensearch/sql/opensearch/response/agg/CheckedLongSumParser.java [41-46] static long narrow(double value) { - if (!Double.isFinite(value) || value > TWO_POW_63 || value < -TWO_POW_63) { + if (!Double.isFinite(value) || value >= TWO_POW_63 || value < -TWO_POW_63) { throw new ArithmeticException("BIGINT overflow in SUM"); } return (long) value; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about the boundary case, but the current implementation appears intentional based on the test saturatesAmbiguousPositiveBoundary. The PR explicitly handles this ambiguity by saturating to Long.MAX_VALUE. Changing to >= would reject valid Long.MAX_VALUE sums.	Low

Suggestions up to commit 146adec

Category	Suggestion	Impact
Possible issue	Check overflow in arithmetic expression The aggregation now uses CHECKED_LONG_SUM which validates overflow on every addition. However, the input expression +($t1, $t2) (balance + 100) can overflow before reaching the aggregation. This overflow should be checked at the expression level to ensure consistent overflow detection throughout the pipeline. integ-test/src/test/resources/expectedOutput/calcite/explain_agg_with_script.yaml [9-13] EnumerableLimit(fetch=[10000]) EnumerableCalc(expr#0..1=[{inputs}], expr#2=[CHAR_LENGTH($t0)], sum=[$t1], len=[$t2], gender=[$t0]) EnumerableAggregate(group=[{0}], sum=[CHECKED_LONG_SUM($1)]) - EnumerableCalc(expr#0..1=[{inputs}], expr#2=[100:BIGINT], expr#3=[+($t1, $t2)], gender=[$t0], $f3=[$t3]) + EnumerableCalc(expr#0..1=[{inputs}], expr#2=[100:BIGINT], expr#3=[CHECKED_ADD($t1, $t2)], gender=[$t0], $f3=[$t3]) CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]], PushDownContext=[[PROJECT->[gender, balance]], OpenSearchRequestBuilder(sourceBuilder={"from":0,"timeout":"1m","_source":{"includes":["gender","balance"]}}, requestedTotalSize=2147483647, pageSize=null, startFrom=0)]) Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the addition operation +($t1, $t2) (balance + 100) could overflow before reaching the CHECKED_LONG_SUM aggregation. However, this is a test expectation file, not production code, and the suggestion asks to verify/ensure overflow checking rather than fixing a definite bug. The score reflects the importance of consistency in overflow detection.	Medium
	Add overflow check to addition The expression +($t7, $t19) (balance + 100) performs unchecked addition before the CHECKED_LONG_SUM aggregation. This creates an inconsistency where overflow can occur in the input expression but not be detected, defeating the purpose of the checked sum. Use checked arithmetic for the addition operation. integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_agg_with_script.yaml [9-13] EnumerableLimit(fetch=[10000]) EnumerableCalc(expr#0..1=[{inputs}], expr#2=[CHAR_LENGTH($t0)], sum=[$t1], len=[$t2], gender=[$t0]) EnumerableAggregate(group=[{0}], sum=[CHECKED_LONG_SUM($1)]) - EnumerableCalc(expr#0..1=[{inputs}], expr#19=[100:BIGINT], expr#20=[+($t7, $t19)], gender=[$t4], $f3=[$t20]) + EnumerableCalc(expr#0..1=[{inputs}], expr#19=[100:BIGINT], expr#20=[CHECKED_ADD($t7, $t19)], gender=[$t4], $f3=[$t20]) CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_bank]]) Suggestion importance[1-10]: 7 __ Why: Similar to suggestion 1, this identifies an inconsistency where +($t7, $t19) performs unchecked addition before CHECKED_LONG_SUM. The suggestion is valid but applies to a test expectation file and asks for verification of overflow checking rather than fixing a critical bug. The score reflects moderate importance for consistency.	Medium
	Add error handling for overflow exceptions The overflow check in CheckedLongSum.add() may throw an exception during document collection, potentially leaving the aggregator in an inconsistent state. Consider wrapping the addition loop in a try-catch block to ensure proper error handling and resource cleanup if overflow occurs mid-collection. opensearch/src/main/java/org/opensearch/sql/opensearch/aggregation/CheckedLongSumAggregator.java [72-84] public void collect(int doc, long bucket) throws IOException { if (!values.advanceExact(doc)) { return; } sums = bigArrays.grow(sums, bucket + 1); seen = bigArrays.grow(seen, bucket + 1); long sum = sums.get(bucket); - for (int i = 0; i < values.docValueCount(); i++) { - sum = CheckedLongSum.add(sum, values.nextValue()); + try { + for (int i = 0; i < values.docValueCount(); i++) { + sum = CheckedLongSum.add(sum, values.nextValue()); + } + sums.set(bucket, sum); + seen.set(bucket, 1); + } catch (IllegalArgumentException e) { + throw new IOException("Overflow during sum aggregation", e); } - sums.set(bucket, sum); - seen.set(bucket, 1); } Suggestion importance[1-10]: 3 __ Why: While wrapping the overflow check could provide cleaner error handling, the current implementation already throws IllegalArgumentException from CheckedLongSum.add() which will propagate correctly. Converting to IOException may not be necessary and could change the error handling semantics. The suggestion has merit but is not critical.	Low
General	Validate aggregation type before casting The reduce() method performs unchecked casting from InternalAggregation to InternalCheckedLongSum. If an incorrect aggregation type is passed, this will throw a ClassCastException at runtime. Add type validation before casting to provide a more informative error message. opensearch/src/main/java/org/opensearch/sql/opensearch/aggregation/InternalCheckedLongSum.java [58-70] public InternalCheckedLongSum reduce( List<InternalAggregation> aggregations, ReduceContext reduceContext) { long reduced = 0L; boolean anyValue = false; for (InternalAggregation aggregation : aggregations) { + if (!(aggregation instanceof InternalCheckedLongSum)) { + throw new IllegalArgumentException( + "Cannot reduce aggregation of type " + aggregation.getClass().getName()); + } InternalCheckedLongSum checkedSum = (InternalCheckedLongSum) aggregation; if (checkedSum.hasValue) { reduced = CheckedLongSum.add(reduced, checkedSum.sum); anyValue = true; } } return new InternalCheckedLongSum(name, reduced, anyValue, getMetadata()); } Suggestion importance[1-10]: 4 __ Why: Adding type validation before casting would provide better error messages if incorrect aggregation types are passed. However, this is a standard pattern in OpenSearch aggregations where type safety is typically enforced by the framework. The suggestion improves defensive programming but is not addressing a likely runtime issue.	Low
	Improve null-safety in validation logic The validation logic should also check if valuesSource is null before calling methods on it. While the outer condition checks for null, the inner condition could still throw a NullPointerException if valuesSource becomes null between checks in a concurrent environment. Consider restructuring to ensure thread-safety or add explicit null checks. opensearch/src/main/java/org/opensearch/sql/opensearch/aggregation/CheckedLongSumAggregator.java [44-47] -if (valuesSource != null && (valuesSource.isFloatingPoint() || valuesSource.isBigInteger())) { - throw new IllegalArgumentException( - "checked_long_sum requires a signed integral numeric field"); +if (valuesSource != null) { + if (valuesSource.isFloatingPoint() || valuesSource.isBigInteger()) { + throw new IllegalArgumentException( + "checked_long_sum requires a signed integral numeric field"); + } } Suggestion importance[1-10]: 2 __ Why: The suggestion misunderstands the code structure. The outer if (valuesSource != null) at line 48 already protects against null access. The inner condition at lines 44-47 is within this null-check block, so no additional null-safety is needed. The restructuring adds unnecessary nesting without improving thread-safety.	Low

Suggestions up to commit 0deaa72

Category	Suggestion	Impact
Possible issue	Add bounds checking for array access Add bounds checking for groupOffset + i to prevent potential IndexOutOfBoundsException when accessing fieldNames. The code assumes fieldNames.size() is always sufficient but doesn't verify this before accessing elements. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/AbstractCalciteIndexScan.java [331-332] protected boolean hasCheckedLongSumCollation(List<String> collations) { return pushDownContext.stream() .filter(action -> action.type() == PushDownType.AGGREGATION) .map(action -> (LogicalAggregate) action.digest()) .anyMatch( aggregate -> { int groupOffset = aggregate.getGroupSet().cardinality(); List<String> fieldNames = aggregate.getRowType().getFieldNames(); List<AggregateCall> aggregateCalls = aggregate.getAggCallList(); for (int i = 0; i < aggregateCalls.size(); i++) { - if (aggregateCalls.get(i).getAggregation() == PPLBuiltinOperators.CHECKED_LONG_SUM - && collations.contains(fieldNames.get(groupOffset + i))) { + int fieldIndex = groupOffset + i; + if (fieldIndex < fieldNames.size() + && aggregateCalls.get(i).getAggregation() == PPLBuiltinOperators.CHECKED_LONG_SUM + && collations.contains(fieldNames.get(fieldIndex))) { return true; } } return false; }); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential IndexOutOfBoundsException when accessing fieldNames.get(groupOffset + i). While the code likely works in practice due to Calcite's internal consistency guarantees, adding bounds checking improves defensive programming and prevents potential runtime errors.	Medium

[penghuo]

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

[ahkcs]

Added this distinction to the overflow documentation.

[github-actions]

Persistent review updated to latest commit 4d2c6fc

[github-actions]

Persistent review updated to latest commit 31bbdf2

[github-actions]

Persistent review updated to latest commit 0a077e0

[penghuo]

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

[ahkcs]

#5604 and this PR now use the same checked-addition primitive, but at different planner levels.

#5604 handles scalar arithmetic such as a + b. Calcite exposes it as RexCall(PLUS, a, b), so it can be rewritten to CHECKED_PLUS, which calls Math.addExact once.

SUM is an AggregateCall; its internal additions are not visible PLUS nodes, so the scalar rewrite cannot intercept them. We now solve that with a custom CHECKED_LONG_SUM aggregate:

• Non-pushdown: CheckedLongSumAggFunction calls Math.addExact(accumulator, value) for every row.
• Pushdown: native checked_long_sum calls Math.addExact during shard accumulation and coordinator reduction, and transports exact long values.
• TINYINT, SMALLINT, INTEGER, and BIGINT inputs all use this checked BIGINT accumulator.

This intentionally fails on an intermediate overflow even if later values would bring the final mathematical sum back into range. For example, Long.MAX_VALUE, 1, -1 throws at Long.MAX_VALUE + 1. We consider that fail-fast, order-dependent behavior acceptable and consistent with checked Java/Spark-style accumulation.

[github-actions]

Persistent review updated to latest commit 9eec01c

[github-actions]

Persistent review updated to latest commit f5d8adc

[github-actions]

Persistent review updated to latest commit f2c52bd

[github-actions]

Persistent review updated to latest commit 0deaa72

[github-actions]

Persistent review updated to latest commit 146adec

[penghuo]

Add an new sum aggregation in DSL? I do not think it is required. I am OK post-processing and DSL-pushdown has percision difference (this is DSL existing feature).

[ahkcs]

Agreed. I removed the custom DSL aggregation and getAggregations() registration. Pushdown now uses native OpenSearch sum with post-processing for BIGINT range validation, while accepting the existing native-double precision behavior. The non-pushdown path still uses Math.addExact during accumulation.

[github-actions]

Persistent review updated to latest commit aab660c

[github-actions]

Persistent review updated to latest commit aab660c

[github-actions]

Persistent review updated to latest commit 2166002

[github-actions]

Persistent review updated to latest commit 481819a