fix(insights): compare numeric property filter values as strings

[sampennington]

Problem

A property filter with a numeric value compared against a string column crashed the entire insight query with ClickHouse NO_COMMON_TYPE. This was the most common deterministic insight-query failure surfaced from system.query_log — it hit group keys ($group_0–$group_4), the event name, distinct_id, and JSON/materialized string properties whenever the filter value arrived as a number (e.g. a group key that looks numeric).

This is part of an effort to reduce deterministic query-builder bugs in product analytics insights, tracked on the Product analytics — insight query failures dashboard. NO_COMMON_TYPE (exception_code 386) was the largest deterministic builder failure in that triage.

Changes

• _expr_to_compare_op in posthog/hogql/property.py: exact and is_not comparisons now compare both sides as strings (toString(expr) = toString(value)) when the filter value is numeric, instead of emitting a raw String vs Float64 comparison.
• The multi-value in/not in path gets the same treatment when any tuple value is numeric.
• Added _is_numeric_scalar / _equality_compare helpers.
• Updated 5 existing property tests where numeric exact filters on virtual numeric properties / session duration now produce the toString form (equivalent for equality).

How did you test this code?

I'm an agent. Automated tests run locally:

• New unit test test_property_to_expr_numeric_value_against_string_column (AST shape, incl. mixed numeric/string tuples and single-element lists).
• New end-to-end class TestNumericPropertyComparisonWithData that executes the generated SQL against ClickHouse (exact, is_not, all-numeric in, mixed in).
• Full posthog/hogql/test/test_property.py suite — 131 passed.

Snapshot (.ambr) files in the trends/funnels/retention suites may need updating since the change touches all numeric-valued exact/is_not/in filters; CI will surface the exact set.

Publish to changelog?

no

🤖 Agent context

Authored by Claude Code (Opus 4.7). Found via a system.query_log analysis of failing product-analytics insight queries: exception_code = 386 (NO_COMMON_TYPE) was the largest deterministic query-builder failure. The investigation also produced the dashboard linked above.

Decisions:

• Considered narrowing the fix to group keys only, but the failure data showed the same crash on the event column, distinct_id, and materialized columns — any string column vs a numeric value. So the fix targets equality comparisons generally.
• Chose toString on both operands rather than stringifying the value in Python, so ClickHouse computes a consistent canonical string for both sides (str(13.0) would give "13.0", but toString(13.0) gives "13").
• Left lt/gt/lte/gte untouched — the query_log data showed only equals/in failing, and ordering comparisons have genuine numeric-vs-string semantic differences.

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 review

Critical Issues

None. The fix is sound, minimal, and well-scoped — no data-loss, security, or breaking-API concerns.

Should Fix

• test_property.py — add a mixed string/numeric tuple test. The multi-value path wraps the whole tuple when any() element is numeric, so [13, "abc"] → toString($group_0) in (toString(13), toString('abc')). That's correct, but only the all-numeric [13, 14] case is tested. Add an assertion for the mixed case.
• test_property.py — add a single-element numeric list test. {"value": [13], "operator": "exact"} collapses to the scalar path and works, but it's an implicit path — a one-line assertion guards against a future refactor silently regressing it.
• test_property.py — add an is_not + NULL-column test. toString(NULL) is NULL-propagating so behavior is preserved, but lock in that an is_not numeric filter still excludes NULL-column rows.

Performance Notes

• toString(column) = '13' on $group_0–$group_4 / event / distinct_id wraps the column in a function, which generally prevents ClickHouse from using skip indexes / PK prefix for that predicate. Non-blocking — it only triggers when the filter value is numeric (uncommon/misconfigured), and correctness outweighs it. A type-aware fix (cast only the constant) would need column type info not available at this layer.

Suggestions

• Add a one-line comment near the LT branch noting that LT/GT/LTE/GTE are deliberately left uncoerced — string-wrapping ordering comparisons would corrupt results ("100" < "9" lexically), so surfacing the error is correct. Prevents a future "fix the inconsistency" mistake.
• Float values aren't normalized: a 10.5 filter won't string-match a stored "10.50". Inherent to string-equality coercion and acceptable for the misconfigured-filter case — worth a sentence in the PR description.

Nits

None.

Positive Observations

• The not isinstance(value, bool) guard in _is_numeric_scalar is important and correct — isinstance(True, int) is True in Python, so without it boolean filters would have been wrongly coerced.
• _equality_compare's docstring explains why (the NO_COMMON_TYPE failure mode), not what — matches repo comment conventions.
• Test includes the negative case (string value keeps the plain comparison), proving the coercion is conditional.
• Reviewer confirmed no missed code paths: IS_DATE_EXACT, IS_SET/IS_NOT_SET, and the arrayExists/exception-array branches all route through the fixed compare_op.

