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

Handle multiple same events in the funnel #4863

Merged
merged 7 commits into from
Jun 25, 2021
Merged

Handle multiple same events in the funnel #4863

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
905814a
dedup + tests
neilkakkar Jun 24, 2021
8a41f5a
deep equality. Tests to come
neilkakkar Jun 24, 2021
d64b58d
fjson dump is better
neilkakkar Jun 24, 2021
3d11ab6
write test for entity equality
neilkakkar Jun 25, 2021
20170c0
finish testing funnels
neilkakkar Jun 25, 2021
0bc8bd8
merge master, resolve conflicts
neilkakkar Jun 25, 2021
5819e03
clean up comments
neilkakkar Jun 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions ee/clickhouse/queries/funnels/funnel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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].equals(self._filter.entities[i - 1]):
duplicate_event = 1
cols.append(
f"min(latest_{i}) over (PARTITION by person_id ORDER BY timestamp DESC ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) latest_{i}"
f"min(latest_{i}) over (PARTITION by person_id ORDER BY timestamp DESC ROWS BETWEEN UNBOUNDED PRECEDING AND {duplicate_event} PRECEDING) latest_{i}"
)
return ", ".join(cols)

Expand Down Expand Up @@ -106,7 +109,7 @@ def _get_sorting_condition(self, curr_index: int, max_steps: int):

conditions: List[str] = []
for i in range(1, curr_index):
conditions.append(f"latest_{i - 1} <= latest_{i }")
conditions.append(f"latest_{i - 1} < latest_{i }")
conditions.append(f"latest_{i} <= latest_0 + INTERVAL {self._filter.funnel_window_days} DAY")

return f"if({' AND '.join(conditions)}, {curr_index}, {self._get_sorting_condition(curr_index - 1, max_steps)})"
Expand Down
Empty file added ee/clickhouse/queries/funnels/funnel_unordered.py
View file
Open in desktop
Empty file.
252 changes: 249 additions & 3 deletions ee/clickhouse/queries/funnels/test/test_funnel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
from uuid import uuid4

from ee.clickhouse.models.event import create_event
from ee.clickhouse.queries.funnels.funnel import ClickhouseFunnel
from ee.clickhouse.queries.funnels.funnel import ClickhouseFunnelNew
from ee.clickhouse.util import ClickhouseTestMixin
from posthog.constants import INSIGHT_FUNNELS
from posthog.models.filters import Filter
from posthog.models.person import Person
from posthog.queries.test.test_funnel import funnel_test_factory

Expand All @@ -22,5 +24,249 @@ def _create_event(**kwargs):
create_event(**kwargs)


class TestFunnel(ClickhouseTestMixin, funnel_test_factory(ClickhouseFunnel, _create_event, _create_person)): # type: ignore
pass
class TestFunnel(ClickhouseTestMixin, funnel_test_factory(ClickhouseFunnelNew, _create_event, _create_person)): # type: ignore
def test_basic_funnel_with_repeat_steps(self):
filters = {
"events": [
{"id": "user signed up", "type": "events", "order": 0},
{"id": "user signed up", "type": "events", "order": 1},
],
"insight": INSIGHT_FUNNELS,
}

filter = Filter(data=filters)
funnel = ClickhouseFunnelNew(filter, self.team)

# event
person1_stopped_after_two_signups = _create_person(distinct_ids=["stopped_after_signup1"], team_id=self.team.pk)
_create_event(team=self.team, event="user signed up", distinct_id="stopped_after_signup1")
_create_event(team=self.team, event="user signed up", distinct_id="stopped_after_signup1")

person2_stopped_after_signup = _create_person(distinct_ids=["stopped_after_signup2"], team_id=self.team.pk)
_create_event(team=self.team, event="user signed up", distinct_id="stopped_after_signup2")

with self.assertNumQueries(1):
result = funnel.run()

self.assertEqual(result[0]["name"], "user signed up")
self.assertEqual(result[0]["count"], 2)
# check ordering of people in first step
self.assertCountEqual(
result[0]["people"], [person1_stopped_after_two_signups.uuid, person2_stopped_after_signup.uuid],
)

