perf(db): extend property-MV router to breakdown-by-property charts - #357
Merged
ayushjhanwar-png merged 4 commits intoJul 7, 2026
Merged
Conversation
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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe ChangesProperty MV Routing and SQL Generation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller as getChartSql
participant Gate as canUsePropertyMV
participant Builder as getChartSqlFromPropertyMV
Caller->>Gate: evaluate filters and breakdowns
Gate-->>Caller: XOR eligibility (filter-mode or breakdown-mode)
Caller->>Builder: pass filters, breakdowns
Builder->>Builder: derive propKey and valueList by mode
alt breakdown-mode
Builder->>Builder: select label_1, group by property_value
else filter-mode
Builder->>Builder: apply property_value IN (...) predicate
end
Builder-->>Caller: return generated SQL
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
… 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.
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/chart.service.ts (1)
972-991: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBreakdown-mode MV route ignores
limit/sortOrder(packages/db/src/services/chart.service.ts:972-991)Charts that use a capped breakdown will return the full series here, ordered by
label_1, instead of the top-N values the main builder applies. Either skip this MV path whenlimitis set or carry the same top-N logic intogetChartSqlFromPropertyMV.🤖 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 972 - 991, The property MV fast path in chart.service.ts bypasses the main breakdown top-N behavior, so charts with a capped breakdown ignore limit and sortOrder and return all labels in label_1 order. Update the canUsePropertyMV/getChartSqlFromPropertyMV route to either opt out whenever limit is present or apply the same top-N/sort logic used by the main chart SQL builder so the MV path matches the standard breakdown results.
🤖 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 `@packages/db/src/services/chart.service.ts`:
- Around line 615-649: The breakdown query in chart.service’s SQL builder uses
the wrong ORDER BY shape for WITH FILL, causing fill to run as one global
timeline instead of per breakdown series. Update the order construction in the
query-building logic around the breakdown path so the grouping prefix column
`label_1` is ordered before the date column and `WITH FILL` follows the date
field, keeping the per-series fill behavior intact in the SQL assembled by the
chart query method.
---
Outside diff comments:
In `@packages/db/src/services/chart.service.ts`:
- Around line 972-991: The property MV fast path in chart.service.ts bypasses
the main breakdown top-N behavior, so charts with a capped breakdown ignore
limit and sortOrder and return all labels in label_1 order. Update the
canUsePropertyMV/getChartSqlFromPropertyMV route to either opt out whenever
limit is present or apply the same top-N/sort logic used by the main chart SQL
builder so the MV path matches the standard breakdown results.
🪄 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: 197fcacc-c7c0-41e9-ab92-2a363742273e
📒 Files selected for processing (1)
packages/db/src/services/chart.service.ts
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.
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.
ayushjhanwar-png
merged commit Jul 7, 2026
f897278
into
perf/chart-property-mv-routing
5 checks passed
This was referenced Jul 7, 2026
3 tasks
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.
Stacked on #356 (property MV routing for single filter). Same MV, same sort-key trick — this time the 3-column prefix `(project_id, name, property_key)` returns `property_value` as the breakdown `b_0` dimension.
Measured (2026-07-07, dashreels, 30-day `logIn` breakdown by `properties.type`)
Real signal returned across 7 login types:
Gate change
`canUsePropertyMV` now accepts XOR(filter, breakdown) — at most one property is bound to this MV row shape. Deferred to the multi-property PR:
Cohort breakdowns are still rejected (need cohort membership joins, not this MV).
SQL builder change
Semantic caveat
MV filters `property_value != ''` on write. The breakdown result therefore omits events with empty values under the breakdown key. Events-table path would show those as a `''` bucket. Documented inline in the SQL builder.
Test plan
Base branch
Based on `perf/chart-property-mv-routing` (#356) so the diff shows ONLY the breakdown additions. When #356 merges, this PR's base auto-updates to `main`.
Summary by CodeRabbit
New Features
Bug Fixes