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 all 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
45 changes: 40 additions & 5 deletions ee/clickhouse/queries/paths/paths.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Dict, List, Optional, Tuple
from typing import Dict, List, Literal, Optional, Tuple

from rest_framework.exceptions import ValidationError

Expand Down Expand Up @@ -65,9 +65,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 @@ -87,6 +97,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) = %(target_point)s", {"target_point": self._filter.end_point}

return "WHERE arrayElement(limited_path, 1) = %(start_point)s", {"start_point": self._filter.start_point}
def get_array_compacting_function(self) -> Literal["arrayResize", "arraySlice"]:
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 (
"arraySlice(filtered_path, (-1) * %(event_in_session_limit)s)",
"arraySlice(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)",
)
122 changes: 122 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 @@ -11,6 +11,7 @@
from posthog.models.filters import Filter, PathFilter
from posthog.models.person import Person
from posthog.queries.test.test_paths import paths_test_factory
from posthog.test.base import test_with_materialized_columns


def _create_event(**kwargs):
Expand Down Expand Up @@ -260,6 +261,127 @@ def test_path_by_funnel(self):
],
)

@test_with_materialized_columns(["$current_url"])
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},
],
)

def test_event_inclusion_exclusion_filters(self):

# P1 for pageview event
Expand Down
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 @@ -39,6 +40,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 @@ -27,6 +28,7 @@

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