refactor(spec,objectql)!: retire AggregationNode.distinct — one face honoured it, five ignored it, and the same query answered two plausible numbers (#6815, ADR-0049) - #7051
Merged
Conversation
…e honoured it, five ignored it (#6815, ADR-0049) `AggregationNode.distinct` was read by exactly one of the six faces that consume an `aggregations[]` entry: objectql's in-memory fallback deduplicated before applying the function, while driver-sql, driver-turso, driver-mongodb, driver-memory and service-analytics' AGGREGATE_SQL all ignored it. So `{ function: 'sum', field: 'amount', distinct: true }` answered a deduplicated sum on the fallback path and an ordinary sum on every SQL datasource — one query, two plausible numbers, chosen by which backend served it. Removed per the maintainer ruling of 2026-08-09: tombstoned with retiredKey() (the schema is non-strict, so a bare deletion would silently strip what callers still send), registered as RETIRED_KEYS_BY_MAJOR[17] 'data/AggregationNode:distinct' plus the D3 semantic migration 'aggregation-node-distinct-retired'. No D2 conversion — QueryAST is a request surface with no stored source. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PiRUoQkTSBBmpyXBY3cVn2
…regation-distinct-retired
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 109 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 7 release-owned page(s) also reference the affected code. These are read-only:
|
github-actions
Bot
added
documentation
os-zhuang
marked this pull request as ready for review
August 9, 2026 12:22
This was referenced Aug 9, 2026
This was referenced Aug 9, 2026
This was referenced Aug 9, 2026
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 #6815
AggregationNode.distinctis retired under ADR-0049. It was read by exactly one of the six faces that consume anaggregations[]entry:objectql's in-memory fallback deduplicated the values before applying the function, while every SQL face ignored it. So{ function: 'sum', field: 'amount', distinct: true }answered a deduplicated sum when the engine fell back in memory and an ordinary sum on every SQL datasource — one query, two numbers, chosen by which backend happened to serve it. Both answers are plausible, so nothing surfaced the split.Maintainer ruling (2026-08-09), quoted verbatim
Premise verification (before implementing)
Re-verified on
origin/mainin the worktree, and it holds:packages/spec/src/data/query.zod.ts:234—distinct: z.boolean().optional().describe('Apply DISTINCT before aggregation')packages/objectql/src/in-memory-aggregation.ts:173—const values = collectValues(rows, field, !!agg.distinct);— the sole consumerAggregationNodeSchemaisz.object(), not.strict()→ tombstone route, per the playbook and the refactor(spec,client)!: retire the notification-listcursoron both halves and drop the fictionallimitdefault (#6361) #6866 precedent.One refinement the issue body did not have, measured rather than assumed: the blast radius inside the fallback was
sumandavgonly.countreturns from its own branch before ever callingcollectValues;count_distinctfeeds the values into aSet, and dedupe-then-SetisSet; dedupe does not movemin/max. Four of the six functions were already uniform. That narrows the impact statement without changing the verdict — the divergence is still real, still silent, and still decided by the backend.The six-face table — a conclusion for every face, with grep evidence
This is the semantic surface of the change, so each face in the issue's table gets an explicit verdict. Expected end state for a removal: uniform by construction — no face reads the key. Confirmed.
objectqlin-memory fallbackin-memory-aggregation.tsnow callscollectValues(rows, field); the function'sdistinctparameter and its dedupe branch are gonedriver-sqlSqlDriver.aggregatedistincthit insql-driver.tsis either theSQL_AGGREGATE_FUNCTIONSlowering table ('count_distinct'maps to{ sql: 'count', distinct: true }, keyed on the FUNCTION NAME) orbuilder.distinct(field), the driver's owndistinct()door. No read of an aggregation entry's keydriver-tursoRemoteTransport.aggregateremote-transport.ts:541,562-567,1030,1052);grep -c 'agg\.distinct|node\.distinct|entry\.distinct'returns 0driver-mongodbbuildAggregationStagebuildAccumulator(agg)(mongodb-aggregation.ts:120-154) switches onagg.functionand readsagg.field; nothing else.AggregationInputinterface (same file, line 17) still declaresdistinct?: boolean— a hand-mirrored copy of the spec shape that nothing reads. Not in scope + #5499: this driver is inside the investment freeze, and the ruling scoped the cost to the fallback flag "and nothing else". Filed as an observation-class finding instead (below)driver-memorycomputeAggregatememory-driver.ts:1123opens withconst { function: func, field } = agg;—distinctis structurally unreachable. Frozen under #5499; zero change was already compliance, so the freeze is not even engagedservice-analyticsAGGREGATE_SQLnative-sql-strategy.ts:34declaresAGGREGATE_SQLas aRecordof column-to-string functions — the lowering receives a column and nothing else, so there is no parameter a per-aggregation flag could ever arrive through. Structural, not incidentalWhat changed
distinct: retiredKey(AGGREGATION_DISTINCT_REMOVED)onAggregationNodeSchema. The prescription names the one honouring face, the five ignoring ones, and the live replacement. The constant is module-private (unlikeQUERY_CURSOR_REMOVED/QUERY_DISTINCT_REMOVED, which are exported because two schemas re-declare those keys) — one tombstone covers every aggregation door, becauseQuerySchema.aggregationsandEngineAggregateOptionsSchema.aggregationsboth reuse this one schema by reference rather than.extend()ing it.objectql—collectValues(rows, field)loses itsdistinctparameter and its dedupe branch, plus the module-header capability line. The whole runtime cost of the removal.RETIRED_KEYS_BY_MAJOR[17]gains'data/AggregationNode:distinct', plus a D3SemanticMigrationaggregation-node-distinct-retiredand a step-17rationaleparagraph. No D2 conversion, per the ruling:QueryASTis a request surface with no stored source, the same dispositionquery-joins-retired/query-cursor-retired/query-distinct-retired/query-window-functions-retiredalready take.query.jsonaggregations.children.distinctflipslivetodeadwith a note recording why the 2026-07-31 verdict was true evidence and still the wrong verdict, the ledger_noterecords the re-open, and the README row + counts regenerate to15 / 1 / 5 / 0.protocol/objectql/query-syntax.mdxandkernel/contracts/data-engine.mdx(hand-written);content/docs/references/**,spec-changes.json,docs/protocol-upgrade-guide.md,authorable-surface/data.jsonregenerated.authorable-surface/data.jsongainsdata/AggregationNode:distinct [RETIRED]— the tombstone route's signature, versus the line vanishing on a strict removal.majorfor@objectstack/specand@objectstack/objectql, carrying the FROM/TO mapping and theadr-0087: registeredmarker.Producer sweep (the #6866 precedent's other half)
Swept
packages/clientfor a producer to delete alongside the key, asQueryBuilder.cursor()/.distinct()were deleted with theirs in #4286. There is none.QueryBuilder(packages/client/src/query-builder.ts) has no aggregation builder at all —grep -n 'aggregat\|AggregationNode'on that file returns nothing. The SDK's only contact with the shape is the pass-through typeaggregations?: AggregationNode[](client/src/index.ts:158,188), which inherits the tombstone from the spec type, so authoringdistinctthrough the SDK is atscerror with no producer edit needed. That is the desired end state the dispatch asked for, reached by construction.Reverse verification — direction predicted BEFORE running
Predicted: RED, on all three new pin groups. Restoring the key to the schema and the dedupe limb to
collectValuesshould make the rejection pins parse clean and make the fallback answer the deduplicated number again. Method:git checkout origin/main -- ...on the three source files (nevergit stash— shared stack), rebuild spec, re-run, restore.specquery.test.ts6 failed | 85 passedobjectqlin-memory-aggregation.test.tsAssertionError: expected 500 to be 600, i.e. the dedupe limb collapsed the100/100pair; thecount_distinct-unaffected pin stayed green, correctlyrestrequest-schema-gate.conformance.test.tsPOST /api/v1/data/:object/query accepted {"aggregations":[{...,"distinct":true}]} (status 200 ...): expected 200 to be 400gen:schemagate (b2)1 RETIRED_KEYS_BY_MAJOR entr(ies) name a key that is still LIVE: data/AggregationNode:distinct. The registration gate refuses a retirement nobody performed, which is a second, independent proof that the tombstone is what the baseline is readingAll restored afterwards; the tree is back to the retirement and every gate is green on it.
Rejection-class assertions (ADR-0112, #6142)
The tombstone creates a new refusal face at the REST boundary, so the pin asserts the envelope, not the throw. The case rides the existing
#3899conformance loop, which assertsstatus === 400andbody.code === 'VALIDATION_FAILED'and a non-emptyfields[]and that the protocol was never invoked. Thedistinctbody is the only retired key on that route that is not top-level — it sits inside anaggregations[]entry, so the case also proves entry validation descends into the array. Spec-level pins assert the prescription's own text (@objectstack/spec 17,count_distinct, and each of the four ignoring driver names) plus thepathof the reported issue (aggregations.0.distinct) on a mixed list where the siblingsumentry is legal.Verification
Foreground, serialized on the shared lock.
pnpm --filter @objectstack/spec testgivesTest Files 349 passed (349) | Tests 9094 passed (9094)pnpm --filter @objectstack/objectql testgivesTest Files 160 passed (160) | Tests 2766 passed (2766)pnpm --filter @objectstack/rest testgivesTest Files 73 passed (73) | Tests 1142 passed (1142)turbo run build --filter='./packages/*' --filter='./packages/*/*'gives70 successful, 70 totalturbo run typecheck(same filters) gives120 successful, 120 total.github/workflows/lint.ymland run one by one — ESLint plus all ~40check:*gates, every spec gate (check:liveness,check:empty-state,check:authorable-surface,check:api-surface,check:spec-changes,check:upgrade-guide,check:docs,check:generated,check:skill-*,check:variant-docs,check:exported-any,check:dual-source-exports), andcheck:i18n/check:i18n-coverage/check:app-nav-i18nafter a full workspace build (they refuse to run on an unbuilt tree and say so). All PASS.node scripts/check-adr-0087-registration.mjs --base origin/maingives1 declared-breaking changeset(s), each carrying an ADR-0087 disposition. [major+bang] registered aggregation-node-distinct-retired (new here)node scripts/check-nul-bytes.mjsOK, plus a targeted control-byte self-scan over every file this PR touches (clean).Merge lap over
main: mergedorigin/main(27 commits, including sibling #6998 / #4697), reset the generated tree toorigin/main, committed the merge — theos-regenpre-commit hook refused the commit until the artifacts were regenerated from the merged tree, which is the mechanism working — then full regen. Sibling entries asserted by recounting from the file, not from history: ADR-0087 registry ids50to51withlost from main: [], andRETIRED_KEYS_BY_MAJOR[17]11to12with all eleven of main's keys present. Union-keep confirmed.Out of scope, deliberately
sum(*)with nofielddying as a raw dialect error with no ADR-0112 envelope — the ruling names this as a separate gap and explicitly forbids smuggling it in. Untouched.AggregationNode.filter— still[EXPERIMENTAL — not enforced]per [P2] data:QueryASTdeclares 12 members no executor runs — the liveness ledger governs metadata types, not the request surface #4286. Untouched.driver-mongodb's localAggregationInput.distinct?: boolean— a dead declaration in a package frozen by [裁决] driver-memory / driver-mongodb 投入冻结 —— 维护者 2026-08-05 口径（跨单锚点） #5499. Filed as an observation-class finding rather than fixed here.Generated by Claude Code