Skip to content

fix(service-analytics): apply the dataset-level filter on the ObjectQL analytics path (#10413 phase 1) - #10758

Merged
os-warren merged 1 commit into
mainfrom
claude/issue-10413-objectql-dataset-filter
Aug 21, 2026
Merged

fix(service-analytics): apply the dataset-level filter on the ObjectQL analytics path (#10413 phase 1)#10758
os-warren merged 1 commit into
mainfrom
claude/issue-10413-objectql-dataset-filter

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Part of #10413 — phase 1 only. This card stays open for phase 2, which waits on #10576.

The three doors, measured in one tree and one run

A dataset carries two declarations the Cube model has no room for: a definition-level
filter (its intrinsic scope) and a per-measure filter. Three doors read them, and
before this change they disagreed three ways:

doordataset-level filterper-measure filter
native SQL (NativeSQLStrategy)applied — a WHERE conjunctapplied — one conditional aggregate per measure (landed in PR #10411 for #10298)
dashboard (queryDatasetDatasetExecutor)applied — combineFilters ANDs it into the runtime filterapplied — splitMeasuresByFilter / runMeasurePass fan out one query per filtered measure
ObjectQL (ObjectQLStrategy)droppeddropped

Reproduced on this branch's merge base (53a48c93f) with a stub executeAggregate that
records its arguments: svc.query({ cube: 'opportunity_metrics', measures: [...] }) on a
service whose capabilities are { nativeSql: false, objectqlAggregate: true } reached the
engine with filter: undefined — no filter key at all. The pre-fix run of the new test
file failed with expected undefined to deeply equal { '$and': [ { is_deleted: false } ] }.

What this PR changes

ObjectQLStrategy.execute now reads the dataset scope from the same channel
NativeSQLStrategy reads it from — getDatasetScope(cubeName) on the strategy context,
added for #10298 — and ANDs the definition-level filter into the whole-call filter
engine.aggregate already accepts. No contract movement: engine.aggregate has always
taken one predicate for the whole call.

Merge semantics, read from the code rather than assumed. The scope is pushed onto
conjuncts and folded by the line that already existed at the end of the filter build:

filter.$and=[...(Array.isArray(filter.$and) ? filter.$and : []), ...conjuncts];

so it is ANDed, never merged key-by-key — the posture withReadScope states in the
same file ("Composed with $and, never by key merge: the query's own filter and the scope
can name the SAME field … a spread would let caller input silently overwrite"). The
dashboard door reaches the same semantics through combineFilters in dataset-executor.ts
(if (a && b) return { $and: [a, b] }), so both doors AND. An empty scope is
represented by absence, not by an empty object: filterNodeToCondition returns null for a
node that constrains nothing, and withReadScope only marks a filter when
Object.keys(filter).length > 0 — so a dataset with no filter still calls the engine with
no filter key.

The SQL echo renders it too.generateSql on this path already renders the read scope
deliberately, on the rule that "a rendering that contradicts execution is worse than no
rendering" (#3601 / #3602 / #3650) — leaving the newly-applied predicate out of the echo
would have been the same lie in the other direction. This is the one place the change goes
beyond the strictly minimal edit, and it is called out here for review: same defect class,
same file, same gate family, and the correct shape was already pinned by the read-scope
block eight lines below it.

Phase 2 is NOT done, and is pinned open rather than left silent

Per-measure filters still do not reach this door: an aggregation is { field, method, alias }
and cannot carry a predicate. Folding one into the whole-call filter would narrow every
measure at once — trading a wrong won_count for a wrong opp_count too — so
measureFilters is deliberately unread here (the only occurrence of that identifier in the
file is the comment saying so; the sibling strategy that does read it has one code hit,
which is the positive control for that search).

objectql-dataset-filter.test.ts therefore contains a [#10413 phase 2 — NOT DONE] block
asserting the current, wrong numbers — won_count: 24, won_amount: 5_632_500 where the
truth is 8 and 1_290_000 — with a comment naming #10576 and instructing that those
assertions be flipped, not deleted, when the contract widens. Without it this path reads
all-green while still answering the whole book for won revenue.

Pins

#pintest
a dataset-level filter reaches engine.aggregatefilter equals { $and: [{ is_deleted: false }] }
none is fabricated when the dataset declares nonefilter is undefined
the caller's where and the time windows still merge in and are not clobbered{ stage: 'closed_won', $and: [{ is_deleted: false }] }, and 8 rows — not 11 (scope dropped) and not 24 (where clobbered); a dateRange still lands on its own field
the native-SQL path is untouchedone is_deleted conjunct, bound once — not doubled
phase 2 still absentaggregations carry exactly {alias, field, method}; closed_won reaches the engine nowhere
a cube that is not a compiled dataset compiles unchangedmanifest cube still calls with no filter, answers 30
the echo tells the same story as the executionWHERE is_deleted = $1, params: [false]; nothing extra for an unscoped dataset

Verification

pnpm --filter @objectstack/service-analytics exec vitest run79 files, 1748 tests, all
passing
(14 of them new).

Ablation, predictions written before mutating, neither leg rebuilt because the subject is
source-resolved (the test imports ../analytics-service.js relatively and
packages/services/service-analytics/dist does not exist — the mutation changing results with
no build is itself the positive control):

legpredictedmeasured
A — drop conjuncts.push(scopeCondition)5 of 14 red, echo tests stay greenexactly those 5, echo green
B — drop the echo block in generateSql1 of 14 redexactly that one

Both restored by git checkout and verified byte-identical with git hash-object
(96b2bbb687a09f45ed6a7c59fdf930ec6f49bac2 before mutation, after each restore, and at the
end), with 14/14 green on the restored tree.

Gatesnode scripts/pm/dispatch-gates.mjs with no path arguments, on the final commit
5933b981c with a clean tree; exit codes captured before any pipe. All 17 exit 0:
check:changeset-gate-self-tests, check:objectui-changeset, check:slot-lookup,
check:test-source-alias, check:type-source-resolution, check-adr-0087-registration,
check-changeset-no-major, check-empty-changeset, check-affected-docs,
check:query-options-erasure, check:type-check-coverage, check:type-check-debt --re-measure,
check:engine-double-contract, check:where-matcher, check:nul-bytes, plus
check:route-envelope --self-test and check:dispatcher-error-vocabulary --self-test, which the
derivation did not name (the known-short-union class #10309).

check:type-check-debt --re-measure: "33 ledger entr(ies) re-measured in 364.6s, 1912 raw tsc
error(s) total, none above its recorded number."
It reports @objectstack/plugin-auth as
lowerable (records 109, measures 97) — untouched here on purpose, per #10615.


Generated by Claude Code

…L path (#10413 phase 1)
`ObjectQLStrategy.execute` built its engine filter from
`normalizeAnalyticsFilterTree(query)` alone — the caller's `where` and the time
windows — and consulted the dataset registry nowhere. On a deployment whose
driver reports `objectqlAggregate` but not `nativeSql`, `engine.aggregate` was
therefore called with no `filter` key at all: the dataset's definition-level
scope was dropped and every measure aggregated the whole table, while the
dashboard door answered the scoped numbers for the same cube.
The scope now travels as its own `$and` conjunct — never a key merge, for the
reason `withReadScope` states: the caller's `where` and the dataset scope can
name the same field. The representative SQL echo renders it as well, on the
#3601/#3602 rule that an echo omitting an applied predicate is the same lie as
one inventing a predicate.
Phase 1 only: per-measure filters cannot be expressed against an aggregation
typed `{ field, method, alias }`. That contract widening is #10576 and the
lowering is phase 2 — pinned open, wrong numbers and all, in
`objectql-dataset-filter.test.ts`.
Part of #10413
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 1 documentable anchor(s).

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

  • content/docs/releases/v14.mdx(via generateSql (symbol))
  • content/docs/releases/v17.mdx(via generateSql (symbol))

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
  • 1 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 78ac958552d3749793dd7d4cc612c651cc7df2b9packageMentionDocs.

Which tree this was computed on

This run read content/docs from f022a72c25e2f416e85f00bcdcdf19d17b44c38e — the merge of head 5933b981cfd174370306ee262ae0092866c2fa4b into base 78ac958552d3749793dd7d4cc612c651cc7df2b9, 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 f022a72c25e2f416e85f00bcdcdf19d17b44c38e && git checkout f022a72c25e2f416e85f00bcdcdf19d17b44c38e
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 78ac958552d3749793dd7d4cc612c651cc7df2b9 5933b981cfd174370306ee262ae0092866c2fa4b && git checkout -B drift-repro 78ac958552d3749793dd7d4cc612c651cc7df2b9 && git merge --no-ff 5933b981cfd174370306ee262ae0092866c2fa4b
node scripts/docs-audit/affected-docs.mjs --json 78ac958552d3749793dd7d4cc612c651cc7df2b9

⚠️ 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 78ac958552d3749793dd7d4cc612c651cc7df2b9 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@os-warren@claude