csun5285 commented on PR #68632:
URL: https://github.com/apache/doris/pull/68632#issuecomment-5906340931
<!-- doris-repo-review:v1:begin -->
### Local pipeline review — ✅ PASS
```yaml
schema: doris-repo-review/v1
status: PASS
pr: apache/doris#68632
commit: 75a339b772b9d44b9a899755fb57bceeb9e4d2f8
base: d2cfcc1dcd4019fbae0d6ffbce16a7563bd9ecf4
reviewed_at: 2026-09-30T15:27+08:00
reviewer: csun5285
model: claude-fable-5-1
effort: max
findings: {blocker: 0, major: 0, minor: 1, nit: 2}
rounds: 2
converged: true
```
**Notes for maintainers**
- `be/src/exprs/function/array/function_array_aggregation.cpp:94` — the
LARGEINT Int128 wraparound raised in thread r4131617267 was reviewed as a
design trade-off, not a defect: the array path now uses the very same
`AggregateFunctionAvg<T, TYPE_DOUBLE, AvgData<avg_sum_type(T)>>` specialization
as `avg()`, every sum-based integer path in Doris (`+`, `sum()`, `array_sum`,
`array_cum_sum`) wraps under `NO_SANITIZE_UNDEFINED`, and ClickHouse 26.3.9.8
`arrayAvg`/`avg` return the identical `5.671372782015641e37` / `-1` for the
same Int128 inputs. BIGINT arrays cannot reach the wrap (|sum| < 2^126). An
overflow-safe accumulator would break `array_avg(arr) == avg(x) over
explode(arr)` unless `avg()`'s raw `sizeof(Data)` shuffle state changed too,
which has no version gate.
- `be/test/exprs/function/function_array_aggregation_test.cpp:343-347` — no
test added by the PR distinguishes the BIGINT -> Int128 half of `avg_sum_type`
from an Int64 sum (all new BIGINT rows cancel); one non-cancelling row such as
`[MAX64, MAX64]` (head 9.223372036854776e+18, an Int64 sum would give -1) in
the UT and/or the new suite would pin it. The same rows for LARGEINT (`[MAX,
MAX]` -> -1) would also document the wrap the bot asked about.
- `regression-test/suites/doc/sql-manual/ArrayNullsafe.groovy:257` — after
this PR the fixture row `[MAX, MAX-1, MAX-2]` is the only place the LARGEINT
wrap is pinned (`ArrayNullsafe.out:882` = `5.671372782015641e+37`); a one-line
comment there ("sum overflows Int128 and wraps, same as avg()") would stop the
`.out` from reading as a bug.
- Not verified locally: no compile, BE UT run, or regression run (read-only
review); `build-support/check-build-hygiene.sh` and clang-format 16 `--dry-run
-Werror` on the four C++ files both pass. The `dev/4.1.x` pick target already
sums `avg(BIGINT/LARGEINT)` in Int128 (branch-4.1 and branch-4.2 checked), so
the parity claim holds there.
<sub>Reviewed locally with the `doris-repo-review` pipeline. Repository
policy may accept this receipt for the matching commit; it is not a human
Apache approval.</sub>
<!-- doris-repo-review:v1:end -->
--
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]
