Skip to content

Commit fd5cff2

Browse files
os-trumpclaude
andauthored
fix(service-analytics): draft preview counts a declared field's non-null values (#16218) (#17131)
* fix(service-analytics): draft preview counts a declared field's non-null values (#16218) `aggregate()` in `preview-evaluator.ts` opened with if (metricType === 'count' || field === '*') return rows.length; so a dataset measure `{ aggregate: 'count', field: 'payer' }` — lowered by `dataset-compiler` to the cube metric `{ type: 'count', sql: 'payer' }` — had its declared FIELD carried in and then never read. The preview answered the ROW count, nulls included, while every SQL face lowers the same measure to `COUNT("payer")`, defined over non-null values (`AGGREGATE_SQL.count`, `native-sql-strategy.ts`, #10298). A drafted chart showed a different number than the published one, silently, and it showed the number `count(*)` gives — so the author's choice to count a specific column had no effect on this path. One arm, beside the `count_distinct` arm #16203 restored on the same function: the `*` test moves ahead of it (unchanged for every metric type), and `count` over a declared column counts its non-null values. Reproduced and closed through the differential harness #16203 built — one dataset, one row set, two `AnalyticsService` instances differing only in `draftRowsResolver`, the live half being `NativeSQLStrategy`'s generated SQL executed on a real SQLite (sql.js) seeded from the same rows. Before: `payer_count` 1 live / 2 preview on the card's cell, 2/3 and 0/2 on the two others. After: equal on every cell. Controls held by the same fixture, which keeps `count(*)`, `count(field)` and `count_distinct(field)` three different numbers per group: both `count(*)` spellings still answer the row count, `count_distinct` still answers a cardinality, and a group where no row carries a value counts `0` rather than null — `emptyGroupValueFor` rules counting nothing the identity `0`. No behaviour change on the live path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * docs(changeset): argue the patch bump rather than defaulting to it State in the changeset why this lands `patch` and not `minor`: the package's published surface is byte-unchanged (`src/index.ts` is not in the diff, `aggregate()` is module-private, `evaluateAnalyticsQueryOverRows` is not on the barrel), and the only user-visible effect is a drafted chart's number moving to the number the published chart already showed — a correction toward the live standard, not the backwards-compatible feature addition `minor` denotes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent df8a16d commit fd5cff2

3 files changed

Lines changed: 359 additions & 3 deletions

