feat(drivers): lower count_distinct on the SQL family — the enforce half of the #6188 ruling (#6409) - #6816
Merged
Conversation
Implementation half of the #6188 split ruling: count_distinct stays declared and now compiles to COUNT(DISTINCT column) on driver-sql's SqlDriver.aggregate and driver-turso's RemoteTransport.aggregate. Adds the shared AGGREGATION_CASES conformance table so the two faces are pinned against each other on values rather than on a side-by-side read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RWMbS2ctjZSADPJiSdUtMA
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RWMbS2ctjZSADPJiSdUtMA
…nce, add changeset (#6409) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RWMbS2ctjZSADPJiSdUtMA
…trix (#6409) Enrols driver-sql, driver-turso and driver-sqlite-wasm; records honest DEBT rows for the two #5499-frozen packages (tracked as #6814). Regenerates the spec API surface for the new shared exports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RWMbS2ctjZSADPJiSdUtMA
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 3 package(s): 116 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
github-actions
Bot
added
size/xl
documentation
os-zhuang
marked this pull request as ready for review
August 8, 2026 22:48
…erQuery declares (#6409) `sort` is not a member of QueryAST; the fixture read-back compiled locally only because the package typecheck had not been re-run since the suite was written. Caught by CI's workspace typecheck, which the local gate enumeration had missed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RWMbS2ctjZSADPJiSdUtMA
os-zhuang
marked this pull request as draft
August 8, 2026 23:09
Contributor
Author
|
ACCEPT — drivers lane, PM dispatch loop. Step-7 gate passed on all six criteria, each verified against the diff and the JOB logs rather than off the report's own claims.
CI gated on the JOB conclusions: 26 check runs on Two things the seat is recording as credit rather than passing over:
The three open questions (producer deletion; enrolling Marking ready and enabling auto-merge. The full suite runs again on the speculative merge result in the queue; per #4859 a red queue build gets diagnosed before any requeue, not blind-requeued. Generated by Claude Code |
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
2 participants
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.
Fixes #6409
Premise, re-verified before implementing
The issue's lead held. On
origin/main@6de592c, both SQL faces refusedcount_distinctwithNOT_IMPLEMENTED/ 501, and the two pin files recorded it as the sole inhabitant of #5907's class 2. The issue's line anchors had drifted (as the dispatch predicted) —mapAggregateFuncis atsql-driver.ts:8035on this branch, not:7071— but the shape was exactly as described.One thing the issue's framing understates and this PR had to handle:
AggregationFunctiondeclares six values andSQL_AGGREGATE_FUNCTIONScompiled five, so landing the sixth empties class 2 entirely. That is not a side effect to tidy — it is a decision about #5907's capability-gap producer, taken explicitly below.What changed
The lowering, on both faces in one change
packages/drivers/driver-sql/src/sql-driver.ts—SQL_AGGREGATE_FUNCTIONSnow maps each name to aSqlAggregateLoweringrecord ({ sql, distinct }) instead of a bare SQL function name, andSqlDriver.aggregateemitscount(distinct ??)for the distinct entry. The column is still bound as a knex identifier; the keyword is fragment text, never a binding.packages/drivers/driver-turso/src/remote-transport.ts— the twin,RemoteTransport.aggregate, emittingcount(distinct "column")through its own hand-built SELECT list.Both faces, one change, deliberately:
TursoDriverpicks between them fromurl, so a lowering that landed on one alone would be #6203's shape again — one query, two answers, decided by a connection string.mapAggregateFunc's return type moved fromstringto the record. It isprotectedand had exactly one caller in tree; every entry answering{ sql, distinct: false }compiles to byte-identical text to what thestringreturn produced.Semantics: distinct NON-NULL values
Measured rather than assumed, against a real better-sqlite3 database (local face) and SQLite behind the libsql interface (remote face):
COUNT(DISTINCT col)excludes NULLs on both. That matches the standard's definition ofCOUNTover non-null values (PostgreSQL, MySQL alike), and matches whatobjectql's in-memory fallback andservice-analytics'sAGGREGATE_SQLalready answer. The fixture pins it:count(*)is 6,count(stage)is 4,count_distinct(stage)is 2 — three different numbers over one column, so a lowering that collapsed into either neighbour fails on a value.fieldis now required forcount_distinct— a new refusalAggregationNodeSchemamakesfieldoptional becauseCOUNT(*)is a real spelling and the schema cannot say "optional for this function, required for that one". So{ function: 'count_distinct', alias: 'n' }parses, reaches the driver, and asks forCOUNT(DISTINCT *)— a syntax error in every dialect. Both faces now refuse it up front withINVALID_QUERY/ 400 and a message naming the fix, rather than emitting it and letting a dialect error arrive as an opaque 500 (mapDataError's default branch — the #1116/#1117 gap #5907 closed at this same door).Class 1, not class 2, and the messages say so: the function is compiled here; it is this aggregation node that is malformed. Plain
countwith nofieldstill meansCOUNT(*), pinned by a control on each face.Class 2 is now empty — and its producer is kept, on purpose
With
count_distinctlowered,AggregationFunctionand each face's compiled vocabulary are the same set, souncompilableAggregateFunctionErrorhas nothing to produce.Deleting it as dead code was considered and rejected. That branch is not an unenforced declaration — the ADR-0049 shape — it is the classifier deciding which of two truths a future name is told. Removing it does not remove the condition; it makes
refuseAggregateFunctionanswer 400 for the first function a later spec bump adds, telling the author of a correctly-spelled name that the protocol has no such function. That is precisely the misreport #5907 exists to prevent, and it would land in the window ADR-0049's enforce leg opens on purpose — the window #6188 just opened for this very name. Cost of keeping: one unreachable branch. Cost of dropping: a wrong answer at the moment the vocabulary grows.Both pin files assert the emptiness positively (
the declared-but-uncompiled set is EMPTY), so a spec addition goes red there rather than quietly leaving the block covering nothing.The rejection message
Both faces build their
Compiled here:list from the lowering table, so it now readscount, sum, avg, min, max, count_distinct. Asserted off a real refusal on each face, not off a literal — the acceptance criterion "the rejection message no longer namescount_distinctas unsupported", made checkable.Shared conformance — the multi-face rule
New shared case-set:
packages/spec/src/data/aggregation-conformance.ts—AGGREGATION_ROWS(six rows, two groups, a nullable duplicate-bearing column) andAGGREGATION_CASES(twelve cases covering the whole compiled vocabulary, ungrouped and grouped). Not a standalone test file: the two SQL lowerings are pinned against each other, on values, which is the only instrument that can see this class of bug — every wrongcount_distinctlowering still emits valid SQL and returns a plausible number.Driven by three suites, all executing against a real engine:
driver-sqlsql-driver-aggregation-conformance.test.tsdriver-tursoREMOTEturso-remote-aggregation-conformance.test.tsmakeLibsqlSqliteStubdriver-sqlite-wasmsqlite-wasm-aggregation-conformance.test.tsscripts/check-driver-conformance.mjsclassifies the new column (it failed CLASSIFIED until it did — the gate working as designed). The two remaining cells carry honest DEBT rows, measured from the source, not flipped and not faked:driver-memory—MemoryDriver.computeAggregatehas nocount_distinctarm; theswitchfalls todefault: return null, so the aggregation resolves with{ n: null }. A wrong answer, not a wrong number.driver-mongodb—count_distinctlowers to$addToSetand is sized by.length, so an explicitnullcounts as a distinct value: 3 where the standard says 2.Both are inside the #5499 freeze and #6409's ruling put them explicitly out of scope; their partial implementations are untouched. Filed as #6814 so the rows point at a tracked decision rather than at a note.
Pins flipped, and what replaced them
sql-driver-out-of-contract-aggregate-function.test.tsandremote-transport-aggregate-function-refusal.test.tsboth recordedcount_distinct -> 501. Flipped, never merely deleted — each old assertion was replaced by the new substance: the empty-set assertion, the emittedcount(distinct …)/ the computed value, and the refusal wording that now lists the function as compiled. The remote file's parity block losescount_distinctas a refusal case and gains two: both faces answer it identically, and both refuse the field-less spelling identically (INVALID_QUERY/400, one wording).Class 1 is untouched throughout —
median,array_agg,string_aggand every miscased spelling are still undeclared names, and their refusals stay verbatim. A pin is flipped only where the fact under it moved.Reverse verification — predicted, then measured
Two reverts on both faces, direction written down before running.
(A) the
count_distinctentry deleted (the pre-#6409 state). Predicted: thecount_distinctcases fail by throwing 501, never on a value. Measured: local 5 failed / 11 passed of 16, remote 6 failed / 11 passed of 17 — every value case on the thrown 501, none on a number. One failure was not predicted and is recorded rather than tidied: the new field-less refusal case went red onexpected 'NOT_IMPLEMENTED' to be 'INVALID_QUERY', which is what proves the two refusals are distinguishable rather than interchangeable.(B)
distinctflipped tofalse— the copied-neighbour mistake, and the one a review that only checked the two faces' tables have the same keys would pass. Predicted: failures on VALUES, withcount_distinct(score)staying green because that column has nothing to dedup. Measured: local 4 failed / 12 passed, remote 5 failed / 12 passed, onexpected 4 to be 2andwest 3 vs 2— the same two wrong numbers on both faces, andcount_distinct(score)green throughout, exactly as predicted. That agreement is the pairing doing its job.(B) is the direction this table exists for; (A) is reachable by any test that merely calls the function.
Spec-side wording
AggregationFunction's JSDoc:count_distinctno longer "leads its implementation"; the NULL semantics and the requiredfieldare stated.AGG_RETIRED_MIDDLE(the livearray_agg/string_aggparse-error prescription) said the two SQL faces "lower the same five functions", which this PR makes false. Corrected to "the same set" — one character shorter, deliberately:packages/rest'sanalytics-dataset-refusal-envelope.test.tsasserts that the actionable "Delete the aggregation" survives thedetailtruncation window, and a longer edit pushed it out. Caught by that test, not by review.The #6188 conversion/migration registry entries are not touched: they narrate the state at that ruling and already say the SQL lowering "follows on its own card". Historical narration, accurate as history.
os-dev report
Issue: #6409 — implement
count_distinctin the SQL family (the enforcement half of the #6188 ruling)PR: #6816 (draft) · branch
claude/issue-6409-count-distinct-sql-family· head08f803epremise_still_valid: true
CI: green — all 25 checks completed success or skipped on
08f803e.(The sections above are the detail; this is the structured summary, posted identically as a comment on #6409.)
Gates and tests
.github/workflows/lint.yml, run step by step (not from memory): 58 / 58 PASS. Workspace built first so thecheck:i18n/check:i18n-coverage/check:app-nav-i18ntrio ran for real; each step's exit status read directly, never through a pipe.check:driver-conformance(CLASSIFIED — the new case-set was unregistered) andcheck:api-surface(five new exports; snapshot regenerated withgen:api-surface).driver-sql1095✓/48 skipped ·driver-turso902✓ ·driver-sqlite-wasm302✓ ·driver-memory540✓ ·driver-mongodb214✓/143 skipped ·spec8861✓ ·objectql2656✓ ·service-analytics1415✓ ·metadata-protocol740✓ ·rest1097✓.turbo run typecheck(120 tasks) and both enumeratedturbo run buildfilters: green.One CI round was red, and it is worth recording why the local gate list did not catch it.
TypeScript Type Checkfailed on9b9b429withsrc/sql-driver-aggregation-conformance.test.ts(121,63): error TS2353: 'sort' does not exist in type 'DriverQuery'— the fixture read-back usedsortwhereQueryASTdeclaresorderBy. My enumeration fromlint.ymltranscribed thecheck:*steps but omitted the workspace-wideturbo run typecheck/turbo run buildsteps in the same workflow, and the per-package typecheck I had run was run before that suite was written. Fixed in08f803e, and the two missing commands were then run locally and are green. The lesson is the enumeration's, not the type error's: "everycheck:*step" is narrower than "every step".Out-of-scope findings (search-first dedup, filed unassigned)
count_distinct— the two AGGREGATION_CASES cells #6409 left open #6814 —driver-memory/driver-mongodbdisagree with the SQL family oncount_distinct; the two DEBT cells above, with the measured evidence for each. [裁决] driver-memory / driver-mongodb 投入冻结 —— 维护者 2026-08-05 口径（跨单锚点） #5499-frozen, so this is a decision about the freeze rather than about the code.AggregationNode.distinctis honoured by the in-memory fallback and ignored by every SQL face — one query, two numbers (ADR-0049) #6815 —AggregationNode.distinctis honoured byobjectql's in-memory fallback and ignored by every SQL face, so{ function: 'sum', distinct: true }returns a deduplicated sum in memory and an ordinary sum on SQL: one query, two numbers, no diagnostic. Searched first — not covered by [P2] data:QueryASTdeclares 12 members no executor runs — the liveness ledger governs metadata types, not the request surface #4286's request-surface sweep, which dispositionedQueryAST.distinctandAggregationNode.filter. Both ADR-0049 legs argued, no recommendation offered.AggregationNode.distinctis honoured by the in-memory fallback and ignored by every SQL face — one query, two numbers (ADR-0049) #6815's scope note rather than filed separately:sum/avg/min/maxwritten with nofieldstill emitsum(*)and die as a dialect syntax error with no ADR-0112 envelope — same class as the refusal added here. This PR deliberately did not widen its refusal surface to cover it.Open questions for the maintainer (三轴自裁, recorded not blocked)
uncompilableAggregateFunctionErroron both faces is a small follow-up — the positive emptiness assertions are already in place to make the deletion honest.AGGREGATION_CASES' enrolment boundary. Enrolled the three SQL-family drivers.objectql's in-memory fallback is a fourth lowering that agrees with the table and is not frozen; it is recorded in the case-set as a candidate rather than a debt, because reaching it needs the engine rather than a driver. Enrolling it is a judgement call I left to the seat that owns the objectql lane.service-analytics'sAGGREGATE_SQLis a fifthCOUNT(DISTINCT …)lowering (the Cube face). Left unenrolled on scope discipline — drivers: implementcount_distinctin the SQL family — enforcement half of the #6188 ruling #6409 named the driver family — but it is the obvious next column if the table is meant to cover every SQL-emitting face.Not done, and why
Nothing in the issue's scope was skipped.
driver-memory/driver-mongodbare out of scope by the ruling and stayed untouched; their verdicts are DEBT rows, not changes.content/docs/releases/untouched. The #6188 conversion/migration registry entries were left alone — accurate as history.