Skip to content

Commit 1d4a2aa

Browse files
committed
fix(service-analytics): compareTo applies measure-scoped filters, so <measure>__compare is the same measure as its neighbour (#4820)
A measure declared with its own `filter` was scoped by a supplementary grouped sub-query in the current period, but the `compareTo` pass issued ONE shifted query over every base measure with only the base filter — `measureFilters` was never read on that path. `won_count` counted won deals while `won_count__compare` counted every deal, side by side under one label, biased so the comparison window always looks better. Both windows now run the same `runMeasurePass`: unfiltered measures in one query plus one sub-query per filter-scoped measure, merged by dimension key. The split lives in one exported `splitMeasuresByFilter` so the two paths cannot re-diverge; the only difference between them is the shifted dateRange. Tests build a fake service that really evaluates `where` and groups seed rows, so the pinned number changes with the fix (won_count__compare 5 -> 1), not just the query shape. Selections whose measures carry no filter still compare in a single shifted query. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny
1 parent 61cc079 commit 1d4a2aa

3 files changed

Lines changed: 195 additions & 50 deletions

File tree

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
---
2+
"@objectstack/service-analytics": patch
3+
---
4+
5+
fix(service-analytics): `compareTo` applies measure-scoped filters, so `<measure>__compare` is the same measure as the column beside it (#4820)
6+
7+
A dataset measure declared with its own `filter` is scoped by running a
8+
supplementary grouped sub-query — `combineFilters(baseFilter, measureFilters[m])`
9+
— and merging it back by dimension key. The `compareTo` pass did not: it issued
10+
**one** shifted query over every base measure with only the base filter as its
11+
`where`, and never consulted `compiled.measureFilters` at all.
12+
13+
For a dataset like
14+
15+
```ts
16+
measures: [
17+
{ name: 'revenue', aggregate: 'sum', field: 'amount' },
18+
{ name: 'won_count', aggregate: 'count', filter: { stage: 'closed_won' } },
19+
]
20+
```
21+
22+
the current-period column was scoped and the comparison column was not — two
23+
different measures rendered side by side under one label:
24+
25+
| # | measures | where | |
26+
|:---|:---|:---|:---|
27+
| 1 | `revenue` || current |
28+
| 2 | `won_count` | `{"stage":"closed_won"}` | current |
29+
| 3 | `revenue`, `won_count` | **absent** | shifted |
30+
31+
`won_count__compare` was therefore a count of **every** opportunity in the
32+
previous window, inflated by exactly the rows the measure exists to exclude.
33+
The error runs one way: the comparison period always looks better, so a "won
34+
deals vs. last month" tile reads as a collapse when nothing went wrong. Only
35+
filter-scoped measures were affected — the unfiltered ones next to them compared
36+
correctly, which is what made it survive.
37+
38+
The comparison window now runs the **same pass** as the current period —
39+
unfiltered measures in one shifted query plus one shifted sub-query per
40+
filter-scoped measure, merged by dimension key — through a single shared
41+
implementation, so the two paths cannot re-diverge at the next change. The
42+
dataset filter, the presentation's `runtimeFilter` and the measure's own filter
43+
compose identically in both windows; the only difference between them is the
44+
shifted `dateRange`.
45+
46+
Numbers reported by existing dashboards change where a filtered measure was
47+
compared: with 3 won deals this month against 1 won of 5 opportunities last
48+
month, `won_count__compare` was `5` and is now `1`.
49+
50+
Cost: one extra query per filter-scoped measure when `compareTo` is set.
51+
Selections whose measures carry no filter are untouched and still compare in a
52+
single shifted query.
53+
54+
The empty-group fill (#4708) covers the new seam: a group the measure's filter
55+
empties in the *previous* window now reports `0` for a `count`/`sum` compare
56+
column rather than blanking it, exactly as it already did for the current period.
Binary file not shown.

packages/services/service-analytics/src/dataset-executor.ts

Lines changed: 139 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,14 @@ export type CompareTo = DatasetCompareTo;
2525
* runtime filter, compareTo) into one or more `AnalyticsQuery`s against the Cube
2626
* runtime, then post-processes the results:
2727
* - resolves the base measures a selection needs (including derived deps),
28-
* - applies measure-scoped filters via supplementary grouped queries,
28+
* - applies measure-scoped filters via supplementary grouped queries — in
29+
* EVERY window it runs, the `compareTo` one included (#4820),
2930
* - fills the empty-group value into columns no query reported, by aggregate
3031
* kind (#4708) — a count/sum over an excluded group is 0, avg/min/max null,
3132
* - evaluates derived measures (ratio/sum/difference/product) row-by-row (Q1),
32-
* - shifts the query for `compareTo` (previousPeriod / previousYear) and
33-
* attaches `<measure>__compare` columns,
33+
* - shifts the queries for `compareTo` (previousPeriod / previousYear) and
34+
* attaches `<measure>__compare` columns, re-running the same measure pass
35+
* so a filtered measure means the same thing in both columns,
3436
* - computes server-side totals (`selection.totals.groupings`, #1753) by
3537
* re-running the selection per dimension subset, so matrix subtotals and
3638
* the grand total use each measure's true aggregate,
@@ -133,6 +135,36 @@ export function combineFilters(
133135
return a ?? b;
134136
}
135137

138+
/**
139+
* Partition base measures into those the dataset scopes with their own
140+
* measure-level `filter` and those it does not — the single place that answers
141+
* "does this measure carry its own filter?".
142+
*
143+
* Paired with {@link DatasetExecutor.runMeasurePass}, this is what keeps ONE
144+
* definition of "how a measure filter is applied" for every grouped pass the
145+
* executor runs: the current period, each `totals` subset, and the `compareTo`
146+
* window. `compareTo` used to issue a single shifted query over all base
147+
* measures with only the base filter, consulting `measureFilters` nowhere on
148+
* that path — so a measure declared `filter: { stage: 'closed_won' }` was
149+
* scoped in its own column and unscoped in `<measure>__compare`: two different
150+
* measures rendered side by side under one label, and biased the worst way
151+
* (the comparison window is inflated by exactly the rows the measure exists to
152+
* exclude, so "won deals vs. last month" reads as a collapse). #4820.
153+
*
154+
* The remedy is deliberately NOT a second copy of the filter logic on the
155+
* compare path — two implementations of one rule diverge again at the next
156+
* change. Both paths call the same split and the same pass.
157+
*/
158+
export function splitMeasuresByFilter(
159+
measures: Iterable<string>,
160+
measureFilters: Record<string, FilterCondition | undefined>,
161+
): { unfiltered: string[]; filtered: string[] } {
162+
const unfiltered: string[] = [];
163+
const filtered: string[] = [];
164+
for (const m of measures) (measureFilters[m] ? filtered : unfiltered).push(m);
165+
return { unfiltered, filtered };
166+
}
167+
136168
/**
137169
* Evaluate derived measures on each aggregated row, mutating a shallow copy.
138170
* Division by zero (and missing operands) yields `null` rather than Infinity/NaN.
@@ -544,11 +576,7 @@ export class DatasetExecutor {
544576
}
545577

546578
// Split measures into those with a scoped filter and those without.
547-
const unfiltered: string[] = [];
548-
const filtered: string[] = [];
549-
for (const m of baseMeasures) {
550-
(compiled.measureFilters[m] ? filtered : unfiltered).push(m);
551-
}
579+
const { unfiltered, filtered } = splitMeasuresByFilter(baseMeasures, compiled.measureFilters);
552580

553581
const baseFilter = combineFilters(compiled.filter, selection.runtimeFilter);
554582
const dimensions = selection.dimensions ?? [];
@@ -584,34 +612,17 @@ export class DatasetExecutor {
584612
? { order, limit: selection.limit, offset: selection.offset }
585613
: undefined;
586614

587-
// Primary query: all unfiltered base measures in one pass. When every base
588-
// measure is filter-scoped, the supplementary queries below build the grid.
589-
let result: AnalyticsResult;
590-
if (unfiltered.length > 0 || filtered.length === 0) {
591-
result = await this.service.query(this.buildQuery(compiled, {
592-
measures: unfiltered,
593-
dimensions,
594-
where: baseFilter,
595-
selection,
596-
contextTimezone: context?.timezone,
597-
window: windowQuery,
598-
}), context);
599-
} else {
600-
result = { rows: [], fields: [] };
601-
}
602-
603-
// Supplementary queries: one per measure-scoped filter, merged by dimension key.
604-
for (const m of filtered) {
605-
const mFilter = combineFilters(baseFilter, compiled.measureFilters[m]);
606-
const sub = await this.service.query(this.buildQuery(compiled, {
607-
measures: [m], dimensions, where: mFilter, selection,
608-
contextTimezone: context?.timezone,
609-
}), context);
610-
result.rows = mergeByDimensions(result.rows, sub.rows, dimensions, [m]);
611-
result.fields.push({ name: m, type: 'number' });
612-
}
615+
// The current-period pass: unfiltered base measures in one query plus one
616+
// supplementary query per measure-scoped filter, merged by dimension key.
617+
const result = await this.runMeasurePass(compiled, selection, {
618+
measures: [...baseMeasures],
619+
dimensions,
620+
baseFilter,
621+
window: windowQuery,
622+
context,
623+
});
613624

614-
// compareTo — run a shifted query over the same base measures and attach.
625+
// compareTo — run the SAME pass over the shifted window and attach.
615626
if (selection.compareTo) {
616627
const compareRows = await this.runCompare(compiled, selection, [...baseMeasures], dimensions, baseFilter, context);
617628
result.rows = mergeByDimensions(
@@ -682,6 +693,80 @@ export class DatasetExecutor {
682693
return result;
683694
}
684695

696+
/**
697+
* Run ONE grouped pass over a set of base measures, honouring each measure's
698+
* own scoped `filter`: the unfiltered measures in a single query, plus one
699+
* supplementary query per filter-scoped measure, merged back by dimension key.
700+
*
701+
* **This is the executor's only implementation of "how a measure filter is
702+
* applied", and every window goes through it** — the current period, each
703+
* `totals` subset (which re-enters via `executeSelection`), and the
704+
* `compareTo` window. Before #4820 the comparison window had its own,
705+
* simpler answer: one shifted query over all base measures with only the
706+
* base filter, so `compiled.measureFilters` was never read on that path.
707+
* `won_count` counted won deals and `won_count__compare` counted every deal,
708+
* under one label, in adjacent columns. Only measures carrying a filter were
709+
* wrong — which is what made it survive: the unfiltered ones next to them
710+
* compared correctly.
711+
*
712+
* The caller supplies the `selection` this pass queries under, which is how
713+
* the comparison window differs at all: same measures, same dimensions, same
714+
* filters — a `timeDimensions` shifted by {@link shiftRange}. Nothing else
715+
* about the two passes may drift, because anything that does becomes a
716+
* discrepancy between two columns the reader is invited to subtract.
717+
*
718+
* Cost: one extra query per filter-scoped measure when `compareTo` is set.
719+
* The alternative — declaring the discrepancy in the response — is not one,
720+
* since the two columns exist to be directly comparable.
721+
*
722+
* @param window - Ordering/window to push into the SQL. Only ever set for a
723+
* selection the caller proved is a single self-sufficient query; a pass
724+
* that fans out must return its whole grid for the merge.
725+
*/
726+
private async runMeasurePass(
727+
compiled: CompiledDataset,
728+
selection: DatasetSelection,
729+
opts: {
730+
measures: string[];
731+
dimensions: string[];
732+
baseFilter?: FilterCondition;
733+
window?: { order?: Record<string, 'asc' | 'desc'>; limit?: number; offset?: number };
734+
context?: ExecutionContext;
735+
},
736+
): Promise<AnalyticsResult> {
737+
const { measures, dimensions, baseFilter, window, context } = opts;
738+
const { unfiltered, filtered } = splitMeasuresByFilter(measures, compiled.measureFilters);
739+
740+
// Primary query: all unfiltered base measures in one pass. When every base
741+
// measure is filter-scoped, the supplementary queries below build the grid.
742+
let result: AnalyticsResult;
743+
if (unfiltered.length > 0 || filtered.length === 0) {
744+
result = await this.service.query(this.buildQuery(compiled, {
745+
measures: unfiltered,
746+
dimensions,
747+
where: baseFilter,
748+
selection,
749+
contextTimezone: context?.timezone,
750+
window,
751+
}), context);
752+
} else {
753+
result = { rows: [], fields: [] };
754+
}
755+
756+
// Supplementary queries: one per measure-scoped filter, merged by dimension key.
757+
for (const m of filtered) {
758+
const mFilter = combineFilters(baseFilter, compiled.measureFilters[m]);
759+
const sub = await this.service.query(this.buildQuery(compiled, {
760+
measures: [m], dimensions, where: mFilter, selection,
761+
contextTimezone: context?.timezone,
762+
}), context);
763+
result.rows = mergeByDimensions(result.rows, sub.rows, dimensions, [m]);
764+
result.fields.push({ name: m, type: 'number' });
765+
}
766+
767+
return result;
768+
}
769+
685770
/**
686771
* The selected dimensions the compiled cube types as `time`, in selection
687772
* order (#3916) — the axis {@link resolveOrdering} defaults to ascending.
@@ -796,21 +881,25 @@ export class DatasetExecutor {
796881
const shiftedTd = (selection.timeDimensions ?? []).map((t) =>
797882
t.dimension === cmp.dimension ? { ...t, dateRange: shifted } : t,
798883
);
799-
// Built through `buildQuery` so the comparison pass buckets its date
800-
// dimensions EXACTLY like the primary pass. Hand-rolling the query here
801-
// skipped granularity resolution, so a bucketed primary grid ("2026-04")
802-
// was merged against raw-timestamp comparison rows and no dimension key
803-
// ever matched — every `__compare` column came back empty. The shifted
804-
// `timeDimensions` still win for their own dimension (rule 1 of the
805-
// precedence chain); `window` is deliberately omitted — the comparison grid
806-
// must stay whole for the merge.
807-
const sub = await this.service.query(this.buildQuery(compiled, {
808-
measures,
809-
dimensions,
810-
where: baseFilter,
811-
selection: { ...selection, timeDimensions: shiftedTd },
812-
contextTimezone: context?.timezone,
813-
}), context);
884+
// Run the SAME pass the current period ran, over the shifted window: same
885+
// measures, same dimensions, same base filter, and — since #4820 — the same
886+
// measure-scoped filters, applied by the same supplementary sub-queries.
887+
// Issuing one flat query here instead is what made `<measure>__compare`
888+
// report a different measure than the column beside it.
889+
//
890+
// Going through `runMeasurePass` (and so `buildQuery`) also keeps the
891+
// comparison pass bucketing its date dimensions EXACTLY like the primary
892+
// pass. Hand-rolling the query here skipped granularity resolution, so a
893+
// bucketed primary grid ("2026-04") was merged against raw-timestamp
894+
// comparison rows and no dimension key ever matched — every `__compare`
895+
// column came back empty. The shifted `timeDimensions` still win for their
896+
// own dimension (rule 1 of the precedence chain); `window` is deliberately
897+
// omitted — the comparison grid must stay whole for the merge.
898+
const sub = await this.runMeasurePass(
899+
compiled,
900+
{ ...selection, timeDimensions: shiftedTd },
901+
{ measures, dimensions, baseFilter, context },
902+
);
814903
// Rename measure columns to `<measure>__compare` so they merge alongside primary.
815904
return sub.rows.map((row) => {
816905
const out: Record<string, unknown> = {};

0 commit comments

Comments
 (0)