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

Group reamining breakdown values into "Other" for funnels #5538

Merged
merged 7 commits into from
Aug 12, 2021
Merged

Group reamining breakdown values into "Other" for funnels #5538

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d6dc76a
breakdown grouping
neilkakkar Aug 11, 2021
b8baecb
address comment + update for all types of funnels
neilkakkar Aug 12, 2021
903f3d2
resolve pagination woes
neilkakkar Aug 12, 2021
4c4b5c2
Merge branch 'master' of github.com:PostHog/posthog into breakdown_gr…
neilkakkar Aug 12, 2021
393a7ad
add breakdown_value
neilkakkar Aug 12, 2021
7a55959
explain the offset override
neilkakkar Aug 12, 2021
18a2511
document assumption
neilkakkar Aug 12, 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
34 changes: 14 additions & 20 deletions ee/clickhouse/queries/funnels/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,30 +397,24 @@ def _get_cohort_breakdown_join(self) -> str:

def _get_breakdown_conditions(self) -> str:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this pattern is turning out to be a bit icky, where this function modifies state (self.params) and returns empty string.

Will leave refactoring to a follow up PR - it might make sense to do it for the rest of the functions like this one.

if self._filter.breakdown:
if self._filter.funnel_step_breakdown:
values = self._parse_breakdown_prop_value()
else:
limit = self._filter.breakdown_limit_or_default
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")
values = []
if self._filter.breakdown_type == "person":
values = get_breakdown_person_prop_values(
self._filter, first_entity, "count(*)", self._team.pk, limit
)
elif self._filter.breakdown_type == "event":
values = get_breakdown_event_prop_values(
self._filter, first_entity, "count(*)", self._team.pk, limit
)
limit = self._filter.breakdown_limit_or_default
first_entity = next(x for x in self._filter.entities if x.order == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Code issue:

This doesn't work as you'd like:

In [2]: next(iter([]))
---------------------------------------------------------------------------
StopIteration                             Traceback (most recent call last)
<ipython-input-2-bfed92c5b1cf> in <module>
----> 1 next(iter([]))

StopIteration:

Given that entities are sorted already entities[0] does the trick

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! I guess we never hit this earlier because we have a check that entities exist. I'm just going to get rid of the validation error completely!

if not first_entity:
ValidationError("An entity with order 0 was not provided")
values = []
if self._filter.breakdown_type == "person":
values = get_breakdown_person_prop_values(self._filter, first_entity, "count(*)", self._team.pk, limit)
elif self._filter.breakdown_type == "event":
values = get_breakdown_event_prop_values(self._filter, first_entity, "count(*)", self._team.pk, limit)
Copy link
Contributor

Choose a reason for hiding this comment

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

So what happens if the second entity has a different set of person property values? Is it impossible due to how funnels work?

If so, worth adding a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by different set of person property values for second entity? Maybe an example?

If you mean something like: breakdown by X, and second entity has a filter like: X=a, where a is very rare, then earlier, yes, this would mean that drop off is 100%. After this PR, they'd all get grouped into "Other".

Copy link
Contributor

Choose a reason for hiding this comment

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

So entities in this case are things like:

  1. { type: event, value: 'fooevent' }
  2. { type: event, value: 'barevent' }

Let's walk through a scenario
If you break down first with $some_property you get values A, B. If you broke the second with $some_property you would get B and C

Would the table contain A and B only or would C get included as well or is this scenario impossible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Earlier:
If possible, table would contain A, B only.

Now:
if possible, table will contain A, B, Other

Judging possibility:
Hmm, one case where I can imagine this being possible is if second never happened with A value of $some_property and first never happened with C value of $some_property. Unlikely, but possible. It smells of event-dependent values of $some_property though, which doesn't seem right. Like, it implies the user in a single session did first and then second event, but the properties changed in the middle.

The idea with choosing the first event: anything that occurs for the Nth event but not the first event wouldn't make it inside the funnel, since the first event needs to happen ... first.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hope you see now why this needs at minimum a clarifying comment - there's a ton to unpack here.

Why Other not C? 3 values will fall well below the line of 5 or 10. :)

