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)}`, );