Skip to content

Commit

Permalink
Make Breakdown limit customizable and Allow empty breakdown value in …
Browse files Browse the repository at this point in the history 
…trends and funnels (#5357)

* breakdown limits update

* include null in limit

* clean breakdowns for trends & peeps
  • Loading branch information
@neilkakkar
neilkakkar authored Aug 3, 2021
1 parent 37753fe commit a5d9953
Show file tree
Hide file tree
Showing 20 changed files with 367 additions and 122 deletions.
7 changes: 3 additions & 4 deletions ee/clickhouse/queries/funnels/base.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,7 @@ def _get_inner_event_query(
extra_join = self._get_cohort_breakdown_join()
else:
breakdown_conditions = self._get_breakdown_conditions()
extra_conditions = "AND prop != ''" if select_prop else ""
extra_conditions += f"AND {breakdown_conditions}" if breakdown_conditions and select_prop else ""
extra_conditions = f" AND {breakdown_conditions}" if breakdown_conditions and select_prop else ""

return FUNNEL_INNER_EVENT_STEPS_QUERY.format(
steps=steps,
Expand Down Expand Up @@ -309,7 +308,7 @@ def _get_funnel_person_step_condition(self):
self.params.update({"step_num": abs(step_num) - 1})
conditions.append("steps = %(step_num)s")

if self._filter.funnel_step_breakdown:
if self._filter.funnel_step_breakdown is not None:
prop_vals = self._parse_breakdown_prop_value()
self.params.update({"breakdown_prop_value": prop_vals})
conditions.append("prop IN %(breakdown_prop_value)s")
Expand Down Expand Up @@ -402,7 +401,7 @@ def _get_breakdown_conditions(self) -> str:
if self._filter.funnel_step_breakdown:
values = self._parse_breakdown_prop_value()
else:
limit = 5
limit = self._filter.breakdown_limit or 5
first_entity = next(x for x in self._filter.entities if x.order == 0)
if not first_entity:
ValidationError("An entity with order 0 was not provided")
Expand Down
148 changes: 148 additions & 0 deletions ee/clickhouse/queries/funnels/test/breakdown_cases.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,154 @@ def test_funnel_step_breakdown_limit(self):
breakdown_vals = sorted([res[0]["breakdown"] for res in result])
self.assertEqual(["5", "6", "7", "8", "9"], breakdown_vals)

def test_funnel_step_custom_breakdown_limit_with_nulls(self):

filters = {
"events": [{"id": "sign up", "order": 0}, {"id": "play movie", "order": 1}, {"id": "buy", "order": 2},],
"insight": INSIGHT_FUNNELS,
"date_from": "2020-01-01",
"date_to": "2020-01-08",
"funnel_window_days": 7,
"breakdown_type": "event",
"breakdown_limit": 3,
"breakdown": "some_breakdown_val",
}

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

for num in range(5):
for i in range(num):
_create_person(distinct_ids=[f"person_{num}_{i}"], team_id=self.team.pk)
_create_event(
team=self.team,
event="sign up",
distinct_id=f"person_{num}_{i}",
properties={"key": "val", "some_breakdown_val": num},
timestamp="2020-01-01T12:00:00Z",
)
_create_event(
team=self.team,
event="play movie",
distinct_id=f"person_{num}_{i}",
properties={"key": "val", "some_breakdown_val": num},
timestamp="2020-01-01T13:00:00Z",
)
_create_event(
team=self.team,
event="buy",
distinct_id=f"person_{num}_{i}",
properties={"key": "val", "some_breakdown_val": num},
timestamp="2020-01-01T15:00:00Z",
)

# no breakdown value for this guy
_create_person(distinct_ids=[f"person_null"], team_id=self.team.pk)
_create_event(
team=self.team,
event="sign up",
distinct_id=f"person_null",
properties={"key": "val"},
timestamp="2020-01-01T12:00:00Z",
)
_create_event(
team=self.team,
event="play movie",
distinct_id=f"person_null",
properties={"key": "val"},
timestamp="2020-01-01T13:00:00Z",
)
_create_event(
team=self.team,
event="buy",
distinct_id=f"person_null",
properties={"key": "val"},
timestamp="2020-01-01T15:00:00Z",
)

result = funnel.run()

breakdown_vals = sorted([res[0]["breakdown"] for res in result])
self.assertEqual(["2", "3", "4"], breakdown_vals)
# skipped 1 and '' because the limit was 3.

def test_funnel_step_custom_breakdown_limit_with_nulls_included(self):

filters = {
"events": [{"id": "sign up", "order": 0}, {"id": "play movie", "order": 1}, {"id": "buy", "order": 2},],
"insight": INSIGHT_FUNNELS,
"date_from": "2020-01-01",
"date_to": "2020-01-08",
"funnel_window_days": 7,
"breakdown_type": "event",
"breakdown_limit": 6,
"breakdown": "some_breakdown_val",
}

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

for num in range(5):
for i in range(num):
_create_person(distinct_ids=[f"person_{num}_{i}"], team_id=self.team.pk)
_create_event(
team=self.team,
event="sign up",
distinct_id=f"person_{num}_{i}",
properties={"key": "val", "some_breakdown_val": num},
timestamp="2020-01-01T12:00:00Z",
)
_create_event(
team=self.team,
event="play movie",
distinct_id=f"person_{num}_{i}",
properties={"key": "val", "some_breakdown_val": num},
timestamp="2020-01-01T13:00:00Z",
)
_create_event(
team=self.team,
event="buy",
distinct_id=f"person_{num}_{i}",
properties={"key": "val", "some_breakdown_val": num},
timestamp="2020-01-01T15:00:00Z",
)

# no breakdown value for this guy
p_null = _create_person(distinct_ids=[f"person_null"], team_id=self.team.pk)
_create_event(
team=self.team,
event="sign up",
distinct_id=f"person_null",
properties={"key": "val"},
timestamp="2020-01-01T12:00:00Z",
)
_create_event(
team=self.team,
event="play movie",
distinct_id=f"person_null",
properties={"key": "val"},
timestamp="2020-01-01T13:00:00Z",
)
_create_event(
team=self.team,
event="buy",
distinct_id=f"person_null",
properties={"key": "val"},
timestamp="2020-01-01T15:00:00Z",
)

result = funnel.run()

breakdown_vals = sorted([res[0]["breakdown"] for res in result])
self.assertEqual(["", "1", "2", "3", "4"], breakdown_vals)
# included 1 and '' because the limit was 6.

for i in range(1, 5):
self.assertEqual(len(self._get_people_at_step(filter, 3, str(i))), i)

self.assertEqual([p_null.uuid], self._get_people_at_step(filter, 1, ""))
self.assertEqual([p_null.uuid], self._get_people_at_step(filter, 3, ""))

def test_funnel_step_breakdown_event_single_person_multiple_breakdowns(self):

filters = {
Expand Down
38 changes: 18 additions & 20 deletions ee/clickhouse/queries/test/test_trends.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from posthog.models.filters import Filter
from posthog.models.person import Person
from posthog.queries.test.test_trends import trend_test_factory
from posthog.settings import SHELL_PLUS_PRINT_SQL


def _create_action(**kwargs):
Expand Down Expand Up @@ -60,8 +61,9 @@ def test_breakdown_with_filter(self):
),
self.team,
)
self.assertEqual(len(response), 2)
self.assertEqual(response[1]["breakdown_value"], "val")
self.assertEqual(len(response), 1)
# don't return none option when empty
self.assertEqual(response[0]["breakdown_value"], "val")

def test_breakdown_filtering_limit(self):
self._create_breakdown_events()
Expand All @@ -76,7 +78,7 @@ def test_breakdown_filtering_limit(self):
),
self.team,
)
self.assertEqual(len(response), 26) # We fetch 25 to see if there are more ethan 20 values
self.assertEqual(len(response), 25) # We fetch 25 to see if there are more ethan 20 values