File tree

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
---
2+
'@objectstack/service-analytics': patch
3+
---
4+
5+
Draft-preview analytics: `count` over a declared field counts its non-null values, matching every live face
6+
7+
A dataset measure `{ aggregate: 'count', field: 'payer' }` compiles to the cube
8+
metric `{ type: 'count', sql: 'payer' }`, and the draft-preview evaluator carried
9+
that field in and never read it — it answered the ROW count, nulls included,
10+
while every SQL face lowers the same measure to `COUNT("payer")`, defined over
11+
non-null values. A drafted chart therefore showed a different number than the
12+
published one, silently, and the number it showed was the one `count(*)` gives:
13+
the author's choice to count a specific column had no effect on the preview path.
14+
15+
Measured on one dataset, one row set, two `AnalyticsService` instances differing
16+
only in `draftRowsResolver` (the live half being `NativeSQLStrategy`'s generated
17+
SQL on a real SQLite): rows `{meals, 'bob'}` and `{meals, null}` answered
18+
`payer_count` 1 live and 2 on preview. Both now answer 1.
19+
20+
Unchanged, and pinned by the same differential: `count` with no field and `count`
21+
with `field: '*'` still answer the row count (the compiler writes
22+
`sql: m.field ?? '*'`, so the star is the "no field declared" spelling), and
23+
`count_distinct` still answers a cardinality. A group in which no row carries a
24+
value counts `0`, never null — `emptyGroupValueFor` rules counting nothing the
25+
identity `0`.
26+
27+
The live path is unchanged.
28+
29+
Bumped `patch` rather than `minor`: the package's published surface is
30+
byte-unchanged — `src/index.ts` is not in this diff, `aggregate()` is
31+
module-private and `evaluateAnalyticsQueryOverRows` is not on the barrel — and
32+
the only user-visible effect is a drafted chart's number moving to the number
33+
the published chart already showed, which is a correction toward the live
34+
standard rather than the backwards-compatible feature addition `minor` denotes.
Lines changed: 303 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,303 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #16218 — the draft-preview evaluator's `count` over a FIELD counted ROWS.
5+
*
6+
* `aggregate()` (`preview-evaluator.ts`) opened with
7+
*
8+
* ```ts
9+
* if (metricType === 'count' || field === '*') return rows.length;
10+
* ```
11+
*
12+
* so a dataset measure `{ aggregate: 'count', field: 'payer' }` — which
13+
* `dataset-compiler` lowers to the cube metric `{ type: 'count', sql: 'payer' }`
14+
* — had its declared FIELD carried in and then never read. The preview answered
15+
* the row count, nulls included. Every SQL face lowers that same measure to
16+
* `COUNT("payer")`, which is defined over NON-NULL values:
17+
*
18+
* `native-sql-strategy.ts` AGGREGATE_SQL (#10298)
19+
* `'count': (col) => (col === '*' ? 'COUNT(*)' : \`COUNT(${col})\`)`
20+
*
21+
* ⇒ a drafted chart showed a different number than the published one, silently,
22+
* and the number it showed was the one `count(*)` gives — so the author's
23+
* choice to count a SPECIFIC column had no effect at all on the preview path.
24+
*
25+
* ## The instrument — the same differential #16203 built, not a preview-only pin
26+
*
27+
* One dataset, one row set, two `AnalyticsService` instances differing in
28+
* exactly one config key (`draftRowsResolver`), so a difference between the two
29+
* responses is a difference the preview evaluator caused. The LIVE half is not a
30+
* model of an engine: it is `NativeSQLStrategy`'s generated SQL executed on a
31+
* real SQLite (sql.js) whose table is seeded from {@link ROWS} — the same rows
32+
* the resolver hands the preview. ⭐ A preview-only assertion would have passed
33+
* while the divergence stayed; the differential IS the acceptance shape.
34+
*
35+
* ## The fixture is built so the collapse cannot pass by coincidence
36+
*
37+
* The shared conformance fixture (`AGGREGATION_CASES`,
38+
* `packages/spec/src/data/aggregation-conformance.ts`) is built so that
39+
* `count(*)`, `count(field)` and `count_distinct(field)` are three DIFFERENT
40+
* numbers, precisely so a face that collapses one into another cannot pass by
41+
* coincidence. {@link ROWS} keeps that property per group:
42+
*
43+
* | group | rows | count(*) | count(payer) | count_distinct(payer) |
44+
* |:---------|-----:|---------:|-------------:|----------------------:|
45+
* | `travel` | 3 | 3 | 2 | 1 |
46+
* | `meals` | 2 | 2 | 1 | 1 |
47+
* | `void` | 2 | 2 | 0 | 0 |
48+
*
49+
* `travel` is 3 / 2 / 1 — three different numbers on one group. `meals` is the
50+
* card's measured cell verbatim (LIVE 1, PREVIEW 2). `void` is the arm where NO
51+
* row carries a value.
52+
*
53+
* ⭐ `void` answers **0, not null**, and that is a ruling rather than a
54+
* preference: `COUNT(col)` over no non-null values is `0`, and the spec agrees
55+
* from its own side — `emptyGroupValueFor` (`@objectstack/spec/data`) returns
56+
* `0` for `count` and `count_distinct` because "counting no rows is `0` … those
57+
* are measured facts, not missing data", reserving `undefined` for the
58+
* `avg`/`min`/`max` that genuinely have nothing to answer. ⛔ Reaching for a
59+
* null here would be the wrong answer AND would blur into #16219's territory.
60+
*
61+
* ⛔ Both `count(*)` spellings keep the ROW count. The compiler writes
62+
* `sql: m.field ?? '*'`, so the star IS the "no field declared" spelling —
63+
* `count` with no `field` and `count` with `field: '*'` are one cube metric and
64+
* both must still count rows, nulls included.
65+
*
66+
* ⛔ NOT touched here: `avg` over a group with no numeric values (#16219, same
67+
* function, held behind this card), and `sum`/`avg` over a temporal operand
68+
* (#16099). Neither arm is edited by this change.
69+
*/
70+
71+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
72+
import { DatasetSchema } from '@objectstack/spec/ui';
73+
import type { Cube } from '@objectstack/spec/data';
74+
import { AnalyticsService } from '../analytics-service.js';
75+
import { evaluateAnalyticsQueryOverRows } from '../preview-evaluator.js';
76+
77+
// ── one fixture, two paths ──────────────────────────────────────────────────
78+
79+
/**
80+
* `payer` is the nullable, duplicate-bearing column. Per group it makes
81+
* `count(*)`, `count(payer)` and `count_distinct(payer)` land on the three
82+
* different numbers tabulated in this file's header.
83+
*/
84+
const ROWS: Record<string, unknown>[] = [
85+
{ id: '1', category: 'travel', payer: 'ann' },
86+
{ id: '2', category: 'travel', payer: 'ann' },
87+
{ id: '3', category: 'travel', payer: null },
88+
// ⭐ the card's measured cell, verbatim: LIVE 1, PREVIEW 2.
89+
{ id: '4', category: 'meals', payer: 'bob' },
90+
{ id: '5', category: 'meals', payer: null },
91+
// the group where NO row carries a value — `COUNT(col)` answers 0, not null.
92+
{ id: '6', category: 'void', payer: null },
93+
{ id: '7', category: 'void', payer: null },
94+
];
95+
96+
const DATASET = DatasetSchema.parse({
97+
name: 'expense_ds',
98+
label: 'Expense',
99+
object: 'expense',
100+
dimensions: [
101+
{ name: 'category', field: 'category', type: 'string', label: 'Category' },
102+
],
103+
measures: [
104+
// control — `count` with NO field: the compiler writes `sql: '*'`.
105+
{ name: 'row_count', aggregate: 'count' },
106+
// control — the SAME thing spelled explicitly by the author. `field` is
107+
// `z.string().optional()` on `DatasetMeasureSchema`, so this is authorable,
108+
// and `assertDeclared` lets it through (no relationship path in `*`).
109+
{ name: 'star_count', aggregate: 'count', field: '*' },
110+
// ⭐ the card's measure.
111+
{ name: 'payer_count', aggregate: 'count', field: 'payer' },
112+
// control — the arm #16203 restored; it must still answer a cardinality.
113+
{ name: 'distinct_payers', aggregate: 'count_distinct', field: 'payer' },
114+
],
115+
});
116+
117+
const MEASURES = ['row_count', 'star_count', 'payer_count', 'distinct_payers'];
118+
119+
let db: any;
120+
121+
const runSql = (sql: string, params: unknown[]): Record<string, unknown>[] => {
122+
const stmt = db.prepare(sql.replace(/\$\d+/g, '?'));
123+
stmt.bind(params as any[]);
124+
const rows: Record<string, unknown>[] = [];
125+
while (stmt.step()) rows.push(stmt.getAsObject());
126+
stmt.free();
127+
return rows;
128+
};
129+
130+
async function locateWasm(): Promise<((file: string) => string) | undefined> {
131+
try {
132+
const { createRequire } = await import('node:module');
133+
const require = createRequire(import.meta.url);
134+
const pkgJsonPath = require.resolve('sql.js/package.json');
135+
const { dirname, join } = await import('node:path');
136+
return (file: string) => join(dirname(pkgJsonPath), 'dist', file);
137+
} catch {
138+
return undefined;
139+
}
140+
}
141+
142+
/**
143+
* The two services differ in ONE key. Everything else — the dataset, the rows,
144+
* the capabilities, the SQL engine behind `executeRawSql` — is shared.
145+
*/
146+
function svc(preview: boolean) {
147+
return new AnalyticsService({
148+
queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }),
149+
executeRawSql: async (_object: string, sql: string, params: unknown[]) => runSql(sql, params),
150+
...(preview ? { draftRowsResolver: async () => ROWS } : {}),
151+
});
152+
}
153+
154+
type Grid = Record<string, Record<string, unknown>>;
155+
156+
async function grid(preview: boolean): Promise<Grid> {
157+
const result = await svc(preview).queryDataset(
158+
DATASET,
159+
{ dimensions: ['category'], measures: MEASURES },
160+
undefined,
161+
preview ? { previewDrafts: true } : undefined,
162+
);
163+
return Object.fromEntries(result.rows.map((r) => [String(r.category), r]));
164+
}
165+
166+
beforeAll(async () => {
167+
const mod: any = await import('sql.js');
168+
const initSqlJs = mod.default ?? mod;
169+
const locateFile = await locateWasm();
170+
const SQL = await initSqlJs(locateFile ? { locateFile } : undefined);
171+
db = new SQL.Database();
172+
db.run(`CREATE TABLE "expense" ("id" TEXT PRIMARY KEY, "category" TEXT, "payer" TEXT);`);
173+
const insert = db.prepare(`INSERT INTO "expense" ("id","category","payer") VALUES (?,?,?)`);
174+
for (const r of ROWS) insert.run([r.id, r.category, r.payer] as any[]);
175+
insert.free();
176+
});
177+
178+
afterAll(() => db?.close());
179+
180+
describe('#16218 — `count` over a declared FIELD counts its non-null values, on both faces', () => {
181+
it("the card's measured cell: `meals` answers 1 on live and answered 2 on preview", async () => {
182+
const live = await grid(false);
183+
const preview = await grid(true);
184+
// The live half — `COUNT("payer")` on a real SQLite — is the standard.
185+
expect(live.meals.payer_count).toBe(1);
186+
// Pre-fix the preview answered 2 here: the row count, nulls included.
187+
expect(preview.meals.payer_count).toBe(1);
188+
// ⭐ and the differential itself: the two faces agree on the same rows.
189+
expect(preview.meals.payer_count).toBe(live.meals.payer_count);
190+
});
191+
192+
it('every group agrees across the differential, not just the measured one', async () => {
193+
const live = await grid(false);
194+
const preview = await grid(true);
195+
for (const group of ['travel', 'meals', 'void']) {
196+
for (const measure of MEASURES) {
197+
expect(
198+
preview[group][measure],
199+
`preview ${group}.${measure} must equal live`,
200+
).toBe(live[group][measure]);
201+
}
202+
}
203+
});
204+
205+
it('a count stays NUMERIC — counting nullable text is still counting', async () => {
206+
const preview = await grid(true);
207+
expect(typeof preview.travel.payer_count).toBe('number');
208+
expect(typeof preview.void.payer_count).toBe('number');
209+
});
210+
});
211+
212+
describe('#16218 — the controls that must not move', () => {
213+
it('count(*), count(field) and count_distinct(field) stay THREE different numbers', async () => {
214+
const live = await grid(false);
215+
const preview = await grid(true);
216+
for (const face of [live, preview]) {
217+
// `travel`: 3 rows, 2 non-null payers, 1 distinct payer.
218+
expect(face.travel.row_count).toBe(3);
219+
expect(face.travel.payer_count).toBe(2);
220+
expect(face.travel.distinct_payers).toBe(1);
221+
// Three different numbers on one group — a fix that made any two agree
222+
// would have replaced one collapse with another.
223+
const three = [face.travel.row_count, face.travel.payer_count, face.travel.distinct_payers];
224+
expect(new Set(three).size).toBe(3);
225+
}
226+
});
227+
228+
it('`count` with NO field still answers the ROW count, nulls included', async () => {
229+
const live = await grid(false);
230+
const preview = await grid(true);
231+
expect(preview.travel.row_count).toBe(3);
232+
expect(preview.meals.row_count).toBe(2);
233+
expect(preview.void.row_count).toBe(2); // every payer null — still 2 ROWS
234+
expect(preview.travel.row_count).toBe(live.travel.row_count);
235+
expect(preview.void.row_count).toBe(live.void.row_count);
236+
});
237+
238+
it("`count` with an explicit `field: '*'` is the same thing — still the ROW count", async () => {
239+
const live = await grid(false);
240+
const preview = await grid(true);
241+
// `dataset-compiler` writes `sql: m.field ?? '*'`, so the star IS the
242+
// "no field declared" spelling; the two measures are one cube metric.
243+
expect(preview.void.star_count).toBe(2);
244+
expect(preview.void.star_count).toBe(preview.void.row_count);
245+
expect(preview.void.star_count).toBe(live.void.star_count);
246+
expect(preview.meals.star_count).toBe(2);
247+
expect(preview.meals.star_count).toBe(live.meals.star_count);
248+
});
249+
250+
it('`count_distinct` still answers what #16203 gave it — a cardinality, nulls excluded', async () => {
251+
const live = await grid(false);
252+
const preview = await grid(true);
253+
expect(preview.travel.distinct_payers).toBe(1); // 'ann' twice + a null
254+
expect(preview.meals.distinct_payers).toBe(1);
255+
expect(preview.void.distinct_payers).toBe(0);
256+
expect(preview.travel.distinct_payers).toBe(live.travel.distinct_payers);
257+
expect(preview.void.distinct_payers).toBe(live.void.distinct_payers);
258+
});
259+
});
260+
261+
describe('#16218 — a group where NO row carries a value answers 0, never null', () => {
262+
it('the `void` group: 0 on both faces, and a number rather than a null', async () => {
263+
const live = await grid(false);
264+
const preview = await grid(true);
265+
// `COUNT(col)` over no non-null values is 0, and `emptyGroupValueFor`
266+
// (`@objectstack/spec/data`) rules the same from the other side: `count` and
267+
// `count_distinct` answer the identity `0` because counting nothing is a
268+
// measured fact, while `avg`/`min`/`max` over nothing stay null.
269+
expect(live.void.payer_count).toBe(0);
270+
expect(preview.void.payer_count).toBe(0);
271+
expect(preview.void.payer_count).not.toBeNull();
272+
expect(preview.void.payer_count).toBe(live.void.payer_count);
273+
});
274+
275+
it('over ZERO rows the single overall group still counts 0, both spellings', () => {
276+
const CUBE = {
277+
name: 'e', sql: 'expense',
278+
dimensions: {},
279+
measures: {
280+
rows: { name: 'rows', type: 'count', sql: '*' },
281+
payers: { name: 'payers', type: 'count', sql: 'payer' },
282+
},
283+
} as unknown as Cube;
284+
const r = evaluateAnalyticsQueryOverRows({ measures: ['rows', 'payers'], dimensions: [] }, CUBE, []);
285+
expect(r.rows).toEqual([{ rows: 0, payers: 0 }]);
286+
});
287+
288+
it('a field ABSENT from every row counts 0, exactly like a null-valued one', () => {
289+
const CUBE = {
290+
name: 'e', sql: 'expense',
291+
dimensions: { category: { name: 'category', type: 'string', sql: 'category' } },
292+
measures: { payers: { name: 'payers', type: 'count', sql: 'payer' } },
293+
} as unknown as Cube;
294+
// `undefined` (key never written) and `null` are one population to
295+
// `COUNT(col)`: neither is a value.
296+
const r = evaluateAnalyticsQueryOverRows(
297+
{ measures: ['payers'], dimensions: ['category'] },
298+
CUBE,
299+
[{ category: 'x' }, { category: 'x', payer: null }],
300+
);
301+
expect(r.rows).toEqual([{ category: 'x', payers: 0 }]);
302+
});
303+
});

