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

feat(hogql): sequential steps funnel #19951

Merged
merged 90 commits into from
Feb 7, 2024
Merged

feat(hogql): sequential steps funnel #19951

merged 90 commits into from
Feb 7, 2024

Conversation

thmsobrmlr
Copy link
Contributor

@thmsobrmlr thmsobrmlr commented Jan 24, 2024

Problem

Funnels need to be converted to HogQL (see the funnels section in PostHog/meta#130).

Changes

This PR:

  • Implements all query steps necessary for a sequential (ordered) steps funnel without breakdowns and without converted/dropped people urls.
    • I tried to follow the existing implementation as closely as possible, since funnels span a large surface area.
    • Therefore I also left in a bunch of commented out lines to guide me when implementing the missing features.
    • These comments and the added TODOs grep TODO -rn posthog/hogql_queries/insights/funnels should be gone once the funnels rewrite is complete.
  • Duplicates existing funnels tests for the HogQL variant:
    • posthog/queries/funnels/test/test_funnel.py
    • posthog/queries/funnels/test/conversion_time_cases.py
    • ee/clickhouse/queries/funnels/test/test_funnel.py (merged in with the regular tests)
    • I have removed 3 tests relating to single step funnels (those are not supported in the UI, so I haven't implemented them) and commentes out a test relating to validation errors (test_funnel_exclusions_invalid_params, to be implemented) and a failing cohort test (test_funnel_with_cohorts_step_filter, to be fixed).

Follow-ups

  •  test_funnel_exclusions_invalid_params: implement validations (with Pydantic?)
  • test_funnel_with_cohorts_step_filter: investigate this issue
  • clean up all TODOs and commented out lines of code

How did you test this code?

I adapted the test script here https://gist.github.com/thmsobrmlr/ece8131eb7272893c274b3d9e77fc65c. It runs on most of my local insights, except some with validation errors and in one insight the conversion times don't match (to be investigated).

@thmsobrmlr thmsobrmlr changed the base branch from funnel-events-query to master January 29, 2024 13:19
@PostHog PostHog deleted a comment from github-actions bot Jan 30, 2024
posthog/hogql/visitor.py Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Feb 1, 2024

Size Change: +85 B (0%)

Total Size: 2.22 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 2.22 MB +85 B (0%)

compressed-size-action

@thmsobrmlr thmsobrmlr mentioned this pull request Feb 6, 2024
48 tasks
@thmsobrmlr thmsobrmlr marked this pull request as ready for review February 6, 2024 11:36
@thmsobrmlr thmsobrmlr changed the title feat(hogql): funnel ordered steps feat(hogql): sequential steps funnel Feb 6, 2024
@thmsobrmlr thmsobrmlr requested a review from a team February 6, 2024 11:59
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Wow, that's a lot of code. Reading through it all, it looks good, though obviously I didn't take the time to do a very deep review. Didn't spot anything odd either. Great work. 👍

@thmsobrmlr thmsobrmlr merged commit cbf09b5 into master Feb 7, 2024
85 checks passed
@thmsobrmlr thmsobrmlr deleted the funnel-ordered-steps branch February 7, 2024 08:33
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.

2 participants