Given you can break down by event properties, it's more than possible that the second step in the funnel has a different set of properties than the first. It's kind of how event properties work - they don't stick around for all events for a session.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see what you mean now, "Why other not C" is a very valid question!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh wait, actually, in the funnel, all event properties are constrained to the first in line, so even with other grouping, the breakdown values would only be A, B, no other. (will write a test to confirm). My bad here, I got confused: the query never does anything with props for events following the first one.

I'll chalk this one up to a funnel limitation. With event props, makes sense to extend this like you say.

@EDsCODE can confirm?


self.params.update({"breakdown_values": values})
return "prop IN %(breakdown_values)s"
else:
return ""

def _get_breakdown_prop(self) -> str:
def _get_breakdown_prop(self, group_remaining=False) -> str:
if self._filter.breakdown:
return ", prop"
if group_remaining and self._filter.breakdown_type != "cohort":
return ", if(has(%(breakdown_values)s, prop), prop, 'Other') as prop"
else:
return ", prop"
else:
return ""
6 changes: 4 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 @@ -111,13 +111,15 @@ def get_step_counts_without_aggregation_query(self):
max_steps = len(self._filter.entities)
if max_steps >= 2:
formatted_query = self.build_step_subquery(2, max_steps)
breakdown_query = self._get_breakdown_prop()
else:
formatted_query = self._get_inner_event_query()
breakdown_query = self._get_breakdown_prop(group_remaining=True)

exclusion_clause = self._get_exclusion_condition()

return f"""
SELECT *, {self._get_sorting_condition(max_steps, max_steps)} AS steps {exclusion_clause} {self._get_step_times(max_steps)} {self._get_breakdown_prop()} FROM (
SELECT *, {self._get_sorting_condition(max_steps, max_steps)} AS steps {exclusion_clause} {self._get_step_times(max_steps)} {breakdown_query} FROM (
{formatted_query}
) WHERE step_0 = 1
{'AND exclusion = 0' if exclusion_clause else ''}
Expand Down Expand Up @@ -165,7 +167,7 @@ def build_step_subquery(self, level_index: int, max_steps: int):
person_id,
timestamp,
{self._get_partition_cols(1, max_steps)}
{self._get_breakdown_prop()}
{self._get_breakdown_prop(group_remaining=True)}
FROM ({self._get_inner_event_query()})
"""
else:
Expand Down
157 changes: 154 additions & 3 deletions ee/clickhouse/queries/funnels/test/breakdown_cases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,156 @@ def test_funnel_step_breakdown_event(self):
self.assertCountEqual(self._get_people_at_step(filter, 1, "Safari"), [person2.uuid, person3.uuid])
self.assertCountEqual(self._get_people_at_step(filter, 2, "Safari"), [person2.uuid])

def test_funnel_step_breakdown_event_with_other(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": "$browser",
"breakdown_limit": 1,
}

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

# event
person1 = _create_person(distinct_ids=["person1"], team_id=self.team.pk)
_create_event(
team=self.team,
event="sign up",
distinct_id="person1",
properties={"key": "val", "$browser": "Chrome"},
timestamp="2020-01-01T12:00:00Z",
)
_create_event(
team=self.team,
event="play movie",
distinct_id="person1",
properties={"key": "val", "$browser": "Chrome"},
timestamp="2020-01-01T13:00:00Z",
)
_create_event(
team=self.team,
event="buy",
distinct_id="person1",
properties={"key": "val", "$browser": "Chrome"},
timestamp="2020-01-01T15:00:00Z",
)

person2 = _create_person(distinct_ids=["person2"], team_id=self.team.pk)
_create_event(
team=self.team,
event="sign up",
distinct_id="person2",
properties={"key": "val", "$browser": "Safari"},
timestamp="2020-01-02T14:00:00Z",
)
_create_event(
team=self.team,
event="play movie",
distinct_id="person2",
properties={"key": "val", "$browser": "Safari"},
timestamp="2020-01-02T16:00:00Z",
)

