perf(db): route event-property-filter charts to profile_event_property_summary_mv - #356
Merged
Merged
Conversation
…y_summary_mv Linear-chart queries with a single event-level property filter (e.g. type='truecaller') fall to the events-table path today, which scans the full name-pruned slice when the filter column has no skip index. Measured on dashreels 30-day logIn+truecaller: 52.4s / 6.68 B rows / 152 GiB — over the 40s prod timeout. profile_event_property_summary_mv is already sorted by (project_id, name, property_key, property_value, profile_id, event_date) — the 4-column prefix matches the filter shape exactly. Prefix-scan answers the same query in 0.38s / 1.4 M rows / 128 MiB (~140x faster, same numbers as events-table path). Added canUsePropertyMV gate + getChartSqlFromPropertyMV builder to chart.service.ts, sitting between the existing events_daily_stats route and the events-table fallback. Gate is intentionally conservative: one 'is' filter, no breakdowns, no cohort filters, user/event segment, day/week/month interval, explicit event name, and startDate within the MV's ~3-month retention window. Anything outside falls through unchanged. Retention cutoff is a rolling 92 days computed at query time (giving ~3 months + safety margin for the "3m" dashboard preset). Compares date portion only (`startDate.slice(0, 10)`) — full DateTime comparison rejected a same-day startDate against a later time-of-day cutoff, caught during prod smoke test. Multi-property AND (2+ filters) via intersection JOIN measured at 0.39s in the same session; deferred to a follow-up PR. Conversion/funnel services are separate paths — also follow-up.
|
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)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a new ClickHouse routing path for single event-property chart filters and expands the conversion chart performance document with routing details, shipped query changes, investigation findings, tests, and diagnostic SQL. ChangesConversion chart MV routing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant getChartSql
participant canUsePropertyMV
participant getChartSqlFromPropertyMV
participant QueryBuilder
Client->>getChartSql: request chart SQL
getChartSql->>canUsePropertyMV: check single property filter eligibility
alt eligible and not customEvent
canUsePropertyMV-->>getChartSql: allowed
getChartSql->>getChartSqlFromPropertyMV: build profile_event_property_summary_mv SQL
getChartSqlFromPropertyMV-->>getChartSql: property MV SQL
else not eligible
getChartSql->>QueryBuilder: build standard chart SQL
QueryBuilder-->>getChartSql: standard SQL
end
getChartSql-->>Client: SQL result
Related issues: None provided. Related PRs: None provided. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/db/src/services/chart.service.ts (3)
557-568: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTriplicated day/week/month branching logic.
The interval-based ternary for date bucketing is repeated three times (
dateSelect,dateGroupBy,fillClause). Consolidating into a small per-interval lookup/helper would reduce the chance of exactly the kind of inconsistency flagged above (e.g., one branch missing a parameter that the others have).Also applies to: 580-585
🤖 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/chart.service.ts` around lines 557 - 568, The interval-based date bucketing logic in chart.service.ts is duplicated across dateSelect, dateGroupBy, and fillClause, which risks inconsistencies between branches. Refactor this into a single helper or per-interval lookup keyed by the interval value (day/week/month) and reuse it for all three expressions in the chart aggregation flow so the date formatting, grouping, and fill behavior always stay aligned.
602-604: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueFull generated SQL (including escaped filter values) is logged to console on every property-MV query.
If a filter value carries PII (e.g., an email or identifier used as an event property), it will land in application logs verbatim. This mirrors the pre-existing
console.logpattern elsewhere in this file, so it's consistent with current conventions, but worth reconsidering (e.g., gate behind a debug flag) since this is a new call site.🤖 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/chart.service.ts` around lines 602 - 604, The new console logging in chart.service.ts around the profile_event_property_summary_mv SQL generation can leak PII because it prints the fully rendered query with filter values. Update the logging at the sql.replaceAll call site to be guarded by an explicit debug/trace flag (or remove it entirely) so only safe diagnostics are emitted in normal execution, and keep the surrounding property-MV query flow unchanged.
496-533: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLine 507 is unreachable dead code.
validSegments(line 505) only contains'user'and'event', soevent.segment ?? 'event'failing that check (line 506) already rejects'one_event_per_user'before line 507 is ever reached.🧹 Proposed cleanup
const validSegments = ['user', 'event']; if (!validSegments.includes(event.segment ?? 'event')) return false; - if (event.segment === 'one_event_per_user') return false;🤖 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/chart.service.ts` around lines 496 - 533, The extra `event.segment === 'one_event_per_user'` guard in `canUsePropertyMV` is unreachable because the earlier `validSegments` check already excludes it; remove the redundant branch and keep the segment validation centralized in `canUsePropertyMV` alongside the existing `validSegments` logic.docs/conversion-chart-perf.md (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFenced code blocks missing a language identifier.
Flagged by markdownlint (MD040) at these four fenced blocks. Adding a language (e.g.
sql,text) fixes lint and improves rendering.Also applies to: 180-180, 188-188, 421-421
🤖 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 `@docs/conversion-chart-perf.md` at line 18, The markdown file has fenced code blocks without a language identifier, triggering MD040. Update each affected fenced block in the conversion chart document to include an appropriate language tag such as sql or text so the linter passes and rendering improves; make the same change for all four flagged fences, including the ones around the chart examples and summary snippets.Source: Linters/SAST tools
🤖 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 `@docs/conversion-chart-perf.md`:
- Around line 45-58: Update the gate description to match the dynamic rolling
cutoff used by getPropertyMVCutoffDate in chart.service.ts: replace the fixed
date language in the Router shipped section with a rolling 3-month cutoff
computed at query time. Keep the rest of the gate bullets unchanged, but
describe the date constraint in terms of now()-based coverage so the docs stay
accurate over time.
In `@packages/db/src/services/chart.service.ts`:
- Around line 551-555: The filter value handling in the chart service’s
property-filter builder is missing the same trimming used by the sibling MV
query path. Update the `filter.value` mapping in `chart.service.ts` to trim each
value before `sqlstring.escape`, matching `cohort.service.ts`’s
`profile_event_property_summary_mv` logic so both builders compare against the
same normalized values. Use the existing `filter.value!` mapping block as the
place to apply the trim consistently.
- Around line 535-568: Thread the timezone argument through both MV bucket
builders so they match the non-MV chart SQL. Update
getChartSqlFromMaterializedView and getChartSqlFromPropertyMV to use the passed
timezone in their week/month bucket expressions and fill-range logic instead of
hardcoding toStartOfWeek(..., 1) and the current month boundaries. Make sure the
relevant date grouping and bucket generation inside these helpers consistently
use timezone-aware conversions for non-UTC dashboards.
---
Nitpick comments:
In `@docs/conversion-chart-perf.md`:
- Line 18: The markdown file has fenced code blocks without a language
identifier, triggering MD040. Update each affected fenced block in the
conversion chart document to include an appropriate language tag such as sql or
text so the linter passes and rendering improves; make the same change for all
four flagged fences, including the ones around the chart examples and summary
snippets.
In `@packages/db/src/services/chart.service.ts`:
- Around line 557-568: The interval-based date bucketing logic in
chart.service.ts is duplicated across dateSelect, dateGroupBy, and fillClause,
which risks inconsistencies between branches. Refactor this into a single helper
or per-interval lookup keyed by the interval value (day/week/month) and reuse it
for all three expressions in the chart aggregation flow so the date formatting,
grouping, and fill behavior always stay aligned.
- Around line 602-604: The new console logging in chart.service.ts around the
profile_event_property_summary_mv SQL generation can leak PII because it prints
the fully rendered query with filter values. Update the logging at the
sql.replaceAll call site to be guarded by an explicit debug/trace flag (or
remove it entirely) so only safe diagnostics are emitted in normal execution,
and keep the surrounding property-MV query flow unchanged.
- Around line 496-533: The extra `event.segment === 'one_event_per_user'` guard
in `canUsePropertyMV` is unreachable because the earlier `validSegments` check
already excludes it; remove the redundant branch and keep the segment validation
centralized in `canUsePropertyMV` alongside the existing `validSegments` logic.
🪄 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: 99844ef4-ebd1-472b-ad50-eaede6cad4a7
📒 Files selected for processing (2)
docs/conversion-chart-perf.mdpackages/db/src/services/chart.service.ts
Comment on lines
+535
to
+568
| function getChartSqlFromPropertyMV({ | ||
| event, | ||
| interval, | ||
| startDate, | ||
| endDate, | ||
| projectId, | ||
| }: { | ||
| event: IGetChartDataInput['event']; | ||
| interval: IGetChartDataInput['interval']; | ||
| startDate: string; | ||
| endDate: string; | ||
| projectId: string; | ||
| timezone: string; | ||
| }): string { | ||
| const filter = event.filters![0]!; | ||
| const propKey = extractEventPropertyKey(filter.name)!; | ||
| // filter.value is already validated non-empty in the gate. Escape each | ||
| // value; sqlstring.escape handles ' and other SQL-special chars. | ||
| const valueList = filter.value! | ||
| .map((v) => sqlstring.escape(String(v))) | ||
| .join(', '); | ||
|
|
||
| const dateSelect = | ||
| interval === 'day' | ||
| ? 'toStartOfDay(toDateTime(t.event_date)) as date' | ||
| : interval === 'week' | ||
| ? 'toDateTime(toStartOfWeek(t.event_date, 1)) as date' | ||
| : 'toDateTime(toStartOfMonth(t.event_date)) as date'; | ||
| const dateGroupBy = | ||
| interval === 'day' | ||
| ? 'toStartOfDay(toDateTime(t.event_date))' | ||
| : interval === 'week' | ||
| ? 'toDateTime(toStartOfWeek(t.event_date, 1))' | ||
| : 'toDateTime(toStartOfMonth(t.event_date))'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the surrounding implementation.
wc -l packages/db/src/services/chart.service.ts
ast-grep outline packages/db/src/services/chart.service.ts --view expanded || true
# Read the relevant region around the reported lines.
sed -n '500,650p' packages/db/src/services/chart.service.ts
# Find other timezone-aware bucketing calls in this file.
rg -n "toStartOf(Week|Month|Day)|timezone" packages/db/src/services/chart.service.tsRepository: Dashverse/openpanel-forked
Length of output: 11928
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sibling materialized-view builder and the dispatcher that chooses between paths.
sed -n '323,430p' packages/db/src/services/chart.service.ts
printf '\n---\n'
sed -n '851,980p' packages/db/src/services/chart.service.ts
printf '\n---\n'
sed -n '1080,1175p' packages/db/src/services/chart.service.ts
# Show where getChartSqlFromPropertyMV is called.
rg -n "getChartSqlFromPropertyMV|getChartSqlFromMaterializedView|getPerUserChartSql" packages/db/src/services/chart.service.tsRepository: Dashverse/openpanel-forked
Length of output: 12262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '323,430p' packages/db/src/services/chart.service.ts
printf '\n---\n'
sed -n '851,980p' packages/db/src/services/chart.service.ts
printf '\n---\n'
sed -n '1080,1175p' packages/db/src/services/chart.service.ts
rg -n "getChartSqlFromPropertyMV|getChartSqlFromMaterializedView|getPerUserChartSql" packages/db/src/services/chart.service.tsRepository: Dashverse/openpanel-forked
Length of output: 12262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the MV schemas to see whether the date column is Date or DateTime.
rg -n "events_daily_stats|profile_event_property_summary_mv|MaterializedView|event_date|date\s+Date|date\s+DateTime" packages/db src . -g '!**/node_modules/**'Repository: Dashverse/openpanel-forked
Length of output: 19219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "events_daily_stats|profile_event_property_summary_mv|event_date|date\s+Date|date\s+DateTime" packages/db src . -g '!**/node_modules/**'Repository: Dashverse/openpanel-forked
Length of output: 16251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the MV schemas to see whether the date column is Date or DateTime.
rg -n "events_daily_stats|profile_event_property_summary_mv|event_date|date\s+Date|date\s+DateTime" packages/db src . -g '!**/node_modules/**'Repository: Dashverse/openpanel-forked
Length of output: 16251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the MV definitions and table schemas around the date columns.
sed -n '1,220p' packages/db/code-migrations/9-events-daily-stats.ts
printf '\n---\n'
sed -n '1,260p' packages/db/code-migrations/11-backfill-profile-event-property-v2.ts
printf '\n---\n'
sed -n '1,200p' packages/db/code-migrations/12-backfill-profile-event-summary-v2.ts
printf '\n---\n'
sed -n '364,470p' packages/db/src/clickhouse/migration.tsRepository: Dashverse/openpanel-forked
Length of output: 25670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/db/code-migrations/9-events-daily-stats.ts
printf '\n---\n'
sed -n '1,260p' packages/db/code-migrations/11-backfill-profile-event-property-v2.ts
printf '\n---\n'
sed -n '1,200p' packages/db/code-migrations/12-backfill-profile-event-summary-v2.ts
printf '\n---\n'
sed -n '364,470p' packages/db/src/clickhouse/migration.tsRepository: Dashverse/openpanel-forked
Length of output: 25670
🌐 Web query:
ClickHouse toStartOfWeek toStartOfMonth timezone Date DateTime semantics explicit timezone parameter
💡 Result:
In ClickHouse, date and time functions such as toStartOfWeek and toStartOfMonth generally support an optional timezone parameter to control the temporal context of the calculation [1][2]. Semantics and Usage: 1. Default Behavior: If the optional timezone parameter is omitted, ClickHouse uses the server's default timezone [1][2]. 2. Explicit Timezone: When provided, the timezone parameter (a string, e.g., 'UTC' or 'Europe/Amsterdam') ensures the rounding operation (to the start of the week or month) is performed relative to that specific timezone rather than the server's local time [3][1][2]. 3. Data Independence: ClickHouse stores DateTime values as unix timestamps, which are inherently UTC [4][5]. The timezone parameter in these functions is used to interpret the components (year, month, day, etc.) of that timestamp during the transformation [4]. Function Signatures: - toStartOfWeek(datetime, [mode], [timezone]): Rounds a date/datetime to the start of the week. The mode (optional) determines the first day of the week (default is 0), and the timezone (optional) specifies the temporal alignment [1][2]. - toStartOfMonth(datetime, [timezone]): Rounds a date/datetime to the beginning of the month [2]. When working with these functions, providing an explicit timezone is essential if your data represents events spanning multiple timezones and you need the results to align with local calendar definitions [4]. If the timezone is not specified, consistency with the server's local setting is assumed [1][2].
Citations:
- 1: ClickHouse/ClickHouse@f8ee4c7
- 2: ClickHouse/ClickHouse@61df8aa
- 3: https://github.com/ClickHouse/ClickHouse/blob/master/docs/en/sql-reference/functions/date-time-functions.md
- 4: Support for non-constant timezone argument for DateTime functions ClickHouse/ClickHouse#6948
- 5: https://clickhouse.com/docs/sql-reference/data-types/datetime
Thread timezone through both MV bucket builders.
getChartSqlFromMaterializedView and getChartSqlFromPropertyMV accept timezone, but their week/month buckets and fill ranges still call toStartOfWeek(..., 1) / toStartOfMonth(...) without it. The non-MV path uses explicit timezone here, so non-UTC dashboards can land on different week/month boundaries.
🤖 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/chart.service.ts` around lines 535 - 568, Thread the
timezone argument through both MV bucket builders so they match the non-MV chart
SQL. Update getChartSqlFromMaterializedView and getChartSqlFromPropertyMV to use
the passed timezone in their week/month bucket expressions and fill-range logic
instead of hardcoding toStartOfWeek(..., 1) and the current month boundaries.
Make sure the relevant date grouping and bucket generation inside these helpers
consistently use timezone-aware conversions for non-UTC dashboards.
Author
There was a problem hiding this comment.
Caller sets session_timezone on the CH session; toStartOfWeek() runs in that TZ. Matches existing getChartSqlFromMaterializedView pattern — not a real bug
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "session_timezone|getChartSql\(" packages/db/src -g '!**/node_modules/**' -C3Length of output: 8160
@ayushjhanwar-png Confirmed — packages/db/src/engine/fetch.ts passes session_timezone: plan.timezone in clickhouse_settings when calling getChartSql, and packages/db/src/clickhouse/query-builder.ts does the same for its own callers. Since toStartOfWeek/toStartOfMonth without an explicit timezone argument fall back to the session timezone, and this MV route follows the same established pattern as getChartSqlFromMaterializedView, there's no discrepancy here. Thanks for pointing that out — withdrawing this comment.
🧬 Code Graph Analysis Results
packages/db/src/services/chart.service.ts
Snippet (MV route week bucketing is toStartOfWeek(...) without explicit TZ argument)
Lines ~260-296
} else if (interval === 'week') {
sb.select.date = 'toDateTime(toStartOfWeek(t.date, 1)) as date';
sb.groupBy.date = 'toDateTime(toStartOfWeek(t.date, 1))';
sb.orderBy.date = 'toDateTime(toStartOfWeek(t.date, 1)) ASC';
} else if (interval === 'month') {
sb.select.date = 'toDateTime(toStartOfMonth(t.date)) as date';
sb.groupBy.date = 'toDateTime(toStartOfMonth(t.date))';
sb.orderBy.date = 'toDateTime(toStartOfMonth(t.date)) ASC';
}
// Build WITH FILL for date gaps — use DateTime to match the regular query fill format
let fillClause = '';
if (interval === 'day') {
fillClause = `WITH FILL FROM toStartOfDay(toDateTime(${sqlstring.escape(startDate)})) TO toStartOfDay(toDateTime(${sqlstring.escape(endDate)})) STEP toIntervalDay(1)`;
} else if (interval === 'week') {
fillClause = `WITH FILL FROM toDateTime(toStartOfWeek(toDate(${sqlstring.escape(startDate)}), 1)) TO toDateTime(toStartOfWeek(toDate(${sqlstring.escape(endDate)}), 1)) STEP toIntervalWeek(1)`;
} else if (interval === 'month') {
fillClause = `WITH FILL FROM toDateTime(toStartOfMonth(toDate(${sqlstring.escape(startDate)}))) TO toDateTime(toStartOfMonth(toDate(${sqlstring.escape(endDate)}))) STEP toIntervalMonth(1)`;
}Snippet (non-MV per-user route explicitly passes TZ to toStartOfWeek(..., ..., '${timezone}'))
Lines ~520-575
case 'week':
dateExpr = `toStartOfWeek(created_at, 1, '${timezone}')`;
fill = `WITH FILL FROM toStartOfWeek(toDateTime('${startDate}'), 1, '${timezone}') TO toStartOfWeek(toDateTime('${endDate}'), 1, '${timezone}') STEP toIntervalWeek(1)`;
break;Snippet (property MV route week bucketing is also toStartOfWeek(...) without explicit TZ argument)
Lines ~180-230 (inside getChartSqlFromPropertyMV)
const dateSelect =
interval === 'day'
? 'toStartOfDay(toDateTime(t.event_date)) as date'
: interval === 'week'
? 'toDateTime(toStartOfWeek(t.event_date, 1)) as date'
: 'toDateTime(toStartOfMonth(t.event_date)) as date';
const dateGroupBy =
interval === 'day'
? 'toStartOfDay(toDateTime(t.event_date))'
: interval === 'week'
? 'toDateTime(toStartOfWeek(t.event_date, 1))'
: 'toDateTime(toStartOfMonth(t.event_date))';
const fillClause =
interval === 'day'
? `WITH FILL FROM toStartOfDay(toDateTime(${sqlstring.escape(startDate)})) TO toStartOfDay(toDateTime(${sqlstring.escape(endDate)})) STEP toIntervalDay(1)`
: interval === 'week'
? `WITH FILL FROM toDateTime(toStartOfWeek(toDate(${sqlstring.escape(startDate)}), 1)) TO toDateTime(toStartOfWeek(toDate(${sqlstring.escape(endDate)}), 1)) STEP toIntervalWeek(1)`
: `WITH FILL FROM toStartOfMonth(toDate(${sqlstring.escape(startDate)}))) TO toStartOfMonth(toDate(${sqlstring.escape(endDate)}))) STEP toIntervalMonth(1)`;
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- trim filter values before sqlstring.escape to match the sibling MV
builder in cohort.service.ts. Prevents silent under-matching when a
dashboard value carries incidental whitespace.
- remove unreachable one_event_per_user guard in canUsePropertyMV — the
validSegments check ('user' | 'event') already rejects it.
- update doc gate description to describe the rolling window instead of
the compile-time constant that was renamed away from earlier in the
same doc.
- add language tags (sql / text) to fenced code blocks flagged by
markdownlint MD040.
Skipped:
- Timezone threading in week/month builders. Caller sets
session_timezone via clickhouse_settings; toStartOfWeek etc. run in
that session TZ. Matches existing getChartSqlFromMaterializedView
pattern.
- Guarding console.log behind a debug flag. Existing
getChartSqlFromMaterializedView uses the same unguarded log; changing
one and not the other creates inconsistency. Separate cleanup.
- Extracting per-interval helper for date bucketing. Existing MV
builder in this same file uses the same triplicated shape; refactor
is out of scope for this PR.
5 tasks
ayushjhanwar-png
added a commit
that referenced
this pull request
Jul 7, 2026
…357) * perf(db): extend property-MV router to breakdown-by-property charts Follow-up to PR #356. Same MV, same sort-key prefix trick — but this time the query hits (project_id, name, property_key) as the 3-column prefix and returns property_value as the breakdown b_0 dimension. Prod measurement (dashreels, 30-day logIn breakdown by properties.type): events-table path: not tested (would be ~50s+ range based on filter case) this route: 0.10s / 4.16 M rows / 271 MiB Real signal returned across 7 login types (otpless dominant at 67K/day Jun 7, deviceId 35K, truecaller 32K, then anonymous/google/sms/apple trailing). Matches numbers from the single-filter test path exactly. Gate change: canUsePropertyMV now accepts XOR(filter, breakdown) — at most one property is bound. Both filter+breakdown (needs intersection JOIN) and multi-filter/multi-breakdown stay deferred to the future multi-property PR. Cohort breakdowns rejected — they need cohort membership joins, not this MV. SQL builder change: emits `property_value AS b_0` in SELECT, adds b_0 to GROUP BY, and interleaves WITH FILL inside the ORDER BY (between the date and b_0 columns) — trailing WITH FILL on a String column errors out with INVALID_WITH_FILL_EXPRESSION on CH 25.3+ because it tries to fill b_0 with a day step. Caught by smoke test before push. MV filters `property_value != ''` on write, so the breakdown result omits events with empty values under the breakdown key. Events-table path would show them as a '' bucket. Documented as a semantic caveat in the SQL builder comment. * fix(chart): emit label_1 (not b_0) for breakdown value in property-MV route Prod smoke test caught the frontend collapsing all breakdown values into a single "(Not set)" bucket. Root cause: I used b_0 (conversion service convention) for the breakdown column alias. The linear/bar chart frontend calls groupByLabels which expects the standard {label_0, [label_1], date, count} shape — label_0 for the event name and label_1..N for breakdown values. b_0 is what conversion.service.ts uses because its dense_rank top-N wrap needs a distinct namespace. Rename all b_0 references in the property-MV builder to label_1. Also updated the WITH FILL interleave to match. Conversion service still uses b_0 (correctly, since it's a different result shape); nothing else was touched by the replace. * docs: document anonymous-heavy event safety boundary Both MVs the router uses filter `profile_id != device_id` on write, so anonymous events (installReferrer, first_open, deepLinkCaptured, etc.) are excluded. Filtered/breakdown charts on those events would under-count by ~100x. Not a bug in practice — the dashboard usage pattern doesn't filter or break down anonymous-heavy events (they don't carry `type`, `plan`, etc. that filters target). Empirically verified on dashreels 2026-07-07: installReferrer had 122K events in raw, only 405 in the MV (99.4% anonymous, all excluded). Documented with the empirical numbers + escape hatch (add to a blacklist in canUsePropertyMV) so future maintainers hitting this shape have the context. * fix(chart): per-series WITH FILL layout for breakdown property-MV route Original ORDER BY put the date column first with WITH FILL and label_1 as a secondary sort. WITH FILL applies to the column it directly precedes, so this filled the date sequence globally — gap-day rows generated by the fill couldn't be attributed to a specific label_1 series, breaking per-series timeline continuity in the frontend. Fix: `ORDER BY label_1 ASC, date ASC WITH FILL FROM ... STEP ...`. CH sorts within each label_1 group by date, then fills date gaps per-group. Each breakdown series gets its own contiguous filled timeline. Verified via A/B smoke test — same result count when no gaps present, correct per-series output when gaps exist. Caught by CodeRabbit review.
4 tasks
ayushjhanwar-png
added a commit
that referenced
this pull request
Jul 7, 2026
Follow-up to PR #356. Same MV, same sort-key prefix trick — but this time the query hits (project_id, name, property_key) as the 3-column prefix and returns property_value as the breakdown b_0 dimension. Prod measurement (dashreels, 30-day logIn breakdown by properties.type): events-table path: not tested (would be ~50s+ range based on filter case) this route: 0.10s / 4.16 M rows / 271 MiB Real signal returned across 7 login types (otpless dominant at 67K/day Jun 7, deviceId 35K, truecaller 32K, then anonymous/google/sms/apple trailing). Matches numbers from the single-filter test path exactly. Gate change: canUsePropertyMV now accepts XOR(filter, breakdown) — at most one property is bound. Both filter+breakdown (needs intersection JOIN) and multi-filter/multi-breakdown stay deferred to the future multi-property PR. Cohort breakdowns rejected — they need cohort membership joins, not this MV. SQL builder change: emits `property_value AS b_0` in SELECT, adds b_0 to GROUP BY, and interleaves WITH FILL inside the ORDER BY (between the date and b_0 columns) — trailing WITH FILL on a String column errors out with INVALID_WITH_FILL_EXPRESSION on CH 25.3+ because it tries to fill b_0 with a day step. Caught by smoke test before push. MV filters `property_value != ''` on write, so the breakdown result omits events with empty values under the breakdown key. Events-table path would show them as a '' bucket. Documented as a semantic caveat in the SQL builder comment.
4 tasks
ayushjhanwar-png
added a commit
that referenced
this pull request
Jul 7, 2026
Follow-up to PR #356. Same MV, same sort-key prefix trick — but this time the query hits (project_id, name, property_key) as the 3-column prefix and returns property_value as the breakdown b_0 dimension. Prod measurement (dashreels, 30-day logIn breakdown by properties.type): events-table path: not tested (would be ~50s+ range based on filter case) this route: 0.10s / 4.16 M rows / 271 MiB Real signal returned across 7 login types (otpless dominant at 67K/day Jun 7, deviceId 35K, truecaller 32K, then anonymous/google/sms/apple trailing). Matches numbers from the single-filter test path exactly. Gate change: canUsePropertyMV now accepts XOR(filter, breakdown) — at most one property is bound. Both filter+breakdown (needs intersection JOIN) and multi-filter/multi-breakdown stay deferred to the future multi-property PR. Cohort breakdowns rejected — they need cohort membership joins, not this MV. SQL builder change: emits `property_value AS b_0` in SELECT, adds b_0 to GROUP BY, and interleaves WITH FILL inside the ORDER BY (between the date and b_0 columns) — trailing WITH FILL on a String column errors out with INVALID_WITH_FILL_EXPRESSION on CH 25.3+ because it tries to fill b_0 with a day step. Caught by smoke test before push. MV filters `property_value != ''` on write, so the breakdown result omits events with empty values under the breakdown key. Events-table path would show them as a '' bucket. Documented as a semantic caveat in the SQL builder comment.
ayushjhanwar-png
added a commit
that referenced
this pull request
Jul 7, 2026
…redo) (#359) * perf(db): extend property-MV router to breakdown-by-property charts Follow-up to PR #356. Same MV, same sort-key prefix trick — but this time the query hits (project_id, name, property_key) as the 3-column prefix and returns property_value as the breakdown b_0 dimension. Prod measurement (dashreels, 30-day logIn breakdown by properties.type): events-table path: not tested (would be ~50s+ range based on filter case) this route: 0.10s / 4.16 M rows / 271 MiB Real signal returned across 7 login types (otpless dominant at 67K/day Jun 7, deviceId 35K, truecaller 32K, then anonymous/google/sms/apple trailing). Matches numbers from the single-filter test path exactly. Gate change: canUsePropertyMV now accepts XOR(filter, breakdown) — at most one property is bound. Both filter+breakdown (needs intersection JOIN) and multi-filter/multi-breakdown stay deferred to the future multi-property PR. Cohort breakdowns rejected — they need cohort membership joins, not this MV. SQL builder change: emits `property_value AS b_0` in SELECT, adds b_0 to GROUP BY, and interleaves WITH FILL inside the ORDER BY (between the date and b_0 columns) — trailing WITH FILL on a String column errors out with INVALID_WITH_FILL_EXPRESSION on CH 25.3+ because it tries to fill b_0 with a day step. Caught by smoke test before push. MV filters `property_value != ''` on write, so the breakdown result omits events with empty values under the breakdown key. Events-table path would show them as a '' bucket. Documented as a semantic caveat in the SQL builder comment. * fix(chart): emit label_1 (not b_0) for breakdown value in property-MV route Prod smoke test caught the frontend collapsing all breakdown values into a single "(Not set)" bucket. Root cause: I used b_0 (conversion service convention) for the breakdown column alias. The linear/bar chart frontend calls groupByLabels which expects the standard {label_0, [label_1], date, count} shape — label_0 for the event name and label_1..N for breakdown values. b_0 is what conversion.service.ts uses because its dense_rank top-N wrap needs a distinct namespace. Rename all b_0 references in the property-MV builder to label_1. Also updated the WITH FILL interleave to match. Conversion service still uses b_0 (correctly, since it's a different result shape); nothing else was touched by the replace. * fix(chart): per-series WITH FILL layout for breakdown property-MV route Original ORDER BY put the date column first with WITH FILL and label_1 as a secondary sort. WITH FILL applies to the column it directly precedes, so this filled the date sequence globally — gap-day rows generated by the fill couldn't be attributed to a specific label_1 series, breaking per-series timeline continuity in the frontend. Fix: `ORDER BY label_1 ASC, date ASC WITH FILL FROM ... STEP ...`. CH sorts within each label_1 group by date, then fills date gaps per-group. Each breakdown series gets its own contiguous filled timeline. Verified via A/B smoke test — same result count when no gaps present, correct per-series output when gaps exist. Caught by CodeRabbit review.
3 tasks
ayushjhanwar-png
added a commit
that referenced
this pull request
Jul 16, 2026
) * Revert "perf(db): extend property-MV router to breakdown-by-property charts (redo) (#359)" This reverts commit 8213214. * Revert "safety: gate property-MV route on explicit project allowlist (#358)" This reverts commit 5f9a286. * Revert "perf(db): route event-property-filter charts to profile_event_property_summary_mv (#356)" This reverts commit a1d884f.
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
1 participant
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.
Summary
Why not the existing paths
Current linear-chart flow in `chart.service.ts`:
Concrete: `logIn + type='truecaller'` 30-day query took 52.4s direct (6.68 B rows, 152 GiB). `type` had no skip index and isn't in `proj_funnel`, so full-scan territory.
`profile_event_property_summary_mv` already exists and has EXACTLY the right sort key:
```
ORDER BY (project_id, name, property_key, property_value, profile_id, event_date)
```
Query hits the 4-column sort-key prefix → CH jumps straight to the tiny matching slice.
Measured (2026-07-07, dashreels, prod CH)
Multi-property AND (`type='truecaller' AND $os='Android'`) via intersection JOIN: 0.39 s / 5.6 M rows / 457 MiB — basically same speed as single-property. Not shipped in this PR (deferred to follow-up); router only handles ONE filter for now.
Local prod smoke test
Ran API + dashboard locally against prod CH/DB. Loaded a linear chart on dashreels `logIn` with `properties.type = 'truecaller'` filter:
Bug fixed during smoke test
First cut used `formatClickhouseDate()` for the retention cutoff, returning full `YYYY-MM-DD HH:MM:SS`. Same-day startDate at midnight (`2026-04-07 00:00:00`) compared as less than a same-day cutoff at query time (`2026-04-07 22:XX:XX`) → gate rejected valid 3-month queries.
Fix: date-only comparison via `startDate.slice(0, 10)` + widened window to 92 days (safety margin over the 90-day "3m" preset).
Router gate (intentionally conservative)
Anything outside falls through to the existing paths — zero behavior change for those queries.
What this doesn't cover (yet)
Doc dependency
`docs/conversion-chart-perf.md` was created in an earlier open PR (`#352`, `perf/conversion-fast-path-breakdown`). This PR includes it too because we're new-file-creating on both branches. When one merges, the other will conflict on file creation — trivial resolve, keep both sections.
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
ref_utm_sourceas a materialized field on events to speed up conversion chart reads.Documentation