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

Funnels reorganization, persons pagination, and conversion window support #4810

Merged
merged 15 commits into from
Jun 24, 2021

Conversation

buwilliams
Copy link
Contributor

@buwilliams buwilliams commented Jun 19, 2021

Changes

  • Reorganize code structure for funnels to support differences between queries and formats
  • GenerateLocal class to generate data for local development
  • microseconds_from_days to convert days to microseconds
  • Handle conversion window for funnels
  • If offset > 0 use new SQL to fetch offset list of persons

GenerateLocal Usage

Open django shell with Clickhouse environment variables

from ee.clickhouse.generate_local import GenerateLocal
g = GenerateLocal()
g.generate()

You can also cleanup the data with but you'll need to drop the Clickhouse db manually.

g.destroy()

@buwilliams buwilliams marked this pull request as draft June 19, 2021 14:03
@timgl timgl temporarily deployed to posthog-pr-4810 June 19, 2021 14:06 Inactive
@timgl timgl temporarily deployed to posthog-pr-4810 June 19, 2021 18:23 Inactive
@timgl timgl temporarily deployed to posthog-pr-4810 June 20, 2021 14:24 Inactive
@timgl timgl temporarily deployed to posthog-pr-4810 June 21, 2021 12:35 Inactive
@buwilliams buwilliams marked this pull request as ready for review June 21, 2021 13:04
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.

Comment below + backend tests. Otherwise looks good

ee/clickhouse/queries/clickhouse_funnel.py Outdated Show resolved Hide resolved
…ed funnel_persons and funnel_trends_persons into individual classes;
@timgl timgl temporarily deployed to posthog-pr-4810 June 21, 2021 20:53 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-pr-4810 June 22, 2021 15:48 Inactive
…com:PostHog/posthog into funnel-persons-pagination-conversion-window
@timgl timgl temporarily deployed to posthog-pr-4810 June 22, 2021 19:42 Inactive
@timgl timgl temporarily deployed to posthog-pr-4810 June 22, 2021 20:04 Inactive
@EDsCODE EDsCODE mentioned this pull request Jun 22, 2021
7 tasks
@@ -79,6 +79,22 @@ def merge_people(self, people_to_merge: List["Person"]):
# Has an index on properties -> email, built concurrently
# See migration 0121

@staticmethod
def get_distinct_ids_and_email_by_ids(person_ids, team_id):
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be the opposite of what it should be?

I.e., bulk querying uses the bulk API (which gains the advantage of querying in bulk), while the single query makes a call to the bulk end point with a list of one item.

https://docs.djangoproject.com/en/3.2/ref/models/querysets/#in-bulk - in_bulk achieves this, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a quick pass but I'll be refactoring with ejecting to SQL for best performance. Just trying to get it out the door quickly.

@buwilliams buwilliams changed the title Funnel persons pagination & conversion window Funnels reorganization, persons pagination, and conversion window support Jun 23, 2021
@timgl timgl temporarily deployed to posthog-pr-4810 June 23, 2021 19:13 Inactive
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.

🚀

@buwilliams buwilliams merged commit d9de618 into master Jun 24, 2021
@buwilliams buwilliams deleted the funnel-persons-pagination-conversion-window branch June 24, 2021 01:49
@buwilliams buwilliams removed their assignment Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants