Skip to content

Commit f3b28eb

Browse files
Trumpclaude
andauthored
fix(service-analytics): draft preview averages non-null operands and answers null over none (#16219) (#17191)
* test(service-analytics): differential harness for the preview `avg` empty-group divergence The failing half of #16219, written before the fix so its direction is a measurement rather than a claim. Two `AnalyticsService` instances differing in exactly one config key (`draftRowsResolver`); the live half is `NativeSQLStrategy`'s SQL on a real SQLite seeded from the same rows. Carries the four controls that must NOT move: `sum` over the all-null group stays the ruled identity `0`, `count(field)` over it stays `0` (#16218), `min`/`max` stay `null` (#16203), and `avg` over a group that has values still answers the mean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * fix(service-analytics): draft preview averages non-null operands and answers null over none (#16219) `aggregate()` in `preview-evaluator.ts` built its operand list with const nums = rows.map((r) => Number(r[field])).filter((n) => Number.isFinite(n)); and `Number(null)` is `0`, which `Number.isFinite` accepts. So every NULL entered an `avg` as a zero OPERAND and was counted in the divisor, while `AVG(col)` is defined over non-null values in every SQL dialect. A group whose column was NULL in every row therefore averaged to `0` — a plausible-looking average nobody measured — and a group that did carry values averaged low: `(10 + 20 + 0) / 3` against SQLite's 15. ⭐ The card attributed the defect to the arm's `: 0` fallback. Measured, that branch never ran on the card's own cell: `nums` was `[0, 0]`, so the ternary took its TRUE branch. The fallback is reachable only where no row carries a parseable operand at all, and both limbs had to move for the divergence to close. One arm. The operand list is rebuilt over the rows that carry a value — exactly as `extremumOf` (`v == null` → skip) and #16218's `count` arm (`r[field] != null`) already do — and the answer over none is READ from `emptyGroupValueFor` (`@objectstack/spec/data`) rather than restated: it rules `0` where counting or summing nothing is a measured fact and `undefined` (spelled `null` on this wire) where there is nothing to answer. #16203 cited the same function when it moved `min`/`max` off the same idiom here. `sum` and the numeric `default` arm keep the existing `nums`: `0` is the additive identity so the coercion never moved `sum`'s answer, and `default` serves the custom-SQL metric types, which have no live standard to be moved towards. Reproduced and closed through the differential harness #16203 built and #16218 reused — 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. Controls held by the same fixture: `sum` over the all-null group stays the ruled identity `0`, `count(field)` over it stays `0` (#16218), `min`/`max` stay `null` (#16203), and `travel`'s six answers stay six different numbers. No behaviour change on the live path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * fix(service-analytics): the preview `avg` null fires on an EMPTY group, never an incoherent one (#16219) The full `service-analytics` suite caught a control the card's list did not name: `preview-aggregate-operand-type.test.ts` (#16203) pins `avg` over a TEMPORAL operand at the numeric identity `0`, and reading the whole "no numeric operand" condition as "empty group" moved it to `null`. "No numeric operand" is two situations, and only one of them is averaging NOTHING: • no row carried a value at all — the empty group, which `emptyGroupValueFor` rules and now answers; • rows carried values that do not read as numbers — a `date` column under `avg`. That is an incoherent aggregate/field-type pair, #16099 owns the refusal, no layer refuses it yet, and the live face answers a different number again (SQLite's numeric affinity over TEXT). A `null` there would invent a THIRD answer to a question nobody has ruled on. So the arm splits the two: `present.length ? 0 : (emptyGroupValueFor(...) ?? null)`. The boundary is now pinned from both sides — the #16203 file keeps its `0`, and the new differential carries a control asserting the same `0` for values that are present and unreadable as numbers. 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 d61139f commit f3b28eb

3 files changed

Lines changed: 486 additions & 4 deletions

File tree

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
---
2+
'@objectstack/service-analytics': patch
3+
---
4+
5+
Draft-preview analytics: `avg` answers the mean of the NON-NULL operands, and `null` when there are none — matching every live face
6+
7+
A dataset measure `{ aggregate: 'avg', field: 'amount' }` compiles to the cube
8+
metric `{ type: 'avg', sql: 'amount' }`, and the draft-preview evaluator built
9+
its operand list with `rows.map((r) => Number(r[field]))`. `Number(null)` is `0`
10+
and `Number.isFinite` accepts it, so every NULL entered the average as a zero
11+
OPERAND and was counted in the divisor. `AVG(col)` is defined over non-null
12+
values in every SQL dialect, so a drafted chart showed a different number than
13+
the published one, silently — and where a group's column was NULL in every row
14+
the number it showed was `0`: a plausible-looking average that a reader cannot
15+
tell from one somebody measured.
16+
17+
Measured on one dataset, one row set, two `AnalyticsService` instances differing
18+
only in `draftRowsResolver` (the live half being `NativeSQLStrategy`'s generated
19+
SQL on a real SQLite). Rows `{meals, null}` and `{meals, null}` answered
20+
`avg_amount` null live and `0` on preview; rows `{travel, 10}`, `{travel, 20}`,
21+
`{travel, null}` answered 15 live and 10 on preview. Both cells now answer the
22+
live number.
23+
24+
The empty answer is READ from the platform's own ruling rather than restated
25+
here: `emptyGroupValueFor` (`@objectstack/spec/data`) returns the identity `0`
26+
where counting or summing nothing is a measured fact and `undefined` — spelled
27+
`null` on this wire — where there is nothing to answer. It is the same function
28+
`fillEmptyGroups`, `sql-driver` and `driver-turso` read, and the one #16203 cited
29+
when it moved `min`/`max` off the same idiom in this function.
30+
31+
Unchanged, and pinned by the same differential: `sum` over a group with no values
32+
still answers the ruled identity `0`, `count` over one still answers `0`
33+
(#16218), `min`/`max` still answer `null` (#16203), and `avg` over a group that
34+
has values still answers its mean. `sum` and the numeric `default` arm keep their
35+
existing operand list — `0` is the additive identity, so the coercion never moved
36+
`sum`'s answer, and the `default` arm serves the custom-SQL metric types, which
37+
have no live standard to be moved towards.
38+
39+
The `null` fires on an EMPTY group and never on an incoherent one. "No numeric
40+
operand" is two different situations: no row carried a value at all — the empty
41+
group the policy rules on — or rows carried values that do not read as numbers,
42+
such as a `date` column under `avg`. The second is an incoherent
43+
aggregate/field-type pair that #16099 owns and no layer refuses yet; it keeps the
44+
numeric identity it has always had, since the live face answers a different
45+
number again (SQLite's numeric affinity over a TEXT column) and a `null` there
46+
would invent a third answer. That boundary is pinned from both sides — by
47+
`preview-aggregate-operand-type.test.ts` (#16203) and by a control in the new
48+
differential.
49+
50+
The live path is unchanged.
51+
52+
Bumped `patch` rather than `minor`, on the same reasoning the sibling #16218
53+
shipped under: the package's published surface is byte-unchanged — `src/index.ts`
54+
is not in this diff and does not re-export `preview-evaluator.ts` at all, and
55+
`aggregate()` is module-private — and the only user-visible effect is a drafted
56+
chart's number moving to the number the published chart already showed. A value
57+
correcting toward the live standard is a fix, not the backwards-compatible
58+
feature addition `minor` denotes. It is a real value change for a consumer
59+
reading the preview response (`0` becomes blank), which is why the card was filed
60+
separately rather than ridden along with #16203 — but the `0` it replaces was
61+
never a number the platform promised.

0 commit comments

Comments
 (0)