Skip to content

Commit 9f47b7b

Browse files
feat(cohorts): anon-inclusive event cohorts — property→v2, name-only→raw events
Event-based cohorts silently undercounted anonymous users: both cohort MVs bake `WHERE profile_id != device_id` at write time, so pre-identify events never entered them. - Property criteria ("did X where flag=Y") → profile_event_property_summary_v2 (anon-inclusive), gated on timeframe start >= COHORTS_V2_START_DATE (default 2026-07-01, env-overridable); older timeframes fall back to the v1 MV. Pure table-name swap — same countMerge(event_count)/event_date aggregates. - Name-only criteria ("did X", any N×) → raw events (GROUP BY profile_id HAVING count() <op> / SELECT DISTINCT). Exact for any frequency, anon-inclusive, faster than v2 (whose ARRAY JOIN explodes name-only reads). Removes the only cohort read of cohort_events_mv. - Extract operatorClause into shared filter-operators.ts; use in the cohort property branch + profile.service. Fixes doesNotContain/startsWith/gt/regex silently falling through to `IN`. - Drop dead filterClause chain in buildEventCriteriaQuery. Validated on shortreels: property cohort 13,862 (v1) → 110,651 (v2) in 0.3s. cohort_events_mv table kept — retention (trpc/chart.ts) still reads it.
1 parent 155fff2 commit 9f47b7b

5 files changed

Lines changed: 134 additions & 104 deletions

File tree

.env.example

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,3 +67,11 @@ PROFILES_BEHAVIORAL_V2_PROJECTS=""
6767
# docs/v2-migration-progress.md). Drop to 2026-07-01 once the 07-10 → 07-12
6868
# recovery backfill lands.
6969
PROFILES_BEHAVIORAL_V2_START_DATE="2026-07-12"
70+
71+
# Cohorts: property criteria ("did X where flag=Y") route to the anon-inclusive
72+
# v2 property MV when the criterion's timeframe STARTS on/after this date (all
73+
# projects — no allowlist); older timeframes fall back to the v1 anon-excluded
74+
# MV. Name-only criteria always use raw events (source of truth, no gate).
75+
# NOTE: the 07-11 v2 gap (~80% coverage) still sits inside this window — bump to
76+
# 2026-07-12 to exclude it until the 07-11 re-backfill lands.
77+
COHORTS_V2_START_DATE="2026-07-01"

packages/db/src/services/cohort.service.test.ts

Lines changed: 21 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,23 @@ import {
1616
updateCohortMembership,
1717
} from './cohort.service';
1818

19-
// Mock the ch and db modules
19+
// Mock the ch and db modules. mockChQuery is created via vi.hoisted so the
20+
// hoisted vi.mock factory can reference it (a plain top-level const would be
21+
// initialised AFTER the hoisted factory runs → ReferenceError).
22+
const { mockChQuery } = vi.hoisted(() => ({ mockChQuery: vi.fn() }));
23+
2024
vi.mock('../clickhouse/client', () => ({
2125
ch: vi.fn(),
22-
chQuery: vi.fn(),
26+
chQuery: mockChQuery,
2327
TABLE_NAMES: {
2428
events: 'events',
2529
profiles: 'profiles',
2630
cohort_members: 'cohort_members',
2731
cohort_metadata: 'cohort_metadata',
2832
profile_event_summary_mv: 'profile_event_summary_mv',
33+
profile_event_property_summary_mv: 'profile_event_property_summary_mv',
34+
profile_event_property_summary_v2: 'profile_event_property_summary_v2',
35+
cohort_events_mv: 'cohort_events_mv',
2936
},
3037
}));
3138

@@ -38,15 +45,6 @@ vi.mock('../prisma-client', () => ({
3845
},
3946
}));
4047

