[Charan-place]

Problem

prop_filter_json_extract() only checked whether a JSON property key existed, not whether its value was an explicit JSON null. So a person/event property explicitly set to null showed up as "is set" — and is_not_set missed those same rows.

Closes #29916

Changes

• Updated both the denormalized and JSON-extract branches of prop_filter_json_extract() so is_set excludes an explicit JSON null, and is_not_set includes it — consistently on both sides.
• Added a regression test for the change.

How did you test this code?

Built this with Claude (Cowork). I haven't run the Postgres/ClickHouse suite in this sandbox, so I'm not pasting fabricated pytest output — the new test exists but I haven't executed it myself.

One known gap: the new test only covers the allow_denormalized_props=False path. A reviewer on the earlier combined PR (#65069) flagged that the denormalized branch needs its own test too, and I haven't added that yet — calling it out rather than burying it.

Automatic notifications

• Publish to changelog?
• Alert Sales and Marketing teams?

Docs update

Not needed.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Used Claude (Cowork) to track down the bug, write the fix, and add the test above. I reviewed the diff and I'm the one submitting and owning this PR.

[greptile-apps]

Comments Outside Diff (1)

1. posthog/models/property/util.py, line 523-531 (link)

   Same string-"null" confusion on the is_not_set branch

   The OR {left} = 'null' clause has the same problem as the is_set branch: a property whose value is the JSON string "null" produces {left} = 'null' after quote-trimming, causing is_not_set to match it even though the key is genuinely set. Replacing {left} = 'null' with JSONExtractRaw({prop_var}, %(k{prepend}_{idx})s) = 'null' (no trimming) would correctly fire only for the JSON null literal.

_{Reviews (1): Last reviewed commit: "fix: treat explicit JSON null as not-set..." | Re-trigger Greptile}

[greptile-apps]

Prefer @pytest.mark.parametrize over a for loop

is_set and is_not_set run through different branches in prop_filter_json_extract, so they are genuinely distinct behaviours worth parameterising. The loop also hides the first case's failure if it crashes, and the assertion message (operator=…) only partially compensates for that. Converting to @pytest.mark.parametrize("property,expected_event_indexes", [...]) matches both the project convention and the existing test_prop_filter_json_extract_person_on_events_materialized pattern directly above.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[thmsobrmlr]

Thanks for digging into this one — the intent is right (an explicit JSON null should count as "not set", and is_not_set should include it). But I think this is likely the wrong layer to fix it in, and there are a couple of correctness issues in the implementation. Details below.

1. Cohorts don't go through prop_filter_json_extract

The bug is about cohorts, but cohort calculation no longer runs through this function. The only calculation path is HogQL:

• recalculate_cohortpeople → _recalculate_cohortpeople_for_team_hogql (products/cohorts/backend/models/util.py) → HogQLCohortQuery, with no legacy fallback.
• HogQLCohortQuery.get_person_condition builds an ActorsQuery + PersonPropertyFilter and runs it through ActorsQueryRunner — the HogQL engine — where is_set becomes expr != NULL in _expr_to_compare_op (posthog/hogql/property.py).

prop_filter_json_extract lives in the legacy posthog/queries/ engine (live callers are PersonQuery, event_query.py, breakdown_props.py), which cohorts don't touch.

And the HogQL path already treats JSON null as not-set — there's an existing, passing fixture asserting exactly that for both materialized and plain columns:

# posthog/hogql/test/test_property.py
("string_null_prop", None, PropertyType.String, False),

So this change patches a path the reported bug doesn't run through, while the path it does run through already models JSON null as not-set. (This matches the "two implementations, only one patched" point in the issue thread on #29916.) Worth confirming the reproducing path on the actual repro cohort (122367) first — the fix may already be live for cohorts, or the real gap may be the materialized-column quirk / the realtime conditionHash path.

2. Correctness: JSON null vs the string "null"

The check compares the quote-trimmed value against 'null'. After trim_quotes_expr, both {"email": null} (JSON null) and {"email": "null"} (a real string value) become 'null', so a genuine "null" string gets wrongly treated as not-set. (This is the same thing Greptile flagged as P1.)

The codebase already solves this in one place — json_extract_trim_quotes in posthog/clickhouse/kafka_engine.py:

replaceRegexpAll(nullIf(nullIf(JSONExtractRaw(...), ''), 'null'), '^"|"$', '')

It applies nullIf(..., 'null') before trimming quotes, so JSON null → NULL while the string "null" survives. Its docstring calls itself the "single source of truth" for this coercion (HogQL printer, SQL backfill, plugin-server ingest). Reusing it — or comparing the untrimmed JSONExtractRaw(...) != 'null' — would be both correct and consistent.

3. This adds a new cross-engine inconsistency

On the HogQL (non-materialized) path the string "null" is treated as set (it's marked ONLY_WHEN_NOT_MATERIALIZED in test_property.py). This change makes the legacy path treat "null" as not-set, so the two engines would now disagree on that case — which is the opposite of the issue's goal of removing a set/not-set inconsistency. This is also the unresolved "what about the string null?" question from the issue thread; a fix shouldn't quietly pick an answer that diverges from the source-of-truth engine.

4. Test coverage

• The new test only exercises allow_denormalized_props=False; the denormalized branch that's also changed is untested (same gap flagged on fix(insights): group-flag, null-property, and dashboard filter fixes #65069). For materialized columns JSON null is typically stored as '', not 'null', so the denormalized != 'null' change mostly can't help the null case and mainly just mis-handles legit "null" strings.
• Per the description the tests haven't been run yet.

Suggested path forward

1. Confirm where the reported cohort actually reproduces. Given cohort calc is HogQL-only and already maps JSON null → not-set, verify the bug still reproduces and on which path.
2. If a fix is still needed, do it on the HogQL path (json_extract_trim_quotes / clickhouse_property_resolution.py) and settle all three cases — key absent, JSON null, string "null" — consistently across materialized and non-materialized reads. The existing test file even invites this ("it's ok to change these tests if you fix it!").
3. Only if a live legacy posthog/queries/ endpoint genuinely needs it, fix prop_filter_json_extract by reusing json_extract_trim_quotes (or the untrimmed JSONExtractRaw(...) != 'null') so it matches HogQL semantics, and add the denormalized-branch test.

Happy to help trace the repro path if useful.

[greptile-apps]

_{Reviews (2): Last reviewed commit: "fix: treat explicit JSON null as not-set..." | Re-trigger Greptile}

[scheduled-actions-posthog]

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the stale label – otherwise this will be closed in another week. If you want to permanently keep it open, use the waiting label.

[Charan-place]

@posthog-bot Can I please get an update on this PR. Pleae mention if I have to change anything in this PR.
Looking forward to your reply.

[Charan-place]

@posthog-bot Can I please get an update on this PR. Pleae mention if I have to change anything in this PR.
Looking forward to your reply.

[scheduled-actions-posthog]

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the stale label – otherwise this will be closed in another week. If you want to permanently keep it open, use the waiting label.

[sampennington]

Hey @Charan-place did you see the comment from Thomas? Here

The posthog bot there wont be able to tell you anything

[trunk-io]

Merging to master in this repository is managed by Trunk.

• To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

[scheduled-actions-posthog]

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the stale label – otherwise this will be closed in another week. If you want to permanently keep it open, use the waiting label.

[scheduled-actions-posthog]

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.