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.

Sign up for GitHub

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

Jump to bottom

feat(correlation): Enable person-on-events querying #9952

Merged
merged 6 commits into from
May 26, 2022
Merged

feat(correlation): Enable person-on-events querying #9952

merged 6 commits into from
May 26, 2022

Conversation

neilkakkar
Copy link
Collaborator

@neilkakkar neilkakkar commented May 24, 2022 •
edited
Loading

Problem

Part X of #9802

Changes

  1. Correlation Persons for properties now don't re-join persons to filter on properties. Rather, this filtering happens in the base actor query itself
    1. The above is not possible for correlation events (since filtering on that would destroy the funnel)
    2. This became a lot more feasible after and-or filtering got sorted out
  2. When persons on event is enabled, persons for event correlations push up person properties, rather than join in persons again to get them. Same for the actual property parsing.

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

@neilkakkar neilkakkar mentioned this pull request May 25, 2022
Closed
19 tasks
@neilkakkar neilkakkar marked this pull request as ready for review May 25, 2022 13:28
@neilkakkar neilkakkar requested a review from EDsCODE May 25, 2022 13:31
EDsCODE
EDsCODE approved these changes May 26, 2022
View reviewed changes
Copy link
Member

@EDsCODE EDsCODE left a comment

Choose a reason for hiding this comment

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

Apart from the NIT reuse, looks good. Still not super comfortable with how many inline checks we do to swap in and out given the flag, but seems to check out

ee/clickhouse/queries/funnels/base.py
entities_to_use = entities or self._filter.entities

extra_fields = []
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Can reuse _get_person_and_group_properties here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

extra_fields is reused below, hence didn't 😅

ee/clickhouse/queries/funnels/base.py Outdated
parsed_fields = f", {', '.join(fields)}" if fields else ""
return parsed_fields

def _get_person_and_group_properties_aggregate(self) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Can consider just making _get_person_and_group_properties an "aggregate" bool field that determines how the select field should look.

** These two NITS were mainly readability. I was confused at first why it was repeating

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense, done!

@EDsCODE
Copy link
Member

EDsCODE commented May 26, 2022

🚀

@neilkakkar neilkakkar merged commit 52252e0 into master May 26, 2022
@neilkakkar neilkakkar deleted the correlation-person-events branch May 26, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EDsCODE EDsCODE EDsCODE approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@neilkakkar @EDsCODE