self.assertCountEqual(
result[1]["people"], [person1_stopped_after_two_signups.uuid],
)

def test_advanced_funnel_with_repeat_steps(self):
filters = {
"events": [
{"id": "user signed up", "type": "events", "order": 0},
{"id": "$pageview", "type": "events", "order": 1},
{"id": "$pageview", "type": "events", "order": 2},
{"id": "$pageview", "type": "events", "order": 3},
{"id": "$pageview", "type": "events", "order": 4},
],
"insight": INSIGHT_FUNNELS,
}

filter = Filter(data=filters)
funnel = ClickhouseFunnelNew(filter, self.team)

# event
person1_stopped_after_signup = _create_person(distinct_ids=["stopped_after_signup1"], team_id=self.team.pk)
_create_event(team=self.team, event="user signed up", distinct_id="stopped_after_signup1")

person2_stopped_after_one_pageview = _create_person(
distinct_ids=["stopped_after_pageview1"], team_id=self.team.pk
)
_create_event(team=self.team, event="user signed up", distinct_id="stopped_after_pageview1")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview1")

person3_stopped_after_two_pageview = _create_person(
distinct_ids=["stopped_after_pageview2"], team_id=self.team.pk
)
_create_event(team=self.team, event="user signed up", distinct_id="stopped_after_pageview2")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview2")
_create_event(team=self.team, event="blaah blaa", distinct_id="stopped_after_pageview2")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview2")

person4_stopped_after_three_pageview = _create_person(
distinct_ids=["stopped_after_pageview3"], team_id=self.team.pk
)
_create_event(team=self.team, event="user signed up", distinct_id="stopped_after_pageview3")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview3")
_create_event(team=self.team, event="blaah blaa", distinct_id="stopped_after_pageview3")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview3")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview3")
_create_event(team=self.team, event="blaah blaa", distinct_id="stopped_after_pageview3")

person5_stopped_after_many_pageview = _create_person(
distinct_ids=["stopped_after_pageview4"], team_id=self.team.pk
)
_create_event(team=self.team, event="user signed up", distinct_id="stopped_after_pageview4")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview4")
_create_event(team=self.team, event="blaah blaa", distinct_id="stopped_after_pageview4")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview4")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview4")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview4")

with self.assertNumQueries(1):
result = funnel.run()

self.assertEqual(result[0]["name"], "user signed up")
self.assertEqual(result[1]["name"], "$pageview")
self.assertEqual(result[4]["name"], "$pageview")
self.assertEqual(result[0]["count"], 5)
# check ordering of people in every step
self.assertCountEqual(
result[0]["people"],
[
person1_stopped_after_signup.uuid,
person2_stopped_after_one_pageview.uuid,
person3_stopped_after_two_pageview.uuid,
person4_stopped_after_three_pageview.uuid,
person5_stopped_after_many_pageview.uuid,
],
)

self.assertCountEqual(
result[1]["people"],
[
person2_stopped_after_one_pageview.uuid,
person3_stopped_after_two_pageview.uuid,
person4_stopped_after_three_pageview.uuid,
person5_stopped_after_many_pageview.uuid,
],
)

self.assertCountEqual(
result[2]["people"],
[
person3_stopped_after_two_pageview.uuid,
person4_stopped_after_three_pageview.uuid,
person5_stopped_after_many_pageview.uuid,
],
)

self.assertCountEqual(
result[3]["people"], [person4_stopped_after_three_pageview.uuid, person5_stopped_after_many_pageview.uuid],
)

self.assertCountEqual(
result[4]["people"], [person5_stopped_after_many_pageview.uuid],
)

def test_advanced_funnel_with_repeat_steps_out_of_order_events(self):
filters = {
"events": [
{"id": "user signed up", "type": "events", "order": 0},
{"id": "$pageview", "type": "events", "order": 1},
{"id": "$pageview", "type": "events", "order": 2},
{"id": "$pageview", "type": "events", "order": 3},
{"id": "$pageview", "type": "events", "order": 4},
],
"insight": INSIGHT_FUNNELS,
}

filter = Filter(data=filters)
funnel = ClickhouseFunnelNew(filter, self.team)

