sqlite_linq: expression keys in _group_by + PR #2845 review fixes#2847
Merged
Conversation
Two improvements this PR: 1. Expression-key support in `_group_by` (Session 2 from the post-PR #2843 plan). `_group_by(_.Price % 100)` (and tuples mixing field keys with expression keys) now lower to `GROUP BY ((price) % (100))`. The rendered fragment is reused verbatim in SELECT (`K = _._0`) and ORDER BY (`_order_by(_._0)`) positions, so the SQL stays a single source of truth across all three clauses. Mechanism: `collect_group_keys` calls `pred_to_sql` with a new `q.inlineConstants` mode so the bound `_.X` field refs render as columns and `ExprConst*` literals (ints, floats, strings, bools) inline as SQL literals instead of `?` placeholders. The fragment carries no binds, which lets us re-use it at multiple SQL positions without bind-position bookkeeping. Runtime values (`_.Col - capturedVar`) reject loudly with a precise diagnostic — same one exercised by `failed_sql_macro` case 25. Order swap in the `_group_by` peel: recurse into the source first so `q.rootType` is set before `pred_to_sql` runs on the key (the existing field-key path didn't need this since translation was deferred to emission; expression keys do). Backfills `benchmarks/sql/groupby_select_sum.das` m1 lane. The bench uses the explicit-inner-select shape inside SUM (`_._1 |> select($(c : Car) => c.price) |> sum()`) — m3f/m4 keep their splice-friendly bare-`sum` form; both emit equivalent SQL/compute. results.md refreshed per the living-doc policy, "Notes on missing lanes" bullet for `groupby_select_sum SQL` removed. 2. Two PR #2845 review fixes (Copilot, both real): - `peel_count_terminal`'s predicate-overload error mis-named the terminal: it said `_count(predicate)` and suggested `_count()`, but in `_sql` chains the bare `count()` / `long_count()` linq functions are the actual terminals. Drop the underscores in both the offending name and the suggested fix. - `try_peel_distinct_by_field` pinned receiver to `ExprVar` but didn't verify the var IS the lambda's bound parameter. So `_distinct_by(capturedRow.Brand) |> count()` would silently emit `COUNT(DISTINCT "Brand")` against the SQL source — wrong result, no diagnostic. Now extracts the lambda's arg name from `keyLambda._block.arguments[0]` and rejects when receiver name differs (`failed_sql_macro` case 24). Test surface: - `tests/dasSQLITE/test_32_group_by_expression_keys.das` — 6 tests covering SQL emission + runtime for single expression keys, multi-key tuples mixing field + expression keys, ORDER BY on the group key, and aggregate-over-expression-key projection. - `failed_sql_macro.das` cases 24 + 25 added (50503:23). Validation: 796/796 dasSQLITE (interp + AOT), 1390/1390 linq, 0 JIT bench failures, lint clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends sqlite_linq’s _group_by lowering to support computed (expression) keys by rendering the key expression once (with inlined literals) and reusing the same SQL fragment across SELECT, GROUP BY, and ORDER BY. It also incorporates two review-driven correctness fixes around _distinct_by(... ) |> count() and count/long_count terminal error messaging, and updates tests/benchmarks accordingly.
Changes:
- Add expression-key support for
_group_byby caching a bind-free SQL fragment (with constant literals inlined) and reusing it across clauses. - Fix
_distinct_bytranslation to reject outer-capture keys that would otherwise silently miscompile, and adjustcount/long_countpredicate error wording. - Add targeted tests for expression keys + update SQL benchmark lane + refresh benchmark results documentation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| modules/dasSQLITE/daslib/sqlite_linq.das | Implements expression-key handling for _group_by (including literal inlining) and folds in _distinct_by/count review fixes. |
| tests/dasSQLITE/test_32_group_by_expression_keys.das | New coverage for SQL emission and runtime correctness of expression-key group-by (single + tuple keys, order-by reuse, aggregates). |
| tests/dasSQLITE/failed_sql_macro.das | Adds new negative cases for outer-capture distinct_by keys and runtime-value expression group keys; updates expected error counts. |
| benchmarks/sql/groupby_select_sum.das | Backfills the SQL lane using the new expression-key support and updates bench commentary accordingly. |
| benchmarks/sql/results.md | Refreshes benchmark tables and removes the previously-missing-lane note for groupby_select_sum SQL. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Two improvements in one PR:
1. Expression-key support in
_group_by(Session 2 of post-#2843 plan)_group_by(_.Price % 100)(and tuples mixing field keys with expression keys) now lower toGROUP BY (("Price") % (100)). The rendered fragment is re-used verbatim inSELECT(K = _._0) andORDER BY(_order_by(_._0)) positions, so the SQL stays a single source of truth across all three clauses.Mechanism:
collect_group_keyscallspred_to_sqlwith a newq.inlineConstantsmode so the bound_.Xfield refs render as columns andExprConst*literals (ints, floats, strings, bools) inline as SQL literals instead of?placeholders. The fragment carries no binds, which lets it be re-used at multiple SQL positions without bind-position bookkeeping.Runtime values reject loudly:
_.Col - capturedVarwould need bind re-pushing at each use site, so the analyzer rejects with a precise diagnostic (failed_sql_macrocase 25).Order swap in the
_group_bypeel: recurse into the source first soq.rootTypeis set beforepred_to_sqlruns on the key (the existing field-key path didn't need this since translation was deferred to emission; expression keys do).Bench:
benchmarks/sql/groupby_select_sum.dasm1 backfilled. The bench uses the explicit-inner-select shape inside SUM (_._1 |> select($(c : Car) => c.price) |> sum()); m3f/m4 keep their splice-friendly bare-sumform; both emit equivalent SQL/compute.results.mdrefreshed per the living-doc policy (groupby_select_sum SQLbullet removed from the missing-lanes notes).2. PR #2845 review fixes (Copilot, both real)
peel_count_terminalerror wording — said_count(predicate)and suggested_count(); the actual terminals in_sqlchains are barecount()/long_count(). Underscores dropped in both the offending name and the suggested fix.try_peel_distinct_by_fieldouter-capture silent miscompile — receiver was pinned toExprVarbut the var wasn't checked against the lambda's bound parameter._distinct_by(capturedRow.Brand) |> count()would silently emitCOUNT(DISTINCT "Brand")against the SQL source — wrong result, no diagnostic. Now extracts the lambda's arg name and rejects on mismatch (failed_sql_macrocase 24).What's in the diff
modules/dasSQLITE/daslib/sqlite_linq.das— newq.inlineConstantsflag + ExprConst* inlining inpred_to_sql; newpush_group_keyhelper called bycollect_group_keys;_group_bypeel recurses-first;analyze_grouped_projection/collect_order_keys/ GROUP BY emitter all branch ongroupByKeyExprs[i] != nullto use the cached expression fragment. Copilot fixes folded in.tests/dasSQLITE/test_32_group_by_expression_keys.das(new) — 6 tests covering SQL emission + runtime values for single expression keys, multi-key tuples mixing field + expression keys, ORDER BY on the group key, and aggregate-over-expression-key projection.tests/dasSQLITE/failed_sql_macro.das— cases 24 (outer-capture distinct_by) + 25 (runtime-value expression key) added;expect 50503bumped 21 → 23.benchmarks/sql/groupby_select_sum.das— m1 lane backfilled; bench comment refreshed (drops the dated TODO, notes the lane-specific shape).benchmarks/sql/results.md— INTERP + JIT tables refreshed; commit-hash header bumped; missing-lanes bullet forgroupby_select_sum SQLremoved.Test plan
bin/daslang dastest/dastest.das -- --test tests/dasSQLITE/— 796/796 pass (interpreted)bin/test_aot dastest/dastest.das -- --test tests/dasSQLITE/— 796/796 pass (AOT)bin/daslang dastest/dastest.das -- --test tests/linq/— 1390/1390 pass (no shared-machinery regression)benchmarks/sql/— 0 failures, results.md refreshedmcp__daslang__lint+format_fileon all touched files — clean🤖 Generated with Claude Code