Doris-Breakwater commented on issue #68123: URL: https://github.com/apache/doris/issues/68123#issuecomment-5710516884
### Initial assessment **Verdict: confirmed high-impact decimal correctness bug on `c4dee4bd5e82c6f0f380e0243cbaf265999e6a6a`.** The report is sufficiently detailed to start work; no query profile or additional logs are needed for the query-time cases. I confirmed the mechanisms by inspecting the exact commit and reproducing the arithmetic/string behavior locally, although I did not rebuild and run a Doris cluster. The issue currently has no labels, assignee, milestone, or prior comments. There are two cooperating defects, not one: 1. Decimal `SUM`/`AVG` aggregation does not observe `check_overflow_for_decimal`, so an out-of-range logical `DECIMAL(38,s)` value is allowed into the result column and later native `int128` overflow is also unchecked. 2. Decimal-to-string formatting assumes the native value has at most the declared physical precision. Once aggregation violates that invariant, a 39-digit positive `int128` (or sign plus 39 digits) is written past the logical size of a string preallocated for 38 digits, and the last digit is lost when the string is resized. This explains the apparently plausible “one tenth” result. ### Verified code path and exact values - With `enable_decimal256 = false`, FE assigns decimal `SUM` a `DECIMAL(38, input_scale)` result; when enabled it assigns precision 76 ([`ComputePrecisionForSum`](https://github.com/apache/doris/blob/c4dee4bd5e82c6f0f380e0243cbaf265999e6a6a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/ComputePrecisionForSum.java#L28-L47)). BE then uses that planned result type as the aggregate state type. - The decimal sum state performs plain `sum += value` and `sum += rhs.sum`, with no logical-precision or native-overflow check ([sum state](https://github.com/apache/doris/blob/c4dee4bd5e82c6f0f380e0243cbaf265999e6a6a/be/src/exprs/aggregate/aggregate_function_sum.h#L51-L65)). Row add, partial-state merge, and final insertion simply call those operations and push the raw state into the result column ([aggregate implementation](https://github.com/apache/doris/blob/c4dee4bd5e82c6f0f380e0243cbaf265999e6a6a/be/src/exprs/aggregate/aggregate_function_sum.h#L105-L132)). Serialized-state range merge and window incremental paths contain the same unchecked additions elsewhere in this class. - `check_overflow_for_decimal` is copied into scalar `FunctionContext` ([context setup](https://github.com/apache/doris/blob/c4dee4bd5e82c6f0f380e0243cbaf265999e6a6a/be/src/exprs/vexpr_context.cpp#L188-L193)), which is why binary arithmetic checks it. Aggregate construction has no corresponding policy field in `AggregateFunctionAttr` ([aggregate attributes](https://github.com/apache/doris/blob/c4dee4bd5e82c6f0f380e0243cbaf265999e6a6a/be/src/exprs/aggregate/aggregate_function.h#L50-L56)). Therefore the session variable cannot currently affect `SUM`, `AVG`, window aggregation, or storage reader/load sum. - At 100,000 rows of `10^33`, the aggregate's native state is still the mathematically correct `10^38`; it fits in signed `int128` but exceeds `DECIMAL(38,0)`. The formatter allocates 38 characters based on `MAX_DECIMAL128_PRECISION`, writes all 39 digits, then resizes from the old logical size ([formatter](https://github.com/apache/doris/blob/c4dee4bd5e82c6f0f380e0243cbaf265999e6a6a/be/src/core/types.h#L140-L185)). With the same libstdc++ behavior, a focused probe produces 38 visible digits followed by `NUL`, exactly matching the reported value divided by ten. This formatter behavior is unsafe and should be fixed independently, but the primary invariant violation originates in aggregation. - For two inputs of `10^38 - 1`, the native two's-complement result after overflowing signed 128-bit storage is actually `-140282366920938463463374607431768211458`. The reported `-14028236692093846346337460743176821145` is that value with its final digit lost by the same formatter. Consequently, the text in the `SUM(a) + 0` error is not the exact native aggregate state; the binary operator does receive an already-wrapped negative value, but its diagnostic formatting drops the final digit too. - `AVG` has unchecked decimal accumulation and merge, then also computes `sum * multiplier` in the same result type before division ([AVG result calculation](https://github.com/apache/doris/blob/c4dee4bd5e82c6f0f380e0243cbaf265999e6a6a/be/src/exprs/aggregate/aggregate_function_avg.h#L64-L83), [AVG accumulation](https://github.com/apache/doris/blob/c4dee4bd5e82c6f0f380e0243cbaf265999e6a6a/be/src/exprs/aggregate/aggregate_function_avg.h#L168-L182)). Applying 128-bit wrap to the reported inputs, then the scale-4 multiplication and division by two, gives exactly `8026480282232511287371606174911062.6544`. - Storage aggregation reuses the same unchecked sum implementation for both `_reader` and `_load` ([registration](https://github.com/apache/doris/blob/c4dee4bd5e82c6f0f380e0243cbaf265999e6a6a/be/src/exprs/aggregate/aggregate_function_reader.cpp#L34-L47)); aggregate-key memtables select the `_load` function ([memtable path](https://github.com/apache/doris/blob/c4dee4bd5e82c6f0f380e0243cbaf265999e6a6a/be/src/load/memtable/memtable.cpp#L108-L132)). Thus storage materialization is a credible and code-confirmed risk, not only a SQL result-sink problem. - `SUM(DISTINCT a)` is not a counterexample: the two equal inputs reduce to one value, so that case never overflows. ### One clarification needed for the on-disk claim The shown aggregate-key reproduction uses **two separate `INSERT` statements**. Those can create separate rowsets and overflow later in the `_reader` merge; that SQL alone does not prove the negative native value was already persisted in a segment. The `_load` path is nevertheless unchecked and can materialize it when duplicate keys are combined in one memtable, and a later compaction can do likewise. Please strengthen that part of the reproduction with either: - one `INSERT ... VALUES (1, max), (1, max)` so both duplicate keys aggregate in one load memtable, followed by a segment/rowset inspection; or - the existing two inserts followed by a forced compaction, then evidence that the compacted segment contains the overflowed value. The referenced `repro.py` and `verify-main.py` contents are not attached to the issue. The SQL already suffices for the query bug, but attaching them would make the boundary and storage regression cases reusable. ### Recommended fix scope 1. Propagate the decimal-overflow policy into aggregate construction/execution, or use a wider internal decimal state and enforce the planned result precision at finalization. Cover **every** arithmetic site: row add/subtract, partial-state merge, serialized-state range/vector merge, window incremental updates, and `AVG`'s scale multiplication. Checking only `insert_result_into` is too late after native `int128` wrap. 2. Apply equivalent protection to storage `_load`/`_reader` aggregation. Storage operations do not naturally have a query session policy, so their failure contract must be explicit; silently storing a wrapped `DECIMAL(38,s)` is not acceptable. 3. Fix `decimal_to_string` so capacity is based on the actual formatted value (or use a bounded/dynamically growing formatter). It must never write outside the string's logical size or silently emit a truncated value, even if an invalid internal decimal reaches serialization. 4. If the aggregate state is widened from 128 to 256 bits, account for serialized aggregate-state size, spill/exchange compatibility, mixed-BE-version execution, and storage reader/load compatibility rather than changing the state layout unversioned. 5. Add focused regression coverage for: - logical precision overflow below `int128` max (`10^38`) with `check_overflow_for_decimal = 1`; - native `int128` overflow in both row accumulation and partial-state merge; - grouped, global, window, and multi-stage/distributed `SUM`; - `AVG`, including the scale-multiplication overflow; - aggregate-key same-load aggregation, read merge, and post-compaction data; - decimal string conversion of 39-digit positive and negative `int128` values, ensuring no dropped byte or embedded trailing `NUL`; - `enable_decimal256 = true` as the non-overflow control case. For ordinary query `SUM`/`AVG`, `enable_decimal256 = true` is a valid temporary mitigation for these magnitudes because it changes the planned state/result to Decimal256. It should not be presented as protection for an existing `DECIMAL(38,0) SUM` aggregate-key column: storage `_load`/`_reader` aggregation follows the schema's Decimal128 type and does not consume that session setting. Maintainers should apply the project-standard high-severity/data-correctness, BE aggregate/expression, decimal, and storage labels and assign owners from both aggregate execution and storage aggregation. Breakwater-GitHub-Analysis-Slot: slot_47c5ef12290a -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
