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

Path end point target #5746

Merged
merged 5 commits into from
Aug 27, 2021
Merged

Path end point target #5746

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b305259
make functions dynamic
EDsCODE Aug 25, 2021
2a90315
add end test
EDsCODE Aug 25, 2021
277c3d3
add adjustments
EDsCODE Aug 26, 2021
4a17e7f
merge master
EDsCODE Aug 26, 2021
f5c6706
remove reverse
EDsCODE Aug 27, 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
48 changes: 39 additions & 9 deletions ee/clickhouse/queries/paths/paths.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,11 @@
from abc import ABC
from typing import Dict, List, Optional, Tuple

import sqlparse

from ee.clickhouse.client import sync_execute
from ee.clickhouse.queries.funnels.funnel_persons import ClickhouseFunnelPersons
from ee.clickhouse.queries.paths.path_event_query import PathEventQuery
from ee.clickhouse.sql.paths.path import PATH_ARRAY_QUERY
from posthog.constants import LIMIT
from posthog.models import Filter, Team
from posthog.models.filters.path_filter import PathFilter
from posthog.queries.paths import Paths

EVENT_IN_SESSION_LIMIT_DEFAULT = 5
SESSION_TIME_THRESHOLD_DEFAULT = 1800000 # milliseconds to 30 minutes
Expand Down Expand Up @@ -62,9 +57,19 @@ def get_path_query(self) -> str:
path_event_query, params = PathEventQuery(filter=self._filter, team_id=self._team.pk).get_query()
self.params.update(params)

boundary_event_filter, start_params = self.get_start_point_filter()
boundary_event_filter, start_params = (
self.get_end_point_filter() if self._filter.end_point else self.get_start_point_filter()
)
path_limiting_clause, time_limiting_clause = self.get_filtered_path_ordering()
compacting_function = self.get_array_compacting_function()
self.params.update(start_params)
return PATH_ARRAY_QUERY.format(path_event_query=path_event_query, boundary_event_filter=boundary_event_filter)
return PATH_ARRAY_QUERY.format(
path_event_query=path_event_query,
boundary_event_filter=boundary_event_filter,
path_limiting_clause=path_limiting_clause,
time_limiting_clause=time_limiting_clause,
compacting_function=compacting_function,
)

def get_path_query_by_funnel(self, funnel_filter: Filter):
path_query = self.get_path_query()
Expand All @@ -84,6 +89,31 @@ def get_path_query_by_funnel(self, funnel_filter: Filter):
def get_start_point_filter(self) -> Tuple[str, Dict]:

if not self._filter.start_point:
return "", {"start_point": None}
return "", {"target_point": None}

return "WHERE arrayElement(limited_path, 1) = %(target_point)s", {"target_point": self._filter.start_point}

def get_end_point_filter(self) -> Tuple[str, Dict]:
if not self._filter.end_point:
return "", {"target_point": None}

return "WHERE arrayElement(limited_path, 1) = %(start_point)s", {"start_point": self._filter.start_point}
return "WHERE arrayElement(limited_path, -1) = %(target_point)s", {"target_point": self._filter.end_point}

def get_array_compacting_function(self) -> str:
EDsCODE marked this conversation as resolved.
Show resolved Hide resolved
if self._filter.end_point:
return "arrayResize"
else:
return "arraySlice"

def get_filtered_path_ordering(self) -> Tuple[str, str]:

if self._filter.end_point:
return (
"reverse(arraySlice(reverse(filtered_path), 1, %(event_in_session_limit)s))",
EDsCODE marked this conversation as resolved.
Show resolved Hide resolved
"reverse(arraySlice(reverse(filtered_timings), 1, %(event_in_session_limit)s))",
)
else:
return (
"arraySlice(filtered_path, 1, %(event_in_session_limit)s)",
"arraySlice(filtered_timings, 1, %(event_in_session_limit)s)",
)
120 changes: 120 additions & 0 deletions ee/clickhouse/queries/test/test_paths.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,3 +258,123 @@ def test_path_by_funnel(self):
},
],
)

