695-Optimize funnel rendering #792
Conversation
posthog/models/funnel.py
Outdated
| with connection.cursor() as cursor: | ||
| query_bodies = {step: cursor.mogrify(*q.query.sql_with_params()) for step, q in query_bodies.items()} | ||
| query = sql.SQL(QUERY_HEADER).format( | ||
| # step=steps[0], |
There was a problem hiding this comment.
woops! yeah, definitely
| .replace("'2000-01-01T00:00:00+00:00'::timestamptz", "{prev_step_ts}") | ||
| .replace('"posthog_event"."distinct_id"', '"pdi"."person_id"') | ||
| .replace('FROM "posthog_event"', 'FROM posthog_event JOIN posthog_persondistinctid pdi ON pdi.distinct_id = posthog_event.distinct_id')) | ||
| if i == 0: |
There was a problem hiding this comment.
It might be worth splitting out each of these if/elif statements into each own function with a descriptive function name, so it's more obvious what's happening here.
There was a problem hiding this comment.
I've been trying to split these out into functions, but I think it is more readable in this if/elif/else block, but that might just be my golang showing. How about I annotate this with comments and see if it's more readable? If it's still ambiguous I'll refactor into individual funcs.
There was a problem hiding this comment.
I tend to like smaller functions as a way of avoiding big blocks of logic, but I'm actually happy with these comments as they explain it clearly!
There was a problem hiding this comment.
Generally me too, but this is so procedural in nature and depends on the surrounding context it was becoming a bowl of spaghetti when broken out into sep functions
posthog/models/funnel.py
Outdated
| i = 0 | ||
| for step, qb in query_bodies.items(): | ||
| q = sql.SQL(qb.decode('utf-8') | ||
| .replace("'1234321'", "{prev_step_person_id}") |
There was a problem hiding this comment.
Is there a reason this couldn't happen in _gen_lateral_bodies? There might be, though if so might be good to add comment explaining that
There was a problem hiding this comment.
Good point. Moved to _gen_lateral_bodies
| with connection.cursor() as cursor: | ||
| event_string = cursor.mogrify(*event.query.sql_with_params()) | ||
| query = sql.SQL(event_string.decode('utf-8') | ||
| .replace("'1234321'", "{prev_step_person_id}") |
There was a problem hiding this comment.
I'm a little confused about why these have to be replaced at all? I assume it's because the Django ORM doesn't support {}? In that case maybe worth adding a comment explaining
| .replace("'2000-01-01T00:00:00+00:00'::timestamptz", "{prev_step_ts}") | ||
| .replace('"posthog_event"."distinct_id"', '"pdi"."person_id"') | ||
| .replace('FROM "posthog_event"', 'FROM posthog_event JOIN posthog_persondistinctid pdi ON pdi.distinct_id = posthog_event.distinct_id')) | ||
| if i == 0: |
There was a problem hiding this comment.
I tend to like smaller functions as a way of avoiding big blocks of logic, but I'm actually happy with these comments as they explain it clearly!
Changes
Checklist