Skip to content

fix(service-analytics): compile a measure's field and filter, on both doors - #10411

Merged
os-warren merged 3 commits into
mainfrom
claude/issue-10298-measure-compiler-field-and-filter
Aug 20, 2026
Merged

fix(service-analytics): compile a measure's field and filter, on both doors#10411
os-warren merged 3 commits into
mainfrom
claude/issue-10298-measure-compiler-field-and-filter

Conversation

@os-warren

@os-warren os-warren commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Fixes #10298

A dataset measure declares three things — aggregate, field and filter — and the compiled SQL used only the first. Both defects the card measured are that one gap, and this repairs it in the strategy that produced the statements the card quoted.

The premise, re-established on this branch before anything changed

Both defects reproduce byte-for-byte against origin/main (8012960), compiling the card's own cubes through AnalyticsService.generateSql:

DEFECT-1 (count + field)
  SELECT COUNT(*) AS "closed_count", COUNT(*) AS "kb_resolved_count" FROM "crm_case"
  -- kb_resolved_count declares field: 'resolved_by_article'; it is not in the statement

DEFECT-2 (/api/v1/analytics/query)
  SELECT COUNT(*) AS "opp_count", COUNT(*) AS "won_count", COUNT(*) AS "lost_count",
         SUM(amount) AS "won_amount" FROM "crm_opportunity"
  params: []
  -- three measures declare a `filter`; no FILTER, no CASE WHEN, no bound comparand

That is the card's reported SQL, reproduced. premise_still_valid: true.

The fork triage named — not reached

Triage ruled that if per-measure filter turned out to be genuinely unsupported on the strict wrapper, the 400-naming-the-measure variant would be a contract change and forks to a decision. That determination was made before writing the fix, and it lands on the other side: the filter is supportable here with no change to what the endpoint accepts or rejects. The endpoint's accept/reject set is untouched; only the arithmetic moves, from "what the compiler happened to emit" to "what the author declared". Clause-②: no.

Two facts settled it. MetricSchema in packages/spec has declared filters on a metric since #4001 — the cube model has always had room for per-metric filtering, it simply had no consumer. And the compiled dataset already carries both filters in the registry beside its Cube; nothing was missing but the wire between them.

The fix

count compiles its column. AGGREGATE_SQL['count'] took the resolved column and discarded it. It now wraps that column — COUNT(resolved_by_article) — when the measure names a field, and keeps COUNT(*) when it does not: sql: '*' is the compiler's own "no field declared" spelling, so the star must go on counting rows.

The dataset's filters reach the strict door. compileDataset splits a dataset into the half a Cube can express and the half it cannot: the definition-level filter, and each measure's own scoped filter. Only DatasetExecutor — the dashboard's door — ever read the second half. POST /api/v1/analytics/query addresses the registered Cube directly and never touches the executor, so it answered unfiltered aggregates under the author's measure names.

That half now reaches the strategy through getDatasetScope(cubeName) on the context the analytics package builds for its own strategies — the same shape, the same registry and the same "cannot answer, do not block" tiering as the getAllowedRelationships hook directly above it. A per-measure filter becomes a conditional aggregate rather than a WHERE conjunct, because one statement carries several measures and a WHERE would narrow all of them; the definition-level filter becomes a plain conjunct, because narrowing the whole statement is exactly what it means.

CASE WHEN, not FILTER (WHERE …): FILTER is Postgres and SQLite ≥ 3.30 only — MySQL has never had it — and this strategy hand-compiles one statement for whichever SQL driver owns the object. A portable conditional aggregate is the only form that cannot answer a syntax error on one supported driver and a number on another.

No packages/spec edit. The hook is declared in service-analytics's own strategies/types.ts as an extension of the spec's StrategyContext. Nothing about it is an authorable surface — no metadata key, no wire shape, no error code — so widening the published contract would have bought nothing.

What the SQL looks like now

COUNT(resolved_by_article) AS "article_count"                                  -- count + field
COUNT("crm_case"."resolved_by_article")                                        -- …qualified, when the cube joins
COUNT(CASE WHEN stage = $1 THEN 1 END) AS "won_count"                          -- count + filter
COUNT(CASE WHEN is_closed = $2 THEN resolved_by_article END) AS "kb_resolved_count"  -- both
SUM(CASE WHEN stage = $3 THEN amount END) AS "won_amount"                      -- sum + filter
… FROM "crm_opportunity" WHERE is_deleted = $4                                 -- the dataset's own scope
params: ['closed_won', 1, 'closed_won', 0]

Comparands are bound, in the order their placeholders appear: the SELECT list precedes the WHERE clause and $n is positional, so each measure filter is compiled inside the SELECT loop. A filter compiled anywhere else would misalign every later bind — the pin asserts the params array, not just the text.

