perf(funnels): add filter on pdi.team_id to speed up query #5997
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hazzadous
force-pushed
the
fix/gh-5888-funnels-query-perf-part-2
branch
from
be15b62 to
a3f0e2d
Compare
hazzadous
force-pushed
the
fix/gh-5888-funnels-query-perf-part-2
branch
from
a3f0e2d to
443f9eb
Compare
added 3 commits
September 16, 2021 16:28
`_gen_lateral_bodies` result seems to always be passed to `_build_query` so it doesn't make too much sense to have them required to be called in conjunction. There are probably further changes that could be made. There is some implicit coupling around the return value and the joining of the bodies into a LATERAL JOIN, which should be made explicit.
This is mainly just to make it a little more clear how the query fits together. I have done something naughty here in that I have changed from a using a GROUP BY to using DISTINCT ON for the top level. I think you'd get uniqueness of funnel path anyway, as each lateral join should only produce only one result.
hazzadous
force-pushed
the
fix/gh-5888-funnels-query-perf-part-2
branch
from
443f9eb to
601b350
Compare
hazzadous
force-pushed
the
fix/gh-5888-funnels-query-perf-part-2
branch
from
601b350 to
70df475
Compare
Have tested on prod with team_id=2. Seems to work at making the query
complete in time. Quite quick there but not much data.
There's probably a lot more we could do here but I don't want to burn
too much time on this.
Future improvements could be:
1. Remove the first subquery and just do a join
2. Investigate if there's benefit in removing the group by and using
distinct on. I don't know enough about this tbh.
3. Remove all the string manipulations with re. Just write up the query
by hand.
4. Remove the implicit coupling between query_bodies gen and the `for step,
qb` look. It's probably best to do this in one pass.
Closes #5519
hazzadous
force-pushed
the
fix/gh-5888-funnels-query-perf-part-2
branch
from
70df475 to
84f2610
Compare
EDsCODE
approved these changes
Sep 20, 2021
3 tasks
hazzadous
pushed a commit
that referenced
this pull request
Oct 19, 2021
) * Revert "perf(funnels): add filter on pdi.team_id to speed up query (#5997)" This merged caused an issue with the funnels endpoint when display was set to `ActionsLineGraph` I'm reverting so we can add in a snapshot test that will fail when this revert is reverted. This reverts commit 2fb7cf8. * test(funnel): add snapshot test for funnel trend query for postgres This test just checks the Funnel with `display="ActionsLineGraph"` against what it previously did before #5997 was merged. This code path was previously untested, so the issue wasn't picked up and resulted in #6530 * Revert "Revert "perf(funnels): add filter on pdi.team_id to speed up query" Now that we have a test in place for #6530 I'm reverting this revert so we can implement a fix. This reverts commit 49761a3. * only test on postgres * fix(funnel): `TypeError` for funnel with display=ActionsLineGraph This resolves an issue what was introduced by #5997 where we would fail to calculate a funnel result if display="ActionsLineGraph" was specified. The fix was simply to wrap a string in `sql.SQL`. Resolves #6530 * fix import order * move test outside of test factory * remove imports * move test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Have tested on prod with team_id=2. Seems to work at making the query
complete in time. Quite quick there but not much data.
There's probably a lot more we could do here but I don't want to burn
too much time on this.
Future improvements could be:
distinct on. I don't know enough about this tbh.
by hand.
for step, qblook. It's probably best to do this in one pass.Closes #5519