fix(charts): route property-MV charts to anon-inclusive v2 (Jul-1 cutoff), else events - #412
Open
ayushjhanwar-png wants to merge 1 commit into
Open
fix(charts): route property-MV charts to anon-inclusive v2 (Jul-1 cutoff), else events#412ayushjhanwar-png wants to merge 1 commit into
ayushjhanwar-png wants to merge 1 commit into
Conversation
…lse events The property-MV chart path read v1 profile_event_property_summary_mv, which has a profile_id != device_id filter -> counts identified users ONLY, silently dropping anonymous events. For web appOpen by $referring_domain this undercounts ~5-7x (validated: Aug 7 appOpen+google v1=131 vs raw=821). v2 profile_event_property_summary_v2 has NO such filter (anon-inclusive), holds ALL properties (not just materialized ones), is pre-aggregated (appOpen breakdown over 5 weeks = 0.21s / 758K rows), and has data from 2026-07-01. - getPropertyMvTableForRange(startDate): v2 when startDate >= PROPERTY_MV_V2_MIN_DATE (mirrors getEventsTableForRange). Both FROMs in getChartSqlFromPropertyMV use it. - canUsePropertyMV gate: when the env is set, use the v2 cutoff -> ranges before it return false and fall THROUGH to the events_v2/events path (correct) instead of the undercounting v1 MV. Env unset = legacy (v1) unchanged. Logs [property-mv-routing] -> <table>. Extend correctness before Jul 1 by backfilling the v2 property MV further and lowering the cutoff.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe ClickHouse client now routes property-summary materialized-view queries between v1 and v2 tables by date. Chart SQL uses this routing for the top-values CTE and main query while preserving legacy cutoff behavior when no v2 cutoff is configured. ChangesProperty MV routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ChartService
participant PropertyMVRouter
participant ClickHouse
ChartService->>PropertyMVRouter: Select table for query start date
PropertyMVRouter-->>ChartService: Return v1 or v2 table
ChartService->>ClickHouse: Run top-values and main queries
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Property-filter/breakdown charts (the
canUsePropertyMV→getChartSqlFromPropertyMVpath) read v1profile_event_property_summary_mv, which carries aprofile_id != device_idfilter — so it counts identified users only and silently drops anonymous events.Validated on prod (appOpen +
$referring_domain=www.google.com, Aug 7):→ v1 undercounts ~5–7× for anonymous-heavy web traffic.
Why v2 property MV (not events_v2)
profile_event_property_summary_v2:property_key/property_valuefor all of them) — no materialized column needed, unlike events_v2.$referring_domainover the full 5-week window (Jul 1→Aug 8, top-50/day) = 0.21s / 758K rows.Change
getPropertyMvTableForRange(startDate)(mirrorsgetEventsTableForRange): returns v2 whenstartDate >= PROPERTY_MV_V2_MIN_DATE, else v1. Used for bothFROMs ingetChartSqlFromPropertyMV.canUsePropertyMVgate: whenPROPERTY_MV_V2_MIN_DATEis set, the cutoff becomes the v2 window — ranges before it returnfalseand fall through to the events_v2/events path (correct) instead of the undercounting v1 MV. Env unset = legacy v1 behavior, byte-identical.[property-mv-routing] -> <table>.Routing result (with
PROPERTY_MV_V2_MIN_DATE=2026-07-01)Follow-up (not here)
To make pre-Jul-1 ranges correct+fast too, backfill the v2 property MV (migration 22) toward June/May and lower
PROPERTY_MV_V2_MIN_DATE.Enable on prod via
openpanel-configPROPERTY_MV_V2_MIN_DATE="2026-07-01 00:00:00"after this deploys.Summary by CodeRabbit