Skip to content

Commit 92a67f2

Browse files
claude[bot]os-zhuangclaude
authored
feat(drivers,spec)!: GroupByNode.alias is honoured by the three SQL faces (#6401) (#6849)
* feat(drivers,spec)!: `GroupByNode.alias` is honoured by the three SQL faces (#6401) `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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USxSgh1HUGZuh8vuFXKTGQ * chore(changeset): answer the ADR-0087 disposition question in writing (#6401) 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USxSgh1HUGZuh8vuFXKTGQ --------- Co-authored-by: os-zhuang <steve@objectstack.ai> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 04c56aa commit 92a67f2

11 files changed

Lines changed: 516 additions & 49 deletions

‎.changeset/groupby-alias-multi-face.md‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
---
2+
"@objectstack/driver-sql": minor
3+
"@objectstack/driver-turso": minor
4+
"@objectstack/driver-sqlite-wasm": minor
5+
"@objectstack/spec": minor
6+
"@objectstack/objectql": patch
7+
---
8+
9+
feat(drivers,spec)!: `GroupByNode.alias` is honoured by the SQL faces — one aggregate, one column key (#6401)
10+
11+
`GroupByNodeSchema` has declared `alias` ("Alias for the projected group
12+
value", defaulting to `field`) for as long as the structured `groupBy` entry has
13+
existed. Exactly one execution path read it. The result: the SAME query came
14+
back with a different result-column key depending on which path the engine
15+
happened to take.
16+
17+
```ts
18+
groupBy: [{ field: 'closed_at', dateGranularity: 'month', alias: 'qtr' }]
19+
```
20+
21+
- pushed down to a driver ⇒ rows keyed **`closed_at`**
22+
- run through the in-memory fallback ⇒ rows keyed **`qtr`**
23+
24+
And the choice between them is `engine.ts`'s
25+
`allStructuredSupported && !tzRequiresInMemory` — a driver capability bit and a
26+
`timezone`, neither of which the caller can see. That is the multi-face
27+
consistency invariant broken in its quietest form: both answers are valid rows,
28+
so nothing throws and nothing looks wrong.
29+
30+
**Resolved to ENFORCE**, and the leg was chosen by measurement rather than
31+
taste. ADR-0049 splits on whether the feature already exists: a *dangling*
32+
promise is removed, a *live* one with a missing gate is enforced. `alias` is
33+
live — three consumers read it and change behaviour
34+
(`in-memory-aggregation.ts`, `MemoryDriver.performAggregation`, and
35+
`chartAggregateCategoryKey`), and the publish gate *compels* it:
36+
`validate-react-page-props.ts` errors `REACT_CHART_AXIS_UNKNOWN` unless a
37+
chart's category axis is bound to `alias ?? field`, telling the author in so
38+
many words to "bind it to" the alias. A key the build gate makes you write is
39+
not a dangling promise. The count of real non-test producers is **zero**, which
40+
is what makes enforcing safe rather than what argues against it: no shipped
41+
payload changes its result keys.
42+
43+
**What changed, on every SQL face at once** — a fix landing on one and not its
44+
twin is the #6203 shape, and `TursoDriver` picks its face from `url`:
45+
46+
- **`driver-sql`** — both limbs of the structured `groupBy` branch project
47+
`alias ?? field`: the date-bucket limb aliases the bucket expression to it,
48+
and the plain limb emits `?? as ??` (only when the name actually moves — an
49+
alias equal to the field emits no self-rename). `presentedOutput` is now keyed
50+
by the OUTPUT column, matching how the aggregation branch beside it has always
51+
worked; an aliased group value went unpresented before.
52+
- **`driver-turso` REMOTE** — the same projection, `"field" AS "alias"`. The
53+
alias reaches the statement as a quoted identifier and is therefore held to
54+
`assertSafeIdentifier`, exactly like `field`.
55+
- **`driver-sqlite-wasm`** — inherits `SqlDriver`'s compiler; covered by its own
56+
conformance suite rather than by assumption.
57+
58+
**GROUP BY still keys on the FIELD** on every face. Only the projection is
59+
renamed, so the buckets are unchanged. This is deliberate and pinned: SQLite
60+
resolves output names in `GROUP BY`, so a face that grouped by the alias would
61+
look correct here and diverge on a dialect that does not.
62+
63+
`having` needed no change and now means one thing: it is applied over the
64+
aggregated row's own columns, so a filter on a group projection references the
65+
alias on every path — previously the alias on one path and the field on the
66+
other.
67+
68+
**Conformance.** `AGGREGATION_CASES` (#6409) gains a `groupByAlias` axis and two
69+
cases. Their VALUES are an existing case verbatim — only the key moves — so they
70+
can fail only on the key, which is the point: every wrong answer in this area is
71+
a valid query returning plausible rows. `objectql`'s in-memory fallback is now
72+
**enrolled** as a fourth face, answering #6409's open question ②: it is the face
73+
the SQL three were converged onto, so the new behaviour would otherwise be
74+
pinned against nothing, and reaching it needs no engine at all —
75+
`applyInMemoryAggregation` is a pure function of rows and an AST.
76+
77+
**Reverse verification**, predicted before running. Reverting the in-memory face
78+
to `g.field`: only the two alias cases move and only ONE fails — the degenerate
79+
`alias === field` case stays green, which is why both are in the table.
80+
Reverting the harness to read `c.groupBy` instead of `c.groupByAlias ?? c.groupBy`
81+
— the copied-neighbour mistake: everything passes on an unmodified face, a false
82+
GREEN, which is the failure mode that would have made the axis vacuous.
83+
84+
**Frozen drivers (#5499), measured from source, not flipped.** `driver-memory`
85+
already returned `{ field, alias: node.alias ?? node.field }` and projects under
86+
the alias — it had independently reached the enforce answer, so it needed no
87+
alignment. `driver-mongodb` is a recorded DEBT row and the defect is wider than
88+
`alias`: `buildAggregationPipeline` types `groupBy` as `string[]` and builds
89+
`groupId[field] = '$' + field`, so a structured node — aliased or not — becomes
90+
the literal key `"[object Object]"`. It cannot take a structured `GroupByNode`
91+
at all; `mongodb-driver.ts` passes `(query as any).groupBy`, which is why `tsc`
92+
never saw it. Tracked on #6814.
93+
94+
**Compatibility.** A caller who writes `alias` and reads the result under
95+
`field` on a pushdown path will now find the value under `alias` — which is what
96+
the key has always meant on the fallback path, and what the chart gate already
97+
required. Callers who never write `alias` are unaffected: the emitted SQL is
98+
byte-identical.
99+
100+
<!-- adr-0087: not-required (no-migration-prescription) Nothing is retired: `GroupByNodeSchema.alias` keeps its declaration, its spelling and its type — it starts being HONOURED by three faces that parsed and ignored it. There is no tombstone to write and no authored metadata to rewrite, so there is no mechanical transform a migration could prescribe: every stack that validated before validates after, unchanged. The behaviour change is in the RESULT of a runtime query (a result-column key moves from `field` to `alias` on the pushdown path, converging on what the in-memory path and the chart publish gate already required), which the ledger has no channel for and no upgrader could apply a codemod to. The bang is on the changeset because callers who read that column by the field name must move, and the measured non-test producer count for the key is zero. -->
101+

‎packages/drivers/driver-sql/src/sql-driver-aggregation-conformance.test.ts‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,12 @@ const CONFORMANCE_OBJECT = {
8080
const astFor = (c: AggregationCase): QueryAST => ({
8181
object: CONFORMANCE_OBJECT.name,
8282
aggregations: [{ function: c.function, ...(c.field ? { field: c.field } : {}), alias: 'n' }],
83-
...(c.groupBy ? { groupBy: [c.groupBy] } : {}),
83+
// [#6401] A case carrying `groupByAlias` is sent as the STRUCTURED node, so
84+
// what the face receives is the union member that declares `alias`. Without
85+
// this the alias cases would send a bare string and pin nothing.
86+
...(c.groupBy
87+
? { groupBy: [c.groupByAlias ? { field: c.groupBy, alias: c.groupByAlias } : c.groupBy] }
88+
: {}),
8489
});
8590

8691
/**
@@ -89,10 +94,15 @@ const astFor = (c: AggregationCase): QueryAST => ({
8994
* numbers — SQLite hands `avg` back as a float and `count` as an integer, and
9095
* neither is the property under test.
9196
*/
92-
const actualFor = (c: AggregationCase, rows: Array<Record<string, unknown>>) =>
93-
rows
94-
.map((r) => ({ group: c.groupBy ? String(r[c.groupBy]) : null, value: Number(r.n) }))
97+
const actualFor = (c: AggregationCase, rows: Array<Record<string, unknown>>) => {
98+
// [#6401] The group value is read from the column the case SAYS it lands in —
99+
// `groupByAlias ?? groupBy`. Reading `c.groupBy` unconditionally is the bug
100+
// this axis exists to catch: it is green on a face that ignores the alias.
101+
const groupKey = c.groupByAlias ?? c.groupBy;
102+
return rows
103+
.map((r) => ({ group: groupKey ? String(r[groupKey]) : null, value: Number(r.n) }))
95104
.sort((x, y) => String(x.group).localeCompare(String(y.group)));
105+
};
96106

97107
describe('[#6409] SqlDriver — aggregate vocabulary conformance', () => {
98108
let driver: SqlDriver;

‎packages/drivers/driver-sql/src/sql-driver.ts‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4103,7 +4103,8 @@ export class SqlDriver implements IDataDriver {
41034103
// groupBy items may be plain strings ('region') or structured objects
41044104
// ({ field: 'closed_at', dateGranularity: 'quarter' }). For structured
41054105
// items we emit a dialect-specific bucket expression aliased as the
4106-
// field name so the resulting row keys match in-memory bucketDateValue.
4106+
// projected column name so the resulting row keys match in-memory
4107+
// bucketDateValue — see the `outKey` note below for what that name is.
41074108
// [#6212] The element type is `GroupByNode` — the spec's own union — so
41084109
// the local `Array<string | { field, dateGranularity? }>` restatement is
41094110
// gone. It had drifted from the declaration it was restating: `alias` was
@@ -4116,6 +4117,18 @@ export class SqlDriver implements IDataDriver {
41164117
const kind = this.readPresentationKind(table, g);
41174118
if (kind) presentedOutput.set(g, kind);
41184119
} else if (g && typeof g === 'object' && g.field) {
4120+
// [#6401] The projected column is named `alias ?? field` — the rule
4121+
// `AggregationNodeSchema.alias` already gets a few dozen lines below,
4122+
// and the one `in-memory-aggregation.ts` has always applied
4123+
// (`g.alias ?? g.field`). This face was the half that PARSED the key
4124+
// and ignored it, so one aggregate came back keyed by `closed_at`
4125+
// under pushdown and by `qtr` under the in-memory fallback — decided
4126+
// by a driver capability bit and a timezone the caller cannot see
4127+
// (`engine.ts`'s `allStructuredSupported && !tzRequiresInMemory`
4128+
// fork). GROUP BY still keys on the FIELD; only the projection is
4129+
// renamed, so the buckets are identical and only their column name
4130+
// moves.
4131+
const outKey = g.alias ?? g.field;
41194132
if (g.dateGranularity) {
41204133
const bucket = this.buildDateBucketExpr(g.field, g.dateGranularity, table);
41214134
if (!bucket) {
@@ -4129,12 +4142,18 @@ export class SqlDriver implements IDataDriver {
41294142
);
41304143
}
41314144
builder.groupByRaw(bucket.sql, bucket.bindings);
4132-
builder.select(this.knex.raw(`${bucket.sql} as ??`, [...bucket.bindings, g.field]));
4145+
builder.select(this.knex.raw(`${bucket.sql} as ??`, [...bucket.bindings, outKey]));
41334146
} else {
41344147
builder.groupBy(g.field);
4135-
builder.select(g.field);
4148+
// `?? as ??` only when the name actually moves: an alias equal to
4149+
// the field would otherwise rewrite `select "region"` into
4150+
// `select "region" as "region"` on every dialect for no gain.
4151+
builder.select(outKey === g.field ? g.field : this.knex.raw('?? as ??', [g.field, outKey]));
4152+
// Keyed by the OUTPUT column, like the aggregation branch below —
4153+
// `presentReadColumns` matches on the name the row actually
4154+
// carries, so an aliased group value went unpresented before.
41364155
const kind = this.readPresentationKind(table, g.field);
4137-
if (kind) presentedOutput.set(g.field, kind);
4156+
if (kind) presentedOutput.set(outKey, kind);
41384157
}
41394158
}
41404159
}

‎packages/drivers/driver-sqlite-wasm/src/sqlite-wasm-aggregation-conformance.test.ts‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,23 @@ const OBJECT = 'conformance_agg';
3434
const astFor = (c: AggregationCase): QueryAST => ({
3535
object: OBJECT,
3636
aggregations: [{ function: c.function, ...(c.field ? { field: c.field } : {}), alias: 'n' }],
37-
...(c.groupBy ? { groupBy: [c.groupBy] } : {}),
37+
// [#6401] A case carrying `groupByAlias` is sent as the STRUCTURED node, so
38+
// what the face receives is the union member that declares `alias`. Without
39+
// this the alias cases would send a bare string and pin nothing.
40+
...(c.groupBy
41+
? { groupBy: [c.groupByAlias ? { field: c.groupBy, alias: c.groupByAlias } : c.groupBy] }
42+
: {}),
3843
});
3944

40-
const actualFor = (c: AggregationCase, rows: Array<Record<string, unknown>>) =>
41-
rows
42-
.map((r) => ({ group: c.groupBy ? String(r[c.groupBy]) : null, value: Number(r.n) }))
45+
const actualFor = (c: AggregationCase, rows: Array<Record<string, unknown>>) => {
46+
// [#6401] The group value is read from the column the case SAYS it lands in —
47+
// `groupByAlias ?? groupBy`. Reading `c.groupBy` unconditionally is the bug
48+
// this axis exists to catch: it is green on a face that ignores the alias.
49+
const groupKey = c.groupByAlias ?? c.groupBy;
50+
return rows
51+
.map((r) => ({ group: groupKey ? String(r[groupKey]) : null, value: Number(r.n) }))
4352
.sort((x, y) => String(x.group).localeCompare(String(y.group)));
53+
};
4454

4555
describe('[#6409] driver-sqlite-wasm — aggregate vocabulary conformance', () => {
4656
let driver: SqliteWasmDriver;

‎packages/drivers/driver-turso/src/remote-transport-groupby-node.test.ts‎

Lines changed: 62 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -47,14 +47,21 @@
4747
* envelope would have been a new fork, so both faces were moved together and the
4848
* parity block below compares their runtime messages.
4949
*
50-
* # `alias` is deliberately NOT read
50+
* # `alias` IS read — as of #6401
5151
*
52-
* `GroupByNodeSchema.alias` is honoured by the in-memory path
53-
* (`in-memory-aggregation.ts` projects `g.alias ?? g.field`) and ignored by
54-
* `SqlDriver.aggregate`. Reading it here would make this transport the only SQL
55-
* face that honours it — a new divergence dressed as a fix. It is ignored, in
56-
* step with the local face, and the pushdown/in-memory disagreement is filed
57-
* separately; it is not created here.
52+
* This section used to read "`alias` is deliberately NOT read", and the reason
53+
* it gave was sound: `GroupByNodeSchema.alias` was honoured by the in-memory
54+
* path (`in-memory-aggregation.ts` projects `g.alias ?? g.field`) and ignored
55+
* by `SqlDriver.aggregate`, so reading it HERE alone would have made this
56+
* transport the only SQL face that did — a new divergence dressed as a fix. The
57+
* disagreement was filed separately instead (#6401).
58+
*
59+
* That issue resolved to ENFORCE, and moved all three SQL faces in one change:
60+
* `driver-sql`, this transport, and `driver-sqlite-wasm` (which inherits
61+
* `SqlDriver`'s compiler). The projected column is `alias ?? field` everywhere;
62+
* GROUP BY still keys on the FIELD. So the deferral is discharged rather than
63+
* reversed — the condition it named ("only one face would read it") is what
64+
* stopped being true.
5865
*
5966
* # Reverse verification — direction predicted BEFORE it was run, per case
6067
*
@@ -176,17 +183,60 @@ describe('[#6212] RemoteTransport compiles the GroupByNode union', () => {
176183
expect(calls[0].sql).toBe('SELECT "stage", count("stage") AS "n" FROM "deal" GROUP BY "stage"');
177184
});
178185

179-
it('ignores `alias`, in step with the local face', async () => {
180-
// Pinned as a DELIBERATE choice, not an oversight: `SqlDriver.aggregate`
181-
// does not read `alias` either, so honouring it only here would make this
182-
// transport the one SQL face that does. See the file header.
186+
it('[#6401] projects `alias` as the column name, and still GROUPS BY the field', async () => {
187+
// Flipped from 'ignores `alias`, in step with the local face'. That pin
188+
// was correct when written — at #6212 this transport was the only face
189+
// that could have started reading the key, and doing so alone would have
190+
// been a new divergence. #6401 moved all three SQL faces together, so the
191+
// step it was keeping is now a step toward the alias, not away from it.
192+
//
193+
// The old assertion is REPLACED, not dropped: it claimed the alias never
194+
// reaches the statement; the new truth is the exact statement that
195+
// carries it — projection renamed, grouping untouched.
183196
const { t, calls } = transportWithCapturingClient();
184197
await t.aggregate('deal', {
185198
groupBy: [{ field: 'stage', alias: 'bucket' }],
186199
aggregations: [{ function: 'count', field: 'stage', alias: 'n' }],
187200
});
201+
expect(calls[0].sql).toBe(
202+
'SELECT "stage" AS "bucket", count("stage") AS "n" FROM "deal" GROUP BY "stage"',
203+
);
204+
// ⛔ The grouping key is the FIELD. SQLite resolves output names in
205+
// GROUP BY, so a face that grouped by the alias would return identical
206+
// rows here and diverge on a dialect that does not.
207+
expect(calls[0].sql).not.toContain('GROUP BY "bucket"');
208+
});
209+
210+
it('[#6401] an alias equal to the field name emits no self-rename', async () => {
211+
const { t, calls } = transportWithCapturingClient();
212+
await t.aggregate('deal', {
213+
groupBy: [{ field: 'stage', alias: 'stage' }],
214+
aggregations: [{ function: 'count', field: 'stage', alias: 'n' }],
215+
});
216+
// Byte-identical to the string spelling above — the degenerate alias must
217+
// not start rewriting statements on every dialect for no gain.
188218
expect(calls[0].sql).toBe('SELECT "stage", count("stage") AS "n" FROM "deal" GROUP BY "stage"');
189-
expect(calls[0].sql).not.toContain('bucket');
219+
});
220+
221+
it('[#6401] refuses an unsafe identifier in `alias`, not only in `field`', async () => {
222+
// The alias is caller-supplied text that now reaches the statement as a
223+
// quoted identifier, so it needs the gate `field` already has. The
224+
// assertion names the OFFENDING TEXT, not just the sentence (#6144): a
225+
// `field` that is itself safe is what makes this case reach the alias
226+
// check at all.
227+
const { t, calls } = transportWithCapturingClient();
228+
const err = await t
229+
.aggregate('deal', {
230+
groupBy: [{ field: 'stage', alias: 'bucket"; DROP TABLE deal; --' }],
231+
aggregations: [{ function: 'count', alias: 'n' }],
232+
})
233+
.then(
234+
() => { throw new Error('expected the transport to refuse an unsafe alias'); },
235+
(e) => e as Error,
236+
);
237+
expect(err.message).toContain('unsafe identifier rejected');
238+
expect(err.message).toContain('bucket"; DROP TABLE deal; --');
239+
expect(calls).toEqual([]);
190240
});
191241

192242
it('still refuses an unsafe identifier inside a structured entry', async () => {

0 commit comments

Comments
 (0)