Skip to content

fix(db): qualify event-level properties access in funnel & conversion queries#404

Open
ayushjhanwar-png wants to merge 1 commit into
Openpanel-dev:mainfrom
Dashverse:fix/profile-properties-collision-upstream
Open

fix(db): qualify event-level properties access in funnel & conversion queries#404
ayushjhanwar-png wants to merge 1 commit into
Openpanel-dev:mainfrom
Dashverse:fix/profile-properties-collision-upstream

Conversation

@ayushjhanwar-png

@ayushjhanwar-png ayushjhanwar-png commented Jun 26, 2026

Copy link
Copy Markdown

Summary

Fixes a 500 error when a funnel mixes profile.* filters with event-level properties.* filters on different steps (and the equivalent for conversion charts).

Repro (funnel)

A funnel with 4 steps:

  1. $identify filtered by profile.properties.quotaPlan = 'FRAMEO_BASIC'
  2. homePageViewed (no filter)
  3. pricingModalViewed filtered by properties.$current_url contains 'canvas'
  4. paymentSuccess

Produces:

JOIN ... ambiguous identifier 'properties'. In scope funnel AS (SELECT ... 
windowFunnel(...)(..., 
  ((profile.properties['quotaPlan']) = 'FRAMEO_BASIC') AND (name = '$identify'), 
  name = 'homePageViewed', 
  ((properties['$current_url']) LIKE '%canvas%') AND (name = 'pricingModalViewed'),  ← unqualified
  name = 'paymentSuccess') AS level 
FROM events 
LEFT JOIN (SELECT id, properties FROM profiles FINAL WHERE project_id = 'X') AS profile
  ON profile.id = events.profile_id ...)

Removing either the $current_url filter OR the profile.properties.quotaPlan filter makes the query work.

Root cause

The funnel CTE adds a profile JOIN that selects properties (the whole Map) whenever any step references profile.properties.X. ClickHouse then has TWO properties columns in scope — events.properties and profile.properties — and any unqualified properties['key'] in a step condition is ambiguous.

The conversion service has the equivalent issue when a profile.properties.X breakdown is combined with event-level properties.X filters.

Fix

getEventFiltersWhereClause already accepts an eventsAlias parameter (line 1125, documented to handle exactly this case). It's used at chart.service.ts:448 and :866 with 'e' for the regular chart paths. The funnel and conversion services just weren't passing it.

Pass eventsAlias='events' (matching the unaliased table name used in those services):

  • funnel.service.ts:getFunnelConditions → propagates to windowFunnel step conditions
  • conversion.service.ts → propagates to the two whereA / whereB step conditions

After the fix, the SQL becomes:

windowFunnel(...)(..., 
  ((profile.properties['quotaPlan']) = 'FRAMEO_BASIC') AND (events.name = '$identify'), 
  events.name = 'homePageViewed', 
  ((events.properties['$current_url']) LIKE '%canvas%') AND (events.name = 'pricingModalViewed'),
  ...
)

events.properties[...] is explicitly bound to the events table — no ambiguity with profile.properties[...].

Why this approach

The codebase already has the eventsAlias plumbing for exactly this purpose. Other chart paths (regular chart and chart breakdown) already use it. Funnel + conversion were the two paths still missing it — likely an oversight from when eventsAlias was added.

This keeps the patch minimal (15 lines, 2 files) and consistent with the existing pattern instead of introducing a different mechanism.

Test plan

  • Verified locally — the prod-failing funnel runs correctly with the fix
  • Funnel with profile.* filter only — unchanged
  • Funnel with event-level properties.* filter only — unchanged
  • Funnel mixing both (the bug case) — runs without error
  • Conversion with profile breakdown + event-level filter — works
  • Conversion / funnel with no profile filters — unchanged

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of event filter conditions in conversion and funnel queries.
    • Prevented ambiguous properties references when filters are used alongside profile-related data.
    • Made funnel and conversion results more reliable in cases with mixed event and profile attributes.

… queries

When a funnel mixes a profile.* filter with an event-level properties.*
filter on different steps, or a conversion has a profile.* breakdown
alongside an event-level properties.* filter, ClickHouse fails with:

    JOIN ... ambiguous identifier 'properties'

Root cause: the profile JOIN selects the 'properties' Map from
profiles. The windowFunnel step conditions then have two columns
named 'properties' in scope — events.properties and profile.properties
— and any unqualified properties['key'] reference becomes ambiguous.

Fix: pass eventsAlias='events' to getEventFiltersWhereClause in
funnel.service.ts:getFunnelConditions and conversion.service.ts where-
clause builders. This reuses the existing eventsAlias plumbing in
getSelectPropertyKey (lines 322-328, already used by other call sites
at chart.service.ts:448 and :866 with 'e' alias), so event-level
properties[...] becomes events.properties[...] — explicitly bound to
the events table, no ambiguity with profile.properties[...].
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Conversion and funnel query builders now pass an events table qualifier into getEventFiltersWhereClause, so event property filters are generated as events.properties[...] when profiles are joined. The funnel service also adds comments describing the disambiguation.

Changes

Event filter qualification

Layer / File(s) Summary
Qualify event filters with events
packages/db/src/services/conversion.service.ts, packages/db/src/services/funnel.service.ts
getEventFiltersWhereClause now receives events in both query builders so generated properties[...] predicates are qualified as events.properties[...]; the funnel service adds comments about the alias disambiguation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A bunny hopped through SQL lanes,
With events.properties in tidy chains.
No muddled properties left to fight,
Just funnels and conversions shining bright. 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main fix: qualifying event-level properties in funnel and conversion queries.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/conversion.service.ts`:
- Around line 121-131: The conversion query still fails for profile-filter-only
cases because profile joins are gated only by breakdowns. Update the profile
join condition in conversion.service’s conversion query builder so it also
enables the profiles join when either event’s filters reference profile fields,
matching the funnel logic. Use the existing getEventFiltersWhereClause /
profileJoin / breakdown handling in the conversion service to detect profile.*
filters and ensure the query joins profiles whenever profile-based filters or
breakdowns are present, not just when a profile breakdown is selected.
🪄 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

Run ID: 63b94a57-e5e3-4221-a632-ccb6ad206e9a

📥 Commits

Reviewing files that changed from the base of the PR and between 4ed4d39 and f5949a1.

📒 Files selected for processing (2)
  • packages/db/src/services/conversion.service.ts
  • packages/db/src/services/funnel.service.ts

Comment on lines +121 to +131
// Qualify with 'events' so event-level `properties[...]` becomes
// `events.properties[...]` — required when the conversion query also
// joins the profiles table (which exposes a `properties` column).
// Without the qualifier ClickHouse fails with "ambiguous identifier
// 'properties'" whenever a step filters on properties.X while a
// breakdown is on profile.properties.Y.
const whereA = Object.values(
getEventFiltersWhereClause(eventA.filters, projectId),
getEventFiltersWhereClause(eventA.filters, projectId, 'events'),
).join(' AND ');
const whereB = Object.values(
getEventFiltersWhereClause(eventB.filters, projectId),
getEventFiltersWhereClause(eventB.filters, projectId, 'events'),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

This still misses profile-filter-only conversion queries.

Line 127 fixes ambiguous events.properties[...], but profileJoin is still only enabled for profile breakdowns at Lines 61-98. In the reported case where an event filter includes profile.* and another includes properties.*, this path now emits profile.properties[...] plus events.properties[...], and the query still 500s unless a profile breakdown also happens to be present. Funnel already handles this by joining profiles when either filters or breakdowns reference profile.; conversion needs the same gating here.

🤖 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/conversion.service.ts` around lines 121 - 131, The
conversion query still fails for profile-filter-only cases because profile joins
are gated only by breakdowns. Update the profile join condition in
conversion.service’s conversion query builder so it also enables the profiles
join when either event’s filters reference profile fields, matching the funnel
logic. Use the existing getEventFiltersWhereClause / profileJoin / breakdown
handling in the conversion service to detect profile.* filters and ensure the
query joins profiles whenever profile-based filters or breakdowns are present,
not just when a profile breakdown is selected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant