feat(funnels): speed up funnel queries by preventing properties being passed across shards #13935

yakkomajuri · 2023-01-26T14:11:00Z

Problem

Changes

Derive the funnel steps in the innermost subquery in order to prevent sending full properties across shards. This can speed up queries by up to 3-5x (estimate based on the fact that the slower the query today the greater the gains).

The current query will send data in the order of megabytes to the other shard, whereas the previous query would send gigabytes and gigabytes (see e.g. first pic).

Benchmarks:

See more

Note that I chose the path of least resistance to implement this. I think there's a refactor in order to simplify a lot of the logic (which is very messy) but I decided not to do that here. I kept things mostly as they are, with few changes.

A bunch of stale snapshots got deleted as a result of my testing.

How did you test this code?

Existing tests for funnels are all passing. Also tested manually.

Marked it as a feat because of the perf improvement but from the perspective of the query results it's just a refactor - no behavior should change.

… passed across shards

yakkomajuri · 2023-01-26T14:12:15Z

posthog/queries/funnels/test/__snapshots__/test_funnel.ambr

@@ -30,8 +30,11 @@
                     min(latest_1) over (PARTITION by aggregation_target
                                         ORDER BY timestamp DESC ROWS BETWEEN UNBOUNDED PRECEDING AND 0 PRECEDING) latest_1
              FROM