def test_breakdown_by_person_property(self):
person1, person2, person3, person4 = self._create_multiple_people()
Expand Down Expand Up @@ -107,8 +109,7 @@ def test_breakdown_by_person_property(self):
)

self.assertListEqual(
sorted([res["breakdown_value"] for res in event_response]),
sorted(["none", "person1", "person2", "person3"]),
sorted([res["breakdown_value"] for res in event_response]), sorted(["person1", "person2", "person3"]),
)

for response in event_response:
Expand Down Expand Up @@ -263,17 +264,13 @@ def test_breakdown_filtering_with_properties(self):

response = sorted(response, key=lambda x: x["label"])
self.assertEqual(response[0]["label"], "sign up - first url")
self.assertEqual(response[1]["label"], "sign up - none")
self.assertEqual(response[2]["label"], "sign up - second url")
self.assertEqual(response[1]["label"], "sign up - second url")

self.assertEqual(sum(response[0]["data"]), 1)
self.assertEqual(response[0]["breakdown_value"], "first url")

self.assertEqual(sum(response[1]["data"]), 0)
self.assertEqual(response[1]["breakdown_value"], "none")

self.assertEqual(sum(response[2]["data"]), 1)
self.assertEqual(response[2]["breakdown_value"], "second url")
self.assertEqual(sum(response[1]["data"]), 1)
self.assertEqual(response[1]["breakdown_value"], "second url")

