fix(insights): fix funnel correlation join for HogQL aggregation

[sampennington]

Problem

A funnel correlation query crashed with ClickHouse TYPE_MISMATCH (no supertype for types UUID, String) when the funnel aggregated by a custom HogQL expression. Surfaced from system.query_log (exception_code = 53) as part of the effort to reduce deterministic query-builder bugs (dashboard).

_get_aggregation_target_join_query only handled two cases — person aggregation (event.person_id) and group aggregation (event.$group_N). With a custom HogQL aggregation (funnelAggregateByHogQL), it fell back to the person join, comparing event.person_id (UUID) against funnel_actors.actor_id (the HogQL expression's value, e.g. a string).

Changes

• _get_aggregation_target_join_query now joins on the funnel's funnelAggregateByHogQL expression when one is set, matching how the funnel actors CTE computes actor_id.

How did you test this code?

I'm an agent. Automated tests run locally:

• New test test_funnel_correlation_with_events_and_hogql_aggregation — a correlation query over a funnel aggregating by properties.session_id. Verified it fails (TYPE_MISMATCH) without the fix and passes with it.
• Full test_funnel_correlation.py suite — 19 passed.

Publish to changelog?

no

🤖 Agent context

Authored by Claude Code (Opus 4.7). Found via system.query_log analysis (exception_code = 53, Can't infer common type for joined columns).

Scope note: the sibling _get_aggregation_join_query (person/group properties correlation) has the same person/group-only assumption, but property correlation against a non-person aggregation target is a different code path and wasn't in the observed failures — left for a separate change if it surfaces.

Agent-authored; requires human review.

[github-actions]

🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down.

Add the run-playwright label if you want an E2E sweep before merging — CI will pick it up automatically.

Most PRs don't need this. Real regressions still get caught on master and fix-forward.

[sampennington]

Automated code review

Reviewed funnel_correlation_query_runner.py and the new test.

Correctness

The fix correctly mirrors the upstream actor expression — _aggregation_target_expr in funnel_event_query.py uses the identical guard (funnelAggregateByHogQL and != "person_id"), so the new join condition matches how funnel_actors.actor_id is actually computed, resolving the actor_id (HogQL value) vs event.person_id (UUID) type mismatch. funnelsFilter None-handling is fine.

SQL safety

No new injection surface — the interpolated expression lands inside the parse_select(...) HogQL body and goes through the same parser/printer as the upstream parse_expr(funnelAggregateByHogQL). Consistent with the existing trust model in this file.

Possible concern — column resolution scope

The join evaluates the expression against the correlation event alias, whereas the funnel actors CTE evaluates it against the funnel's own event query. Worth confirming a person.properties.* expression (a valid funnelAggregateByHogQL) resolves identically in both scopes — otherwise the join could silently mismatch rather than error. The test only covers properties.session_id (event-level).

Tests

test_funnel_correlation_with_events_and_hogql_aggregation reproduces the failure and is a good regression guard. The assertion result is not None only proves it executes — consider asserting an expected correlated event appears, so a future regression that makes the join match zero rows is caught.

Verdict: Ship it — strengthen the test assertion (and ideally add a person.properties.* case) before merge or as a quick follow-up.

---

Automated review — posted on behalf of a code-reviewer agent.