From a01f3d6269c0d9894ecfe37ec65aa483e8337504 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl-Gerhard=20Lindesv=C3=A4rd?= Date: Tue, 1 Sep 2026 19:50:50 +0000 Subject: [PATCH 1/2] feat(cohorts): let a criterion say "never did this event" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Completed signup but never started a subscription" could not be built. The frequency input rejected 0, and even with 0 accepted the generated query returned nothing: the summary MVs hold a row only for an event that actually fired, so no GROUP BY over them can produce a group with countMerge(event_count) = 0. Relaxing the input alone would have shipped a cohort that silently comes back empty. So a count of 0 with "exactly" or "at most" now builds an inverted query — the project's profiles minus the ones the MV knows about — with the timeframe and any event-property predicates inside the exclusion, so "never did X in the last 30 days" still includes someone who did X 60 days ago. "At least 0" matches every profile and is rejected instead. The LIMIT in computeEventBasedCohort now wraps the combined query rather than trailing it. ClickHouse applies a trailing LIMIT to the last SELECT of an INTERSECT chain alone, which was survivable while every operand was a narrow event-derived set; a "never did X" operand is most of the project, so the preview's limit of 10 would have cut the cohort to nothing. Co-Authored-By: Claude Opus 5 --- .../cohort/cohort-criteria-builder.tsx | 26 ++- .../db/src/services/cohort.service.test.ts | 176 +++++++++++++++++- packages/db/src/services/cohort.service.ts | 117 ++++++++++-- .../validation/src/cohort.validation.test.ts | 33 +++- packages/validation/src/cohort.validation.ts | 18 +- 5 files changed, 336 insertions(+), 34 deletions(-) diff --git a/apps/start/src/components/cohort/cohort-criteria-builder.tsx b/apps/start/src/components/cohort/cohort-criteria-builder.tsx index ed9c199fc..e535d46c7 100644 --- a/apps/start/src/components/cohort/cohort-criteria-builder.tsx +++ b/apps/start/src/components/cohort/cohort-criteria-builder.tsx @@ -273,15 +273,19 @@ function EventCriteriaItem({
+ onChange={(operator) => { + const count = criteria.frequency?.count ?? 1; onChange({ ...criteria, frequency: { - ...(criteria.frequency ?? { count: 1 }), operator, + // "At least 0" matches everyone and the server rejects it, + // so a count of 0 only survives the operators that mean + // "never". + count: operator === 'gte' && count === 0 ? 1 : count, }, - }) - } + }); + }} items={[ { value: 'gte', label: 'At least' }, { value: 'eq', label: 'Exactly' }, @@ -297,17 +301,21 @@ function EventCriteriaItem({ + onChange={(e) => { + // Explicit NaN check, not `|| 1`: 0 is falsy, so the fallback + // rewrote a typed 0 back to 1 and "never did this event" could + // not be entered. + const parsed = Number.parseInt(e.target.value, 10); onChange({ ...criteria, frequency: { operator: criteria.frequency?.operator ?? 'gte', - count: Number.parseInt(e.target.value) || 1, + count: Number.isNaN(parsed) ? 1 : parsed, }, - }) - } + }); + }} className="w-20 rounded border px-2 py-1 text-sm" /> diff --git a/packages/db/src/services/cohort.service.test.ts b/packages/db/src/services/cohort.service.test.ts index f052988d2..9d13cfd6b 100644 --- a/packages/db/src/services/cohort.service.test.ts +++ b/packages/db/src/services/cohort.service.test.ts @@ -1,6 +1,13 @@ -import type { EventCriteria } from '@openpanel/validation'; +import type { + EventBasedCohortDefinition, + EventCriteria, +} from '@openpanel/validation'; import { describe, expect, it } from 'vitest'; -import { buildEventCriteriaQuery } from './cohort.service'; +import { TABLE_NAMES } from '../clickhouse/client'; +import { + buildEventBasedCohortQuery, + buildEventCriteriaQuery, +} from './cohort.service'; const PROJECT_ID = 'test-cohort-timeframe'; @@ -88,3 +95,168 @@ describe('buildEventCriteriaQuery timeframe escaping', () => { ).toContain('event_date >= toDate(now() - INTERVAL 30 DAY)'); }); }); + +const LAST_30_DAYS = { + type: 'relative', + value: '30d', +} satisfies EventCriteria['timeframe']; + +function frequencyCriteria( + frequency: EventCriteria['frequency'], + filters: EventCriteria['filters'] = [] +): EventCriteria { + return { + name: 'subscription_started', + filters, + timeframe: LAST_30_DAYS, + frequency, + }; +} + +// Everything the outer profile scan sees, i.e. the query with the NOT IN +// subquery cut out. +function outsideExclusion(sql: string): string { + const open = sql.indexOf('NOT IN ('); + if (open === -1) { + return sql; + } + return sql.slice(0, open) + sql.slice(sql.lastIndexOf(')') + 1); +} + +describe('buildEventCriteriaQuery zero frequency', () => { + it.each(['eq', 'lte'] as const)( + 'excludes anyone who did the event when the count is 0 (%s)', + (operator) => { + const sql = buildEventCriteriaQuery( + PROJECT_ID, + frequencyCriteria({ operator, count: 0 }) + ); + + // The summary MV has no row for zero occurrences, so a HAVING can never + // match here — the query has to be inverted. + expect(sql).not.toContain('HAVING'); + expect(sql).toContain('SELECT DISTINCT id AS profile_id'); + expect(sql).toContain(`FROM ${TABLE_NAMES.profiles}`); + expect(sql).toContain('NOT IN ('); + expect(sql).toContain(`FROM ${TABLE_NAMES.event_profile_summary_mv}`); + expect(sql).toContain("name = 'subscription_started'"); + } + ); + + it('gives eq 0 and lte 0 the same query', () => { + expect( + buildEventCriteriaQuery( + PROJECT_ID, + frequencyCriteria({ operator: 'eq', count: 0 }) + ) + ).toBe( + buildEventCriteriaQuery( + PROJECT_ID, + frequencyCriteria({ operator: 'lte', count: 0 }) + ) + ); + }); + + it('keeps the timeframe inside the exclusion, not on the profile scan', () => { + const sql = buildEventCriteriaQuery( + PROJECT_ID, + frequencyCriteria({ operator: 'eq', count: 0 }) + ); + + // "Never did X in the last 30 days" still includes someone who did X 60 + // days ago — which only holds while the date bound sits in the subquery. + expect(sql).toContain('event_date >= toDate(now() - INTERVAL 30 DAY)'); + expect(outsideExclusion(sql)).not.toContain('event_date'); + expect(outsideExclusion(sql)).toContain( + `project_id = '${PROJECT_ID}'` + ); + }); + + it('excludes on the matching property row when the criterion has property filters', () => { + const sql = buildEventCriteriaQuery( + PROJECT_ID, + frequencyCriteria({ operator: 'eq', count: 0 }, [ + { name: 'properties.plan', operator: 'is', value: ['pro'] }, + ]) + ); + + // Read as "has no matching (event, property) row": someone who did the + // event with plan = free is a member. + expect(sql).not.toContain('HAVING'); + expect(sql).toContain( + `FROM ${TABLE_NAMES.event_property_profile_summary_mv}` + ); + expect(sql).toContain("property_key = 'plan'"); + expect(outsideExclusion(sql)).not.toContain('property_key'); + }); + + it('leaves gte 0 on the ordinary path (zFrequency rejects it upstream)', () => { + const sql = buildEventCriteriaQuery( + PROJECT_ID, + frequencyCriteria({ operator: 'gte', count: 0 }) + ); + + expect(sql).toContain('HAVING countMerge(event_count) >= 0'); + expect(sql).not.toContain('NOT IN'); + }); + + it.each([ + ['gte', 1, '>= 1'], + ['eq', 2, '= 2'], + ['lte', 3, '<= 3'], + ] as const)( + 'still groups and filters for positive counts (%s %i)', + (operator, count, expected) => { + const sql = buildEventCriteriaQuery( + PROJECT_ID, + frequencyCriteria({ operator, count }) + ); + + expect(sql).toContain('GROUP BY profile_id'); + expect(sql).toContain(`HAVING countMerge(event_count) ${expected}`); + expect(sql).not.toContain(`FROM ${TABLE_NAMES.profiles}`); + } + ); +}); + +describe('buildEventBasedCohortQuery with a zero-count criterion', () => { + const definition = { + type: 'event', + criteria: { + operator: 'and', + events: [ + { + name: 'signup', + filters: [], + timeframe: LAST_30_DAYS, + frequency: { operator: 'gte', count: 1 }, + }, + frequencyCriteria({ operator: 'eq', count: 0 }), + ], + }, + } satisfies EventBasedCohortDefinition; + + it('intersects two sets of profile_id', () => { + const sql = buildEventBasedCohortQuery(PROJECT_ID, definition); + const [signedUp, neverSubscribed] = sql.split(' INTERSECT '); + + expect(neverSubscribed).toBeDefined(); + // Both operands have to be a bare set of profile_id for the INTERSECT to + // mean anything: same column name, one row per profile, no LIMIT or + // ORDER BY of their own. + expect(signedUp).toContain('SELECT profile_id'); + expect(neverSubscribed).toContain('SELECT DISTINCT id AS profile_id'); + expect(sql).not.toContain('ORDER BY'); + expect(sql).not.toContain('LIMIT'); + }); + + it('unions them under "or"', () => { + const sql = buildEventBasedCohortQuery(PROJECT_ID, { + ...definition, + criteria: { ...definition.criteria, operator: 'or' }, + }); + + expect(sql).toContain(' UNION DISTINCT '); + expect(sql).not.toContain(' INTERSECT '); + }); +}); diff --git a/packages/db/src/services/cohort.service.ts b/packages/db/src/services/cohort.service.ts index 508ed7b05..7c72b9c4e 100644 --- a/packages/db/src/services/cohort.service.ts +++ b/packages/db/src/services/cohort.service.ts @@ -129,6 +129,45 @@ function getFrequencyOperator(frequency: Frequency): string { } } +// "Exactly 0" and "at most 0" both mean the profile never did the event. +// Neither can be expressed as a HAVING on the summary MVs: those hold a row +// only for a (project, profile, event, day) that actually happened, so every +// group that reaches the HAVING already has countMerge(event_count) >= 1 and +// the criterion returns nothing. The query has to be inverted instead. +// +// `gte 0` is not "never" — it matches every profile — and zFrequency rejects +// it, so it stays on the ordinary HAVING path. +function isNeverFrequency(frequency: Frequency): boolean { + return ( + frequency.count === 0 && + (frequency.operator === 'eq' || frequency.operator === 'lte') + ); +} + +// Every profile in the project except the ones the summary MV knows about. +// The timeframe stays inside the subquery, so "never did X in the last 30 +// days" keeps including someone who did X 60 days ago, matching how the +// timeframe control reads for a positive criterion. +// +// DISTINCT rather than FINAL: profiles is a ReplacingMergeTree and FINAL +// cannot spill to disk, so on wide projects the dedup is what runs out of +// memory (same reason buildPropertyBasedCohortQuery groups instead of reading +// through FINAL). Only the id is needed here, so deduplicating it is enough — +// and the other branches of this function also emit one row per profile, which +// the INTERSECT / UNION DISTINCT combination in computeEventBasedCohort +// depends on. +function buildNeverDidEventQuery( + projectId: string, + didEventQuery: string, +): string { + return ` + SELECT DISTINCT id AS profile_id + FROM ${TABLE_NAMES.profiles} + WHERE project_id = ${sqlstring.escape(projectId)} + AND id NOT IN (${didEventQuery}) + `; +} + export function buildEventCriteriaQuery( projectId: string, criteria: EventCriteria, @@ -189,6 +228,23 @@ export function buildEventCriteriaQuery( .join(' OR '); if (frequency) { + if (isNeverFrequency(frequency)) { + // "Never did X where plan = pro" reads as "has no matching (event, + // property) row", so the property predicates go inside the exclusion: + // someone who did the event with plan = free is a member. + return buildNeverDidEventQuery( + projectId, + ` + SELECT profile_id + FROM ${TABLE_NAMES.event_property_profile_summary_mv} + WHERE project_id = ${sqlstring.escape(projectId)} + AND name = ${sqlstring.escape(name)} + AND ${timeConstraint.replace('created_at', 'event_date')} + AND (${propertyConditions}) + `, + ); + } + const frequencyOp = getFrequencyOperator(frequency); return ` SELECT profile_id @@ -213,6 +269,19 @@ export function buildEventCriteriaQuery( } if (frequency) { + if (isNeverFrequency(frequency)) { + return buildNeverDidEventQuery( + projectId, + ` + SELECT profile_id + FROM ${TABLE_NAMES.event_profile_summary_mv} + WHERE project_id = ${sqlstring.escape(projectId)} + AND name = ${sqlstring.escape(name)} + AND ${timeConstraint.replace('created_at', 'event_date')} + `, + ); + } + const frequencyOp = getFrequencyOperator(frequency); return ` SELECT profile_id @@ -305,23 +374,40 @@ export function buildPropertyBasedCohortQuery( `; } -export async function computeEventBasedCohort( +// Every criterion emits one row per matching profile under the column name +// profile_id, which is what lets them be combined as sets. +export function buildEventBasedCohortQuery( projectId: string, definition: EventBasedCohortDefinition, - limit?: number, -): Promise { +): string { const { events, operator } = definition.criteria; const queries = events.map((eventCriteria) => buildEventCriteriaQuery(projectId, eventCriteria), ); - const combinedQuery = - operator === 'and' - ? queries.join(' INTERSECT ') - : queries.join(' UNION DISTINCT '); + return operator === 'and' + ? queries.join(' INTERSECT ') + : queries.join(' UNION DISTINCT '); +} - const finalQuery = limit ? `${combinedQuery} LIMIT ${limit}` : combinedQuery; +export async function computeEventBasedCohort( + projectId: string, + definition: EventBasedCohortDefinition, + limit?: number, +): Promise { + const combinedQuery = buildEventBasedCohortQuery(projectId, definition); + + // The LIMIT has to wrap the combination, not trail it: appended to an + // INTERSECT / UNION chain, ClickHouse applies it to the last SELECT alone. + // That was survivable while every operand was a narrow event-derived set; + // a "never did X" operand is most of the project's profiles, so limiting it + // before the INTERSECT would cut the cohort down to an arbitrary slice — + // and at the preview's limit of 10, almost always to nothing. The count + // query below already wraps for the same reason. + const finalQuery = limit + ? `SELECT profile_id FROM (${combinedQuery}) LIMIT ${limit}` + : combinedQuery; const results = await chQuery<{ profile_id: string }>(finalQuery); return results.map((r) => r.profile_id); @@ -331,22 +417,17 @@ export async function countEventBasedCohort( projectId: string, definition: EventBasedCohortDefinition, ): Promise { - const { events, operator } = definition.criteria; - - const queries = events.map((eventCriteria) => - buildEventCriteriaQuery(projectId, eventCriteria), - ); - - const combinedQuery = - operator === 'and' - ? queries.join(' INTERSECT ') - : queries.join(' UNION DISTINCT '); + const combinedQuery = buildEventBasedCohortQuery(projectId, definition); const countQuery = `SELECT count() as count FROM (${combinedQuery})`; const results = await chQuery<{ count: number }>(countQuery); return results[0]?.count ?? 0; } +// Known gap, not fixed here: the switch below has no case for 'inCohort' or +// 'notInCohort', so one of those inside a cohort definition is dropped without +// an error and the cohort silently widens. The same operators do work at +// report level — see buildCohortClause in filter-where.service.ts. function getProfileFiltersWhereClause( filters: IChartEventFilter[], { latestPerProfileKey }: { latestPerProfileKey?: string } = {}, diff --git a/packages/validation/src/cohort.validation.test.ts b/packages/validation/src/cohort.validation.test.ts index 132ec03be..da675ea0a 100644 --- a/packages/validation/src/cohort.validation.test.ts +++ b/packages/validation/src/cohort.validation.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import { zAbsoluteTimeframe } from './cohort.validation'; +import { zAbsoluteTimeframe, zFrequency } from './cohort.validation'; describe('zAbsoluteTimeframe', () => { it('accepts a date without an end', () => { @@ -40,3 +40,34 @@ describe('zAbsoluteTimeframe', () => { ).toBe(false); }); }); + +describe('zFrequency', () => { + it.each(['eq', 'lte'] as const)( + 'accepts a count of 0 with %s ("never did this event")', + (operator) => { + expect(zFrequency.safeParse({ operator, count: 0 }).success).toBe(true); + } + ); + + it('rejects a count of 0 with gte, which would match every profile', () => { + expect(zFrequency.safeParse({ operator: 'gte', count: 0 }).success).toBe( + false + ); + }); + + it.each(['gte', 'eq', 'lte'] as const)( + 'still accepts positive counts with %s', + (operator) => { + expect(zFrequency.safeParse({ operator, count: 3 }).success).toBe(true); + } + ); + + it('still rejects negative and fractional counts', () => { + expect(zFrequency.safeParse({ operator: 'eq', count: -1 }).success).toBe( + false + ); + expect(zFrequency.safeParse({ operator: 'eq', count: 1.5 }).success).toBe( + false + ); + }); +}); diff --git a/packages/validation/src/cohort.validation.ts b/packages/validation/src/cohort.validation.ts index 00ec079b9..f2318117b 100644 --- a/packages/validation/src/cohort.validation.ts +++ b/packages/validation/src/cohort.validation.ts @@ -47,10 +47,20 @@ export const zTimeframe = z.discriminatedUnion('type', [ export type Timeframe = z.infer; -export const zFrequency = z.object({ - operator: z.enum(['gte', 'eq', 'lte']), - count: z.number().int().min(1), -}); +// A count of 0 is how a criterion says "never did this event", so it has to +// be accepted — but only with the two operators that read that way. `gte 0` +// matches every profile, which is not a criterion at all, and letting it +// through would leave the query builder with a third case to guess at. +export const zFrequency = z + .object({ + operator: z.enum(['gte', 'eq', 'lte']), + count: z.number().int().min(0), + }) + .refine((frequency) => frequency.count > 0 || frequency.operator !== 'gte', { + message: + 'A count of 0 means "never", which only "exactly" and "at most" express', + path: ['count'], + }); export type Frequency = z.infer; From a1bf1320591205bc9f281c635bb523cbf4f0502d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carl-Gerhard=20Lindesv=C3=A4rd?= Date: Tue, 1 Sep 2026 20:02:59 +0000 Subject: [PATCH 2/2] fix(cohorts): clamp gte+0 count input to 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The count input's onChange only guarded against NaN, so typing 0 while the operator is "At least" produced {operator: 'gte', count: 0} — a frequency zFrequency rejects. Apply the same "At least 0 matches everyone" clamp the operator-change handler already uses. --- .../src/components/cohort/cohort-criteria-builder.tsx | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/apps/start/src/components/cohort/cohort-criteria-builder.tsx b/apps/start/src/components/cohort/cohort-criteria-builder.tsx index e535d46c7..8e23e3e38 100644 --- a/apps/start/src/components/cohort/cohort-criteria-builder.tsx +++ b/apps/start/src/components/cohort/cohort-criteria-builder.tsx @@ -308,11 +308,17 @@ function EventCriteriaItem({ // rewrote a typed 0 back to 1 and "never did this event" could // not be entered. const parsed = Number.parseInt(e.target.value, 10); + const operator = criteria.frequency?.operator ?? 'gte'; onChange({ ...criteria, frequency: { - operator: criteria.frequency?.operator ?? 'gte', - count: Number.isNaN(parsed) ? 1 : parsed, + operator, + // Same "At least 0 matches everyone" clamp as the + // operator-change handler above. + count: + Number.isNaN(parsed) || (operator === 'gte' && parsed === 0) + ? 1 + : parsed, }, }); }}