fix(agg): keep sum/prod list reductions exact and UB-free for i64

[belowzeroff]

Problem

Reducing a boxed (list ...) of integers with sum / prod mishandles large i64 values. Both are confirmed by UBSan (debug build):

(sum  (list 9223372036854775807 1))   ;; src/ops/agg.c:464 — signed integer overflow (UB)
(prod (list 9007199254740993 1))      ;; -> 9007199254740992  (off by one: lossy)
(prod (list 9223372036854775807 4))   ;; src/ops/agg.c:497 — 9.2e18 outside int64 range (UB)

Root causes in the list (scalar) reduction path:

• sum accumulates with signed arithmetic — isum += elems[i]->i64 (and the mixed-int fallback isum += v). Signed overflow is undefined behaviour.
• prod reads every non-float element through (int64_t)as_f64(atom). Round-tripping an i64 through double loses precision above 2^53, and the cast is UB when the value rounds to ≥ 2^63.

The typed-vector reduction path already wraps safely, and prod's multiply already uses unsigned math — only the boxed-list integer reads/accumulation were wrong, so a single-element list and a multi-element list of the same values disagreed.

Fix

• Accumulate integers via unsigned math ((int64_t)((uint64_t)acc + …)), giving well-defined wraparound instead of UB — matching the typed-vector path and the unsigned idiom already present in prod's multiply.
• Read i64 atoms directly (elems[i]->i64) instead of via as_f64, so results stay exact above 2^53 and never hit the float→int cast UB.

This makes sum/prod symmetric and keeps single-element and multi-element list reductions consistent.

Testing

• make test — full suite green (3701/3702 pass, 1 pre-existing skip, 0 failed), UBSan/ASan clean.
• Added regression assertions:
  • test/rfl/agg/sum.rfl: 2*(2^63-1) wraps to -2; value at 2^53 + 1 stays exact.
  • test/rfl/agg/prod_builtin.rfl: exact product above 2^53; (2^63-1)*4 wraps to -4.

[singaraiona]

Reviewed (with an adversarial verification pass). The diff itself checks out — the unsigned-wrap idiom is sound, the tests' expected wrap values are correct, and wrap-landing-on-NULL_I64 is the engine's pre-existing tested design. The findings are all about the fix stopping short of its own premise: sibling sum accumulators in the same dispatch family retain the identical signed-overflow UB this PR eliminates for boxed lists.

1. agg_parted_sum still accumulates signed (src/ops/agg.c:153) — sum += agg_read_i64(seg, i) is the exact UB class this PR fixes, and ray_sum_fn dispatches to it (line ~402) before any admission check. Its sibling agg_parted_prod (line ~217) already uses the unsigned-wrap idiom. Two INT64_MAX rows in a parted I64 column ⇒ UB / UBSan trap, while the same values in a flat vector or boxed list now deterministically wrap per the new tests.

2. Parted path admits TIMESTAMP where flat rejects it (src/ops/agg.c:130) — the guard rejects only DATE and even returns ray_timestamp(sum), while agg_type_admitted (src/ops/ops.h:269) rejects TIMESTAMP for flat vectors. ~6 rows of current-epoch ns timestamps overflow mid-loop. Routing the parted path through agg_type_admitted (which already unwraps parted tags) fixes both #1's admission side and this inconsistency.

3. Table-scan scalar-sum kernels (src/ops/group.c:5934 scalar_sum_i64_fn, and scalar_sum_linear_i64_fn at ~5952) — signed accumulation (and multiplication in the linear variant) on the far hotter select {s: (sum v)} path. Same values that now wrap defined through (sum (list ...)) trap here.

4. Affine-sum fast path (src/lang/eval.c:269) — int64_t out = ce.sum_i + numeric_atom_i64(c_expr) * n; unguarded; (sum (+ v 9223372036854775807)) with 2 elements activates the fast path (dispatch ~eval.c:3817) and computes INT64_MAX * 2 signed.

