From ea66911eff7a056edbb7157d5997a5f65f8fc943 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Tue, 28 Jul 2026 12:05:17 +0800 Subject: [PATCH] fix(driver-sql): bucket a SQLite `Field.datetime` by its stored instant (#3773) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On SQLite every trend chart bucketed by day/week/month/year over a `Field.datetime` column put every record in a single `(null)` bucket — one bar carrying the whole total. The measure was right; only the bucket key was wrong. better-sqlite3 stores a `Field.datetime` as INTEGER epoch milliseconds, and `buildDateBucketExpr` emitted a flat `strftime('%Y-%m', col)`. SQLite reads a bare integer as a Julian day number, and epoch ms is far outside the legal range, so `strftime` returned NULL for every row. Nothing downstream noticed: SQLite advertises `queryDateGranularity.month`, so `engine.aggregate` pushes the bucketing down, and its in-memory fallback only engages for an unsupported granularity or a non-UTC timezone. The SQLite expression is now storage-aware, sharing one `isEpochStoredDatetime` predicate with the filter-comparand coercion added for the same root cause in #2034 — a window and a bucket that disagree about storage is exactly how an epoch column ended up correctly filtered and then entirely bucketed as NULL. Postgres and MySQL are untouched and pinned as such: `defineColumn` maps `Field.datetime` to a native timestamp there. Two details are load-bearing and each has a test that fails without it: - The conversion dispatches on each stored value's type, not just the declared one. A SQLite `Field.datetime` column is genuinely mixed-form — `formatInput` passes datetime values through, so a `Date` lands as INTEGER while an ISO string lands as TEXT. Dividing TEXT by 1000 coerces it to its leading year, filing live rows under 1970 — worse than the NULL it replaces. - Division is `/1000.0`, not `/1000`: integer division truncates toward zero, so a pre-1970 instant would surface a day late. `bucketDateValue` (the in-memory fallback) now reads a finite number as epoch ms. `new Date(String(1767225600000))` is an Invalid Date, so fixing only the driver would have traded one wrong answer for two different ones — the two paths have to label the same instant identically for a drill-down to survive crossing them. Coverage goes through `initObjects` rather than `knex.schema.createTable`, which is why the existing date-bucket suite never saw this: its fixture is ISO TEXT, the half `strftime` parses natively. Four granularities x both storage forms, plus a mixed-form column, a pre-1970 instant, and dialect gating for pg/mysql. `SqliteWasmDriver` inherits the expression, so it carried the bug and is pinned too. Co-Authored-By: Claude --- .changeset/sqlite-datetime-date-bucket.md | 49 ++++ .../src/in-memory-aggregation.test.ts | 23 ++ .../objectql/src/in-memory-aggregation.ts | 14 +- ...l-driver-aggregate-datetime-window.test.ts | 29 ++- .../sql-driver-date-bucket-storage.test.ts | 217 ++++++++++++++++++ .../src/sql-driver-date-bucket.test.ts | 4 +- .../src/sql-driver-temporal-dialect.test.ts | 84 +++++++ packages/plugins/driver-sql/src/sql-driver.ts | 88 ++++++- .../sqlite-wasm-driver-date-bucket.test.ts | 40 +++- 9 files changed, 522 insertions(+), 26 deletions(-) create mode 100644 .changeset/sqlite-datetime-date-bucket.md create mode 100644 packages/plugins/driver-sql/src/sql-driver-date-bucket-storage.test.ts diff --git a/.changeset/sqlite-datetime-date-bucket.md b/.changeset/sqlite-datetime-date-bucket.md new file mode 100644 index 0000000000..e294478005 --- /dev/null +++ b/.changeset/sqlite-datetime-date-bucket.md @@ -0,0 +1,49 @@ +--- +"@objectstack/driver-sql": patch +"@objectstack/objectql": patch +--- + +fix(driver-sql): bucket a SQLite `Field.datetime` by its stored instant instead of collapsing every row into one `(null)` (#3773) + +On SQLite, any trend chart bucketed by day/week/month/year over a +`Field.datetime` column put **every record in a single `(null)` bucket** — one +bar, carrying the whole total. The measure was right; only the bucket key was +wrong. `Field.date` (ISO TEXT storage) was unaffected, so the same dashboard +could show one column working and the next one flat. + +better-sqlite3 stores a `Field.datetime` as INTEGER epoch **milliseconds** (knex +binds a JS `Date` as `.getTime()`), and `buildDateBucketExpr` emitted a flat +`strftime('%Y-%m', col)`. SQLite reads a bare integer as a **Julian day +number**; an epoch-ms value is far outside the legal range, so `strftime` +returned NULL for every row. Nothing downstream noticed: SQLite advertises +`queryDateGranularity.month`, so `engine.aggregate` pushes the bucketing down, +and its in-memory fallback only engages for an *unsupported* granularity or a +non-UTC timezone. + +The SQLite expression is now storage-aware, sharing one `isEpochStoredDatetime` +predicate with the filter-comparand coercion added for the same root cause in +\#2034 — a window and a bucket that disagree about storage is exactly how an +epoch column ended up correctly filtered and then entirely bucketed as NULL. +Postgres and MySQL are untouched: `defineColumn` maps `Field.datetime` to a +native timestamp there, which is also why their comparands are left alone. + +Two details are load-bearing and pinned by tests: + +- The conversion dispatches on each **stored value's** type, not just the + declared one. A SQLite `Field.datetime` column is genuinely mixed-form — + `formatInput` passes datetime values through, so a `Date` lands as INTEGER + while an ISO string (including an unresolved `defaultValue: 'NOW()'`) lands as + TEXT. Dividing TEXT by 1000 coerces it to its leading year, filing live rows + under 1970 — worse than the NULL it replaced. +- Division is `/1000.0`, not `/1000`. Integer division truncates toward zero, so + a pre-1970 instant (`-1` ms) would surface as 1970-01-01. + +`bucketDateValue` (the in-memory fallback in `@objectstack/objectql`) now reads a +finite **number** as epoch milliseconds. `new Date(String(1767225600000))` is an +Invalid Date, so a driver handing back raw storage values bucketed as `'(null)'` +there while the pushed-down SQL bucketed correctly — fixing only the driver would +have traded one wrong answer for two different ones, and the two paths have to +label the same instant identically for a drill-down to survive crossing them. + +`SqliteWasmDriver` inherits `buildDateBucketExpr`, so it carried the bug and gets +the fix. diff --git a/packages/objectql/src/in-memory-aggregation.test.ts b/packages/objectql/src/in-memory-aggregation.test.ts index 42fccf8cb6..4a543a553a 100644 --- a/packages/objectql/src/in-memory-aggregation.test.ts +++ b/packages/objectql/src/in-memory-aggregation.test.ts @@ -106,6 +106,29 @@ describe('bucketDateValue', () => { expect(bucketDateValue('not-a-date', 'month')).toBe('(null)'); }); + // #3773 — parity with the pushed-down SQL. SQLite stores a `Field.datetime` + // as epoch milliseconds, so a driver that hands back raw storage values feeds + // this a NUMBER. `new Date(String(1767225600000))` is an Invalid Date, so + // these all bucketed as '(null)' while the native SQL bucketed them correctly + // — the two paths have to label the same instant identically. + it('reads a finite number as epoch milliseconds', () => { + const ms = Date.parse('2026-01-10T09:00:00Z'); + expect(bucketDateValue(ms, 'year')).toBe('2026'); + expect(bucketDateValue(ms, 'quarter')).toBe('2026-Q1'); + expect(bucketDateValue(ms, 'month')).toBe('2026-01'); + expect(bucketDateValue(ms, 'day')).toBe('2026-01-10'); + // Same instant, all three shapes a driver might return. + for (const g of ['year', 'quarter', 'month', 'day'] as const) { + expect(bucketDateValue(ms, g)).toBe(bucketDateValue(new Date(ms), g)); + expect(bucketDateValue(ms, g)).toBe(bucketDateValue(new Date(ms).toISOString(), g)); + } + }); + + it('reads a negative epoch as a pre-1970 instant', () => { + expect(bucketDateValue(-1, 'day')).toBe('1969-12-31'); + expect(bucketDateValue(0, 'day')).toBe('1970-01-01'); + }); + // ADR-0053 Phase 2 (D2): a non-UTC reference timezone shifts the calendar day. describe('timezone-aware bucketing', () => { // 2024-03-01T03:00Z is still 2024-02-29 (22:00) in America/New_York. diff --git a/packages/objectql/src/in-memory-aggregation.ts b/packages/objectql/src/in-memory-aggregation.ts index 4e98d0269b..20c4e5ba9c 100644 --- a/packages/objectql/src/in-memory-aggregation.ts +++ b/packages/objectql/src/in-memory-aggregation.ts @@ -181,6 +181,13 @@ function toNumber(v: any): number { * The y/m/d are taken in the reference zone and the ISO-week math then runs on * a UTC date built from those parts — the parts already carry the zone shift, * so the week boundary lands correctly without re-applying any offset. + * + * 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. */ export function bucketDateValue( value: unknown, @@ -188,7 +195,12 @@ export function bucketDateValue( timezone?: string, ): string { if (value == null) return '(null)'; - const d = value instanceof Date ? value : new Date(String(value)); + const d = + value instanceof Date + ? value + : typeof value === 'number' + ? new Date(value) + : new Date(String(value)); if (Number.isNaN(d.getTime())) return '(null)'; const { year: y, month: m, day } = calendarPartsInTzOrUtc(d, timezone); switch (granularity) { diff --git a/packages/plugins/driver-sql/src/sql-driver-aggregate-datetime-window.test.ts b/packages/plugins/driver-sql/src/sql-driver-aggregate-datetime-window.test.ts index ab1abfdc9e..b5b8eb9781 100644 --- a/packages/plugins/driver-sql/src/sql-driver-aggregate-datetime-window.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-aggregate-datetime-window.test.ts @@ -100,29 +100,26 @@ describe('SqlDriver.aggregate — ISO window over epoch-stored datetime (#3650)' expect(byMonth).toEqual({ '2026-01': 300, '2026-02': 30 }); }); - it('KNOWN GAP — bucketing an EPOCH-stored datetime collapses into one null bucket', async () => { - // Pre-existing, unrelated to #3650, and deliberately NOT fixed here — but - // it lands on the exact same query shape, so it is pinned rather than left - // to be rediscovered as "the dateRange fix did nothing". - // - // SQLite advertises `queryDateGranularity.month`, so `engine.aggregate` - // pushes the bucketing down to the driver — `engine.ts` only falls back to - // in-memory bucketing when a granularity is UNSUPPORTED or a non-UTC - // timezone is in play, neither of which applies here. The dialect - // expression is `strftime('%Y-%m', col)`, and SQLite reads a bare INTEGER - // as a Julian day number; an epoch-ms value is far outside the legal range, - // so every row buckets as NULL. + it('buckets an EPOCH-stored datetime inside the window (was one null bucket)', async () => { + // This assertion was pinned as a KNOWN GAP by #3650 and is the acceptance + // gate of the follow-up fix (#3773): SQLite advertises + // `queryDateGranularity.month`, so `engine.aggregate` pushes the bucketing + // down to the driver — `engine.ts` only falls back to in-memory bucketing + // when a granularity is UNSUPPORTED or a non-UTC timezone is in play, + // neither of which applies here. The dialect expression used to be a flat + // `strftime('%Y-%m', col)`, and SQLite reads a bare INTEGER as a Julian day + // number; an epoch-ms value is far outside the legal range, so every row + // bucketed as NULL and the whole trend chart collapsed to one bar. const rows = await driver.aggregate(TABLE, { groupBy: [{ field: 'closed_at', dateGranularity: 'month' }], aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], where: window('closed_at'), } as any); - // The WINDOW works — the total is the in-window 330, not the full 1930 — - // which is what #3650 is responsible for. The BUCKETS are what is broken. - // When that is fixed, this becomes `{ '2026-01': 300, '2026-02': 30 }`. + // Two things have to hold at once: the WINDOW (#3650 — the total is the + // in-window 330, not the full 1930) and the BUCKETS (#3773). const byMonth = Object.fromEntries(rows.map((r: any) => [String(r.closed_at), Number(r.total)])); - expect(byMonth).toEqual({ null: 330 }); + expect(byMonth).toEqual({ '2026-01': 300, '2026-02': 30 }); }); it('confines a date (TEXT-stored) aggregate to the same window', async () => { 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 new file mode 100644 index 0000000000..5b8501d9bf --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-date-bucket-storage.test.ts @@ -0,0 +1,217 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Date bucketing across the two SQLite storage forms (#3773). + * + * `sql-driver-date-bucket.test.ts` builds its fixture with + * `knex.schema.createTable` + `t.string('ts')`, so every value it buckets is ISO + * TEXT. That is only half of what SQLite actually holds: a `Field.datetime` + * declared through `initObjects` becomes INTEGER epoch **milliseconds** (knex + * binds a JS `Date` as `.getTime()`), and `strftime` reads a bare integer as a + * Julian day number — epoch ms is orders of magnitude outside the legal range, + * so every row bucketed as NULL and any datetime trend chart rendered as one + * `(null)` bar carrying the whole total. + * + * So this suite goes through `driver.initObjects([...])` — the path a real + * object takes — and sweeps every supported granularity against BOTH storage + * forms, asserting against the same `bucketDateValue` labels the in-memory + * fallback produces. Anything that buckets differently depending on how the + * column happens to be stored is the bug this file exists to catch. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +type Granularity = 'day' | 'month' | 'quarter' | 'year'; + +/** Every granularity SQLite advertises natively (week is bucketed in-memory). */ +const GRANULARITIES: Granularity[] = ['day', 'month', 'quarter', 'year']; + +/** ⚠️ Keep in sync with `packages/objectql/src/in-memory-aggregation.ts#bucketDateValue` */ +function bucketDateValue(value: unknown, g: Granularity): string { + 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)'; + const y = d.getUTCFullYear(); + const m = d.getUTCMonth() + 1; + switch (g) { + case 'year': return String(y); + case 'quarter': return `${y}-Q${Math.floor((m - 1) / 3) + 1}`; + case 'month': return `${y}-${String(m).padStart(2, '0')}`; + case 'day': return `${y}-${String(m).padStart(2, '0')}-${String(d.getUTCDate()).padStart(2, '0')}`; + } +} + +const TABLE = 'deal'; + +/** + * One UTC instant per row, with its `Field.date` twin on the same calendar day + * so the two columns MUST produce identical labels at every granularity — the + * whole point being that storage form may not change the answer. + * + * Amounts are distinct powers of two: a bucket's sum names exactly which rows + * landed in it, so a mis-bucketing can't hide behind a coincidental total. + */ +const FIXTURE: Array<{ id: string; iso: string; amount: number }> = [ + { id: 'r1', iso: '1969-12-31T23:59:59.999Z', amount: 1 }, // pre-epoch, 1ms before 1970 + { id: 'r2', iso: '2025-11-15T09:00:00.000Z', amount: 2 }, + { id: 'r3', iso: '2026-01-10T09:00:00.000Z', amount: 4 }, + { id: 'r4', iso: '2026-01-20T23:59:59.000Z', amount: 8 }, // same month as r3 + { id: 'r5', iso: '2026-02-14T00:00:00.000Z', amount: 16 }, // exact midnight + { id: 'r6', iso: '2026-06-30T23:59:59.000Z', amount: 32 }, // last instant of Q2 + { id: 'r7', iso: '2026-07-01T00:00:00.000Z', amount: 64 }, // first instant of Q3 +]; + +/** 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); + out[key] = (out[key] ?? 0) + row.amount; + } + return out; +} + +async function bucketSums(driver: SqlDriver, field: string, g: Granularity) { + const rows = await driver.aggregate(TABLE, { + 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)])); +} + +describe('SqlDriver date bucketing is storage-form independent (#3773)', () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + + await driver.initObjects([ + { + name: TABLE, + fields: { + closed_at: { type: 'datetime' }, // INTEGER epoch ms under better-sqlite3 + closed_on: { type: 'date' }, // YYYY-MM-DD TEXT + amount: { type: 'number' }, + }, + }, + ]); + + for (const { id, iso, amount } of FIXTURE) { + await driver.create( + TABLE, + // A real `Date` for the datetime column — the path the seed loader and + // every normal write take, and the one that produces epoch storage. + { id, closed_at: new Date(iso), closed_on: iso.slice(0, 10), amount }, + { bypassTenantAudit: true }, + ); + } + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + it('really does store the two columns in the two different forms', async () => { + // The premise of this whole file. If better-sqlite3 ever stops binding a + // `Date` as an integer, this fails first and explains the rest. + const res: any = await driver.execute( + `SELECT typeof("closed_at") AS at_t, typeof("closed_on") AS on_t FROM "${TABLE}" WHERE id = 'r3'`, + ); + const row = Array.isArray(res) ? res[0] : (res?.rows?.[0] ?? res); + expect(['integer', 'real']).toContain(row.at_t); + expect(row.on_t).toBe('text'); + }); + + for (const g of GRANULARITIES) { + describe(`granularity '${g}'`, () => { + it('buckets the epoch-stored datetime column', async () => { + expect(await bucketSums(driver, 'closed_at', g)).toEqual(expectedBuckets(g)); + }); + + it('buckets the TEXT-stored date column', async () => { + expect(await bucketSums(driver, 'closed_on', g)).toEqual(expectedBuckets(g)); + }); + + it('gives both columns the same labels', async () => { + const [byAt, byOn] = await Promise.all([ + bucketSums(driver, 'closed_at', g), + bucketSums(driver, 'closed_on', g), + ]); + expect(Object.keys(byAt).sort()).toEqual(Object.keys(byOn).sort()); + }); + }); + } + + it('keeps a pre-1970 instant on its own calendar day', async () => { + // Guards the `/1000.0` in the bucket expression. Integer division truncates + // toward zero, so `-1 / 1000` is 0 and this row would surface as 1970-01-01 + // — a full day, year and quarter wrong, and only for negative epochs. + const byDay = await bucketSums(driver, 'closed_at', 'day'); + expect(byDay['1969-12-31']).toBe(1); + expect(byDay['1970-01-01']).toBeUndefined(); + }); +}); + +describe('SqlDriver date bucketing over a MIXED-form datetime column (#3773)', () => { + // One SQLite `Field.datetime` column legitimately holds both forms at once: + // `formatInput` leaves datetime values alone, so a `Date` lands as INTEGER + // epoch ms while an ISO string (what an unresolved `defaultValue: 'NOW()'` + // slot and any string-valued write produce) lands as TEXT. A bucket + // expression that assumed epoch for the whole column would divide the TEXT by + // 1000 — `'2026-01-10T…' / 1000.0` is 2.026 seconds past the epoch — and file + // live rows under 1970, which is worse than the NULL it replaced. + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await driver.initObjects([ + { name: TABLE, fields: { closed_at: { type: 'datetime' }, amount: { type: 'number' } } }, + ]); + await driver.create(TABLE, { id: 'int', closed_at: new Date('2026-01-10T09:00:00Z'), amount: 1 }, { bypassTenantAudit: true }); + await driver.create(TABLE, { id: 'txt', closed_at: '2026-02-14T09:00:00Z', amount: 2 }, { bypassTenantAudit: true }); + await driver.create(TABLE, { id: 'naive', closed_at: '2026-02-20 09:00:00', amount: 4 }, { bypassTenantAudit: true }); + await driver.create(TABLE, { id: 'nil', closed_at: null, amount: 8 }, { bypassTenantAudit: true }); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + it('stores the fixture in both forms', async () => { + const res: any = await driver.execute( + `SELECT id, typeof("closed_at") AS t FROM "${TABLE}" ORDER BY id`, + ); + const rows = Array.isArray(res) ? res : (res?.rows ?? []); + const byId = Object.fromEntries(rows.map((r: any) => [r.id, r.t])); + expect(['integer', 'real']).toContain(byId.int); + expect(byId.txt).toBe('text'); + expect(byId.naive).toBe('text'); + expect(byId.nil).toBe('null'); + }); + + it('buckets each row by its own stored form', async () => { + const byMonth = await bucketSums(driver, 'closed_at', 'month'); + expect(byMonth['2026-01']).toBe(1); // INTEGER epoch ms + expect(byMonth['2026-02']).toBe(6); // ISO TEXT (2) + zone-naive TEXT (4) + expect(byMonth['1970-01']).toBeUndefined(); // TEXT never divided by 1000 + }); + + 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); + }); +}); 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 87726ed674..2e85221bab 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 @@ -20,7 +20,9 @@ type Granularity = 'day' | 'week' | 'month' | 'quarter' | 'year'; /** ⚠️ Keep in sync with `packages/objectql/src/in-memory-aggregation.ts#bucketDateValue` */ function bucketDateValue(value: unknown, g: Granularity): string { if (value == null) return '(null)'; - const d = value instanceof Date ? value : new Date(String(value)); + // 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)'; const y = d.getUTCFullYear(); const m = d.getUTCMonth() + 1; diff --git a/packages/plugins/driver-sql/src/sql-driver-temporal-dialect.test.ts b/packages/plugins/driver-sql/src/sql-driver-temporal-dialect.test.ts index bc5811c97e..1215eb23a6 100644 --- a/packages/plugins/driver-sql/src/sql-driver-temporal-dialect.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-temporal-dialect.test.ts @@ -67,3 +67,87 @@ describe('temporalFilterValue dialect gating', () => { } }); }); + +/** + * The same dialect gating for the aggregate BUCKET expression (#3773). It shares + * `isEpochStoredDatetime` with the filter coercion above, so the two can only be + * wrong together — which is the point: a window and a bucket that disagree about + * storage is how the epoch column ended up correctly filtered and then entirely + * bucketed as NULL. + * + * Postgres and MySQL need no normalization because `defineColumn` maps + * `Field.datetime` to a native timestamp there (`table.timestamp`), which is + * also why `temporalFilterValue` leaves their comparands alone. If a column ever + * WERE an integer on those dialects (an external table declaring `datetime` over + * a `bigint`), Postgres refuses the `::timestamptz` cast outright rather than + * bucketing silently — the loud failure SQLite did not give us. + */ +describe('buildDateBucketExpr dialect gating (#3773)', () => { + const GRANULARITIES = ['day', 'month', 'quarter', 'year'] as const; + const expr = (d: ProbeDriver, field: string, g: string, table?: string) => + (d as any).buildDateBucketExpr(field, g, table) as { sql: string; bindings: any[] } | null; + + it('SQLite: a declared Field.datetime is normalised from epoch ms', () => { + const d = makeDriver('better-sqlite3'); + d.seedDatetime('t', 'at'); + for (const g of GRANULARITIES) { + const e = expr(d, 'at', g, 't')!; + expect(e.sql).toContain(`julianday(??/1000.0, 'unixepoch')`); + // Real division, not integer: `-1 / 1000` truncates to 0 and moves a + // pre-1970 instant forward a day. + expect(e.sql).not.toContain('/1000,'); + } + }); + + it('SQLite: Field.date and undeclared columns keep the plain column form', () => { + const d = makeDriver('better-sqlite3'); + d.seedDate('t', 'on'); + for (const g of GRANULARITIES) { + expect(expr(d, 'on', g, 't')!.sql).not.toContain('julianday'); + expect(expr(d, 'anything', g, 't')!.sql).not.toContain('julianday'); + // No table key at all (a caller outside the aggregate path) → plain form. + expect(expr(d, 'at', g)!.sql).not.toContain('julianday'); + } + }); + + it('Postgres keeps the native timestamptz cast even for a declared datetime', () => { + const d = makeDriver('pg'); + d.seedDatetime('t', 'at'); + for (const g of GRANULARITIES) { + const e = expr(d, 'at', g, 't')!; + expect(e.sql).toContain(`(??)::timestamptz`); + expect(e.sql).not.toContain('unixepoch'); + expect(e.sql).not.toContain('/1000'); + } + }); + + it('MySQL keeps convert_tz even for a declared datetime', () => { + const d = makeDriver('mysql2'); + d.seedDatetime('t', 'at'); + for (const g of GRANULARITIES) { + const e = expr(d, 'at', g, 't')!; + expect(e.sql).toContain('convert_tz(??'); + expect(e.sql).not.toContain('unixepoch'); + expect(e.sql).not.toContain('/1000'); + } + }); + + it('every emitted expression binds exactly as many identifiers as it references', () => { + // The quarter expression references the column twice; an epoch-normalised + // one references it six times. A mismatch here is knex silently shifting + // bindings into the wrong slots. + for (const client of ['better-sqlite3', 'pg', 'mysql2']) { + const d = makeDriver(client); + d.seedDatetime('t', 'at'); + d.seedDate('t', 'on'); + for (const field of ['at', 'on']) { + for (const g of GRANULARITIES) { + const e = expr(d, field, g, 't'); + if (!e) continue; + expect(e.bindings.length).toBe((e.sql.match(/\?\?/g) ?? []).length); + expect(new Set(e.bindings)).toEqual(new Set([field])); + } + } + } + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index f0c6674b14..4a463014d9 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -496,10 +496,16 @@ export class SqlDriver implements IDataDriver { * Exposed as `{sql, bindings}` (not `Knex.Raw`) so callers can both * `groupByRaw()` and embed the same expression inside a `select() as alias` * with correctly forwarded identifier bindings. + * + * `table` is the coercion key of the object being aggregated (what + * {@link coercionKey} returns for the builder). It is what makes the SQLite + * expression storage-aware — see {@link sqliteTemporalArg}. Omitting it yields + * the plain column form, which is correct for any TEXT-stored column. */ protected buildDateBucketExpr( field: string, granularity: 'day' | 'week' | 'month' | 'quarter' | 'year', + table?: string | null, ): { sql: string; bindings: any[] } | null { if (!this.dateGranularityCapabilities[granularity]) return null; @@ -524,11 +530,15 @@ export class SqlDriver implements IDataDriver { } if (this.isSqlite) { + // `arg` is a bare `??` for a TEXT-stored column and an epoch→julian-day + // normalization for a `Field.datetime` one (#3773). + const { sql: arg, bindings: argBindings } = this.sqliteTemporalArg(field, table); + const fmt = (f: string) => ({ sql: `strftime('${f}', ${arg})`, bindings: [...argBindings] }); switch (granularity) { - case 'year': return { sql: `strftime('%Y', ??)`, bindings: [field] }; - case 'month': return { sql: `strftime('%Y-%m', ??)`, bindings: [field] }; - case 'day': return { sql: `strftime('%Y-%m-%d', ??)`, bindings: [field] }; - case 'quarter': return { sql: `(strftime('%Y', ??) || '-Q' || ((cast(strftime('%m', ??) as integer) - 1) / 3 + 1))`, bindings: [field, field] }; + case 'year': return fmt('%Y'); + case 'month': return fmt('%Y-%m'); + case 'day': return fmt('%Y-%m-%d'); + case 'quarter': return { sql: `(strftime('%Y', ${arg}) || '-Q' || ((cast(strftime('%m', ${arg}) as integer) - 1) / 3 + 1))`, bindings: [...argBindings, ...argBindings] }; case 'week': return null; // see capabilities note } } @@ -1579,13 +1589,19 @@ export class SqlDriver implements IDataDriver { // ({ field: 'closed_at', dateGranularity: 'quarter' }). For structured // items we emit a dialect-specific bucket expression aliased as the // field name so the resulting row keys match in-memory bucketDateValue. + // + // The bucket expression needs the same coercion key `applyFilters` just + // used above, so the WHERE window and the GROUP BY buckets agree on how + // the column is stored — disagreeing is exactly how #3773 produced an + // in-window total spread over a single `(null)` bucket. + const bucketTable = this.coercionKey(builder); for (const g of query.groupBy as Array) { if (typeof g === 'string') { builder.groupBy(g); builder.select(g); } else if (g && typeof g === 'object' && g.field) { if (g.dateGranularity) { - const bucket = this.buildDateBucketExpr(g.field, g.dateGranularity as any); + const bucket = this.buildDateBucketExpr(g.field, g.dateGranularity as any, bucketTable); if (!bucket) { throw new Error( `SqlDriver: dateGranularity '${g.dateGranularity}' not supported on dialect ` + @@ -3332,8 +3348,9 @@ export class SqlDriver implements IDataDriver { // that never matches. Postgres/MySQL map datetime to a native TIMESTAMP // (see `defineColumn` → `table.timestamp`), where Knex binds an ISO string // or `Date` correctly — coercing to an epoch integer there would compare an - // INTEGER against a TIMESTAMP and break the query. So gate on dialect. - if (!this.isSqlite) return value; + // INTEGER against a TIMESTAMP and break the query. So gate on the storage + // form, via the predicate the bucket expression reads too. + if (!this.isEpochStoredDatetime(table, field)) return value; const ms = toMs(value); return ms == null ? value : ms; } @@ -3342,6 +3359,63 @@ export class SqlDriver implements IDataDriver { return this.toDateOnly(value); } + /** + * Does this column store instants as epoch **milliseconds** rather than as a + * temporal type SQL understands natively? + * + * True for a SQLite `Field.datetime` and nothing else: better-sqlite3 binds a + * JS `Date` as `.getTime()`, while Postgres/MySQL get a real TIMESTAMP column + * (`defineColumn` → `table.timestamp`) and `Field.date` is ISO TEXT on every + * dialect. + * + * This is the ONE predicate for that storage convention. Both consumers of it + * have already been bitten by disagreeing about storage — the filter comparand + * (#2034: an ISO string compared against an INTEGER column matched nothing) + * and the aggregate bucket expression (#3773: `strftime` read the same INTEGER + * as a Julian day and bucketed everything as NULL) — so they share it rather + * than each carrying their own copy of the rule. + */ + protected isEpochStoredDatetime(table: string | null | undefined, field: string): boolean { + if (!table || !this.isSqlite) return false; + return this.datetimeFields[table]?.has(field) === true; + } + + /** + * The value expression to hand SQLite's `strftime()` for a bucketed column. + * + * A TEXT-stored column is passed straight through — `strftime` already parses + * `YYYY-MM-DD`, `YYYY-MM-DDTHH:MM:SSZ` and the zone-naive `CURRENT_TIMESTAMP` + * form. An epoch-stored `Field.datetime` (see {@link isEpochStoredDatetime}) + * must first be converted, or `strftime` reads the bare integer as a Julian + * day number — epoch ms is far outside the legal range, so every row buckets + * as NULL and a trend chart collapses into a single `(null)` bar. + * + * The conversion dispatches on the STORED value's type, not just the declared + * one, because a SQLite `Field.datetime` column is genuinely mixed-form: an + * explicit value bound as a JS `Date` lands as INTEGER/REAL epoch ms, while a + * `defaultValue: 'NOW()'` slot lands as TEXT (the same mix `formatOutput` → + * `normalizeSqliteDatetimeOutput` already repairs on read). Dividing a TEXT + * timestamp by 1000 would coerce it to its leading year — `'2026-01-10T…'/1000.0` + * is `2.026` seconds past the epoch, bucketing real rows into 1970 — which is + * strictly worse than the NULL it replaces, so the CASE is load-bearing. + * + * Normalising to a julian day (rather than emitting `strftime(fmt, x/1000.0, + * 'unixepoch')`) keeps ONE reusable scalar the caller can drop into any format + * string, including the two-reference quarter expression. `/1000.0` is real + * division on purpose: integer `/1000` truncates toward zero, which pushes a + * pre-1970 instant forward a day (`-1` → 1970-01-01 instead of 1969-12-31). + */ + protected sqliteTemporalArg( + field: string, + table: string | null | undefined, + ): { sql: string; bindings: any[] } { + if (!this.isEpochStoredDatetime(table, field)) return { sql: '??', bindings: [field] }; + return { + sql: `(case when typeof(??) in ('integer','real') then julianday(??/1000.0, 'unixepoch') else julianday(??) end)`, + bindings: [field, field, field], + }; + } + /** * Public, dialect-correct temporal filter-value coercion for callers that * build SQL *outside* the normal `find()`/`applyFilters()` path — chiefly the 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 ba61af3440..2714397595 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 @@ -20,7 +20,9 @@ type Granularity = 'day' | 'week' | 'month' | 'quarter' | 'year'; /** ⚠️ Keep in sync with `packages/objectql/src/in-memory-aggregation.ts#bucketDateValue` */ function bucketDateValue(value: unknown, g: Granularity): string { if (value == null) return '(null)'; - const d = value instanceof Date ? value : new Date(String(value)); + // 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)'; const y = d.getUTCFullYear(); const m = d.getUTCMonth() + 1; @@ -114,6 +116,42 @@ describe('SqliteWasmDriver date bucket (dateGranularity)', () => { }, ); + // #3773 — the fixture above is TEXT (`t.string('ts')`). A `Field.datetime` + // declared through `initObjects` is stored as epoch ms instead, and `strftime` + // reads a bare integer as a Julian day number → every row bucketed as NULL. + // SqliteWasmDriver inherits `buildDateBucketExpr` from SqlDriver, so it + // inherited the bug and now inherits the fix; pinned here so a wasm-side + // divergence can't reintroduce it. + describe('epoch-stored Field.datetime', () => { + it('buckets by the stored instant, not into one null bucket', async () => { + const d2 = new SqliteWasmDriver({ filename: ':memory:' }); + try { + await d2.initObjects([ + { name: 'deal', fields: { closed_at: { type: 'datetime' }, amount: { type: 'number' } } }, + ]); + for (const [id, iso, amount] of [ + ['d1', '2026-01-10T09:00:00Z', 1], + ['d2', '2026-01-20T09:00:00Z', 2], + ['d3', '2026-02-14T09:00:00Z', 4], + ] as const) { + await d2.create('deal', { id, closed_at: new Date(iso), amount }, { bypassTenantAudit: true }); + } + + const rows = await d2.aggregate('deal', { + groupBy: [{ field: 'closed_at', dateGranularity: 'month' }], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + } as any); + + const byMonth = Object.fromEntries( + rows.map((r: any) => [String(r.closed_at), Number(r.total)]), + ); + expect(byMonth).toEqual({ '2026-01': 3, '2026-02': 4 }); + } finally { + await d2.disconnect(); + } + }); + }); + describe('unsupported granularity', () => { it('throws a loud error for week on SQLite (so engine routes to in-memory)', async () => { await expect(