sqlite_linq: _distinct_by composition + reverse() MAX(pk) + group_by+first projection - #2909
Merged
Merged
Conversation
PR #2906 added `_distinct_by(K)` recognition as a wrapped chain op, but only the aggregate terminators (count(P)/long_count(P)/sum/min/max/average over _select) finalized the bare-aggregate inner-subquery wrap. Row-returning chains (`_distinct_by(K) |> to_array`, `|> first()`) silently dropped the dedup — emitted `SELECT … FROM "Cars"` with no GROUP BY. Add `finalize_distinct_by_passthrough_wrap` + `check_distinct_by_passthrough_gating` and a single `maybe_finalize_distinct_by_passthrough` helper invoked by every `analyze_chain` consumer (_sql / _try_sql / _sql_text / _each_sql / _create_view). The wrap relies on `build_sql_select`'s existing outer ORDER BY / LIMIT / OFFSET emission around `q.innerSql`, so trailing `_order_by` / `take` / `skip` compose for free; gating rejects `_where` / `_join` / `_group_by` / `_having` / set ops / additional `distinct` (each needs explicit composition logic, deferred to a follow-up). Plumb a new `distinctByLast : bool` field on SqlQuery (default false; written by the upcoming reverse() recognition branch). Extend `build_distinct_by_inner_subquery` to take a `useMax : bool` so the same helper emits MIN(pk) (current behavior) or MAX(pk) (Arm B's `reverse() |> _distinct_by`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
7 new positive tests in test_32_distinct.das covering the new passthrough finalize: bare `_distinct_by(K) |> to_array` (closes silent-drop bug), composition with trailing `_order_by`, `take`, and `skip`. Each test asserts both the emitted SQL shape (via _sql_text) and runtime correctness against the existing 5-row Cars fixture. New rejection probe failed_distinct_by_with_where.das exercises the v1 gating: `_where(P) |> _distinct_by(K)` is rejected with macro_error 50503 because inner-WHERE vs outer-WHERE semantics differ and v1 defers the composition. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
New `reverse` analyze_chain branch at PHASE_DISTINCT. When wrapped by `_distinct_by(_.K)` it flips the bare-aggregate inner subquery from MIN(pk) to MAX(pk), picking the LAST row per K by source/PK order — mirrors linq's `each(arr).reverse()._distinct_by(K)` "last per group" splice from PR #2875. Strict gating: `reverse()` only valid when `q.distinctByCol` is already set (i.e. the immediately-outer chain op was `_distinct_by`). Bare `reverse() |> to_array` rejects loudly with a fixit pointing to `_order_by_descending(...)` (SQL has no inherent row order to reverse). Double-reverse rejects as a no-op. 3 new positive tests in test_32 (emit shape + runtime "last per K" correctness + composition with outer ORDER BY). New rejection probe failed_reverse_without_distinct_by.das. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…)))` Whole-row tuple projection over `_group_by` lowers via the same bare-aggregate inner subquery as `_distinct_by(K)` passthrough. `analyze_grouped_projection` now recognizes `_._1 |> first()` (via the new `is_first_of_group_call` helper, parallel to `try_translate_group_aggregate`), tracks per-entry "is row" flags (`q.groupedSelectIsRow`), and synthesizes `q.distinctByCol` from `q.groupByCols[0]` so the existing passthrough rail builds the wrap. `build_sql_select`'s GroupedNamedTuple branch expands row entries to all source columns inline. `build_row_builder`'s GroupedNamedTuple branch emits `ExprMakeStruct` for row entries with per-field `read_via_adapter(stmt, baseCol+j, type<FieldT>)` calls — a new `build_row_make_struct` helper mirrors sqlite_boost's `make_sql_read_row_fn` pattern but with the column offset baked in at this macro pass (no sqlite_boost change needed). v1 constraints: single-key groups only; no mixing first() with column aggregates (length/sum/min/max/average) in the same projection. Computed-expression keys with first() rejected too. 3 new positive tests (emit shape, runtime first-row-per-K correctness, row field population). New rejection probe failed_groupby_first_mixed_with_aggregate.das. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Each bench now has a `run_m1` calling `_sql(...)` over the new sqlite_linq chain shape, with an explanatory comment pointing at the load-bearing sqlite_linq helper: - distinct_by_order_take — `_distinct_by(K) |> _order_by(S) |> take(N)` - distinct_by_order_to_array — same without `take` - reverse_distinct_by — `reverse() |> _distinct_by(K)` (MAX(pk)) - groupby_first — `_group_by(K) |> _select((K=_._0, F=_._1|>first()))` `reverse_distinct_by` notes the SQL-vs-linq order-divergence caveat (bare-aggregate returns engine-defined row order; linq returns reverse-source-order). `reverse_distinct_by` still has no m4 lane — closed by design (decs splice gates on `array_source`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Extend the SQL-12 tutorial RST with 3 new sections for the chunk N+1 chain shapes shipped in this PR: - "Distinct + composition (row passthrough)" — `_distinct_by(K) |> _order_by(S) |> take(N)` - "Reverse + distinct_by (last per group)" — `reverse() |> _distinct_by(K)`, MAX(pk) variant - "Group-by + first row per group" — `_group_by(K) |> _select((K=_._0, R=_._1|>first()))` Each section shows the chain, the emitted SQL, and the v1 constraints (`_where`/`_join`/etc rejection, multi-key reject, mixing first() with aggregates reject). Notes the SQL-vs-linq order-divergence caveat for reverse(). Mirror the 3 chain shapes in the executable tutorial .das. Runs cleanly via `mcp__daslang__run_script` with expected first-per/last-per/tuple output (Id=1/2/4 first per Name; Id=3/5/4 last per Name; (Audi,Id=2) (BMW,Id=1) (Tesla,Id=4) groups). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…nk N+1 results.md — full INTERP+JIT rerun across benchmarks/sql/ per the living-doc policy (this PR touches sqlite_linq.das). 4 new m1 SQL cells populated (distinct_by_order_take/distinct_by_order_to_array/ reverse_distinct_by/groupby_first); drift on existing cells mostly converges back toward pre-#2906 baselines. indexed_lookup m4 now populated (482.9 INTERP / 107.5 JIT — earlier `—` was stale). Notes section drops the 4 SQL window-function blocker bullets; reverse_distinct_by note trimmed to the (decs-only) remaining gap. sqlite_linq_gaps.md — entire "Window-function lowerings" group struck through. Closed in PR #2906 (2 cells) + PR #YYYY (4 cells via bare-aggregate composition + MAX(pk) + tuple projection). Note that 3 of the 4 cells in this PR did NOT need window functions — the gaps doc's original ROW_NUMBER prescription was conservative. API_REWORK.md — new "Shipped — chunk N+1" section covering all 3 arms: Arm A passthrough composition (closes silent-drop bug), Arm B reverse() MAX(pk), Arm C group-by + first() tuple projection. Strikes the 2 now-shipped items from chunk N's "Deferred to N+1" list. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Extends sqlite_linq’s SQLite “bare-aggregate” lowering to cover additional “first/last row per group” query shapes, closing remaining SQL gaps in the benchmark suite’s previously “window-function” bucket.
Changes:
- Add row-returning
_distinct_by(K)passthrough wrap (and composition with trailing_order_by/take/skip), including a finalize step so non-aggregate_distinct_byno longer silently drops the GROUP BY. - Add
reverse() |> _distinct_by(K)lowering viaMAX(pk)to select the “last” row per group by PK order. - Add
_group_by(K) |> _select((K=_._0, R=_._1 |> first()))support by expanding whole-row picks into source columns and reconstructing the struct in the row builder.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
modules/dasSQLITE/daslib/sqlite_linq.das |
Implements the new analyzer/finalize paths, reverse() handling, group-by-first recognition, SQL emission, and row reconstruction. |
tests/dasSQLITE/test_32_distinct.das |
Adds runtime + SQL-shape coverage for the new _distinct_by passthrough, reverse_distinct_by, and groupby_first behaviors. |
tests/dasSQLITE/failed_distinct_by_with_where.das |
Adds compile-fail coverage for explicitly unsupported `_where |
tests/dasSQLITE/failed_reverse_without_distinct_by.das |
Adds compile-fail coverage for reverse() used in _sql without _distinct_by. |
tests/dasSQLITE/failed_groupby_first_mixed_with_aggregate.das |
Adds compile-fail coverage for mixing first() with grouped aggregates in a single projection. |
tutorials/sql/12-distinct.das |
Demonstrates the new chain shapes end-to-end in the tutorial. |
doc/source/reference/tutorials/sql_12_distinct.rst |
Documents the new query shapes and v1 constraints/caveats. |
modules/dasSQLITE/API_REWORK.md |
Updates the shipped/deferred tracking for the sqlite_linq rework. |
benchmarks/sql/sqlite_linq_gaps.md |
Marks the “window-function lowerings” group as fully closed by bare-aggregate + composition. |
benchmarks/sql/distinct_by_order_take.das |
Adds the SQL (m1) lane using the new passthrough lowering. |
benchmarks/sql/distinct_by_order_to_array.das |
Adds the SQL (m1) lane using the new passthrough lowering. |
benchmarks/sql/reverse_distinct_by.das |
Adds the SQL (m1) lane using the new MAX(pk) variant. |
benchmarks/sql/groupby_first.das |
Adds the SQL (m1) lane using the new group-by-first tuple projection. |
benchmarks/sql/results.md |
Refreshes published benchmark results and notes based on the newly populated SQL lanes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nner subquery
Two Copilot R1 findings; both real silent-emission bugs in edge-case chain shapes.
1. **`take(N) |> _distinct_by(K)` silently dropped take**. Phase-divert
ran when `take` peeled (PHASE_TAKE=8 > PHASE_DISTINCT=3), populating
`q.innerSql` + `q.innerBindExprs`. `finalize_distinct_by_passthrough_wrap`
then overwrote innerSql with the bare-aggregate while orphaning the take's
bind. Probe confirmed: `_sql_text(... take(5) ... _distinct_by(_.Name))` emits
`SELECT … FROM (SELECT *, MIN(\"Id\") FROM \"Cars\" GROUP BY \"Name\")` with
the LIMIT silently gone. Runtime returned 2 rows instead of (at most) 5.
Fix: gate in `check_distinct_by_passthrough_gating` — reject when
`!empty(q.innerSql) || !empty(q.innerBindExprs)`. Clear macro_error points
the user to restructure as `_distinct_by(K) |> take(N)` for cap-after-distinct.
2. **`_group_by |> _select(...first()) |> _where(P)` was unsafe inside divert**.
`_where` triggers a SELECT divert (PHASE_WHERE forces SELECT into a wrap),
which wraps the grouped+first projection as inner. The row entry expands
to N source columns without aliases under `force_aliases=true`, and
`apply_passthrough_projection` expects 1 aliased column per record name.
Fix: in `divert_to_inner`, after `analyze_chain` on subQ returns:
(a) invoke `maybe_finalize_distinct_by_passthrough(subQ, ...)` — mirrors the
top-level rail so nested `_distinct_by` chains correctly wrap (closes
a separate silent-drop bug for nested chains).
(b) reject when subQ has any `groupedSelectIsRow=true` — clear macro_error
points the user to restructure (run wrap-triggering ops before
`_group_by`, or split into separate `_sql` calls).
Both rejections documented in API_REWORK.md "Deferred to chunk N+2" with the
follow-up design notes — these are explicit v1 constraints, not permanent.
Co-Authored-By: Claude Opus 4.7 <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.
Summary
Closes 4 SQL
—cells frombenchmarks/sql/sqlite_linq_gaps.md's "window-function lowerings" group. Plan-mode probing established that 3 of the 4 actually need composition over PR #2906's bare-aggregate trick, not window functions — the gaps doc'sROW_NUMBERprescription was conservative. The 4th (groupby_first) is a genuinely new arm but reuses the bare-aggregate machinery.distinct_by_order_take_distinct_by(K) |> _order_by(S) |> take(N)SELECT … FROM (SELECT *, MIN(pk) FROM t GROUP BY K) AS t0 ORDER BY S LIMIT Ndistinct_by_order_to_array_distinct_by(K) |> _order_by(S)reverse_distinct_byreverse() |> _distinct_by(K)SELECT … FROM (SELECT *, MAX(pk) FROM t GROUP BY K)groupby_first_group_by(K) |> _select((K=_._0, R=_._1|>first()))Bonus: closes a latent silent-drop bug from PR #2906 —
_sql(db \|> select_from(T) \|> _distinct_by(K))(no aggregate terminator) used to emitSELECT … FROM "T"with no GROUP BY. Now wraps in the bare-aggregate subquery via the new passthrough finalize.Arms (3)
Arm A —
_distinct_by(K)passthrough + composition with trailing_order_by/take/skip. Newfinalize_distinct_by_passthrough_wrapinvoked bymaybe_finalize_distinct_by_passthroughfrom everyanalyze_chainconsumer (_sql/_try_sql/_sql_text/_each_sql/_create_view). Outer SELECT preserves source columns viafill_default_columns; outer ORDER BY / LIMIT / OFFSET piggyback onbuild_sql_select's existing emission aroundq.innerSql. Gating rejects_where/_join/_group_by/_having/ set ops / additionaldistinct.Arm B —
reverse()recognition. Newanalyze_chainbranch at PHASE_DISTINCT; flips MIN(pk) → MAX(pk) in the bare-aggregate when wrapped by_distinct_by. Strict gating:reverse()only legal immediately above_distinct_by(SQL has no inherent row ordering). Barereverse() |> to_arrayrejects with a fixit pointing to_order_by_descending(...).Arm C —
_group_by(K) |> _select((K=_._0, R=_._1 |> first()))whole-row tuple projection.analyze_grouped_projectionrecognizes_._1 |> first()via the newis_first_of_group_callhelper, tracks per-entrygroupedSelectIsRowflags, and synthesizesq.distinctByColfromq.groupByCols[0]so the existing passthrough rail builds the inner subquery.build_sql_select's GroupedNamedTuple branch expands row entries to all source columns inline.build_row_builderemitsExprMakeStructfor row entries with per-fieldread_via_adapter(stmt, baseCol+j, type<FieldT>)calls (newbuild_row_make_structhelper mirrorsmake_sql_read_row_fn's pattern but with the offset baked in — no sqlite_boost change needed).v1 constraints
@sql_primary_key, single source table, no_join, no repeat_distinct_by)._where/_join/_group_by/_having/ set ops / additionaldistinct.reverse()immediately above_distinct_by; double-reverse rejects as a no-op.first()with column aggregates (length/sum/min/max/average); no computed-expression group keys withfirst().Test plan
mcp__daslang__run_test tests/dasSQLITE/test_32_distinct.das— 24 tests, 0 failures (11 from PR sqlite_linq: _distinct_by as chain operator + first-row aggregates #2906 + 13 new: 7 Arm A / 3 Arm B / 3 Arm C, each covering emit shape + runtime correctness)mcp__daslang__run_test tests/dasSQLITE/test_31_long_count_distinct_by_canary.das— 8 tests, 0 failures (PR sqlite_linq: _distinct_by as chain operator + first-row aggregates #2906 fast-path regression guard)mcp__daslang__run_test tests/dasSQLITE/test_05_sql_macro.das/test_07_sql_composability.das— sanity greenfailed_distinct_by_with_where.das,failed_reverse_without_distinct_by.das,failed_groupby_first_mixed_with_aggregate.dasmcp__daslang__run_script tutorials/sql/12-distinct.das— tutorial executable + emits expected SQL strings (3 new chain shapes demonstrated end-to-end)benchmarks/sql/*.das— drift documented inresults.mdheader (mostly converges back toward pre-sqlite_linq: _distinct_by as chain operator + first-row aggregates #2906 baselines)mcp__daslang__lintclean on every touched.dasfilemcp__daslang__format_fileno-op on every touched.dasfileDeferred to chunk N+2
_where(P) |> _distinct_by(K) |> ...— inner-WHERE composition (SELECT … FROM (SELECT *, MIN(pk) FROM t WHERE P GROUP BY K)); semantically distinct from outer-WHERE; needs design._group_by(K) |> _select((K=_._0, N=_._1|>length, R=_._1|>first()))— would require inner subquery to addCOUNT(*) AS group_countalongsideMIN(pk), and outer row builder to interleave scalar + offset row reads._._1 |> first()(single-key only today).ROW_NUMBER() OVER (PARTITION BY K ORDER BY S)) — not blocked by today's benches but useful for_distinct_by_min_by(K, S)shape if a future bench needs it.🤖 Generated with Claude Code