fix(insights): handle non-boolean filter values on boolean properties

[sampennington]

Problem

Filtering a Boolean-typed property with a value that isn't a valid boolean crashed the whole insight query with ClickHouse CANNOT_PARSE_BOOL. Surfaced from system.query_log (exception_code = 467) as part of the effort to reduce deterministic query-builder bugs (dashboard).

The property-type swapper coerces a Boolean property to toBool(...). _handle_bool_values only resolved the filter value when it was literally "true"/"false" — any other value (e.g. a stray non-boolean string from a bad saved filter) was passed through as a string, producing equals(toBool(...), '<non-boolean string>'), which ClickHouse can't parse.

Changes

• New _resolve_boolean_value helper: maps recognized boolean representations (true/false/yes/no/1/0/…) to a bool, and anything else to None.
• _handle_bool_values now resolves the value against the boolean property for every value, so a non-boolean value yields None — the comparison matches no rows instead of crashing. (This also means "1"/"yes"-style values now correctly resolve to a bool.)

How did you test this code?

I'm an agent. Automated tests run locally:

• New end-to-end class TestBooleanPropertyComparisonWithData executing the generated SQL — true/false match, a non-boolean value returns 0 rows.
• Full posthog/hogql/test/test_property.py suite — 129 passed.

Publish to changelog?

no

🤖 Agent context

Authored by Claude Code (Opus 4.7). Found via system.query_log analysis (exception_code = 467, CANNOT_PARSE_BOOL).

Trade-off: the early-return fast path in _handle_bool_values (which skipped the PropertyDefinition lookup for non-true/false values) is removed, since detecting a Boolean property requires the lookup regardless of the value. This adds one indexed lookup per exact/is_not comparison at query-build time. Batching this with the property-type swapper's existing bulk lookup is a possible follow-up.

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 the diff and surrounding _handle_bool_values callers. The core fix is sound: resolving every value against the Boolean property (instead of only literal "true"/"false") and returning None for non-boolean input is a reasonable way to dodge CANNOT_PARSE_BOOL. The new _resolve_boolean_value helper is clean and well-placed. One real correctness concern, plus a couple of smaller notes.

Needs attention — IS_NOT with a None right-hand side (property.py:518-523)

_handle_bool_values is called for both EXACT and IS_NOT. For IS_NOT, a non-boolean value now resolves to None, producing NotEq(toBool(expr), Constant(None)) → SQL toBool(...) != NULL.

In ClickHouse three-valued logic, x != NULL evaluates to NULL, which a WHERE clause treats as false. So is_not "garbage" matches zero rows. That is arguably wrong: every event "is not" the garbage value, so the user almost certainly expects all rows back (the pre-PR behavior was a crash, so any non-crash is an improvement — but matching nothing is a surprising, silent semantic). Compare with exact "garbage" → 0 rows, which is correct.

The new test class only covers EXACT. Please add an is_not parameterized case and decide intentionally what it should return. If "match everything" is desired, IS_NOT likely needs to special-case a None-resolved value (e.g. emit a constant-true expr, or wrap in ifNull(..., 1) the way NOT_REGEX above already does). At minimum, document the chosen behavior.

Notes

• Expanded boolean vocabulary changes behavior beyond the bug fix (property.py:229-230). The PR body frames this as a fix, but accepting 1/0/yes/y/on/enabled/etc. is a genuine behavior change for exact filters on boolean properties — previously "1" fell through as a string. For a property stored as the string "1" (not a real bool), toBool coercion makes this fine, but it is worth a one-line test asserting "1"/"yes" resolve to True so the intent is locked in and a future reader doesn't think it is accidental. Also consider whether y/n/t/f single-letter forms are wide enough to cause false positives on real saved filters — they are unlikely but cheap to drop if not needed.

• Removed fast path (property.py, old value != "true" and value != "false" early return). The added per-comparison PropertyDefinition lookup is on name + effective_project_id + type, which is indexed, so the cost is bounded. Acceptable. The PR body's suggested follow-up (batching with the property-type swapper's existing bulk lookup) is the right call — worth a tracked issue rather than leaving it only in the PR description.

• None value short-circuit (_resolve_boolean_value, property.py:239-240). value is None returns None before normalization — fine. But note an explicit None filter value reaching EXACT already had ambiguous meaning; no regression, just flagging that None in / None out is intentional.

• Test class docstring is helpful here (explains the runtime-vs-build-time failure), but per repo convention Python tests generally avoid doc comments — consider trimming to a short comment or dropping it. Minor.

Positives

• Helper is pure, isolated, and easy to unit-test directly — consider adding a couple of direct _resolve_boolean_value unit assertions in addition to the e2e test.
• End-to-end test that actually executes the generated SQL is exactly the right level for this class of bug, since the failure was a ClickHouse runtime parse error invisible to AST-only tests.
• Return type correctly widened to ... | None.

Verdict: Needs changes — resolve the IS_NOT / NULL semantics (and cover it with a test) before shipping.