diff --git a/.changeset/analytics-empty-group-fill-compare-seam.md b/.changeset/analytics-empty-group-fill-compare-seam.md new file mode 100644 index 0000000000..d88b67f712 --- /dev/null +++ b/.changeset/analytics-empty-group-fill-compare-seam.md @@ -0,0 +1,50 @@ +--- +"@objectstack/service-analytics": minor +--- + +fix(service-analytics): a measure a query never reported reads 0 for a count/sum on every merge seam (#4708) + +A dataset measure carrying its own `filter` runs as a separate grouped +sub-query and is merged back onto the selected dimensions. A `GROUP BY` over a +filtered row set emits **no group at all** for a dimension value the filter +excludes entirely, so the measure comes back **absent**, not `0` — and +`computeDerived` treats an absent operand as unknowable, so every ratio over it +goes null too. The cell then renders blank, which is visually identical to "no +data for this row" and means the opposite. + +The bias runs the worst possible way: the rows that blank are the ones whose +numerator matched nothing — the **worst-performing rows**. A `lead_source` that +won nothing rendered as "no data" while one that won everything rendered fine. + +The empty-group value is now filled **by aggregate kind** into every measure +column the assembled grid lists but no query reported: + +| aggregate | over an excluded group | why | +|:---|:---|:---| +| `count`, `count_distinct` | `0` | "how many rows matched" has an exact answer when the answer is none | +| `sum` | `0` | the identity element of the empty set | +| `avg`, `min`, `max` | stays `null` | genuinely undefined — there is nothing to average | + +Filling all five with `0` would trade this lie for its mirror image, reporting a +measurement nobody made, so the kinds are judged separately (via +`emptyGroupValueFor`, shared with the authoring-side coherence checks). + +**Only cells are filled, never rows.** A dimension value no query reported at +all has genuinely no data and stays out of the grid. + +**What changes beyond the measure-scoped seam.** The fill previously ran before +the `compareTo` merge, and that merge *appends* a row for every bucket the +PREVIOUS window had and this one does not. Every base measure on those rows — +including unfiltered ones — was absent, so a lead source that sold last month +and nothing this month rendered as "no data" instead of `0`: the same worst-row +bias, one merge later. The fill now runs after every merge and covers all base +measures plus their `__compare` columns. + +Widgets that worked around this with `?? 0` in the consumer or a `coalesce` in +the measure can drop it; the coercion belongs in the executor, which is the only +layer that knows which aggregate produced the gap. + +**New export.** `fillEmptyGroups(rows, columnAggregates)` is exported from the +package root beside `mergeByDimensions`, so a host assembling a grid outside +`DatasetExecutor` can apply the same aggregate-kind rule rather than +reimplementing it — which is what makes this a `minor` rather than a `patch`. diff --git a/packages/services/service-analytics/src/__tests__/dataset-empty-group-fill.test.ts b/packages/services/service-analytics/src/__tests__/dataset-empty-group-fill.test.ts new file mode 100644 index 0000000000..cbbaf9326b --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/dataset-empty-group-fill.test.ts @@ -0,0 +1,273 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Empty-group fill (#4708, objectui#3136). + * + * A measure carrying its own `filter` runs as a separate grouped sub-query and + * is merged back by dimension key. A `GROUP BY` over a filtered row set emits + * NO group for a dimension value the filter excludes entirely, so the measure + * comes back ABSENT rather than `0` — and a derived ratio over an absent + * operand goes null, so the cell renders blank: visually identical to "no data + * for this row", which is the opposite of what the row means. + * + * The bias is what makes it worth a dedicated suite: the rows that blank are + * exactly the WORST-performing ones (nothing matched the numerator's filter), + * while a row that matched everything renders fine. A dashboard that hides its + * worst rows and shows its best is the least acceptable direction for the + * error to run, so `renders the worst row …` below asserts the asymmetry + * directly rather than only the individual cells. + * + * The fill is strictly by aggregate kind — over-filling would trade this lie + * for its mirror image (an `avg` of nothing reported as 0 is a measurement + * nobody made), so `does NOT fill avg/min/max` pins the other side. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { IAnalyticsService, AnalyticsQuery, AnalyticsResult } from '@objectstack/spec/contracts'; +import { DatasetSchema } from '@objectstack/spec/ui'; +import { compileDataset } from '../dataset-compiler.js'; +import { DatasetExecutor, fillEmptyGroups } from '../dataset-executor.js'; + +function fakeService(handler: (q: AnalyticsQuery) => AnalyticsResult): IAnalyticsService { + return { query: vi.fn(async (q: AnalyticsQuery) => handler(q)), getMeta: async () => [] }; +} + +// ── the issue's reproduction, verbatim (hotcrm#593 / hotcrm#656) ───────────── + +const winRate = DatasetSchema.parse({ + name: 'pipeline', label: 'Pipeline', object: 'opportunity', + dimensions: [{ name: 'lead_source', field: 'lead_source', type: 'string' }], + measures: [ + { name: 'won_count', aggregate: 'count', filter: { stage: 'closed_won' } }, + { name: 'lost_count', aggregate: 'count', filter: { stage: 'closed_lost' } }, + { name: 'decided_count', aggregate: 'count', filter: { stage: { $in: ['closed_won', 'closed_lost'] } } }, + { name: 'win_rate', derived: { op: 'ratio', of: ['won_count', 'decided_count'] } }, + ], +}); + +/** + * The four lead sources of the issue's table. Each sub-query returns only the + * groups its filter left non-empty — which is what a real `GROUP BY` does: + * - `partner` won everything → absent from the `lost_count` result + * - `cold_call` won nothing → absent from the `won_count` result + */ +const winRateService = fakeService((q) => { + switch (q.measures[0]) { + case 'won_count': + return { rows: [ + { lead_source: 'content', won_count: 2 }, + { lead_source: 'referral', won_count: 1 }, + { lead_source: 'partner', won_count: 1 }, + ], fields: [] }; + case 'lost_count': + return { rows: [ + { lead_source: 'content', lost_count: 1 }, + { lead_source: 'referral', lost_count: 1 }, + { lead_source: 'cold_call', lost_count: 1 }, + ], fields: [] }; + case 'decided_count': + return { rows: [ + { lead_source: 'content', decided_count: 3 }, + { lead_source: 'referral', decided_count: 2 }, + { lead_source: 'partner', decided_count: 1 }, + { lead_source: 'cold_call', decided_count: 1 }, + ], fields: [] }; + default: + return { rows: [], fields: [] }; + } +}); + +const runWinRate = () => + new DatasetExecutor(winRateService).execute(compileDataset(winRate), { + dimensions: ['lead_source'], + measures: ['won_count', 'lost_count', 'decided_count', 'win_rate'], + }); + +describe('empty-group fill — a filtered count that matched nothing (#4708)', () => { + it('reports 0 (not blank) for the group its filter excluded, so the ratio computes', async () => { + const rows = (await runWinRate()).rows; + const cold = rows.find((r) => r.lead_source === 'cold_call')!; + // cold_call won nothing and lost one. The correct answer is 0%, not blank. + expect(cold.won_count).toBe(0); + expect(cold.decided_count).toBe(1); + expect(cold.win_rate).toBe(0); + }); + + it('fills the mirror gap too — a source that never lost reads 0 losses', async () => { + const rows = (await runWinRate()).rows; + expect(rows.find((r) => r.lead_source === 'partner')).toMatchObject({ + won_count: 1, lost_count: 0, decided_count: 1, win_rate: 1, + }); + }); + + it('renders the worst row exactly as legibly as the best one (the asymmetry)', async () => { + const rows = (await runWinRate()).rows; + // The defect's signature: `partner` (won everything) rendered fine while + // `cold_call` (won nothing) rendered blank — the dashboard hid its worst + // row. No row may come back with an unmeasured cell. + for (const row of rows) { + expect(row.win_rate, `win_rate blank for ${row.lead_source}`).not.toBeNull(); + expect(row.win_rate).toEqual(expect.any(Number)); + for (const m of ['won_count', 'lost_count', 'decided_count']) { + expect(row[m], `${m} blank for ${row.lead_source}`).toEqual(expect.any(Number)); + } + } + // …and the worst row is the one the reader must be able to act on. + const byRate = [...rows].sort((a, b) => Number(a.win_rate) - Number(b.win_rate)); + expect(byRate[0]).toMatchObject({ lead_source: 'cold_call', win_rate: 0 }); + expect(byRate[byRate.length - 1]).toMatchObject({ lead_source: 'partner', win_rate: 1 }); + }); + + it('invents no group — a dimension value no query reported stays out of the grid', async () => { + const rows = (await runWinRate()).rows; + // Only the four sources some sub-query actually returned. `webinar` has no + // opportunities at all, so it is absent from every result and must not be + // materialised as a row of zeroes. + expect(rows.map((r) => r.lead_source).sort()).toEqual(['cold_call', 'content', 'partner', 'referral']); + }); +}); + +// ── the other side: aggregates with no answer over an empty set ────────────── + +const amounts = DatasetSchema.parse({ + name: 'deals', label: 'Deals', object: 'opportunity', + dimensions: [{ name: 'lead_source', field: 'lead_source', type: 'string' }], + measures: [ + { name: 'won_count', aggregate: 'count', filter: { stage: 'closed_won' } }, + { name: 'won_total', aggregate: 'sum', field: 'amount', filter: { stage: 'closed_won' } }, + { name: 'won_avg', aggregate: 'avg', field: 'amount', filter: { stage: 'closed_won' } }, + { name: 'won_min', aggregate: 'min', field: 'amount', filter: { stage: 'closed_won' } }, + { name: 'won_max', aggregate: 'max', field: 'amount', filter: { stage: 'closed_won' } }, + { name: 'all_count', aggregate: 'count' }, + ], +}); + +describe('empty-group fill — strictly by aggregate kind (#4708)', () => { + it('does NOT fill avg/min/max: nothing to average over an empty group', async () => { + const svc = fakeService((q) => { + if (q.measures.includes('all_count')) { + return { rows: [ + { lead_source: 'content', all_count: 4 }, + { lead_source: 'cold_call', all_count: 1 }, + ], fields: [] }; + } + // every won_* sub-query: cold_call won nothing, so no group for it + const m = q.measures[0]; + return { rows: [{ lead_source: 'content', [m]: 7 }], fields: [] }; + }); + const res = await new DatasetExecutor(svc).execute(compileDataset(amounts), { + dimensions: ['lead_source'], + measures: ['all_count', 'won_count', 'won_total', 'won_avg', 'won_min', 'won_max'], + }); + const cold = res.rows.find((r) => r.lead_source === 'cold_call')!; + // Measured facts: no rows matched, so the count is 0 and the sum is 0. + expect(cold.won_count).toBe(0); + expect(cold.won_total).toBe(0); + // Genuinely unknown — filling these would report a measurement nobody made + // ("average deal size 0" is a different claim from "no deals"). + for (const m of ['won_avg', 'won_min', 'won_max']) { + expect(cold[m] ?? null, `${m} must stay null`).toBeNull(); + expect(cold[m], `${m} must not be flattened to 0`).not.toBe(0); + } + // The group that did have data is untouched. + expect(res.rows.find((r) => r.lead_source === 'content')).toMatchObject({ + won_count: 7, won_total: 7, won_avg: 7, won_min: 7, won_max: 7, + }); + }); +}); + +// ── the compareTo seam: rows APPENDED by the comparison merge ──────────────── + +const compareDs = DatasetSchema.parse({ + name: 'trend', label: 'Trend', object: 'opportunity', + dimensions: [ + { name: 'lead_source', field: 'lead_source', type: 'string' }, + { name: 'close_date', field: 'close_date', type: 'date' }, + ], + measures: [ + { name: 'revenue', aggregate: 'sum', field: 'amount' }, + { name: 'avg_deal', aggregate: 'avg', field: 'amount' }, + { name: 'won_count', aggregate: 'count', filter: { stage: 'closed_won' } }, + ], +}); + +describe('empty-group fill — buckets the comparison window added (#4708)', () => { + it('fills every base measure on a row only the previous period produced', async () => { + const svc = fakeService((q) => { + const shifted = JSON.stringify(q.timeDimensions ?? []).includes('2025-12'); + if (shifted) { + return { rows: [ + { lead_source: 'content', revenue: 80, avg_deal: 40, won_count: 2 }, + { lead_source: 'cold_call', revenue: 10, avg_deal: 10, won_count: 1 }, + ], fields: [] }; + } + if (q.measures.includes('won_count')) { + return { rows: [{ lead_source: 'content', won_count: 3 }], fields: [] }; + } + // this period, `cold_call` has no opportunities at all + return { rows: [{ lead_source: 'content', revenue: 100, avg_deal: 50 }], fields: [] }; + }); + const res = await new DatasetExecutor(svc).execute(compileDataset(compareDs), { + dimensions: ['lead_source'], + measures: ['revenue', 'avg_deal', 'won_count'], + timeDimensions: [{ dimension: 'close_date', dateRange: ['2026-01-01', '2026-01-31'] }], + compareTo: { kind: 'previousPeriod', dimension: 'close_date' }, + }); + const cold = res.rows.find((r) => r.lead_source === 'cold_call')!; + // The row exists because the comparison window had data for it; "how much + // did cold_call sell this period" therefore has an exact answer — none. + // Before #4708 the fill ran BEFORE this merge, so all three read blank. + expect(cold.revenue).toBe(0); + expect(cold.won_count).toBe(0); + expect(cold.avg_deal ?? null).toBeNull(); + // the comparison columns are untouched — they were reported + expect(cold).toMatchObject({ revenue__compare: 10, won_count__compare: 1 }); + }); + + it('fills a comparison column the previous window never reported', async () => { + const svc = fakeService((q) => { + const shifted = JSON.stringify(q.timeDimensions ?? []).includes('2025-12'); + // the previous period knew nothing of `content` + if (shifted) return { rows: [], fields: [] }; + if (q.measures.includes('won_count')) { + return { rows: [{ lead_source: 'content', won_count: 3 }], fields: [] }; + } + return { rows: [{ lead_source: 'content', revenue: 100, avg_deal: 50 }], fields: [] }; + }); + const res = await new DatasetExecutor(svc).execute(compileDataset(compareDs), { + dimensions: ['lead_source'], + measures: ['revenue', 'avg_deal', 'won_count'], + timeDimensions: [{ dimension: 'close_date', dateRange: ['2026-01-01', '2026-01-31'] }], + compareTo: { kind: 'previousPeriod', dimension: 'close_date' }, + }); + expect(res.rows[0]).toMatchObject({ + revenue: 100, won_count: 3, + // "sold nothing last month" — a fact, not a gap + revenue__compare: 0, won_count__compare: 0, + }); + expect(res.rows[0].avg_deal__compare ?? null).toBeNull(); + }); +}); + +// ── the helper in isolation ───────────────────────────────────────────────── + +describe('fillEmptyGroups', () => { + it('fills count/count_distinct/sum, leaves avg/min/max and reported values alone', () => { + const rows = [ + { g: 'a', c: 2, d: 1, s: 5, avg: 2.5, min: 1, max: 4 }, + { g: 'b' }, + ]; + fillEmptyGroups(rows, { + c: 'count', d: 'count_distinct', s: 'sum', avg: 'avg', min: 'min', max: 'max', + unknown: undefined, + }); + expect(rows[0]).toEqual({ g: 'a', c: 2, d: 1, s: 5, avg: 2.5, min: 1, max: 4 }); + expect(rows[1]).toEqual({ g: 'b', c: 0, d: 0, s: 0 }); + }); + + it('touches only the columns it is given, and never adds rows', () => { + const rows = [{ g: 'a' }]; + expect(fillEmptyGroups(rows, { c: 'count' })).toHaveLength(1); + expect(rows[0]).toEqual({ g: 'a', c: 0 }); + }); +}); diff --git a/packages/services/service-analytics/src/dataset-executor.ts b/packages/services/service-analytics/src/dataset-executor.ts index 4bbd296e68..3f153f7a70 100644 --- a/packages/services/service-analytics/src/dataset-executor.ts +++ b/packages/services/service-analytics/src/dataset-executor.ts @@ -26,6 +26,8 @@ export type CompareTo = DatasetCompareTo; * runtime, then post-processes the results: * - resolves the base measures a selection needs (including derived deps), * - applies measure-scoped filters via supplementary grouped queries, + * - fills the empty-group value into columns no query reported, by aggregate + * kind (#4708) — a count/sum over an excluded group is 0, avg/min/max null, * - evaluates derived measures (ratio/sum/difference/product) row-by-row (Q1), * - shifts the query for `compareTo` (previousPeriod / previousYear) and * attaches `__compare` columns, @@ -149,6 +151,61 @@ export function evaluateDerivedMeasures( }); } +/** + * Fill the EMPTY-GROUP value into every measure column the assembled grid + * LISTS but no query REPORTED — by aggregate kind (#4708, objectui#3136). + * + * The grid is assembled from several results: the primary query, one + * supplementary query per measure-scoped filter, and (for `compareTo`) a + * shifted pass. {@link mergeByDimensions} writes a measure's column only onto + * rows its source result returned, and a `GROUP BY` over a filtered row set + * emits NO group at all for a dimension value the filter excludes entirely. + * The column therefore comes back **absent**, not `0` — and absent renders as + * "no data for this row", which for a count is the opposite of what the row + * means. A derived ratio over it goes null as well ({@link computeDerived} + * treats a missing operand as unknowable), so the blank spreads. + * + * The bias runs the worst possible way: the rows that blank are the ones whose + * numerator the filter excluded — the WORST-performing rows. A `lead_source` + * that won nothing renders as "no data" while one that won everything renders + * fine. + * + * **Filled strictly by aggregate kind**, never wholesale. `count` / + * `count_distinct` over an excluded group is unambiguously `0` ("how many rows + * matched" has an exact answer when the answer is none), and `sum` over the + * empty set is its identity `0`. `avg` / `min` / `max` are genuinely null — + * there is nothing to average — and flattening those to `0` would trade this + * lie for the opposite one, reporting a measurement nobody made. The + * kind→identity mapping is `emptyGroupValueFor` in `@objectstack/spec/data`, + * shared with the authoring-side coherence checks so the two cannot drift. + * + * **Only rows that already exist are touched** — no group is invented. A + * dimension value no query reported at all has genuinely no data and stays out + * of the grid; this fills the cell, never the row. + * + * Deliberately NOT a `?? 0` in the widget or a `coalesce` in the measure: a + * consumer-side patch must be repeated by every author of every ratio widget + * forever, and forgetting it is silent. Only the executor knows which aggregate + * produced the gap, so only the executor can tell `0` from unknown. + * + * Mutates `rows` in place (they are already this pipeline's own copies) and + * returns them for chaining. + * + * @param columnAggregates - Grid column → the aggregate that produced it. + * Includes `__compare` columns, which merge through the same seam. + */ +export function fillEmptyGroups( + rows: Record[], + columnAggregates: Record, +): Record[] { + for (const [column, aggregate] of Object.entries(columnAggregates)) { + const empty = emptyGroupValueFor(aggregate); + if (empty === undefined) continue; + for (const row of rows) if (row[column] == null) row[column] = empty; + } + return rows; +} + function num(v: unknown): number | null { if (v == null) return null; const n = typeof v === 'number' ? v : Number(v); @@ -554,23 +611,6 @@ export class DatasetExecutor { result.fields.push({ name: m, type: 'number' }); } - // A measure-scoped filter can exclude EVERY row of a group the grid still - // lists, and the database reports that by omitting the group from the - // sub-result — indistinguishable, after the merge, from "not measured". - // For a count or a sum it IS measured: the answer is 0. Fill it in, so a - // "0 of 12 paid" group reads as 0 rather than blank and any ratio built on - // it computes instead of going null (objectui#3136). avg/min/max keep their - // null — there is nothing to average over an empty group. - // - // Runs after ALL supplementary merges, not inside the loop: a later - // measure's merge can append rows for dimension keys no earlier query saw, - // and those rows need the same fill. - for (const m of filtered) { - const empty = emptyGroupValueFor(compiled.cube.measures?.[m]?.type); - if (empty === undefined) continue; - for (const row of result.rows) if (row[m] == null) row[m] = empty; - } - // compareTo — run a shifted query over the same base measures and attach. if (selection.compareTo) { const compareRows = await this.runCompare(compiled, selection, [...baseMeasures], dimensions, baseFilter, context); @@ -583,6 +623,34 @@ export class DatasetExecutor { for (const m of baseMeasures) result.fields.push({ name: `${m}__compare`, type: 'number' }); } + // Empty-group fill (#4708) — a group a query never reported reads `0` for a + // count/sum and stays null for avg/min/max. See {@link fillEmptyGroups} for + // why this belongs to the executor and not to every widget author. + // + // Placed after EVERY merge and before the derived pass, because each merge + // is a place a column can go missing and `mergeByDimensions` APPENDS rows: + // - a supplementary measure-scoped query omits the groups its filter + // excluded (the "won nothing" rows — the original defect); + // - a later supplementary query can append rows for dimension keys no + // earlier query saw, and those need the same fill; + // - the compareTo pass appends a row for every bucket that existed in the + // PREVIOUS window and not in this one, on which *every* base measure is + // absent — including unfiltered ones, which is why the fill covers all + // base measures rather than only the filter-scoped ones. + // Running it before the compare merge left that last class blank, so a lead + // source that sold last month and nothing this month rendered as "no data" + // instead of 0 — the same worst-row bias, one merge later. + // + // Derived measures are evaluated AFTER, so a ratio over a filled 0 computes + // (0%) instead of being poisoned by an absent operand. + const fillColumns: Record = {}; + for (const m of baseMeasures) { + const aggregate = compiled.cube.measures?.[m]?.type; + fillColumns[m] = aggregate; + if (selection.compareTo) fillColumns[`${m}__compare`] = aggregate; + } + fillEmptyGroups(result.rows, fillColumns); + // Derived measures (computed from base + compare columns already present). result.rows = evaluateDerivedMeasures(result.rows, selectedDerived); for (const d of selectedDerived) result.fields.push({ name: d.name, type: 'number' }); diff --git a/packages/services/service-analytics/src/index.ts b/packages/services/service-analytics/src/index.ts index 4b72b4d6dc..f26f0188c3 100644 --- a/packages/services/service-analytics/src/index.ts +++ b/packages/services/service-analytics/src/index.ts @@ -28,6 +28,7 @@ export { combineFilters, shiftRange, mergeByDimensions, + fillEmptyGroups, } from './dataset-executor.js'; export type { DatasetSelection, CompareTo } from './dataset-executor.js'; export { compileScopedFilterToSql } from './read-scope-sql.js';