person3 = _create_person(distinct_ids=["person3"], team_id=self.team.pk)
_create_event(
team=self.team,
event="sign up",
distinct_id="person3",
properties={"key": "val", "$browser": "Safari"},
timestamp="2020-01-02T14:00:00Z",
)

result = funnel.run()
self.assertEqual(
result[1],
[
{
"action_id": "sign up",
"name": "sign up",
"order": 0,
"people": [person1.uuid] if Funnel == ClickhouseFunnel else [], # backwards compatibility
"count": 1,
"type": "events",
"average_conversion_time": None,
"median_conversion_time": None,
"breakdown": "Chrome",
},
{
"action_id": "play movie",
"name": "play movie",
"order": 1,
"people": [person1.uuid] if Funnel == ClickhouseFunnel else [], # backwards compatibility
"count": 1,
"type": "events",
"average_conversion_time": 3600.0,
"median_conversion_time": 3600.0,
"breakdown": "Chrome",
},
{
"action_id": "buy",
"name": "buy",
"order": 2,
"people": [person1.uuid] if Funnel == ClickhouseFunnel else [], # backwards compatibility
"count": 1,
"type": "events",
"average_conversion_time": 7200.0,
"median_conversion_time": 7200.0,
"breakdown": "Chrome",
},
],
)
self.assertCountEqual(self._get_people_at_step(filter, 1, "Chrome"), [person1.uuid])
self.assertCountEqual(self._get_people_at_step(filter, 2, "Chrome"), [person1.uuid])
self.assertEqual(
result[0],
[
{
"action_id": "sign up",
"name": "sign up",
"order": 0,
"people": [person2.uuid, person3.uuid]
if Funnel == ClickhouseFunnel
else [], # backwards compatibility
"count": 2,
"type": "events",
"average_conversion_time": None,
"median_conversion_time": None,
"breakdown": "Other",
},
{
"action_id": "play movie",
"name": "play movie",
"order": 1,
"people": [person2.uuid] if Funnel == ClickhouseFunnel else [], # backwards compatibility
"count": 1,
"type": "events",
"average_conversion_time": 7200.0,
"median_conversion_time": 7200.0,
"breakdown": "Other",
},
{
"action_id": "buy",
"name": "buy",
"order": 2,
"people": [],
"count": 0,
"type": "events",
"average_conversion_time": None,
"median_conversion_time": None,
"breakdown": "Other",
},
],
)

self.assertCountEqual(self._get_people_at_step(filter, 1, "Other"), [person2.uuid, person3.uuid])
self.assertCountEqual(self._get_people_at_step(filter, 2, "Other"), [person2.uuid])

def test_funnel_step_breakdown_event_no_type(self):

filters = {
Expand Down Expand Up @@ -492,7 +642,7 @@ def test_funnel_step_breakdown_limit(self):

# assert that we give 5 at a time at most and that those values are the most popular ones
breakdown_vals = sorted([res[0]["breakdown"] for res in result])
self.assertEqual(["5", "6", "7", "8", "9"], breakdown_vals)
self.assertEqual(["5", "6", "7", "8", "9", "Other"], breakdown_vals)

def test_funnel_step_custom_breakdown_limit_with_nulls(self):

Expand Down Expand Up @@ -536,7 +686,7 @@ def test_funnel_step_custom_breakdown_limit_with_nulls(self):
)

# no breakdown value for this guy
_create_person(distinct_ids=[f"person_null"], team_id=self.team.pk)
person0 = _create_person(distinct_ids=[f"person_null"], team_id=self.team.pk)
_create_event(
team=self.team,
event="sign up",
Expand All @@ -562,8 +712,9 @@ def test_funnel_step_custom_breakdown_limit_with_nulls(self):
result = funnel.run()

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

def test_funnel_step_custom_breakdown_limit_with_nulls_included(self):

Expand Down