[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 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.

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 `5c7fbc2`)

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 Precision Loss The narrowing conversion at line 45 silently loses precision when the double value is near but not exactly at the BIGINT boundary. For example, a double value of 9.223372036854776e18 (which is slightly above Long.MAX_VALUE but rounds to it in double arithmetic) will be narrowed to Long.MAX_VALUE without indicating that precision was already lost. This is intentional per the PR description, but the boundary check at line 42 uses `value > TWO_POW_63` which allows `value == TWO_POW_63` to pass through. Since `TWO_POW_63` (0x1p63) equals 9.223372036854776e18 and is not representable as a long, this creates an ambiguous case where the narrowing conversion saturates to Long.MAX_VALUE. The test at line 35-36 in CheckedLongSumParserTest confirms this behavior is expected, but it means the parser cannot distinguish between a true Long.MAX_VALUE sum and a sum that overflowed to 2^63 in the backend. if (!Double.isFinite(value) \|\| value > TWO_POW_63 \|\| value < -TWO_POW_63) { throw new ArithmeticException("BIGINT overflow in SUM"); } return (long) value; Null Handling The `add` method at lines 15-20 ignores null values by checking `if (value != null)` before accumulating. However, if all input values are null, the `result` method at line 24 returns `null` when `count == 0`. This is correct per SQL semantics, but the accumulator's `sum` field is a primitive `double` initialized to 0.0. If the first non-null value is negative, the sum starts from 0.0 and adds the negative value, which is correct. However, if the accumulator is reused or merged (in a distributed aggregation scenario), the initial state of `sum = 0.0` could be problematic. Calcite's reflective UDAF framework may not handle accumulator merging for this class, so this is only a concern if future changes introduce a merge operation. public static Accumulator add(Accumulator accumulator, Long value) { if (value != null) { accumulator.sum += value; accumulator.count++; } return accumulator; } public static Double result(Accumulator accumulator) { return accumulator.count == 0 ? null : accumulator.sum / accumulator.count;

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 5c7fbc2

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix boundary overflow detection The boundary check `value > TWO_POW_63` allows exactly `2^63` (which equals `Long.MAX_VALUE + 1` in double representation) to pass through. When cast to long, Java's narrowing conversion saturates this to `Long.MAX_VALUE`, masking a true overflow. Consider using `value >= TWO_POW_63` to reject this ambiguous boundary case consistently. 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 `value > TWO_POW_63` allows exactly `2^63` to pass, which saturates to `Long.MAX_VALUE` when cast. However, the PR's comment at line 22-23 explicitly acknowledges this ambiguity and accepts it as a design choice. The suggestion would make the check stricter, but the current implementation appears intentional.	Medium

Previous suggestions

Suggestions up to commit 5c7fbc2

Category	Suggestion	Impact
Possible issue	Fix boundary overflow detection The boundary check `value > TWO_POW_63` allows exactly `2^63` (which equals `Long.MAX_VALUE + 1` in double representation) to pass through. When cast to long, Java's narrowing conversion saturates this to `Long.MAX_VALUE`, masking a true overflow. Consider using `value >= TWO_POW_63` to reject this ambiguous boundary case consistently. 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 `value > TWO_POW_63` allows exactly `2^63` to pass, which saturates to `Long.MAX_VALUE` when cast. However, the PR explicitly documents this ambiguous boundary behavior as acceptable (see line 23 comment and test at line 36). The suggestion would change intended behavior, so it receives a moderate score for raising a valid design consideration rather than fixing a bug.	Medium

Suggestions up to commit 47145b5

Category	Suggestion	Impact
Possible issue	Reject ambiguous positive boundary value The boundary check `value > TWO_POW_63` allows exactly `2^63` (which equals `Long.MAX_VALUE + 1` in double precision) to pass through. When cast to long, Java's narrowing conversion saturates this to `Long.MAX_VALUE`, masking a true overflow. Strengthen the condition to `value >= TWO_POW_63` to reject ambiguous boundary values. 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]: 8 __ Why: The boundary check allows exactly `2^63` to pass, which saturates to `Long.MAX_VALUE` during narrowing conversion, masking a true overflow. Changing to `>=` correctly rejects this ambiguous value and prevents silent data corruption.	Medium
	Add null-safety for field type The `isIntegral` check uses `field.getType()` directly, which may return null if the field's type is not yet resolved during planning. Add a null-safety guard before calling `getSqlTypeName()` to prevent potential `NullPointerException` during query optimization. core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java [1620-1628] void registerSumOperator() { registerOperator( SUM, SqlStdOperatorTable.SUM, - field -> - isIntegral(field.getType().getSqlTypeName()) - ? PPLBuiltinOperators.CHECKED_LONG_SUM - : SqlStdOperatorTable.SUM); + field -> { + RelDataType fieldType = field.getType(); + return fieldType != null && isIntegral(fieldType.getSqlTypeName()) + ? PPLBuiltinOperators.CHECKED_LONG_SUM + : SqlStdOperatorTable.SUM; + }); } Suggestion importance[1-10]: 7 __ Why: The code calls `field.getType().getSqlTypeName()` without null-checking `getType()`, which could cause a `NullPointerException` during query planning if the field type is unresolved. Adding a null guard improves robustness.	Medium
	Prevent exception on empty arguments The method retrieves `args.getFirst()` without verifying that the list is non-empty, despite checking `args.size() != 1`. If `args` is empty, `getFirst()` throws `NoSuchElementException`. Reorder the logic to check for emptiness explicitly or use a safer access pattern. opensearch/src/main/java/org/opensearch/sql/opensearch/request/AggregateAnalyzer.java [660-669] private static Pair<AggregationBuilder, MetricParser> createCheckedLongSumAggregation( List<Pair<RexNode, String>> args, String aggName, AggregateBuilderHelper helper) { - if (args.size() != 1) { + if (args.isEmpty() \|\| args.size() != 1) { throw new AggregateAnalyzerException("CHECKED_LONG_SUM requires exactly one argument"); } return Pair.of( - helper.build(args.getFirst().getKey(), AggregationBuilders.sum(aggName)), + helper.build(args.get(0).getKey(), AggregationBuilders.sum(aggName)), new CheckedLongSumParser(aggName)); } Suggestion importance[1-10]: 6 __ Why: The method checks `args.size() != 1` but then calls `args.getFirst()`, which throws `NoSuchElementException` if the list is empty. Explicitly checking for emptiness or using `args.get(0)` prevents this potential runtime error.	Low

Suggestions up to commit 481819a

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

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

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

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 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 · 2026-07-20T23:02:39Z

Persistent review updated to latest commit 9eec01c

github-actions · 2026-07-22T17:37:25Z

Persistent review updated to latest commit f5d8adc

github-actions · 2026-07-22T18:29:34Z

Persistent review updated to latest commit f2c52bd

github-actions · 2026-07-22T21:33:58Z

Persistent review updated to latest commit 0deaa72

github-actions · 2026-07-22T21:46:36Z

Persistent review updated to latest commit 146adec

penghuo · 2026-07-23T16:35:49Z


+  @Override
+  public List<AggregationSpec> getAggregations() {
+    return List.of(


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).

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 · 2026-07-23T18:10:54Z

Persistent review updated to latest commit aab660c

github-actions · 2026-07-23T18:11:32Z

Persistent review updated to latest commit aab660c

github-actions · 2026-07-23T18:51:25Z

Persistent review updated to latest commit 2166002

github-actions · 2026-07-23T22:23:06Z

Persistent review updated to latest commit 481819a

penghuo · 2026-07-24T16:17:03Z

+    if (!Double.isFinite(value) || value > TWO_POW_63 || value < -TWO_POW_63) {
+      throw new ArithmeticException("BIGINT overflow in SUM");
+    }


Any test cover this?

Added unit and IT coverage for finite/infinite values, both boundaries, and positive/negative overflow.

penghuo · 2026-07-24T16:27:28Z

          AVG,
-          (distinct, field, argList, ctx) -> ctx.relBuilder.avg(distinct, null, field),
+          (distinct, field, argList, ctx) -> {
+            if (field instanceof RexInputRef


Why limit to RexInputRef? field could be RexCall, e.g. avg(toInt(value)).

Agreed, removed it. All BIGINT expressions now use BIGINT_AVG, with RexCall IT coverage.

penghuo · 2026-07-24T16:39:59Z

+    private static boolean isIntegral(SqlTypeName typeName) {
+      return switch (typeName) {
+        case TINYINT, SMALLINT, INTEGER, BIGINT -> true;
+        default -> false;
+      };
+    }


Use "SqlTypeName.INT_TYPES.contains(...)"

penghuo · 2026-07-24T17:00:59Z

+      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());
+            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);
+          };


Code is duplicate with registerOperator. simplify it.

github-actions · 2026-07-24T20:24:02Z

Persistent review updated to latest commit 47145b5

Integral SUM used Calcite's unchecked long accumulator and could silently wrap. BIGINT AVG could likewise overflow its intermediate long sum before division. Use CHECKED_LONG_SUM for integral inputs and Math.addExact during enumerable accumulation. Preserve SqlKind.SUM so planner rewrites and native OpenSearch sum pushdown remain unchanged. Range-check and narrow pushed results while retaining the backend's double semantics. Use a double sum and long count for enumerable BIGINT AVG. Preserve SqlKind.AVG and native avg pushdown. Add unit, integration, REST, documentation, and explain coverage for both execution paths. Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-07-29T22:18:02Z

Persistent review updated to latest commit 5c7fbc2

github-actions · 2026-07-29T22:18:56Z

Persistent review updated to latest commit 5c7fbc2

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

ahkcs force-pushed the fix/sum-avg-bigint-overflow branch from f5d8adc to f2c52bd Compare July 22, 2026 18:27

penghuo reviewed Jul 23, 2026

View reviewed changes

ahkcs force-pushed the fix/sum-avg-bigint-overflow branch from 78015b4 to aab660c Compare July 23, 2026 18:09

ahkcs force-pushed the fix/sum-avg-bigint-overflow branch from aab660c to 2166002 Compare July 23, 2026 18:49

ahkcs force-pushed the fix/sum-avg-bigint-overflow branch from 2166002 to 481819a Compare July 23, 2026 22:21

penghuo requested changes Jul 24, 2026

View reviewed changes

ahkcs requested a review from penghuo July 27, 2026 17:38

penghuo approved these changes Jul 29, 2026

View reviewed changes

ahkcs force-pushed the fix/sum-avg-bigint-overflow branch from 47145b5 to e247144 Compare July 29, 2026 22:16

ahkcs force-pushed the fix/sum-avg-bigint-overflow branch from e247144 to 5c7fbc2 Compare July 29, 2026 22:17

ahkcs merged commit de95ffb into opensearch-project:main Jul 30, 2026
39 of 40 checks passed

ahkcs mentioned this pull request Jul 30, 2026

[analytics-engine] Fix CHECKED_LONG_SUM conversion opensearch-project/OpenSearch#22611

Open

		### 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

Architecture

Precision behavior

Overflow semantics

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 5c7fbc2)

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!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 20, 2026

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahkcs commented Jul 7, 2026 •

edited

Loading

github-actions Bot commented Jul 7, 2026 •

edited

Loading

(Review updated until commit `5c7fbc2`)

github-actions Bot commented Jul 7, 2026 •

edited

Loading

ahkcs Jul 17, 2026 •

edited

Loading