def test_paths_end(self):
EDsCODE marked this conversation as resolved.
Show resolved Hide resolved
Person.objects.create(team_id=self.team.pk, distinct_ids=["person_1"])
_create_event(
properties={"$current_url": "/1"},
distinct_id="person_1",
event="$pageview",
team=self.team,
timestamp="2021-05-01 00:01:00",
)
_create_event(
properties={"$current_url": "/2"},
distinct_id="person_1",
event="$pageview",
team=self.team,
timestamp="2021-05-01 00:02:00",
)
_create_event(
properties={"$current_url": "/3"},
distinct_id="person_1",
event="$pageview",
team=self.team,
timestamp="2021-05-01 00:03:00",
)
_create_event(
properties={"$current_url": "/4"},
distinct_id="person_1",
event="$pageview",
team=self.team,
timestamp="2021-05-01 00:04:00",
)
_create_event(
properties={"$current_url": "/5"},
distinct_id="person_1",
event="$pageview",
team=self.team,
timestamp="2021-05-01 00:05:00",
)
_create_event(
properties={"$current_url": "/about"},
distinct_id="person_1",
event="$pageview",
team=self.team,
timestamp="2021-05-01 00:06:00",
)
_create_event(
properties={"$current_url": "/after"},
distinct_id="person_1",
event="$pageview",
team=self.team,
timestamp="2021-05-01 00:07:00",
)

Person.objects.create(team_id=self.team.pk, distinct_ids=["person_2"])
_create_event(
properties={"$current_url": "/5"},
distinct_id="person_2",
event="$pageview",
team=self.team,
timestamp="2021-05-01 00:01:00",
)
_create_event(
properties={"$current_url": "/about"},
distinct_id="person_2",
event="$pageview",
team=self.team,
timestamp="2021-05-01 00:02:00",
)

Person.objects.create(team_id=self.team.pk, distinct_ids=["person_3"])
_create_event(
properties={"$current_url": "/3"},
distinct_id="person_3",
event="$pageview",
team=self.team,
timestamp="2021-05-01 00:01:00",
)
_create_event(
properties={"$current_url": "/4"},
distinct_id="person_3",
event="$pageview",
team=self.team,
timestamp="2021-05-01 00:02:00",
)
_create_event(
properties={"$current_url": "/about"},
distinct_id="person_3",
event="$pageview",
team=self.team,
timestamp="2021-05-01 00:03:00",
)
_create_event(
properties={"$current_url": "/after"},
distinct_id="person_3",
event="$pageview",
team=self.team,
timestamp="2021-05-01 00:04:00",
)

filter = PathFilter(
data={
"path_type": "$pageview",
"end_point": "/about",
"date_from": "2021-05-01 00:00:00",
"date_to": "2021-05-07 00:00:00",
}
)
response = ClickhousePathsNew(team=self.team, filter=filter).run(team=self.team, filter=filter,)
self.assertEqual(
response,
[
{"source": "1_/2", "target": "2_/3", "value": 1, "average_conversion_time": 60000.0},
{"source": "1_/3", "target": "2_/4", "value": 1, "average_conversion_time": 60000.0},
{"source": "1_/5", "target": "2_/about", "value": 1, "average_conversion_time": 60000.0},
{"source": "2_/3", "target": "3_/4", "value": 1, "average_conversion_time": 60000.0},
{"source": "2_/4", "target": "3_/about", "value": 1, "average_conversion_time": 60000.0},
{"source": "3_/4", "target": "4_/5", "value": 1, "average_conversion_time": 60000.0},
{"source": "4_/5", "target": "5_/about", "value": 1, "average_conversion_time": 60000.0},
],
)
10 changes: 5 additions & 5 deletions ee/clickhouse/sql/paths/path.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,11 +168,11 @@
, arrayMap((x,y) -> if(x=y, 0, 1), path_basic, path_basic_0) as mapping
, arrayFilter((x,y) -> y, time, mapping) as timings
, arrayFilter((x,y)->y, path_basic, mapping) as compact_path
, indexOf(compact_path, %(start_point)s) as start_index
, if(start_index > 0, arraySlice(compact_path, start_index), compact_path) as filtered_path
, if(start_index > 0, arraySlice(timings, start_index), timings) as filtered_timings
, arraySlice(filtered_path, 1, %(event_in_session_limit)s) as limited_path
, arraySlice(filtered_timings, 1, %(event_in_session_limit)s) as limited_timings
, indexOf(compact_path, %(target_point)s) as target_index
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even without a funnel path pinning, I think we should allow having both start and end points?

