From 02923771020e4f4ee5a79639a5c43b8c115d6dc4 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:01:45 +0800 Subject: [PATCH] fix(driver-sql,analytics): stop aggregate()/distinct() leaking SQLite's raw epoch storage (#3797) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: Claude --- .changeset/aggregate-temporal-output.md | 48 ++++ ...l-driver-aggregate-temporal-output.test.ts | 230 ++++++++++++++++++ packages/plugins/driver-sql/src/sql-driver.ts | 101 +++++++- .../__tests__/cross-object-rebucket.test.ts | 40 +++ .../src/strategies/cross-object-rebucket.ts | 48 +++- 5 files changed, 446 insertions(+), 21 deletions(-) create mode 100644 .changeset/aggregate-temporal-output.md create mode 100644 packages/plugins/driver-sql/src/sql-driver-aggregate-temporal-output.test.ts 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; } /**