# event
person1_stopped_after_signup = _create_person(
distinct_ids=["random", "stopped_after_signup1"], team_id=self.team.pk
)
_create_event(team=self.team, event="$pageview", distinct_id="random")
_create_event(team=self.team, event="user signed up", distinct_id="stopped_after_signup1")

person2_stopped_after_one_pageview = _create_person(
distinct_ids=["stopped_after_pageview1"], team_id=self.team.pk
)
_create_event(team=self.team, event="user signed up", distinct_id="stopped_after_pageview1")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview1")

person3_stopped_after_two_pageview = _create_person(
distinct_ids=["stopped_after_pageview2"], team_id=self.team.pk
)
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview2")
_create_event(team=self.team, event="user signed up", distinct_id="stopped_after_pageview2")
_create_event(team=self.team, event="blaah blaa", distinct_id="stopped_after_pageview2")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview2")

person4_stopped_after_three_pageview = _create_person(
distinct_ids=["stopped_after_pageview3"], team_id=self.team.pk
)
_create_event(team=self.team, event="blaah blaa", distinct_id="stopped_after_pageview3")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview3")
_create_event(team=self.team, event="blaah blaa", distinct_id="stopped_after_pageview3")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview3")
_create_event(team=self.team, event="user signed up", distinct_id="stopped_after_pageview3")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview3")

person5_stopped_after_many_pageview = _create_person(
distinct_ids=["stopped_after_pageview4"], team_id=self.team.pk
)
_create_event(team=self.team, event="user signed up", distinct_id="stopped_after_pageview4")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview4")
_create_event(team=self.team, event="blaah blaa", distinct_id="stopped_after_pageview4")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview4")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview4")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview4")

person6_stopped_after_many_pageview_without_signup = _create_person(
distinct_ids=["stopped_after_pageview5"], team_id=self.team.pk
)
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview5")
_create_event(team=self.team, event="blaah blaa", distinct_id="stopped_after_pageview5")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview5")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview5")
_create_event(team=self.team, event="$pageview", distinct_id="stopped_after_pageview5")

with self.assertNumQueries(1):
result = funnel.run()

self.assertEqual(result[0]["name"], "user signed up")
self.assertEqual(result[1]["name"], "$pageview")
self.assertEqual(result[4]["name"], "$pageview")
self.assertEqual(result[0]["count"], 5)
# check ordering of people in every step
self.assertCountEqual(
result[0]["people"],
[
person1_stopped_after_signup.uuid,
person2_stopped_after_one_pageview.uuid,
person3_stopped_after_two_pageview.uuid,
person4_stopped_after_three_pageview.uuid,
person5_stopped_after_many_pageview.uuid,
],
)

self.assertCountEqual(
result[1]["people"],
[
person2_stopped_after_one_pageview.uuid,
person3_stopped_after_two_pageview.uuid,
person4_stopped_after_three_pageview.uuid,
person5_stopped_after_many_pageview.uuid,
],
)

self.assertCountEqual(
result[2]["people"], [person5_stopped_after_many_pageview.uuid],
)

self.assertCountEqual(
result[3]["people"], [person5_stopped_after_many_pageview.uuid],
)

self.assertCountEqual(
result[4]["people"], [person5_stopped_after_many_pageview.uuid],
)

def test_rest(self):
pass
# test: with actions
# test: with properties
# separate tests for testing the equals functionality on Entity
# Maybe better to just use __eq__ + __hash__ ?
26 changes: 24 additions & 2 deletions posthog/models/entity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import json
from typing import Any, Dict, List, Optional, Union

from posthog.constants import TREND_FILTER_TYPE_ACTIONS, TREND_FILTER_TYPE_EVENTS
from posthog.models.filters.mixins.property import PropertyMixin

from .property import Property


class Entity(PropertyMixin):
"""
Expand Down Expand Up @@ -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:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

""" Checks if two entities are semantically equal."""
# Not using __eq__ since that affects hashability

if self.id != other.id:
return False

if self.type != other.type:
return False

if self.math != other.math:
return False

if self.math_property != other.math_property:
return False

if json.dumps(sorted(self.properties_to_dict().items())) != json.dumps(
sorted(other.properties_to_dict().items())
):
return False

return True