fix(agg): reduction of a scalar sub-expression matches the scalar builtin#238
Merged
Conversation
A reduction applied to a scalar sub-expression — (sum (max v)), (sum (count (distinct v))), (sum (sum v)), (avg (max v)) — compiles to a DAG whose OP_SUM/OP_MIN/... input materialises to an ATOM (the inner reduction's scalar result). exec_reduction's atom branch only handled COUNT and returned a `type` error for everything else, while the direct scalar form (sum 6) accepted it — a scalar<->DAG divergence (same family as M1-M5). Materialising the inner value out of the DAG ((set x (count (distinct v))) then (sum x), or (sum (+ 0 (max v)))) already worked, confirming it is purely the DAG atom-input path that diverged. Fix: exec_reduction's atom branch now delegates to the scalar aggregate builtins (ray_sum_fn / ray_avg_fn / ray_min_fn / ray_max_fn / ray_first_fn / ray_last_fn / ray_med_fn / ray_var*_fn / ray_stddev*_fn), so the DAG agrees with the direct scalar form by construction — including type/admission (sum of a SYM atom reduces to itself exactly as the scalar (sum 'c) does, and sum of a TIME atom stays TIME). OP_PROD (no scalar builtin) returns the lone element. The builtins handle the atom in place without recursing back here and return an owned value without consuming `input`. Tests: agg/reduce_scalar_subexpr.rfl pins the reduction-of-scalar forms, scalar<->DAG parity, and type preservation. Full suite green under gcc+ASan (no leak/UAF from the delegation), 0 failed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A reduction applied to a scalar sub-expression raised a spurious
typeerror, while the same reduction on a literal scalar worked:These compile to a DAG whose
OP_SUM/OP_MIN/… input materialises to an atom (the inner reduction's scalar result).exec_reduction's atom branch handled onlyCOUNTand returned atypeerror for everything else — a scalar↔DAG divergence in the same family as the M1–M5 aggregation reconciliation. Materialising the inner value out of the DAG ((set x (count (distinct v)))then(sum x), or(sum (+ 0 (max v)))) already worked, confirming it is purely the DAG atom-input path that diverged.Fix
exec_reduction's atom branch now delegates to the scalar aggregate builtins (ray_sum_fn/ray_avg_fn/ray_min_fn/ray_max_fn/ray_first_fn/ray_last_fn/ray_med_fn/ray_var*_fn/ray_stddev*_fn), so the DAG agrees with the direct scalar form by construction — including type/admission: sum of aSYMatom reduces to itself exactly as the scalar(sum 'c)does, and sum of aTIMEatom staysTIME.OP_PROD(no scalar builtin) returns the lone element. The builtins handle the atom in place (no recursion back intoexec_reduction) and return an owned value without consuminginput.Verification
agg/reduce_scalar_subexpr.rflpins the reduction-of-scalar forms, the scalar↔DAG parity ((sum (max v)) == (max v)), and type preservation (i64 / f64 / time).Not included
A separate, distinct issue remains open and is not addressed here:
(sum (map (fn […] (count (distinct v))) range))still errors, but that issumover amap-produced LIST (a LIST→vector coercion path), not the atom-input path fixed here —(sum (at … 0))and(fold + … )over the same map result both work. Tracked separately.🤖 Generated with Claude Code