Skip to content

fix(cohorts): Treat null properties as unset - #59962

Closed
an8kk wants to merge 1 commit into
PostHog:masterfrom
an8kk:fix/cohort-set-excludes-null
Closed

an8kk wants to merge 1 commit into
PostHog:masterfrom
an8kk:fix/cohort-set-excludes-null

Conversation

@an8kk

@an8kk an8kk commented May 25, 2026

Copy link
Copy Markdown

Treat explicit JSON/null property values as unset for cohort and person-property matching.

Previously, set filters could match properties whose key existed but whose value was JSON null. This makes set require a non-null value and makes not set include missing or null values across ClickHouse SQL, Django query construction, override matching, and the Rust cohort/feature-flag matcher.

ClickHouse materialized-property filters now check the raw JSON value for null semantics, keeping the literal string "null" distinct from JSON null.

Verified with focused Python property-filter tests and targeted Rust matcher tests.

Fixes #29916

@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team May 25, 2026 20:52
@greptile-apps

greptile-apps Bot commented May 25, 2026 •

Copy link
Copy Markdown
Contributor

Comments Outside Diff (2)

  1. posthog/queries/base.py, line 83-84 (link)

    P2 The docstring comment on line 84 is now inaccurate. is_not_set is supported by this PR when the key exists in override_property_values with a None value.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: posthog/queries/base.py
    Line: 83-84
    
    Comment:
    The docstring comment on line 84 is now inaccurate. `is_not_set` is supported by this PR when the key exists in `override_property_values` with a `None` value.
    
    
    
    How can I resolve this? If you propose a fix, please make it concise.
  2. ee/clickhouse/models/test/test_property.py, line 1856-1893 (link)

    P2 Non-parameterised test for the materialized case

    The codebase convention is to prefer parameterised tests. test_prop_filter_json_extract_set_operators_use_raw_json_when_materialized tests is_set and is_not_set as separate assertions inside one test body. These two operator cases could be expressed as a @pytest.mark.parametrize table (similar to the adjacent test_prop_filter_json_extract_set_operators_handle_json_null_values test), making each case independently named and easier to debug in CI.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: ee/clickhouse/models/test/test_property.py
    Line: 1856-1893
    
    Comment:
    **Non-parameterised test for the materialized case**
    
    The codebase convention is to prefer parameterised tests. `test_prop_filter_json_extract_set_operators_use_raw_json_when_materialized` tests `is_set` and `is_not_set` as separate assertions inside one test body. These two operator cases could be expressed as a `@pytest.mark.parametrize` table (similar to the adjacent `test_prop_filter_json_extract_set_operators_handle_json_null_values` test), making each case independently named and easier to debug in CI.
    
    How can I resolve this? If you propose a fix, please make it concise.

    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!

Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
posthog/queries/base.py:115-120
The `is_not_set` block contains dead code. The guard at lines 86–87 (`if property.key not in override_property_values: raise ValidationError(...)`) guarantees that `key` is always in `override_property_values` by the time we reach this branch — so `if key in override_property_values:` is always `True` and the `raise` on line 120 can never be reached. The whole block reduces to a single `return`.

```suggestion
    if operator == "is_not_set":
        return override_value is None
```

### Issue 2 of 3
posthog/queries/base.py:83-84
The docstring comment on line 84 is now inaccurate. `is_not_set` is supported by this PR when the key exists in `override_property_values` with a `None` value.

```suggestion
    # only looks for matches where key exists in override_property_values
    # supports is_not_set when the key is present with a None value
```

### Issue 3 of 3
ee/clickhouse/models/test/test_property.py:1856-1893
**Non-parameterised test for the materialized case**

The codebase convention is to prefer parameterised tests. `test_prop_filter_json_extract_set_operators_use_raw_json_when_materialized` tests `is_set` and `is_not_set` as separate assertions inside one test body. These two operator cases could be expressed as a `@pytest.mark.parametrize` table (similar to the adjacent `test_prop_filter_json_extract_set_operators_handle_json_null_values` test), making each case independently named and easier to debug in CI.

Reviews (1): Last reviewed commit: "fix(cohorts): Treat null properties as u..." | Re-trigger Greptile

Comment thread posthog/queries/base.py Resolved
Exclude JSON null values from set filters and include them in not set filters across ClickHouse, Django, override matching, and Rust cohort evaluation.

Fixes #29916
@an8kk

an8kk commented May 25, 2026

Copy link
Copy Markdown
Author

Fixed in 266afc6: simplified the is_not_set branch, updated the matcher comment, and parameterized the materialized-property test cases.

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.

Bug: In Cohorts, the 'set' operator filters in null values for person properties

1 participant