Verdict

Ship it — after adding the three test cases above (all test-only, no production code change). Next steps:

1. Add mixed-tuple, single-element-list, and is_not+NULL tests to test_property_to_expr_numeric_value_against_string_column.
2. Run the affected trends/funnels/retention snapshot suites with --snapshot-update (CI will surface the exact .ambr set).

---

Posted by /r — reviewers: code-reviewer

[sampennington]

Review follow-up — Should Fix items addressed

All three test-only follow-ups from the automated review are done in 923f1da0399:

• ✅ Mixed string/numeric tuple — added AST assertion for [13, "abc"] → toString($group_0) in (toString(13), toString('abc')).
• ✅ Single-element numeric list — added AST assertion for [13] collapsing to the scalar toString(...) = toString(13) path.
• ✅ is_not + NULL column — covered by the new end-to-end TestNumericPropertyComparisonWithData class, which executes the generated SQL against ClickHouse. It confirmed is_not against a numeric value includes the NULL-property row (a missing property counts as "not 13") — PostHog's existing behavior, unchanged by this fix.

Also added the suggestions:

• Float values aren't normalized (10.5 won't string-match a stored "10.50") — noted as an inherent limitation of string-equality coercion for the misconfigured-filter case.

Full test_property.py suite: 131 passed.

[sampennington]

Automated code review

Reviewed the diff and surrounding context in posthog/hogql/property.py. This is a focused, well-scoped fix with strong test coverage. The end-to-end class that actually executes the generated SQL against ClickHouse is the right call here — pure AST-shape assertions would not have caught the original NO_COMMON_TYPE failure.

Strengths

• TestNumericPropertyComparisonWithData executes real SQL rather than only asserting AST shape — correctly addresses the fact that the bug is a runtime ClickHouse error, not a HogQL one.
• The is_not + NULL-property case (expected_count=2) is explicitly tested, which is the subtle one. Good.
• Mixed numeric/string tuple and single-element list collapse are both covered.
• _is_numeric_scalar correctly excludes bool (which is an int subclass) so boolean filter values still flow through _handle_bool_values unchanged.
• PR description's rationale for toString on both operands (vs str(value) in Python, avoiding 13.0 divergence) is sound and worth keeping in the commit history.

Observations / non-blocking

• Genuinely-numeric columns get toString-wrapped too. _equality_compare keys off the value type only, so an exact/is_not filter with a numeric value against an actually-numeric column (e.g. a numeric materialized property, or $session_duration — see the changed test at test_property.py:1028) now emits toString(col) = toString(13). Equality semantics are preserved, but this defeats any index/PK usage on that column and pushes a per-row toString. For the exact/is_not scalar path the blast radius is small; just flagging that the fix is broader than "string column vs numeric value" — it's "numeric value, always". Acceptable given equality-only scope, but worth a one-line note in the helper docstring that it intentionally also covers numeric columns.

• property.py:~960 tuple path — left may be v (a lambda arg over an extracted array). For is_exception_string_array_property / is_visited_page_property, left is ast.Field(chain=["v"]), an element of an Array(String) from JSONExtract. Wrapping toString(v) is harmless (already a string), and the numeric-tuple branch only triggers when a tuple value is numeric — which for exception-type filters should never happen. Fine, but the inline comment ("a numeric value in the tuple against a string column") slightly understates that left isn't always the column. Minor.

• Comment style. Per repo conventions, the multi-line docstring on _equality_compare and the inline comment at property.py:~958 are a touch verbose. The why (NO_COMMON_TYPE) is genuinely load-bearing and should stay; consider trimming each to ~1–2 lines. Not blocking.

• Snapshot files. PR description already acknowledges .ambr snapshots in trends/funnels/retention may need regenerating since every numeric-valued exact/is_not/in filter now changes shape. Make sure CI's snapshot job is green before merge — this is the most likely source of CI churn.

Tests

Coverage is good. Two optional additions worth considering:

• A not_in (multi-value is_not) end-to-end case — the tuple path handles both In and NotIn, but only exact/in are exercised end-to-end. A numeric not_in against the NULL row would confirm null handling on the negated tuple path.
• A float value (e.g. 13.0) end-to-end — the description specifically calls out toString(13.0) → "13", but no test pins that behavior; a stray str(13.0) regression would silently pass everything else.

Verdict

Ship it — solid fix with real end-to-end coverage; just confirm .ambr snapshot CI is green and consider trimming the comments / adding the not_in + float cases.