fix(objectql,driver-sql)!: a group key is the column's value, in the shape find() presents it (#3849)

.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<number>` against `false<boolean>`.

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', () => {

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<string, any> {