Skip to content

Commit

Permalink
fix: null properties (#28297)
Browse files Browse the repository at this point in the history
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
  • Loading branch information
aspicer and github-actions[bot] authored Feb 6, 2025
1 parent 6552418 commit 896b6bd
Show file tree
Hide file tree
Showing 22 changed files with 135 additions and 58 deletions.
53 changes: 53 additions & 0 deletions ee/clickhouse/queries/test/test_cohort_query.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from posthog.models.filters.filter import Filter
from posthog.models.property import Property, PropertyGroup

from posthog.models.property_definition import PropertyDefinition
from posthog.test.base import (
BaseTest,
ClickhouseTestMixin,
Expand Down Expand Up @@ -3364,3 +3365,55 @@ def test_basic_valid_negation_tree_with_no_negations(self):
has_pending_neg, has_reg = check_negation_clause(property_group)
self.assertEqual(has_pending_neg, False)
self.assertEqual(has_reg, True)

def test_type_misalignment(self):
PropertyDefinition.objects.create(
team=self.team,
name="createdDate",
property_type="DateTime",
type=PropertyDefinition.Type.PERSON,
)

_create_person(
team_id=self.team.pk,
distinct_ids=["p3"],
properties={"name": "test2", "email": "[email protected]", "createdDate": "2022-10-11"},
)

cohort = Cohort.objects.create(
team=self.team,
name="cohort",
is_static=False,
filters={
"properties": {
"type": "OR",
"values": [
{
"type": "OR",
"values": [
{
"key": "createdDate",
"type": "person",
"value": "2022",
"negation": False,
"operator": "icontains",
}
],
}
],
}
},
)

filter = Filter(
data={
"properties": {
"type": "OR",
"values": [{"key": "id", "value": cohort.pk, "type": "cohort"}],
}
},
team=self.team,
)

res, q, params = execute(filter, self.team)
assert 1 == len(res)
1 change: 0 additions & 1 deletion mypy-baseline.txt
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,6 @@ posthog/hogql/resolver.py:0: error: Item "None" of "SelectQuery | SelectSetQuery
posthog/hogql/resolver.py:0: error: Incompatible types in assignment (expression has type "Type | Any | None", variable has type "BaseTableType | SelectSetQueryType | SelectQueryType | SelectQueryAliasType | SelectViewType | None") [assignment]
posthog/hogql/resolver.py:0: error: Argument 1 to "append" of "list" has incompatible type "BaseTableType | SelectSetQueryType | SelectQueryType | SelectQueryAliasType | SelectViewType | None"; expected "SelectQueryType | SelectSetQueryType" [arg-type]
posthog/hogql/resolver.py:0: error: Statement is unreachable [unreachable]
posthog/hogql/resolver.py:0: error: Statement is unreachable [unreachable]
posthog/hogql/resolver.py:0: error: Item "None" of "Type | None" has no attribute "resolve_constant_type" [union-attr]
posthog/hogql/resolver.py:0: error: Item "None" of "Type | None" has no attribute "resolve_constant_type" [union-attr]
posthog/hogql/resolver.py:0: error: Item "None" of "Type | None" has no attribute "resolve_constant_type" [union-attr]
Expand Down
4 changes: 2 additions & 2 deletions posthog/api/test/__snapshots__/test_query.ambr
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
'a%sd',
concat(ifNull(toString(events.event), ''), ' ', ifNull(toString(replaceRegexpAll(nullIf(nullIf(JSONExtractRaw(events.properties, 'key'), ''), 'null'), '^"|"$', '')), ''))
FROM events
WHERE and(equals(events.team_id, 99999), ifNull(ilike(replaceRegexpAll(nullIf(nullIf(JSONExtractRaw(events.properties, 'path'), ''), 'null'), '^"|"$', ''), '%/%'), 0), less(toTimeZone(events.timestamp, 'UTC'), toDateTime64('2020-01-10 12:14:05.000000', 6, 'UTC')), greater(toTimeZone(events.timestamp, 'UTC'), toDateTime64('2020-01-09 12:14:00.000000', 6, 'UTC')))
WHERE and(equals(events.team_id, 99999), ifNull(ilike(toString(replaceRegexpAll(nullIf(nullIf(JSONExtractRaw(events.properties, 'path'), ''), 'null'), '^"|"$', '')), '%/%'), 0), less(toTimeZone(events.timestamp, 'UTC'), toDateTime64('2020-01-10 12:14:05.000000', 6, 'UTC')), greater(toTimeZone(events.timestamp, 'UTC'), toDateTime64('2020-01-09 12:14:00.000000', 6, 'UTC')))
ORDER BY events.event ASC
LIMIT 101
OFFSET 0 SETTINGS readonly=2,
Expand Down Expand Up @@ -113,7 +113,7 @@
'a%sd',
concat(ifNull(toString(events.event), ''), ' ', ifNull(toString(nullIf(nullIf(events.mat_key, ''), 'null')), ''))
FROM events
WHERE and(equals(events.team_id, 99999), ifNull(ilike(nullIf(nullIf(events.mat_path, ''), 'null'), '%/%'), 0), less(toTimeZone(events.timestamp, 'UTC'), toDateTime64('2020-01-10 12:14:05.000000', 6, 'UTC')), greater(toTimeZone(events.timestamp, 'UTC'), toDateTime64('2020-01-09 12:14:00.000000', 6, 'UTC')))
WHERE and(equals(events.team_id, 99999), ifNull(ilike(toString(nullIf(nullIf(events.mat_path, ''), 'null')), '%/%'), 0), less(toTimeZone(events.timestamp, 'UTC'), toDateTime64('2020-01-10 12:14:05.000000', 6, 'UTC')), greater(toTimeZone(events.timestamp, 'UTC'), toDateTime64('2020-01-09 12:14:00.000000', 6, 'UTC')))
ORDER BY events.event ASC
LIMIT 101
OFFSET 0 SETTINGS readonly=2,
Expand Down
6 changes: 6 additions & 0 deletions posthog/api/test/test_hog_function.py
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,9 @@ def test_generates_filters_bytecode(self, *args):
"person",
1,
3,
2,
"toString",
1,
20,
32,
"$pageview",
Expand All @@ -827,6 +830,9 @@ def test_generates_filters_bytecode(self, *args):
"person",
1,
3,
2,
"toString",
1,
20,
32,
"$pageview",
Expand Down
21 changes: 21 additions & 0 deletions posthog/cdp/test/test_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ def test_filters_events(self):
"properties",
1,
2,
2,
"toString",
1,
18,
3,
2,
Expand Down Expand Up @@ -160,6 +163,9 @@ def test_filters_properties(self):
"person",
1,
3,
2,
"toString",
1,
18,
32,
"ben",
Expand Down Expand Up @@ -193,6 +199,9 @@ def test_filters_full(self):
"person",
1,
3,
2,
"toString",
1,
20,
32,
"%@posthog.com%",
Expand All @@ -204,6 +213,9 @@ def test_filters_full(self):
"person",
1,
3,
2,
"toString",
1,
18,
32,
"ben",
Expand Down Expand Up @@ -231,6 +243,9 @@ def test_filters_full(self):
"properties",
1,
2,
2,
"toString",
1,
18,
3,
5,
Expand All @@ -244,6 +259,9 @@ def test_filters_full(self):
"person",
1,
3,
2,
"toString",
1,
20,
32,
"%@posthog.com%",
Expand All @@ -255,6 +273,9 @@ def test_filters_full(self):
"person",
1,
3,
2,
"toString",
1,
18,
32,
"ben",
Expand Down
2 changes: 1 addition & 1 deletion posthog/cdp/test/test_site_functions.py
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ def test_get_transpiled_function_with_event_filter(self):
assert "const filterMatches = " in result
assert '__getGlobal("event") == "$pageview"' in result
assert (
'(ilike(__getProperty(__getProperty(__getGlobal("person"), "properties", true), "email", true), "%@test.com%")'
'(ilike(toString(__getProperty(__getProperty(__getGlobal("person"), "properties", true), "email", true)), "%@test.com%")'
in result
)

Expand Down
4 changes: 3 additions & 1 deletion posthog/hogql/ast.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import dataclasses
import inspect
import sys
from enum import StrEnum
Expand Down Expand Up @@ -569,7 +570,8 @@ def resolve_constant_type(self, context: HogQLContext) -> ConstantType:
if self.joined_subquery is not None and self.joined_subquery_field_name is not None:
return self.joined_subquery.resolve_column_constant_type(self.joined_subquery_field_name, context)

return self.field_type.resolve_constant_type(context)
# PropertyTypes are always nullable
return dataclasses.replace(self.field_type.resolve_constant_type(context), nullable=True)


@dataclass(kw_only=True)
Expand Down
4 changes: 2 additions & 2 deletions posthog/hogql/property.py
Original file line number Diff line number Diff line change
Expand Up @@ -184,13 +184,13 @@ def _expr_to_compare_op(
elif operator == PropertyOperator.ICONTAINS:
return ast.CompareOperation(
op=ast.CompareOperationOp.ILike,
left=expr,
left=ast.Call(name="toString", args=[expr]),
right=ast.Constant(value=f"%{value}%"),
)
elif operator == PropertyOperator.NOT_ICONTAINS:
return ast.CompareOperation(
op=ast.CompareOperationOp.NotILike,
left=expr,
left=ast.Call(name="toString", args=[expr]),
right=ast.Constant(value=f"%{value}%"),
)
elif operator == PropertyOperator.REGEX:
Expand Down
9 changes: 3 additions & 6 deletions posthog/hogql/resolver.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import dataclasses
from datetime import date, datetime
from typing import Any, Literal, Optional, cast
from uuid import UUID
Expand Down Expand Up @@ -502,12 +503,8 @@ def visit_call(self, node: ast.Call):
signatures = HOGQL_CLICKHOUSE_FUNCTIONS[node.name].signatures
if signatures:
for sig_arg_types, sig_return_type in signatures:
if sig_arg_types is None:
return_type = sig_return_type
break

if compare_types(arg_types, sig_arg_types):
return_type = sig_return_type
if sig_arg_types is None or compare_types(arg_types, sig_arg_types):
return_type = dataclasses.replace(sig_return_type)
break

if return_type is None:
Expand Down
24 changes: 12 additions & 12 deletions posthog/hogql/test/__snapshots__/test_resolver.ambr
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@
arg_types: [
{
data_type: "str"
nullable: False
nullable: True
}
]
name: "toUUID"
Expand Down Expand Up @@ -1271,7 +1271,7 @@
arg_types: [
{
data_type: "str"
nullable: False
nullable: True
}
]
name: "toUUID"
Expand Down Expand Up @@ -2008,7 +2008,7 @@
arg_types: [
{
data_type: "str"
nullable: False
nullable: True
}
]
name: "toUUID"
Expand Down Expand Up @@ -2187,7 +2187,7 @@
arg_types: [
{
data_type: "str"
nullable: False
nullable: True
}
]
name: "toUUID"
Expand Down Expand Up @@ -3323,7 +3323,7 @@
arg_types: [
{
data_type: "str"
nullable: False
nullable: True
}
]
name: "toUUID"
Expand Down Expand Up @@ -4346,7 +4346,7 @@
arg_types: [
{
data_type: "str"
nullable: False
nullable: True
}
]
name: "toUUID"
Expand Down Expand Up @@ -5043,7 +5043,7 @@
arg_types: [
{
data_type: "str"
nullable: False
nullable: True
}
]
name: "toUUID"
Expand Down Expand Up @@ -5222,7 +5222,7 @@
arg_types: [
{
data_type: "str"
nullable: False
nullable: True
}
]
name: "toUUID"
Expand Down Expand Up @@ -6231,7 +6231,7 @@
arg_types: [
{
data_type: "str"
nullable: False
nullable: True
}
]
name: "toUUID"
Expand Down Expand Up @@ -6410,7 +6410,7 @@
arg_types: [
{
data_type: "str"
nullable: False
nullable: True
}
]
name: "toUUID"
Expand Down Expand Up @@ -7163,7 +7163,7 @@
arg_types: [
{
data_type: "str"
nullable: False
nullable: True
}
]
name: "toUUID"
Expand Down Expand Up @@ -7342,7 +7342,7 @@
arg_types: [
{
data_type: "str"
nullable: False
nullable: True
}
]
name: "toUUID"
Expand Down
2 changes: 1 addition & 1 deletion posthog/hogql/test/test_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,5 +153,5 @@ def test_replace_filters_test_accounts(self):
)
self.assertEqual(
self._print_ast(select),
f"SELECT event FROM events WHERE notILike(person.properties.email, '%posthog.com%') LIMIT {MAX_SELECT_RETURNED_ROWS}",
f"SELECT event FROM events WHERE notILike(toString(person.properties.email), '%posthog.com%') LIMIT {MAX_SELECT_RETURNED_ROWS}",
)
10 changes: 5 additions & 5 deletions posthog/hogql/test/test_property.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,11 @@ def test_property_to_expr_event(self):
)
self.assertEqual(
self._property_to_expr({"type": "event", "key": "a", "value": "3", "operator": "icontains"}),
self._parse_expr("properties.a ilike '%3%'"),
self._parse_expr("toString(properties.a) ilike '%3%'"),
)
self.assertEqual(
self._property_to_expr({"type": "event", "key": "a", "value": "3", "operator": "not_icontains"}),
self._parse_expr("properties.a not ilike '%3%'"),
self._parse_expr("toString(properties.a) not ilike '%3%'"),
)
self.assertEqual(
self._property_to_expr({"type": "event", "key": "a", "value": ".*", "operator": "regex"}),
Expand Down Expand Up @@ -233,7 +233,7 @@ def test_property_to_expr_event_list(self):
"operator": "icontains",
}
),
self._parse_expr("properties.a ilike '%b%' or properties.a ilike '%c%'"),
self._parse_expr("toString(properties.a) ilike '%b%' or toString(properties.a) ilike '%c%'"),
)
a = self._property_to_expr({"type": "event", "key": "a", "value": ["b", "c"], "operator": "regex"})
self.assertEqual(
Expand All @@ -258,7 +258,7 @@ def test_property_to_expr_event_list(self):
"operator": "not_icontains",
}
),
self._parse_expr("properties.a not ilike '%b%' and properties.a not ilike '%c%'"),
self._parse_expr("toString(properties.a) not ilike '%b%' and toString(properties.a) not ilike '%c%'"),
)
a = self._property_to_expr(
{
Expand Down Expand Up @@ -353,7 +353,7 @@ def test_property_to_expr_element(self):
"operator": "icontains",
}
),
self._parse_expr("elements_chain_href ilike '%href-text.%'"),
self._parse_expr("toString(elements_chain_href) ilike '%href-text.%'"),
)
self.assertEqual(
self._property_to_expr(
Expand Down
Loading

0 comments on commit 896b6bd

Please sign in to comment.