Skip to content

Commit c7f4417

Browse files
os-zhuangclaude
andauthored
fix(driver-sql,analytics): stop aggregate()/distinct() leaking SQLite's raw epoch storage (#3797) (#3804)
Both returned `await builder` directly, without the `formatOutput` pass every `find()` row gets. On SQLite — the one dialect where a `Field.datetime` is stored as INTEGER epoch milliseconds rather than a native timestamp — that raw storage form went straight to the caller, while the same column read through `find()` came back as canonical ISO-Z. Same root cause as #3773, different exit. `Field.date` was never affected: it is ISO TEXT on every dialect, so its storage form already equals its presentation. The visible surfaces were a `_max`/`_min` measure over a datetime (a "last closed" KPI tile rendered 1768035600000) and a `groupBy` on a raw datetime dimension, which also disagreed with the in-memory `applyInMemoryAggregation` fallback — that one consumes already-formatted `find()` rows, so the same dataset changed key type depending on which path served it. Which columns hold an instant is recorded while the statement is built, because that is the only point where a column name and its meaning are both known: a `min()` lands under its alias and never under the field name, while a date-BUCKETED column lands under the field name but holds a label ('2026-01') rather than an instant. Matching on names afterwards gets both backwards, and there is a test for each direction. `distinct()` additionally re-deduplicates after presenting: SQL DISTINCT compares STORED values and one SQLite datetime column holds both forms, so two rows recording the same instant survived as two and then presented identically. It has no in-repo callers today; fixed for consistency, not urgency. cross-object-rebucket is fixed alongside it, because presenting min/max correctly is what exposed it. `recombine()` coerced every operand with `Number()`, which silently depended on receiving an epoch: handed the ISO string the driver now returns it produced NaN, and on Postgres/MySQL (where knex returns a Date) it had always flattened the value back to an epoch integer one layer above the driver. min/max now order by the instant and return the winning value in the shape it arrived in; sum/count stay numeric. Verified red first — the temporal cases produce NaN against the old implementation. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
1 parent ce09d4e commit c7f4417

5 files changed

Lines changed: 446 additions & 21 deletions

