feat(person-on-events): Person on events trends #9645
Conversation
| FROM events e | ||
| INNER JOIN ({GET_TEAM_PERSON_DISTINCT_IDS}) as pdi |
There was a problem hiding this comment.
this was a nasty bug. Always having these here meant we'll have two pdi joins when we also want to join persons
https://sentry.io/organizations/posthog2/issues/3154668145/?query=is%3Aunresolved
| @@ -95,10 +95,11 @@ def actor_query(self, limit_actors: Optional[bool] = True) -> Tuple[str, Dict]: | |||
| filter=self._filter, | |||
| team=self._team, | |||
| entity=self.entity, | |||
| should_join_distinct_ids=not self.is_aggregating_by_groups, | |||
| should_join_persons=not self.is_aggregating_by_groups, | |||
There was a problem hiding this comment.
Person join only needs to happen if we have person properties to handle, not every time. (Notice snapshot tests being updated, where the person join was unnecessary)
| @@ -59,7 +60,7 @@ def __init__( | |||
| self._should_join_distinct_ids = should_join_distinct_ids | |||
| self._should_join_persons = should_join_persons | |||
| self._extra_fields = extra_fields | |||
| self._extra_person_fields = extra_person_fields | |||
There was a problem hiding this comment.
annoying diff: extra_person_fields are set in a line above
| @@ -186,7 +186,7 @@ | |||
| GROUP BY entrance_period_start) data | |||
| RIGHT OUTER JOIN | |||
| (SELECT toStartOfDay(toDateTime('2021-04-30 07:00:00') + toIntervalDay(number), 'US/Pacific') AS entrance_period_start | |||
| FROM numbers(dateDiff('day', toDateTime('2021-04-30 07:00:00'), toDateTime('2021-05-08 06:59:59')) + 1) AS period_offsets) fill USING (entrance_period_start) | |||
| FROM numbers(dateDiff('day', toDateTime('2021-04-30 07:00:00'), toDateTime('2021-05-07 07:00:00')) + 1) AS period_offsets) fill USING (entrance_period_start) | |||
There was a problem hiding this comment.
hmm, why are these changes happening in my PR? 🤔
CH version change issue?
| with override_instance_config("ENABLE_ACTOR_ON_EVENTS_TEAMS", f"{self.team.pk}"): | ||
| new_response = ClickhouseTrends().run(filter, self.team) | ||
|
|
||
| self.assertEqual(response, new_response) |
There was a problem hiding this comment.
Do we still want this now that we have new testing pattern?
There was a problem hiding this comment.
Not really. Will get rid of it
|
@neilkakkar This should be good to go! |
| @@ -1849,6 +1814,20 @@ def test_person_property_filtering(self): | |||
| ), | |||
| self.team, | |||
| ) | |||
|
|
|||
| with freeze_time("2020-01-04"), override_instance_config("ENABLE_ACTOR_ON_EVENTS_TEAMS", f"{self.team.pk}"): | |||
There was a problem hiding this comment.
will remove in a follow up 🙈
Problem
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?