perf(ai): resolve the profile_list page in two stages - #761
Conversation
* fix(insights): verify unfiltered native analytics reads * test(insights): match source fixture to supported read windows
profile_list read 3,324,273 rows and 361 MiB to return 50. visitor_profiles is only all_visitor_profiles ordered by last_visit with a limit, but it was referenced four times as visitor_id IN (SELECT visitor_id FROM visitor_profiles), inside visitor_custom_events, profile_payment_intents, profile_payment_context and visitor_revenue. ClickHouse inlines a CTE at every reference, so each of those re-ran the whole pipeline to recover 50 ids, driving visitor_identity_rows to 26 executions and 75 base table scans. prepareSql is a new optional builder hook. It resolves that key set in its own round trip and customSql binds it as preparedKeys, so the semi-joins become a parameter instead of a repeated subquery. The subquery form is kept as the fallback whenever no keys are supplied, which is why compile() alone still produces the original query. Builders declaring prepareSql are given a unique batch group so they always run through execute() rather than being folded into a UNION, since a staged builder needs its own round trip. Measured on the busiest website over a closed date range, three runs each: 1072ms to 554ms, a 1.94x improvement. Output is byte identical, compared against the single-query form executed in the same instant to rule out drift from live data. 673 tests pass and two batched profile_list requests plus a top_pages request each resolve correctly.
…ery (#759) * feat(insights): retain independent business changes and simplify discovery * test(insights): preserve compact discovery in holdouts * fix(ai): distinguish saved definitions from measured cohorts
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Greptile SummaryThis PR introduces optional two-stage query execution for
Confidence Score: 4/5The PR is not safe to merge until the two-stage profile query prevents live updates from producing profiles with incorrectly zeroed enrichment. The preparation and full queries independently calculate the requested profile page, while only the first page’s IDs control enrichment; a page change between requests therefore returns materially incorrect custom-event and revenue fields. Files Needing Attention: packages/ai/src/query/simple-builder.ts, packages/ai/src/query/builders/profiles.ts Important Files Changed
Sequence DiagramsequenceDiagram
participant B as SimpleQueryBuilder
participant C as ClickHouse
B->>C: Prepare query: resolve visitor IDs
C-->>B: preparedKeys
Note over C: Live events may change profile ordering
B->>C: Full query: recompute visitor_profiles
Note over B,C: Enrichment CTEs still filter by earlier preparedKeys
C-->>B: Current profile page with potentially zeroed enrichment
Reviews (1): Last reviewed commit: "feat(insights): retain business changes ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const preparedKeys = await this.resolvePreparedKeys(abortSignal); | ||
| const { sql, params } = this.compile(preparedKeys); |
There was a problem hiding this comment.
If live activity changes the requested profile page between the prepare query and the full query, the full query recomputes visitor_profiles but restricts its enrichment CTEs to IDs from the earlier read. A profile that enters the page during that interval is therefore returned with its custom-event counts and LTV incorrectly set to zero. The two stages need to use a consistent key set or equivalent snapshot semantics.
1 participant
1072ms → 554ms (1.94x), byte-identical output.
The problem
profile_listread 3,324,273 rows and 361 MiB to return 50 rows.visitor_profilesis onlyall_visitor_profiles ORDER BY last_visit LIMIT 50, but it was referenced four times asvisitor_id IN (SELECT visitor_id FROM visitor_profiles)— insidevisitor_custom_events,profile_payment_intents,profile_payment_contextandvisitor_revenue. ClickHouse inlines a CTE at every reference, so each of those re-ran the whole pipeline just to recover 50 ids. That drovevisitor_identity_rowsto 26 executions and the query to 75 base-table scans.The change
prepareSqlis a new optional builder hook. It resolves the key set in its own round trip, andcustomSqlbinds it aspreparedKeys, turning the four semi-joins into a parameter.The subquery form is kept as the fallback whenever no keys are supplied, so
compile()on its own still emits the original single query. That is deliberate: it is what makes the two forms directly comparable.Builders declaring
prepareSqlget a unique batch group so they always run throughexecute()rather than being folded into a UNION, since a staged builder needs its own round trip.Verification
Output compared against the single-query form executed in the same instant on a closed date range. All 50 rows and 17 columns identical.
That detail matters: my first comparison reported a difference, but the baseline was 30 minutes stale and the query orders by
last_visit DESC, so live events had reshuffled the top rows. Comparing stale output against a live query is not a valid check.Batching verified too — two
profile_listrequests plus atop_pagesin one batch resolved withbatch_union_groups: 0. 673 tests pass, typecheck and lint clean.Also carries #759 and #760, already reviewed.
Summary by cubic
Resolves the
profile_listquery in two stages, cutting average runtime from 1072ms to 554ms with byte-identical output. The new optionalprepareSqlbuilder hook fetches the visitor key set in its own round trip, andcustomSqlbinds it aspreparedKeysso ClickHouse stops re-running the CTE at every reference.Query builder
prepareSqlget a unique batch group so they always run throughexecute()instead of being folded into a UNION.compile()still emits the original subquery form, keeping the two forms directly comparable.Behavior changes
categoryandsearchto search across all categories or list a compact catalog without output fields.savedDefinitionfrom read-time cohort filters.Written for commit 4e5de43. Summary will update on new commits.