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

Three code paths produce a `groupBy` key and no two 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()`, which the pushed-down path never did. And 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 — no native type, stored 0/1 — surfaced as an integer from
`aggregate()` and a real boolean from `find()`.

`engine.aggregate` picks between the two aggregate paths per query, so the same
column changed shape with no change to the data or the query.

The measures were always right, which is why nobody noticed. What broke was
downstream code probing a raw `Map` keyed by the value's own type — `Map` lookup
is SameValueZero, so '1' never finds 1: select-option labels fell back to raw
values, lookup labels missed whenever the referenced object had a numeric id
(routine for external objects), and cross-object rebucketing filed every row
under RESTRICTED_BUCKET — one bar, correct grand total, no error. A boolean
dimension drilled from the in-memory path also sent { won: 'true' } to SQLite,
which an INTEGER column can never match.

- `applyInMemoryAggregation` emits the value verbatim; its rows are find()
  output, so passing through is what makes the key equal the column's read shape.
- The internal bucket id is type-preserving, so 1 and '1', true and 'true' stay
  distinct groups. BigInt is encoded explicitly — JSON.stringify throws on it,
  and a value that bucketed fine under String() must not start crashing.
- `SqlDriver.aggregate`/`.distinct` present group keys and min/max with the same
  rules `formatOutput` applies, generalizing #3797 to boolean and numeric
  columns. The protected helpers are renamed to match (temporalFieldKind →
  readPresentationKind, presentTemporalValue → presentReadValue,
  presentTemporalColumns → presentReadColumns) and the kind union is exported.

Date-bucketed groupBy is unaffected: both sides already produce canonical string
labels, and #3839 pinned their empty bucket.

Gate: group-key-read-shape-parity.test.ts measures both aggregate paths against
find() for number, boolean and text columns on driver-sql and
driver-sqlite-wasm, asserting the runtime TYPE — folding through String() is the
reflex that hid this and would pass against the bug. Each half was confirmed to
redden the gate on its own.

Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

.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> {