linq_fold: PR D1 — group_by adapter reconciliation + 2 migrations + reducer-spec table#2886
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the linq_fold pattern-table refactor by migrating the group_by planner family onto pattern-table rows, partially reconciling GroupBySourceAdapter with the newer SourceAdapter helpers, and converting reducer emission dispatch into a data-table lookup. It also updates the corresponding documentation and refreshes benchmark result matrices for the affected group-by benchmarks.
Changes:
- Migrate
plan_group_by/plan_decs_group_byto pattern-table stubs backed by sharedemit_group_by→plan_group_by_core. - Introduce
to_source_adapter(GroupBySourceAdapter)and reuseadapter_wrap_source_loop/adapter_wrap_invokefor non-join group-by sources. - Replace
emit_reducer_branchesif/elif chain withreducer_emitterstable +mk_reducer_*helpers; update docs/bench outputs accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
daslib/linq_fold.das |
Core refactor: group_by patterns/emitter, adapter projection, reducer emitter table, and minor cleanup in bounded-heap emission. |
daslib/linq_fold.md |
Masterplan/status + decision log updates documenting the D1 refactor decisions and scope split (D1/D2). |
doc/source/reference/linq_fold_patterns.rst |
Updates pattern documentation to reflect new group_by pattern rows and reducer dispatch mechanism. |
benchmarks/sql/results.md |
Refreshes benchmark matrix rows for group_by/join_groupby and updates the generation metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…educer-spec table Continues the linq_fold pattern-table refactor (PRs A / B1 / B2 / C merged). PR D1 is the first half of PR D's masterplan scope: tackle the group_by family. Pure refactor — semantics unchanged, expected behavior identical. ## Changes **Bundled R1 follow-up to PR #2885:** drop dead `retType` assignments in emit_bounded_heap (3 LOC). Return type correctly inferred from inner `return <-` statements; `null` was already passed to adapter_wrap_invoke. **Partial GroupBy ↔ SourceAdapter reconciliation:** new `to_source_adapter(GroupBySourceAdapter) : SourceAdapter` projection maps array/decs/decs-join onto PR C's variant. `adapter_emit_source_loop` keeps decs-join inline shape (~40 LOC) but delegates array + plain-decs to `adapter_wrap_source_loop`. `adapter_finalize_emission` collapses to a one-line `adapter_wrap_invoke` call. Full unification deferred to PR D2. **`plan_group_by` + `plan_decs_group_by` migrated to pattern-table stubs:** 2 SplicePattern rows (group_by_array, group_by_decs) capture the tail-pop recognizer shape: head segments, optional upstream join (decs only), group_by_lazy, optional having_, select(groupproj), optional trailing where_, optional trailing order_by[_descending], optional count terminator. `Slot.arity` enforces args-length guards. New `order_by_family` alias (excludes bare order/order_descending — no key). New `decs_join_invariants` predicate enforces v1 join-shape limits (empty head, no having, no trailing_where when upstream_join captured). Shared `emit_group_by` reconstructs head calls from c.many["head"] (mirrors emit_loop_or_count_lane_decs precedent), builds the GroupBySourceAdapter per source-shape (Array / Decs / Decs-with-join), and delegates to plan_group_by_core unchanged. Both planner bodies (~165 LOC) hard-deleted. **`emit_reducer_branches` 13-arm if/elif → `reducer_emitters` data table:** table<string; ReducerEmitterFn> with 10 named `mk_reducer_*` fns. Shared `mk_strictly_preferred(workhorse, isMin, valExpr, cmpExpr)` helper collapses the 4-arm workhorse split between mk_reducer_min_max and mk_reducer_min_max_inner. `mk_*` naming (vs `emit_*`) marks these as sub-codegen building blocks, not pattern-table emit fns. ## Tests - All 624 tests across 13 group_by / theme / decs / fold-ast test files pass (test_linq_group_by 18, test_linq_from_decs 198, test_linq_fold_theme3_c2 14, test_linq_fold_theme3_decs_join_groupby 14, test_linq_fold_theme2 22, test_linq_fold_theme45 32, test_linq_fold_ast 228, test_linq_fold_walker 16, test_linq_fold_terminal_select 28, test_linq_fold_theme7 18, test_linq_fold_theme8 36, test_linq_fold_order_family 12). - Lint clean, compile clean. - INTERP + JIT bench matrix for 16 group_by + join_groupby benches refreshed — all deltas within run-to-run noise (~±1 ns INTERP, ~±0.3 ns JIT). ## Docs - Masterplan: PR D row split into D1 (complete) + D2 (deferred); PR D1 shipped scope detail; 7 new decision log entries; reducer-spec data table open question answered. - linq_fold_patterns.rst: group_by rows rewritten as "pattern `<name>` (sub-codegen `plan_group_by_core`) handles …". - benchmarks/sql/results.md: group_by rows refreshed against PR D1 branch. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
7042ee9 to
c601219
Compare
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.
Summary
Continues the
linq_foldpattern-table refactor (PRs A / B1 / B2 / C merged). PR D1 is the first half of PR D's masterplan scope: tackle the group_by family. Pure refactor — semantics unchanged.retTypeinemit_bounded_heap(3 LOC). Return type was already being inferred; the assignment was dead.to_source_adapter(GroupBySourceAdapter) : SourceAdapterprojection.adapter_emit_source_loopkeeps decs-join inline (~40 LOC, no SourceAdapter variant for it yet) but delegates array + plain-decs to PR C'sadapter_wrap_source_loop.adapter_finalize_emissioncollapses to a one-liner overadapter_wrap_invoke. Full unification deferred to PR D2 (whenplan_zip/plan_decs_joinmigrate and Zip/DecsJoin variants join).plan_group_by+plan_decs_group_by→ pattern-table stubs: 2SplicePatternrows (group_by_array,group_by_decs) + 1 sharedemit_group_bydelegating toplan_group_by_core(unchanged sub-codegen).Slot.arityenforces the args-length guards (count=1, join=5, group_by_lazy=2, order_by[descending]=2, where=2). Neworder_by_familyalias excludes bareorder/order_descending(no key arg). Newdecs_join_invariantspredicate enforces v1 join-shape limits (empty head, no having, no trailing_where whenupstream_joincaptured). ~165 LOC of imperative tail-pop hard-deleted across both planners.emit_reducer_branches13-arm if/elif →reducer_emittersdata table:table<string; ReducerEmitterFn>lookup with 10 namedmk_reducer_*fns. Sharedmk_strictly_preferred(workhorse, isMin, valExpr, cmpExpr)helper collapses the 4-arm workhorse split (workhorse × isMin) shared betweenmk_reducer_min_maxandmk_reducer_min_max_inner.mk_*naming marks these as sub-codegen building blocks, not pattern-table emit fns.Architectural decisions
GroupBySourceAdapterstays load-bearing (per-mode field bags diverge;plan_group_by_corereadsinitialBindName/initialElemTypedirectly per mode). Projection unlocks helper reuse without restructuring.Decs(null, "djoin_jres")is safe at finalize:adapter_wrap_invoke's Decs branch reads only the variant tag, never the tuple fields. Array path's invoke shifts untyped→typed;plan_group_by_coreunconditionally sets retType so the inference shift is a no-op.upstream_joinslot (not 2 separate rows). Predicate-gated invariants moved from emit-fn-land to slot/predicate-land viaSlot.arity+decs_join_invariants.Full decision log in
daslib/linq_fold.md(7 new entries: D1-A through D1-E + 2 impl notes).Out of scope (deferred to PR D2)
plan_zip(352 LOC) +plan_decs_join(189 LOC) migrationsSourceAdapterwidening toArray | Decs | DecsJoin | Zipreverse |> distinct[_by]on decs sources (D6 from PR C plan)Files
daslib/linq_fold.das— all implementationdaslib/linq_fold.md— masterplan refresh (PR D row split into D1 complete + D2 deferred; 7 new decision log entries)doc/source/reference/linq_fold_patterns.rst— group_by rows rewritten aspattern <name> (sub-codegen plan_group_by_core) handles …benchmarks/sql/results.md— INTERP + JIT matrix refresh for 16 group_by + join_groupby benchesTest plan
mcp__daslang__compile_check daslib/linq_fold.das— cleanmcp__daslang__lint daslib/linq_fold.das— cleantests/linq/test_linq_group_by.das— 18 passedtests/linq/test_linq_fold_theme3_c2_group_by_order_by.das— 14 passedtests/linq/test_linq_fold_theme3_decs_join_groupby.das— 14 passedtests/linq/test_linq_fold_theme2_trailing_where.das— 22 passed (HAVING shapes)tests/linq/test_linq_fold_theme45_quick_wins.das— 32 passedtests/linq/test_linq_from_decs.das— 198 passedtests/linq/test_linq_fold_ast.das— 228 passedtests/linq/test_linq_fold_pattern_walker.das— 16 passedtests/linq/test_linq_fold_terminal_select.das— 28 passedtests/linq/test_linq_fold_theme7_chained_select.das— 18 passedtests/linq/test_linq_fold_theme8_fusion_arms.das— 36 passed🤖 Generated with Claude Code