-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Funnel steps breakdown #4949
Funnel steps breakdown #4949
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of small nits, looks good to me otherwise!
else: | ||
return "" | ||
|
||
def _get_breakdown_prop(self) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit / not sure if worth tackling here since other discrepancies remain: we decided not to return query strings with a comma in the beginning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will save that change for a later update where we adjust how the handling is done in all this query formatting
} | ||
|
||
try: | ||
top_elements_array_result = sync_execute(query, element_params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Radical question: could the results of this class be achieved using CTEs, so that the broken down funnel calculation is a single SQL query without a round trip?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, this in interesting: Completely missed the execution at the end of both prop_values
functions.
I don't see why not, sounds like a reasonable suggestion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless they increased the support, but from what I remember clickhouse CTEs only allow you to use the reference in the immediate level below the CTE, so I dont think we're able to use the results of that CTE on the innermost query where we would need to do the filtering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I've tried, they should work – though I may have missed something! I'd give this a go in any case, as even though this does not affect big-O, doing a round trip definitely is enough overhead to be degrading the experience for users.
Changes
Please describe.
If this affects the frontend, include screenshots.
Checklist