diff --git a/apps/start/src/components/cohort/cohort-criteria-builder.tsx b/apps/start/src/components/cohort/cohort-criteria-builder.tsx
index ed9c199fc..8e23e3e38 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,27 @@ 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);
+ const operator = criteria.frequency?.operator ?? 'gte';
onChange({
...criteria,
frequency: {
- operator: criteria.frequency?.operator ?? 'gte',
- count: Number.parseInt(e.target.value) || 1,
+ operator,
+ // Same "At least 0 matches everyone" clamp as the
+ // operator-change handler above.
+ count:
+ Number.isNaN(parsed) || (operator === 'gte' && parsed === 0)
+ ? 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;