Handle multiple same events in the funnel #4863

neilkakkar · 2021-06-24T11:34:49Z

Changes

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.

EDsCODE · 2021-06-24T14:24:33Z

ee/clickhouse/queries/funnels/funnel.py

@@ -69,8 +69,11 @@ def get_partition_cols(self, level_index: int, max_steps: int):
            if i < level_index:
                cols.append(f"latest_{i}")
            else:
+                duplicate_event = 0
+                if i > 0 and self._filter.entities[i].id == self._filter.entities[i - 1].id:


we're going to need a more complex condition for determining duplicates. Technically, if you have $pageview with some list of properties A, B, etc and another step with $pageview and properties X, Y, Z, they don't need to be treated as duplicates because they won't match the same

Ah, interesting!

I also need to write tests with action events. Will add one for this too.

Can you point me to where we parse these properties for the query?

Yup, it's actually right in the funnel base class. Look for _build_step_query and subsequently _build_filters

Perfect, thanks!

neilkakkar · 2021-06-24T16:15:12Z

posthog/models/entity.py

@@ -48,3 +47,26 @@ def to_dict(self) -> Dict[str, Any]:
            "math_property": self.math_property,
            "properties": [prop.to_dict() for prop in self.properties],
        }
+
+    def equals(self, other) -> bool:


Would appreciate a look here: unsure if there should be anything else inside here, or if this is too much, for two entities to be considered equal

I think this can work. Math checks are unnecessary iirc. Funnels shouldn't need to use any of those math properties they should just be counting persons going through the funnel. Otherwise, adding some tests and this should be good

EDsCODE

lgtm

* wip: pagination for persons on clickhouse funnels * wip: added offset support for getting a list of persons; added support for conversion window; * fixed mypy exception * helper function to insert data for local testing * moved generate code into separate class for more functionality later * corrected person_distinct_id to use the person id from postgres * minor corrections to generate local class along with addition of data cleanup via destroy() method * reduce the number of persons who make it to each step * moved funnel queries to a new folder for better organization; separated funnel_persons and funnel_trends_persons into individual classes; * funnel persons and tests * initial implementation * invoke the funnel or funnel trends class respectively * add a test * add breakdown handling and first test * add test stubs * remove repeats * mypy corrections and PR feedback * run funnel test suite on new query implementation * remove imports * corrected tests * minor test updates * correct func name * fix types * func name change * move builder functions to funnel base * add test classe for new funnel * Handle multiple same events in the funnel (#4863) * dedup + tests * deep equality. Tests to come * write test for entity equality * finish testing funnels * clean up comments Co-authored-by: Buddy Williams <buddy@posthog.com> Co-authored-by: Neil Kakkar <neilkakkar@gmail.com>

* wip: pagination for persons on clickhouse funnels * wip: added offset support for getting a list of persons; added support for conversion window; * fixed mypy exception * helper function to insert data for local testing * moved generate code into separate class for more functionality later * corrected person_distinct_id to use the person id from postgres * minor corrections to generate local class along with addition of data cleanup via destroy() method * reduce the number of persons who make it to each step * moved funnel queries to a new folder for better organization; separated funnel_persons and funnel_trends_persons into individual classes; * funnel persons and tests * initial implementation * invoke the funnel or funnel trends class respectively * add a test * add breakdown handling and first test * add test stubs * remove repeats * mypy corrections and PR feedback * run funnel test suite on new query implementation * remove imports * corrected tests * minor test updates * correct func name * fix types * func name change * move builder functions to funnel base * add test classe for new funnel * Handle multiple same events in the funnel (#4863) * dedup + tests * deep equality. Tests to come * write test for entity equality * finish testing funnels * clean up comments * add ability to specify per step or dropoff persons * remove defaults * remove funnel_window parameter unless it's needed * add param to filters * test api * remove print * fix tests * change distribution * add none condition for funnel step * add order by * remove funnel window days Co-authored-by: Buddy Williams <buddy@posthog.com> Co-authored-by: Neil Kakkar <neilkakkar@gmail.com>

* wip: pagination for persons on clickhouse funnels * wip: added offset support for getting a list of persons; added support for conversion window; * fixed mypy exception * helper function to insert data for local testing * moved generate code into separate class for more functionality later * corrected person_distinct_id to use the person id from postgres * minor corrections to generate local class along with addition of data cleanup via destroy() method * reduce the number of persons who make it to each step * moved funnel queries to a new folder for better organization; separated funnel_persons and funnel_trends_persons into individual classes; * funnel persons and tests * initial implementation * invoke the funnel or funnel trends class respectively * add a test * add breakdown handling and first test * add test stubs * remove repeats * mypy corrections and PR feedback * run funnel test suite on new query implementation * remove imports * corrected tests * minor test updates * correct func name * fix types * unordered step and test * func name change * move builder functions to funnel base * add test classe for new funnel * resolve issues with unordered funnel * oops * remove breakdown, fix mypy error * Handle multiple same events in the funnel (#4863) * dedup + tests * deep equality. Tests to come * write test for entity equality * finish testing funnels * clean up comments * from O(2^N) to O(N) * add query intuition blurb * rm todo * address comments Co-authored-by: Buddy Williams <buddy@posthog.com> Co-authored-by: eric <eeoneric@gmail.com>

* wip: pagination for persons on clickhouse funnels * wip: added offset support for getting a list of persons; added support for conversion window; * fixed mypy exception * helper function to insert data for local testing * moved generate code into separate class for more functionality later * corrected person_distinct_id to use the person id from postgres * minor corrections to generate local class along with addition of data cleanup via destroy() method * reduce the number of persons who make it to each step * moved funnel queries to a new folder for better organization; separated funnel_persons and funnel_trends_persons into individual classes; * funnel persons and tests * initial implementation * invoke the funnel or funnel trends class respectively * add a test * add breakdown handling and first test * add test stubs * remove repeats * mypy corrections and PR feedback * run funnel test suite on new query implementation * remove imports * corrected tests * minor test updates * correct func name * fix types * unordered step and test * func name change * move builder functions to funnel base * add test classe for new funnel * resolve issues with unordered funnel * oops * remove breakdown, fix mypy error * Handle multiple same events in the funnel (#4863) * dedup + tests * deep equality. Tests to come * write test for entity equality * finish testing funnels * clean up comments * from O(2^N) to O(N) * add query intuition blurb * rm todo * wip persons * wip persons 2 * address comments * test things, fix bugs * match result format to funnel.py Co-authored-by: Buddy Williams <buddy@posthog.com> Co-authored-by: eric <eeoneric@gmail.com>

* wip: pagination for persons on clickhouse funnels * wip: added offset support for getting a list of persons; added support for conversion window; * fixed mypy exception * helper function to insert data for local testing * moved generate code into separate class for more functionality later * corrected person_distinct_id to use the person id from postgres * minor corrections to generate local class along with addition of data cleanup via destroy() method * reduce the number of persons who make it to each step * moved funnel queries to a new folder for better organization; separated funnel_persons and funnel_trends_persons into individual classes; * funnel persons and tests * initial implementation * invoke the funnel or funnel trends class respectively * add a test * add breakdown handling and first test * add test stubs * remove repeats * mypy corrections and PR feedback * run funnel test suite on new query implementation * remove imports * corrected tests * minor test updates * correct func name * fix types * func name change * move builder functions to funnel base * add test classe for new funnel * Handle multiple same events in the funnel (#4863) * dedup + tests * deep equality. Tests to come * write test for entity equality * finish testing funnels * clean up comments * add strict option for funnels * typing * use new persons pattern * persons for funnel strict ordering Co-authored-by: Buddy Williams <buddy@posthog.com> Co-authored-by: eric <eeoneric@gmail.com>

dedup + tests

905814a

timgl temporarily deployed to posthog-pr-4863 June 24, 2021 11:36 Inactive

EDsCODE reviewed Jun 24, 2021

View reviewed changes

deep equality. Tests to come

8a41f5a

timgl temporarily deployed to posthog-pr-4863 June 24, 2021 15:57 Inactive

fjson dump is better

d64b58d

timgl temporarily deployed to posthog-pr-4863 June 24, 2021 16:00 Inactive

neilkakkar commented Jun 24, 2021

View reviewed changes

neilkakkar added 2 commits June 25, 2021 10:57

write test for entity equality

3d11ab6

finish testing funnels

20170c0

timgl temporarily deployed to posthog-pr-4863 June 25, 2021 11:25 Inactive

merge master, resolve conflicts

0bc8bd8

neilkakkar marked this pull request as ready for review June 25, 2021 11:31

timgl temporarily deployed to posthog-pr-4863 June 25, 2021 11:31 Inactive

EDsCODE approved these changes Jun 25, 2021

View reviewed changes

clean up comments

5819e03

timgl temporarily deployed to posthog-pr-4863 June 25, 2021 14:06 Inactive

neilkakkar merged commit 716e8af into funnel-step-query-new Jun 25, 2021

neilkakkar deleted the funnel-step-dedupe branch June 25, 2021 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle multiple same events in the funnel #4863

Handle multiple same events in the funnel #4863

neilkakkar commented Jun 24, 2021

EDsCODE Jun 24, 2021

neilkakkar Jun 24, 2021

EDsCODE Jun 24, 2021

neilkakkar Jun 24, 2021

neilkakkar Jun 24, 2021

EDsCODE Jun 25, 2021

EDsCODE left a comment

Handle multiple same events in the funnel #4863

Handle multiple same events in the funnel #4863

Conversation

neilkakkar commented Jun 24, 2021

Changes

Checklist

EDsCODE Jun 24, 2021

Choose a reason for hiding this comment

neilkakkar Jun 24, 2021

Choose a reason for hiding this comment

EDsCODE Jun 24, 2021

Choose a reason for hiding this comment

neilkakkar Jun 24, 2021

Choose a reason for hiding this comment

neilkakkar Jun 24, 2021

Choose a reason for hiding this comment

EDsCODE Jun 25, 2021

Choose a reason for hiding this comment

EDsCODE left a comment

Choose a reason for hiding this comment