def test_dau_with_breakdown_filtering(self):
sign_up_action, _ = self._create_events()
Expand Down Expand Up @@ -331,10 +328,10 @@ def test_dau_with_breakdown_filtering_with_prop_filter(self):
self.team,
)

self.assertEqual(event_response[1]["label"], "sign up - other_value")
self.assertEqual(event_response[0]["label"], "sign up - other_value")

self.assertEqual(sum(event_response[1]["data"]), 1)
self.assertEqual(event_response[1]["data"][5], 1) # property not defined
self.assertEqual(sum(event_response[0]["data"]), 1)
self.assertEqual(event_response[0]["data"][5], 1) # property not defined

self.assertEntityResponseEqual(action_response, event_response)

Expand Down Expand Up @@ -412,8 +409,8 @@ def test_breakdown_user_props_with_filter(self):
),
self.team,
)
self.assertEqual(len(response), 2)
self.assertEqual(response[1]["breakdown_value"], "test@gmail.com")
self.assertEqual(len(response), 1)
self.assertEqual(response[0]["breakdown_value"], "test@gmail.com")

def _create_active_user_events(self):
p0 = Person.objects.create(team_id=self.team.pk, distinct_ids=["p0"], properties={"name": "p1"})
Expand Down Expand Up @@ -541,7 +538,7 @@ def test_breakdown_active_user_math(self):

filter = Filter(data=data)
result = ClickhouseTrends().run(filter, self.team,)
self.assertEqual(result[1]["data"], [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 2.0, 2.0, 2.0, 0.0])
self.assertEqual(result[0]["data"], [0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 2.0, 2.0, 2.0, 0.0])

def test_filter_test_accounts(self):
p1 = Person.objects.create(team_id=self.team.pk, distinct_ids=["p1"], properties={"name": "p1"})
Expand Down Expand Up @@ -577,7 +574,7 @@ def test_filter_test_accounts(self):
result = ClickhouseTrends().run(filter_2, self.team,)
self.assertEqual(result[0]["count"], 2)
result = ClickhouseTrends().run(filter_3, self.team,)
self.assertEqual(result[1]["count"], 1)
self.assertEqual(result[0]["count"], 1)

def test_breakdown_filtering_bar_chart_by_value(self):
self._create_events()
Expand All @@ -596,8 +593,9 @@ def test_breakdown_filtering_bar_chart_by_value(self):
self.team,
)

self.assertEqual(response[0]["aggregated_value"], 1)
self.assertEqual(response[0]["aggregated_value"], 2) # the events without breakdown value
self.assertEqual(response[1]["aggregated_value"], 1)
self.assertEqual(response[2]["aggregated_value"], 1)
self.assertEqual(
response[0]["days"],
[
Expand Down
Loading

0 comments on commit a5d9953

Please sign in to comment.