packages/services/service-analytics/src/preview-evaluator.ts

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,8 @@ function extremumOf(rows: Row[], field: string, kind: 'min' | 'max'): unknown {
235235
*
236236
* | metric type | answer |
237237
* |:-----------------|:---------------------------------------------------------|
238-
* | `count` | the row count — numeric whatever it counted |
238+
* | `count` | over `*` the ROW count; over a declared COLUMN that |
239+
* | | column's NON-NULL count — numeric either way |
239240
* | `count_distinct` | the cardinality of the non-null values — numeric |
240241
* | `sum` / `avg` | arithmetic over the operands that read as numbers |
241242
* | `min` / `max` | the winning operand, IN ITS OWN TYPE ({@link extremumOf}) |
@@ -260,8 +261,26 @@ function extremumOf(rows: Row[], field: string, kind: 'min' | 'max'): unknown {
260261
* the sibling descriptor rule (`measure-result-type.ts`) answers the same way.
261262
*/
262263
function aggregate(rows: Row[], metricType: string, field: string): unknown {
263-
// `count`, and any metric aggregating over rows rather than a column.
264-
if (metricType === 'count' || field === '*') return rows.length;
264+
// `*` is the "no column declared" spelling — `dataset-compiler` writes
265+
// `sql: m.field ?? '*'` — so a metric over it aggregates over ROWS. Every
266+
// metric type keeps that reading, exactly as before.
267+
if (field === '*') return rows.length;
268+
// [#16218] `count` over a DECLARED column counts that column's non-null
269+
// values, which is what `COUNT(col)` means. The condition above used to be
270+
// `metricType === 'count' || field === '*'`, so the field a measure declared
271+
// was carried in and never read: the preview answered the row count and a
272+
// drafted chart showed a different number than the published one, silently —
273+
// the number `count(*)` gives, so the author's choice to count a specific
274+
// column had no effect on this path at all. The live faces settled the same
275+
// question the other way round in #10298: `AGGREGATE_SQL.count` in
276+
// `native-sql-strategy.ts` emits COUNT(*) for the star and COUNT(col) for a
277+
// real column, and every SQL dialect defines the latter over non-null values.
278+
//
279+
// A group with no non-null value counts `0`, never null: `emptyGroupValueFor`
280+
// (`@objectstack/spec/data`) rules `count` over nothing the identity `0`
281+
// because counting nothing is a measured fact — the opposite of the
282+
// `min`/`max` reading in {@link extremumOf}.
283+
if (metricType === 'count') return rows.filter((r) => r[field] != null).length;
265284
const nums = rows.map((r) => Number(r[field])).filter((n) => Number.isFinite(n));
266285
switch (metricType) {
267286
// The spec's spelling (`AggregationFunction`), which is what the compiler

0 commit comments

Comments
 (0)