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

Funnel step query new #4851

Merged
merged 32 commits into from
Jun 28, 2021
Merged

Funnel step query new #4851

merged 32 commits into from
Jun 28, 2021

Conversation

EDsCODE
Copy link
Member

@EDsCODE EDsCODE commented Jun 22, 2021 •
edited
Loading

Changes

Please describe.

If this affects the frontend, include screenshots.

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
  • Frontend/CSS is usable at 320px (iPhone SE) and decent at 360px (most phones)
  • Breaking changes are backwards-compatible. Ensure old/new frontend requests work with new/old backends, and vice versa.

buwilliams and others added 15 commits June 18, 2021 08:48
@buwilliams
wip: pagination for persons on clickhouse funnels
537bdb3
@buwilliams
wip: added offset support for getting a list of persons; added suppor…
605f9de 
…t for conversion window;
@buwilliams
fixed mypy exception
a9a5f12
@buwilliams
helper function to insert data for local testing
e952308
@buwilliams
moved generate code into separate class for more functionality later
ceda12e
@buwilliams
corrected person_distinct_id to use the person id from postgres
1a29fdb
@buwilliams
minor corrections to generate local class along with addition of data…
1990907 
… cleanup via destroy() method
@buwilliams
reduce the number of persons who make it to each step
7daeaaa
@buwilliams
moved funnel queries to a new folder for better organization; separat…
fba259e 
…ed funnel_persons and funnel_trends_persons into individual classes;
@EDsCODE
Merge branch 'master' into funnel-persons-pagination-conversion-window
cc24758
@buwilliams
funnel persons and tests
142e7b6
@buwilliams
Merge branch 'funnel-persons-pagination-conversion-window' of github.…
df44123 
…com:PostHog/posthog into funnel-persons-pagination-conversion-window
@EDsCODE
initial implementation
589e937
@buwilliams
invoke the funnel or funnel trends class respectively
ac6d601
@EDsCODE
add a test
34d5bad
@EDsCODE EDsCODE mentioned this pull request Jun 22, 2021
Closed
6 tasks
@EDsCODE EDsCODE temporarily deployed to posthog-pr-4851 June 22, 2021 22:10 Inactive
neilkakkar
neilkakkar reviewed Jun 23, 2021
View reviewed changes
ee/clickhouse/queries/funnels/funnel.py Outdated
SELECT
person_id,
timestamp,
{self.get_partition_cols(1, max_steps)}
Copy link
Contributor

@neilkakkar neilkakkar Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If I understand correctly

            {self.get_partition_cols(level_index - 1, max_steps)}

Edit: Ah, no, my bad, it's the innermost one, so 1 is right.

Copy link
Contributor

Choose a reason for hiding this comment

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

step_i seems to be 0 indexed, while level_index seems to be 1 indexed? I think easier if both follow the same?

@EDsCODE EDsCODE temporarily deployed to posthog-pr-4851 June 23, 2021 17:51 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-pr-4851 June 23, 2021 17:52 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-pr-4851 June 23, 2021 20:32 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-pr-4851 June 23, 2021 20:51 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-pr-4851 June 23, 2021 20:58 Inactive
Base automatically changed from funnel-persons-pagination-conversion-window to master June 24, 2021 01:49
@EDsCODE EDsCODE temporarily deployed to posthog-pr-4851 June 24, 2021 04:12 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-pr-4851 June 24, 2021 20:41 Inactive
@Twixes Twixes mentioned this pull request Jun 24, 2021
Merged
7 tasks
@EDsCODE EDsCODE temporarily deployed to posthog-pr-4851 June 25, 2021 01:53 Inactive
@EDsCODE EDsCODE temporarily deployed to posthog-pr-4851 June 25, 2021 01:54 Inactive
@neilkakkar
Handle multiple same events in the funnel (#4863)
716e8af 
* dedup + tests

* deep equality. Tests to come

* write test for entity equality

* finish testing funnels

* clean up comments
@timgl timgl temporarily deployed to posthog-pr-4851 June 25, 2021 14:27 Inactive
@EDsCODE EDsCODE mentioned this pull request Jun 25, 2021
Merged
7 tasks
@EDsCODE EDsCODE marked this pull request as ready for review June 28, 2021 14:12
neilkakkar
neilkakkar approved these changes Jun 28, 2021
View reviewed changes
Copy link
Contributor

@neilkakkar neilkakkar left a comment

Choose a reason for hiding this comment

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

Think this is good enough for now!

@EDsCODE EDsCODE merged commit 41aa793 into master Jun 28, 2021
@EDsCODE EDsCODE deleted the funnel-step-query-new branch June 28, 2021 14:20
@neilkakkar neilkakkar mentioned this pull request Jun 30, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neilkakkar neilkakkar neilkakkar 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.
4 participants
@EDsCODE @neilkakkar @timgl @buwilliams