File tree

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
---
2+
"@objectstack/driver-sql": patch
3+
"@objectstack/service-analytics": patch
4+
---
5+
6+
fix(driver-sql,analytics): stop `aggregate()` / `distinct()` leaking SQLite's raw epoch storage (#3797)
7+
8+
Both returned `await builder` directly, without the `formatOutput` pass every
9+
`find()` row gets. On SQLite — the one dialect where a `Field.datetime` is
10+
stored as INTEGER epoch milliseconds rather than a native timestamp — that raw
11+
storage form went straight to the caller:
12+
13+
| call | before | after |
14+
| --- | --- | --- |
15+
| `find()` | `"2026-01-10T09:00:00.000Z"` | unchanged |
16+
| `distinct('closed_at')` | `[1768035600000]` | `["2026-01-10T09:00:00.000Z"]` |
17+
| `aggregate()` `max(closed_at)` | `1768035600000` | `"2026-01-10T09:00:00.000Z"` |
18+
| `aggregate()` `groupBy: ['closed_at']` | key `1768035600000` | key `"2026-01-10T09:00:00.000Z"` |
19+
20+
Same root cause as #3773, different exit. `Field.date` was never affected — it
21+
is ISO TEXT on every dialect, so its storage form already equals its
22+
presentation.
23+
24+
The visible surfaces were a `_max`/`_min` measure over a datetime (a "last
25+
closed" KPI tile rendered `1768035600000`) and a `groupBy` on a raw datetime
26+
dimension, which also disagreed with the in-memory `applyInMemoryAggregation`
27+
fallback — that one consumes already-formatted `find()` rows, so the same
28+
dataset changed key type depending on which path served it.
29+
30+
Which columns hold an instant is now recorded while the statement is built,
31+
because that is the only point where a column name and its meaning are both
32+
known: a `min()` lands under its alias and never under the field name, while a
33+
date-BUCKETED column lands under the field name but holds a label (`'2026-01'`)
34+
rather than an instant. Matching on names afterwards gets both backwards.
35+
36+
`distinct()` additionally re-deduplicates after presenting: SQL `DISTINCT`
37+
compares STORED values, and one SQLite datetime column holds both INTEGER and
38+
TEXT forms, so two rows recording the same instant survived as two and then
39+
presented identically. It has no in-repo callers today; this keeps it honest
40+
rather than leaving a second convention in the driver.
41+
42+
**`cross-object-rebucket` was fixed alongside it, because presenting min/max
43+
correctly is what exposed it.** `recombine()` coerced every operand with
44+
`Number()`, which silently depended on receiving an epoch: handed the ISO string
45+
the driver now returns it produced `NaN`, and on Postgres/MySQL (where knex
46+
returns a `Date`) it had always flattened the value back to an epoch integer one
47+
layer above the driver. `min`/`max` now order by the instant and return the
48+
winning value in the shape it arrived in; `sum`/`count` stay numeric.
Lines changed: 230 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,230 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Temporal values leaving `aggregate()` / `distinct()` (#3797).
5+
*
6+
* Both return `await builder` directly, without the `formatOutput` pass every
7+
* `find()` row gets — so on SQLite, where a `Field.datetime` is stored as
8+
* INTEGER epoch milliseconds, the raw storage form leaked straight to the
9+
* caller while the same column read through `find()` came back as canonical
10+
* ISO-`Z`. Same root cause as #3773, different exit.
11+
*
12+
* The contract asserted here: **a datetime that leaves the driver is a datetime
13+
* in the same shape, whichever call produced it** — and the in-memory
14+
* `applyInMemoryAggregation` fallback (which consumes already-formatted
15+
* `find()` rows) has to agree, or a dataset changes key type depending on which
16+
* path served it.
17+
*
18+
* The two shapes that must NOT be normalized are covered too: a date-bucketed
19+
* column is a LABEL (`'2026-01'`), not an instant, and a numeric aggregate over
20+
* a datetime is a number.
21+
*/
22+
23+
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
24+
import { SqlDriver } from '../src/index.js';
25+
26+
const TABLE = 'deal';
27+
28+
/** The canonical presentation `find()` has always produced. */
29+
const ISO = '2026-01-10T09:00:00.000Z';
30+
const ISO_LATER = '2026-02-14T09:00:00.000Z';
31+
32+
describe('temporal values leaving aggregate()/distinct() (#3797)', () => {
33+
let driver: SqlDriver;
34+
35+
beforeEach(async () => {
36+
driver = new SqlDriver({
37+
client: 'better-sqlite3',
38+
connection: { filename: ':memory:' },
39+
useNullAsDefault: true,
40+
});
41+
42+
await driver.initObjects([
43+
{
44+
name: TABLE,
45+
fields: {
46+
closed_at: { type: 'datetime' }, // INTEGER epoch ms under better-sqlite3
47+
closed_on: { type: 'date' }, // YYYY-MM-DD TEXT
48+
region: { type: 'string' },
49+
amount: { type: 'number' },
50+
},
51+
},
52+
]);
53+
54+
for (const [id, iso, region, amount] of [
55+
['d1', ISO, 'east', 1],
56+
['d2', ISO, 'east', 2], // duplicate instant — distinct() must collapse it
57+
['d3', ISO_LATER, 'west', 4],
58+
] as const) {
59+
await driver.create(
60+
TABLE,
61+
{ id, closed_at: new Date(iso), closed_on: iso.slice(0, 10), region, amount },
62+
{ bypassTenantAudit: true },
63+
);
64+
}
65+
});
66+
67+
afterEach(async () => {
68+
await driver.disconnect();
69+
});
70+
71+
/** What the same column looks like through the path that always formatted it. */
72+
const viaFind = async (field: string) => {
73+
const rows = await driver.find(TABLE, { orderBy: [['id', 'asc']] } as any);
74+
return rows.map((r: any) => r[field]);
75+
};
76+
77+
describe('distinct()', () => {
78+
it('presents a datetime the same way find() does', async () => {
79+
const values = await driver.distinct(TABLE, 'closed_at');
80+
expect(values.sort()).toEqual([ISO, ISO_LATER]);
81+
// Not the raw storage form.
82+
expect(values.some((v: any) => typeof v === 'number')).toBe(false);
83+
});
84+
85+
it('agrees with find() on the same column', async () => {
86+
const [distinctValues, foundValues] = await Promise.all([
87+
driver.distinct(TABLE, 'closed_at'),
88+
viaFind('closed_at'),
89+
]);
90+
expect(new Set(distinctValues)).toEqual(new Set(foundValues));
91+
});
92+
93+
it('leaves a date column as YYYY-MM-DD', async () => {
94+
const values = await driver.distinct(TABLE, 'closed_on');
95+
expect(values.sort()).toEqual(['2026-01-10', '2026-02-14']);
96+
});
97+
98+
it('leaves a non-temporal column alone', async () => {
99+
expect((await driver.distinct(TABLE, 'region')).sort()).toEqual(['east', 'west']);
100+
});
101+
102+
it('collapses two storage forms of the same instant into one value', async () => {
103+
// SQL `DISTINCT` compares STORED values, and one SQLite `Field.datetime`
104+
// column holds both forms — `formatInput` passes datetime values through,
105+
// so a `Date` lands as INTEGER epoch ms and an ISO string lands as TEXT.
106+
// Two rows recording the SAME instant therefore survive `DISTINCT` as two
107+
// rows and then present identically, which is a duplicate unless the
108+
// presented values are re-deduplicated.
109+
await driver.create(
110+
TABLE,
111+
{ id: 'd4', closed_at: ISO, closed_on: '2026-01-10', region: 'east', amount: 8 },
112+
{ bypassTenantAudit: true },
113+
);
114+
115+
const raw: any = await driver.execute(
116+
`SELECT DISTINCT typeof("closed_at") AS t FROM "${TABLE}" ORDER BY t`,
117+
);
118+
const forms = (Array.isArray(raw) ? raw : (raw?.rows ?? [])).map((r: any) => r.t);
119+
expect(forms).toContain('text'); // the row just written
120+
expect(forms.some((f: string) => f === 'integer' || f === 'real')).toBe(true);
121+
122+
// Three stored rows for two instants → two values, not three.
123+
expect((await driver.distinct(TABLE, 'closed_at')).sort()).toEqual([ISO, ISO_LATER]);
124+
});
125+
});
126+
127+
describe('aggregate() — min/max over a temporal column', () => {
128+
it('presents max(datetime) as an instant, not an epoch integer', async () => {
129+
const rows = await driver.aggregate(TABLE, {
130+
aggregations: [
131+
{ function: 'max', field: 'closed_at', alias: 'latest' },
132+
{ function: 'min', field: 'closed_at', alias: 'earliest' },
133+
],
134+
} as any);
135+
expect(rows[0].latest).toBe(ISO_LATER);
136+
expect(rows[0].earliest).toBe(ISO);
137+
});
138+
139+
it('presents min/max of a date column as YYYY-MM-DD', async () => {
140+
const rows = await driver.aggregate(TABLE, {
141+
aggregations: [{ function: 'max', field: 'closed_on', alias: 'latest' }],
142+
} as any);
143+
expect(rows[0].latest).toBe('2026-02-14');
144+
});
145+
146+
it('follows the alias, not the field name', async () => {
147+
// The column is called `latest`; a name-driven fix would never find it.
148+
const rows = await driver.aggregate(TABLE, {
149+
aggregations: [{ function: 'max', field: 'closed_at', alias: 'whatever_i_called_it' }],
150+
} as any);
151+
expect(rows[0].whatever_i_called_it).toBe(ISO_LATER);
152+
});
153+
154+
it('leaves a NUMERIC aggregate over a datetime as a number', async () => {
155+
// count/sum/avg over a datetime are numbers by construction — presenting
156+
// them as instants would be a different kind of wrong.
157+
const rows = await driver.aggregate(TABLE, {
158+
aggregations: [{ function: 'count', field: 'closed_at', alias: 'n' }],
159+
} as any);
160+
expect(rows[0].n).toBe(3);
161+
});
162+
});
163+
164+
describe('aggregate() — groupBy on a raw temporal column', () => {
165+
it('keys the group by an instant, not an epoch integer', async () => {
166+
const rows = await driver.aggregate(TABLE, {
167+
groupBy: ['closed_at'],
168+
aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }],
169+
} as any);
170+
const byInstant = Object.fromEntries(rows.map((r: any) => [r.closed_at, Number(r.total)]));
171+
expect(byInstant).toEqual({ [ISO]: 3, [ISO_LATER]: 4 });
172+
});
173+
174+
it('keys a structured groupBy without granularity the same way', async () => {
175+
const rows = await driver.aggregate(TABLE, {
176+
groupBy: [{ field: 'closed_at' }],
177+
aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }],
178+
} as any);
179+
const byInstant = Object.fromEntries(rows.map((r: any) => [r.closed_at, Number(r.total)]));
180+
expect(byInstant).toEqual({ [ISO]: 3, [ISO_LATER]: 4 });
181+
});
182+
183+
it('matches what the in-memory fallback would key on', async () => {
184+
// `applyInMemoryAggregation` groups over `driver.find()` rows, which are
185+
// already formatted — so its keys are `String(<ISO>)`. The pushed-down
186+
// path has to produce the same key or a dataset changes shape depending
187+
// on which path served it.
188+
const rows = await driver.aggregate(TABLE, {
189+
groupBy: ['closed_at'],
190+
aggregations: [{ function: 'count', alias: 'n' }],
191+
} as any);
192+
const driverKeys = rows.map((r: any) => String(r.closed_at)).sort();
193+
const inMemoryKeys = [...new Set((await viaFind('closed_at')).map(String))].sort();
194+
expect(driverKeys).toEqual(inMemoryKeys);
195+
});
196+
197+
it('leaves a date-BUCKETED column as its label, not an instant', async () => {
198+
// Since #3773 the bucket expression is aliased AS the field name, so its
199+
// column collides with a real datetime field — but its value is a label.
200+
// Normalizing by column name would feed '2026-01' into the datetime
201+
// presenter; this is the assertion that keeps the two apart.
202+
const rows = await driver.aggregate(TABLE, {
203+
groupBy: [{ field: 'closed_at', dateGranularity: 'month' }],
204+
aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }],
205+
} as any);
206+
const byMonth = Object.fromEntries(rows.map((r: any) => [r.closed_at, Number(r.total)]));
207+
expect(byMonth).toEqual({ '2026-01': 3, '2026-02': 4 });
208+
});
209+
210+
it('leaves a non-temporal groupBy key alone', async () => {
211+
const rows = await driver.aggregate(TABLE, {
212+
groupBy: ['region'],
213+
aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }],
214+
} as any);
215+
expect(Object.fromEntries(rows.map((r: any) => [r.region, Number(r.total)]))).toEqual({
216+
east: 3,
217+
west: 4,
218+
});
219+
});
220+
221+
it('handles a mixed groupBy — temporal key formatted, plain key untouched', async () => {
222+
const rows = await driver.aggregate(TABLE, {
223+
groupBy: ['region', 'closed_at'],
224+
aggregations: [{ function: 'count', alias: 'n' }],
225+
} as any);
226+
const norm = rows.map((r: any) => `${r.region}|${r.closed_at}=${Number(r.n)}`).sort();
227+
expect(norm).toEqual([`east|${ISO}=2`, `west|${ISO_LATER}=1`]);
228+
});
229+
});
230+
});

0 commit comments

Comments
 (0)