41-
const mockChQuery = vi.fn();
42-
vi.mock('../clickhouse/client', async () => {
43-
const actual = await vi.importActual('../clickhouse/client');
44-
return {
45-
...actual,
46-
chQuery: mockChQuery,
47-
};
48-
});
49-
5048
describe('Cohort Service', () => {
5149
beforeEach(() => {
5250
vi.clearAllMocks();
@@ -62,7 +60,7 @@ describe('Cohort Service', () => {
6260
name: 'page_view',
6361
filters: [],
6462
timeframe: { type: 'relative', value: '30d' },
65-
frequency: { operator: 'gte', value: 1 },
63+
frequency: { operator: 'at_least', count: 1 },
6664
},
6765
],
6866
operator: 'or',
@@ -94,7 +92,7 @@ describe('Cohort Service', () => {
9492
name: 'purchase',
9593
filters: [],
9694
timeframe: { type: 'relative', value: '7d' },
97-
frequency: { operator: 'gte', value: 1 },
95+
frequency: { operator: 'at_least', count: 1 },
9896
},
9997
],
10098
operator: 'and',
@@ -111,7 +109,7 @@ describe('Cohort Service', () => {
111109
expect(result).toEqual(['user1', 'user2']);
112110
expect(mockChQuery).toHaveBeenCalledTimes(1);
113111
// Verify INTERSECT is used for AND
114-
const query = mockChQuery.mock.calls[0][0];
112+
const query = mockChQuery.mock.calls[0]![0];
115113
expect(query).toContain('INTERSECT');
116114
});
117115

@@ -131,7 +129,7 @@ describe('Cohort Service', () => {
131129
},
132130
],
133131
timeframe: { type: 'relative', value: '7d' },
134-
frequency: { operator: 'gte', value: 3 },
132+
frequency: { operator: 'at_least', count: 3 },
135133
},
136134
],
137135
operator: 'or',
@@ -154,7 +152,7 @@ describe('Cohort Service', () => {
154152
{
155153
name: 'signup',
156154
filters: [],
157-
timeframe: { type: 'absolute', value: '2024-01-01' },
155+
timeframe: { type: 'absolute', start: '2024-01-01' },
158156
},
159157
],
160158
operator: 'or',
@@ -166,7 +164,7 @@ describe('Cohort Service', () => {
166164
const result = await computeEventBasedCohort('project-123', definition);
167165

168166
expect(result).toEqual(['user1']);
169-
const query = mockChQuery.mock.calls[0][0];
167+
const query = mockChQuery.mock.calls[0]![0];
170168
expect(query).toContain('2024-01-01');
171169
});
172170
});
@@ -180,7 +178,7 @@ describe('Cohort Service', () => {
180178
{
181179
id: 'p1',
182180
name: 'email',
183-
operator: 'isSet',
181+
operator: 'isNotNull',
184182
value: [],
185183
},
186184
],
@@ -267,7 +265,7 @@ describe('Cohort Service', () => {
267265
{
268266
id: 'p1',
269267
name: 'email',
270-
operator: 'isSet',
268+
operator: 'isNotNull',
271269
value: [],
272270
},
273271
],
@@ -288,13 +286,14 @@ describe('Cohort Service', () => {
288286
mockChQuery.mockResolvedValueOnce(undefined);
289287

290288
await storeCohortMembership(
291-
'cohort-123',
292289
'project-123',
290+
'cohort-123',
293291
['user1', 'user2', 'user3'],
292+
1,
294293
);
295294

296295
expect(mockChQuery).toHaveBeenCalledTimes(1);
297-
const query = mockChQuery.mock.calls[0][0];
296+
const query = mockChQuery.mock.calls[0]![0];
298297
expect(query).toContain('cohort_members');
299298
expect(query).toContain('cohort-123');
300299
expect(query).toContain('project-123');
@@ -303,7 +302,7 @@ describe('Cohort Service', () => {
303302
it('should handle empty profile list', async () => {
304303
mockChQuery.mockResolvedValueOnce(undefined);
305304

306-
await storeCohortMembership('cohort-123', 'project-123', []);
305+
await storeCohortMembership('project-123', 'cohort-123', [], 1);
307306

308307
// Should still execute to clear old members
309308
expect(mockChQuery).toHaveBeenCalledTimes(1);

packages/db/src/services/cohort.service.ts

Lines changed: 57 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,13 @@ import type { IChartEventFilter } from '@openpanel/validation';
1111

1212
import { ch, chQuery, TABLE_NAMES } from '../clickhouse/client';
1313
import { db } from '../prisma-client';
14-
import { getEventFiltersWhereClause } from './chart.service';
14+
import { operatorClause } from './filter-operators';
15+
16+
// v2 property MV (profile_event_property_summary_v2) is anon-inclusive but only
17+
// backfilled from this date forward; June/pre-July is dirty/partial. Property
18+
// cohorts whose timeframe STARTS on/after this route to v2 (all projects); older
19+
// ones fall back to the anon-excluded v1 MV. Env-overridable coverage date.
20+
const COHORTS_V2_START_DATE = process.env.COHORTS_V2_START_DATE || '2026-07-01';
1521

1622
/**
1723
* Build time constraint SQL from timeframe
@@ -39,6 +45,32 @@ function buildTimeConstraint(timeframe: Timeframe): string {
3945
}
4046
}
4147

48+
/**
49+
* The UTC start day of a criterion's timeframe. Relative "Nd" resolves to
50+
* N days before today; absolute uses its `start`. Used only to gate v1 vs v2.
51+
*/
52+
function criteriaTimeframeStart(timeframe: Timeframe): Date {
53+
if (timeframe.type === 'relative') {
54+
const match = timeframe.value.match(/^(\d+)d$/);
55+
const days = match ? Number.parseInt(match[1]!, 10) : 0;
56+
const d = new Date();
57+
d.setUTCDate(d.getUTCDate() - days);
58+
return d;
59+
}
60+
return new Date(`${timeframe.start}T00:00:00Z`);
61+
}
62+
63+
/**
64+
* Property cohorts route to the anon-inclusive v2 MV only when the whole
65+
* timeframe sits within v2's coverage window (start >= COHORTS_V2_START_DATE);
66+
* otherwise fall back to v1 so we never read a partial/absent v2 range.
67+
*/
68+
function canRouteCohortToV2(criteria: EventCriteria): boolean {
69+
const start = criteriaTimeframeStart(criteria.timeframe);
70+
const gate = new Date(`${COHORTS_V2_START_DATE}T00:00:00Z`);
71+
return start.getTime() >= gate.getTime();
72+
}
73+
4274
/**
4375
* Convert frequency operator to SQL comparison
4476
*/
@@ -74,56 +106,39 @@ export function buildEventCriteriaQuery(
74106
? `AND profile_id IN (${profileIdPrefilter})`
75107
: '';
76108

77-
// Build event filters
78-
const filterWhere = filters.length > 0
79-
? getEventFiltersWhereClause(filters)
80-
: {};
81-
const filterClauses = Object.values(filterWhere);
82-
const filterClause = filterClauses.length > 0
83-
? `AND ${filterClauses.join(' AND ')}`
84-
: '';
85-
86109
// Check if there are event property filters
87110
const hasEventPropertyFilters = filters.some(
88111
(f) => f.name.startsWith('properties.') && !f.name.startsWith('profile.properties.')
89112
);
90113

91-
// Use property-aware MV for event property filters
114+
// PROPERTY criteria ("did X where flag=Y") → property summary MV. Anon-inclusive
115+
// v2 when the timeframe sits in v2's coverage window, else the v1 fallback. Both
116+
// MVs share the same schema (name/property_key/property_value/event_date/
117+
// countMerge(event_count)/profile_id) so this is a pure table-name swap.
92118
if (hasEventPropertyFilters) {
119+
const propertyTable = canRouteCohortToV2(criteria)
120+
? TABLE_NAMES.profile_event_property_summary_v2
121+
: TABLE_NAMES.profile_event_property_summary_mv;
122+
93123
const propertyFilters = filters.filter(
94124
(f) => f.name.startsWith('properties.')
95125
);
96126

97-
// Build WHERE conditions for property filters
127+
// One `(property_key = k AND <value predicate>)` per filter, OR'd together.
128+
// operatorClause handles ALL operators (contains/doesNotContain/gt/regex/…);
129+
// the old inline switch fell through to `IN` for anything but is/isNot/
130+
// contains/doesNotContain — silently inverting doesNotContain etc.
98131
const propertyConditions = propertyFilters.map((filter) => {
99132
const propertyKey = filter.name.replace('properties.', '');
100133
const { value, operator } = filter;
101-
102-
switch (operator) {
103-
case 'is':
104-
if (value.length === 1) {
105-
return `(property_key = ${sqlstring.escape(propertyKey)} AND property_value = ${sqlstring.escape(String(value[0]).trim())})`;
106-
}
107-
return `(property_key = ${sqlstring.escape(propertyKey)} AND property_value IN (${value.map((val) => sqlstring.escape(String(val).trim())).join(', ')}))`;
108-
case 'isNot':
109-
if (value.length === 1) {
110-
return `(property_key = ${sqlstring.escape(propertyKey)} AND property_value != ${sqlstring.escape(String(value[0]).trim())})`;
111-
}
112-
return `(property_key = ${sqlstring.escape(propertyKey)} AND property_value NOT IN (${value.map((val) => sqlstring.escape(String(val).trim())).join(', ')}))`;
113-
case 'contains':
114-
return `(property_key = ${sqlstring.escape(propertyKey)} AND (${value.map((val) => `property_value LIKE ${sqlstring.escape(`%${String(val).trim()}%`)}`).join(' OR ')}))`;
115-
case 'doesNotContain':
116-
return `(property_key = ${sqlstring.escape(propertyKey)} AND (${value.map((val) => `property_value NOT LIKE ${sqlstring.escape(`%${String(val).trim()}%`)}`).join(' AND ')}))`;
117-
default:
118-
return `(property_key = ${sqlstring.escape(propertyKey)} AND property_value IN (${value.map((val) => sqlstring.escape(String(val).trim())).join(', ')}))`;
119-
}
134+
return `(property_key = ${sqlstring.escape(propertyKey)} AND ${operatorClause('property_value', operator, value)})`;
120135
}).join(' OR ');
121136

122137
if (frequency) {
123138
const frequencyOp = getFrequencyOperator(frequency);
124139
return `
125140
SELECT profile_id
126-
FROM ${TABLE_NAMES.profile_event_property_summary_mv}
141+
FROM ${propertyTable}
127142
WHERE project_id = ${sqlstring.escape(projectId)}
128143
AND name = ${sqlstring.escape(name)}
129144
AND ${timeConstraint.replace('created_at', 'event_date')}
@@ -136,7 +151,7 @@ export function buildEventCriteriaQuery(
136151

137152
return `
138153
SELECT DISTINCT profile_id
139-
FROM ${TABLE_NAMES.profile_event_property_summary_mv}
154+
FROM ${propertyTable}
140155
WHERE project_id = ${sqlstring.escape(projectId)}
141156
AND name = ${sqlstring.escape(name)}
142157
AND ${timeConstraint.replace('created_at', 'event_date')}
@@ -145,27 +160,31 @@ export function buildEventCriteriaQuery(
145160
`;
146161
}
147162

148-
// cohort_events_mv: name is 2nd in sort key (vs profile_id 2nd in
149-
// profile_event_summary_mv) — much better prefix match for these filters.
163+
// NAME-ONLY criteria ("did X", any N×) → raw events. The property MV explodes
164+
// each event into one row per property (ARRAY JOIN), so countMerge over it
165+
// massively overcounts frequency; and cohort_events_mv is anon-excluded. Raw
166+
// events is exact for any frequency, anon-inclusive, and faster than v2 here
167+
// (name is an effective sort-key prefix via the proj_funnel projection). Uses
168+
// created_at + plain count() — no event_date/countMerge rewrite.
150169
if (frequency) {
151170
const frequencyOp = getFrequencyOperator(frequency);
152171

153172
return `
154173
SELECT profile_id
155-
FROM ${TABLE_NAMES.cohort_events_mv}
174+
FROM ${TABLE_NAMES.events}
156175
WHERE project_id = ${sqlstring.escape(projectId)}
157176
AND name = ${sqlstring.escape(name)}
158177
AND ${timeConstraint}
159178
${prefilterClause}
160179
GROUP BY profile_id
161-
HAVING sum(event_count) ${frequencyOp}
180+
HAVING count() ${frequencyOp}
162181
`;
163182
}
164183

165184
// For simple "did event" queries
166185
return `
167186
SELECT DISTINCT profile_id
168-
FROM ${TABLE_NAMES.cohort_events_mv}
187+
FROM ${TABLE_NAMES.events}
169188
WHERE project_id = ${sqlstring.escape(projectId)}
170189
AND name = ${sqlstring.escape(name)}
171190
AND ${timeConstraint}
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
import sqlstring from 'sqlstring';
2+
3+
// Full operator support for a value filter, shared by the profiles behavioural
4+
// filter and cohort event-property criteria. `col` is the SQL expression the
5+
// operator compares (e.g. `property_value`, `properties['x']`, a bare column).
6+
//
7+
// Fixes the old "every unhandled operator falls through to IN" bug — e.g.
8+
// `doesNotContain` used to emit `col IN (...)`, the exact OPPOSITE set.
9+
export function operatorClause(
10+
col: string,
11+
operator: string | undefined,
12+
values: (string | number | boolean | null)[],
13+
): string {
14+
const trimmed = values.map((v) => String(v).trim());
15+
const esc = (v: string) => sqlstring.escape(v);
16+
const inList = trimmed.map(esc).join(', ');
17+
const anyLike = (pat: (v: string) => string) =>
18+
`(${trimmed.map((v) => `${col} LIKE ${esc(pat(v))}`).join(' OR ')})`;
19+
switch (operator) {
20+
case 'isNot':
21+
return `${col} NOT IN (${inList})`;
22+
case 'contains':
23+
return anyLike((v) => `%${v}%`);
24+
case 'doesNotContain':
25+
return `(${trimmed.map((v) => `${col} NOT LIKE ${esc(`%${v}%`)}`).join(' OR ')})`;
26+
case 'startsWith':
27+
return anyLike((v) => `${v}%`);
28+
case 'endsWith':
29+
return anyLike((v) => `%${v}`);
30+
case 'regex':
31+
return `(${trimmed.map((v) => `match(${col}, ${esc(v)})`).join(' OR ')})`;
32+
case 'isNull':
33+
return `(${col} = '' OR ${col} IS NULL)`;
34+
case 'isNotNull':
35+
return `(${col} != '' AND ${col} IS NOT NULL)`;
36+
case 'gt':
37+
return `(${trimmed.map((v) => `toFloat64OrZero(${col}) > toFloat64(${esc(v)})`).join(' OR ')})`;
38+
case 'lt':
39+
return `(${trimmed.map((v) => `toFloat64OrZero(${col}) < toFloat64(${esc(v)})`).join(' OR ')})`;
40+
case 'gte':
41+
return `(${trimmed.map((v) => `toFloat64OrZero(${col}) >= toFloat64(${esc(v)})`).join(' OR ')})`;
42+
case 'lte':
43+
return `(${trimmed.map((v) => `toFloat64OrZero(${col}) <= toFloat64(${esc(v)})`).join(' OR ')})`;
44+
default: // 'is' and any unknown operator
45+
return `${col} IN (${inList})`;
46+
}
47+
}

0 commit comments

Comments
 (0)