5. Minor, in the touched function: the RAY_TIMESTAMP else-arm in ray_sum_fn (agg.c:440-445) is dead — agg_type_admitted rejects TIMESTAMP before dispatch — and contradicts the function's own "absolute points → type error" comment; the surviving I32/I16/U8/TIME arms still use signed += (hard to trigger, ~16 GiB of max-magnitude i32s, but same class). The new -RAY_I64 branches also duplicate the accumulate lines verbatim in both list loops; int64_t v = (elems[i]->type == -RAY_I64) ? elems[i]->i64 : (int64_t)as_f64(elems[i]); followed by one shared wrap-accumulate collapses four copies to two.

Suggestion: extract a wrap_add_i64/wrap_mul_i64 helper (ops/internal.h) and apply it at the sibling sites — that's also what would have prevented these being missed.

[singaraiona]

Follow-up on the wrap_add_i64 suggestion, since a fair concern is whether a helper inside the hot sum kernels would break autovectorization: it doesn't, verified with the repo's own toolchain (clang 18, -O3 -march=x86-64-v3, the release AVX2 baseline).

Three variants of the reduction loop compiled side by side:

s += d[i];                                    // signed (current UB form)
s = (int64_t)((uint64_t)s + (uint64_t)d[i]);  // inline wrap expression (this PR's idiom)
s = wrap_add_i64(s, d[i]);                    // static inline helper

All three get vectorized loop (vectorization width: 4, interleaved count: 4) from -Rpass=loop-vectorize, all three emit exactly the same 9 vpaddqs, and the function bodies are byte-identical except for local label numbers. Expected at the ISA level: two's-complement signed and unsigned 64-bit add are the same instruction, so the UB fix changes language semantics only, and a static inline helper dissolves at -O3 before the vectorizer runs.

The one real constraint: the helper must be static inline in a header (e.g. ops/internal.h) so it's visible in the translation unit — an out-of-line function call in the loop body would kill vectorization. With that spelled out, applying the wrap idiom (helper or inline, whichever reads better) to the sibling kernels in group.c/agg.c/eval.c is codegen-neutral.

Nuance for scalar_sum_linear_i64_fn specifically: 64-bit integer multiply has no AVX2 vector instruction (vpmullq is AVX-512), so that multiply isn't vectorized today and won't be either way — signedness is irrelevant to its codegen too.

[belowzeroff]

Thanks for the thorough pass — you're right that the fix stopped at the boxed-list path while the siblings carried the same UB. Fixed all of them in 23132ec, via the wrap_add_i64 / wrap_mul_i64 helper you suggested.

Helper placement: put it in core/types.h rather than ops/internal.h — lang/eval.c (finding #4) doesn't include ops/internal.h, whereas core/types.h is reachable from all three TUs via lang/internal.h and already hosts the sibling ray_cast_f64_to_* scalar helpers. static inline, so your codegen analysis holds (thanks for pre-empting the autovectorization concern).

Point by point:

1. agg_parted_sum signed accumulate — now wrap_add_i64, matching agg_parted_prod (which I also switched onto the shared helper).
2. Parted admits TIMESTAMP — routed the guard through agg_type_admitted(OP_SUM, x->type) (it unwraps the parted tag), so DATE/TIMESTAMP are rejected exactly as flat sum does; dropped the now-dead ray_timestamp(sum) return.
3. Table-scan kernels — scalar_sum_i64_fn (both the per-row add and the accumulator merge) and scalar_sum_linear_i64_fn (bias/coeff multiplies + term adds) wrapped.
4. Affine fast path — eval.c:269 now wrap_add_i64(ce.sum_i, wrap_mul_i64(numeric_atom_i64(c_expr), n)).
5. Minor — dead RAY_TIMESTAMP vector arm removed; the surviving I32/I16/U8/TIME arms wrapped; the two list loops collapsed to a single (-RAY_I64 ? ->i64 : as_f64) read + one shared wrap-accumulate, as suggested.

Tests: added select-scan (scalar, linear, by-group) coverage to agg/sum.rfl and affine-fold coverage to arith/sum_affine.rfl, each with values that wrap deterministically (2*(2^63-1) → -2, 2^62+2^62 per row → 0, etc.) and verified UBSan-clean. One note: I couldn't drive agg_parted_sum from the rfl surface — (at parted 'col) materializes to a flat vector and select … from: parted goes through the group scan kernels, so neither reaches ray_sum_fn(parted). The fix there is by parity with agg_parted_prod; happy to add a targeted C-level test if you know the construct that keeps the column parted into ray_sum_fn.

