diff --git a/.changeset/memory-boolean-aggregand.md b/.changeset/memory-boolean-aggregand.md new file mode 100644 index 0000000000..4659cf0346 --- /dev/null +++ b/.changeset/memory-boolean-aggregand.md @@ -0,0 +1,31 @@ +--- +"@objectstack/driver-memory": patch +--- + +**Bug fix (one query, two answers):** `avg` and `sum` over a **boolean** column now answer the same numbers on `driver-memory` that every SQL face and objectql's in-memory fallback already answered, instead of `null` and `0` (#11065). + +Measured on one `AnalyticsService.queryDataset` call, one dataset, one set of rows, run twice — five `crm_case` records, four closed (three with `is_sla_violated: false`, one `true`), one open with `true`: + +``` +[memory] unfiltered {"avg_sla_violated":null, "closed_count":4} +[memory] closed-filter {"avg_sla_violated":null, "closed_count":4} +[sqlite] unfiltered {"avg_sla_violated":0.4, "closed_count":4} +[sqlite] closed-filter {"avg_sla_violated":0.25, "closed_count":4} +``` + +SQLite's are the arithmetically correct numbers (2/5 unfiltered, 1/4 over the closed cases). The `count` measures beside them agreed on both drivers, and so did a `derived` ratio built on those counts — the divergence was specific to averaging a boolean. + +**There were three implementations of "average a column" in play, and this driver was the lone outlier.** SQLite lowers `AVG(col)`; objectql's in-memory fallback (`in-memory-aggregation.ts`) coerces with `Number(v)`, and `Number(true) === 1`; `driver-memory` selected its aggregands with `typeof v === 'number'`, which drops every boolean, leaving `nums.length === 0` and returning `null`. So this is an alignment to the two faces that already agreed, not a new convention. + +**Both of this package's faces carried the defect independently**, and both are fixed: + +- the **data face** (`computeAggregate`, reached by `engine.aggregate` pushdown — the door the report's own repro takes) selected with `typeof v === 'number'`; +- the **analytics face** (`buildAggregator`) emitted a bare mingo `$avg`, and mingo ignores a non-numeric value exactly as MongoDB does — measured at `{avg: null, sum: 0}` over the same five rows. + +Fixing one alone would have left the other free to keep its own answer, which is the shape of this package's recurring defect (#5374, #6814). + +**`sum` is aligned with `avg` deliberately.** The two share the data face's arm, and `SUM(bool)` is "how many true" on every SQL face and in the objectql fallback; correcting only the function the report named would have left the identical defect alive one function over, answering `0` for a column with two `true` rows. + +**The coercion is boolean-only, and the narrowness is the point.** `null`, a missing key and a non-numeric string reach the accumulators unchanged and stay excluded. Adopting the wider half of objectql's `toNumber` — which maps a non-numeric string to `0` — would average garbage as zero rather than excluding it; whether that is right is a separate question this change does not open, and a regression row pins the exclusion on both faces so it cannot arrive by accident. + +**Why it mattered beyond the number.** Neither face errors, so a dashboard tile bound to a rate measure rendered a percentage under SQL and a blank here, indistinguishable from "no matching rows". The test-facing half is worse: a suite pinning such a measure on the in-memory driver asserted against `null` and could not fail in the direction that matters. Nothing in the repo pinned the old value — the shared cross-driver fixture (`AGGREGATION_ROWS`) carries no boolean column at all, which is why every face could disagree here unobserved. diff --git a/packages/drivers/driver-memory/src/memory-analytics.ts b/packages/drivers/driver-memory/src/memory-analytics.ts index d8b2503e34..fcbc021080 100644 --- a/packages/drivers/driver-memory/src/memory-analytics.ts +++ b/packages/drivers/driver-memory/src/memory-analytics.ts @@ -480,6 +480,52 @@ function sizeDistinctSet(values: readonly unknown[]): number { return new Set(values.filter((v) => v !== null && v !== undefined)).size; } +/** + * [#11065] `path`, with a BOOLEAN rendered as the number it is worth — the + * aggregand expression `$sum` and `$avg` consume on this face. + * + * ## What it is for + * + * mingo's `$avg` mirrors MongoDB's and IGNORES a non-numeric value, so a + * whole boolean column averaged to `null` and summed to `0` (measured: five + * bools in, `{avg: null, sum: 0}` out). Under SQL the same rows answer + * `AVG(col)` = 0.4 and `SUM(col)` = 2, and objectql's in-memory fallback + * (`in-memory-aggregation.ts`) answers those numbers too, because its + * `toNumber` is `Number(v)` and `Number(true) === 1`. A rate measure over a + * flag column — an SLA-violation rate, a win rate — is the ordinary shape of + * that query, and the two answers are not two spellings of one: a tile bound + * to the measure renders a percentage on one driver and a blank on the other, + * with no error on either path. + * + * ## Why an EXPRESSION and not post-processing + * + * The `count_distinct` neighbour above collects with `$addToSet` and sizes the + * array after `driver.aggregate` returns ({@link sizeDistinctSet}). Sum and + * average must NOT be built that way: the post-processing step runs after the + * pipeline's own `$sort` and `$limit` stages, so a measure left as an array + * until then would be SORTED as an array — `order` over a `sum` or `avg` + * measure is an ordinary analytics query, unlike ordering by `count_distinct`. + * Keeping the rule inside the `$group` expression leaves every later stage + * looking at the number it expects. + * + * ## The narrowness is deliberate + * + * Only `bool` is rewritten. Everything else — null, missing, a non-numeric + * string — reaches mingo exactly as before and is ignored by the accumulator + * exactly as before (measured: a numeric column carrying a null, a string and + * a missing key answers identically with and without this wrapper). Coercing + * wider would mean adopting `toNumber`'s other half, which maps a non-numeric + * string to `0` and so averages garbage as zero rather than excluding it — + * a separate question from this one. + * + * The data face carries the same rule in JavaScript (`memory-driver.ts`, + * `computeAggregate`); `memory-boolean-aggregand.test.ts` drives both, because + * one face aligned alone is how this package's faces come to disagree. + */ +function numericAggregandExpr(path: string): Record { + return { $cond: [{ $eq: [{ $type: path }, 'bool'] }, { $cond: [path, 1, 0] }, path] }; +} + /** * [#7853] A `JSON.stringify` replacer that renders a `RegExp` operand instead of * dropping it — the one value type the pipeline dump carries that @@ -1192,9 +1238,9 @@ export class MemoryAnalyticsService implements IAnalyticsService { case 'count': return { $sum: 1 }; case 'sum': - return { $sum: `$${fieldPath}` }; + return { $sum: numericAggregandExpr(`$${fieldPath}`) }; case 'avg': - return { $avg: `$${fieldPath}` }; + return { $avg: numericAggregandExpr(`$${fieldPath}`) }; case 'min': return { $min: `$${fieldPath}` }; case 'max': diff --git a/packages/drivers/driver-memory/src/memory-boolean-aggregand.test.ts b/packages/drivers/driver-memory/src/memory-boolean-aggregand.test.ts new file mode 100644 index 0000000000..22fa753b15 --- /dev/null +++ b/packages/drivers/driver-memory/src/memory-boolean-aggregand.test.ts @@ -0,0 +1,303 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11065] `avg` and `sum` over a BOOLEAN column answer the SQL number here too. + * + * ## The measurement this file pins + * + * One `AnalyticsService.queryDataset` call, one dataset, one set of rows, run + * twice — once on `InMemoryDriver`, once on `SqliteWasmDriver`: + * + * ``` + * [memory] unfiltered {"avg_sla_violated":null, "closed_count":4} + * [memory] closed-filter {"avg_sla_violated":null, "closed_count":4} + * [sqlite] unfiltered {"avg_sla_violated":0.4, "closed_count":4} + * [sqlite] closed-filter {"avg_sla_violated":0.25, "closed_count":4} + * ``` + * + * SQLite's are the arithmetically correct numbers (2/5 unfiltered, 1/4 over the + * closed cases). The rows below are that dataset, reduced to the columns the + * divergence needs: five `crm_case`-shaped records, four closed (three with + * `is_sla_violated: false`, one `true`), one open with `true`. + * + * ## Why `null` needed a test rather than a fix alone + * + * `null` and a number are not two spellings of one answer, and neither face + * ERRORS. A dashboard tile bound to a rate measure renders a percentage under + * SQL and a blank here, indistinguishable from "no matching rows". The + * test-facing half is worse and is this file's reason for existing: a suite + * that pins such a measure on the in-memory driver asserts against `null` and + * **cannot fail in the direction that matters**. So every assertion below is + * written against the SQL number, and the `count` control beside it exists so + * that a driver which stopped aggregating altogether — the failure a + * value-only pin would sail through by answering `null` again — is visible. + * + * ## Reverse verification — direction predicted BEFORE it was run + * + * With both coercions reverted to `origin/main`'s expressions + * (`values.filter(v => typeof v === 'number')` on the data face, a bare + * `{ $avg: '$path' }` on the analytics face), predicted: **10 of the 15 fail** (11 measured; see below), + * every one of them on the VALUE `null` or `0` rather than on a throw, because + * both faces drop the aggregands silently. The `count` controls do not survive + * as separate rows — they are asserted beside the rate they control, so the + * rows carrying them go red on the rate — which is the intended reading: the + * control's job is to make a stopped aggregator visible, not to stay green. + * + * Measured: **11 failed / 4 passed.** The predicted DIRECTION held exactly — + * every failure landed on a value (`expected null to be 0.4`, `expected [+0, + * +0] to deeply equal [2, 1]`, and `expected 'object' to be 'number'` where the + * declared-type row read the `null`), not one on a throw. The predicted COUNT + * was off by one: 10 was written, 11 measured. The survivors are the four rows + * the reverted expressions answer identically — the fixture row, one + * non-numeric-text row per face, and the empty-column row — and naming them was + * the half of the prediction worth having, since a survivor list is what + * distinguishes "the pin works" from "the pin is red for some other reason". + * + * No build stands between this file and the mutation: it imports the driver by + * relative path, so vitest runs `src`. That is measured rather than assumed — + * the package's `dist/` was built from `origin/main` before the fix and still + * contains neither coercion, yet the unmutated run is green, which it could not + * be if these assertions were reading `dist`. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import type { DriverQuery } from '@objectstack/spec/contracts'; +import type { Cube } from '@objectstack/spec/data'; +import { InMemoryDriver } from './memory-driver.js'; +import { MemoryAnalyticsService } from './memory-analytics.js'; + +const TABLE = 'crm_case'; + +/** + * The card's dataset. `note` is a non-numeric TEXT column carried alongside on + * purpose: it is the control for the OTHER direction — averaging it must keep + * excluding the values rather than folding them to `0`, which is what adopting + * objectql's `toNumber` wholesale would have done. + */ +const ROWS = [ + { id: 'c1', is_closed: true, is_sla_violated: false, note: 'alpha' }, + { id: 'c2', is_closed: true, is_sla_violated: false, note: 'bravo' }, + { id: 'c3', is_closed: true, is_sla_violated: false, note: 'charlie' }, + { id: 'c4', is_closed: true, is_sla_violated: true, note: 'delta' }, + { id: 'c5', is_closed: false, is_sla_violated: true, note: 'echo' }, +] as const; + +const CLOSED = { is_closed: true }; + +async function seed(): Promise { + const driver = new InMemoryDriver(); + for (const row of ROWS) await driver.create(TABLE, { ...row }); + return driver; +} + +/** + * The measures the card reported, as the `DriverQuery` both data-face doors + * consume: the rate under test, and the `count` that agreed across drivers. + */ +const query = (where?: Record): DriverQuery => ({ + ...(where ? { where } : {}), + aggregations: [ + { function: 'avg', field: 'is_sla_violated', alias: 'avg_sla_violated' }, + { function: 'sum', field: 'is_sla_violated', alias: 'sum_sla_violated' }, + { function: 'count', field: 'id', alias: 'row_count' }, + ], +}); + +describe('[#11065] InMemoryDriver data face — a boolean aggregand is worth 1 or 0', () => { + let driver: InMemoryDriver; + beforeEach(async () => { driver = await seed(); }); + + /** + * The fixture first, read back rather than trusted: booleans stored as the + * strings `'true'`/`'false'` would make every assertion below pass through a + * path that has nothing to do with the defect. + */ + it('the fixture is five rows whose flags are stored AS booleans', async () => { + const rows = await driver.find(TABLE, { orderBy: [{ field: 'id', order: 'asc' }] }) as any[]; + expect(rows.map((r) => r.id)).toEqual(['c1', 'c2', 'c3', 'c4', 'c5']); + for (const r of rows) expect(typeof r.is_sla_violated, r.id).toBe('boolean'); + expect(rows.filter((r) => r.is_sla_violated)).toHaveLength(2); + expect(rows.filter((r) => r.is_closed)).toHaveLength(4); + }); + + /** + * `aggregate(object, AST)` is the door the card's own repro reaches: the + * analytics strategy's `executeAggregate` bridge calls `engine.aggregate`, + * which pushes the aggregate down to `driver.aggregate` whenever the driver + * has the method and the query needs no in-memory bucketing — traced on this + * dataset, not inferred. + */ + it('aggregate(AST): avg over the boolean is 0.4, the number SQLite answers', async () => { + const [row] = await driver.aggregate(TABLE, query()) as any[]; + expect(row.avg_sla_violated).toBe(0.4); + // The control: a face that stopped aggregating cannot keep this green. + expect(row.row_count).toBe(5); + }); + + it('aggregate(AST): avg under the closed filter is 0.25', async () => { + const [row] = await driver.aggregate(TABLE, query(CLOSED)) as any[]; + expect(row.avg_sla_violated).toBe(0.25); + expect(row.row_count).toBe(4); + }); + + /** + * The SECOND door onto the same `performAggregation`. Two doors that can + * disagree is this package's recurring defect class (#5374, #6814), so + * neither stands for the other. + */ + it('find(): the same two numbers through the other door', async () => { + const [all] = await driver.find(TABLE, query()) as any[]; + const [closed] = await driver.find(TABLE, query(CLOSED)) as any[]; + expect([all.avg_sla_violated, closed.avg_sla_violated]).toEqual([0.4, 0.25]); + expect([all.row_count, closed.row_count]).toEqual([5, 4]); + }); + + /** + * `sum` shares the arm, and was left answering `0` — "how many true" is what + * `SUM(col)` means on every SQL face and in objectql's in-memory fallback, so + * aligning `avg` alone would have kept the same defect alive one function + * over. Pinned in both directions of the filter for the same reason the `avg` + * rows are. + */ + it('sum over the boolean counts the true rows — 2 unfiltered, 1 closed', async () => { + const [all] = await driver.aggregate(TABLE, query()) as any[]; + const [closed] = await driver.aggregate(TABLE, query(CLOSED)) as any[]; + expect([all.sum_sla_violated, closed.sum_sla_violated]).toEqual([2, 1]); + }); + + /** Grouped, because a face aggregating the whole table and repeating the + * result per group answers 0.4/0.4 and the ungrouped rows cannot see it. */ + it('grouped by is_closed: 0.25 closed / 1 open, counts 4 and 1', async () => { + const rows = await driver.aggregate(TABLE, { + groupBy: ['is_closed'], + aggregations: [ + { function: 'avg', field: 'is_sla_violated', alias: 'rate' }, + { function: 'count', field: 'id', alias: 'row_count' }, + ], + }) as any[]; + const byClosed = Object.fromEntries(rows.map((r) => [String(r.is_closed), [r.rate, r.row_count]])); + expect(byClosed).toEqual({ true: [0.25, 4], false: [1, 1] }); + }); + + /** The declared answer is a NUMBER — `null` under a numeric measure is the + * shape the card measured, and a value comparison alone reports it as an + * ordinary wrong number rather than as the missing answer it is. */ + it('answers a number, never null, for a column that has rows', async () => { + const [row] = await driver.aggregate(TABLE, query()) as any[]; + expect(row.avg_sla_violated).not.toBeNull(); + expect(typeof row.avg_sla_violated).toBe('number'); + }); + + /** + * The narrowness, asserted rather than assumed. Coercion is boolean-only: + * a non-numeric TEXT column keeps being EXCLUDED, so `avg` over it stays + * `null` and `sum` stays `0`. Averaging those strings as `0` — what + * `toNumber` does — would be a different and much louder change, and this row + * is what stops it arriving by accident. + */ + it('a non-numeric text column is still excluded, not folded to zero', async () => { + const [row] = await driver.aggregate(TABLE, { + aggregations: [ + { function: 'avg', field: 'note', alias: 'avg_note' }, + { function: 'sum', field: 'note', alias: 'sum_note' }, + ], + }) as any[]; + expect(row.avg_note).toBeNull(); + expect(row.sum_note).toBe(0); + }); + + /** An empty column still has no average — the coercion adds aggregands, it + * does not invent one. */ + it('avg over a column with no rows at all is still null', async () => { + const [row] = await driver.aggregate(TABLE, { + where: { id: 'nobody' }, + aggregations: [{ function: 'avg', field: 'is_sla_violated', alias: 'rate' }], + }) as any[]; + expect(row.rate).toBeNull(); + }); +}); + +/** + * The ANALYTICS face answers the same measure the same way. + * + * It reaches the numbers by a different route — a mingo `$group` expression + * rather than JavaScript — and mingo's `$avg` ignores a non-numeric value + * exactly as MongoDB's does, so before #11065 this face had the identical + * divergence on its own account: `{avg: null, sum: 0}` over the same five rows. + * Aligning the data face alone would have left it free to keep that answer, + * which is the same mistake #6814 recorded on `count_distinct`. + */ +describe('[#11065] the analytics face answers the same rate', () => { + const cube = { + name: 'cases', + title: 'Cases', + sql: TABLE, + measures: { + slaViolationRate: { name: 'sla_violation_rate', label: 'SLA Violation Rate', type: 'avg', sql: 'is_sla_violated' }, + slaViolations: { name: 'sla_violations', label: 'SLA Violations', type: 'sum', sql: 'is_sla_violated' }, + count: { name: 'count', label: 'Cases', type: 'count', sql: 'id' }, + avgNote: { name: 'avg_note', label: 'Avg note', type: 'avg', sql: 'note' }, + }, + dimensions: { + isClosed: { name: 'is_closed', label: 'Closed', type: 'boolean', sql: 'is_closed' }, + }, + } as unknown as Cube; + + let service: MemoryAnalyticsService; + + beforeEach(async () => { + const driver = await seed(); + service = new MemoryAnalyticsService({ driver, cubes: [cube] }); + }); + + const run = async (where?: Record) => { + const result = await service.query({ + cube: 'cases', + measures: ['cases.slaViolationRate', 'cases.slaViolations', 'cases.count'], + ...(where ? { where } : {}), + } as any); + return result.rows[0] as Record; + }; + + it('avg over the boolean is 0.4 unfiltered, with the count control at 5', async () => { + const row = await run(); + expect(row['cases.slaViolationRate']).toBe(0.4); + expect(row['cases.count']).toBe(5); + }); + + it('avg over the boolean is 0.25 under the closed filter, count 4', async () => { + const row = await run(CLOSED); + expect(row['cases.slaViolationRate']).toBe(0.25); + expect(row['cases.count']).toBe(4); + }); + + it('sum over the boolean counts the true rows here too — 2 and 1', async () => { + expect([(await run())['cases.slaViolations'], (await run(CLOSED))['cases.slaViolations']]).toEqual([2, 1]); + }); + + /** Grouped, for the reason the data face's grouped row states. */ + it('grouped by is_closed: 0.25 closed / 1 open', async () => { + const result = await service.query({ + cube: 'cases', + measures: ['cases.slaViolationRate', 'cases.count'], + dimensions: ['cases.isClosed'], + } as any); + const byClosed = Object.fromEntries( + (result.rows as any[]).map((r) => [String(r['cases.isClosed']), [r['cases.slaViolationRate'], r['cases.count']]]), + ); + expect(byClosed).toEqual({ true: [0.25, 4], false: [1, 1] }); + }); + + /** The same narrowness guard the data face carries: text stays excluded. */ + it('a non-numeric text column is still excluded here too', async () => { + const result = await service.query({ cube: 'cases', measures: ['cases.avgNote'] } as any); + expect((result.rows[0] as Record)['cases.avgNote']).toBeNull(); + }); + + /** The response describes the measure as `number`; the cell must be one. */ + it('answers a number, matching the field type the response declares', async () => { + const result = await service.query({ cube: 'cases', measures: ['cases.slaViolationRate'] } as any); + expect((result.fields as any[]).find((f) => f.name === 'cases.slaViolationRate')?.type).toBe('number'); + expect(typeof (result.rows[0] as Record)['cases.slaViolationRate']).toBe('number'); + }); +}); diff --git a/packages/drivers/driver-memory/src/memory-driver.ts b/packages/drivers/driver-memory/src/memory-driver.ts index 5a6de1ee66..589a3b9e3d 100644 --- a/packages/drivers/driver-memory/src/memory-driver.ts +++ b/packages/drivers/driver-memory/src/memory-driver.ts @@ -1242,9 +1242,42 @@ export class InMemoryDriver implements IDataDriver { if (!field || field === '*') return records.length; return values.filter(v => v !== null && v !== undefined).length; + // [#11065] A BOOLEAN is an aggregand worth 1 or 0 — not a value to + // drop. `typeof v === 'number'` dropped every one of them, so a + // whole boolean column aggregated to `nums.length === 0` and `avg` + // returned `null` while `sum` returned `0`. That is one query with + // two answers: `AVG(col)`/`SUM(col)` over the same rows on SQLite + // answer `0.4` and `2`, and objectql's in-memory fallback + // (`in-memory-aggregation.ts`) answers the same numbers because its + // `toNumber` is `Number(v)` and `Number(true) === 1`. driver-memory + // was the lone outlier of the three, and the divergence is silent on + // both faces: a dashboard tile renders a rate under SQL and a blank + // here, indistinguishable from "no matching rows", while an + // in-memory suite pinning such a measure asserts `null` and cannot + // fail in the direction that matters. + // + // `sum` is coerced with `avg` deliberately rather than left one arm + // over: `SUM(bool)` is "how many true" on every SQL face and in the + // objectql fallback, and fixing only the arm the report named would + // have kept the same defect alive under a different function name. + // + // The coercion is BOOLEAN-ONLY, and that narrowness is the point. + // `toNumber` also maps a non-numeric STRING to `0`, which averages + // garbage as zero rather than excluding it; whether that is right is + // a separate question, so everything that is not a boolean reaches + // the same `typeof === 'number'` gate it always did. + // + // The analytics face carries this rule as a mingo expression + // (`memory-analytics.ts`, `buildAggregator`) — same numbers, other + // language. Both are driven by `memory-boolean-aggregand.test.ts`, + // because "the faces disagree" is this package's recurring defect + // class (#5374, #6814) and one face aligned alone leaves the other + // free to keep its own answer. case 'sum': case 'avg': { - const nums = values.filter(v => typeof v === 'number'); + const nums = values + .map(v => (typeof v === 'boolean' ? (v ? 1 : 0) : v)) + .filter(v => typeof v === 'number'); const sum = nums.reduce((a, b) => a + b, 0); if (func === 'sum') return sum; return nums.length > 0 ? sum / nums.length : null;