Skip to content

drivers: implement count_distinct in the SQL family — enforcement half of the #6188 ruling #6409

Closed

Description

@os-project-manager

Follow-up filed from the 2026-08-07 maintainer ruling on #6188 (decision batch accepted via the PM session). Unclaimed; no labels applied — routing belongs to the triage seat.

Ruling context

#6188 was ruled as a split: array_agg / string_agg retire from AggregationFunction (that card keeps the spec retirement half), while count_distinct stays declared and gets enforced — this card is the enforcement half.

Scope

Implement count_distinct in the SQL family:

  • packages/drivers/driver-sql — SqlDriver.mapAggregateFunc (currently rejects it alongside the two now-retiring keys): lower to COUNT(DISTINCT column).
  • packages/drivers/driver-turso (or wherever RemoteTransport.aggregate lives per current main) — same lowering on the remote path.

Out of scope: driver-mongodb / driver-memory (inside the #5499 investment freeze; their existing partial implementations are untouched either way).

Acceptance

  • An analytics query using count_distinct compiles and returns correct values on the SQL family (dedup semantics: distinct non-null values of the target column).
  • A conformance cell covering count_distinct is added to the shared cases so the two SQL lowerings are pinned against each other (multi-face component rule: shared coverage, not a standalone test file).
  • The rejection message for unknown aggregation functions no longer names count_distinct as unsupported.

Ordering

Independent of the #6188 retirement half (different keys, different files); no Blocked-by needed. If the retirement PR touches the same mapAggregateFunc switch, serialize per same-file discipline.

Refs: #6188 (ruling), #5907 (runtime capability-gap envelope — its wording loses these keys as subjects once both halves land), #5499 (freeze boundary).

Activity

  1. os-zhuang commented on Aug 7, 2026

    @os-zhuang
    Contributor

    Triage: pm:queue + domain:drivers + target:v17.

    Domain: packages/drivers/driver-sql (SqlDriver.mapAggregateFunc) and the turso remote path ⇒ the domain:drivers row. Routing confirmed rather than taken on faith from the filing note.

    Freeze check — clears. The maintainer's 2026-08-05 investment freeze (#5499) covers driver-memory and driver-mongodb only. This card is the SQL family and explicitly holds the two frozen drivers out of scope, so it queues normally; no pm:on-hold.

    target:v17 applied under the release-board criterion ②, declared ≠ enforced on a public contract: the #6188 ruling keeps count_distinct declared in AggregationFunction while the SQL family still rejects it, so shipping in that state means the schema promises an aggregation the engine refuses. The retirement half (#6188) already carries target:v17; tagging this half keeps the pair on the board together, which is what the ruling's split implies. If the drivers seat judges the exposure narrower, escalate as a mis-tag — ⛔ do not self-remove.

    Ordering, as filed: independent of #6188 (different keys, different files), so no Blocked-by:. The one caveat worth repeating in the claim comment: if the retirement PR is in flight on the same mapAggregateFunc switch, same-file discipline applies and one of the two waits.

    本评论来自分诊座位 Routine(#5474 试点),不构成认领。


    Generated by Claude Code

  2. self-assigned this
    on Aug 8, 2026
  3. os-zhuang commented on Aug 8, 2026

    @os-zhuang
    Contributor

    Claim: PM loop, drivers lane (serial sql-driver.ts chain, position 2 of 3) — dispatched under the maintainer's standing authorization of 2026-08-08 ~17:15Z: 「6577 落地后继续按序派发,不用等我确认」.
    Session: session_01Hg9Pkg5nDedCRihRsdeCdX
    Branch: claude/issue-6409-count-distinct-sql-family
    Worktree: objectstack-issue-6409 (cloud session — own container, Opus)
    Domain: domain:drivers
    File surface: packages/drivers/driver-sql/src/sql-driver.ts (mapAggregateFunc), packages/drivers/driver-turso/src/remote-transport.ts (the remote aggregate lowering), the shared aggregate conformance case-set, and packages/drivers/driver-sql/src/sql-driver-out-of-contract-aggregate-function.test.ts (it currently pins count_distinct as REFUSED on both faces — that pin flips, and flipping it must assert the new substance, not merely delete the old assertion).
    Serial constraints cleared: PR #6809 (issue #6543) MERGED — it was the last in-flight change to sql-driver.ts; its diff touched the unique-violation discriminator and createNullSafeUniqueIndex, disjoint from mapAggregateFunc. Re-priced against that diff and against #6793/#6706 earlier today: no effect — different function, no shared seam. The triage caveat about the #6188 retirement PR racing this switch is discharged: #6188 closed completed 2026-08-08T03:43Z, so array_agg / string_agg are already out and nothing is in flight on that switch. #6401 remains queued behind this card.

    Stale-premise re-verified on current origin/main: both SQL faces still refuse — sql-driver-out-of-contract-aggregate-function.test.ts records LOCAL count_distinct -> THREW … "Unsupported aggregate function: count_distinct" and the same for REMOTE.


    Generated by Claude Code

  4. os-zhuang commented on Aug 8, 2026

    @os-zhuang
    Contributor

    os-dev report

    Issue: #6409 — implement count_distinct in the SQL family (the enforcement half of the #6188 ruling)
    PR: #6816 (draft) · branch claude/issue-6409-count-distinct-sql-family · head 08f803e
    premise_still_valid: true
    CI: green — all 25 checks completed success or skipped on 08f803e.

    Premise verification (before implementing)

    Re-verified on origin/main @ 6de592c. Both SQL faces refused count_distinct with NOT_IMPLEMENTED / 501, and both pin files recorded it as the sole inhabitant of #5907's class 2. The issue's line anchors had drifted as the dispatch predicted (mapAggregateFunc sits at sql-driver.ts:8035 on this branch, not :7071); located by pattern instead.

    One thing the issue's framing understated and the work had to decide: AggregationFunction declares six values and the lowering table compiled five, so landing the sixth empties class 2 entirely. Handled explicitly rather than as a side effect — see below.

    What was delivered

    Lowering, on both SQL faces in one change. SQL_AGGREGATE_FUNCTIONS / REMOTE_AGGREGATE_FUNCTIONS now map each name to a { sql, distinct } record instead of a bare function name; SqlDriver.aggregate emits count(distinct ??) (column bound as a knex identifier, keyword as fragment text) and RemoteTransport.aggregate emits count(distinct "column"). Both faces together, deliberately — TursoDriver picks between them from url, so a one-sided lowering would be #6203's shape again.

    Semantics: distinct NON-NULL values. Measured, not assumed, against a real better-sqlite3 database and SQLite behind the libsql interface: COUNT(DISTINCT col) excludes NULLs on both. Agrees with objectql's in-memory fallback and service-analytics's AGGREGATE_SQL. The fixture makes the three neighbours disagree on purpose — count(*)=6, count(stage)=4, count_distinct(stage)=2 — so a lowering that collapsed into either fails on a value.

    New refusal: field is required for count_distinct. COUNT(DISTINCT *) is a syntax error everywhere; AggregationNodeSchema cannot say "optional for this function, required for that one". Both faces refuse it up front with INVALID_QUERY / 400 (class 1 — the function is compiled here; the node is malformed) instead of emitting it and letting a dialect error arrive as an opaque 500. Plain count with no field still means COUNT(*), pinned by a control on each face.

    Class 2 emptied — producer kept, argued. uncompilableAggregateFunctionError now has nothing to produce. Deleting it was considered and rejected: it is not an unenforced declaration (the ADR-0049 shape) but the classifier deciding which of two truths a future name is told, and removing it makes refuseAggregateFunction answer 400 for the first function a later spec bump adds — the exact misreport #5907 exists to prevent, inside the window ADR-0049's enforce leg opens on purpose. Both pin files assert the emptiness positively so a spec addition goes red there.

    Rejection wording. Both faces build the "Compiled here:" list from the lowering table, so it now reads count, sum, avg, min, max, count_distinct — asserted off a real refusal on each face, not off a literal. The acceptance criterion is checkable rather than assumed.

    Shared conformance (multi-face rule). New packages/spec/src/data/aggregation-conformance.ts — AGGREGATION_ROWS (six rows, two groups, a nullable duplicate-bearing column) and AGGREGATION_CASES (twelve cases, whole compiled vocabulary, ungrouped and grouped). Not a standalone test file: the two SQL lowerings are pinned against each other on values, the only instrument that sees this class of bug — every wrong count_distinct lowering still emits valid SQL and returns a plausible number. Driven by three suites, each against a real engine: driver-sql (better-sqlite3), driver-turso REMOTE (makeLibsqlSqliteStub), driver-sqlite-wasm (sql.js dialect).

    Pins flipped to the new substance, never merely deleted. Each 501 assertion was replaced: the empty-set assertion, the emitted count(distinct …) / computed value, and the wording that now lists the function as compiled. The remote parity block loses count_distinct as a refusal case and gains two — both faces answer it identically, and both refuse the field-less spelling identically. Class 1 (median, array_agg, string_agg, miscased spellings) untouched, verbatim.

    Changeset: patch on @objectstack/driver-sql, @objectstack/driver-turso, @objectstack/spec.

    Frozen packages — honest DEBT, no flips, no fake green

    The new matrix column forced a verdict for all five drivers (check:driver-conformance CLASSIFIED failed until classified — the gate working as designed). Three enrolled; the two #5499-frozen ones carry DEBT rows measured from source, tracked as #6814:

    Driver Verdict Evidence
    driver-memory RED — answers null MemoryDriver.computeAggregate has no count_distinct arm; default: return null. A wrong answer, not a wrong number. Its analytics face does implement it, so the package answers one declared function two ways.
    driver-mongodb RED — counts NULL $addToSet sized by .length, so an explicit null is a distinct value: 3 where the standard says 2. Read from source; not executed.

    Neither package was touched. Neither cell was flipped or faked.

    Reverse verification — predicted, then measured

    (A) the count_distinct entry deleted (pre-#6409 state). Predicted: the count_distinct cases 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 on expected 'NOT_IMPLEMENTED' to be 'INVALID_QUERY', which is what proves the two refusals are distinguishable rather than interchangeable.

    (B) distinct flipped to false — the copied-neighbour mistake, and the one a review checking only that the two tables have the same keys would pass. Predicted: failures on VALUES, count_distinct(score) staying green (nothing to dedup). Measured: local 4 failed / 12 passed, remote 5 failed / 12 passed, on expected 4 to be 2 and west 3 vs 2 — the same two wrong numbers on both faces, count_distinct(score) green throughout, exactly as predicted.

    (B) is the direction the shared table exists for; (A) is reachable by any test that merely calls the function.

    Gates and tests

    • The gate list enumerated from .github/workflows/lint.yml, run step by step (not from memory): 58 / 58 PASS. Workspace built first so the check:i18n / check:i18n-coverage / check:app-nav-i18n trio ran for real; each step's exit status read directly, never through a pipe.
    • Two failed on the first pass and both were fixed here, not worked around: check:driver-conformance (CLASSIFIED — the new case-set was unregistered) and check:api-surface (five new exports; snapshot regenerated with gen:api-surface).
    • Tests: driver-sql 1095✓/48 skipped · driver-turso 902✓ · driver-sqlite-wasm 302✓ · driver-memory 540✓ · driver-mongodb 214✓/143 skipped · spec 8861✓ · objectql 2656✓ · service-analytics 1415✓ · metadata-protocol 740✓ · rest 1097✓.
    • Workspace-wide turbo run typecheck (120 tasks) and both enumerated turbo run build filters: green.

    One CI round was red, and it is worth recording why the local gate list did not catch it. TypeScript Type Check failed on 9b9b429 with src/sql-driver-aggregation-conformance.test.ts(121,63): error TS2353: 'sort' does not exist in type 'DriverQuery' — the fixture read-back used sort where QueryAST declares orderBy. My enumeration from lint.yml transcribed the check:* steps but omitted the workspace-wide turbo run typecheck / turbo run build steps in the same workflow, and the per-package typecheck I had run was run before that suite was written. Fixed in 08f803e, and the two missing commands were then run locally and are green. The lesson is the enumeration's, not the type error's: "every check:* step" is narrower than "every step".

    Out-of-scope findings (search-first dedup, filed unassigned)

    Open questions for the maintainer (三轴自裁, recorded not blocked)

    1. The kept-but-unreachable class-2 producer. Judged: keep, on the argument above. If the maintainer prefers ADR-0049's letter over the misreport risk, deleting uncompilableAggregateFunctionError on both faces is a small follow-up — the positive emptiness assertions are already in place to make the deletion honest.
    2. 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.
    3. service-analytics's AGGREGATE_SQL is a fifth COUNT(DISTINCT …) lowering (the Cube face). Left unenrolled on scope discipline — drivers: implement count_distinct in 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-mongodb are 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 — they narrate the state at that ruling and already say the SQL lowering "follows on its own card", so they are accurate as history. The one live spec string that did become false (AGG_RETIRED_MIDDLE's "the same five functions") was corrected, and deliberately made one character shorter: packages/rest's analytics-dataset-refusal-envelope.test.ts asserts that the actionable remedy survives the detail truncation window, and a longer edit pushed it out. Caught by that test, not by review.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions