fix(insights): coerce funnel aggregation target before empty check

[sampennington]

Problem

Funnel queries that aggregate by a HogQL expression resolving to a Numeric-typed property fail deterministically with Cannot read floating point value: while converting '' to Float64 (code 72).

FunnelEventQuery._aggregation_target_filter filters out rows whose aggregation key is empty using aggregation_target != ''. When the aggregation key is a property with a Numeric PropertyDefinition, the property-types transform coerces it to Float64, so ClickHouse tries to parse the empty-string literal as a number.

Part of an effort to reduce deterministic product-analytics query-builder failures — tracking dashboard: https://metabase.prod-us.posthog.dev/dashboard/207-product-analytics-insight-query-failures

Changes

Wrap the aggregation target in toString() for the empty-string comparison so it works regardless of the resolved column type. The isNotNull check is unchanged.

Updated funnel snapshots — the only diff is the mechanical notEquals(aggregation_target, '') → notEquals(toString(aggregation_target), '') substitution (72 occurrences).

How did you test this code?

I'm an agent. Added an executing funnel test (test_hogql_aggregation_by_numeric_property) that aggregates a funnel by a Numeric property and runs against ClickHouse. Verified the test fails on origin/master with the exact code 72 error and passes with the fix. Ran the funnel, strict, unordered, and correlation test suites — all pass. Snapshot diff is limited to the intended substitution.

Publish to changelog?

no

🤖 Agent context

Agent-authored (Claude Code, Opus 4.7). Requires human review.

Root cause traced from a production failing query where the funnel aggregated by properties.bidId (Numeric property). The Float64 coercion is applied by the property-types transform; the empty-string comparison is the only place that assumes a string-typed aggregation target. Snapshots were regenerated, then narrowed back to only the intended substitution (a full --snapshot-update also deleted unrelated stale snapshots, which were reverted to keep the PR scoped).

[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.

[tests-posthog]

Query snapshots: Backend query snapshots updated

Changes: 1 snapshots (1 modified, 0 added, 0 deleted)

What this means:

• Query snapshots have been automatically updated to match current output
• These changes reflect modifications to database queries or schema

Next steps:

• Review the query changes to ensure they're intentional
• If unexpected, investigate what caused the query to change

Review snapshot changes →

[sampennington]

Automated code review

Reviewed the diff end-to-end (core change, new test, all 5 snapshot files).

Critical issues

None.

Functional gaps

None blocking. test_hogql_aggregation_by_numeric_property is an executing test that reproduces the production failure path (Numeric PropertyDefinition → Float64 coercion → CANNOT_PARSE_NUMBER).

• Optional: the test only exercises a single matching person. A row with an empty/missing aggregation property would more directly assert the empty-string branch — not blocking, since the repro depends only on column type.

Snapshot verification

Confirmed the .ambr diff is exactly the intended substitution: every changed line is notEquals(aggregation_target, '') → notEquals(toString(aggregation_target), '') and nothing else — no unrelated churn. The isNotNull(aggregation_target) half is correctly untouched.

Positive

• Minimal, targeted one-line fix at the root cause; reproducing test verified to fail on master and pass with the fix.

Verdict: Ship it

---

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