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

chore(hogql): add comparision task for hogql funnels insights #20780

Closed
wants to merge 20 commits into from

Conversation

thmsobrmlr
Copy link
Contributor

@thmsobrmlr thmsobrmlr commented Mar 8, 2024

Problem

We've converted funnel insights to HogQL and want to make sure we're not introducing regressions.

Changes

This PR contains a Django script for comparing the legacy version of the insight to the HogQL one. It also serves as a checklist for these tests and todo list for things to fix.

python manage.py shell_plus --command compare_hogql_funnel_insights
python manage.py compare_hogql_funnel_insights

Checklist

  • adapt compare script for breakdowns
  • len(results) should always be equal
steps time_to_convert trends
ordered - [ ] Done - [ ] Done - [ ] Done
strict - [ ] Done - [n] Done - [n] Done
unordered - [ ] Done - [n] Done - [ ] Done

Todos

  • implement validation errors (follow-ups of first pr)
  •  test_funnel_exclusions_invalid_params: implement validations (with Pydantic?)
  •  frontend: correlation details background
  • frontend: correlation dark mode
  • make "view recording" work
  • make wrapping sql from actors work with funnels test(hogql): replace actors class in funnel tests #20510
  •  (a) It would be great to keep changed math types when switching between insights.
  •  (b) Ideally we would have differently typed entities for trends, stickiness and other insights.
  •  test_funnel_with_cohorts_step_filter: investigate this issue
  •  clean up all TODOs and commented out lines of code
  • commented out materialized columns for funnel correlations
  • commented out actors test in test_correlation_with_multiple_properties
  • clean up AND event.team_id = {self.context.team.pk}
  • settings= in funnel_trends_persons, search all SETTINGS
  • cache invalidation for actors
  • InsightActorsQuery messy, should we have different keys for funnel types?! e.g. actorsFilter or funnelActorsFilter
    procedural and class based code e.g. to_actors_query takes parameters, but in funnels all subclasses methods access these method, so it can't be passed down
    include recordings in trends and stickiness?
  • Convert missing actions into Validation errors
  • add QUERY_TYPE = "funnel_unordered" to hogql implementation
  • fix this https://posthog.slack.com/archives/C0368RPHLQH/p1708519310821229

How did you test this code?

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@mariusandra
Copy link
Collaborator

I guess we moved to a different comparison system and this can be closed? HogQL funnels are live after all.

@mariusandra mariusandra deleted the compare-hogql-funnels branch April 17, 2024 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants