Skip to content

fix(cohorts): exclude null values from 'is set' operator on materialized properties #53250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
okxint wants to merge 1 commit into from
Closed

fix(cohorts): exclude null values from 'is set' operator on materialized properties #53250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions ee/clickhouse/queries/test/test_cohort_query.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1398,6 +1398,40 @@ def test_person(self):

assert [p1.uuid] == [r[0] for r in res]

@also_test_with_materialized_columns(person_properties=["$some_prop"])
@snapshot_clickhouse_queries
def test_is_set_operator_excludes_null_values_for_materialized_columns(self):
p1 = _create_person(
team_id=self.team.pk,
distinct_ids=["p1"],
properties={"$some_prop": "a_value"},
)
_create_person(
team_id=self.team.pk,
distinct_ids=["p2"],
properties={},
)
filter = Filter(
data={
"properties": {
"type": "AND",
"values": [
{
"key": "$some_prop",
"type": "person",
"operator": "is_set",
"value": "is_set",
},
],
}
}
)
flush_persons_and_events()

res, q, params = execute(filter, self.team)

assert [p1.uuid] == [r[0] for r in res]
Comment on lines +1401 to +1433

@greptile-apps greptile-apps Bot Apr 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Missing snapshot files will cause CI failure

This test is decorated with @snapshot_clickhouse_queries, which uses syrupy to compare queries against stored snapshots in __snapshots__/test_cohort_query.ambr. The snapshot file contains no entry for test_is_set_operator_excludes_null_values_for_materialized_columns (or the materialized-column variant generated by @also_test_with_materialized_columns).

Without the committed snapshot, syrupy will fail the assertion on every run unless --snapshot-update is explicitly passed. These snapshots need to be generated (e.g. pytest ... --snapshot-update) and committed alongside the test.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: ee/clickhouse/queries/test/test_cohort_query.py
Line: 1401-1433

Comment:
**Missing snapshot files will cause CI failure**

This test is decorated with `@snapshot_clickhouse_queries`, which uses `syrupy` to compare queries against stored snapshots in `__snapshots__/test_cohort_query.ambr`. The snapshot file contains no entry for `test_is_set_operator_excludes_null_values_for_materialized_columns` (or the materialized-column variant generated by `@also_test_with_materialized_columns`).

Without the committed snapshot, syrupy will fail the assertion on every run unless `--snapshot-update` is explicitly passed. These snapshots need to be generated (e.g. `pytest ... --snapshot-update`) and committed alongside the test.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +1401 to +1433

@greptile-apps greptile-apps Bot Apr 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 is_not_set fix is untested

The PR also changes empty(col) → isNull(col) for the is_not_set denormalized path, which is a symmetric behavioral change (a property set to an empty string "" will no longer match is_not_set). However, there is no corresponding test for this operator. Mirroring the existing test to cover is_not_set (and ideally parameterising the two operators together, per the team's preference for parameterised tests) would ensure both halves of the fix are verified.

For example, a parameterised approach:

@parameterized.expand(
    [
        ("is_set", "is_set", True),
        ("is_not_set", "is_not_set", False),
    ]
)
@also_test_with_materialized_columns(person_properties=["$some_prop"])
@snapshot_clickhouse_queries
def test_set_operators_with_materialized_columns(self, _name, operator, person_with_prop_included):
    p1 = _create_person(team_id=self.team.pk, distinct_ids=["p1"], properties={"$some_prop": "a_value"})
    _create_person(team_id=self.team.pk, distinct_ids=["p2"], properties={})
    ...
    expected = [p1.uuid] if person_with_prop_included else []
    assert expected == [r[0] for r in res]
Prompt To Fix With AI 
This is a comment left during a code review.
Path: ee/clickhouse/queries/test/test_cohort_query.py
Line: 1401-1433

Comment:
**`is_not_set` fix is untested**

The PR also changes `empty(col)` → `isNull(col)` for the `is_not_set` denormalized path, which is a symmetric behavioral change (a property set to an empty string `""` will no longer match `is_not_set`). However, there is no corresponding test for this operator. Mirroring the existing test to cover `is_not_set` (and ideally parameterising the two operators together, per the team's preference for parameterised tests) would ensure both halves of the fix are verified.

For example, a parameterised approach:

```python
@parameterized.expand(
    [
        ("is_set", "is_set", True),
        ("is_not_set", "is_not_set", False),
    ]
)
@also_test_with_materialized_columns(person_properties=["$some_prop"])
@snapshot_clickhouse_queries
def test_set_operators_with_materialized_columns(self, _name, operator, person_with_prop_included):
    p1 = _create_person(team_id=self.team.pk, distinct_ids=["p1"], properties={"$some_prop": "a_value"})
    _create_person(team_id=self.team.pk, distinct_ids=["p2"], properties={})
    ...
    expected = [p1.uuid] if person_with_prop_included else []
    assert expected == [r[0] for r in res]
```

How can I resolve this? If you propose a fix, please make it concise.


def test_earliest_date_clause(self):
filter = Filter(
data={
Expand Down
4 changes: 2 additions & 2 deletions posthog/models/property/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ def prop_filter_json_extract(
}
if is_denormalized:
return (
" {property_operator} notEmpty({left})".format(left=property_expr, property_operator=property_operator),
" {property_operator} isNotNull({left})".format(left=property_expr, property_operator=property_operator),
params,
)
return (
Expand All @@ -510,7 +510,7 @@ def prop_filter_json_extract(
}
if is_denormalized:
return (
" {property_operator} empty({left})".format(left=property_expr, property_operator=property_operator),
" {property_operator} isNull({left})".format(left=property_expr, property_operator=property_operator),
params,
)
return (
Expand Down
Loading