diff --git a/.changeset/20387-aggregate-one-double.md b/.changeset/20387-aggregate-one-double.md new file mode 100644 index 00000000000..7194e30ee2e --- /dev/null +++ b/.changeset/20387-aggregate-one-double.md @@ -0,0 +1,50 @@ +--- +'@objectstack/driver-sql': patch +--- + +fix(driver-sql): `sum` / `avg` accumulate in double on PostgreSQL and MySQL, as they do on SQLite and on the engine's rows path + +Clause-②: no + +`SqlDriver.aggregate` let PostgreSQL (`numeric`) and MySQL (`DECIMAL`) add exact decimals, +while SQLite and the engine's rows path add JS doubles. So a `number` column holding `0.1` +and `0.2` summed to `0.3` on the PostgreSQL and MySQL native paths and to +`0.30000000000000004` everywhere else, and `having { s: { $eq: 0.3 } }` kept the group on +those two faces only. `avg` over an integer column diverged too: MySQL rounds a decimal +average to 4 places (`avg` of 1, 2, 2 answered `1.6667`), and PostgreSQL's `numeric` +average rounds to 16 places before the answer becomes a double (`11 / 9` answered +`1.2222222222222222`, where every other face answers `1.2222222222222223`). + +**The precision policy, applied to the arithmetic.** The policy already stated for the +answer's type (one JS double on every dialect, the loss beyond a double's precision declared) +now also decides how the answer is computed: + +- `avg` accumulates in double on PostgreSQL and MySQL, over every declared numeric or + boolean column. +- `sum` accumulates in double over a column that holds fractions: `number`, `currency`, + `percent`, `slider`, `progress`, `summary`, and the driver's `float` alias. +- `sum` over an integer-valued column (`rating`, the `integer` / `int` aliases, a boolean) + keeps the database's exact integer total, rounded once to the double. +- `count`, `count_distinct`, `min` and `max` are unchanged. SQLite is unchanged. + +Each value added is the column's text parsed as a double: the value the SQL client hands +`find()`, and so the value the rows path adds. For the exact-decimal columns this equals a +plain cast. For a binary `real` / `FLOAT` column, which a table created before the +exact-decimal columns still has, a plain cast would add the widened binary value +(`0.30000000447034836` for `0.1 + 0.2`). MySQL's `CAST(… AS DOUBLE)` needs MySQL 8.0.17 or +later. + +Route chosen: (a), accumulate in double on the native faces. The other route, (b), was to make +the rows path add exact decimals and round once. It was rejected because SQLite's native `sum` +adds the stored doubles (`0.30000000000000004`), so the rows path would then disagree with SQLite +for exactly `0.1 + 0.2`. + +**Residual, stated.** Double sums are added in row order, one after another, as the rows path +adds them. SQLite 3.43 and later adds with compensated summation. So for a group of three or +more fractions, SQLite's native answer can still differ from every other face in the last +place (`0.1 + 0.2 + 0.3`: SQLite `0.6`, every other face `0.6000000000000001`). This was +already true of SQLite's two paths before this change. Two addends cannot differ. + +A consumer that compared `sum` / `avg` over a fractional column with a decimal literal on +PostgreSQL or MySQL (`$eq: 0.3`) now gets the answer SQLite and the rows path already gave: +compare with a range, or with the double the arithmetic produces. diff --git a/packages/drivers/driver-sql/src/sql-driver-20387-aggregate-double-accumulation.test.ts b/packages/drivers/driver-sql/src/sql-driver-20387-aggregate-double-accumulation.test.ts new file mode 100644 index 00000000000..53e0b007f92 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-20387-aggregate-double-accumulation.test.ts @@ -0,0 +1,216 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#20387] `sum` / `avg` from `SqlDriver.aggregate` accumulate in double on + * every dialect: the arithmetic the engine's rows path (`objectql`'s + * `in-memory-aggregation.ts`) and SQLite already use, under the one-double + * policy `AGGREGATE_ANSWER_KIND` states (`sql-driver.ts`, + * `AGGREGATE_ACCUMULATION`). + * + * Measured on the base (`75b216924`) through `engine.aggregate` and + * `POST /api/v1/data/:object/query`, a `number` column holding `0.1` and `0.2`: + * + * | face | `sum` | `avg` | `having { s: { $eq: 0.3 } }` | + * |:--|:--|:--|:--| + * | PostgreSQL 16.13 native (`numeric`) | `0.3` | `0.15` | keeps the group | + * | MySQL 8.0.46 native (`DECIMAL`) | `0.3` | `0.15` | keeps the group | + * | SQLite native (REAL) | `0.30000000000000004` | `0.15000000000000002` | keeps no group | + * | the rows path, every dialect | `0.30000000000000004` | `0.15000000000000002` | keeps no group | + * + * `avg` over an integer-valued column diverged as well: MySQL answered + * `1.6667` for a `rating` of 1, 2, 2 (every other face `5 / 3`), and + * PostgreSQL's `numeric` average of nine ratings summing to 11 answered + * `1.2222222222222222` (JS `11 / 9` is `1.2222222222222223`). + * + * `having` is evaluated by the engine, over the value this door answers + * (`having-filter.ts`: `$eq` compares the value with `==`), so the value pinned + * here is what decides `having` `$eq` / `$in` on every face. The expected + * values are computed from `find()`'s own rows with the rows path's arithmetic + * ({@link rowsPathSum}), plus the literal the triage named, so a decimal + * answer, a compensated sum or a string each fail. + * + * What stays as it was, pinned too: `count` / `min` / `max`, and a `sum` over + * an integer-valued column, which keeps the database's exact total (above 2^53 + * that is not what adding doubles gives, and the policy declares that bound). + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import type { DriverQuery } from '@objectstack/spec/contracts'; +import { SqlDriver } from './sql-driver.js'; +import { DIALECT_CELLS, declareDialectCell, type DialectCell } from './live-dialect-matrix.testkit.js'; + +const TABLE = 'os20387_agg_double'; + +/** The rows path's `toNumber` (`in-memory-aggregation.ts`): a number as-is, null as 0, else `Number()`. */ +function toNumber(v: unknown): number { + if (typeof v === 'number') return v; + if (v == null) return 0; + const n = Number(v); + return Number.isFinite(n) ? n : 0; +} + +/** The rows path's `sum`: `values.reduce((a, b) => a + toNumber(b), 0)`, in row order. */ +const rowsPathSum = (values: readonly unknown[]) => values.reduce((a, b) => a + toNumber(b), 0); + +/** The rows path's `avg`: the non-null values' `sum`, divided by how many there are. */ +function rowsPathAvg(values: readonly unknown[]): number | null { + const defined = values.filter((v) => v != null); + return defined.length === 0 ? null : rowsPathSum(defined) / defined.length; +} + +interface Row { + id: string; + g: string; + w?: number; + price?: number; + legacy?: number; + stars?: number; + flag?: boolean; +} + +const ROWS: readonly Row[] = [ + // The triage's pin: `0.1 + 0.2`. Two addends, so no summation order or + // compensation scheme can move the last place. + { id: 'p1', g: 'pin', w: 0.1, price: 0.1, legacy: 0.1, stars: 1, flag: true }, + { id: 'p2', g: 'pin', w: 0.2, price: 0.2, legacy: 0.2, stars: 2, flag: false }, + // `avg` over integer-valued columns: 5 / 3 (MySQL's `DECIMAL` average rounds + // it to 1.6667) and 1 / 3. + { id: 't1', g: 'three', stars: 1, flag: true }, + { id: 't2', g: 'three', stars: 2, flag: false }, + { id: 't3', g: 'three', stars: 2, flag: false }, + // 11 / 9: PostgreSQL's `numeric` average rounds it to 16 places first. + ...[1, 1, 1, 1, 1, 1, 1, 2, 2].map((stars, i) => ({ id: `n${i}`, g: 'nine', stars })), +]; + +type Aggregation = NonNullable[number]; +const agg = (fn: Aggregation['function'], field: string, alias: string): Aggregation => ({ function: fn, field, alias }); + +function declareCell(cell: DialectCell): void { + describe(`[#20387] driver-sql — sum / avg accumulate in double (${cell.label})`, () => { + let driver: SqlDriver; + + async function answer(g: string, aggregations: Aggregation[]) { + const query: DriverQuery = { where: { g }, groupBy: ['g'], aggregations }; + const rows = (await driver.aggregate(TABLE, query)) as Array>; + expect(rows, `one group for ${g}`).toHaveLength(1); + return rows[0]; + } + + async function column(g: string, field: string): Promise { + const rows = (await driver.find(TABLE, { where: { g }, orderBy: [{ field: 'id', order: 'asc' }] })) as Array< + Record + >; + return rows.map((r) => r[field]); + } + + beforeAll(async () => { + driver = new SqlDriver(cell.config()); + await driver.execute(`drop table if exists ${TABLE}`).catch(() => {}); + await driver.initObjects([ + { + name: TABLE, + fields: { + g: { type: 'text' }, + w: { type: 'number' }, + price: { type: 'currency' }, + // A `number` column as a table created before the exact-decimal + // columns carries it: binary32 `real` / `FLOAT` (retyped below). + legacy: { type: 'number' }, + stars: { type: 'rating' }, + flag: { type: 'boolean' }, + // The driver's `integer` alias — how an introspected `bigint` + // reaches it (retyped to `bigint` below on the servers). + big: { type: 'integer' }, + }, + }, + ] as never); + if (cell.id === 'pg') { + await driver.execute(`alter table ${TABLE} alter column legacy type real`); + await driver.execute(`alter table ${TABLE} alter column big type bigint`); + } else if (cell.id === 'mysql') { + await driver.execute(`alter table ${TABLE} modify legacy float(8,2)`); + await driver.execute(`alter table ${TABLE} modify big bigint`); + } + for (const row of ROWS) await driver.create(TABLE, { ...row }, { bypassTenantAudit: true }); + // Written by SQL: a JS number cannot carry 2^53 + 1 to the column. + await driver.execute(`insert into ${TABLE} (id, g, big) values ('b1', 'big', 9007199254740993)`); + await driver.execute(`insert into ${TABLE} (id, g, big) values ('b2', 'big', 1)`); + }); + + afterAll(async () => { + await driver?.execute(`drop table if exists ${TABLE}`).catch(() => {}); + await driver?.disconnect(); + }); + + it('the pin: sum / avg over 0.1 and 0.2 answer 0.1 + 0.2 and its half, as the rows path adds them', async () => { + const a = await answer('pin', [ + agg('sum', 'w', 's'), + agg('avg', 'w', 'a'), + agg('sum', 'price', 'sp'), + agg('avg', 'price', 'ap'), + ]); + const w = await column('pin', 'w'); + expect(w, 'find() reads the numbers written').toStrictEqual([0.1, 0.2]); + expect(a.s, 'sum(number)').toBe(rowsPathSum(w)); + expect(a.a, 'avg(number)').toBe(rowsPathAvg(w)); + expect(a.sp, 'sum(currency)').toBe(rowsPathSum(await column('pin', 'price'))); + expect(a.ap, 'avg(currency)').toBe(rowsPathAvg(await column('pin', 'price'))); + // The literal the triage named, so the oracle above cannot drift with it. + expect(a.s).toBe(0.30000000000000004); + expect(a.a).toBe(0.15000000000000002); + // `having { s: { $eq: 0.3 } }` / `{ $in: [0.3] }` therefore keep no group + // here, as on SQLite and the rows path; `0.1 + 0.2` keeps it everywhere. + expect(a.s == 0.3, 'having s $eq 0.3').toBe(false); + expect([0.3].some((t) => a.s == t), 'having s $in [0.3]').toBe(false); + expect(a.a == 0.15, 'having a $eq 0.15').toBe(false); + expect(a.s == 0.1 + 0.2, 'having s $eq 0.1 + 0.2').toBe(true); + }); + + it('a binary32 column is added as the client reads it, not as its widened binary value', async () => { + // `find()` reads `0.1`; a plain cast to double would add + // 0.10000000149011612 (sum 0.30000000447034836) on PostgreSQL `real` + // and MySQL `FLOAT`. The text of the column is what the rows path adds. + const a = await answer('pin', [agg('sum', 'legacy', 's'), agg('avg', 'legacy', 'a')]); + const legacy = await column('pin', 'legacy'); + expect(legacy, 'find() reads the numbers written').toStrictEqual([0.1, 0.2]); + expect(a.s, 'sum(legacy)').toBe(rowsPathSum(legacy)); + expect(a.a, 'avg(legacy)').toBe(rowsPathAvg(legacy)); + }); + + it('avg over an integer-valued column is the double quotient, as the rows path divides', async () => { + for (const g of ['three', 'nine']) { + const a = await answer(g, [agg('avg', 'stars', 'as'), agg('avg', 'flag', 'af')]); + expect(a.as, `${g} avg(rating)`).toBe(rowsPathAvg(await column(g, 'stars'))); + const flags = (await column(g, 'flag')).map((f) => (f == null ? null : Number(f))); + expect(a.af, `${g} avg(boolean)`).toBe(rowsPathAvg(flags)); + } + expect((await answer('three', [agg('avg', 'stars', 'as')])).as).toBe(5 / 3); + expect((await answer('nine', [agg('avg', 'stars', 'as')])).as).toBe(11 / 9); + }); + + it('sum over an integer-valued column keeps the exact total, rounded once', async () => { + const a = await answer('big', [agg('sum', 'big', 's')]); + // 2^53 + 1 + 1 = 2^53 + 2, a double. Adding the doubles would answer + // 2^53: the bound the one-double policy declares, not taken here. + expect(a.s).toBe(9007199254740994); + const three = await answer('three', [agg('sum', 'stars', 's')]); + expect(three.s, 'sum(rating)').toBe(5); + }); + + it('count, min and max are untouched', async () => { + const a = await answer('pin', [ + { function: 'count', alias: 'n' }, + agg('count', 'w', 'nw'), + agg('min', 'w', 'mn'), + agg('max', 'w', 'mx'), + agg('min', 'stars', 'smn'), + agg('max', 'stars', 'smx'), + ]); + expect(a).toMatchObject({ n: 2, nw: 2, mn: 0.1, mx: 0.2, smn: 1, smx: 2 }); + }); + }); +} + +for (const cell of DIALECT_CELLS) { + declareDialectCell(cell, 'aggregate double accumulation (#20387)', declareCell); +} diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 0b9924ccae4..63e5dac4557 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -321,6 +321,15 @@ const NUMERIC_SCALAR_TYPES = new Set([ 'integer', 'int', 'float', ]); +/** + * [#20387] Whether a numeric field type's column holds FRACTIONS rather than + * integers: the exact-decimal members of `NUMERIC_COLUMN_REPRESENTATION` and + * the driver's `float` alias. Read into {@link SqlDriver.fractionalNumericFields}. + */ +function isFractionalNumericType(type: string): boolean { + return type === 'float' || numericColumnFor(type)?.kind === 'exact'; +} + /** * The builtin audit-timestamp columns every managed object carries. They are * stamped to a single canonical instant format on SQLite (see @@ -1540,6 +1549,65 @@ const AGGREGATE_ANSWER_KIND: Readonly +> = { + count: 'as-stored', + count_distinct: 'as-stored', + sum: 'double-over-fractional', + avg: 'double', + min: 'as-stored', + max: 'as-stored', +}; + /** * [#5907] The aggregate vocabulary the Query Protocol DECLARES, read from the * spec rather than restated — `AggregationNodeSchema.function` is this enum, so @@ -5660,6 +5728,20 @@ export class SqlDriver implements IDataDriver { * originally written for actually lives. */ protected numericValueFields: Record = {}; + /** + * [#20387] The subset of {@link numericFields} whose declared type holds + * FRACTIONS — the exact-decimal members of `NUMERIC_COLUMN_REPRESENTATION` + * (`numericColumnFor(type).kind === 'exact'`) and the driver's `float` alias, + * which is how an introspected decimal / floating column reaches it. What is + * left out is integer-valued: `rating`, and the `integer` / `int` aliases (an + * introspected `bigint` among them). + * + * Read by {@link SqlDriver.aggregate} only: a `sum` over one of these columns + * accumulates in double on PostgreSQL and MySQL, and a `sum` over an + * integer-valued column keeps the database's exact integer total. See + * `AGGREGATE_ACCUMULATION`. + */ + protected fractionalNumericFields: Record = {}; protected dateFields: Record> = {}; protected datetimeFields: Record> = {}; /** @@ -10145,7 +10227,14 @@ export class SqlDriver implements IDataDriver { fieldExpr !== '*' && table !== null && (this.booleanFields[table]?.includes(fieldExpr) ?? false); - const argExpr = castBooleanAggregand ? 'cast(?? as int)' : '??'; + const columnExpr = castBooleanAggregand ? 'cast(?? as int)' : '??'; + // [#20387] `sum` / `avg` accumulate in double on PostgreSQL and MySQL, + // the arithmetic SQLite and the engine's rows path already use, so one + // query answers one number on every face (`AGGREGATE_ACCUMULATION`). + // Still one `??` binding: the wrap is SQL text around it. + const argExpr = fieldExpr !== '*' && this.accumulatesInDouble(funcName, table, fieldExpr) + ? this.doubleAccumulationOperand(columnExpr) + : columnExpr; const rawFunc = lowering.distinct ? `${lowering.sql}(distinct ${argExpr})` : `${lowering.sql}(${argExpr})`; @@ -10266,6 +10355,40 @@ export class SqlDriver implements IDataDriver { return this.presentReadColumns(this.foldEmptyAggregateAnswers(rows, foldedOutput), presentedOutput); } + /** + * [#20387] Whether {@link aggregate} accumulates this aggregation's operand in + * double — `AGGREGATE_ACCUMULATION` read against the column's declaration. + * + * PostgreSQL and MySQL only. SQLite stores the fractional family as REAL + * (`ColumnCompiler_SQLite3.prototype.decimal` is `'float'`), so its `sum` / + * `avg` already add doubles. A column this driver has no numeric or boolean + * declaration for keeps the database's own arithmetic, as before. + */ + protected accumulatesInDouble(func: AggregationFunction, table: string | null, field: string): boolean { + if (table === null || !(this.isPostgres || this.isMysql)) return false; + switch (AGGREGATE_ACCUMULATION[func]) { + case 'double': + return (this.numericFields[table]?.includes(field) ?? false) + || (this.booleanFields[table]?.includes(field) ?? false); + case 'double-over-fractional': + return this.fractionalNumericFields[table]?.includes(field) ?? false; + case 'as-stored': + return false; + } + } + + /** + * [#20387] The operand of a double-accumulated `sum` / `avg`: the column's + * text, parsed as a double — the value the SQL client hands `find()`, and so + * the value the rows path adds. See `AGGREGATE_ACCUMULATION` for why the text + * and not a plain cast. + */ + protected doubleAccumulationOperand(operand: string): string { + return this.isPostgres + ? `cast(cast(${operand} as text) as double precision)` + : `cast(cast(${operand} as char) as double)`; + } + /** * [#15546] Fold the NULL a SQL aggregate answers over an all-NULL aggregand * to the identity the platform declares for that aggregate over NOTHING. @@ -11374,6 +11497,7 @@ export class SqlDriver implements IDataDriver { this.booleanFields[shard] = this.booleanFields[base] ?? []; this.numericFields[shard] = this.numericFields[base] ?? []; this.numericValueFields[shard] = this.numericValueFields[base] ?? []; + this.fractionalNumericFields[shard] = this.fractionalNumericFields[base] ?? []; this.autoNumberFields[shard] = this.autoNumberFields[base] ?? []; if (this.dateFields[base]) this.dateFields[shard] = this.dateFields[base]; if (this.datetimeFields[base]) this.datetimeFields[shard] = this.datetimeFields[base]; @@ -11502,6 +11626,7 @@ export class SqlDriver implements IDataDriver { const booleanCols: string[] = []; const numericCols: string[] = []; const numericValueCols: string[] = []; + const fractionalCols: string[] = []; const dateCols: string[] = []; const datetimeCols: string[] = []; const timeCols: string[] = []; @@ -11526,6 +11651,8 @@ export class SqlDriver implements IDataDriver { if (NUMERIC_SCALAR_TYPES.has(type) && !isMultiValuedColumn(type, field)) numericCols.push(name); // [#16318] The authorable half only — see {@link numericValueFields}. if (NUMERIC_VALUE_TYPES.has(type) && !isMultiValuedColumn(type, field)) numericValueCols.push(name); + // [#20387] See {@link fractionalNumericFields}. + if (isFractionalNumericType(type) && !isMultiValuedColumn(type, field)) fractionalCols.push(name); if (type === 'date') dateCols.push(name); if (type === 'datetime') datetimeCols.push(name); if (type === 'time') timeCols.push(name); @@ -11540,6 +11667,7 @@ export class SqlDriver implements IDataDriver { this.booleanFields[key] = booleanCols; this.numericFields[key] = numericCols; this.numericValueFields[key] = numericValueCols; + this.fractionalNumericFields[key] = fractionalCols; this.autoNumberFields[key] = autoNumberCols; this.tenantFieldByTable[key] = tenantField; if (dateCols.length) this.dateFields[key] = new Set(dateCols); @@ -11588,6 +11716,7 @@ export class SqlDriver implements IDataDriver { const booleanCols: string[] = []; const numericCols: string[] = []; const numericValueCols: string[] = []; + const fractionalCols: string[] = []; const autoNumberCols: Array<{ name: string; format: string; tokens: AutonumberToken[]; tenantField: string | null }> = []; // Tenant-isolation column: explicit tenancy opt-out → declared field → // implicit `organization_id`. See {@link computeAndRecordTenantField} @@ -11625,6 +11754,10 @@ export class SqlDriver implements IDataDriver { if (NUMERIC_VALUE_TYPES.has(type) && !isMultiValuedColumn(type, field)) { numericValueCols.push(name); } + // [#20387] See {@link fractionalNumericFields}. + if (isFractionalNumericType(type) && !isMultiValuedColumn(type, field)) { + fractionalCols.push(name); + } if (type === 'date') { (this.dateFields[tableName] ??= new Set()).add(name); } @@ -11649,6 +11782,7 @@ export class SqlDriver implements IDataDriver { this.booleanFields[tableName] = booleanCols; this.numericFields[tableName] = numericCols; this.numericValueFields[tableName] = numericValueCols; + this.fractionalNumericFields[tableName] = fractionalCols; this.autoNumberFields[tableName] = autoNumberCols; this.tenantFieldByTable[tableName] = tenantField; // [#11067] The declared shape's answer to "does this table carry