Skip to content

Commit 49d265c

Browse files
borisbatclaude
andcommitted
Address Copilot R1: reject pre-distinct phase-divert + row-entry as inner 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>
1 parent b2f9322 commit 49d265c

2 files changed

Lines changed: 22 additions & 0 deletions

File tree

modules/dasSQLITE/API_REWORK.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5567,6 +5567,8 @@ All 4 m1 lanes backfilled in `benchmarks/sql/{distinct_by_order_take,distinct_by
55675567
- Multi-key groups with `_._1 |> first()` (single-key only today).
55685568
- Computed-expression group keys with `first()` (only `_.<field>` keys today).
55695569
- Real window functions (`ROW_NUMBER() OVER (PARTITION BY K ORDER BY S)`) — not blocked by today's benches but useful for `_distinct_by_min_by(K, S)` ("min-S row per K") shape if a future bench needs it.
5570+
- **Pre-distinct phase-divert composition** (Copilot R1 #2909). Chains like `take(N) |> _distinct_by(K)` or `skip(N) |> _distinct_by(K)` trigger `divert_to_inner` BEFORE `_distinct_by` peels — `q.innerSql`/`q.innerBindExprs` get populated by the take's inner subquery, and chunk N+1's passthrough finalize would overwrite the inner SQL while orphaning the take's bind. Currently rejected with a clear macro_error pointing the user to restructure as `_distinct_by(K) |> take(N)` (cap-after-distinct semantics). A future composition would 2-level wrap: `SELECT … FROM (SELECT *, MIN(pk) FROM ({prior_innerSql}) GROUP BY K) AS t0` to preserve cap-before-distinct semantics — needs design + tests for arbitrary phase-divert combinations.
5571+
- **Whole-row projection as inner subquery** (Copilot R1 #2909). `_group_by(K) |> _select((K=_._0, R=_._1 |> first())) |> _where(P) |> to_array` triggers `divert_to_inner` (PHASE_WHERE forces a SELECT divert), which would wrap the grouped+first projection as inner. The row entry expands to N source columns without `AS <alias>` under `force_aliases=true`, and `apply_passthrough_projection` expects 1 column per record name. Currently rejected with a clear macro_error pointing the user to restructure (run the wrap-triggering op before `_group_by`, or split into separate `_sql` calls). Resolution options: (a) emit deterministic per-field aliases (`"col1" AS "<RecordName>_col1"`) under `force_aliases` and teach `apply_passthrough_projection` to reconstruct the row entry from those, or (b) keep the rejection and document the constraint. Same PR could lift the v1 single-key-only constraint on `first()` projections.
55705572

55715573
## Plan (to be written after all tutorials are reviewed)
55725574

modules/dasSQLITE/daslib/sqlite_linq.das

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -643,6 +643,12 @@ def private check_distinct_by_passthrough_gating(var q : SqlQuery&; prog : Progr
643643
q.hadError = true
644644
return false
645645
}
646+
// A phase-divert (e.g. `take(N) |> _distinct_by(K)` — take's PHASE_TAKE > distinct_by's PHASE_DISTINCT triggers divert_to_inner) populates q.innerSql + q.innerBindExprs before _distinct_by peels. finalize_distinct_by_passthrough_wrap would overwrite q.innerSql (silently dropping the pre-distinct op) and leave q.innerBindExprs orphaned (bind/placeholder mismatch). Reject loudly until composition with pre-distinct phase-divert is designed.
647+
if (!empty(q.innerSql) || !empty(q.innerBindExprs)) {
648+
macro_error(prog, at, "_sql: chain operators that phase-divert before `_distinct_by(K)` (e.g. `take(N) |> _distinct_by(K)` or `skip(N) |> _distinct_by(K)`) are not yet supported — restructure so `_distinct_by` sits directly above the source (e.g. `_distinct_by(K) |> take(N)` for cap-after-distinct semantics)")
649+
q.hadError = true
650+
return false
651+
}
646652
return true
647653
}
648654
@@ -1431,6 +1437,20 @@ def private divert_to_inner(var node : ExpressionPtr; var q : SqlQuery&;
14311437
macro_error(prog, at, "_sql: cannot lower a chain whose inner subquery projects a single value (`_select(_.X)` or an aggregate) into an outer query — there is no named row to filter on. Restructure the chain so the divergent op runs before the projection, or wrap the projection in a named tuple `(X=_.X)`.")
14321438
return false
14331439
}
1440+
// Mirror the top-level emit_sql_macro_body path: a nested `_distinct_by(K)` chain (set by analyze_chain via the wrapped chain-op branch) needs the bare-aggregate inner-subquery wrap built BEFORE build_sql_string runs — otherwise the subquery silently drops the dedup (same silent-drop bug chunk N+1 fixed at the top level).
1441+
if (!maybe_finalize_distinct_by_passthrough(subQ, prog, at)) return false
1442+
// Whole-row entries in a grouped projection (`_._1 |> first()`) expand to N source columns in the inner SQL but apply_passthrough_projection / outer SELECT expect 1 aliased column per record name. Reject when this projection shape is being wrapped as an inner subquery; user must keep `_group_by |> _select(...first())` at the outermost level (no `_where` / `_take` after it that would trigger divert).
1443+
var anyRowEntry = false
1444+
for (b in subQ.groupedSelectIsRow) {
1445+
if (b) {
1446+
anyRowEntry = true
1447+
break
1448+
}
1449+
}
1450+
if (anyRowEntry) {
1451+
macro_error(prog, at, "_sql: `_group_by |> _select((K=_._0, R=_._1 |> first()))` cannot be wrapped as an inner subquery — the whole-row entry expands to multiple unaliased columns that the outer query can't reference by name. Restructure so the chain that triggers wrap (e.g. `_where`) runs before `_group_by`, or split into separate `_sql` calls.")
1452+
return false
1453+
}
14341454
// nolint:STYLE015 force_aliases=true: outer references inner cols by alias. Without AS, SQLite
14351455
// names the column as the raw expr (e.g. `COUNT(*)`); outer can't bind `_.alias` → `"alias"`.
14361456
let innerSql = build_sql_string(subQ, true)

0 commit comments

Comments
 (0)