-                (SELECT aggregation_target,
-                        timestamp,
+                (SELECT e.event as event,


This file is the one showing the flattening of the query clearly.

yakkomajuri · 2023-01-26T14:42:39Z

Well.... Seems all's good for the "traditional funnel" and f'd for everything else :D

…o funnel-props

yakkomajuri · 2023-01-26T15:48:04Z

Seems only unordered is the problem (besides this one tricky thing with exclusions that I know how to fix), looking into it

…o funnel-props

Remove more unnecessary fields

macobo · 2023-01-27T13:02:25Z

posthog/queries/funnels/funnel_event_query.py

+            + "{step_filter}"
+        )
+
+        query = f"""


Q: Why split the overall thing up? IMO not adding too much clarity here.

We spoke about this on a call - essentially there are ordering requirements with the functions building this query (as they have side effects) and this was the cleanest way to build things now without a large refactor

macobo · 2023-01-27T13:02:49Z

posthog/queries/funnels/funnel_event_query.py

-        query = f"""
-            SELECT {', '.join(_fields)} FROM events {self.EVENT_TABLE_ALIAS}
+        select_fragment = (
+            f"SELECT {', '.join(_fields)}" + "{extra_select_fields}" + f" FROM events {self.EVENT_TABLE_ALIAS}"


You can do the following in f-strings: {{extra_select_fields}} if really needed, though the logic behind it is sus.

updated to this

yakkomajuri · 2023-01-27T17:19:45Z

Ok this is ready to go @macobo

yakkomajuri · 2023-01-27T17:20:56Z

ee/clickhouse/views/test/__snapshots__/test_clickhouse_experiments.ambr

+                           pdi.person_id as aggregation_target,
+                           pdi.person_id as person_id ,


we certainly don't need this :D

Kinda don't wanna touch this PR anymore though so will fix in a follow up if OK

macobo · 2023-01-30T07:45:08Z

posthog/queries/funnels/base.py

-        parsed_extra_fields = f", {', '.join(extra_fields)}" if extra_fields else ""
-
-        event_query, params = FunnelEventQuery(
+        inner_query, params = FunnelEventQuery(


Q: Should we name this events_query?

macobo · 2023-01-30T07:46:52Z

posthog/queries/funnels/base.py

        extra_join = ""

        if self._filter.breakdown:
            if self._filter.breakdown_type == "cohort":
                extra_join = self._get_cohort_breakdown_join()
+


Nit: Let's remove this empty line

Suggested change

macobo · 2023-01-30T07:47:41Z

posthog/queries/funnels/base.py

-            event_query=event_query,
+        extra_select_fields = f", {', '.join(all_step_cols)}" if all_step_cols else ""
+
+        inner_query = inner_query.format(


Q: Should we name this events_query or funnel_events_query? inner_query had me going in cycles since it was ambigious.

Remnant of the old implementation where the events_query had an inner_query. Will rename

macobo · 2023-01-30T07:47:56Z

Amazing work!

yakkomajuri · 2023-01-30T11:21:48Z

I'll get this in because every change I've been making seems to be a bit of a struggle with checks failing due to API limits and what not. Want to get this out of sight out of mind :D Will put a PR right away addressing the review points though @macobo

… passed across shards (#13935) * feat(funnels): speed up funnel queries by preventing properties being passed across shards * Update snapshots * Update snapshots * testing * Update snapshots * Update snapshots * new approach * Update snapshots * Update snapshots * Update snapshots * remove more unnecessary fields * update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * remove more cols and see :) * remove event_columns_to_query * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * update query building * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots * Update snapshots --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

feat(funnels): speed up funnel queries by preventing properties being…

e0de148

… passed across shards

yakkomajuri commented Jan 26, 2023

View reviewed changes

github-actions bot added 2 commits January 26, 2023 14:15

Update snapshots

33d60aa

Update snapshots

9689ccf

yakkomajuri and others added 4 commits January 26, 2023 11:45

testing

9f50c75

Merge branch 'funnel-props' of https://github.com/PostHog/posthog int…

2bef6d6

…o funnel-props

Update snapshots

8990a17

Update snapshots

45f826d

yakkomajuri marked this pull request as draft January 26, 2023 15:30

yakkomajuri and others added 19 commits January 26, 2023 16:09

new approach

271b884

Merge branch 'funnel-props' of https://github.com/PostHog/posthog int…

3b4ad84

…o funnel-props

merge master

c773f42

Update snapshots

a0a6952

Update snapshots

25ac140

Update snapshots

35dec3b

remove more unnecessary fields

2673d9e

Merge branch 'funnel-props-playground' into funnel-props

d313ef6

Remove more unnecessary fields

update snapshots

1705db3

Update snapshots

8aa4aba

Update snapshots

16642a4

Update snapshots

0a3cbde

Update snapshots

f02d843

Update snapshots

dfeb284

Update snapshots

f652c04

Update snapshots

8d0ad67

Update snapshots

25db7ae

Update snapshots

6cd69dd

Update snapshots

30758b0

yakkomajuri and others added 8 commits January 27, 2023 09:04

remove event_columns_to_query

6da0480

Update snapshots

9146bff

Update snapshots

a4ecdd9

Update snapshots

dbeae52

Update snapshots

a4415d5

Update snapshots

d93bfb8

Update snapshots

7a63586

Update snapshots

355a8d1

macobo reviewed Jan 27, 2023

View reviewed changes

yakkomajuri and others added 2 commits January 27, 2023 12:26

update query building

26e7dbd

Update snapshots

7ffbe28

yakkomajuri marked this pull request as ready for review January 27, 2023 15:41

github-actions bot and others added 5 commits January 27, 2023 15:43

Update snapshots

d10d071

Update snapshots

ee64c03

Update snapshots

5ef6b1f

Merge branch 'master' into funnel-props

dc53a5f

Update snapshots

8015807

yakkomajuri commented Jan 27, 2023

View reviewed changes

github-actions bot added 2 commits January 27, 2023 17:21

Update snapshots

8bebe4c

Update snapshots

92bf2ef

macobo approved these changes Jan 30, 2023

View reviewed changes

yakkomajuri merged commit 531c367 into master Jan 30, 2023

yakkomajuri deleted the funnel-props branch January 30, 2023 11:22

This was referenced Jan 30, 2023

refactor(funnels): rename variable #13984

Merged

CHQueryErrorSyntaxError - funnels #14022

Open

mariusandra mentioned this pull request Mar 6, 2023

feat(hogql): properties between shards #14549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(funnels): speed up funnel queries by preventing properties being passed across shards #13935

feat(funnels): speed up funnel queries by preventing properties being passed across shards #13935

yakkomajuri commented Jan 26, 2023 •

edited

Loading

yakkomajuri Jan 26, 2023

yakkomajuri commented Jan 26, 2023

yakkomajuri commented Jan 26, 2023

macobo Jan 27, 2023

yakkomajuri Jan 27, 2023

macobo Jan 27, 2023

yakkomajuri Jan 27, 2023

yakkomajuri commented Jan 27, 2023

yakkomajuri Jan 27, 2023

macobo Jan 30, 2023

macobo Jan 30, 2023

macobo Jan 30, 2023

yakkomajuri Jan 30, 2023

macobo commented Jan 30, 2023

yakkomajuri commented Jan 30, 2023

		pdi.person_id as aggregation_target,
		pdi.person_id as person_id ,

feat(funnels): speed up funnel queries by preventing properties being passed across shards #13935

feat(funnels): speed up funnel queries by preventing properties being passed across shards #13935

Conversation

yakkomajuri commented Jan 26, 2023 • edited Loading

Problem

Changes

How did you test this code?

Choose a reason for hiding this comment

yakkomajuri commented Jan 26, 2023

yakkomajuri commented Jan 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yakkomajuri commented Jan 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macobo commented Jan 30, 2023

yakkomajuri commented Jan 30, 2023

yakkomajuri commented Jan 26, 2023 •

edited

Loading