diff --git a/.changeset/aggregate-temporal-output.md b/.changeset/aggregate-temporal-output.md new file mode 100644 index 0000000000..46415b60a5 --- /dev/null +++ b/.changeset/aggregate-temporal-output.md @@ -0,0 +1,48 @@ +--- +"@objectstack/driver-sql": patch +"@objectstack/service-analytics": patch +--- + +fix(driver-sql,analytics): stop `aggregate()` / `distinct()` leaking SQLite's raw epoch storage (#3797) + +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: + +| call | before | after | +| --- | --- | --- | +| `find()` | `"2026-01-10T09:00:00.000Z"` | unchanged | +| `distinct('closed_at')` | `[1768035600000]` | `["2026-01-10T09:00:00.000Z"]` | +| `aggregate()` `max(closed_at)` | `1768035600000` | `"2026-01-10T09:00:00.000Z"` | +| `aggregate()` `groupBy: ['closed_at']` | key `1768035600000` | key `"2026-01-10T09:00:00.000Z"` | + +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 now 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. + +`distinct()` additionally re-deduplicates after presenting: SQL `DISTINCT` +compares STORED values, and one SQLite datetime column holds both INTEGER and +TEXT forms, so two rows recording the same instant survived as two and then +presented identically. It has no in-repo callers today; this keeps it honest +rather than leaving a second convention in the driver. + +**`cross-object-rebucket` was 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. diff --git a/packages/plugins/driver-sql/src/sql-driver-aggregate-temporal-output.test.ts b/packages/plugins/driver-sql/src/sql-driver-aggregate-temporal-output.test.ts new file mode 100644 index 0000000000..69a3a2ce36 --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-aggregate-temporal-output.test.ts @@ -0,0 +1,230 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Temporal values leaving `aggregate()` / `distinct()` (#3797). + * + * Both return `await builder` directly, without the `formatOutput` pass every + * `find()` row gets — so on SQLite, where a `Field.datetime` is stored as + * INTEGER epoch milliseconds, the raw storage form leaked straight to the + * caller while the same column read through `find()` came back as canonical + * ISO-`Z`. Same root cause as #3773, different exit. + * + * The contract asserted here: **a datetime that leaves the driver is a datetime + * in the same shape, whichever call produced it** — and the in-memory + * `applyInMemoryAggregation` fallback (which consumes already-formatted + * `find()` rows) has to agree, or a dataset changes key type depending on which + * path served it. + * + * The two shapes that must NOT be normalized are covered too: a date-bucketed + * column is a LABEL (`'2026-01'`), not an instant, and a numeric aggregate over + * a datetime is a number. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +const TABLE = 'deal'; + +/** The canonical presentation `find()` has always produced. */ +const ISO = '2026-01-10T09:00:00.000Z'; +const ISO_LATER = '2026-02-14T09:00:00.000Z'; + +describe('temporal values leaving aggregate()/distinct() (#3797)', () => { + 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 + region: { type: 'string' }, + amount: { type: 'number' }, + }, + }, + ]); + + for (const [id, iso, region, amount] of [ + ['d1', ISO, 'east', 1], + ['d2', ISO, 'east', 2], // duplicate instant — distinct() must collapse it + ['d3', ISO_LATER, 'west', 4], + ] as const) { + await driver.create( + TABLE, + { id, closed_at: new Date(iso), closed_on: iso.slice(0, 10), region, amount }, + { bypassTenantAudit: true }, + ); + } + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + /** What the same column looks like through the path that always formatted it. */ + const viaFind = async (field: string) => { + const rows = await driver.find(TABLE, { orderBy: [['id', 'asc']] } as any); + return rows.map((r: any) => r[field]); + }; + + describe('distinct()', () => { + it('presents a datetime the same way find() does', async () => { + const values = await driver.distinct(TABLE, 'closed_at'); + expect(values.sort()).toEqual([ISO, ISO_LATER]); + // Not the raw storage form. + expect(values.some((v: any) => typeof v === 'number')).toBe(false); + }); + + it('agrees with find() on the same column', async () => { + const [distinctValues, foundValues] = await Promise.all([ + driver.distinct(TABLE, 'closed_at'), + viaFind('closed_at'), + ]); + expect(new Set(distinctValues)).toEqual(new Set(foundValues)); + }); + + it('leaves a date column as YYYY-MM-DD', async () => { + const values = await driver.distinct(TABLE, 'closed_on'); + expect(values.sort()).toEqual(['2026-01-10', '2026-02-14']); + }); + + it('leaves a non-temporal column alone', async () => { + expect((await driver.distinct(TABLE, 'region')).sort()).toEqual(['east', 'west']); + }); + + it('collapses two storage forms of the same instant into one value', async () => { + // SQL `DISTINCT` compares STORED values, and one SQLite `Field.datetime` + // column holds both forms — `formatInput` passes datetime values through, + // so a `Date` lands as INTEGER epoch ms and an ISO string lands as TEXT. + // Two rows recording the SAME instant therefore survive `DISTINCT` as two + // rows and then present identically, which is a duplicate unless the + // presented values are re-deduplicated. + await driver.create( + TABLE, + { id: 'd4', closed_at: ISO, closed_on: '2026-01-10', region: 'east', amount: 8 }, + { bypassTenantAudit: true }, + ); + + const raw: any = await driver.execute( + `SELECT DISTINCT typeof("closed_at") AS t FROM "${TABLE}" ORDER BY t`, + ); + const forms = (Array.isArray(raw) ? raw : (raw?.rows ?? [])).map((r: any) => r.t); + expect(forms).toContain('text'); // the row just written + expect(forms.some((f: string) => f === 'integer' || f === 'real')).toBe(true); + + // Three stored rows for two instants → two values, not three. + expect((await driver.distinct(TABLE, 'closed_at')).sort()).toEqual([ISO, ISO_LATER]); + }); + }); + + describe('aggregate() — min/max over a temporal column', () => { + it('presents max(datetime) as an instant, not an epoch integer', async () => { + const rows = await driver.aggregate(TABLE, { + aggregations: [ + { function: 'max', field: 'closed_at', alias: 'latest' }, + { function: 'min', field: 'closed_at', alias: 'earliest' }, + ], + } as any); + expect(rows[0].latest).toBe(ISO_LATER); + expect(rows[0].earliest).toBe(ISO); + }); + + it('presents min/max of a date column as YYYY-MM-DD', async () => { + const rows = await driver.aggregate(TABLE, { + aggregations: [{ function: 'max', field: 'closed_on', alias: 'latest' }], + } as any); + expect(rows[0].latest).toBe('2026-02-14'); + }); + + it('follows the alias, not the field name', async () => { + // The column is called `latest`; a name-driven fix would never find it. + const rows = await driver.aggregate(TABLE, { + aggregations: [{ function: 'max', field: 'closed_at', alias: 'whatever_i_called_it' }], + } as any); + expect(rows[0].whatever_i_called_it).toBe(ISO_LATER); + }); + + it('leaves a NUMERIC aggregate over a datetime as a number', async () => { + // count/sum/avg over a datetime are numbers by construction — presenting + // them as instants would be a different kind of wrong. + const rows = await driver.aggregate(TABLE, { + aggregations: [{ function: 'count', field: 'closed_at', alias: 'n' }], + } as any); + expect(rows[0].n).toBe(3); + }); + }); + + describe('aggregate() — groupBy on a raw temporal column', () => { + it('keys the group by an instant, not an epoch integer', async () => { + const rows = await driver.aggregate(TABLE, { + groupBy: ['closed_at'], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + } as any); + const byInstant = Object.fromEntries(rows.map((r: any) => [r.closed_at, Number(r.total)])); + expect(byInstant).toEqual({ [ISO]: 3, [ISO_LATER]: 4 }); + }); + + it('keys a structured groupBy without granularity the same way', async () => { + const rows = await driver.aggregate(TABLE, { + groupBy: [{ field: 'closed_at' }], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + } as any); + const byInstant = Object.fromEntries(rows.map((r: any) => [r.closed_at, Number(r.total)])); + expect(byInstant).toEqual({ [ISO]: 3, [ISO_LATER]: 4 }); + }); + + it('matches what the in-memory fallback would key on', async () => { + // `applyInMemoryAggregation` groups over `driver.find()` rows, which are + // already formatted — so its keys are `String()`. The pushed-down + // path has to produce the same key or a dataset changes shape depending + // on which path served it. + const rows = await driver.aggregate(TABLE, { + groupBy: ['closed_at'], + aggregations: [{ function: 'count', alias: 'n' }], + } as any); + const driverKeys = rows.map((r: any) => String(r.closed_at)).sort(); + const inMemoryKeys = [...new Set((await viaFind('closed_at')).map(String))].sort(); + expect(driverKeys).toEqual(inMemoryKeys); + }); + + it('leaves a date-BUCKETED column as its label, not an instant', async () => { + // Since #3773 the bucket expression is aliased AS the field name, so its + // column collides with a real datetime field — but its value is a label. + // Normalizing by column name would feed '2026-01' into the datetime + // presenter; this is the assertion that keeps the two apart. + const rows = await driver.aggregate(TABLE, { + groupBy: [{ field: 'closed_at', dateGranularity: 'month' }], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + } as any); + const byMonth = Object.fromEntries(rows.map((r: any) => [r.closed_at, Number(r.total)])); + expect(byMonth).toEqual({ '2026-01': 3, '2026-02': 4 }); + }); + + it('leaves a non-temporal groupBy key alone', async () => { + const rows = await driver.aggregate(TABLE, { + groupBy: ['region'], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + } as any); + expect(Object.fromEntries(rows.map((r: any) => [r.region, Number(r.total)]))).toEqual({ + east: 3, + west: 4, + }); + }); + + it('handles a mixed groupBy — temporal key formatted, plain key untouched', async () => { + const rows = await driver.aggregate(TABLE, { + groupBy: ['region', 'closed_at'], + aggregations: [{ function: 'count', alias: 'n' }], + } as any); + const norm = rows.map((r: any) => `${r.region}|${r.closed_at}=${Number(r.n)}`).sort(); + expect(norm).toEqual([`east|${ISO}=2`, `west|${ISO_LATER}=1`]); + }); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index ef99e5b2fb..5b6168425f 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -1663,24 +1663,34 @@ export class SqlDriver implements IDataDriver { this.applyFilters(builder, query.where); } + // The same coercion key `applyFilters` just used, so every part of this + // statement agrees on how each column is stored — the WHERE window, the + // GROUP BY bucket expression (#3773) and the result presentation (#3797). + const table = this.coercionKey(builder); + + // Result columns that carry a raw temporal VALUE, keyed by the column name + // the caller will read. Collected 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 (never under the field name), and a + // date-BUCKETED column lands under the field name while holding a label + // (`'2026-01'`), not an instant. Matching on names after the fact gets both + // of those backwards. See {@link presentTemporalColumns}. + const temporalOutput = new Map(); + if (query.groupBy) { // groupBy items may be plain strings ('region') or structured objects // ({ 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); + const kind = this.temporalFieldKind(table, g); + if (kind) temporalOutput.set(g, kind); } else if (g && typeof g === 'object' && g.field) { if (g.dateGranularity) { - const bucket = this.buildDateBucketExpr(g.field, g.dateGranularity as any, bucketTable); + const bucket = this.buildDateBucketExpr(g.field, g.dateGranularity as any, table); if (!bucket) { throw new Error( `SqlDriver: dateGranularity '${g.dateGranularity}' not supported on dialect ` + @@ -1692,6 +1702,8 @@ export class SqlDriver implements IDataDriver { } else { builder.groupBy(g.field); builder.select(g.field); + const kind = this.temporalFieldKind(table, g.field); + if (kind) temporalOutput.set(g.field, kind); } } } @@ -1710,6 +1722,16 @@ export class SqlDriver implements IDataDriver { } else { builder.select(this.knex.raw(`${rawFunc}(??) as ??`, [fieldExpr, agg.alias])); } + // `min`/`max` are the only supported functions that hand back a value + // OF the column rather than a count/total derived from it, so they are + // the only ones whose result is still an instant. `alias` is required + // by `AggregationNodeSchema`; the unaliased branch below lands under a + // dialect-dependent column name (`max("closed_at")` on SQLite, `max` on + // Postgres) and is defensive only, so it is deliberately not tracked. + if ((funcName === 'min' || funcName === 'max') && agg.field) { + const kind = this.temporalFieldKind(table, agg.field); + if (kind) temporalOutput.set(agg.alias, kind); + } } else { if (fieldExpr === '*') { builder.select(this.knex.raw(`${rawFunc}(*)`)); @@ -1720,7 +1742,8 @@ export class SqlDriver implements IDataDriver { } } - return await builder; + const rows = await builder; + return this.presentTemporalColumns(rows, temporalOutput); } // =================================== @@ -1736,7 +1759,17 @@ export class SqlDriver implements IDataDriver { builder.distinct(field); const results = await builder; - return results.map((row: any) => row[field]); + const values = results.map((row: any) => row[field]); + + // Same presentation `find()` gives the column (#3797) — a caller listing a + // datetime's values should not get epoch integers here and ISO strings + // there. Re-deduplicate afterwards: SQL `DISTINCT` compares STORED values, + // and one SQLite `Field.datetime` column holds both INTEGER epoch ms and + // ISO TEXT, so two rows recording the same instant survive as two rows and + // then collapse to the same presented value. + const kind = this.temporalFieldKind(this.coercionKey(builder), field); + if (!kind) return values; + return [...new Set(values.map((v: any) => this.presentTemporalValue(kind, v)))]; } // =================================== @@ -3484,6 +3517,56 @@ export class SqlDriver implements IDataDriver { * 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). */ + /** + * Which temporal presentation rule, if any, a declared field takes — + * `null` for everything that is not a `Field.datetime` / `Field.date`. + */ + protected temporalFieldKind( + table: string | null | undefined, + field: string, + ): 'datetime' | 'date' | null { + if (!table) return null; + if (this.datetimeFields[table]?.has(field)) return 'datetime'; + if (this.dateFields[table]?.has(field)) return 'date'; + return null; + } + + /** + * Present one temporal value exactly the way `formatOutput` presents it on a + * `find()` row, for the read paths that return raw builder output instead + * (`aggregate`, `distinct` — #3797). + * + * The dialect gating mirrors `formatOutput`: the `Field.datetime` repair is + * SQLite-only (it is the one dialect where storage ≠ presentation), while the + * `Field.date` → `YYYY-MM-DD` collapse runs everywhere. + */ + protected presentTemporalValue(kind: 'datetime' | 'date', value: any): any { + if (value == null) return value; + if (kind === 'date') return this.toDateOnly(value); + return this.isSqlite ? normalizeSqliteDatetimeOutput(value) : value; + } + + /** + * Apply {@link presentTemporalValue} to the result columns a caller of + * `aggregate()` will read as instants. + * + * Which columns those are cannot be recovered from the rows — the driver has + * to be told, because the mapping from column name to meaning is only + * unambiguous while the statement is being built (a `min()` lands under its + * alias; a date-BUCKETED column lands under the field name but holds a label). + * Rows are mutated in place, as `formatOutput` does. + */ + protected presentTemporalColumns(rows: any, columns: Map): any { + if (columns.size === 0 || !Array.isArray(rows)) return rows; + for (const row of rows) { + if (!row || typeof row !== 'object') continue; + for (const [column, kind] of columns) { + if (row[column] !== undefined) row[column] = this.presentTemporalValue(kind, row[column]); + } + } + return rows; + } + protected sqliteTemporalArg( field: string, table: string | null | undefined, diff --git a/packages/services/service-analytics/src/__tests__/cross-object-rebucket.test.ts b/packages/services/service-analytics/src/__tests__/cross-object-rebucket.test.ts index 5ceeb07220..350257db56 100644 --- a/packages/services/service-analytics/src/__tests__/cross-object-rebucket.test.ts +++ b/packages/services/service-analytics/src/__tests__/cross-object-rebucket.test.ts @@ -69,6 +69,46 @@ describe('rebucketCrossObject (#3654)', () => { ]); }); + // #3797 — a min/max measure over a `Field.datetime` arrives as an ISO string + // (SQLite, since the driver stopped leaking its epoch storage) or as a `Date` + // (Postgres/MySQL, where knex maps a native timestamp). Coercing either with + // `Number()` gave `NaN` and epoch-int respectively; min/max must order by the + // instant and hand back the winning value in the shape it arrived in. + it('recombines a temporal min/max by instant, preserving the value shape', () => { + const base = [ + { account: 'acc_w1', first: '2026-03-01T00:00:00.000Z', last: '2026-03-09T00:00:00.000Z' }, + { account: 'acc_w2', first: '2026-01-10T09:00:00.000Z', last: '2026-02-14T09:00:00.000Z' }, + { account: 'acc_e1', first: '2025-12-31T23:59:59.000Z', last: '2025-12-31T23:59:59.000Z' }, + ]; + const dims: CrossObjectDim[] = [{ outputName: 'region', fkField: 'account', fkToAttr: fkToRegion }]; + const measures: MeasureRecombine[] = [ + { alias: 'first', method: 'min' }, + { alias: 'last', method: 'max' }, + ]; + const out = rebucketCrossObject(base, [], dims, measures); + expect(out).toEqual([ + // West merges the two accounts: earliest first, latest last — and the + // values come back as the ISO strings they went in as, not as epochs. + { region: 'West', first: '2026-01-10T09:00:00.000Z', last: '2026-03-09T00:00:00.000Z' }, + { region: 'East', first: '2025-12-31T23:59:59.000Z', last: '2025-12-31T23:59:59.000Z' }, + ]); + }); + + it('recombines a temporal min/max delivered as Date objects', () => { + const d = (iso: string) => new Date(iso); + const base = [ + { account: 'acc_w1', last: d('2026-03-09T00:00:00.000Z') }, + { account: 'acc_w2', last: d('2026-02-14T09:00:00.000Z') }, + ]; + const out = rebucketCrossObject( + base, + [], + [{ outputName: 'region', fkField: 'account', fkToAttr: fkToRegion }], + [{ alias: 'last', method: 'max' }], + ); + expect(out).toEqual([{ region: 'West', last: d('2026-03-09T00:00:00.000Z') }]); + }); + it('keeps a base dimension alongside the cross-object dimension', () => { const base = [ { stage: 'won', account: 'acc_w1', revenue: 100 }, 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 50d9fb7a9b..249a3bd3bc 100644 --- a/packages/services/service-analytics/src/strategies/cross-object-rebucket.ts +++ b/packages/services/service-analytics/src/strategies/cross-object-rebucket.ts @@ -52,20 +52,44 @@ export interface MeasureRecombine { method: RecombinableMethod; } -/** Combine two measure values under an aggregation method (either may be undefined). */ +/** + * Order two measure values. A number orders as itself; a `Date` or an ISO + * timestamp orders as its instant, so a `min`/`max` over a temporal measure + * compares correctly instead of collapsing to `NaN` (#3797). `NaN` means "not + * orderable" and the caller keeps the other side. + */ +function orderableValue(v: unknown): number { + if (v == null) return NaN; + if (typeof v === 'number') return v; + if (v instanceof Date) return v.getTime(); + const n = Number(v); + if (Number.isFinite(n)) return n; + return Date.parse(String(v)); +} + +/** + * Combine two measure values under an aggregation method (either may be + * undefined). + * + * `sum`/`count` are numeric by construction and stay so. `min`/`max` return the + * winning ORIGINAL value rather than a number: the value they pick is a value + * OF the column, so a temporal measure has to come back out in the same shape + * the driver presented it (#3797) — coercing it to a number here would put the + * epoch leak back one layer up, and on any dialect whose driver returns an ISO + * string it would produce `NaN` outright. + */ function recombine(method: RecombinableMethod, acc: unknown, next: unknown): unknown { - const n = Number(next ?? 0); - if (acc === undefined) return method === 'min' || method === 'max' ? Number(next ?? 0) : n; - const a = Number(acc); - switch (method) { - case 'sum': - case 'count': - return a + n; - case 'min': - return Math.min(a, n); - case 'max': - return Math.max(a, n); + if (method === 'min' || method === 'max') { + if (acc === undefined) return next ?? 0; + const a = orderableValue(acc); + const n = orderableValue(next); + if (Number.isNaN(n)) return acc; + if (Number.isNaN(a)) return next; + const nextWins = method === 'min' ? n < a : n > a; + return nextWins ? next : acc; } + const n = Number(next ?? 0); + return acc === undefined ? n : Number(acc) + n; } /**