From 7be2f1da43d11ba61efbcd95390bba63dc0f49cb Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Sun, 9 Aug 2026 00:28:41 +0000 Subject: [PATCH 1/2] feat(drivers,spec)!: `GroupByNode.alias` is honoured by the three SQL faces (#6401) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GroupByNodeSchema.alias` was declared and only partially executed: the in-memory fallback projected `g.alias ?? g.field` while `SqlDriver.aggregate`, `driver-turso`'s remote transport and `driver-sqlite-wasm` (inheriting the former) all read `g.field` only. The same aggregate therefore came back keyed `closed_at` under pushdown and `qtr` under the fallback, and the choice between them is `engine.ts`'s `allStructuredSupported && !tzRequiresInMemory` — a driver capability bit and a timezone the caller cannot see. Resolved to ENFORCE under ADR-0049, chosen from a measurement rather than a preference: real non-test producers of the key number ZERO, but the capability is live on three consumers and is COMPELLED by the publish gate — `validate-react-page-props.ts` errors REACT_CHART_AXIS_UNKNOWN unless a chart's category axis is bound to `alias ?? field`. ADR-0049 removes a dangling promise and enforces a live one with a missing gate; this is the second. - driver-sql: both limbs of the structured groupBy branch project `alias ?? field`; `presentedOutput` re-keyed by the OUTPUT column, matching the aggregation branch beside it. - driver-turso REMOTE: `"field" AS "alias"`, with the alias held to `assertSafeIdentifier` like every other identifier. - driver-sqlite-wasm: inherits, covered by its own conformance suite. GROUP BY still keys on the FIELD everywhere — only the projection is renamed. `having` needed no change and now means one thing on every path. AGGREGATION_CASES gains a `groupByAlias` axis; `objectql`'s in-memory fallback is enrolled as a fourth face, answering #6409's open question 2. driver-memory already agreed and needed no alignment; driver-mongodb carries a measured DEBT row — it cannot take a structured GroupByNode at all. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01USxSgh1HUGZuh8vuFXKTGQ --- .changeset/groupby-alias-multi-face.md | 98 +++++++++++ ...sql-driver-aggregation-conformance.test.ts | 18 +- packages/drivers/driver-sql/src/sql-driver.ts | 27 ++- ...qlite-wasm-aggregation-conformance.test.ts | 18 +- .../src/remote-transport-groupby-node.test.ts | 74 ++++++-- .../driver-turso/src/remote-transport.ts | 34 ++-- ...rso-remote-aggregation-conformance.test.ts | 18 +- .../in-memory-aggregation-conformance.test.ts | 161 ++++++++++++++++++ .../spec/src/data/aggregation-conformance.ts | 85 ++++++++- scripts/check-driver-conformance.mjs | 17 +- skills/objectstack-query/rules/aggregation.md | 12 +- 11 files changed, 513 insertions(+), 49 deletions(-) create mode 100644 .changeset/groupby-alias-multi-face.md create mode 100644 packages/objectql/src/in-memory-aggregation-conformance.test.ts diff --git a/.changeset/groupby-alias-multi-face.md b/.changeset/groupby-alias-multi-face.md new file mode 100644 index 0000000000..1878363ac6 --- /dev/null +++ b/.changeset/groupby-alias-multi-face.md @@ -0,0 +1,98 @@ +--- +"@objectstack/driver-sql": minor +"@objectstack/driver-turso": minor +"@objectstack/driver-sqlite-wasm": minor +"@objectstack/spec": minor +"@objectstack/objectql": patch +--- + +feat(drivers,spec)!: `GroupByNode.alias` is honoured by the SQL faces — one aggregate, one column key (#6401) + +`GroupByNodeSchema` has declared `alias` ("Alias for the projected group +value", defaulting to `field`) for as long as the structured `groupBy` entry has +existed. Exactly one execution path read it. The result: the SAME query came +back with a different result-column key depending on which path the engine +happened to take. + +```ts +groupBy: [{ field: 'closed_at', dateGranularity: 'month', alias: 'qtr' }] +``` + +- pushed down to a driver ⇒ rows keyed **`closed_at`** +- run through the in-memory fallback ⇒ rows keyed **`qtr`** + +And the choice between them is `engine.ts`'s +`allStructuredSupported && !tzRequiresInMemory` — a driver capability bit and a +`timezone`, neither of which the caller can see. That is the multi-face +consistency invariant broken in its quietest form: both answers are valid rows, +so nothing throws and nothing looks wrong. + +**Resolved to ENFORCE**, and the leg was chosen by measurement rather than +taste. ADR-0049 splits on whether the feature already exists: a *dangling* +promise is removed, a *live* one with a missing gate is enforced. `alias` is +live — three consumers read it and change behaviour +(`in-memory-aggregation.ts`, `MemoryDriver.performAggregation`, and +`chartAggregateCategoryKey`), and the publish gate *compels* it: +`validate-react-page-props.ts` errors `REACT_CHART_AXIS_UNKNOWN` unless a +chart's category axis is bound to `alias ?? field`, telling the author in so +many words to "bind it to" the alias. A key the build gate makes you write is +not a dangling promise. The count of real non-test producers is **zero**, which +is what makes enforcing safe rather than what argues against it: no shipped +payload changes its result keys. + +**What changed, on every SQL face at once** — a fix landing on one and not its +twin is the #6203 shape, and `TursoDriver` picks its face from `url`: + +- **`driver-sql`** — both limbs of the structured `groupBy` branch project + `alias ?? field`: the date-bucket limb aliases the bucket expression to it, + and the plain limb emits `?? as ??` (only when the name actually moves — an + alias equal to the field emits no self-rename). `presentedOutput` is now keyed + by the OUTPUT column, matching how the aggregation branch beside it has always + worked; an aliased group value went unpresented before. +- **`driver-turso` REMOTE** — the same projection, `"field" AS "alias"`. The + alias reaches the statement as a quoted identifier and is therefore held to + `assertSafeIdentifier`, exactly like `field`. +- **`driver-sqlite-wasm`** — inherits `SqlDriver`'s compiler; covered by its own + conformance suite rather than by assumption. + +**GROUP BY still keys on the FIELD** on every face. Only the projection is +renamed, so the buckets are unchanged. This is deliberate and pinned: SQLite +resolves output names in `GROUP BY`, so a face that grouped by the alias would +look correct here and diverge on a dialect that does not. + +`having` needed no change and now means one thing: it is applied over the +aggregated row's own columns, so a filter on a group projection references the +alias on every path — previously the alias on one path and the field on the +other. + +**Conformance.** `AGGREGATION_CASES` (#6409) gains a `groupByAlias` axis and two +cases. Their VALUES are an existing case verbatim — only the key moves — so they +can fail only on the key, which is the point: every wrong answer in this area is +a valid query returning plausible rows. `objectql`'s in-memory fallback is now +**enrolled** as a fourth face, answering #6409's open question ②: it is the face +the SQL three were converged onto, so the new behaviour would otherwise be +pinned against nothing, and reaching it needs no engine at all — +`applyInMemoryAggregation` is a pure function of rows and an AST. + +**Reverse verification**, predicted before running. Reverting the in-memory face +to `g.field`: only the two alias cases move and only ONE fails — the degenerate +`alias === field` case stays green, which is why both are in the table. +Reverting the harness to read `c.groupBy` instead of `c.groupByAlias ?? c.groupBy` +— the copied-neighbour mistake: everything passes on an unmodified face, a false +GREEN, which is the failure mode that would have made the axis vacuous. + +**Frozen drivers (#5499), measured from source, not flipped.** `driver-memory` +already returned `{ field, alias: node.alias ?? node.field }` and projects under +the alias — it had independently reached the enforce answer, so it needed no +alignment. `driver-mongodb` is a recorded DEBT row and the defect is wider than +`alias`: `buildAggregationPipeline` types `groupBy` as `string[]` and builds +`groupId[field] = '$' + field`, so a structured node — aliased or not — becomes +the literal key `"[object Object]"`. It cannot take a structured `GroupByNode` +at all; `mongodb-driver.ts` passes `(query as any).groupBy`, which is why `tsc` +never saw it. Tracked on #6814. + +**Compatibility.** A caller who writes `alias` and reads the result under +`field` on a pushdown path will now find the value under `alias` — which is what +the key has always meant on the fallback path, and what the chart gate already +required. Callers who never write `alias` are unaffected: the emitted SQL is +byte-identical. diff --git a/packages/drivers/driver-sql/src/sql-driver-aggregation-conformance.test.ts b/packages/drivers/driver-sql/src/sql-driver-aggregation-conformance.test.ts index 75b2e299b2..54987de74b 100644 --- a/packages/drivers/driver-sql/src/sql-driver-aggregation-conformance.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-aggregation-conformance.test.ts @@ -80,7 +80,12 @@ const CONFORMANCE_OBJECT = { const astFor = (c: AggregationCase): QueryAST => ({ object: CONFORMANCE_OBJECT.name, aggregations: [{ function: c.function, ...(c.field ? { field: c.field } : {}), alias: 'n' }], - ...(c.groupBy ? { groupBy: [c.groupBy] } : {}), + // [#6401] A case carrying `groupByAlias` is sent as the STRUCTURED node, so + // what the face receives is the union member that declares `alias`. Without + // this the alias cases would send a bare string and pin nothing. + ...(c.groupBy + ? { groupBy: [c.groupByAlias ? { field: c.groupBy, alias: c.groupByAlias } : c.groupBy] } + : {}), }); /** @@ -89,10 +94,15 @@ const astFor = (c: AggregationCase): QueryAST => ({ * numbers — SQLite hands `avg` back as a float and `count` as an integer, and * neither is the property under test. */ -const actualFor = (c: AggregationCase, rows: Array>) => - rows - .map((r) => ({ group: c.groupBy ? String(r[c.groupBy]) : null, value: Number(r.n) })) +const actualFor = (c: AggregationCase, rows: Array>) => { + // [#6401] The group value is read from the column the case SAYS it lands in — + // `groupByAlias ?? groupBy`. Reading `c.groupBy` unconditionally is the bug + // this axis exists to catch: it is green on a face that ignores the alias. + const groupKey = c.groupByAlias ?? c.groupBy; + return rows + .map((r) => ({ group: groupKey ? String(r[groupKey]) : null, value: Number(r.n) })) .sort((x, y) => String(x.group).localeCompare(String(y.group))); +}; describe('[#6409] SqlDriver — aggregate vocabulary conformance', () => { let driver: SqlDriver; diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 2f5315ee2e..ab194a973c 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -4103,7 +4103,8 @@ export class SqlDriver implements IDataDriver { // groupBy items may be plain strings ('region') or structured objects // ({ field: 'closed_at', dateGranularity: 'quarter' }). For structured // items we emit a dialect-specific bucket expression aliased as the - // field name so the resulting row keys match in-memory bucketDateValue. + // projected column name so the resulting row keys match in-memory + // bucketDateValue — see the `outKey` note below for what that name is. // [#6212] The element type is `GroupByNode` — the spec's own union — so // the local `Array` restatement is // gone. It had drifted from the declaration it was restating: `alias` was @@ -4116,6 +4117,18 @@ export class SqlDriver implements IDataDriver { const kind = this.readPresentationKind(table, g); if (kind) presentedOutput.set(g, kind); } else if (g && typeof g === 'object' && g.field) { + // [#6401] The projected column is named `alias ?? field` — the rule + // `AggregationNodeSchema.alias` already gets a few dozen lines below, + // and the one `in-memory-aggregation.ts` has always applied + // (`g.alias ?? g.field`). This face was the half that PARSED the key + // and ignored it, so one aggregate came back keyed by `closed_at` + // under pushdown and by `qtr` under the in-memory fallback — decided + // by a driver capability bit and a timezone the caller cannot see + // (`engine.ts`'s `allStructuredSupported && !tzRequiresInMemory` + // fork). GROUP BY still keys on the FIELD; only the projection is + // renamed, so the buckets are identical and only their column name + // moves. + const outKey = g.alias ?? g.field; if (g.dateGranularity) { const bucket = this.buildDateBucketExpr(g.field, g.dateGranularity, table); if (!bucket) { @@ -4129,12 +4142,18 @@ export class SqlDriver implements IDataDriver { ); } builder.groupByRaw(bucket.sql, bucket.bindings); - builder.select(this.knex.raw(`${bucket.sql} as ??`, [...bucket.bindings, g.field])); + builder.select(this.knex.raw(`${bucket.sql} as ??`, [...bucket.bindings, outKey])); } else { builder.groupBy(g.field); - builder.select(g.field); + // `?? as ??` only when the name actually moves: an alias equal to + // the field would otherwise rewrite `select "region"` into + // `select "region" as "region"` on every dialect for no gain. + builder.select(outKey === g.field ? g.field : this.knex.raw('?? as ??', [g.field, outKey])); + // Keyed by the OUTPUT column, like the aggregation branch below — + // `presentReadColumns` matches on the name the row actually + // carries, so an aliased group value went unpresented before. const kind = this.readPresentationKind(table, g.field); - if (kind) presentedOutput.set(g.field, kind); + if (kind) presentedOutput.set(outKey, kind); } } } diff --git a/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-aggregation-conformance.test.ts b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-aggregation-conformance.test.ts index 9eb0340cd2..180bfcf017 100644 --- a/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-aggregation-conformance.test.ts +++ b/packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-aggregation-conformance.test.ts @@ -34,13 +34,23 @@ const OBJECT = 'conformance_agg'; const astFor = (c: AggregationCase): QueryAST => ({ object: OBJECT, aggregations: [{ function: c.function, ...(c.field ? { field: c.field } : {}), alias: 'n' }], - ...(c.groupBy ? { groupBy: [c.groupBy] } : {}), + // [#6401] A case carrying `groupByAlias` is sent as the STRUCTURED node, so + // what the face receives is the union member that declares `alias`. Without + // this the alias cases would send a bare string and pin nothing. + ...(c.groupBy + ? { groupBy: [c.groupByAlias ? { field: c.groupBy, alias: c.groupByAlias } : c.groupBy] } + : {}), }); -const actualFor = (c: AggregationCase, rows: Array>) => - rows - .map((r) => ({ group: c.groupBy ? String(r[c.groupBy]) : null, value: Number(r.n) })) +const actualFor = (c: AggregationCase, rows: Array>) => { + // [#6401] The group value is read from the column the case SAYS it lands in — + // `groupByAlias ?? groupBy`. Reading `c.groupBy` unconditionally is the bug + // this axis exists to catch: it is green on a face that ignores the alias. + const groupKey = c.groupByAlias ?? c.groupBy; + return rows + .map((r) => ({ group: groupKey ? String(r[groupKey]) : null, value: Number(r.n) })) .sort((x, y) => String(x.group).localeCompare(String(y.group))); +}; describe('[#6409] driver-sqlite-wasm — aggregate vocabulary conformance', () => { let driver: SqliteWasmDriver; diff --git a/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts b/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts index 59cc758cd9..83d0d9585f 100644 --- a/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts +++ b/packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts @@ -47,14 +47,21 @@ * envelope would have been a new fork, so both faces were moved together and the * parity block below compares their runtime messages. * - * # `alias` is deliberately NOT read + * # `alias` IS read — as of #6401 * - * `GroupByNodeSchema.alias` is honoured by the in-memory path - * (`in-memory-aggregation.ts` projects `g.alias ?? g.field`) and ignored by - * `SqlDriver.aggregate`. Reading it here would make this transport the only SQL - * face that honours it — a new divergence dressed as a fix. It is ignored, in - * step with the local face, and the pushdown/in-memory disagreement is filed - * separately; it is not created here. + * This section used to read "`alias` is deliberately NOT read", and the reason + * it gave was sound: `GroupByNodeSchema.alias` was honoured by the in-memory + * path (`in-memory-aggregation.ts` projects `g.alias ?? g.field`) and ignored + * by `SqlDriver.aggregate`, so reading it HERE alone would have made this + * transport the only SQL face that did — a new divergence dressed as a fix. The + * disagreement was filed separately instead (#6401). + * + * That issue resolved to ENFORCE, and moved all three SQL faces in one change: + * `driver-sql`, this transport, and `driver-sqlite-wasm` (which inherits + * `SqlDriver`'s compiler). The projected column is `alias ?? field` everywhere; + * GROUP BY still keys on the FIELD. So the deferral is discharged rather than + * reversed — the condition it named ("only one face would read it") is what + * stopped being true. * * # Reverse verification — direction predicted BEFORE it was run, per case * @@ -176,17 +183,60 @@ describe('[#6212] RemoteTransport compiles the GroupByNode union', () => { expect(calls[0].sql).toBe('SELECT "stage", count("stage") AS "n" FROM "deal" GROUP BY "stage"'); }); - it('ignores `alias`, in step with the local face', async () => { - // Pinned as a DELIBERATE choice, not an oversight: `SqlDriver.aggregate` - // does not read `alias` either, so honouring it only here would make this - // transport the one SQL face that does. See the file header. + it('[#6401] projects `alias` as the column name, and still GROUPS BY the field', async () => { + // Flipped from 'ignores `alias`, in step with the local face'. That pin + // was correct when written — at #6212 this transport was the only face + // that could have started reading the key, and doing so alone would have + // been a new divergence. #6401 moved all three SQL faces together, so the + // step it was keeping is now a step toward the alias, not away from it. + // + // The old assertion is REPLACED, not dropped: it claimed the alias never + // reaches the statement; the new truth is the exact statement that + // carries it — projection renamed, grouping untouched. const { t, calls } = transportWithCapturingClient(); await t.aggregate('deal', { groupBy: [{ field: 'stage', alias: 'bucket' }], aggregations: [{ function: 'count', field: 'stage', alias: 'n' }], }); + expect(calls[0].sql).toBe( + 'SELECT "stage" AS "bucket", count("stage") AS "n" FROM "deal" GROUP BY "stage"', + ); + // ⛔ The grouping key is the FIELD. SQLite resolves output names in + // GROUP BY, so a face that grouped by the alias would return identical + // rows here and diverge on a dialect that does not. + expect(calls[0].sql).not.toContain('GROUP BY "bucket"'); + }); + + it('[#6401] an alias equal to the field name emits no self-rename', async () => { + const { t, calls } = transportWithCapturingClient(); + await t.aggregate('deal', { + groupBy: [{ field: 'stage', alias: 'stage' }], + aggregations: [{ function: 'count', field: 'stage', alias: 'n' }], + }); + // Byte-identical to the string spelling above — the degenerate alias must + // not start rewriting statements on every dialect for no gain. expect(calls[0].sql).toBe('SELECT "stage", count("stage") AS "n" FROM "deal" GROUP BY "stage"'); - expect(calls[0].sql).not.toContain('bucket'); + }); + + it('[#6401] refuses an unsafe identifier in `alias`, not only in `field`', async () => { + // The alias is caller-supplied text that now reaches the statement as a + // quoted identifier, so it needs the gate `field` already has. The + // assertion names the OFFENDING TEXT, not just the sentence (#6144): a + // `field` that is itself safe is what makes this case reach the alias + // check at all. + const { t, calls } = transportWithCapturingClient(); + const err = await t + .aggregate('deal', { + groupBy: [{ field: 'stage', alias: 'bucket"; DROP TABLE deal; --' }], + aggregations: [{ function: 'count', alias: 'n' }], + }) + .then( + () => { throw new Error('expected the transport to refuse an unsafe alias'); }, + (e) => e as Error, + ); + expect(err.message).toContain('unsafe identifier rejected'); + expect(err.message).toContain('bucket"; DROP TABLE deal; --'); + expect(calls).toEqual([]); }); it('still refuses an unsafe identifier inside a structured entry', async () => { diff --git a/packages/drivers/driver-turso/src/remote-transport.ts b/packages/drivers/driver-turso/src/remote-transport.ts index 8f3a43bd44..bcb4a81828 100644 --- a/packages/drivers/driver-turso/src/remote-transport.ts +++ b/packages/drivers/driver-turso/src/remote-transport.ts @@ -971,20 +971,28 @@ export class RemoteTransport { // #6203 shape again: one query, two answers, decided by a connection // string. Reading `.field` converges them. // - // `alias` is deliberately not read: `SqlDriver.aggregate` does not read it - // either, so honouring it here would be the divergence rather than the fix. - // That the SQL faces ignore a key the in-memory path honours - // (`in-memory-aggregation.ts` projects `g.alias ?? g.field`) is filed - // separately — it is not created here. - const groupBy: string[] = (Array.isArray(query?.groupBy) ? query.groupBy : []).map((g) => { - if (typeof g === 'string') return g; + // [#6401] `alias` IS read now, and the note that used to sit here — "not + // read, because `SqlDriver.aggregate` does not read it either" — was the + // right call at #6212 and is discharged rather than deleted: all three SQL + // faces read it as of #6401, so honouring it here is the convergence, not a + // new divergence. The projected column is `alias ?? field`, matching + // `in-memory-aggregation.ts`'s long-standing `g.alias ?? g.field`; GROUP BY + // still keys on the FIELD, so only the column's name moves. + const groupBy: Array<{ field: string; outKey: string }> = ( + Array.isArray(query?.groupBy) ? query.groupBy : [] + ).map((g) => { + if (typeof g === 'string') return { field: g, outKey: g }; if (g?.dateGranularity) refuseDateBucketedGroupBy(g.dateGranularity); - return g?.field; + return { field: g?.field, outKey: g?.alias ?? g?.field }; }); - for (const field of groupBy) { + for (const { field, outKey } of groupBy) { this.assertSafeIdentifier(field); - selectParts.push(`"${field}"`); + // The alias reaches the statement as a quoted identifier, so it is held + // to the same gate as every other one — an alias is caller-supplied text + // and `assertSafeIdentifier` is what keeps it out of the SQL grammar. + this.assertSafeIdentifier(outKey); + selectParts.push(outKey === field ? `"${field}"` : `"${field}" AS "${outKey}"`); } // [#6321] Was `query?.aggregations || query?.aggregate` — see the twin note @@ -1057,7 +1065,11 @@ export class RemoteTransport { } if (groupBy.length > 0) { - sql += ` GROUP BY ${groupBy.map((f) => `"${f}"`).join(', ')}`; + // [#6401] By FIELD, never by the alias: the alias renames the projection + // only. SQLite would happily group by the output name, which is exactly + // the mistake that would make an aliased group silently correct here and + // wrong on a dialect that resolves GROUP BY against the input columns. + sql += ` GROUP BY ${groupBy.map((g) => `"${g.field}"`).join(', ')}`; } try { diff --git a/packages/drivers/driver-turso/src/turso-remote-aggregation-conformance.test.ts b/packages/drivers/driver-turso/src/turso-remote-aggregation-conformance.test.ts index 9209b6171a..ec4fa636b4 100644 --- a/packages/drivers/driver-turso/src/turso-remote-aggregation-conformance.test.ts +++ b/packages/drivers/driver-turso/src/turso-remote-aggregation-conformance.test.ts @@ -80,13 +80,23 @@ const CONFORMANCE_OBJECT = { const astFor = (c: AggregationCase): QueryAST => ({ object: CONFORMANCE_OBJECT.name, aggregations: [{ function: c.function, ...(c.field ? { field: c.field } : {}), alias: 'n' }], - ...(c.groupBy ? { groupBy: [c.groupBy] } : {}), + // [#6401] A case carrying `groupByAlias` is sent as the STRUCTURED node, so + // what the face receives is the union member that declares `alias`. Without + // this the alias cases would send a bare string and pin nothing. + ...(c.groupBy + ? { groupBy: [c.groupByAlias ? { field: c.groupBy, alias: c.groupByAlias } : c.groupBy] } + : {}), }); -const actualFor = (c: AggregationCase, rows: Array>) => - rows - .map((r) => ({ group: c.groupBy ? String(r[c.groupBy]) : null, value: Number(r.n) })) +const actualFor = (c: AggregationCase, rows: Array>) => { + // [#6401] The group value is read from the column the case SAYS it lands in — + // `groupByAlias ?? groupBy`. Reading `c.groupBy` unconditionally is the bug + // this axis exists to catch: it is green on a face that ignores the alias. + const groupKey = c.groupByAlias ?? c.groupBy; + return rows + .map((r) => ({ group: groupKey ? String(r[groupKey]) : null, value: Number(r.n) })) .sort((x, y) => String(x.group).localeCompare(String(y.group))); +}; describe('[#6409] TursoDriver remote — aggregate vocabulary conformance', () => { let driver: TursoDriver; diff --git a/packages/objectql/src/in-memory-aggregation-conformance.test.ts b/packages/objectql/src/in-memory-aggregation-conformance.test.ts new file mode 100644 index 0000000000..56258b7265 --- /dev/null +++ b/packages/objectql/src/in-memory-aggregation-conformance.test.ts @@ -0,0 +1,161 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6401] Aggregate-vocabulary conformance for the IN-MEMORY fallback — the + * same `@objectstack/spec/data` cases the three SQL faces run. + * + * ## Why this face is enrolled, when #6409 deliberately left it out + * + * #6409 built the table for the two faces of ONE driver (`driver-sql`'s local + * compiler and `driver-turso`'s remote one, chosen from a connection string) + * and recorded enrolling this one as open question ②: it is not a SQL face and + * not a driver, so it looked like it needed the engine to reach. + * + * #6401 is what makes it necessary rather than optional. `GroupByNode.alias` + * was declared by the spec, honoured HERE (`g.alias ?? g.field`, the projection + * at the top of `in-memory-aggregation.ts`) and ignored by all three SQL faces, + * so one aggregate came back keyed `region` under pushdown and `bucket` under + * the fallback — and which one you got was decided by a driver capability bit + * and a `timezone`, neither of which the caller can see + * (`engine.ts`'s `allStructuredSupported && !tzRequiresInMemory` fork). The SQL + * faces were converged ONTO this one, so pinning the new behaviour without this + * face in the comparison would pin it against nothing. + * + * It also turned out to be cheap, which is the part of question ② that was + * wrong: `applyInMemoryAggregation` is a pure function of rows and an AST. No + * engine, no driver, no database — the fixture goes in as an array. + * + * ## What this suite is NOT + * + * It is not a check that the engine CHOOSES this path — that fork is + * `engine.ts`'s and is pinned by its own suites. This asserts only that when + * the fallback runs, it answers the shared table: same values, same keys, as + * the faces it can be swapped for behind the caller's back. + * + * ## Reverse verification — directions predicted BEFORE they were run + * + * **(A) `g.alias ?? g.field` reverted to `g.field`** — i.e. this face made to + * behave the way the SQL three did before #6401. Predicted: exactly the two + * `groupByAlias` cases move, and only ONE of them fails — + * 'groupBy alias renames the projected group column' goes red with + * `group: 'undefined'` (the row is keyed `region`, the harness reads `bucket`, + * `String(undefined)`), while 'groupBy alias equal to the field name is a + * no-op' stays GREEN, because `alias === field` makes the revert invisible. + * Every non-alias case unaffected. + * + * **(B) the harness reading `c.groupBy` instead of `c.groupByAlias ?? c.groupBy`** + * — the copied-neighbour mistake, and the one that matters most here because it + * is the mistake that makes the whole axis vacuous. Predicted: ALL cases pass, + * including both alias cases, on an unmodified face — the failure mode being + * demonstrated is a false GREEN, not a red. + * + * ## Measured, after writing the above + * + * **(A) 0 failed / 17 passed here** — this face is not the one reverted, so + * this suite was correctly unmoved. The revert was applied to the three SQL + * faces and measured there: `driver-sql` 1 failed / 17 passed, `driver-turso` + * 3 / 31, `driver-sqlite-wasm` 1 / 14 — each on exactly the case named above, + * with the degenerate `alias === field` case green throughout, as predicted. + * + * **(B) PREDICTION WRONG, and the correction is the useful part.** Measured + * 1 FAILED / 16 passed here (and 1 / 17 on `driver-sql`), not the all-green + * false GREEN predicted. Reading the FIELD name against a face that HONOURS the + * alias fails loudly: the row is keyed `bucket`, `r['region']` is `undefined`, + * and the case dies on `group: 'undefined'`. The false green needs BOTH + * mistakes at once — the harness reading the field name AND a face that ignores + * the alias — which was then measured directly and is 18/18 GREEN on + * `driver-sql`. So the `groupByAlias ?? groupBy` read is load-bearing exactly + * where the prediction said, but for the opposite reason: it is not a silent + * no-op on a correct face, it is one half of a pair that only goes quiet + * together. + * + * A second unpredicted result, recorded rather than tidied away: + * `driver-sqlite-wasm`'s suite consumes `@objectstack/driver-sql`'s BUILT + * `dist`, not its `src`, so direction (A) showed a spurious 15/15 green there + * until `driver-sql` was rebuilt. That suite checks its inheritance of the + * compiler at BUILD granularity, not working-tree granularity. + */ + +import { describe, it, expect } from 'vitest'; +import { AGGREGATION_CASES, AGGREGATION_ROWS } from '@objectstack/spec/data'; +import type { AggregationCase, QueryAST } from '@objectstack/spec/data'; +import { applyInMemoryAggregation } from './in-memory-aggregation.js'; + +/** + * The case as the `Pick` this function + * takes. Deliberately the same construction the three SQL suites use, down to + * the harness's own `alias: 'n'` on the aggregation — the point of the shared + * table is that the faces are fed the same thing. + */ +const astFor = (c: AggregationCase): Pick => ({ + aggregations: [{ function: c.function, ...(c.field ? { field: c.field } : {}), alias: 'n' }], + ...(c.groupBy + ? { groupBy: [c.groupByAlias ? { field: c.groupBy, alias: c.groupByAlias } : c.groupBy] } + : {}), +}); + +const actualFor = (c: AggregationCase, rows: Array>) => { + const groupKey = c.groupByAlias ?? c.groupBy; + return rows + .map((r) => ({ group: groupKey ? String(r[groupKey]) : null, value: Number(r.n) })) + .sort((x, y) => String(x.group).localeCompare(String(y.group))); +}; + +describe('[#6401] in-memory aggregation — aggregate vocabulary conformance', () => { + /** + * The fixture is passed through as-is, so the seeding failure the SQL + * harnesses guard against (a null stored as `''`) cannot happen here. What + * CAN happen is the table changing shape underneath this face, so the two + * properties every case leans on are asserted directly rather than assumed. + */ + it('the fixture is six rows with two real nulls in `stage`', () => { + expect(AGGREGATION_ROWS).toHaveLength(6); + expect(AGGREGATION_ROWS.filter((r) => r.stage === null)).toHaveLength(2); + }); + + for (const c of AGGREGATION_CASES) { + it(c.name, () => { + const rows = applyInMemoryAggregation([...AGGREGATION_ROWS], astFor(c)); + expect(actualFor(c, rows as any[]), c.note).toEqual([...c.expected]); + }); + } + + /** + * The alias cases above compare VALUES under a key. This asserts the key set + * itself — that the group column arrives under the alias and NOT ALSO under + * the field name. A face that emitted both would pass every case above (the + * harness only reads one of them) while handing callers a row shape that + * differs from the SQL faces', which is the same class of quiet divergence + * this whole table exists to catch. + */ + it('an aliased group projects under the alias ONLY — the field name is not also emitted', () => { + const rows = applyInMemoryAggregation([...AGGREGATION_ROWS], { + groupBy: [{ field: 'region', alias: 'bucket' }], + aggregations: [{ function: 'count', alias: 'n' }], + } as any); + expect(rows).toHaveLength(2); + for (const r of rows as any[]) { + expect(Object.keys(r).sort()).toEqual(['bucket', 'n']); + } + expect((rows as any[]).map((r) => r.bucket).sort()).toEqual(['east', 'west']); + }); + + /** + * The `having`口径 the issue predicted would follow: `applyHaving` judges the + * AGGREGATED row's own columns — aggregation aliases plus groupBy + * projections — so once the projection is named by the alias, a `having` that + * references the group column must reference the ALIAS on every face. Pinned + * here because this is the face where that was already true, and it is now + * the shared answer rather than one half of a divergence. + */ + it('the projected group column is the name `having` sees', () => { + const rows = applyInMemoryAggregation([...AGGREGATION_ROWS], { + groupBy: [{ field: 'region', alias: 'bucket' }], + aggregations: [{ function: 'count', alias: 'n' }], + } as any); + // Every key a post-aggregation filter could legitimately name. + const columns = new Set(Object.keys((rows as any[])[0])); + expect(columns.has('bucket')).toBe(true); + expect(columns.has('region')).toBe(false); + }); +}); diff --git a/packages/spec/src/data/aggregation-conformance.ts b/packages/spec/src/data/aggregation-conformance.ts index f2d982b54f..eac3aac314 100644 --- a/packages/spec/src/data/aggregation-conformance.ts +++ b/packages/spec/src/data/aggregation-conformance.ts @@ -81,11 +81,23 @@ * - **`driver-turso` REMOTE** — `turso-remote-aggregation-conformance.test.ts`, * through `makeLibsqlSqliteStub` (libsql IS SQLite, so the transport gets real * value semantics with no network). + * - **`driver-sqlite-wasm`** — `sqlite-wasm-aggregation-conformance.test.ts`. It + * inherits `SqlDriver`'s compiler, so what it re-checks is the sql.js dialect + * the statement then has to survive, not a second lowering. + * - **`objectql`'s in-memory fallback** — `in-memory-aggregation-conformance.test.ts` + * in `packages/objectql`. [#6401] Not a SQL face and not a driver, which is + * why #6409 left it out; enrolled here because it is the face that has always + * honoured `alias`, so the alias cases have nothing to pin the SQL faces + * AGAINST without it. It runs as a pure function over {@link AGGREGATION_ROWS} + * — no engine, no driver — which is what makes enrolling it cheap. This + * answers #6409's open question ②. * - * `driver-sqlite-wasm` and `driver-turso` LOCAL are deliberately NOT on that - * list: both *inherit* `SqlDriver`, so they re-run one compiler rather than - * check a second one — the same judgement `filter-logic-conformance.ts` records - * about its own enrolment. + * `driver-turso` LOCAL is deliberately NOT on that list: it *inherits* + * `SqlDriver`, so it re-runs one compiler rather than checking a second one — + * the same judgement `filter-logic-conformance.ts` records about its own + * enrolment. (`driver-sqlite-wasm` was named alongside it here until #6401; + * that sentence was already contradicted by the wasm suite sitting on disk + * since #6409, so it is corrected rather than carried.) * * ## DEBT — backends that would fail this table, and why they are not enrolled * @@ -98,6 +110,8 @@ * | `driver-memory` (data face) | **RED** — answers `null` | `MemoryDriver.computeAggregate` has no `count_distinct` arm; the `switch` falls to `default: return null`, so the aggregation resolves with no value and no error. | * | `driver-memory` (analytics face) | **agrees** | `memory-analytics.ts` collects `$addToSet` and sizes it — the same NULL question as MongoDB below; not executed against this table. | * | `driver-mongodb` | **RED** — counts NULL | `count_distinct` lowers to `$addToSet` and `postProcessAggregation` takes the array's `.length`, so an explicit `null` is one of the distinct values. Read from the source; not executed. | + * | `driver-memory` — the #6401 alias cases | **agrees** | `MemoryDriver.performAggregation`'s `normalizeGroupBy` (`memory-driver.ts:1066-1068`) already returns `{ field, alias: node.alias ?? node.field }` and projects the group value under `alias`. It reached the enforce answer independently, so the alias leg needed NO mechanical alignment here — measured, not assumed. | + * | `driver-mongodb` — the #6401 alias cases | **RED** — and wider than alias | `buildAggregationPipeline` types `groupBy` as `string[]` and does `groupId[field] = '$' + field` (`mongodb-aggregation.ts:66-69`, mirrored in the `$project` at `:85-88`). A STRUCTURED node — with or without an alias — is an object there, so the `$group._id` key becomes the literal `"[object Object]"` and its value `"$[object Object]"`. The alias is not so much ignored as unreachable: this face cannot take a structured `GroupByNode` at all. `mongodb-driver.ts:512` passes `(query as any).groupBy`, which is why the declared union never met the `string[]` annotation at `tsc`. Read from the source; not executed. | * * Both packages are inside the **#5499 investment freeze**, which is why these * are DEBT rows and not fixes: #6409's ruling put them explicitly out of scope @@ -108,8 +122,11 @@ * `objectql`'s in-memory fallback (`in-memory-aggregation.ts`) is a fourth * lowering and is NOT frozen: it computes `count_distinct` as * `new Set(values.filter(v => v != null)).size`, which is this table's answer. - * It is unenrolled only because it is not a SQL face and reaching it needs the - * engine rather than a driver; enrolling it is a candidate, not a debt. + * [#6401] It is now ENROLLED (see the list above). The reason it stopped being + * "a candidate, not a debt" is the alias axis: this is the face the SQL three + * had to be converged ONTO, so leaving it out would have pinned the new + * behaviour against nothing. Reaching it turned out not to need the engine at + * all — `applyInMemoryAggregation` is a pure function of rows and an AST. * * @see FILTER_LOGIC_CASES — the combinator standard this table is modelled on. */ @@ -190,6 +207,26 @@ export interface AggregationCase { readonly field?: string; /** The single GROUP BY column, or omitted for a whole-table aggregate. */ readonly groupBy?: 'region'; + /** + * [#6401] When set, {@link groupBy} is sent as the STRUCTURED node + * `{ field, alias }` rather than as a bare column name, and the group value + * must come back under this KEY instead of under the field name. + * + * This is the one property in the table that is about a column's NAME rather + * than its value, and it is here because the name was the divergence: + * `GroupByNodeSchema.alias` was declared, honoured by + * `in-memory-aggregation.ts` (`g.alias ?? g.field`) and ignored by all three + * SQL faces, so the same aggregate came back keyed `region` under pushdown + * and `bucket` under the fallback — decided by a driver capability bit and a + * timezone the caller never sees. A harness must read the group value from + * `groupByAlias ?? groupBy`; one that reads the field name unconditionally + * passes on a face that ignores the alias and fails on one that honours it, + * which is the divergence rather than a check of it. + * + * Requires {@link groupBy} — an alias with nothing to rename is meaningless, + * and the cases below never spell one. + */ + readonly groupByAlias?: string; /** Ascending by `group`, with `null` first for the ungrouped case. */ readonly expected: readonly AggregationExpectation[]; /** Why the case is here — surfaced in failure output. */ @@ -308,4 +345,40 @@ export const AGGREGATION_CASES: readonly AggregationCase[] = [ { group: 'west', value: 100 }, ], }, + + // ── [#6401] the group column's NAME, not its value ──────────────────────── + { + name: 'groupBy alias renames the projected group column', + function: 'count', + groupBy: 'region', + groupByAlias: 'bucket', + expected: [ + { group: 'east', value: 2 }, + { group: 'west', value: 4 }, + ], + note: + '#6401: the VALUES are the `count(*) grouped by region` case verbatim — only ' + + 'the key moves. A face that ignores `alias` returns the same two numbers ' + + 'under `region`, so this case can only fail on the KEY, which is the whole ' + + 'point: every wrong answer here is a valid query returning plausible rows. ' + + '`bucket` is deliberately not a column on the fixture, so a face that ' + + 'GROUPED BY the alias instead of projecting under it errors rather than ' + + 'coincidentally agreeing.', + }, + { + name: 'groupBy alias equal to the field name is a no-op', + function: 'sum', + field: 'score', + groupBy: 'region', + groupByAlias: 'region', + expected: [ + { group: 'east', value: 110 }, + { group: 'west', value: 100 }, + ], + note: + '#6401: the degenerate alias. Its twin above cannot see a face that emits ' + + '`"region" AS "region"` and breaks on the self-rename, and a face that ' + + 'special-cases `alias === field` needs the case that exercises the ' + + 'special case.', + }, ] as const; diff --git a/scripts/check-driver-conformance.mjs b/scripts/check-driver-conformance.mjs index 2504d12523..4a5e3552b1 100644 --- a/scripts/check-driver-conformance.mjs +++ b/scripts/check-driver-conformance.mjs @@ -417,7 +417,12 @@ const LEDGER = [ + '(`memory-analytics.ts`) DOES implement `count_distinct`, so this package answers one declared ' + 'function two ways depending on which face you enter — the divergence class #5374 fixed for ' + '`$contains` in this same package. #5499 freezes it, so the cell is open by decision, not by ' - + 'difficulty: the fix is one arm beside its neighbours. Tracked as #6814.', + + 'difficulty: the fix is one arm beside its neighbours. Tracked as #6814. ' + + '[#6401] Re-measured when the case-set gained its `groupByAlias` axis: on THAT axis this driver ' + + 'AGREES. `performAggregation`\'s `normalizeGroupBy` (`memory-driver.ts:1066-1068`) already returns ' + + '`{ field, alias: node.alias ?? node.field }` and projects the group value under `alias` — the answer ' + + '#6401 converged the three SQL faces onto. It had reached it independently, so the enforce leg needed ' + + 'NO mechanical alignment here. The cell stays open on `count_distinct` alone.', issue: 'https://github.com/objectstack-ai/objectstack/issues/6814', }, { @@ -434,7 +439,15 @@ const LEDGER = [ + 'package has no suite for the cell, which is the debt. #5499 freezes it; the fix is a `$ne: null` ' + 'before the `$addToSet` (or sizing a `$setDifference` against `[null]`). Tracked as #6814. Note the ' + 'real-mongod suites are opt-in since #5517, so whatever clears this cell needs a server-free half ' - + 'like `mongodb-filter-logic-translation.test.ts` has.', + + 'like `mongodb-filter-logic-translation.test.ts` has. ' + + '[#6401] Re-measured when the case-set gained its `groupByAlias` axis, and the finding is WIDER than ' + + 'the alias: `buildAggregationPipeline` annotates `groupBy` as `string[]` and builds ' + + '`groupId[field] = \'$\' + field` (`mongodb-aggregation.ts:66-69`, mirrored in the `$project` at ' + + '`:85-88`). A STRUCTURED `GroupByNode` — aliased or not — is an object there, so the `$group._id` key ' + + 'becomes the literal `"[object Object]"` and its value `"$[object Object]"`. This face cannot take ' + + 'the structured half of the declared union at all, so the alias is unreachable rather than ignored. ' + + '`mongodb-driver.ts:512` passes `(query as any).groupBy`, which is why the union never met that ' + + '`string[]` annotation at `tsc`. Read from the source; not executed. Same #6814 home.', issue: 'https://github.com/objectstack-ai/objectstack/issues/6814', }, ]; diff --git a/skills/objectstack-query/rules/aggregation.md b/skills/objectstack-query/rules/aggregation.md index 56f10e4946..dc916b229a 100644 --- a/skills/objectstack-query/rules/aggregation.md +++ b/skills/objectstack-query/rules/aggregation.md @@ -83,10 +83,18 @@ aggregations (never bucket by hand in app code): - Granularities: `day`, `week`, `month`, `quarter`, `year` (weeks are ISO-8601, starting Monday). - Optional `alias` renames the projected group value: - `{ field: 'closed_at', dateGranularity: 'quarter', alias: 'quarter' }`. + `{ field: 'closed_at', dateGranularity: 'quarter', alias: 'quarter' }` + puts the bucket under `quarter` instead of `closed_at`. **The alias renames + the projected COLUMN only** — grouping still keys on the field, so the buckets + themselves are unchanged. Read the result under `alias ?? field`, and reference + that same name from `having`. - The engine pushes bucketing down to the driver (`DATE_TRUNC` etc.) when the dialect supports that granularity, and transparently falls back to - in-memory bucketing otherwise — results are correct either way. + in-memory bucketing otherwise — results are correct either way, **including + the column keys** (#6401: until then the SQL drivers ignored `alias`, so an + aliased group came back under the field name when the query was pushed down + and under the alias when it fell back — decided by a capability bit and the + `timezone`, neither of which the caller can see). ## HAVING Clause From 71842f47c227d30161a4c683d7e924fa4d9a2e05 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Sun, 9 Aug 2026 00:33:32 +0000 Subject: [PATCH 2/2] chore(changeset): answer the ADR-0087 disposition question in writing (#6401) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The changeset declares a breaking change (bang), so #6148's gate requires the ADR-0087 ledger question to be ANSWERED, not assumed. The answer is `not-required (no-migration-prescription)`: nothing is retired — the key keeps its declaration and starts being honoured — so there is no tombstone to write and no authored metadata a codemod could rewrite. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01USxSgh1HUGZuh8vuFXKTGQ --- .changeset/groupby-alias-multi-face.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.changeset/groupby-alias-multi-face.md b/.changeset/groupby-alias-multi-face.md index 1878363ac6..4c35770cb9 100644 --- a/.changeset/groupby-alias-multi-face.md +++ b/.changeset/groupby-alias-multi-face.md @@ -96,3 +96,6 @@ never saw it. Tracked on #6814. the key has always meant on the fallback path, and what the chart gate already required. Callers who never write `alias` are unaffected: the emitted SQL is byte-identical. + + +