diff --git a/.changeset/group-key-read-shape.md b/.changeset/group-key-read-shape.md new file mode 100644 index 0000000000..211da037f4 --- /dev/null +++ b/.changeset/group-key-read-shape.md @@ -0,0 +1,92 @@ +--- +"@objectstack/objectql": minor +"@objectstack/driver-sql": minor +--- + +fix(objectql,driver-sql)!: a group key is the column's value, in the shape `find()` presents it (#3849) + +`groupBy: ['qty']` now returns `3`, not `'3'`. `groupBy: ['won']` returns `true` / +`false`, not `'true'` / `'false'` on one path and `1` / `0` on the other. A bucket +key is a column value, so there is one right answer for what it looks like — +whatever that column looks like on a `find()` row — and all three paths that +produce one now give it. + +### What was wrong + +Three code paths produce a group key, and no two of them agreed: + +| | `qty` (number) | `won` (boolean) | +|---|---|---| +| `find()` | `3` number | `true` boolean | +| `aggregate()` pushed down | `3` number | `0` / `1` **number** | +| in-memory fallback | `'3'` **string** | `'false'` / `'true'` **string** | + +Two independent causes: + +- `applyInMemoryAggregation` ran every key through `String()`. The pushed-down + path never did. +- The pushed-down path returns raw builder output. #3797 taught it to present + temporal columns the way `formatOutput` does on a `find()` row, but not the + boolean and numeric repairs — so a SQLite boolean, which has no native type and + is stored as `0`/`1`, surfaced as an integer from `aggregate()` and as a real + boolean from `find()`. + +`engine.aggregate` chooses between the two aggregate paths per query — by whether +the driver aggregates natively, whether it advertises the requested granularity, +and whether the reference timezone is UTC — so the same column changed shape with +no change to the data or the query. + +### Why it mattered + +The measures were always right, which is why this went unnoticed. What broke was +downstream code that probes a raw `Map` keyed by the value's own type. `Map` +lookup is SameValueZero, so `'1'` never finds `1`: + +- **Select-option labels** (`dimension-labels.ts`) — the label table is keyed by + the option's own `value`. A numeric option value never matched a stringified + key, so the chart rendered the raw stored value instead of its label. +- **Lookup / master-detail labels** — the id → record-name table is built by an + inner query that always pushes down (raw ids), then probed with the outer + query's keys, which may be in-memory (stringified). With a numeric primary key + — routine for external/federated objects — every label missed. +- **Cross-object rebucketing** (`cross-object-rebucket.ts`) — the FK → attribute + map is built and probed the same way, and a miss is not a fallback but + `RESTRICTED_BUCKET`. A numeric FK filed **every row** under `'(restricted)'`: + one bar, correct grand total, no error. +- **Drill-through** — the raw dimension value goes into the drill filter + verbatim, so a boolean dimension drilled from the in-memory path sent + `{ won: 'true' }` to SQLite, whose INTEGER column cannot equal the text + `'true'`. Zero rows. + +### What changed + +- `applyInMemoryAggregation` (`@objectstack/objectql`) emits the value verbatim. + Its rows come straight from `driver.find()`, so passing the value through is + what makes the key equal the column's own read shape. +- The internal composite bucket id is now type-preserving, so `1` and `'1'`, + `true` and `'true'` stay distinct groups rather than merging on the way in. + BigInt is encoded explicitly — `JSON.stringify` throws on it, and a value that + used to bucket under `String()` must not start crashing the aggregate. +- `SqlDriver.aggregate` / `.distinct` (`@objectstack/driver-sql`) present group + keys and `min`/`max` results with the same rules `formatOutput` applies on a + `find()` row, generalizing the #3797 temporal fix to boolean and numeric + columns. The `protected` helpers behind it are renamed accordingly + (`temporalFieldKind` → `readPresentationKind`, `presentTemporalValue` → + `presentReadValue`, `presentTemporalColumns` → `presentReadColumns`) and the + kind union is exported as `ReadPresentationKind`. + +Date-bucketed `groupBy` items are unaffected: `bucketDateValue` and the dialect +bucket expressions both produce canonical string labels, and #3839 already pinned +their empty bucket. + +### Gate + +`packages/qa/dogfood/test/group-key-read-shape-parity.test.ts` measures both +aggregate paths against `find()` for a number, boolean and text column, on +`driver-sql` and `driver-sqlite-wasm`. It asserts the runtime TYPE, not just the +value — folding both sides through `String()` is the reflex that hid this in the +first place and would make the check pass against the bug it exists to catch. + +Each half was confirmed to fail the gate on its own: reverting only the +in-memory change reddens the number and boolean cases, reverting only the driver +change reddens the boolean cases with `0` against `false`. diff --git a/packages/objectql/src/in-memory-aggregation.test.ts b/packages/objectql/src/in-memory-aggregation.test.ts index 992263bbd7..50bf4e8884 100644 --- a/packages/objectql/src/in-memory-aggregation.test.ts +++ b/packages/objectql/src/in-memory-aggregation.test.ts @@ -115,6 +115,69 @@ describe('applyInMemoryAggregation', () => { expect(out.find((r) => r.stage === null)!.total).toBe(10); expect(out.find((r) => r.stage === 'null')!.total).toBe(5); }); + + // #3849 — these rows ARE `driver.find()` output, so a key that is not the + // value verbatim is a key that disagrees with every other read of the column. + // This used to `String()` everything, which the pushed-down path never did. + it('keys a non-empty bucket with the value verbatim, not a string of it', () => { + const dataset = [ + { qty: 3, won: true, amount: 1 }, + { qty: 3, won: false, amount: 2 }, + { qty: 7, won: false, amount: 4 }, + ]; + const agg = [{ function: 'sum' as const, field: 'amount', alias: 'total' }]; + + const byQty = applyInMemoryAggregation(dataset, { groupBy: ['qty'], aggregations: agg }); + expect(byQty.map((r) => r.qty).sort()).toEqual([3, 7]); + expect(byQty.find((r) => r.qty === 3)!.total).toBe(3); + + const byWon = applyInMemoryAggregation(dataset, { groupBy: ['won'], aggregations: agg }); + expect(byWon.map((r) => r.won).sort()).toEqual([false, true]); + expect(byWon.find((r) => r.won === false)!.total).toBe(6); + }); + + // The bucket id is built from the key, so preserving the key's type is only + // half of it — the id has to preserve it too, or `1` and `'1'` merge on the + // way in and the surviving key is whichever row happened to arrive first. + it('keeps values of different types in different buckets', () => { + const dataset = [ + { v: 1, amount: 1 }, + { v: '1', amount: 2 }, + { v: true, amount: 4 }, + { v: 'true', amount: 8 }, + { v: null, amount: 16 }, + { v: 'null', amount: 32 }, + ]; + const out = applyInMemoryAggregation(dataset, { + groupBy: ['v'], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + }); + expect(out).toHaveLength(6); + const total = (v: unknown) => out.find((r) => Object.is(r.v, v))!.total; + expect(total(1)).toBe(1); + expect(total('1')).toBe(2); + expect(total(true)).toBe(4); + expect(total('true')).toBe(8); + expect(total(null)).toBe(16); + expect(total('null')).toBe(32); + }); + + // `JSON.stringify` throws on a BigInt, and the id builder runs on every row of + // every grouped query — a shape that used to bucket fine under `String()` must + // not start crashing the aggregate. + it('buckets a BigInt key without throwing', () => { + const dataset = [ + { v: 9007199254740993n, amount: 1 }, + { v: 9007199254740993n, amount: 2 }, + { v: 9007199254740994n, amount: 4 }, + ]; + const out = applyInMemoryAggregation(dataset, { + groupBy: ['v'], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + }); + expect(out).toHaveLength(2); + expect(out.find((r) => r.v === 9007199254740993n)!.total).toBe(3); + }); }); describe('bucketDateValue', () => { diff --git a/packages/objectql/src/in-memory-aggregation.ts b/packages/objectql/src/in-memory-aggregation.ts index 54e34a950b..a065d2d964 100644 --- a/packages/objectql/src/in-memory-aggregation.ts +++ b/packages/objectql/src/in-memory-aggregation.ts @@ -40,6 +40,20 @@ // '(empty)', a localized "Uncategorized") and build drill filters as // `field = null`, so a sentinel leaked a raw English debug string into the UI // and turned the empty bucket's drill-through into a zero-row query. +// +// A NON-EMPTY KEY IS THE VALUE AS `find()` PRESENTS IT — no `String()` (#3849). +// The rows this function groups come straight from `driver.find()`, so passing +// the value through is what makes a bucket key equal the column's own read +// shape: a `number` stays a number, a `boolean` stays a boolean. This used to +// `String()` every key, which the pushed-down path never did, so `1` became +// `'1'` and `true` became `'true'` depending only on which path ran. +// +// That was not cosmetic. Several consumers probe a raw `Map` keyed by the +// value's own type — the select-option label table, the lookup FK → record-name +// table, the cross-object FK → attribute table — and `Map` lookup is +// SameValueZero, so `'1'` never finds `1`. A stringified key silently missed +// every entry: labels fell back to raw ids, and cross-object rebucketing filed +// every row under `'(restricted)'` while the grand total still reconciled. import { calendarPartsInTzOrUtc } from '@objectstack/core'; import type { QueryAST, GroupByNode, AggregationNode, DateGranularityValue } from '@objectstack/spec/data'; @@ -75,11 +89,11 @@ export function applyInMemoryAggregation( const fieldName = typeof g === 'string' ? g : (g.alias ?? g.field); const value = projectGroupValue(row, g, timezone); key[fieldName] = value; - // JSON-encoded so the empty bucket's `null` key cannot collide with a row - // whose value is the literal STRING `"null"` — plain interpolation renders - // both as `null` and would merge two distinct groups. This id is internal - // to the bucketing loop; only `key` is emitted. - parts.push(`${fieldName}=${JSON.stringify(value)}`); + // Type-preserving, because the key no longer is: `1` and `'1'`, `true` and + // `'true'`, `null` and `'null'` are all distinct groups and must not merge + // just because they interpolate the same. This id is internal to the + // bucketing loop; only `key` is emitted. + parts.push(`${fieldName}=${bucketIdPart(value)}`); } const id = parts.join('\u0001'); let bucket = buckets.get(id); @@ -98,15 +112,29 @@ export function applyInMemoryAggregation( return out; } -function projectGroupValue(row: any, g: GroupByNode, timezone?: string): string | null { +/** + * Stable, TYPE-PRESERVING encoding of one group value, for the internal bucket + * id only. `JSON.stringify` gives that for every shape a column can hold — + * `1` → `1`, `'1'` → `"1"`, `true` → `true`, `null` → `null` — except the two + * it refuses, which are handled explicitly so a value that used to bucket fine + * under `String()` cannot start throwing mid-aggregate. + */ +function bucketIdPart(v: unknown): string { + if (typeof v === 'bigint') return `bigint:${v}`; // JSON.stringify throws on these + return JSON.stringify(v) ?? `${typeof v}:${String(v)}`; // undefined / symbol / function +} + +function projectGroupValue(row: any, g: GroupByNode, timezone?: string): unknown { const field = typeof g === 'string' ? g : g.field; const v = row?.[field]; if (typeof g !== 'string' && g.dateGranularity) { return bucketDateValue(v, g.dateGranularity, timezone); } - // `null`, not a sentinel string — same key the pushed-down SQL gives a NULL - // group column (#3839). See the empty-bucket note at the top of this file. - return v == null ? null : String(v); + // The value as `driver.find()` presented it — these rows ARE find() output, so + // passing it through is what makes the bucket key equal the column's own read + // shape (#3849). `undefined` (absent field) folds into the empty bucket's + // `null` (#3839). See both notes at the top of this file. + return v ?? null; } function aggregateBucket(rows: any[], aggregations: AggregationNode[]): Record { diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index c564f1c237..6e4be246bd 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -243,6 +243,14 @@ export interface IntrospectedSchema { * all schema-mutating DDL. Defaults to `'managed'` when omitted, preserving * legacy behaviour. */ +/** + * How a stored column value must be reshaped to become the value a `find()` row + * presents. One entry per rule `formatOutput` applies to a scalar column; the + * read paths that bypass `formatOutput` (`aggregate`, `distinct`) name the rule + * per column instead. See {@link SqlDriver.readPresentationKind}. + */ +export type ReadPresentationKind = 'datetime' | 'date' | 'boolean' | 'number'; + export type SqlDriverConfig = Knex.Config & { schemaMode?: SchemaMode; /** @@ -1668,14 +1676,15 @@ export class SqlDriver implements IDataDriver { // GROUP BY bucket expression (#3773) and the result presentation (#3797). const table = this.coercionKey(builder); - // Result columns that carry a raw temporal VALUE, keyed by the column name - // the caller will read. Collected while the statement is built because that - // is the only point where a column name and its meaning are both known: a - // `min()` lands under its alias (never under the field name), and a - // date-BUCKETED column lands under the field name while holding a label - // (`'2026-01'`), not an instant. Matching on names after the fact gets both - // of those backwards. See {@link presentTemporalColumns}. - const temporalOutput = new Map(); + // Result columns that carry a raw column VALUE (rather than a count/total + // derived from one), keyed by the column name the caller will read. + // Collected while the statement is built because that is the only point + // where a column name and its meaning are both known: a `min()` lands under + // its alias (never under the field name), and a date-BUCKETED column lands + // under the field name while holding a label (`'2026-01'`), not a value. + // Matching on names after the fact gets both of those backwards. + // See {@link presentReadColumns}. + const presentedOutput = new Map(); if (query.groupBy) { // groupBy items may be plain strings ('region') or structured objects @@ -1686,8 +1695,8 @@ export class SqlDriver implements IDataDriver { if (typeof g === 'string') { builder.groupBy(g); builder.select(g); - const kind = this.temporalFieldKind(table, g); - if (kind) temporalOutput.set(g, kind); + const kind = this.readPresentationKind(table, g); + if (kind) presentedOutput.set(g, kind); } else if (g && typeof g === 'object' && g.field) { if (g.dateGranularity) { const bucket = this.buildDateBucketExpr(g.field, g.dateGranularity as any, table); @@ -1702,8 +1711,8 @@ export class SqlDriver implements IDataDriver { } else { builder.groupBy(g.field); builder.select(g.field); - const kind = this.temporalFieldKind(table, g.field); - if (kind) temporalOutput.set(g.field, kind); + const kind = this.readPresentationKind(table, g.field); + if (kind) presentedOutput.set(g.field, kind); } } } @@ -1724,13 +1733,14 @@ export class SqlDriver implements IDataDriver { } // `min`/`max` are the only supported functions that hand back a value // OF the column rather than a count/total derived from it, so they are - // the only ones whose result is still an instant. `alias` is required - // by `AggregationNodeSchema`; the unaliased branch below lands under a - // dialect-dependent column name (`max("closed_at")` on SQLite, `max` on - // Postgres) and is defensive only, so it is deliberately not tracked. + // the only ones whose result still needs the column's presentation. + // `alias` is required by `AggregationNodeSchema`; the unaliased branch + // below lands under a dialect-dependent column name + // (`max("closed_at")` on SQLite, `max` on Postgres) and is defensive + // only, so it is deliberately not tracked. if ((funcName === 'min' || funcName === 'max') && agg.field) { - const kind = this.temporalFieldKind(table, agg.field); - if (kind) temporalOutput.set(agg.alias, kind); + const kind = this.readPresentationKind(table, agg.field); + if (kind) presentedOutput.set(agg.alias, kind); } } else { if (fieldExpr === '*') { @@ -1743,7 +1753,7 @@ export class SqlDriver implements IDataDriver { } const rows = await builder; - return this.presentTemporalColumns(rows, temporalOutput); + return this.presentReadColumns(rows, presentedOutput); } // =================================== @@ -1761,15 +1771,15 @@ export class SqlDriver implements IDataDriver { const results = await builder; const values = results.map((row: any) => row[field]); - // Same presentation `find()` gives the column (#3797) — a caller listing a - // datetime's values should not get epoch integers here and ISO strings - // there. Re-deduplicate afterwards: SQL `DISTINCT` compares STORED values, - // and one SQLite `Field.datetime` column holds both INTEGER epoch ms and - // ISO TEXT, so two rows recording the same instant survive as two rows and - // then collapse to the same presented value. - const kind = this.temporalFieldKind(this.coercionKey(builder), field); + // Same presentation `find()` gives the column (#3797, #3849) — a caller + // listing a datetime's values should not get epoch integers here and ISO + // strings there, nor `0`/`1` for a boolean. Re-deduplicate afterwards: SQL + // `DISTINCT` compares STORED values, and one SQLite `Field.datetime` column + // holds both INTEGER epoch ms and ISO TEXT, so two rows recording the same + // instant survive as two rows and then collapse to the same presented value. + const kind = this.readPresentationKind(this.coercionKey(builder), field); if (!kind) return values; - return [...new Set(values.map((v: any) => this.presentTemporalValue(kind, v)))]; + return [...new Set(values.map((v: any) => this.presentReadValue(kind, v)))]; } // =================================== @@ -3532,23 +3542,64 @@ export class SqlDriver implements IDataDriver { } /** - * Present one temporal value exactly the way `formatOutput` presents it on a - * `find()` row, for the read paths that return raw builder output instead - * (`aggregate`, `distinct` — #3797). + * Which read-presentation rule, if any, a declared field takes — the same + * question {@link formatOutput} answers implicitly while walking a `find()` + * row, asked one field at a time so the paths that return raw builder output + * can ask it too. `null` means the stored form already IS the presented form. + * + * The boolean / numeric rules are SQLite-only because `formatOutput` gates + * them that way: SQLite is the dialect without a native boolean, and the + * numeric repair only exists for legacy TEXT-affinity columns. + */ + protected readPresentationKind( + table: string | null | undefined, + field: string, + ): ReadPresentationKind | null { + if (!table) return null; + const temporal = this.temporalFieldKind(table, field); + if (temporal) return temporal; + if (!this.isSqlite) return null; + if (this.booleanFields[table]?.includes(field)) return 'boolean'; + if (this.numericFields[table]?.includes(field)) return 'number'; + return null; + } + + /** + * Present one value exactly the way `formatOutput` presents it on a `find()` + * row, for the read paths that return raw builder output instead + * (`aggregate`, `distinct` — #3797 for instants, #3849 for scalars). * - * The dialect gating mirrors `formatOutput`: the `Field.datetime` repair is - * SQLite-only (it is the one dialect where storage ≠ presentation), while the - * `Field.date` → `YYYY-MM-DD` collapse runs everywhere. + * The dialect gating mirrors `formatOutput`: the `Field.datetime` repair and + * the boolean / numeric coercions are SQLite-only (it is the one dialect where + * storage ≠ presentation), while the `Field.date` → `YYYY-MM-DD` collapse runs + * everywhere. {@link readPresentationKind} does the SQLite gating for the + * scalar kinds, so by the time one arrives here the dialect is settled. */ - protected presentTemporalValue(kind: 'datetime' | 'date', value: any): any { + protected presentReadValue(kind: ReadPresentationKind, value: any): any { if (value == null) return value; - if (kind === 'date') return this.toDateOnly(value); - return this.isSqlite ? normalizeSqliteDatetimeOutput(value) : value; + switch (kind) { + case 'date': + return this.toDateOnly(value); + case 'datetime': + return this.isSqlite ? normalizeSqliteDatetimeOutput(value) : value; + case 'boolean': + return Boolean(value); + case 'number': { + // Only strings are repaired, exactly as in `formatOutput`: a fresh + // REAL/INTEGER column already yields a number, and genuinely + // non-numeric legacy junk is left intact rather than turned into NaN. + if (typeof value === 'string' && value.trim() !== '') { + const n = Number(value); + if (!Number.isNaN(n)) return n; + } + return value; + } + } } /** - * Apply {@link presentTemporalValue} to the result columns a caller of - * `aggregate()` will read as instants. + * Apply {@link presentReadValue} to the result columns a caller of + * `aggregate()` will read as column VALUES — group keys, and `min`/`max`. * * Which columns those are cannot be recovered from the rows — the driver has * to be told, because the mapping from column name to meaning is only @@ -3556,12 +3607,12 @@ export class SqlDriver implements IDataDriver { * alias; a date-BUCKETED column lands under the field name but holds a label). * Rows are mutated in place, as `formatOutput` does. */ - protected presentTemporalColumns(rows: any, columns: Map): any { + protected presentReadColumns(rows: any, columns: Map): any { if (columns.size === 0 || !Array.isArray(rows)) return rows; for (const row of rows) { if (!row || typeof row !== 'object') continue; for (const [column, kind] of columns) { - if (row[column] !== undefined) row[column] = this.presentTemporalValue(kind, row[column]); + if (row[column] !== undefined) row[column] = this.presentReadValue(kind, row[column]); } } return rows; diff --git a/packages/qa/dogfood/test/group-key-read-shape-parity.test.ts b/packages/qa/dogfood/test/group-key-read-shape-parity.test.ts new file mode 100644 index 0000000000..d0b9356bd7 --- /dev/null +++ b/packages/qa/dogfood/test/group-key-read-shape-parity.test.ts @@ -0,0 +1,152 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Group-key read-shape parity (#3849). +// +// A `groupBy` key is a COLUMN VALUE, so there is only one right answer for what +// it looks like: whatever that column looks like on a `find()` row. Three code +// paths produce it and all three must agree — +// +// find() → `formatOutput`, the canonical read shape +// aggregate() pushed down → raw builder output + per-column presentation +// applyInMemoryAggregation() → passes through find()'s own output +// +// — because `engine.aggregate` picks between the latter two per query (does the +// driver aggregate natively? does it advertise the requested granularity? is the +// reference timezone UTC?), and `find()` is what every non-aggregated read of +// the same column returns. +// +// They did not agree. The in-memory path `String()`-coerced every key (`3` → +// `'3'`, `true` → `'true'`), and the pushed-down path skipped the boolean and +// numeric repairs `formatOutput` applies, so a SQLite boolean surfaced as `0`/`1` +// there and `'false'`/`'true'` here. Three paths, three answers, same column. +// +// The damage was silent because every measure still reconciled. What broke was +// downstream code that probes a raw `Map` keyed by the value's own type — the +// select-option label table, the lookup FK → record-name table, the cross-object +// FK → attribute table. `Map` lookup is SameValueZero, so `'1'` never finds `1`: +// labels fell back to raw ids, and cross-object rebucketing filed every row +// under `'(restricted)'` while the totals stayed correct. +// +// This asserts the TYPE as well as the value. Folding both sides through +// `String()` — the reflex when comparing bucket labels — is exactly what hid +// this, and would make the check pass against the bug it exists to catch. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { applyInMemoryAggregation } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { SqliteWasmDriver } from '@objectstack/driver-sqlite-wasm'; + +const TABLE = 'deal'; + +const DRIVERS = [ + { + name: 'driver-sql (better-sqlite3 :memory:)', + make: () => + new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }) as any, + }, + { + name: 'driver-sqlite-wasm (:memory:)', + make: () => new SqliteWasmDriver({ filename: ':memory:' }) as any, + }, +]; + +/** + * Every column whose stored form differs from its presented form on SQLite, plus + * a text control. `text` is the shape that always agreed — if it ever fails, the + * harness itself is wrong, not the driver. + */ +const COLUMNS = ['qty', 'won', 'stage'] as const; + +/** `value` pairs, sorted. Carrying the TYPE is the whole point. */ +function shape(rows: any[], field: string): string[] { + return rows + .map((r) => `${JSON.stringify(r[field]) ?? 'undefined'}<${r[field] === null ? 'null' : typeof r[field]}>`) + .sort(); +} + +describe.each(DRIVERS)('group-key read-shape parity: $name', ({ make }) => { + let driver: any; + let rows: any[]; + + beforeEach(async () => { + driver = make(); + await driver.initObjects([ + { + name: TABLE, + fields: { + qty: { type: 'number' }, + won: { type: 'boolean' }, + stage: { type: 'text' }, + amount: { type: 'number' }, + }, + }, + ]); + const opts = { bypassTenantAudit: true }; + // Two rows share `qty`/`stage` and differ on `won`, so every column produces + // both a merged and a split bucket — a key that silently changed shape would + // otherwise be able to hide behind a one-row-per-bucket fixture. + await driver.create(TABLE, { id: 'a', qty: 3, won: true, stage: 'won', amount: 1 }, opts); + await driver.create(TABLE, { id: 'b', qty: 3, won: false, stage: 'won', amount: 2 }, opts); + await driver.create(TABLE, { id: 'c', qty: 7, won: false, stage: 'lost', amount: 4 }, opts); + // What `engine.aggregate` feeds the in-memory fallback, and the canonical + // presentation both aggregate paths are measured against. + rows = await driver.find(TABLE, { object: TABLE }); + }); + + afterEach(async () => { + await driver?.disconnect?.(); + }); + + it.each(COLUMNS)("groups by '%s' identically on both paths, in find()'s shape", async (field) => { + const ast = { + object: TABLE, + groupBy: [field], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + }; + + const pushedDown = await driver.aggregate(TABLE, ast); + const inMemory = applyInMemoryAggregation(rows, ast as never); + + // The two aggregate paths agree with each other… + expect(shape(pushedDown, field)).toEqual(shape(inMemory, field)); + // …and with the shape the same column takes on a plain read. Deduplicated, + // because find() returns one entry per ROW and aggregate one per BUCKET. + const fromFind = [...new Set(shape(rows, field))].sort(); + expect([...new Set(shape(pushedDown, field))].sort()).toEqual(fromFind); + + // Totals were never the broken part; assert them so a "fix" that lost rows + // while making the keys agree cannot pass. + const total = (rs: any[]) => rs.reduce((a, r) => a + Number(r.total), 0); + expect(total(pushedDown)).toBe(7); + expect(total(inMemory)).toBe(7); + }); + + // Spelled out separately from the parametrised case above: these are the two + // concrete shapes #3849 was filed about, and naming them makes a regression + // report readable without decoding a `shape()` string. + it('keeps a number key a number and a boolean key a boolean', async () => { + const by = async (field: string) => { + const ast = { + object: TABLE, + groupBy: [field], + aggregations: [{ function: 'count', alias: 'n' }], + }; + return { + sql: (await driver.aggregate(TABLE, ast)).map((r: any) => r[field]).sort(), + mem: applyInMemoryAggregation(rows, ast as never).map((r: any) => r[field]).sort(), + }; + }; + + const qty = await by('qty'); + expect(qty.sql).toEqual([3, 7]); + expect(qty.mem).toEqual([3, 7]); + + const won = await by('won'); + expect(won.sql).toEqual([false, true]); + expect(won.mem).toEqual([false, true]); + }); +});