Skip to content

Commit

Permalink
fix: use uint8 instead of bool where clickhouse does (#23414)
Browse files Browse the repository at this point in the history
  • Loading branch information
aspicer authored Jul 2, 2024
1 parent d1d0320 commit d45c5ee
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 65 deletions.
16 changes: 8 additions & 8 deletions posthog/hogql/property.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,13 @@ def property_to_expr(
# The property was saved as an incomplete object. Instead of crashing the entire query, pretend it's not there.
# TODO: revert this when removing legacy insights?
except ValueError:
return ast.Constant(value=True)
return ast.Constant(value=1)
except TypeError:
return ast.Constant(value=True)
return ast.Constant(value=1)
elif isinstance(property, list):
properties = [property_to_expr(p, team, scope) for p in property]
if len(properties) == 0:
return ast.Constant(value=True)
return ast.Constant(value=1)
if len(properties) == 1:
return properties[0]
return ast.And(exprs=properties)
Expand All @@ -111,7 +111,7 @@ def property_to_expr(
raise QueryError(f'PropertyGroupFilter of unknown type "{property.type}"')

if len(property.values) == 0:
return ast.Constant(value=True)
return ast.Constant(value=1)
if len(property.values) == 1:
return property_to_expr(property.values[0], team, scope)

Expand All @@ -120,13 +120,13 @@ def property_to_expr(
else:
return ast.Or(exprs=[property_to_expr(p, team, scope) for p in property.values])
elif isinstance(property, EmptyPropertyFilter):
return ast.Constant(value=True)
return ast.Constant(value=1)
elif isinstance(property, BaseModel):
try:
property = Property(**property.dict())
except ValueError:
# The property was saved as an incomplete object. Instead of crashing the entire query, pretend it's not there.
return ast.Constant(value=True)
return ast.Constant(value=1)
else:
raise QueryError(f"property_to_expr with property of type {type(property).__name__} not implemented")

Expand Down Expand Up @@ -251,7 +251,7 @@ def property_to_expr(
name="ifNull",
args=[
ast.Call(name="match", args=[ast.Call(name="toString", args=[field]), ast.Constant(value=value)]),
ast.Constant(value=False),
ast.Constant(value=0),
],
)
elif operator == PropertyOperator.NOT_REGEX:
Expand All @@ -266,7 +266,7 @@ def property_to_expr(
)
],
),
ast.Constant(value=True),
ast.Constant(value=1),
],
)
elif operator == PropertyOperator.EXACT or operator == PropertyOperator.IS_DATE_EXACT:
Expand Down
27 changes: 16 additions & 11 deletions posthog/hogql/test/test_property.py
Original file line number Diff line number Diff line change
Expand Up @@ -234,12 +234,15 @@ def test_property_to_expr_event_list(self):
),
self._parse_expr("properties.a ilike '%b%' or properties.a ilike '%c%'"),
)
a = self._property_to_expr({"type": "event", "key": "a", "value": ["b", "c"], "operator": "regex"})
self.assertEqual(
self._property_to_expr({"type": "event", "key": "a", "value": ["b", "c"], "operator": "regex"}),
a,
self._parse_expr(
"ifNull(match(toString(properties.a), 'b'), false) or ifNull(match(toString(properties.a), 'c'), false)"
"ifNull(match(toString(properties.a), 'b'), 0) or ifNull(match(toString(properties.a), 'c'), 0)"
),
)
# Want to make sure this returns 0, not false. Clickhouse uses UInt8s primarily for booleans.
self.assertIs(0, a.exprs[1].args[1].value)
# negative
self.assertEqual(
self._property_to_expr({"type": "event", "key": "a", "value": ["b", "c"], "operator": "is_not"}),
Expand All @@ -256,19 +259,21 @@ def test_property_to_expr_event_list(self):
),
self._parse_expr("properties.a not ilike '%b%' and properties.a not ilike '%c%'"),
)
a = self._property_to_expr(
{
"type": "event",
"key": "a",
"value": ["b", "c"],
"operator": "not_regex",
}
)
self.assertEqual(
self._property_to_expr(
{
"type": "event",
"key": "a",
"value": ["b", "c"],
"operator": "not_regex",
}
),
a,
self._parse_expr(
"ifNull(not(match(toString(properties.a), 'b')), true) and ifNull(not(match(toString(properties.a), 'c')), true)"
"ifNull(not(match(toString(properties.a), 'b')), 1) and ifNull(not(match(toString(properties.a), 'c')), 1)"
),
)
self.assertIs(1, a.exprs[1].args[1].value)

def test_property_to_expr_feature(self):
self.assertEqual(
Expand Down
12 changes: 7 additions & 5 deletions posthog/models/test/test_hog_function.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ def test_hog_function_filters_compilation(self):
33,
1,
11,
29,
33,
1,
32,
"^(localhost|127\\.0\\.0\\.1)($|:)",
32,
Expand Down Expand Up @@ -85,7 +86,8 @@ def test_hog_function_filters_compilation(self):
1,
1,
11,
29,
33,
1,
32,
"^(localhost|127\\.0\\.0\\.1)($|:)",
32,
Expand Down Expand Up @@ -124,7 +126,7 @@ def test_hog_function_team_filters_only_compilation(self):
json_filters = to_dict(item.filters)

assert json.dumps(json_filters["bytecode"]) == snapshot(
'["_h", 29, 32, "^(localhost|127\\\\.0\\\\.0\\\\.1)($|:)", 32, "$host", 32, "properties", 1, 2, 2, "toString", 1, 2, "match", 2, 5, 2, "ifNull", 2, 3, 1]'
'["_h", 33, 1, 32, "^(localhost|127\\\\.0\\\\.0\\\\.1)($|:)", 32, "$host", 32, "properties", 1, 2, 2, "toString", 1, 2, "match", 2, 5, 2, "ifNull", 2, 3, 1]'
)


Expand Down Expand Up @@ -275,9 +277,9 @@ def test_hog_functions_reload_on_team_saved(self):
hog_function_3.refresh_from_db()

assert json.dumps(hog_function_1.filters["bytecode"]) == snapshot(
'["_h", 30, 32, "test", 32, "$pageview", 32, "properties", 1, 2, 2, "toString", 1, 2, "match", 2, 2, "ifNull", 2, 30, 32, "^(localhost|127\\\\.0\\\\.0\\\\.1)($|:)", 32, "$host", 32, "properties", 1, 2, 2, "toString", 1, 2, "match", 2, 2, "ifNull", 2, 3, 2]'
'["_h", 33, 0, 32, "test", 32, "$pageview", 32, "properties", 1, 2, 2, "toString", 1, 2, "match", 2, 2, "ifNull", 2, 33, 0, 32, "^(localhost|127\\\\.0\\\\.0\\\\.1)($|:)", 32, "$host", 32, "properties", 1, 2, 2, "toString", 1, 2, "match", 2, 2, "ifNull", 2, 3, 2]'
)
assert json.dumps(hog_function_2.filters["bytecode"]) == snapshot(
'["_h", 32, "$pageview", 32, "event", 1, 1, 11, 30, 32, "test", 32, "$pageview", 32, "properties", 1, 2, 2, "toString", 1, 2, "match", 2, 2, "ifNull", 2, 30, 32, "^(localhost|127\\\\.0\\\\.0\\\\.1)($|:)", 32, "$host", 32, "properties", 1, 2, 2, "toString", 1, 2, "match", 2, 2, "ifNull", 2, 3, 3, 4, 1]'
'["_h", 32, "$pageview", 32, "event", 1, 1, 11, 33, 0, 32, "test", 32, "$pageview", 32, "properties", 1, 2, 2, "toString", 1, 2, "match", 2, 2, "ifNull", 2, 33, 0, 32, "^(localhost|127\\\\.0\\\\.0\\\\.1)($|:)", 32, "$host", 32, "properties", 1, 2, 2, "toString", 1, 2, "match", 2, 2, "ifNull", 2, 3, 3, 4, 1]'
)
assert json.dumps(hog_function_3.filters["bytecode"]) == snapshot('["_h", 29]')
Loading

0 comments on commit d45c5ee

Please sign in to comment.