feat(cohorts): anon-inclusive event cohorts — property→v2, name-only→raw events - #397
feat(cohorts): anon-inclusive event cohorts — property→v2, name-only→raw events#397ayushjhanwar-png wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughCohort property queries now route to v1 or v2 ClickHouse summary tables based on timeframe start dates, use shared operator SQL generation, and query raw events for name-only frequency criteria. Environment documentation, table mappings, mocks, fixtures, and assertions were updated. ChangesCohort query routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CohortCriteria
participant buildEventCriteriaQuery
participant ClickHouse
CohortCriteria->>buildEventCriteriaQuery: event property criteria and timeframe
buildEventCriteriaQuery->>buildEventCriteriaQuery: select v1 or v2 property summary table
buildEventCriteriaQuery->>ClickHouse: execute property predicate query
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…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.
- New shared filter-operators.ts (operatorClause) used in the property branch —
fixes doesNotContain/startsWith/gt/regex silently falling through to `IN`.
- Add profile_event_property_summary_v2 to TABLE_NAMES.
- 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.
9f47b7b to
0396bbd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/src/services/cohort.service.ts (1)
114-187: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftRoute these new ClickHouse paths through the required query builder.
The v1/v2 table selection and raw-events branches continue constructing SQL templates directly. Refactor this path to use the custom query builder/query functions and ensure the execution path receives the timezone through
clix(ch, tz); escaped literals reduce injection risk but do not satisfy the repository query contract.As per coding guidelines, “ClickHouse queries must use the custom query builder at
packages/db/src/clickhouse/query-builder.ts” and “pass the timezone intoclix(ch, tz).”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/services/cohort.service.ts` around lines 114 - 187, The cohort query branches around canRouteCohortToV2, the propertyTable path, and the raw-events frequency/simple-event paths must stop returning interpolated SQL templates directly. Refactor these queries to use the repository’s custom query builder/query functions from query-builder.ts, preserving the existing table selection, filters, frequency behavior, and aggregation semantics, and ensure execution invokes clix(ch, tz) so the timezone is propagated.Source: Coding guidelines
🧹 Nitpick comments (1)
packages/db/src/services/cohort.service.test.ts (1)
116-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the property-filter route and assert the generated query.
This fixture uses
name: 'path', so it takes the name-only raw-events branch; it does not cover the newproperties.*routing. Useproperties.pathand assert the predicate/table, with cases on each side of the v1/v2 start-date gate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/src/services/cohort.service.test.ts` around lines 116 - 145, The test around computeEventBasedCohort should exercise the property-filter route by changing the filter name to properties.path and asserting the generated query includes the expected predicate and table. Add cases covering both sides of the v1/v2 start-date gate, while preserving the existing cohort result assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Around line 55-61: Update the COHORTS_V2_START_DATE default in .env.example
lines 55-61 from 2026-07-01 to 2026-07-12, and update the fallback default in
packages/db/src/services/cohort.service.ts line 20 to the same date. Keep the
routing behavior unchanged when an explicit configuration value is provided.
In `@packages/db/src/services/filter-operators.ts`:
- Around line 24-25: Update the doesNotContain branch in the filter operator
switch to join the generated NOT LIKE predicates with AND instead of OR,
ensuring every excluded value is absent while preserving the existing trimming,
escaping, and grouping behavior.
---
Outside diff comments:
In `@packages/db/src/services/cohort.service.ts`:
- Around line 114-187: The cohort query branches around canRouteCohortToV2, the
propertyTable path, and the raw-events frequency/simple-event paths must stop
returning interpolated SQL templates directly. Refactor these queries to use the
repository’s custom query builder/query functions from query-builder.ts,
preserving the existing table selection, filters, frequency behavior, and
aggregation semantics, and ensure execution invokes clix(ch, tz) so the timezone
is propagated.
---
Nitpick comments:
In `@packages/db/src/services/cohort.service.test.ts`:
- Around line 116-145: The test around computeEventBasedCohort should exercise
the property-filter route by changing the filter name to properties.path and
asserting the generated query includes the expected predicate and table. Add
cases covering both sides of the v1/v2 start-date gate, while preserving the
existing cohort result assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cdaeeca9-4719-44a0-a551-9ea353bc37ef
📒 Files selected for processing (5)
.env.examplepackages/db/src/clickhouse/client.tspackages/db/src/services/cohort.service.test.tspackages/db/src/services/cohort.service.tspackages/db/src/services/filter-operators.ts
| # Cohorts: property criteria ("did X where flag=Y") route to the anon-inclusive | ||
| # v2 property MV when the criterion's timeframe STARTS on/after this date (all | ||
| # projects — no allowlist); older timeframes fall back to the v1 anon-excluded | ||
| # MV. Name-only criteria always use raw events (source of truth, no gate). | ||
| # NOTE: the 07-11 v2 gap (~80% coverage) still sits inside this window — bump to | ||
| # 2026-07-12 to exclude it until the 07-11 re-backfill lands. | ||
| COHORTS_V2_START_DATE="2026-07-01" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not default routing into the documented partial-coverage window.
The July 11 v2 gap is explicitly documented as roughly 80% coverage, but both defaults remain 2026-07-01. Until the re-backfill is confirmed, this undercounts cohorts whose timeframe starts from July 1 through July 11.
.env.example#L55-L61: defaultCOHORTS_V2_START_DATEto2026-07-12.packages/db/src/services/cohort.service.ts#L20-L20: change the fallback default to2026-07-12as well.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 61-61: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
📍 Affects 2 files
.env.example#L55-L61(this comment)packages/db/src/services/cohort.service.ts#L20-L20
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.env.example around lines 55 - 61, Update the COHORTS_V2_START_DATE default
in .env.example lines 55-61 from 2026-07-01 to 2026-07-12, and update the
fallback default in packages/db/src/services/cohort.service.ts line 20 to the
same date. Keep the routing behavior unchanged when an explicit configuration
value is provided.
| case 'doesNotContain': | ||
| return `(${trimmed.map((v) => `${col} NOT LIKE ${esc(`%${v}%`)}`).join(' OR ')})`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Join multi-value doesNotContain predicates with AND.
NOT LIKE A OR NOT LIKE B admits values containing either excluded pattern. A “does not contain” filter must reject matches for all supplied values.
Proposed fix
- return `(${trimmed.map((v) => `${col} NOT LIKE ${esc(`%${v}%`)}`).join(' OR ')})`;
+ return `(${trimmed.map((v) => `${col} NOT LIKE ${esc(`%${v}%`)}`).join(' AND ')})`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case 'doesNotContain': | |
| return `(${trimmed.map((v) => `${col} NOT LIKE ${esc(`%${v}%`)}`).join(' OR ')})`; | |
| case 'doesNotContain': | |
| return `(${trimmed.map((v) => `${col} NOT LIKE ${esc(`%${v}%`)}`).join(' AND ')})`; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/src/services/filter-operators.ts` around lines 24 - 25, Update
the doesNotContain branch in the filter operator switch to join the generated
NOT LIKE predicates with AND instead of OR, ensuring every excluded value is
absent while preserving the existing trimming, escaping, and grouping behavior.
1 participant
Problem
Event-based cohorts silently undercount anonymous users. Both cohort MVs bake
WHERE profile_id != device_idat write time, so pre-identify (anonymous) events never enter them.Validated live on shortreels for a real property cohort (
userFlags where reel-based-for-you=…, since 2026-07-12):~8× audience was being dropped. The new query ran in 0.31s (635K rows read — property-pinned prefix scan).
Change
buildEventCriteriaQueryincohort.service.ts:profile_event_property_summary_v2(anon-inclusive), gated on the criterion's timeframe start ≥COHORTS_V2_START_DATE(default2026-07-01, env-overridable); older timeframes fall back to the v1 MV so we never read a partial/absent v2 range. Pure table-name swap — identicalcountMerge(event_count)/event_dateaggregates. All projects, no allowlist.events(GROUP BY profile_id HAVING count() <op>/SELECT DISTINCT). Exact for any frequency, anon-inclusive, and faster than v2 (whoseARRAY JOINexplodes name-only reads → massive over-count on≥N). Removes the only cohort read ofcohort_events_mv.filter-operators.ts(operatorClause) used in the property branch. FixesdoesNotContain/startsWith/gt/regex/… silently falling through toIN.profile_event_property_summary_v2toTABLE_NAMES.filterClausechain..env.example: addsCOHORTS_V2_START_DATE="2026-07-01".Scope / notes
main— 5 files. (The profiles PR feat(profiles): Mixpanel-style profiles page — perf, filters, search #371 also adds theprofile_event_property_summary_v2key; when both merge there's a trivial one-line overlap onclient.ts.)cohort_events_mvtable is kept — retention (trpc/chart.ts) +delete.service.tsstill use it. This PR only stops cohorts depending on it; physically dropping it is a follow-up.Behaviour to expect
computeOnDemandcohorts pick it up live and their count is not subject to the 10K membership cap.COHORTS_V2_START_DATEto2026-07-12to exclude it until the 07-11 re-backfill lands.Tests / known debt
cohort.service.test.tswas non-compiling onmain(stalefrequency/timeframe shapes, invalid operators, wrongstoreCohortMembershiparity, a vitest mock-hoisting bug). Fixed those so it compiles + runs; the 8 routing tests now pass (computeEventBasedCohort/computePropertyBasedCohort/computeCohort).storeCohortMembership/getCohortMembers/getCohortCount/updateCohortMembership) — pre-existing drift in functions this PR does not touch: they mockchQuerywhile the real functions usech.insert()/ Postgres. Follow-up.@openpanel/db typecheckis red onmainfor unrelated reasons (import.service,notification.service,reports.service) — not introduced here.🤖 Generated with Claude Code
Summary by CodeRabbit