So, thoughts on making this a two step thing? First filter for start point, then for end point?

Copy link
Member Author

Choose a reason for hiding this comment

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

if we limit start and end, that's equivalent to funnel path pinning right? I was going to implement that differently using your new query, unless i'm thinking about the functionalities incorrectly

Copy link
Collaborator

Choose a reason for hiding this comment

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

Almost equivalent, just the timestamp boundaries & person joins are missing.

but yeah, no issues, if you have a different way in mind, go for it :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about it a bit more, this sounds like two separate features to me:

(1) On a general Paths view, limit start and end points. (and this can also have Cohort filters, so you're limiting start and end points for a specific set of people)

(2) Find Paths for specific people at specific points in time (a.k.a funnel path pinning). This looks the same as (1), since we have start and end points, but this is different from (1) since our timeframe for each person is different (unlike the Cohort): we care about the user at a point in the funnel, and where they go from there.

(2) definitely makes sense, and (1) sounds great for Paths exploration apart from funnels. (we already did start points)

I think we should build both.

cc: @paolodamico , @marcushyett-ph @clarkus for product input

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this makes more sense. Originally I had a notion that they were different and then I forgot because I was only thinking about the start and end point. thanks for the clarification

, if(target_index > 0, {compacting_function}(compact_path, target_index), compact_path) as filtered_path
, if(target_index > 0, {compacting_function}(timings, target_index), timings) as filtered_timings
, {path_limiting_clause} as limited_path
, {time_limiting_clause} as limited_timings
, arrayZip(limited_path, limited_timings) as limited_path_timings
FROM (
SELECT person_id
Expand Down
1 change: 1 addition & 0 deletions posthog/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@
TOTAL_INTERVALS = "total_intervals"
SELECTED_INTERVAL = "selected_interval"
START_POINT = "start_point"
END_POINT = "end_point"
STEP_LIMIT = "step_limit"
TARGET_ENTITY = "target_entity"
RETURNING_ENTITY = "returning_entity"
Expand Down
11 changes: 11 additions & 0 deletions posthog/models/filters/mixins/paths.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from posthog.constants import (
AUTOCAPTURE_EVENT,
CUSTOM_EVENT,
END_POINT,
FUNNEL_PATHS,
PAGEVIEW_EVENT,
PATH_TYPE,
Expand Down Expand Up @@ -36,6 +37,16 @@ def start_point_to_dict(self):
return {"start_point": self.start_point} if self.start_point else {}


class EndPointMixin(BaseParamMixin):
@cached_property
def end_point(self) -> Optional[str]:
return self._data.get(END_POINT, None)

@include_dict
def end_point_to_dict(self):
return {"end_point": self.end_point} if self.end_point else {}


class PropTypeDerivedMixin(PathTypeMixin):
@cached_property
def prop_type(self) -> str:
Expand Down
2 changes: 2 additions & 0 deletions posthog/models/filters/path_filter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
)
from posthog.models.filters.mixins.paths import (
ComparatorDerivedMixin,
EndPointMixin,
FunnelPathsMixin,
PathStepLimitMixin,
PropTypeDerivedMixin,
Expand All @@ -26,6 +27,7 @@

class PathFilter(
StartPointMixin,
EndPointMixin,
TargetEventDerivedMixin,
ComparatorDerivedMixin,
PropTypeDerivedMixin,
Expand Down