Full suite green (3701/3702, 1 pre-existing skip, 0 failed), ASan/UBSan clean.

[singaraiona]

Re-reviewed the head (23132ec + 0b05c1b) with a fresh ASan/UBSan build. Almost everything checks out — including the 0b05c1b decision to replace agg_type_admitted with an explicit whitelist, which is the right call (agg_type_admitted is a permissive plan-time gate that admits unknown wrappers including LIST, and would have let agg_parted_sum silently sum a parted LIST column to 0). Helper placement in core/types.h is fine and reachable from all three TUs.

Three remaining items:

1. One merge site missed — reproduced UB. The intra-kernel accumulation in scalar_sum_i64_fn/scalar_sum_linear_i64_fn is fully wrapped, but the cross-worker merge of those same accumulators is still a bare signed += at src/ops/group.c:11242 (m->sum[a].i += wa->sum[a].i; in exec_group_run). Reproduced: (select {r: (sum v) from: T}) with 131072 rows of 2^46 and RAYFORCE_CORES>=2 →

group.c:11242:41: runtime error: signed integer overflow

The PR's wrap tests use 2-row tables — below RAY_PARALLEL_THRESHOLD (64×1024) — so only one worker runs and the merge never adds two non-trivial partials. Sibling unwrapped merges in the same family worth sweeping in the same commit: group.c:7940 (da_merge_fn), :11911, :12038, the per-row acc->sum[a].i += iv in the masked/multi-agg DA kernels (:7474, :7588), and the radix sums[a].i += sites (~10131–12738). A wrap test above the parallel threshold (e.g. 131072 × 2^46 → deterministic wrapped total) would pin the merge path.

2. The new parted tests pass for the wrong reason. In test/rfl/agg/parted_f64_agg.rfl, (sum (at Pts 'ts)) !- type and (sum (at Pl 'l)) !- type never reach the new whitelist — ray_at_fn flattens parted columns (collection.c:2203 parted_to_flat_vec), so the TIMESTAMP error comes from flat-sum admission and the LIST error from the boxed-list loop. Verified live: the error text is the flat-path message, not the new "parted column, got …" one.

3. Answer to your reachability question: agg_parted_sum IS reachable from rfl — gdb-proven, no C-level test needed. The key: inside an active query, a quoted symbol naming a from-table column resolves to the RAW column without flattening (eval.c:3694, ray_table_get_col), and when the select expression isn't plan-compilable, eval_expr_per_row (query.c:3072) hands the still-parted vector to a lambda:

• Whitelist gate: (select {r: ((fn [c] (sum c)) 'ts) from: P}) → sum expects a numeric or time-duration parted column, got TIMESTAMP (TIMESTAMP fails plan admission, forcing the eval fallback; backtrace confirms agg_parted_sum ← ray_sum_fn ← vm_exec ← eval_expr_per_row).
• Wrap loop: numeric bases get intercepted by the compiled path, so defeat it — (select {r: ((fn [c] (sum (first (list c)))) 'v) from: P}) on a 2-partition I64 column [MAX],[MAX] → [-2 -2], agg_parted_sum hit twice.
• LIST gate: same construction on a parted LIST column → the new got LIST error, firing for real.

Suggest pinning those three in parted_f64_agg.rfl (and fixing the at-based ones, which pin the flat path). One caveat worth a comment in the test: the numeric repro relies on (first (list c)) defeating expr_compile — if the planner later learns that shape, the test silently shifts to the flattened path.

Side note, pre-existing (not this PR): plain (select {r: (sum l) from: Pl}) on a parted LIST column returns garbage via the group scan (the plan gate admits LIST as an unknown wrapper and read_col_i64 misreads list cells) — exactly the failure mode your 0b05c1b comment describes, one layer up. Worth its own follow-up.