Skip to content

Widen inferred Sum arities before the corgi chunker compares or gathers - #817

Open
frankmcsherry wants to merge 2 commits into
master-nextfrom
corgi-sum-arity
Open

Widen inferred Sum arities before the corgi chunker compares or gathers#817
frankmcsherry wants to merge 2 commits into
master-nextfrom
corgi-sum-arity

Conversation

@frankmcsherry

Copy link
Copy Markdown
Member

infer_shape_cols commits only the arms a batch actually contains, so two containers of one DDIR type can disagree on Sum arity — a collection of only Rare(_) infers Sum([Some(_)]), one of only Common(_) infers Sum([None, Some(_)]). corgi reads a differing arity as a genuine type error (shape::join says so in as many words), and gather_lanes then indexes a lane vector shorter than the tags it was handed:

index out of bounds: the len is 1 but the index is 1
corgi/src/value.rs:390  within_offsets
  -> Value::sum_from_prim -> gather_lanes -> chunk::concat_blocks -> CorgiChunker::flush

This is reachable from a plain program — concatenate two collections that inject different constructors, then arrange the result. The vec backend renders it fine, so it was a corgi-only parity hole. tests/programs/sum_skew.ddp is that program; it panics without this change and passes with it.

The fix

Widen to the common arity with uncommitted (⊥) lanes at each site that reads two independently-inferred columns together. This is sound by corgi's own rule that an uncommitted lane holds no rows, and it leaves within-variant offsets untouched — they depend only on the tags and per-tag cursors, so unused lanes cost nothing.

Four sites, because merge needs it as much as the gathers do: compare_at has no defined order for a tag with no lane, so a differing arity threatens the ordering, not just the gather.

Checks

  • Gate 13/13, debug and release (the 13th is the new program).
  • scc steady-state 17.49s vs 17.46s baseline — median of 3, baseline itself spanning 17.44–17.51. The walk is over the shape spine, not the rows.

For review — is this the right layer?

This reconciles shapes in DDIR, which is arguably data-layer work. Two alternatives, both larger:

  1. Teach corgi's gather_lanes to join source arities. Fixes every caller rather than this one, and Value::empty already fills ⊥ source slots — padding ⊥ lanes is the same idea one level down. Needs a corgi change and a pin bump.
  2. Carry the con declarations into the plan. The program states its variant universe (con Rare(1) = 0) and the parser discards it, so infer_shape_cols rebuilds a weaker, data-dependent approximation per batch. With the declared arity, every batch agrees and no reconciliation is needed anywhere. This is the principled fix.

This PR is the small one that closes the panic. Happy to do either alternative instead.

🤖 Generated with Claude Code

https://claude.ai/code/session_0168NJWzHwrLGW2RXToGooYP

frankmcsherry and others added 2 commits August 3, 2026 20:08
`infer_shape_cols` commits only the arms a batch actually contains, so two
containers of one DDIR type can disagree on Sum arity: a collection of only
`Rare(_)` infers `Sum([Some(_)])`, one of only `Common(_)` infers
`Sum([None, Some(_)])`.  corgi reads a differing arity as a genuine type error
(`shape::join` says so explicitly), and `gather_lanes` then indexes a lane
vector shorter than the tags handed to it:

    index out of bounds: the len is 1 but the index is 1
    corgi/src/value.rs:390  within_offsets
    -> Value::sum_from_prim -> gather_lanes -> chunk::concat_blocks

Reachable from a plain program -- concatenate two collections that inject
different constructors, and arrange the result.  vec renders it fine, so this
was a corgi-only parity hole.  `tests/programs/sum_skew.ddp` is that program;
it panics without this change.

Widen to the common arity with uncommitted (bottom) lanes at each site that
reads two independently-inferred columns together.  Sound by corgi's own rule
that an uncommitted lane holds no rows, and it leaves within-variant offsets
untouched (they depend only on tags and per-tag cursors).  `merge` needs it as
much as the gathers do -- `compare_at` has no defined order for a tag with no
lane.

Gate 13/13 debug and release; scc steady-state 17.49s vs 17.46s baseline
(median of 3, baseline spanning 17.44-17.51) -- the walk is over the shape
spine, not the rows.

Note for review: this reconciles shapes in DDIR, which is arguably data-layer
work.  Two alternatives, both bigger: teach corgi's `gather_lanes` to join
source arities (fixes every caller, needs a pin bump), or carry the `con`
declarations into the plan so every batch infers the declared arity and no
reconciliation is needed anywhere.  The last is the principled fix; this one
is the small one that closes the panic.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0168NJWzHwrLGW2RXToGooYP
The first fixture's comment said `hash` was load-bearing -- that only the row-wise
fallback infers shape from data, so only it could produce a skewed arity.  That is
wrong, and the new program proves it: with `hash` dropped both maps lower to corgi
logic and it still panics identically.

`infer_term_shape` gives `Inject(tag, _)` an arity of `tag + 1`, so the compiled path
under-approximates the variant universe exactly as `infer_shape_cols` does -- from the
term instead of from the data.  A single term reconciles its own arms (`If` joins
them); two separate operators have nothing to reconcile them.

Both programs panic without this PR's widening and pass with it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0168NJWzHwrLGW2RXToGooYP
@frankmcsherry

Copy link
Copy Markdown
Member Author

Follow-up study found the root cause one layer down, and it is smaller than this PR.

The bug is corgi's, and it is one line. engine::gather_lanes takes the output Sum's arity from sums[0], while the gathered tag column names tags from every source:

let arity = sums[0].2.len();   // <- drops lanes the out tags still reference

A shorter source 0 therefore loses lanes the output still names, and sum_from_prim indexes past its own. That contradicts the ⊥ rule the same function already documents for None lanes — the arity simply wasn't reading it. Fixed in frankmcsherry/WIP#10 by taking the max over sources and reading lanes with get.

It subsumes this PR's DDIR-side reconciliation. With wip#10 patched in and all four chunk.rs sites reverted, the gate passes 14/14 in debug and release. So the choice is:

lines layer needs
this PR +74/−7 in DDIR, 4 sites reconciles shapes in DDIR (the P2 smell I flagged) nothing
wip#10 +1 real line in corgi fixes it where it is, for every caller a pin bump

My recommendation: merge wip#10, then reduce this PR to the two test programs plus the pin bump. That is strictly fewer lines and fewer concepts, and it removes a unify_sum_arity walk from merge, which is hot. Perf on the corgi fix is flat (scc 16.92s vs a 17.10s same-build-type control, median of 3).

I have also corrected a wrong claim in the fixture comment I pushed earlier. I wrote that hash was load-bearing — that only the row-wise fallback derives shape from data, so only it could skew. That is false. infer_term_shape gives Inject(tag, _) an arity of tag + 1, so the compiled path under-approximates the variant universe too, just from the term instead of the data. sum_skew_compiled.ddp drops the hash and panics identically. Both programs are now in the PR.

The cause underneath both is still open (charter S18): Parser.cons holds the declared variant universe and discards it after desugaring, so neither shape derivation ever sees the real arity. Carrying it into the plan would make every column agree by construction and need no reconciliation anywhere — but I sized the plumbing at ~45 mechanical touch points (parseVec<Stmt>, 10 call sites; lower_tree, 18; plus the recursive shape functions), for one usize. Worth doing on its own merits, not as a prerequisite for either fix here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant