Repository navigation
Conversation
…n materialized properties
Prompt To Fix All With AIThis 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.
---
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.Reviews (1): Last reviewed commit: "fix(cohorts): use isNotNull instead of n..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @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] |
There was a problem hiding this comment.
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.| @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] |
There was a problem hiding this 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:
@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.
|
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 |
|
Still relevant - this fixes a real bug where the is_set operator incorrectly includes null values on materialized columns. Happy to make any changes if needed. |
|
This fix is still valid - the is_set operator incorrectly includes null values on materialized columns. Happy to rebase if needed, or close if the team has a different approach in mind. |
|
Closing - this has been stale for a month. The fix is still valid if the team wants to revisit. |
Problem
Fixes #29916
When using the
is_setoperator in cohort person property filters with denormalized (materialized) columns, null values are incorrectly included. The code was using ClickHouse'snotEmpty()function which doesn't properly distinguish between "property was never set" (null) and "property exists but is empty string."Changes
Changed the denormalized path in
posthog/models/property/util.py:is_set:notEmpty(col)→isNotNull(col)is_not_set:empty(col)→isNull(col)The non-denormalized (JSON) path using
JSONHasis unaffected and was already correct.How did you test this code?
test_is_set_operator_excludes_null_values_for_materialized_columnsinee/clickhouse/queries/test/test_cohort_query.pythat verifies:is_setcohort@also_test_with_materialized_columnsdecorator to run against both materialized and non-materialized pathsPublish to changelog?
no
Docs update
no