Skip to content

Run broken down analyses in one SQL query like others - #5072

Closed
Twixes wants to merge 2 commits into
masterfrom
single-query-funnel-breakdown
Closed

Twixes wants to merge 2 commits into
masterfrom
single-query-funnel-breakdown

Conversation

@Twixes

@Twixes Twixes commented Jul 12, 2021 •

Copy link
Copy Markdown
Contributor

Changes

Currently our ClickHouse analyses require an additional SQL query when using breakdown – for obtaining values to break down by. This impact speed and therefore user experience, because an additional query round trip before the proper query takes time. We can do away with this using common table expressions though – this way the same breakdown values query is ran before the proper analysis one all in the same query string.

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests

@timgl
timgl temporarily deployed to posthog-pr-5072 July 12, 2021 10:08 Inactive
@Twixes
Twixes temporarily deployed to posthog-pr-5072 July 12, 2021 10:26 Inactive
@neilkakkar

Copy link
Copy Markdown
Contributor

Heads up: I'd like this to go in after #5043 which introduces some tests for breakdown keys having different types (possible with JsonExtractRaw), just to ensure things keep working with this change :)

@macobo

macobo commented Jul 12, 2021 •

Copy link
Copy Markdown
Contributor

Fly-by comment

This impact speed and therefore user experience, because an additional query round trip before the proper query takes time.

Note that this is not always true. More roundtrips can often end up faster if you can optimize the amount of data being sent back or how the query executes as a result. E.g. if you're fetching a collection of size N and collection of size M separately it can be much better than trying to fetch N x M.

A good example of this is how our sessions query works - fetching relevant users -> sessions is much faster than trying to do an inline join in practice because we can set limits on what data to look at and when to stop iteration early.

In practice UX has been affected by queries taking 5 seconds + - one vs two sync_execute matters very little here. It's worth being principled and measuring the effect.

All of that said I haven't dug into the diff - it sounds like it might be a valid optimization. I'd just be wary of generalizing like above. :)

@Twixes

Twixes commented Jul 12, 2021 •

Copy link
Copy Markdown
Contributor Author

No no @macobo, this is literally the same performance wise as with roundtrip, only the roundtrip is eliminated with a CTE. No joins, it's just basically saving the values to break down by as a variable inside ClickHouse before the final query.

@Twixes

Twixes commented Jul 16, 2021

Copy link
Copy Markdown
Contributor Author

Opening a new PR as I'm more confident about doing this on top of master than about reconciling these pretty big merge conflicts.

@Twixes Twixes closed this Jul 16, 2021
@Twixes
Twixes deleted the single-query-funnel-breakdown branch December 16, 2021 13:29

This branch was previously deployed

1 inactive deployment
posthog-pr-5072 — cf51b00b Deployed Jul 12, 2021 by Twixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants