Skip to content
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.

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

chore: trends query runner tests #18479

Merged
merged 15 commits into from
Nov 24, 2023
32 changes: 30 additions & 2 deletions posthog/hogql/database/schema/event_sessions.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,18 @@ def visit_field(self, node: ast.Field):
return super().visit_field(node)


class ContainsLazyJoinType(TraversingVisitor):
contains_lazy_join: bool

def __init__(self, expr: ast.Expr):
super().__init__()
self.contains_lazy_join = False
super().visit(expr)

def visit_lazy_join_type(self, node: ast.LazyJoinType):
self.contains_lazy_join = True


class WhereClauseExtractor:
compare_operators: List[ast.Expr]

Expand Down Expand Up @@ -109,14 +121,30 @@ def _is_field_on_table(self, field: ast.Field) -> bool:
def run(self, expr: ast.Expr) -> List[ast.Expr]:
exprs_to_apply: List[ast.Expr] = []

def should_add(expression: ast.Expr, fields: List[ast.Field]) -> bool:
for field in fields:
on_table = self._is_field_on_table(field)
if not on_table:
return False

# Ignore comparisons on the `event` field for session durations
if field.chain[-1] == "event":
return False

# Ignore if there's a lazy join involved
if ContainsLazyJoinType(expression).contains_lazy_join:
return False
Comment on lines +135 to +136
Copy link
Collaborator

Choose a reason for hiding this comment

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

copout 🤣


return True

if isinstance(expr, ast.And):
for expression in expr.exprs:
if not isinstance(expression, ast.CompareOperation):
continue

fields = GetFieldsTraverser(expression).fields
res = [self._is_field_on_table(field) for field in fields]
if all(res):

if should_add(expression, fields):
exprs_to_apply.append(expression)
elif isinstance(expr, ast.CompareOperation):
exprs_to_apply.extend(self.run(ast.And(exprs=[expr])))
Expand Down
26 changes: 13 additions & 13 deletions posthog/hogql/database/schema/test/test_event_sessions.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,17 @@ def test_with_simple_equality_clause(self):
"""
SELECT event
FROM events
WHERE event = '$pageview'
WHERE team_id = 1
"""
)

compare_operators = self._compare_operators(query, "events")

assert len(compare_operators) == 1
assert compare_operators[0] == ast.CompareOperation(
left=ast.Field(chain=["event"]),
left=ast.Field(chain=["team_id"]),
op=ast.CompareOperationOp.Eq,
right=ast.Constant(value="$pageview"),
right=ast.Constant(value=1),
)

def test_with_timestamps(self):
Expand All @@ -66,35 +66,35 @@ def test_with_alias_table(self):
"""
SELECT e.event
FROM events e
WHERE e.event = '$pageview'
WHERE e.team_id = 1
"""
)

compare_operators = self._compare_operators(query, "e")

