From 59441976174f583183894f5f12c44d0659cb4c5e Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:41:13 +0800 Subject: [PATCH] =?UTF-8?q?fix(objectql)!:=20one=20key=20for=20the=20empty?= =?UTF-8?q?=20group=20bucket=20=E2=80=94=20real=20`null`,=20on=20both=20ag?= =?UTF-8?q?gregation=20paths=20(#3839)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `engine.aggregate` has two implementations of one feature and picks between them per query: it pushes the aggregate down as SQL when the driver advertises every requested granularity and the reference timezone is UTC, otherwise it fetches rows and buckets them in JS. The two disagreed about how to spell "empty" — SQL NULL against the in-memory literal `'(null)'` — so the same dataset produced a different bucket key type on SQLite+UTC+`month` than on `week`, a non-UTC timezone, or driver-rest / driver-memory / a remote Turso. The measures were always right; only the key's type and literal differed, which is why it went unnoticed. It was never date-specific either — a plain `groupBy: ['stage']` over a NULL column diverged the same way. Consumers are written against `null`: they check `== null` and supply their own empty label. The sentinel defeated every one of them, leaking a raw English debug string into the UI and compiling a drill on the empty bucket to `field = '(null)'`, which matches nothing. The comment justifying the string cited the client `useReportData` hook, removed with ADR-0021 — and the literal never appeared in it. - `applyInMemoryAggregation` / `bucketDateValue` key the empty bucket as `null`; `bucketDateValue` returns `string | null`. Null and unparseable instants still share one bucket, because SQL cannot tell them apart either. - The internal composite bucket id is JSON-encoded, so the empty bucket stays distinct from a row whose value is the literal string `"null"`. Same in service-analytics' cross-object rebucketing. - `bucketKeyToCalendarRange` accepts `string | null` — the empty bucket has no calendar span, so a drill on it opens the unscoped superset as before. - The driver output contract in spec now states the rule. Gates: `checkDateBucketParity`'s fixture deliberately had no null instant because the divergence would have failed it for a reason it was not about; it has one now. Two fixes made that meaningful — the check folded labels through `String(value)`, which turns SQL NULL into `'null'` and could compare equal to a sentinel string, and it compared label sets with `JSON.stringify`, which is sensitive to key insertion order that the two paths legitimately differ on (a correct driver could be flagged, with an empty diff message). A new dogfood check covers the non-date half against real drivers. Both gates were confirmed to fail with the sentinel restored. Co-Authored-By: Claude --- .changeset/empty-group-bucket-key-null.md | 87 +++++++++++++++ packages/core/src/utils/datetime.ts | 10 +- .../objectql/src/date-bucket-range.test.ts | 6 ++ .../src/in-memory-aggregation.test.ts | 47 ++++++-- .../objectql/src/in-memory-aggregation.ts | 52 ++++++--- .../sql-driver-date-bucket-storage.test.ts | 30 ++++-- .../src/sql-driver-date-bucket.test.ts | 20 +++- .../sqlite-wasm-driver-date-bucket.test.ts | 20 +++- .../date-bucket-parity-conformance.test.ts | 31 +++++- .../test/empty-group-bucket-parity.test.ts | 100 ++++++++++++++++++ .../src/analytics-service.ts | 4 +- .../src/strategies/cross-object-rebucket.ts | 7 +- packages/spec/src/data/driver.zod.ts | 7 ++ packages/verify/src/date-bucket-parity.ts | 69 +++++++++--- 14 files changed, 433 insertions(+), 57 deletions(-) create mode 100644 .changeset/empty-group-bucket-key-null.md create mode 100644 packages/qa/dogfood/test/empty-group-bucket-parity.test.ts diff --git a/.changeset/empty-group-bucket-key-null.md b/.changeset/empty-group-bucket-key-null.md new file mode 100644 index 0000000000..c01414d38d --- /dev/null +++ b/.changeset/empty-group-bucket-key-null.md @@ -0,0 +1,87 @@ +--- +"@objectstack/objectql": minor +"@objectstack/verify": minor +"@objectstack/core": patch +"@objectstack/service-analytics": patch +"@objectstack/spec": patch +--- + +fix(objectql)!: one key for the empty group bucket — real `null`, on both aggregation paths (#3839) + +A grouped row whose dimension value is empty now carries `null` for that +dimension no matter which way the aggregate ran. Downstream code can test the +empty bucket with a plain `value == null` again: charts render their own empty +label, drill-through on that bucket builds `field = null` and returns the rows +it should, and a dashboard no longer changes shape when the driver, the +granularity or the reference timezone changes. + +### What was wrong + +`engine.aggregate` has two implementations of one feature. It pushes the +aggregate down as SQL when the driver advertises every requested granularity and +the reference timezone is UTC; otherwise it fetches rows and buckets them in JS. +The two disagreed about how to spell "empty": + +``` +--- same dataset, same query, one row with a NULL value --- + pushed-down SQL : [{ "key": null, "type": "null", "total": 2 }, …] + in-memory : [{ "key": "(null)", "type": "string", "total": 2 }, …] +``` + +The measures were always right — only the key's type and literal differed — +which is why this went unnoticed for so long: every total reconciled. But the +engine picks a path per query, so the same data produced a different bucket key +on SQLite-plus-UTC-plus-`month` than on `week` (which SQLite does not advertise), +a non-UTC timezone, or `driver-rest` / `driver-memory` / a remote Turso, all of +which bucket in memory unconditionally. + +It was never date-specific either. A plain `groupBy: ['stage']` over a NULL +column diverged the same way. + +Consumers are written against `null` — they check `== null` and supply their own +empty label ('—', '(empty)', a localized "Uncategorized"). The sentinel defeated +every one of them: it rendered a raw English debug string in the UI, and a drill +on the empty bucket compiled to `field = '(null)'` and matched nothing. + +The in-memory path's comment justified the string as staying "consistent with +the client `useReportData` hook". That hook was removed with ADR-0021, and the +literal never appeared in it. + +### What changed + +- `applyInMemoryAggregation` and `bucketDateValue` (`@objectstack/objectql`) key + the empty bucket as `null`. `bucketDateValue` now returns `string | null`. A + null instant and an unparseable one still share one bucket, because SQL cannot + tell them apart either (`strftime('%Y-%m', 'not-a-date')` is NULL). +- The internal composite bucket id is JSON-encoded, so the empty bucket stays + distinct from a row whose value is the literal string `"null"`. +- `bucketKeyToCalendarRange` (`@objectstack/core`) accepts `string | null`. The + empty bucket has no calendar span, so a drill on it opens the unscoped + superset instead of an invented bound — unchanged behavior, honest signature. +- The driver output contract in `@objectstack/spec` now states the rule: a row + with no value keys as `null`, never a sentinel. Propagating NULL through the + bucket expression is the whole of it; a driver only breaks it by adding a + `COALESCE`. + +### Gates + +`checkDateBucketParity` (`@objectstack/verify`) deliberately carried no null +instant, because the divergence would have failed it for a reason it was not +about. Its fixture now has one, so the convergence is held in place — including +for out-of-tree drivers that run the check against themselves. + +Two fixes were needed to make that fixture meaningful: + +- The check folded bucket labels through `String(value)`, which turns SQL NULL + into `'null'` — a label a TEXT column can genuinely hold. A driver spelling + "empty" as a string could compare equal to one returning real NULL. The empty + bucket is now keyed out of band. +- Label sets were compared with `JSON.stringify`, which is sensitive to key + insertion order. Row order is not part of this contract and the two paths + naturally differ (SQL sorts its groups; the in-memory path emits first-seen + order), so a driver with entirely correct buckets could be reported as + disagreeing — with an empty diff message, since nothing actually differed. + The comparison is now order-insensitive. + +A new dogfood check covers the non-date half against real drivers: same dataset, +plain and date-bucketed `groupBy`, both paths, one key. diff --git a/packages/core/src/utils/datetime.ts b/packages/core/src/utils/datetime.ts index 08791cba0a..68939148e4 100644 --- a/packages/core/src/utils/datetime.ts +++ b/packages/core/src/utils/datetime.ts @@ -144,13 +144,17 @@ function isoWeekLabelUtc(d: Date): string { * `datetime` field in a reference timezone layers that on top (and, per * ADR-0053, a `date` field compares against these `YYYY-MM-DD` bounds directly). * - * Returns `null` for the null/empty bucket, an unparseable key, or a key that - * is shape-valid but out of range (e.g. `2026-13`, a `-W53` in a 52-week year, + * Returns `null` for the empty bucket, an unparseable key, or a key that is + * shape-valid but out of range (e.g. `2026-13`, a `-W53` in a 52-week year, * `2026-02-30`). Callers drop the range and fall back to an unscoped (superset) * drill rather than emit a wrong bound. + * + * `key` admits `null` because that IS the empty bucket's key on both aggregation + * paths (#3839); callers pass a grouped row's dimension value straight through + * rather than casting a lie. */ export function bucketKeyToCalendarRange( - key: string, + key: string | null | undefined, granularity: BucketGranularity, ): { start: string; end: string } | null { if (typeof key !== 'string' || key.length === 0) return null; diff --git a/packages/objectql/src/date-bucket-range.test.ts b/packages/objectql/src/date-bucket-range.test.ts index cb5db037f8..ff56c674f3 100644 --- a/packages/objectql/src/date-bucket-range.test.ts +++ b/packages/objectql/src/date-bucket-range.test.ts @@ -73,6 +73,12 @@ describe('bucketKeyToCalendarRange exact boundaries', () => { describe('bucketKeyToCalendarRange rejects unbucketable / out-of-range keys → null (superset fallback)', () => { it('null and empty buckets', () => { + // #3839 — the empty bucket's key IS `null`, on both the pushed-down and the + // in-memory path. It has no calendar span, so the caller drops the range and + // drills the unscoped superset instead of inventing a bound. + expect(bucketKeyToCalendarRange(null, 'month')).toBeNull(); + expect(bucketKeyToCalendarRange(undefined, 'month')).toBeNull(); + // The pre-#3839 sentinel, in case one survives in stored/replayed data. expect(bucketKeyToCalendarRange('(null)', 'month')).toBeNull(); expect(bucketKeyToCalendarRange('', 'day')).toBeNull(); }); diff --git a/packages/objectql/src/in-memory-aggregation.test.ts b/packages/objectql/src/in-memory-aggregation.test.ts index 4a543a553a..992263bbd7 100644 --- a/packages/objectql/src/in-memory-aggregation.test.ts +++ b/packages/objectql/src/in-memory-aggregation.test.ts @@ -76,13 +76,44 @@ describe('applyInMemoryAggregation', () => { expect(east!.owner_str).toBe('alice,alice,bob'); }); - it('treats null group values as the literal (null) bucket', () => { - const dataset = [{ stage: null, amount: 10 }, { stage: 'won', amount: 5 }]; + // #3839 — this used to be the literal string `'(null)'`, which the pushed-down + // SQL path never produced (a NULL group column stays SQL NULL). The engine + // picks between the two paths per query, so the bucket key's TYPE changed + // under a dashboard when the driver, the granularity or the timezone changed. + it('keys the empty bucket as real null, like the pushed-down SQL', () => { + const dataset = [ + { stage: null, amount: 10 }, + { stage: undefined, amount: 1 }, + { stage: 'won', amount: 5 }, + { amount: 2 }, // field absent entirely + ]; const out = applyInMemoryAggregation(dataset, { groupBy: ['stage'], aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], }); - expect(out.find((r) => r.stage === '(null)')!.total).toBe(10); + // null / undefined / absent all describe the same emptiness → one bucket. + const empty = out.filter((r) => r.stage === null); + expect(empty).toHaveLength(1); + expect(empty[0].total).toBe(13); + // …and it is a real null, not a string that happens to read like one. + expect(out.some((r) => typeof r.stage === 'string' && /null/i.test(r.stage))).toBe(false); + }); + + // The empty bucket's key is now `null`, and `${null}` is the string 'null' — + // so a row whose value IS the string "null" would merge into the empty bucket + // if the internal bucket id were built by plain interpolation. + it('keeps the empty bucket distinct from the literal string "null"', () => { + const dataset = [ + { stage: null, amount: 10 }, + { stage: 'null', amount: 5 }, + ]; + const out = applyInMemoryAggregation(dataset, { + groupBy: ['stage'], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + }); + expect(out).toHaveLength(2); + expect(out.find((r) => r.stage === null)!.total).toBe(10); + expect(out.find((r) => r.stage === 'null')!.total).toBe(5); }); }); @@ -101,9 +132,13 @@ describe('bucketDateValue', () => { expect(bucketDateValue('2024-12-30', 'week')).toBe('2025-W01'); }); - it('returns (null) for null / invalid dates', () => { - expect(bucketDateValue(null, 'month')).toBe('(null)'); - expect(bucketDateValue('not-a-date', 'month')).toBe('(null)'); + // #3839 — `null`, not a sentinel string. SQL propagates NULL through the + // bucket expression for both of these (`strftime('%Y-%m', 'not-a-date')` is + // NULL), so the two paths agree on the empty bucket as well as the full ones. + it('returns null for null / invalid dates', () => { + expect(bucketDateValue(null, 'month')).toBeNull(); + expect(bucketDateValue(undefined, 'month')).toBeNull(); + expect(bucketDateValue('not-a-date', 'month')).toBeNull(); }); // #3773 — parity with the pushed-down SQL. SQLite stores a `Field.datetime` diff --git a/packages/objectql/src/in-memory-aggregation.ts b/packages/objectql/src/in-memory-aggregation.ts index 7357b9d2cf..54e34a950b 100644 --- a/packages/objectql/src/in-memory-aggregation.ts +++ b/packages/objectql/src/in-memory-aggregation.ts @@ -21,9 +21,25 @@ // possible; the in-memory fallback ignores the per-aggregation filter and // logs a warning if one is present. // -// Date bucketing uses ISO-8601 conventions (weeks start Monday). Null / -// invalid values bucket as the literal string `'(null)'` to remain -// consistent with the client `useReportData` hook. +// Date bucketing uses ISO-8601 conventions (weeks start Monday). +// +// THE EMPTY BUCKET'S KEY IS REAL `null` — not a sentinel string (#3839). A +// grouped row whose value is null/absent (or, for a date bucket, unparseable) +// carries `null` for that dimension, which is what the pushed-down SQL path +// emits for the same row: the group column is SQL NULL, or `strftime(...)` / +// `date_trunc(...)` returns NULL for a NULL input. Both paths therefore +// describe "empty" the same way, and `engine.aggregate` picking one per query +// — by driver, granularity, or reference timezone — can no longer change the +// TYPE of a bucket key under a dashboard that drills across the seam. +// +// This previously emitted the literal `'(null)'` "to remain consistent with +// the client `useReportData` hook". That hook was deleted with ADR-0021 (its +// epitaph is objectui `packages/plugin-report/src/index.tsx`), and the string +// never appeared in it anyway. What the string DID do was defeat every +// downstream `== null` check: consumers render their own empty label ('—', +// '(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. import { calendarPartsInTzOrUtc } from '@objectstack/core'; import type { QueryAST, GroupByNode, AggregationNode, DateGranularityValue } from '@objectstack/spec/data'; @@ -59,7 +75,11 @@ export function applyInMemoryAggregation( const fieldName = typeof g === 'string' ? g : (g.alias ?? g.field); const value = projectGroupValue(row, g, timezone); key[fieldName] = value; - parts.push(`${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)}`); } const id = parts.join('\u0001'); let bucket = buckets.get(id); @@ -78,13 +98,15 @@ export function applyInMemoryAggregation( return out; } -function projectGroupValue(row: any, g: GroupByNode, timezone?: string): string { +function projectGroupValue(row: any, g: GroupByNode, timezone?: string): string | null { const field = typeof g === 'string' ? g : g.field; const v = row?.[field]; if (typeof g !== 'string' && g.dateGranularity) { return bucketDateValue(v, g.dateGranularity, timezone); } - return v == null ? '(null)' : String(v); + // `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); } function aggregateBucket(rows: any[], aggregations: AggregationNode[]): Record { @@ -199,23 +221,29 @@ function toNumber(v: any): number { * A finite NUMBER is read as epoch milliseconds — the form SQLite stores a * `Field.datetime` in, and what any driver that hands back raw storage values * yields. `new Date(String(1767225600000))` is an Invalid Date, so without this - * branch such a row bucketed as `'(null)'` while the pushed-down SQL bucketed it - * correctly (#3773) — the two paths must label the same instant identically or a - * drill-down built on one breaks against the other. + * branch such a row landed in the empty bucket while the pushed-down SQL + * bucketed it correctly (#3773) — the two paths must label the same instant + * identically or a drill-down built on one breaks against the other. + * + * Returns `null` for a null/absent or unparseable instant — the same key the + * pushed-down SQL yields, where the bucket expression propagates NULL (#3839). + * Null and unparseable deliberately share one bucket: SQL cannot tell them + * apart either (`strftime('%Y-%m', 'not-a-date')` is NULL), and splitting them + * here would re-open the seam this function exists to close. */ export function bucketDateValue( value: unknown, granularity: DateGranularityValue, timezone?: string, -): string { - if (value == null) return '(null)'; +): string | null { + if (value == null) return null; const d = value instanceof Date ? value : typeof value === 'number' ? new Date(value) : new Date(String(value)); - if (Number.isNaN(d.getTime())) return '(null)'; + if (Number.isNaN(d.getTime())) return null; const { year: y, month: m, day } = calendarPartsInTzOrUtc(d, timezone); switch (granularity) { case 'year': diff --git a/packages/plugins/driver-sql/src/sql-driver-date-bucket-storage.test.ts b/packages/plugins/driver-sql/src/sql-driver-date-bucket-storage.test.ts index 6279481901..7f161a1eb6 100644 --- a/packages/plugins/driver-sql/src/sql-driver-date-bucket-storage.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-date-bucket-storage.test.ts @@ -37,12 +37,12 @@ const GRANULARITIES: Granularity[] = ['day', 'month', 'quarter', 'year']; * in `packages/qa/dogfood/test/date-bucket-parity-conformance.test.ts`, where * the reference side is the REAL `applyInMemoryAggregation`. */ -function bucketDateValue(value: unknown, g: Granularity): string { - if (value == null) return '(null)'; +function bucketDateValue(value: unknown, g: Granularity): string | null { + if (value == null) return null; // A finite number is epoch milliseconds — SQLite's `Field.datetime` storage. const d = value instanceof Date ? value : typeof value === 'number' ? new Date(value) : new Date(String(value)); - if (Number.isNaN(d.getTime())) return '(null)'; + if (Number.isNaN(d.getTime())) return null; const y = d.getUTCFullYear(); const m = d.getUTCMonth() + 1; switch (g) { @@ -73,11 +73,21 @@ const FIXTURE: Array<{ id: string; iso: string; amount: number }> = [ { id: 'r7', iso: '2026-07-01T00:00:00.000Z', amount: 64 }, // first instant of Q3 ]; +/** + * Test-local key for the empty bucket. `String(null)` is `'null'` — a label a + * TEXT column could genuinely hold, and the coercion that used to hide the + * #3839 divergence: it made the SQL side's NULL and a sentinel string compare + * as different strings only by luck. Keying it out of band keeps "empty" + * unmistakable on both sides. + */ +const EMPTY = '‹empty bucket›'; +const labelOf = (v: unknown): string => (v == null ? EMPTY : String(v)); + /** The labels the in-memory path would produce, folded into bucket → sum. */ function expectedBuckets(g: Granularity): Record { const out: Record = {}; for (const row of FIXTURE) { - const key = bucketDateValue(row.iso, g); + const key = labelOf(bucketDateValue(row.iso, g)); out[key] = (out[key] ?? 0) + row.amount; } return out; @@ -88,7 +98,7 @@ async function bucketSums(driver: SqlDriver, field: string, g: Granularity) { groupBy: [{ field, dateGranularity: g }], aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], } as any); - return Object.fromEntries(rows.map((r: any) => [String(r[field]), Number(r.total)])); + return Object.fromEntries(rows.map((r: any) => [labelOf(r[field]), Number(r.total)])); } describe('SqlDriver date bucketing is storage-form independent (#3773)', () => { @@ -218,9 +228,11 @@ describe('SqlDriver date bucketing over a MIXED-form datetime column (#3773)', ( it('leaves a NULL instant in its own bucket', async () => { const byMonth = await bucketSums(driver, 'closed_at', 'month'); - // SQL NULL aliases to the string 'null' through `String(r[field])` — a - // pre-existing divergence from the in-memory label `'(null)'`, unchanged - // here and equally true of a TEXT-stored column. - expect(byMonth.null).toBe(8); + // SQL NULL propagates through the bucket expression, so the row lands in the + // empty bucket instead of under some real month. Since #3839 the in-memory + // path keys that bucket as `null` as well, so the two paths agree on it — + // the executable proof is `checkDateBucketParity`, whose fixture now carries + // a null instant for exactly this. + expect(byMonth[EMPTY]).toBe(8); }); }); diff --git a/packages/plugins/driver-sql/src/sql-driver-date-bucket.test.ts b/packages/plugins/driver-sql/src/sql-driver-date-bucket.test.ts index 1ece0c513b..39470cb2b6 100644 --- a/packages/plugins/driver-sql/src/sql-driver-date-bucket.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-date-bucket.test.ts @@ -17,6 +17,16 @@ import { SqlDriver } from '../src/index.js'; type Granularity = 'day' | 'week' | 'month' | 'quarter' | 'year'; +/** + * Test-local key for the empty bucket, on both sides of the comparison below. + * `String(null)` is `'null'` — a label a TEXT column could genuinely hold — so + * folding the empty bucket through it makes "empty" indistinguishable from a + * real value. Keying it out of band is what lets a sentinel-vs-NULL divergence + * (#3839) show up as a difference instead of comparing equal by coincidence. + */ +const EMPTY = '‹empty bucket›'; +const labelOf = (v: unknown): string => (v == null ? EMPTY : String(v)); + /** * ⚠️ Keep in sync with `packages/objectql/src/in-memory-aggregation.ts#bucketDateValue`. * @@ -27,12 +37,12 @@ type Granularity = 'day' | 'week' | 'month' | 'quarter' | 'year'; * in `packages/qa/dogfood/test/date-bucket-parity-conformance.test.ts`, where * the reference side is the REAL `applyInMemoryAggregation`. */ -function bucketDateValue(value: unknown, g: Granularity): string { - if (value == null) return '(null)'; +function bucketDateValue(value: unknown, g: Granularity): string | null { + if (value == null) return null; // A finite number is epoch milliseconds — SQLite's `Field.datetime` storage. const d = value instanceof Date ? value : typeof value === 'number' ? new Date(value) : new Date(String(value)); - if (Number.isNaN(d.getTime())) return '(null)'; + if (Number.isNaN(d.getTime())) return null; const y = d.getUTCFullYear(); const m = d.getUTCMonth() + 1; switch (g) { @@ -113,13 +123,13 @@ describe('SqlDriver date bucket (dateGranularity)', () => { const expectedBuckets = new Map(); for (const r of FIXTURE) { - const key = bucketDateValue(r.ts, g); + const key = labelOf(bucketDateValue(r.ts, g)); expectedBuckets.set(key, (expectedBuckets.get(key) ?? 0) + 1); } const actualBuckets = new Map(); for (const row of rows) { - actualBuckets.set(String(row.ts), Number(row.n)); + actualBuckets.set(labelOf(row.ts), Number(row.n)); } expect([...actualBuckets.entries()].sort()).toEqual( diff --git a/packages/plugins/driver-sqlite-wasm/src/sqlite-wasm-driver-date-bucket.test.ts b/packages/plugins/driver-sqlite-wasm/src/sqlite-wasm-driver-date-bucket.test.ts index ced80d3b1b..b238846f17 100644 --- a/packages/plugins/driver-sqlite-wasm/src/sqlite-wasm-driver-date-bucket.test.ts +++ b/packages/plugins/driver-sqlite-wasm/src/sqlite-wasm-driver-date-bucket.test.ts @@ -17,6 +17,16 @@ import { SqliteWasmDriver } from '../src/index.js'; type Granularity = 'day' | 'week' | 'month' | 'quarter' | 'year'; +/** + * Test-local key for the empty bucket, on both sides of the comparison below. + * `String(null)` is `'null'` — a label a TEXT column could genuinely hold — so + * folding the empty bucket through it makes "empty" indistinguishable from a + * real value. Keying it out of band is what lets a sentinel-vs-NULL divergence + * (#3839) show up as a difference instead of comparing equal by coincidence. + */ +const EMPTY = '‹empty bucket›'; +const labelOf = (v: unknown): string => (v == null ? EMPTY : String(v)); + /** * ⚠️ Keep in sync with `packages/objectql/src/in-memory-aggregation.ts#bucketDateValue`. * @@ -27,12 +37,12 @@ type Granularity = 'day' | 'week' | 'month' | 'quarter' | 'year'; * in `packages/qa/dogfood/test/date-bucket-parity-conformance.test.ts`, where * the reference side is the REAL `applyInMemoryAggregation`. */ -function bucketDateValue(value: unknown, g: Granularity): string { - if (value == null) return '(null)'; +function bucketDateValue(value: unknown, g: Granularity): string | null { + if (value == null) return null; // A finite number is epoch milliseconds — SQLite's `Field.datetime` storage. const d = value instanceof Date ? value : typeof value === 'number' ? new Date(value) : new Date(String(value)); - if (Number.isNaN(d.getTime())) return '(null)'; + if (Number.isNaN(d.getTime())) return null; const y = d.getUTCFullYear(); const m = d.getUTCMonth() + 1; switch (g) { @@ -109,13 +119,13 @@ describe('SqliteWasmDriver date bucket (dateGranularity)', () => { const expectedBuckets = new Map(); for (const r of FIXTURE) { - const key = bucketDateValue(r.ts, g); + const key = labelOf(bucketDateValue(r.ts, g)); expectedBuckets.set(key, (expectedBuckets.get(key) ?? 0) + 1); } const actualBuckets = new Map(); for (const row of rows) { - actualBuckets.set(String(row.ts), Number(row.n)); + actualBuckets.set(labelOf(row.ts), Number(row.n)); } expect([...actualBuckets.entries()].sort()).toEqual( diff --git a/packages/qa/dogfood/test/date-bucket-parity-conformance.test.ts b/packages/qa/dogfood/test/date-bucket-parity-conformance.test.ts index 6c28d3f038..45f2c38127 100644 --- a/packages/qa/dogfood/test/date-bucket-parity-conformance.test.ts +++ b/packages/qa/dogfood/test/date-bucket-parity-conformance.test.ts @@ -66,6 +66,7 @@ describe('checkDateBucketParity detects a driver whose SQL bucketing is wrong', { id: 'b5', at: '2025-01-01T00:00:00.000Z', on: '2025-01-01', n: 1 }, { id: 'b6', at: '2025-05-19T09:00:00.000Z', on: '2025-05-19', n: 1 }, { id: 'b7', at: '2025-05-19T22:30:00.000Z', on: '2025-05-19', n: 1 }, + { id: 'b8', at: null, on: null, n: 1 }, // the empty bucket (#3839) ]; return { async connect() {}, @@ -117,9 +118,10 @@ describe('checkDateBucketParity detects a driver whose SQL bucketing is wrong', return [ { on: '2024', n: 4 }, { on: '2025', n: 3 }, + { on: null, n: 1 }, ]; } - return [{ at: null, n: 7 }]; + return [{ at: null, n: 8 }]; }, supports: { queryDateGranularity: { year: true } }, }) as never, @@ -130,6 +132,33 @@ describe('checkDateBucketParity detects a driver whose SQL bucketing is wrong', ); }); + // #3839's shape, and the reason the fixture now carries a null instant. This + // driver buckets every real instant correctly and disagrees on ONE row: the + // empty one, which it spells as a sentinel string instead of NULL. That is + // precisely what the in-memory path did before #3839 — and with the old + // `String(row[field])` keying, `'null'` would have compared EQUAL to a real + // `null` and this check would have passed a driver that is out of step. + it.each([ + ['the legacy in-memory sentinel', '(null)'], + ['a stringified SQL NULL', 'null'], + ])('flags a driver that spells the empty bucket as %s', async (_label, sentinel) => { + const problems = await checkDateBucketParity( + brokenDriver({ + async aggregate(_object: string, query: any) { + const field = query.groupBy[0].field; + return [ + { [field]: '2024', n: 4 }, + { [field]: '2025', n: 3 }, + { [field]: sentinel, n: 1 }, + ]; + }, + supports: { queryDateGranularity: { year: true } }, + }) as never, + ); + expect(problems.join('\n')).toMatch(/pushed-down SQL and in-memory bucketing disagree/); + expect(problems.join('\n')).toContain(sentinel); + }); + it('stays quiet on a driver that advertises nothing', async () => { const problems = await checkDateBucketParity( brokenDriver({ supports: { queryDateGranularity: {} } }) as never, diff --git a/packages/qa/dogfood/test/empty-group-bucket-parity.test.ts b/packages/qa/dogfood/test/empty-group-bucket-parity.test.ts new file mode 100644 index 0000000000..f93e9f47e2 --- /dev/null +++ b/packages/qa/dogfood/test/empty-group-bucket-parity.test.ts @@ -0,0 +1,100 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Empty-group-bucket parity (#3839) — the NON-date half. +// +// `checkDateBucketParity` covers the seam for `dateGranularity` groupBy, but the +// divergence it was extended to catch was never date-specific: a plain +// `groupBy: ['stage']` over a NULL column diverged the same way, and nothing +// gates that. Same dataset, same query, one bucket key — pushed-down SQL said +// SQL NULL, the in-memory fallback said the string `'(null)'`, and +// `engine.aggregate` picks between the two paths per query (by driver, by +// advertised granularity, by reference timezone). So a dashboard's empty bucket +// changed TYPE when nothing about the data changed. +// +// The measures were never wrong — only the label's shape — which is why this +// survived so long: every total reconciled. +// +// This is deliberately the driver seam, not a unit test. `applyInMemoryAggregation` +// is unit-tested in objectql, but only a real driver can say what the SQL side +// actually returns for a NULL group column, and that is the half a unit test +// cannot speak for. + +import { describe, it, expect, 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, + }, +]; + +/** `[key, total]` pairs, sorted, with the key's runtime TYPE carried along. */ +function shape(rows: any[], field: string): Array<[string, unknown, number]> { + return rows + .map((r): [string, unknown, number] => [ + r[field] === null ? 'null' : typeof r[field], + r[field], + Number(r.total), + ]) + .sort((a, b) => String(a[1]).localeCompare(String(b[1]))); +} + +describe.each(DRIVERS)('empty group bucket parity: $name', ({ make }) => { + let driver: any; + + afterEach(async () => { + await driver?.disconnect?.(); + }); + + // Both groupBy shapes, because #3839 was originally filed as a date-bucketing + // bug and turned out to be neither caused by nor limited to date bucketing. + it.each([ + ['plain groupBy', ['stage'], 'stage'], + ['date-bucketed groupBy', [{ field: 'at', dateGranularity: 'month' }], 'at'], + ])('keys the empty bucket identically on both paths — %s', async (_label, groupBy, field) => { + driver = make(); + await driver.initObjects([ + { + name: TABLE, + fields: { at: { type: 'datetime' }, stage: { type: 'text' }, amount: { type: 'number' } }, + }, + ]); + const opts = { bypassTenantAudit: true }; + await driver.create(TABLE, { id: 'a', at: new Date('2026-01-10T09:00:00Z'), stage: 'won', amount: 1 }, opts); + // Two empty rows, so a bucket that silently split or merged shows up in the + // total rather than only in the key. + await driver.create(TABLE, { id: 'b', at: null, stage: null, amount: 1 }, opts); + await driver.create(TABLE, { id: 'c', at: null, stage: null, amount: 1 }, opts); + + const ast = { + object: TABLE, + groupBy, + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + }; + + const pushedDown = await driver.aggregate(TABLE, ast); + // The rows the in-memory path would see — the driver's own read output, which + // is exactly what `engine.aggregate` feeds the fallback. + const inMemory = applyInMemoryAggregation(await driver.find(TABLE, { object: TABLE }), ast as never); + + expect(shape(pushedDown, field)).toEqual(shape(inMemory, field)); + // …and both agree on real `null`, not on a sentinel they happen to share. + expect(pushedDown.find((r: any) => r.stage === null || r.at === null)?.total).toBe(2); + expect(inMemory.find((r: any) => r[field] === null)?.total).toBe(2); + expect(inMemory.some((r: any) => typeof r[field] === 'string' && /null/i.test(r[field]))).toBe(false); + }); +}); diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index 30b3bf36c5..8fdd01ae1d 100644 --- a/packages/services/service-analytics/src/analytics-service.ts +++ b/packages/services/service-analytics/src/analytics-service.ts @@ -637,7 +637,9 @@ export class AnalyticsService implements IAnalyticsService { (result as AnalyticsResultWithDrill).drillRanges = result.rows.map((row) => { const ranges: Record = {}; for (const { d, granularity, instant } of rangeDims) { - const cal = bucketKeyToCalendarRange(row[d.name] as string, granularity); + // A row in the empty bucket carries `null` here (#3839) and yields no + // range, so that row simply gets no drill bound — the superset. + const cal = bucketKeyToCalendarRange(row[d.name] as string | null, granularity); if (cal) { ranges[d.name] = { field: d.field as string, gte: bound(cal.start, instant), lt: bound(cal.end, instant) }; } diff --git a/packages/services/service-analytics/src/strategies/cross-object-rebucket.ts b/packages/services/service-analytics/src/strategies/cross-object-rebucket.ts index 249a3bd3bc..a982e0d2f9 100644 --- a/packages/services/service-analytics/src/strategies/cross-object-rebucket.ts +++ b/packages/services/service-analytics/src/strategies/cross-object-rebucket.ts @@ -121,7 +121,12 @@ export function rebucketCrossObject( // Bucket key = base dims (unchanged) + resolved attributes. `` is a // separator no group value contains, matching the engine's own convention. const keyParts: string[] = []; - for (const f of baseDimFields) keyParts.push(`${f}=${String(row[f] ?? '(null)')}`); + // JSON-encoded, so the empty bucket (`null` on both aggregation paths since + // #3839) stays distinct from a row whose value is the literal string + // `"null"` — plain interpolation renders both as `null` and would merge two + // real groups into one. Only this composite id is affected; the emitted + // bucket keeps the row's own value verbatim below. + for (const f of baseDimFields) keyParts.push(`${f}=${JSON.stringify(row[f] ?? null)}`); for (const cd of crossDims) keyParts.push(`${cd.outputName}=${String(resolved[cd.outputName])}`); const key = keyParts.join(''); diff --git a/packages/spec/src/data/driver.zod.ts b/packages/spec/src/data/driver.zod.ts index cead684cb7..6f36f141dd 100644 --- a/packages/spec/src/data/driver.zod.ts +++ b/packages/spec/src/data/driver.zod.ts @@ -184,6 +184,13 @@ export const DriverCapabilitiesSchema = lazySchema(() => z.object({ * `YYYY` / `YYYY-MM` / `YYYY-MM-DD` / `YYYY-Q[1-4]` / `YYYY-W[01-53]` (ISO-8601, * weeks start Monday). Any drift will misalign drill `groupKey` filters * between the two paths. + * + * The EMPTY bucket is part of that contract: a row with no instant MUST key as + * `null`, never a sentinel string (#3839). Propagating NULL through the bucket + * expression — what `strftime` / `date_trunc` / `to_char` already do — is the + * whole of it; a driver only breaks this by going out of its way to COALESCE. + * The same rule holds for a plain (non-date) `groupBy` column: a NULL value + * keys as `null`. `checkDateBucketParity` (`@objectstack/verify`) probes it. */ queryDateGranularity: z.record(DateGranularity, z.boolean()).optional() .describe('Per-granularity native date bucketing (day/week/month/quarter/year). Missing keys fall back to in-memory bucketing.'), diff --git a/packages/verify/src/date-bucket-parity.ts b/packages/verify/src/date-bucket-parity.ts index ae94001e44..3da36fe231 100644 --- a/packages/verify/src/date-bucket-parity.ts +++ b/packages/verify/src/date-bucket-parity.ts @@ -13,6 +13,11 @@ * `applyInMemoryAggregation`, and a non-UTC timezone forces the in-memory path * regardless. A dashboard can cross that seam mid-drill-down. * + * "Same labels" includes the EMPTY bucket: a row with no instant must key the + * same way on both sides (#3839). It is the one label neither side computes — + * each just propagates its own idea of nothing — which is exactly why it drifted + * apart for so long without a test to notice. + * * This is the invariant #3773 broke and nothing caught. SQLite stores a * `Field.datetime` as INTEGER epoch milliseconds; `strftime` read the bare * integer as a Julian day number, so every row bucketed as NULL and a trend @@ -72,16 +77,19 @@ const FIELDS = { /** * Instants chosen to straddle every boundary the labels encode: year, quarter, - * month, ISO week (2024-12-30 is 2025-W01), and midnight in both directions. + * month, ISO week (2024-12-30 is 2025-W01), and midnight in both directions — + * plus, at the end, a NULL instant. * - * Deliberately NO null instant. A NULL group key is a known, pre-existing - * divergence — SQL yields SQL NULL where the in-memory path yields the literal - * `'(null)'` — that is orthogonal to bucketing correctness and equally true of a - * TEXT column. Including it would make this check fail for a reason it is not - * about; it is called out here so its absence reads as a decision, not an - * oversight. + * That last row is here deliberately (#3839), and it used to be deliberately + * absent: the two paths disagreed about how to SPELL "empty" — SQL NULL against + * the in-memory literal `'(null)'` — so including it would have failed this + * check for a reason it is not about. That divergence is closed; both sides now + * key the empty bucket as real `null`. The row belongs here precisely because + * the convergence is the kind that decays silently: nothing else compares the + * two paths on a null input, and either side could drift back to a sentinel + * without a single existing test noticing. */ -const FIXTURE: ReadonlyArray<{ id: string; iso: string }> = [ +const FIXTURE: ReadonlyArray<{ id: string; iso: string | null }> = [ { id: 'b1', iso: '2024-01-15T10:00:00.000Z' }, // 2024 / Q1 / Jan / W03 { id: 'b2', iso: '2024-06-30T23:59:59.000Z' }, // last instant of Q2 { id: 'b3', iso: '2024-07-01T00:00:00.000Z' }, // first instant of Q3 @@ -89,17 +97,48 @@ const FIXTURE: ReadonlyArray<{ id: string; iso: string }> = [ { id: 'b5', iso: '2025-01-01T00:00:00.000Z' }, // exact midnight, year boundary { id: 'b6', iso: '2025-05-19T09:00:00.000Z' }, { id: 'b7', iso: '2025-05-19T22:30:00.000Z' }, // same day bucket as b6 + { id: 'b8', iso: null }, // the empty bucket — both paths must key it `null` ]; -/** `{label: count}` from aggregate rows keyed by `field`. */ +/** + * The empty bucket's label, chosen so no `String(value)` can produce it. That is + * what makes a sentinel visible: a side spelling "empty" as the STRING + * `'(null)'`, or as `'null'` from stringifying SQL NULL, lands under its own + * literal and reads as a different label than the other side's real `null`. + * Under a plain `String()` the two would have compared equal in one of those + * cases and this check would have blessed the divergence. + */ +const EMPTY_LABEL = '‹empty bucket›'; + +/** + * `{label: count}` from aggregate rows keyed by `field`. + * + * A missing property counts as empty alongside `null`: a driver that omits the + * key for a null group is describing the same bucket, and JSON round-tripping + * turns one into the other anyway. + */ function labelCounts(rows: any[], field: string): Record { const out: Record = {}; for (const row of rows ?? []) { - out[String(row?.[field])] = Number(row?.n ?? 0); + const v = row?.[field]; + out[v == null ? EMPTY_LABEL : String(v)] = Number(row?.n ?? 0); } return out; } +/** + * Order-insensitive canonical form. Row ORDER is not part of this contract — + * `engine.aggregate` promises no ordering without an explicit `orderBy`, and the + * two paths naturally differ (SQL sorts its groups, the in-memory path emits + * first-seen order). Comparing the raw objects with `JSON.stringify` made key + * INSERTION order significant, so a driver whose buckets were entirely correct + * but ordered differently was reported as a disagreement — with an empty diff + * message, because {@link describeDiff} is keyed and could not name one. + */ +function canonical(counts: Record): string { + return JSON.stringify(Object.keys(counts).sort().map((k) => [k, counts[k]])); +} + function describeDiff(a: Record, b: Record): string { const keys = [...new Set([...Object.keys(a), ...Object.keys(b)])].sort(); return keys @@ -128,10 +167,12 @@ export async function checkDateBucketParity( await driver.syncSchema(object, { name: object, fields: FIELDS }); for (const { id, iso } of FIXTURE) { // A real `Date` for the datetime column — the shape every normal write - // takes, and the one that produces epoch storage on SQLite. + // takes, and the one that produces epoch storage on SQLite. A null instant + // is written as NULL to both columns, so the empty bucket is reached the + // way a real record reaches it: an unset field, not a magic value. await driver.create( object, - { id, at: new Date(iso), on: iso.slice(0, 10), n: 1 }, + { id, at: iso === null ? null : new Date(iso), on: iso === null ? null : iso.slice(0, 10), n: 1 }, opts.createOptions, ); } @@ -175,7 +216,7 @@ export async function checkDateBucketParity( const sql = labelCounts(pushedDown, field); const inMemory = labelCounts(applyInMemoryAggregation(rows, ast as never), field); - if (JSON.stringify(sql) !== JSON.stringify(inMemory)) { + if (canonical(sql) !== canonical(inMemory)) { problems.push( `${storage} '${field}' @ ${granularity}: pushed-down SQL and in-memory bucketing disagree — ${describeDiff(sql, inMemory)}`, ); @@ -198,7 +239,7 @@ export async function checkDateBucketParity( const [byAt, byOn] = await Promise.all([mk('at'), mk('on')]); const a = labelCounts(byAt, 'at'); const b = labelCounts(byOn, 'on'); - if (JSON.stringify(a) !== JSON.stringify(b)) { + if (canonical(a) !== canonical(b)) { problems.push( `@ ${granularity}: the datetime and date columns describe the same days but bucket differently — ${describeDiff(a, b)}`, );