Mix and Match ordering and visualization of funnels #5031
Conversation
| @@ -66,3 +66,55 @@ def _format_single_funnel(self, result, with_breakdown=False): | |||
| steps.append(serialized_result) | |||
|
|
|||
| return steps[::-1] # reverse | |||
|
|
|||
| def get_step_counts_without_aggregation_query(self): | |||
There was a problem hiding this comment.
this replaces _get_steps_per_person_query
| cols.append(f"if({comparison}, NULL, latest_{i}) as latest_{i}") | ||
| return ", ".join(cols) | ||
|
|
||
| def build_step_subquery(self, level_index: int, max_steps: int): |
There was a problem hiding this comment.
everything that's only used by the ordered funnel moves "up" from base.
| @@ -16,7 +16,18 @@ def get_step_counts_query(self): | |||
|
|
|||
| max_steps = len(self._filter.entities) | |||
|
|
|||
| partition_select = self.get_partition_cols(1, max_steps) | |||
| formatted_query = self.get_step_counts_without_aggregation_query() | |||
There was a problem hiding this comment.
this pattern extends well to all types of orderings: First we get the base un-aggregated query, then perform some aggregation on it.
Split it into these two components, because some visualisations need the un-aggregated version (trends), while others need the aggregated version (time to convert, persons)
| ) GROUP BY person_id | ||
| """ | ||
|
|
||
| def get_step_counts_without_aggregation_query(self): |
There was a problem hiding this comment.
the name kinda implies the "without aggregation" part happens after get_step_counts_query.
Maybe I should rename these to: get_step_counts_query, get_aggregated_step_counts_query.
Hmm, but the existing aggregated version is get_step_counts_query. Maybe this makes the new naming more confusing 😂
| class ClickhouseFunnelTimeToConvert(ClickhouseFunnelBase): | ||
| def __init__(self, filter: Filter, team: Team, funnel_order_class: Type[ClickhouseFunnelBase]) -> None: | ||
| super().__init__(filter, team) | ||
| self.funnel_order = funnel_order_class(filter, team) |
There was a problem hiding this comment.
this is the composition. Every visualization type expects an ordering type via which it generates its inner query.
| @@ -89,7 +89,7 @@ def funnel_order_type(self) -> Optional[FunnelOrderType]: | |||
| def funnel_viz_type(self) -> Optional[FunnelVizType]: | |||
| funnel_viz_type = self._data.get(FUNNEL_VIZ_TYPE) | |||
| if ( | |||
| not funnel_viz_type is None | |||
| funnel_viz_type is None | |||
There was a problem hiding this comment.
@Twixes wasn't sure if the not was intentional?
I'd expect, since viz_type is new setting, if it's set, should override TRENDS_LINEAR? (and when not set, we wouldn't return TRENDS?
There was a problem hiding this comment.
Ahh, this would explain #5015 (comment). Looks like I accidentally did not funnel_viz_type and funnel_viz_type is None at once…
There was a problem hiding this comment.
Ah, I see it now, that makes sense 😂
There was a problem hiding this comment.
Added a test for this, too. I should've done this when I introduced the change in the first place :$
|
Warning: some merge conflicts appeared |
There was a problem hiding this comment.
- Might be worth splitting up the files to have more structure/grouping based on hierarchy of how the classes are used
- I think a follow up here would be to make the orderings work for funnel breakdown too? Because breakdown is only on the
ClickhouseFunnelpath right now
otherwise lgtm. great job
|
Changes
Resolves #4988
More details inline. This PR sets up
get_step_counts_without_aggregation_queryas one step "below"get_step_counts_querysuch that everything which needs these values can use them as they see fit.Checklist