assert len(compare_operators) == 1
assert compare_operators[0] == ast.CompareOperation(
left=ast.Field(chain=["event"]),
left=ast.Field(chain=["team_id"]),
Copy link
Collaborator

Choose a reason for hiding this comment

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

How come you're comparing the team_id here? This field is something that gets handled outside of HogQL... so these queries would have it double. It's in many ways reserved in HogQL (e.g. you can't make an alias with this name), so seeing it in tests makes me automatically wonder if something is wrong and if we're leaking data... 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we not intentionally exclude event from the where clause, so just wanted to pick any other field for the tests instead. But I get it being possibly weird seeing this in a test

op=ast.CompareOperationOp.Eq,
right=ast.Constant(value="$pageview"),
right=ast.Constant(value=1),
)

def test_with_multiple_clauses(self):
query = self._select(
"""
SELECT event
FROM events
WHERE event = '$pageview' AND timestamp > '2023-01-01'
WHERE team_id = 1 AND timestamp > '2023-01-01'
"""
)

compare_operators = self._compare_operators(query, "events")

assert len(compare_operators) == 2
assert compare_operators[0] == ast.CompareOperation(
left=ast.Field(chain=["event"]),
left=ast.Field(chain=["team_id"]),
op=ast.CompareOperationOp.Eq,
right=ast.Constant(value="$pageview"),
right=ast.Constant(value=1),
)
assert compare_operators[1] == ast.CompareOperation(
left=ast.Field(chain=["timestamp"]),
Expand All @@ -109,25 +109,25 @@ def test_with_join(self):
FROM events e
LEFT JOIN persons p
ON e.person_id = p.id
WHERE e.event = '$pageview' and p.is_identified = 0
WHERE e.team_id = 1 and p.is_identified = 0
"""
)

compare_operators = self._compare_operators(query, "e")

assert len(compare_operators) == 1
assert compare_operators[0] == ast.CompareOperation(
left=ast.Field(chain=["event"]),
left=ast.Field(chain=["team_id"]),
op=ast.CompareOperationOp.Eq,
right=ast.Constant(value="$pageview"),
right=ast.Constant(value=1),
)

def test_with_ignoring_ors(self):
query = self._select(
"""
SELECT event
FROM events
WHERE event = '$pageleave' OR event = '$pageview'
WHERE team_id = 1 OR team_id = 2
"""
)

Expand Down
5 changes: 4 additions & 1 deletion posthog/hogql/printer.py
Original file line number Diff line number Diff line change
Expand Up @@ -892,7 +892,10 @@ def visit_field_type(self, type: ast.FieldType):
field_sql = "person_props"

else:
raise HogQLException(f"Unknown FieldType table type: {type.table_type.__class__.__name__}")
error = f"Can't access field '{type.name}' on a table with type '{type.table_type.__class__.__name__}'."
if isinstance(type.table_type, ast.LazyJoinType):
error += f" Lazy joins should have all been replaced in the resolver."
raise HogQLException(error)

return field_sql

Expand Down
23 changes: 17 additions & 6 deletions posthog/hogql/property.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,24 @@ def property_to_expr(

if property.type == "hogql":
return parse_expr(property.key)
elif property.type == "event" or property.type == "feature" or property.type == "person":
elif (
property.type == "event" or property.type == "feature" or property.type == "person" or property.type == "group"
):
if scope == "person" and property.type != "person":
raise NotImplementedException(
f"The '{property.type}' property filter only works in 'event' scope, not in '{scope}' scope"
)
operator = cast(Optional[PropertyOperator], property.operator) or PropertyOperator.exact
value = property.value

if property.type == "person" and scope != "person":
chain = ["person", "properties"]
elif property.type == "group":
chain = [f"group_{property.group_type_index}", "properties"]
else:
chain = ["properties"]
field = ast.Field(chain=chain + [property.key])

if isinstance(value, list):
if len(value) == 0:
return ast.Constant(value=True)
Expand All @@ -135,7 +146,7 @@ def property_to_expr(

return ast.CompareOperation(
op=op,
left=ast.Field(chain=["properties", property.key]),
left=field,
right=ast.Tuple(exprs=[ast.Constant(value=v) for v in value]),
)
else:
Expand All @@ -156,8 +167,6 @@ def property_to_expr(
return ast.And(exprs=exprs)
return ast.Or(exprs=exprs)

chain = ["person", "properties"] if property.type == "person" and scope != "person" else ["properties"]
field = ast.Field(chain=chain + [property.key])
properties_field = ast.Field(chain=chain)

if operator == PropertyOperator.is_set:
Expand Down Expand Up @@ -297,9 +306,11 @@ def property_to_expr(
right=ast.Constant(value=cohort.pk),
)

# TODO: Add support for these types "group", "recording", "behavioral", and "session" types
# TODO: Add support for these types "recording", "behavioral", and "session" types

raise NotImplementedException(f"property_to_expr not implemented for filter type {type(property).__name__}")
raise NotImplementedException(
f"property_to_expr not implemented for filter type {type(property).__name__} and {property.type}"
)


def action_to_expr(action: Action) -> ast.Expr:
Expand Down
23 changes: 23 additions & 0 deletions posthog/hogql/test/test_property.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,29 @@ def test_property_to_expr_hogql(self):
ast.Constant(value=1),
)

def test_property_to_expr_group(self):
self.assertEqual(
self._property_to_expr({"type": "group", "group_type_index": 0, "key": "a", "value": "b"}),
self._parse_expr("group_0.properties.a = 'b'"),
)
self.assertEqual(
self._property_to_expr({"type": "group", "group_type_index": 3, "key": "a", "value": "b"}),
self._parse_expr("group_3.properties.a = 'b'"),
)
self.assertEqual(
self._parse_expr("group_0.properties.a = NULL OR (NOT JSONHas(group_0.properties, 'a'))"),
self._property_to_expr(
{"type": "group", "group_type_index": 0, "key": "a", "value": "b", "operator": "is_not_set"}
),
)

with self.assertRaises(Exception) as e:
self._property_to_expr({"type": "group", "key": "a", "value": "b"})
self.assertEqual(
str(e.exception),
"Missing required key group_type_index for property type group",
)

def test_property_to_expr_event(self):
self.assertEqual(
self._property_to_expr({"key": "a", "value": "b"}),
Expand Down
Loading
Loading