Skip to content

Commit 5f0852f

Browse files
os-zhuangclaude
andauthored
fix(driver-sql): bucket a SQLite Field.datetime by its stored instant (#3773) (#3775)
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: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
1 parent 605c23f commit 5f0852f

9 files changed

Lines changed: 522 additions & 26 deletions
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
"@objectstack/objectql": patch
4+
---
5+
6+
fix(driver-sql): bucket a SQLite `Field.datetime` by its stored instant instead of collapsing every row into one `(null)` (#3773)
7+
8+
On SQLite, any trend chart bucketed by day/week/month/year over a
9+
`Field.datetime` column put **every record in a single `(null)` bucket** — one
10+
bar, carrying the whole total. The measure was right; only the bucket key was
11+
wrong. `Field.date` (ISO TEXT storage) was unaffected, so the same dashboard
12+
could show one column working and the next one flat.
13+
14+
better-sqlite3 stores a `Field.datetime` as INTEGER epoch **milliseconds** (knex
15+
binds a JS `Date` as `.getTime()`), and `buildDateBucketExpr` emitted a flat
16+
`strftime('%Y-%m', col)`. SQLite reads a bare integer as a **Julian day
17+
number**; an epoch-ms value is far outside the legal range, so `strftime`
18+
returned NULL for every row. Nothing downstream noticed: SQLite advertises
19+
`queryDateGranularity.month`, so `engine.aggregate` pushes the bucketing down,
20+
and its in-memory fallback only engages for an *unsupported* granularity or a
21+
non-UTC timezone.
22+
23+
The SQLite expression is now storage-aware, sharing one `isEpochStoredDatetime`
24+
predicate with the filter-comparand coercion added for the same root cause in
25+
\#2034 — a window and a bucket that disagree about storage is exactly how an
26+
epoch column ended up correctly filtered and then entirely bucketed as NULL.
27+
Postgres and MySQL are untouched: `defineColumn` maps `Field.datetime` to a
28+
native timestamp there, which is also why their comparands are left alone.
29+
30+
Two details are load-bearing and pinned by tests:
31+
32+
- The conversion dispatches on each **stored value's** type, not just the
33+
declared one. A SQLite `Field.datetime` column is genuinely mixed-form —
34+
`formatInput` passes datetime values through, so a `Date` lands as INTEGER
35+
while an ISO string (including an unresolved `defaultValue: 'NOW()'`) lands as
36+
TEXT. Dividing TEXT by 1000 coerces it to its leading year, filing live rows
37+
under 1970 — worse than the NULL it replaced.
38+
- Division is `/1000.0`, not `/1000`. Integer division truncates toward zero, so
39+
a pre-1970 instant (`-1` ms) would surface as 1970-01-01.
40+
41+
`bucketDateValue` (the in-memory fallback in `@objectstack/objectql`) now reads a
42+
finite **number** as epoch milliseconds. `new Date(String(1767225600000))` is an
43+
Invalid Date, so a driver handing back raw storage values bucketed as `'(null)'`
44+
there while the pushed-down SQL bucketed correctly — fixing only the driver would
45+
have traded one wrong answer for two different ones, and the two paths have to
46+
label the same instant identically for a drill-down to survive crossing them.
47+
48+
`SqliteWasmDriver` inherits `buildDateBucketExpr`, so it carried the bug and gets
49+
the fix.

‎packages/objectql/src/in-memory-aggregation.test.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,29 @@ describe('bucketDateValue', () => {
106106
expect(bucketDateValue('not-a-date', 'month')).toBe('(null)');
107107
});
108108

109+
// #3773 — parity with the pushed-down SQL. SQLite stores a `Field.datetime`
110+
// as epoch milliseconds, so a driver that hands back raw storage values feeds
111+
// this a NUMBER. `new Date(String(1767225600000))` is an Invalid Date, so
112+
// these all bucketed as '(null)' while the native SQL bucketed them correctly
113+
// — the two paths have to label the same instant identically.
114+
it('reads a finite number as epoch milliseconds', () => {
115+
const ms = Date.parse('2026-01-10T09:00:00Z');
116+
expect(bucketDateValue(ms, 'year')).toBe('2026');
117+
expect(bucketDateValue(ms, 'quarter')).toBe('2026-Q1');
118+
expect(bucketDateValue(ms, 'month')).toBe('2026-01');
119+
expect(bucketDateValue(ms, 'day')).toBe('2026-01-10');
120+
// Same instant, all three shapes a driver might return.
121+
for (const g of ['year', 'quarter', 'month', 'day'] as const) {
122+
expect(bucketDateValue(ms, g)).toBe(bucketDateValue(new Date(ms), g));
123+
expect(bucketDateValue(ms, g)).toBe(bucketDateValue(new Date(ms).toISOString(), g));
124+
}
125+
});
126+
127+
it('reads a negative epoch as a pre-1970 instant', () => {
128+
expect(bucketDateValue(-1, 'day')).toBe('1969-12-31');
129+
expect(bucketDateValue(0, 'day')).toBe('1970-01-01');
130+
});
131+
109132
// ADR-0053 Phase 2 (D2): a non-UTC reference timezone shifts the calendar day.
110133
describe('timezone-aware bucketing', () => {
111134
// 2024-03-01T03:00Z is still 2024-02-29 (22:00) in America/New_York.

‎packages/objectql/src/in-memory-aggregation.ts‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,14 +181,26 @@ function toNumber(v: any): number {
181181
* The y/m/d are taken in the reference zone and the ISO-week math then runs on
182182
* a UTC date built from those parts — the parts already carry the zone shift,
183183
* so the week boundary lands correctly without re-applying any offset.
184+
*
185+
* A finite NUMBER is read as epoch milliseconds — the form SQLite stores a
186+
* `Field.datetime` in, and what any driver that hands back raw storage values
187+
* yields. `new Date(String(1767225600000))` is an Invalid Date, so without this
188+
* branch such a row bucketed as `'(null)'` while the pushed-down SQL bucketed it
189+
* correctly (#3773) — the two paths must label the same instant identically or a
190+
* drill-down built on one breaks against the other.
184191
*/
185192
export function bucketDateValue(
186193
value: unknown,
187194
granularity: DateGranularityValue,
188195
timezone?: string,
189196
): string {
190197
if (value == null) return '(null)';
191-
const d = value instanceof Date ? value : new Date(String(value));
198+
const d =
199+
value instanceof Date
200+
? value
201+
: typeof value === 'number'
202+
? new Date(value)
203+
: new Date(String(value));
192204
if (Number.isNaN(d.getTime())) return '(null)';
193205
const { year: y, month: m, day } = calendarPartsInTzOrUtc(d, timezone);
194206
switch (granularity) {

‎packages/plugins/driver-sql/src/sql-driver-aggregate-datetime-window.test.ts‎

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -100,29 +100,26 @@ describe('SqlDriver.aggregate — ISO window over epoch-stored datetime (#3650)'
100100
expect(byMonth).toEqual({ '2026-01': 300, '2026-02': 30 });
101101
});
102102

103-
it('KNOWN GAP — bucketing an EPOCH-stored datetime collapses into one null bucket', async () => {
104-
// Pre-existing, unrelated to #3650, and deliberately NOT fixed here — but
105-
// it lands on the exact same query shape, so it is pinned rather than left
106-
// to be rediscovered as "the dateRange fix did nothing".
107-
//
108-
// SQLite advertises `queryDateGranularity.month`, so `engine.aggregate`
109-
// pushes the bucketing down to the driver — `engine.ts` only falls back to
110-
// in-memory bucketing when a granularity is UNSUPPORTED or a non-UTC
111-
// timezone is in play, neither of which applies here. The dialect
112-
// expression is `strftime('%Y-%m', col)`, and SQLite reads a bare INTEGER
113-
// as a Julian day number; an epoch-ms value is far outside the legal range,
114-
// so every row buckets as NULL.
103+
it('buckets an EPOCH-stored datetime inside the window (was one null bucket)', async () => {
104+
// This assertion was pinned as a KNOWN GAP by #3650 and is the acceptance
105+
// gate of the follow-up fix (#3773): SQLite advertises
106+
// `queryDateGranularity.month`, so `engine.aggregate` pushes the bucketing
107+
// down to the driver — `engine.ts` only falls back to in-memory bucketing
108+
// when a granularity is UNSUPPORTED or a non-UTC timezone is in play,
109+
// neither of which applies here. The dialect expression used to be a flat
110+
// `strftime('%Y-%m', col)`, and SQLite reads a bare INTEGER as a Julian day
111+
// number; an epoch-ms value is far outside the legal range, so every row
112+
// bucketed as NULL and the whole trend chart collapsed to one bar.
115113
const rows = await driver.aggregate(TABLE, {
116114
groupBy: [{ field: 'closed_at', dateGranularity: 'month' }],
117115
aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }],
118116
where: window('closed_at'),
119117
} as any);
120118

121-
// The WINDOW works — the total is the in-window 330, not the full 1930 —
122-
// which is what #3650 is responsible for. The BUCKETS are what is broken.
123-
// When that is fixed, this becomes `{ '2026-01': 300, '2026-02': 30 }`.
119+
// Two things have to hold at once: the WINDOW (#3650 — the total is the
120+
// in-window 330, not the full 1930) and the BUCKETS (#3773).
124121
const byMonth = Object.fromEntries(rows.map((r: any) => [String(r.closed_at), Number(r.total)]));
125-
expect(byMonth).toEqual({ null: 330 });
122+
expect(byMonth).toEqual({ '2026-01': 300, '2026-02': 30 });
126123
});
127124

128125
it('confines a date (TEXT-stored) aggregate to the same window', async () => {
Lines changed: 217 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,217 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Date bucketing across the two SQLite storage forms (#3773).
5+
*
6+
* `sql-driver-date-bucket.test.ts` builds its fixture with
7+
* `knex.schema.createTable` + `t.string('ts')`, so every value it buckets is ISO
8+
* TEXT. That is only half of what SQLite actually holds: a `Field.datetime`
9+
* declared through `initObjects` becomes INTEGER epoch **milliseconds** (knex
10+
* binds a JS `Date` as `.getTime()`), and `strftime` reads a bare integer as a
11+
* Julian day number — epoch ms is orders of magnitude outside the legal range,
12+
* so every row bucketed as NULL and any datetime trend chart rendered as one
13+
* `(null)` bar carrying the whole total.
14+
*
15+
* So this suite goes through `driver.initObjects([...])` — the path a real
16+
* object takes — and sweeps every supported granularity against BOTH storage
17+
* forms, asserting against the same `bucketDateValue` labels the in-memory
18+
* fallback produces. Anything that buckets differently depending on how the
19+
* column happens to be stored is the bug this file exists to catch.
20+
*/
21+
22+
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
23+
import { SqlDriver } from '../src/index.js';
24+
25+
type Granularity = 'day' | 'month' | 'quarter' | 'year';
26+
27+
/** Every granularity SQLite advertises natively (week is bucketed in-memory). */
28+
const GRANULARITIES: Granularity[] = ['day', 'month', 'quarter', 'year'];
29+
30+
/** ⚠️ Keep in sync with `packages/objectql/src/in-memory-aggregation.ts#bucketDateValue` */
31+
function bucketDateValue(value: unknown, g: Granularity): string {
32+
if (value == null) return '(null)';
33+
// A finite number is epoch milliseconds — SQLite's `Field.datetime` storage.
34+
const d =
35+
value instanceof Date ? value : typeof value === 'number' ? new Date(value) : new Date(String(value));
36+
if (Number.isNaN(d.getTime())) return '(null)';
37+
const y = d.getUTCFullYear();
38+
const m = d.getUTCMonth() + 1;
39+
switch (g) {
40+
case 'year': return String(y);
41+
case 'quarter': return `${y}-Q${Math.floor((m - 1) / 3) + 1}`;
42+
case 'month': return `${y}-${String(m).padStart(2, '0')}`;
43+
case 'day': return `${y}-${String(m).padStart(2, '0')}-${String(d.getUTCDate()).padStart(2, '0')}`;
44+
}
45+
}
46+
47+
const TABLE = 'deal';
48+
49+
/**
50+
* One UTC instant per row, with its `Field.date` twin on the same calendar day
51+
* so the two columns MUST produce identical labels at every granularity — the
52+
* whole point being that storage form may not change the answer.
53+
*
54+
* Amounts are distinct powers of two: a bucket's sum names exactly which rows
55+
* landed in it, so a mis-bucketing can't hide behind a coincidental total.
56+
*/
57+
const FIXTURE: Array<{ id: string; iso: string; amount: number }> = [
58+
{ id: 'r1', iso: '1969-12-31T23:59:59.999Z', amount: 1 }, // pre-epoch, 1ms before 1970
59+
{ id: 'r2', iso: '2025-11-15T09:00:00.000Z', amount: 2 },
60+
{ id: 'r3', iso: '2026-01-10T09:00:00.000Z', amount: 4 },
61+
{ id: 'r4', iso: '2026-01-20T23:59:59.000Z', amount: 8 }, // same month as r3
62+
{ id: 'r5', iso: '2026-02-14T00:00:00.000Z', amount: 16 }, // exact midnight
63+
{ id: 'r6', iso: '2026-06-30T23:59:59.000Z', amount: 32 }, // last instant of Q2
64+
{ id: 'r7', iso: '2026-07-01T00:00:00.000Z', amount: 64 }, // first instant of Q3
65+
];
66+
67+
/** The labels the in-memory path would produce, folded into bucket → sum. */
68+
function expectedBuckets(g: Granularity): Record<string, number> {
69+
const out: Record<string, number> = {};
70+
for (const row of FIXTURE) {
71+
const key = bucketDateValue(row.iso, g);
72+
out[key] = (out[key] ?? 0) + row.amount;
73+
}
74+
return out;
75+
}
76+
77+
async function bucketSums(driver: SqlDriver, field: string, g: Granularity) {
78+
const rows = await driver.aggregate(TABLE, {
79+
groupBy: [{ field, dateGranularity: g }],
80+
aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }],
81+
} as any);
82+
return Object.fromEntries(rows.map((r: any) => [String(r[field]), Number(r.total)]));
83+
}
84+
85+
describe('SqlDriver date bucketing is storage-form independent (#3773)', () => {
86+
let driver: SqlDriver;
87+
88+
beforeEach(async () => {
89+
driver = new SqlDriver({
90+
client: 'better-sqlite3',
91+
connection: { filename: ':memory:' },
92+
useNullAsDefault: true,
93+
});
94+
95+
await driver.initObjects([
96+
{
97+
name: TABLE,
98+
fields: {
99+
closed_at: { type: 'datetime' }, // INTEGER epoch ms under better-sqlite3
100+
closed_on: { type: 'date' }, // YYYY-MM-DD TEXT
101+
amount: { type: 'number' },
102+
},
103+
},
104+
]);
105+
106+
for (const { id, iso, amount } of FIXTURE) {
107+
await driver.create(
108+
TABLE,
109+
// A real `Date` for the datetime column — the path the seed loader and
110+
// every normal write take, and the one that produces epoch storage.
111+
{ id, closed_at: new Date(iso), closed_on: iso.slice(0, 10), amount },
112+
{ bypassTenantAudit: true },
113+
);
114+
}
115+
});
116+
117+
afterEach(async () => {
118+
await driver.disconnect();
119+
});
120+
121+
it('really does store the two columns in the two different forms', async () => {
122+
// The premise of this whole file. If better-sqlite3 ever stops binding a
123+
// `Date` as an integer, this fails first and explains the rest.
124+
const res: any = await driver.execute(
125+
`SELECT typeof("closed_at") AS at_t, typeof("closed_on") AS on_t FROM "${TABLE}" WHERE id = 'r3'`,
126+
);
127+
const row = Array.isArray(res) ? res[0] : (res?.rows?.[0] ?? res);
128+
expect(['integer', 'real']).toContain(row.at_t);
129+
expect(row.on_t).toBe('text');
130+
});
131+
132+
for (const g of GRANULARITIES) {
133+
describe(`granularity '${g}'`, () => {
134+
it('buckets the epoch-stored datetime column', async () => {
135+
expect(await bucketSums(driver, 'closed_at', g)).toEqual(expectedBuckets(g));
136+
});
137+
138+
it('buckets the TEXT-stored date column', async () => {
139+
expect(await bucketSums(driver, 'closed_on', g)).toEqual(expectedBuckets(g));
140+
});
141+
142+
it('gives both columns the same labels', async () => {
143+
const [byAt, byOn] = await Promise.all([
144+
bucketSums(driver, 'closed_at', g),
145+
bucketSums(driver, 'closed_on', g),
146+
]);
147+
expect(Object.keys(byAt).sort()).toEqual(Object.keys(byOn).sort());
148+
});
149+
});
150+
}
151+
152+
it('keeps a pre-1970 instant on its own calendar day', async () => {
153+
// Guards the `/1000.0` in the bucket expression. Integer division truncates
154+
// toward zero, so `-1 / 1000` is 0 and this row would surface as 1970-01-01
155+
// — a full day, year and quarter wrong, and only for negative epochs.
156+
const byDay = await bucketSums(driver, 'closed_at', 'day');
157+
expect(byDay['1969-12-31']).toBe(1);
158+
expect(byDay['1970-01-01']).toBeUndefined();
159+
});
160+
});
161+
162+
describe('SqlDriver date bucketing over a MIXED-form datetime column (#3773)', () => {
163+
// One SQLite `Field.datetime` column legitimately holds both forms at once:
164+
// `formatInput` leaves datetime values alone, so a `Date` lands as INTEGER
165+
// epoch ms while an ISO string (what an unresolved `defaultValue: 'NOW()'`
166+
// slot and any string-valued write produce) lands as TEXT. A bucket
167+
// expression that assumed epoch for the whole column would divide the TEXT by
168+
// 1000 — `'2026-01-10T…' / 1000.0` is 2.026 seconds past the epoch — and file
169+
// live rows under 1970, which is worse than the NULL it replaced.
170+
let driver: SqlDriver;
171+
172+
beforeEach(async () => {
173+
driver = new SqlDriver({
174+
client: 'better-sqlite3',
175+
connection: { filename: ':memory:' },
176+
useNullAsDefault: true,
177+
});
178+
await driver.initObjects([
179+
{ name: TABLE, fields: { closed_at: { type: 'datetime' }, amount: { type: 'number' } } },
180+
]);
181+
await driver.create(TABLE, { id: 'int', closed_at: new Date('2026-01-10T09:00:00Z'), amount: 1 }, { bypassTenantAudit: true });
182+
await driver.create(TABLE, { id: 'txt', closed_at: '2026-02-14T09:00:00Z', amount: 2 }, { bypassTenantAudit: true });
183+
await driver.create(TABLE, { id: 'naive', closed_at: '2026-02-20 09:00:00', amount: 4 }, { bypassTenantAudit: true });
184+
await driver.create(TABLE, { id: 'nil', closed_at: null, amount: 8 }, { bypassTenantAudit: true });
185+
});
186+
187+
afterEach(async () => {
188+
await driver.disconnect();
189+
});
190+
191+
it('stores the fixture in both forms', async () => {
192+
const res: any = await driver.execute(
193+
`SELECT id, typeof("closed_at") AS t FROM "${TABLE}" ORDER BY id`,
194+
);
195+
const rows = Array.isArray(res) ? res : (res?.rows ?? []);
196+
const byId = Object.fromEntries(rows.map((r: any) => [r.id, r.t]));
197+
expect(['integer', 'real']).toContain(byId.int);
198+
expect(byId.txt).toBe('text');
199+
expect(byId.naive).toBe('text');
200+
expect(byId.nil).toBe('null');
201+
});
202+
203+
it('buckets each row by its own stored form', async () => {
204+
const byMonth = await bucketSums(driver, 'closed_at', 'month');
205+
expect(byMonth['2026-01']).toBe(1); // INTEGER epoch ms
206+
expect(byMonth['2026-02']).toBe(6); // ISO TEXT (2) + zone-naive TEXT (4)
207+
expect(byMonth['1970-01']).toBeUndefined(); // TEXT never divided by 1000
208+
});
209+
210+
it('leaves a NULL instant in its own bucket', async () => {
211+
const byMonth = await bucketSums(driver, 'closed_at', 'month');
212+
// SQL NULL aliases to the string 'null' through `String(r[field])` — a
213+
// pre-existing divergence from the in-memory label `'(null)'`, unchanged
214+
// here and equally true of a TEXT-stored column.
215+
expect(byMonth.null).toBe(8);
216+
});
217+
});

‎packages/plugins/driver-sql/src/sql-driver-date-bucket.test.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@ type Granularity = 'day' | 'week' | 'month' | 'quarter' | 'year';
2020
/** ⚠️ Keep in sync with `packages/objectql/src/in-memory-aggregation.ts#bucketDateValue` */
2121
function bucketDateValue(value: unknown, g: Granularity): string {
2222
if (value == null) return '(null)';
23-
const d = value instanceof Date ? value : new Date(String(value));
23+
// A finite number is epoch milliseconds — SQLite's `Field.datetime` storage.
24+
const d =
25+
value instanceof Date ? value : typeof value === 'number' ? new Date(value) : new Date(String(value));
2426
if (Number.isNaN(d.getTime())) return '(null)';
2527
const y = d.getUTCFullYear();
2628
const m = d.getUTCMonth() + 1;

0 commit comments

Comments
 (0)