In scope beyond the two measured defects, and why

The card names the per-measure filter; the definition-level filter was dropped on the same door by the same mechanism, and is repaired here rather than filed. Two reasons, both load-bearing.

It is the card's own acceptance criterion: "the same cube and the same measure names answer two different numbers depending on which door you come in." For any dataset that declares an intrinsic filter, fixing only the measure filters leaves the two doors still disagreeing — so the card could not be closed without it.

And it is mechanical, with its correct shape already pinned by evidence: DatasetExecutor.runMeasurePass combines compiled.filter as the base filter of every pass. The sibling behaviour dictates the shape; nothing here is a judgement call.

Evidence it was broken, measured the same way as the two above:

SELECT COUNT(*) AS "opp_count", COUNT(*) AS "won_count" FROM "crm_opportunity"
-- the dataset declares filter: { is_deleted: false }; there is no WHERE clause at all

Both doors, on a real database

Shape assertions cannot tell a fix from plausible-looking SQL, so the last block runs both doors against a real SQLite (sql.js, the pure-WASM engine driver-sql itself falls back to) over the card's own ground truth: 24 opportunities, 8 won, 5 lost, won revenue 1,290,000, grand total 5,632,500 — the number the broken door answered for won_amount.

The API door now answers 24 / 8 / 5 / 1,290,000, and queryDataset answers identically, measure for measure, both ungrouped and grouped.

The grouped leg groups by owner, never by stage. Grouping by the very column a measure filters on makes the filtered and unfiltered aggregates coincide inside the matching group, so an assertion there passes with the filter dropped. That is not a hypothetical: the first draft of that test grouped by stage, and the ablation below is what caught it — the fix was fine, the test was asleep.

Reverse verification

Predicted signature, written before the run: neutering both halves in native-sql-strategy.ts (revert count to the arity-zero COUNT(*) lambda, force datasetScope to undefined) turns 8 of the 11 new tests red, the three survivors being COUNT(*) for a fieldless count, the manifest-cube no-op, and the fixture self-check; the lockstep pin stays green because it reads the two tables rather than the call site.

Observed, exactly: Tests 8 failed | 12 passed (20), Test Files 1 failed | 1 passed (2), with the card's own wrong numbers back in the failure text —

AssertionError: expected { opp_count: 24, won_count: 24, …(2) } to match object { opp_count: 24, won_count: 8, …(2) }
AssertionError: expected 8 to be 24            // the two doors disagreeing again
AssertionError: expected 'SELECT COUNT(*) AS "article_count" FR…' to contain 'COUNT(resolved_by_article) AS "articl…'

Rebuild: none is required between the edit and the run, argued from the files. Both the ablated module and the test resolve through relative specifiers inside one package (../analytics-service.js, ../strategies/native-sql-strategy.js), so vitest transforms src/ directly; service-analytics ships no vitest config and the repo has no alias table redirecting them. The only exports-map resolution in the file is @objectstack/spec, which this change does not touch and which was built before the first measurement. The ablation was therefore read off the edited source in both legs.

Restore is byte-identical, not merely "reverted":

PRE-ABLATION  hash: b2ac0c23212f1e766c29524bcafb499633f16e0b
POST-ABLATION hash: a51e178f328ef6626054da443042e025fb2e0f31
RESTORED      hash: b2ac0c23212f1e766c29524bcafb499633f16e0b

and the restore leg was re-run green (Tests 20 passed (20)) rather than assumed.

The vocabulary stays in lockstep

aggregation-lockstep.test.ts gains one pin: the conditional table's keys must equal the plain table's. An aggregate added to one and not the other would not fail — it would silently drop the author's filter and answer the unfiltered number under the filtered measure's name, which is this card's defect returning through a new door.

Tests

  • pnpm --filter @objectstack/service-analytics exec vitest run --maxWorkers=2Test Files 78 passed (78), Tests 1734 passed (1734)
  • A cube that is not a compiled dataset (an inferred or manifest cube) is pinned to emit byte-for-byte the statement it emitted before, with an empty params.

Gate union

Derived with node scripts/pm/dispatch-gates.mjs (no paths passed — it reads the change set from the merge base itself), then run after the final commit on a clean worktree, at 69143f891. Nine path-matched families plus the five the test-file convention moves, and check:nul-bytes. Every exit code captured before any pipe; each family's own verdict line quoted:

gate exit its own verdict line
check:changeset-gate-self-tests 0 ✓ check-changeset-no-major --self-test: 116 assertions …
check:objectui-changeset 0 ✓ objectui-range --self-test: all checks passed
check:slot-lookup 0 ✓ slot-lookup ratchet holds: 107 unswept site(s) in 25 file(s), none new
check:test-source-alias 0 check-test-source-alias OK — 72 packages with tests scanned
check:type-source-resolution 0 check-type-source-resolution OK — 76 packages with a tsconfig.json scanned
check-adr-0087-registration.mjs 0 ✓ this PR adds no declared-breaking changeset (1 non-breaking changeset(s) seen)
check-changeset-no-major.mjs 0 ✓ This diff introduces no \major` bump.`
check-empty-changeset.mjs 0 ✓ No empty-frontmatter changeset introduced by this diff (1 declaring changeset(s) added)
docs-audit/check-affected-docs.mjs 0 ✓ affected-docs self-test: 262 cases pass. (its remaining output is the standing route-ledger census, not a verdict about this diff)
check:query-options-erasure 0 ✓ query-options-erasure ratchet holds: 67 unswept non-test site(s) in 17 file(s), none new
check:type-check-coverage 0 check-type-check-coverage: OK — 64/77 workspace packages type-checked (plus the root), 13 in the DEBT ledger (436 frozen raw errors), 1 exempt
check:type-check-debt 0 same line under --re-measure; 436 frozen raw errors, unchanged — service-analytics's ledger entry stays at 10 and no entry was raised
check:engine-double-contract 0 check-engine-double-contract: OK — 338 pinned, 133 in the DEBT ledger, 2 exempt.
check:where-matcher 0 ✓ where-matcher conformance holds: 266 matcher(s) discovered … none new
check:nul-bytes 0 check-nul-bytes: OK (scanned 6116 text file(s) … no raw ASCII control bytes)

The debt gate was run against a fully built workspace closure (turbo run build --filter=./packages/* --filter=./packages/*/*, 70/70 successful) — on an unbuilt tree it refuses, and that refusal is a precondition, not a pass.

Not addressed here

Two neighbouring gaps were measured and filed as their own issues rather than widened into this PR. Neither is fixed by this branch:


Generated by Claude Code

os-warren and others added 3 commits August 20, 2026 17:55
… doors

A dataset measure declares `aggregate`, `field` and `filter`; the compiled SQL
used only `aggregate`.

1. `{ aggregate: 'count', field: 'x' }` emitted `COUNT(*)`. The wrapper table
   took the resolved column and discarded it, so a measure asking how many rows
   carry a value counted every row it was handed.

2. `/api/v1/analytics/query` dropped every per-measure `filter`, and the
   dataset's definition-level `filter` with it. That door addresses the
   registered Cube directly; both filters live beside the cube in the dataset
   registry, and only `DatasetExecutor` — the dashboard's door — ever read them.
   One cube and one set of measure names answered two different numbers
   depending on which door the caller came in.

`count` now takes its column (`*` still counts rows — it is the compiler's "no
field declared" spelling), and a new conditional-aggregate table lowers a
measure filter to a portable `CASE WHEN` (not `FILTER (WHERE …)`, which MySQL
lacks). The dataset scope reaches the strategy through `getDatasetScope` on the
context the analytics package builds for its own strategies — same shape and
same registry as the neighbouring `getAllowedRelationships`, and no spec edit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
… filters do not name

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-analytics, touching 15 documentable anchor(s).

5 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/ai/natural-language-queries.mdx (via count_distinct (literal))
  • content/docs/data-modeling/queries.mdx (via count_distinct (literal))
  • content/docs/kernel/contracts/data-engine.mdx (via count_distinct (literal))
  • content/docs/protocol/objectql/query-syntax.mdx (via count_distinct (literal))
  • content/docs/ui/dashboards.mdx (via count_distinct (literal))

3 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v14.mdx (via generateSql (symbol))
  • content/docs/releases/v15.mdx (via count_distinct (literal))
  • content/docs/releases/v17.mdx (via generateSql (symbol), count_distinct (literal))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 8 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json e502a6a8ebafaee434a14481a45494a1dd4958c9packageMentionDocs.

Which tree this was computed on

This run read content/docs from f4e64efd71c2b643c93f9bb87a1001b3ab5edffa — the merge of head 69143f891cfc53ae5dca740e3f3673ae51538d64 into base e502a6a8ebafaee434a14481a45494a1dd4958c9, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin f4e64efd71c2b643c93f9bb87a1001b3ab5edffa && git checkout f4e64efd71c2b643c93f9bb87a1001b3ab5edffa
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e502a6a8ebafaee434a14481a45494a1dd4958c9 69143f891cfc53ae5dca740e3f3673ae51538d64 && git checkout -B drift-repro e502a6a8ebafaee434a14481a45494a1dd4958c9 && git merge --no-ff 69143f891cfc53ae5dca740e3f3673ae51538d64

node scripts/docs-audit/affected-docs.mjs --json e502a6a8ebafaee434a14481a45494a1dd4958c9

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs e502a6a8ebafaee434a14481a45494a1dd4958c9 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

1 participant