perf(db): extend property-MV router to breakdown-by-property charts (redo) - #359
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR modifies chart service logic to allow the property materialized-view routing path to support either a single property filter or a single breakdown (previously only filters were allowed), adjusting SQL generation and ordering for both modes. Documentation is updated to describe a related anonymous-event safety boundary. ChangesProperty MV Filter/Breakdown Support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant getChartSql
participant canUsePropertyMV
participant getChartSqlFromPropertyMV
Client->>getChartSql: request chart with filters/breakdowns
getChartSql->>canUsePropertyMV: check eligibility (filter XOR breakdown)
canUsePropertyMV-->>getChartSql: eligible mode (filter or breakdown)
getChartSql->>getChartSqlFromPropertyMV: build SQL with breakdowns passed through
getChartSqlFromPropertyMV-->>getChartSql: SQL with label_1/property_value clause per mode
getChartSql-->>Client: return chart data
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
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.
… 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.
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
force-pushed
the
perf/chart-property-mv-breakdown
branch
from
July 7, 2026 12:59
fae9f9a to
69bf3ab
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/conversion-chart-perf.md (1)
61-66: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "doesn't cover yet" bullet contradicts the shipped breakdown support.
Line 64 still lists property breakdown as unsupported ("needs a JOIN with sibling MV rows, adds complexity"), but this same PR ships single-property breakdown routing in
canUsePropertyMV/getChartSqlFromPropertyMV. Worth updating this bullet (or moving it to the "shipped" section) to avoid confusing future readers about current MV capabilities.🤖 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` around lines 61 - 66, The “What this doesn’t cover (yet)” section still claims property breakdown is unsupported, but this PR already adds single-property breakdown support through canUsePropertyMV and getChartSqlFromPropertyMV. Update the docs entry to reflect the shipped behavior by removing that bullet from the unsupported list or moving it into the shipped section, and make sure the wording matches the actual MV capability.
🤖 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 518-538: The property-MV routing in chart.service.ts is
incorrectly accepting capped breakdown charts and bypassing the events-table
top-N trimming. Update the route check near the event filter/breakdown logic so
the property MV path only handles uncapped breakdowns, and return false when a
breakdown has limit or sortOrder set, keeping those charts on the events path
until MV pruning is supported.
---
Outside diff comments:
In `@docs/conversion-chart-perf.md`:
- Around line 61-66: The “What this doesn’t cover (yet)” section still claims
property breakdown is unsupported, but this PR already adds single-property
breakdown support through canUsePropertyMV and getChartSqlFromPropertyMV. Update
the docs entry to reflect the shipped behavior by removing that bullet from the
unsupported list or moving it into the shipped section, and make sure the
wording matches the actual MV capability.
🪄 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: af438585-a24d-4748-bd3f-0c56cb777970
📒 Files selected for processing (2)
docs/conversion-chart-perf.mdpackages/db/src/services/chart.service.ts
Comment on lines
+518
to
+538
| const filters = event.filters ?? []; | ||
| const numFilters = filters.length; | ||
| const numBreakdowns = breakdowns.length; | ||
|
|
||
| // Filter XOR breakdown, each at most one property. Combined | ||
| // (filter AND breakdown) needs an intersection JOIN of two MV rows for | ||
| // the same event — deferred to the multi-property PR. Zero of both | ||
| // means the events_daily_stats route handles it (no reason to be here). | ||
| if (numFilters + numBreakdowns !== 1) return false; | ||
|
|
||
| if (numFilters === 1) { | ||
| const f = filters[0]!; | ||
| if (f.operator !== 'is') return false; | ||
| if (!f.value || f.value.length === 0) return false; | ||
| if (!extractEventPropertyKey(f.name)) return false; | ||
| } else { | ||
| const b = breakdowns[0]!; | ||
| // Cohort breakdowns need cohort membership joins, not this MV | ||
| if (b.cohortId || b.name.startsWith('cohort:')) return false; | ||
| if (!extractEventPropertyKey(b.name)) return false; | ||
| } |
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 structure first.
ast-grep outline packages/db/src/services/chart.service.ts --view expanded > /tmp/chart_outline.txt || true
# Show the relevant ranges around the cited lines and the related call sites.
sed -n '500,580p' packages/db/src/services/chart.service.ts
printf '\n---\n'
sed -n '960,1010p' packages/db/src/services/chart.service.ts
printf '\n---\n'
sed -n '1180,1295p' packages/db/src/services/chart.service.ts
printf '\n---\n'
sed -n '635,665p' packages/db/src/services/chart.service.ts
printf '\n---\n'
sed -n '1,120p' /tmp/chart_outline.txtRepository: Dashverse/openpanel-forked
Length of output: 12208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate all references to canUsePropertyMV and getChartSqlFromPropertyMV to understand the data flow.
rg -n "canUsePropertyMV|getChartSqlFromPropertyMV|top_breakdowns|sortOrder|limit" packages/db/src/services/chart.service.tsRepository: Dashverse/openpanel-forked
Length of output: 764
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact implementations around the cited helpers and the fallback top-N logic.
python3 - <<'PY'
from pathlib import Path
p = Path('packages/db/src/services/chart.service.ts')
lines = p.read_text().splitlines()
for start,end in [(520,570),(970,1005),(1215,1285),(635,660)]:
print(f"\n--- {start}-{end} ---")
for i in range(start, min(end, len(lines))+1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: Dashverse/openpanel-forked
Length of output: 8019
Exclude capped breakdowns from the property MV route The property-MV path still returns every distinct property_value for breakdown charts and ignores limit/sortOrder, while the events-table path trims to top-N via top_breakdowns. That changes chart semantics and can return unbounded series for high-cardinality properties; route capped breakdowns back to the events path until the MV SQL supports the same pruning.
🤖 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 518 - 538, The
property-MV routing in chart.service.ts is incorrectly accepting capped
breakdown charts and bypassing the events-table top-N trimming. Update the route
check near the event filter/breakdown logic so the property MV path only handles
uncapped breakdowns, and return false when a breakdown has limit or sortOrder
set, keeping those charts on the events path until MV pruning is supported.
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.
Re-opening #357. The original was merged into `perf/chart-property-mv-routing` (its stacked base), but that intermediate branch never made it to main. Main only has #356 landed. This PR retargets to main with the same content, minus the allowlist commit (which is now its own PR #358) and minus the console.log tracing.
Summary
Extends `buildArrayPatternSql`'s sibling `getChartSqlFromPropertyMV` and the `canUsePropertyMV` gate to accept one event-level property breakdown, in addition to the single-filter case already shipped in #356.
XOR gate: filter or breakdown, at most one property. Combined filter+breakdown (intersection JOIN of two MV rows) and multi-property AND stay deferred.
Measured (2026-07-07, dashreels, 30-day `logIn` breakdown by `properties.type`)
Real signal returned across 7 login types on Jun 7:
The truecaller number matches the single-filter test at exactly 32,242.
Bugs fixed via local smoke test
Both caught before push. Second one is the CodeRabbit finding from the original #357.
Also removed
Three `console.log` traces from the property-MV builder that matched the existing `getChartSqlFromMaterializedView` pattern but bloat production logs and print filter/breakdown values verbatim. CodeRabbit flagged as a potential PII leak on the sibling PR. Removing here rather than leaving inconsistent — the other builders should follow up separately.
Dependency on #358 (allowlist)
Must merge #358 first. Without the project allowlist guard, this PR would silently under-count by ~6× on shortreels for filtered/breakdown charts on anonymous-heavy events (`showOpen`: 15.7% identified). Doc reflects this — the allowlist section is duplicated in both PRs' docs; conflict will resolve trivially on the second merge.
Test plan
Summary by CodeRabbit
Bug Fixes
Documentation