feat: support AND / OR operands in replay filters #22955
Conversation
|
|
||
| // set the type on the nested child when only using a single filter group | ||
| const hasSingleGroup = universalFilters.filter_group.values.length === 1 | ||
| if (hasSingleGroup) { |
There was a problem hiding this comment.
Given we have two levels of nesting (to make it easier to support multiple filter groups in future) it's necessary to update the operand at both levels if there is only a single filter because the UI makes it look like there is only one level of nesting
| @@ -464,7 +465,7 @@ def _where_predicates(self) -> ast.Expr: | |||
|
|
|||
| (event_where_exprs, _) = self._event_predicates | |||
| if event_where_exprs: | |||
| exprs.append(ast.Or(exprs=event_where_exprs)) | |||
There was a problem hiding this comment.
| person_id_subquery = PersonsIdSubQuery(self._team, self._filter, self.ttl_days).get_query() | ||
| if person_id_subquery: | ||
| exprs.append( | ||
| ast.CompareOperation( | ||
| op=ast.CompareOperationOp.In, | ||
| left=ast.Field(chain=["s", "distinct_id"]), | ||
| right=person_id_subquery, | ||
| ) | ||
| ) | ||
|
|
||
| if self._filter.session_ids: | ||
| exprs.append( | ||
| ast.CompareOperation( | ||
| op=ast.CompareOperationOp.In, | ||
| left=ast.Field(chain=["session_id"]), | ||
| right=ast.Constant(value=self._filter.session_ids), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
No changes, just pulled the mandatory expressions together
| @@ -485,7 +486,7 @@ def _having_predicates(self) -> ast.Expr: | |||
|
|
|||
| if event_names: | |||
| return ast.Call( | |||
| name="hasAll", | |||
| name="hasAll" if self._filter._operand == "AND" else "hasAny", | |||
There was a problem hiding this comment.
I'm assuming this is an optimisation. We cannot rely on every event being in the array when using OR conditions
| @@ -473,7 +474,7 @@ def _where_predicates(self) -> ast.Expr: | |||
| exprs.append( | |||
| ast.CompareOperation( | |||
| op=ast.CompareOperationOp.In, | |||
| left=ast.Constant(value="`$session_id`"), | |||
| left=ast.Field(chain=["$session_id"]), | |||
There was a problem hiding this comment.
Noticed this as a bug thanks to @robbie-c. It was comparing the value "$session" and wasn't being caught by any of our tests :/
There was a problem hiding this comment.
should we have a test? (what's the error?)
There was a problem hiding this comment.
It was using the constant (string value) not the field e.g. "$session_id" in ['uuid1', 'uuid2',...] instead of $session_id in ['uuid1', 'uuid2',...]
I think the effect is that pinned recordings weren't being shown in playlists for HogQL filters.
One of my new tests relies on the $session_id field so that should cover it
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
|
Size Change: +36 B (0%) Total Size: 1.06 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
…ostHog/posthog into dn-feat/support-replay-operand
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated3 snapshot changes in total. 0 added, 3 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated3 snapshot changes in total. 0 added, 3 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
| @@ -473,7 +474,7 @@ def _where_predicates(self) -> ast.Expr: | |||
| exprs.append( | |||
| ast.CompareOperation( | |||
| op=ast.CompareOperationOp.In, | |||
| left=ast.Constant(value="`$session_id`"), | |||
| left=ast.Field(chain=["$session_id"]), | |||
There was a problem hiding this comment.
should we have a test? (what's the error?)
| @@ -273,7 +278,7 @@ def _having_predicates(self) -> ast.And | Constant: | |||
| ), | |||
| ) | |||
|
|
|||
| return ast.And(exprs=exprs) if exprs else Constant(value=True) | |||
There was a problem hiding this comment.
exprs always has at least one value
|
@pauldambra yep, ran it locally and filters still seem to work |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
Problem
Universal filtering does not support AND / OR predicates right now
Changes
Implicit assumption in here that this only works for the new HogQL filters. There's no reason to backport given they're enabled for most customers and haven't been causing issues
(Other comments inline)
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Added tests