From 46ec36c76e33531e2e323faeae827d3852468dcf Mon Sep 17 00:00:00 2001 From: Tom Owers Date: Thu, 1 Feb 2024 12:29:01 +0000 Subject: [PATCH] feat(actors_modal): Added breakdowns and series selectors to the actors modal (#19983) * Added breakdowns and series selectors to the actors modal * Added compare to the actor modal * Fixed breakdowns making a bazillon queries * Repair modal state logic cause loadActors only uses actorsQuery selector I suppose this was changed at some point. We cannot use the query argument anymore. * Adjust the breakdown limit when not hiding other data * Turn off "Other" breakdown option for modal for now --------- Co-authored-by: Julian Bez --- frontend/src/queries/schema.json | 130 +++++++++ frontend/src/queries/schema.ts | 20 +- .../trends/persons-modal/PersonsModal.tsx | 26 +- .../trends/persons-modal/personsModalLogic.ts | 37 ++- .../scenes/trends/viz/ActionsLineGraph.tsx | 5 +- frontend/src/types.ts | 1 + mypy-baseline.txt | 25 +- .../insight_actors_query_options_runner.py | 10 +- .../insights/insight_actors_query_runner.py | 7 +- .../insights/trends/breakdown.py | 20 +- .../insights/trends/breakdown_values.py | 37 +-- .../insights/trends/query_builder.py | 196 +++++++++---- .../insights/trends/series_with_extras.py | 3 + .../test/__snapshots__/test_trends.ambr | 12 +- .../insights/trends/test/test_trends.py | 2 +- .../trends/test/test_trends_query_runner.py | 262 +++++++++++++++++- .../insights/trends/trends_query_runner.py | 165 ++++++++--- .../utils/query_previous_period_date_range.py | 16 +- posthog/schema.py | 42 +++ 19 files changed, 816 insertions(+), 200 deletions(-) diff --git a/frontend/src/queries/schema.json b/frontend/src/queries/schema.json index d441257e746e2..dd5c94cdf0946 100644 --- a/frontend/src/queries/schema.json +++ b/frontend/src/queries/schema.json @@ -393,6 +393,9 @@ "enum": ["cohort", "person", "event", "group", "session", "hogql"], "type": "string" }, + "BreakdownValueInt": { + "type": "integer" + }, "ChartAxis": { "additionalProperties": false, "properties": { @@ -1912,6 +1915,20 @@ "InsightActorsQuery": { "additionalProperties": false, "properties": { + "breakdown": { + "anyOf": [ + { + "type": "string" + }, + { + "$ref": "#/definitions/BreakdownValueInt" + } + ] + }, + "compare": { + "enum": ["current", "previous"], + "type": "string" + }, "day": { "anyOf": [ { @@ -1933,6 +1950,9 @@ "response": { "$ref": "#/definitions/ActorsQueryResponse" }, + "series": { + "type": "integer" + }, "source": { "$ref": "#/definitions/InsightQuerySource" }, @@ -1963,6 +1983,45 @@ "InsightActorsQueryOptionsResponse": { "additionalProperties": false, "properties": { + "breakdown": { + "items": { + "additionalProperties": false, + "properties": { + "label": { + "type": "string" + }, + "value": { + "anyOf": [ + { + "type": "string" + }, + { + "$ref": "#/definitions/BreakdownValueInt" + } + ] + } + }, + "required": ["label", "value"], + "type": "object" + }, + "type": "array" + }, + "compare": { + "items": { + "additionalProperties": false, + "properties": { + "label": { + "type": "string" + }, + "value": { + "type": "string" + } + }, + "required": ["label", "value"], + "type": "object" + }, + "type": "array" + }, "day": { "items": { "additionalProperties": false, @@ -2003,6 +2062,22 @@ }, "type": "array" }, + "series": { + "items": { + "additionalProperties": false, + "properties": { + "label": { + "type": "string" + }, + "value": { + "type": "integer" + } + }, + "required": ["label", "value"], + "type": "object" + }, + "type": "array" + }, "status": { "items": { "additionalProperties": false, @@ -2960,6 +3035,45 @@ { "additionalProperties": false, "properties": { + "breakdown": { + "items": { + "additionalProperties": false, + "properties": { + "label": { + "type": "string" + }, + "value": { + "anyOf": [ + { + "type": "string" + }, + { + "$ref": "#/definitions/BreakdownValueInt" + } + ] + } + }, + "required": ["label", "value"], + "type": "object" + }, + "type": "array" + }, + "compare": { + "items": { + "additionalProperties": false, + "properties": { + "label": { + "type": "string" + }, + "value": { + "type": "string" + } + }, + "required": ["label", "value"], + "type": "object" + }, + "type": "array" + }, "day": { "items": { "additionalProperties": false, @@ -3000,6 +3114,22 @@ }, "type": "array" }, + "series": { + "items": { + "additionalProperties": false, + "properties": { + "label": { + "type": "string" + }, + "value": { + "type": "integer" + } + }, + "required": ["label", "value"], + "type": "object" + }, + "type": "array" + }, "status": { "items": { "additionalProperties": false, diff --git a/frontend/src/queries/schema.ts b/frontend/src/queries/schema.ts index 57d39b2201a64..2719878250da9 100644 --- a/frontend/src/queries/schema.ts +++ b/frontend/src/queries/schema.ts @@ -969,11 +969,16 @@ export interface InsightActorsQuery !!value) + .filter(([, value]) => { + if (Array.isArray(value)) { + return !!value.length + } + + return !!value + }) .map(([key, options]) => (
- - {midEllipsis(actor.distinct_ids[0], 32)} - + {actor.distinct_ids?.[0] && ( + + {midEllipsis(actor.distinct_ids[0], 32)} + + )} )} diff --git a/frontend/src/scenes/trends/persons-modal/personsModalLogic.ts b/frontend/src/scenes/trends/persons-modal/personsModalLogic.ts index 7a51ca349d0ab..b919a96d7e967 100644 --- a/frontend/src/scenes/trends/persons-modal/personsModalLogic.ts +++ b/frontend/src/scenes/trends/persons-modal/personsModalLogic.ts @@ -61,24 +61,22 @@ export const personsModalLogic = kea([ setIsCohortModalOpen: (isOpen: boolean) => ({ isOpen }), loadActors: ({ url, - query, clear, offset, additionalSelect, }: { url?: string | null - query?: InsightActorsQuery | null clear?: boolean offset?: number additionalSelect?: PersonModalLogicProps['additionalSelect'] }) => ({ url, - query, clear, offset, additionalSelect, }), loadNextActors: true, + updateQuery: (query: InsightActorsQuery) => ({ query }), updateActorsQuery: (query: Partial) => ({ query }), loadActorsQueryOptions: (query: InsightActorsQuery) => ({ query }), }), @@ -91,7 +89,7 @@ export const personsModalLogic = kea([ actorsResponse: [ null as ListActorsResponse | null, { - loadActors: async ({ url, query, clear, offset, additionalSelect }, breakpoint) => { + loadActors: async ({ url, clear, offset, additionalSelect }, breakpoint) => { if (url) { url += '&include_recordings=true' @@ -99,7 +97,7 @@ export const personsModalLogic = kea([ url += `&search=${values.searchTerm}` } } - if (url && !query) { + if (url && !values.actorsQuery) { const res = await api.get(url) breakpoint() @@ -108,7 +106,7 @@ export const personsModalLogic = kea([ } return res } - if (query) { + if (values.actorsQuery) { const response = await performQuery( { ...values.actorsQuery, @@ -189,7 +187,7 @@ export const personsModalLogic = kea([ query: [ props.query as InsightActorsQuery | null, { - loadActors: (state, { query }) => query ?? state ?? null, + updateQuery: (_, { query }) => query, }, ], actors: [ @@ -233,7 +231,7 @@ export const personsModalLogic = kea([ listeners(({ actions, values, props }) => ({ setSearchTerm: async (_, breakpoint) => { await breakpoint(500) - actions.loadActors({ query: values.query, url: props.url, clear: true }) + actions.loadActors({ url: props.url, clear: true }) }, saveAsCohort: async ({ cohortName }) => { const cohortParams = { @@ -273,17 +271,18 @@ export const personsModalLogic = kea([ actions.loadActors({ url: values.actorsResponse.next }) } if (values.actorsResponse?.next_offset) { - actions.loadActors({ query: values.query, offset: values.actorsResponse.next_offset }) + actions.loadActors({ offset: values.actorsResponse.next_offset }) } }, - loadActors: ({ query }) => { - if (query && !values.insightActorsQueryOptions) { - actions.loadActorsQueryOptions(query) + loadActors: () => { + if (values.query && !values.insightActorsQueryOptions) { + actions.loadActorsQueryOptions(values.query) } }, - updateActorsQuery: ({ query }) => { - if (query && values.query) { - actions.loadActors({ query: { ...values.query, ...query }, offset: 0, clear: true }) + updateActorsQuery: ({ query: q }) => { + if (q && values.query) { + actions.updateQuery({ ...values.query, ...q }) + actions.loadActors({ offset: 0, clear: true }) } }, })), @@ -334,8 +333,8 @@ export const personsModalLogic = kea([ }, ], actorsQuery: [ - (s) => [(_, p) => p.query, (_, p) => p.orderBy, s.searchTerm, s.selectFields], - (query, orderBy, searchTerm, selectFields): ActorsQuery | null => { + (s) => [(_, p) => p.orderBy, s.query, s.searchTerm, s.selectFields], + (orderBy, query, searchTerm, selectFields): ActorsQuery | null => { if (!query) { return null } @@ -366,7 +365,7 @@ export const personsModalLogic = kea([ }), afterMount(({ actions, props }) => { - actions.loadActors({ query: props.query, url: props.url, additionalSelect: props.additionalSelect }) + actions.loadActors({ url: props.url, additionalSelect: props.additionalSelect }) actions.reportPersonsModalViewed({ url: props.url, @@ -390,7 +389,7 @@ export const personsModalLogic = kea([ propsChanged(({ props, actions }, oldProps) => { if (props.url !== oldProps.url) { - actions.loadActors({ query: props.query, url: props.url, clear: true }) + actions.loadActors({ url: props.url, clear: true }) } }), ]) diff --git a/frontend/src/scenes/trends/viz/ActionsLineGraph.tsx b/frontend/src/scenes/trends/viz/ActionsLineGraph.tsx index f0ef315e075b4..dac3a4d97fda7 100644 --- a/frontend/src/scenes/trends/viz/ActionsLineGraph.tsx +++ b/frontend/src/scenes/trends/viz/ActionsLineGraph.tsx @@ -119,7 +119,7 @@ export function ActionsLineGraph({ return } - const day = dataset?.days?.[index] ?? '' + const day = dataset.action?.days?.[index] ?? dataset?.days?.[index] ?? '' const label = dataset?.label ?? dataset?.labels?.[index] ?? '' const title = isStickiness ? ( @@ -147,6 +147,9 @@ export function ActionsLineGraph({ source: query.source, day, status: dataset.status, + series: dataset.action?.order ?? 0, + breakdown: dataset.breakdown_value, + compare: dataset.compare_label, }, }) } else { diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 23c519ea139d5..ce64eb5ba3c8a 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -2113,6 +2113,7 @@ export interface SystemStatusAnalyzeResult { } export interface ActionFilter extends EntityFilter { + days?: string[] math?: string math_property?: string math_group_type_index?: number | null diff --git a/mypy-baseline.txt b/mypy-baseline.txt index 4be818b4f9aa6..88321ee2abb57 100644 --- a/mypy-baseline.txt +++ b/mypy-baseline.txt @@ -384,12 +384,6 @@ posthog/hogql_queries/insights/trends/breakdown.py:0: error: Incompatible types posthog/hogql_queries/insights/trends/breakdown.py:0: error: Incompatible types in assignment (expression has type "float", variable has type "int") [assignment] posthog/hogql_queries/insights/trends/breakdown.py:0: error: Incompatible types in assignment (expression has type "str", variable has type "int") [assignment] posthog/hogql_queries/insights/trends/breakdown.py:0: error: Incompatible types in assignment (expression has type "str", variable has type "int") [assignment] -posthog/hogql_queries/insights/trends/breakdown.py:0: error: Item "None" of "BreakdownFilter | None" has no attribute "breakdown_type" [union-attr] -posthog/hogql_queries/insights/trends/breakdown.py:0: error: Argument "breakdown_type" to "BreakdownValues" has incompatible type "BreakdownType | Any | None"; expected "str" [arg-type] -posthog/hogql_queries/insights/trends/breakdown.py:0: error: Item "None" of "BreakdownFilter | None" has no attribute "breakdown_histogram_bin_count" [union-attr] -posthog/hogql_queries/insights/trends/breakdown.py:0: error: Item "None" of "BreakdownFilter | None" has no attribute "breakdown_group_type_index" [union-attr] -posthog/hogql_queries/insights/trends/breakdown.py:0: error: Item "None" of "BreakdownFilter | None" has no attribute "breakdown_hide_other_aggregation" [union-attr] -posthog/hogql_queries/insights/trends/breakdown.py:0: error: Item "None" of "BreakdownFilter | None" has no attribute "breakdown_limit" [union-attr] posthog/hogql_queries/insights/trends/breakdown.py:0: error: Unsupported operand types for + ("str" and "float") [operator] posthog/hogql_queries/insights/trends/breakdown.py:0: note: Left operand is of type "str | int" posthog/hogql_queries/insights/trends/breakdown.py:0: error: Incompatible return value type (got "list[tuple[str | int, Any | float | str | int]]", expected "list[tuple[float, float]]") [return-value] @@ -406,15 +400,6 @@ posthog/hogql_queries/events_query_runner.py:0: error: Statement is unreachable posthog/hogql/metadata.py:0: error: Argument "metadata_source" to "translate_hogql" has incompatible type "SelectQuery | SelectUnionQuery"; expected "SelectQuery | None" [arg-type] posthog/hogql/metadata.py:0: error: Incompatible types in assignment (expression has type "Expr", variable has type "SelectQuery | SelectUnionQuery") [assignment] posthog/queries/breakdown_props.py:0: error: Argument 1 to "translate_hogql" has incompatible type "str | int"; expected "str" [arg-type] -posthog/hogql_queries/insights/trends/query_builder.py:0: error: Incompatible types in assignment (expression has type "SelectUnionQuery", variable has type "SelectQuery") [assignment] -posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "extend" [union-attr] -posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "append" [union-attr] -posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "append" [union-attr] -posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "append" [union-attr] -posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "append" [union-attr] -posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "append" [union-attr] -posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "append" [union-attr] -posthog/hogql_queries/insights/trends/query_builder.py:0: error: Item "None" of "list[Expr] | None" has no attribute "append" [union-attr] posthog/queries/funnels/base.py:0: error: "HogQLContext" has no attribute "person_on_events_mode" [attr-defined] posthog/queries/funnels/base.py:0: error: Argument 1 to "translate_hogql" has incompatible type "str | int"; expected "str" [arg-type] ee/clickhouse/queries/funnels/funnel_correlation.py:0: error: Statement is unreachable [unreachable] @@ -427,14 +412,7 @@ posthog/hogql_queries/insights/trends/trends_query_runner.py:0: error: Unused "t posthog/hogql_queries/insights/trends/trends_query_runner.py:0: error: Statement is unreachable [unreachable] posthog/hogql_queries/insights/trends/trends_query_runner.py:0: error: Argument 1 to "FormulaAST" has incompatible type "map[Any]"; expected "list[list[float]]" [arg-type] posthog/hogql_queries/insights/trends/trends_query_runner.py:0: error: Argument 1 to "FormulaAST" has incompatible type "map[Any]"; expected "list[list[float]]" [arg-type] -posthog/hogql_queries/insights/trends/trends_query_runner.py:0: error: Item "None" of "BreakdownFilter | None" has no attribute "breakdown_type" [union-attr] -posthog/hogql_queries/insights/trends/trends_query_runner.py:0: error: Item "None" of "BreakdownFilter | None" has no attribute "breakdown_type" [union-attr] -posthog/hogql_queries/insights/trends/trends_query_runner.py:0: error: Item "None" of "BreakdownFilter | None" has no attribute "breakdown_type" [union-attr] -posthog/hogql_queries/insights/trends/trends_query_runner.py:0: error: Item "None" of "BreakdownFilter | None" has no attribute "breakdown_type" [union-attr] -posthog/hogql_queries/insights/trends/trends_query_runner.py:0: error: Item "None" of "BreakdownFilter | None" has no attribute "breakdown_type" [union-attr] -posthog/hogql_queries/insights/trends/trends_query_runner.py:0: error: Item "None" of "BreakdownFilter | None" has no attribute "breakdown" [union-attr] -posthog/hogql_queries/insights/trends/trends_query_runner.py:0: error: Argument 1 to "_event_property" of "TrendsQueryRunner" has incompatible type "str | float | list[str | float] | Any | None"; expected "str" [arg-type] -posthog/hogql_queries/insights/trends/trends_query_runner.py:0: error: Item "None" of "BreakdownFilter | None" has no attribute "breakdown_group_type_index" [union-attr] +posthog/hogql_queries/insights/trends/trends_query_runner.py:0: error: Argument 1 to "_event_property" of "TrendsQueryRunner" has incompatible type "str | float | list[str | float] | None"; expected "str" [arg-type] posthog/hogql_queries/insights/retention_query_runner.py:0: error: Incompatible types in assignment (expression has type "Expr", variable has type "Call") [assignment] posthog/hogql_queries/insights/retention_query_runner.py:0: error: Incompatible types in assignment (expression has type "Call", variable has type "Field") [assignment] posthog/hogql_queries/insights/retention_query_runner.py:0: error: Argument "select" to "SelectQuery" has incompatible type "list[Alias]"; expected "list[Expr]" [arg-type] @@ -576,7 +554,6 @@ posthog/hogql_queries/legacy_compatibility/test/test_filter_to_query.py:0: error posthog/hogql_queries/legacy_compatibility/test/test_filter_to_query.py:0: error: Item "RetentionQuery" of "TrendsQuery | FunnelsQuery | RetentionQuery | PathsQuery | StickinessQuery | LifecycleQuery" has no attribute "lifecycleFilter" [union-attr] posthog/hogql_queries/legacy_compatibility/test/test_filter_to_query.py:0: error: Item "PathsQuery" of "TrendsQuery | FunnelsQuery | RetentionQuery | PathsQuery | StickinessQuery | LifecycleQuery" has no attribute "lifecycleFilter" [union-attr] posthog/hogql_queries/legacy_compatibility/test/test_filter_to_query.py:0: error: Item "StickinessQuery" of "TrendsQuery | FunnelsQuery | RetentionQuery | PathsQuery | StickinessQuery | LifecycleQuery" has no attribute "lifecycleFilter" [union-attr] -posthog/hogql_queries/insights/trends/test/test_trends_query_runner.py:0: error: Statement is unreachable [unreachable] posthog/hogql_queries/insights/trends/test/test_aggregation_operations.py:0: error: Item "SelectUnionQuery" of "SelectQuery | SelectUnionQuery" has no attribute "select" [union-attr] posthog/hogql_queries/insights/trends/test/test_aggregation_operations.py:0: error: Item "SelectUnionQuery" of "SelectQuery | SelectUnionQuery" has no attribute "select" [union-attr] posthog/hogql_queries/insights/trends/test/test_aggregation_operations.py:0: error: Item "SelectUnionQuery" of "SelectQuery | SelectUnionQuery" has no attribute "group_by" [union-attr] diff --git a/posthog/hogql_queries/insights/insight_actors_query_options_runner.py b/posthog/hogql_queries/insights/insight_actors_query_options_runner.py index d4cef5f8466be..4ce58263a8c06 100644 --- a/posthog/hogql_queries/insights/insight_actors_query_options_runner.py +++ b/posthog/hogql_queries/insights/insight_actors_query_options_runner.py @@ -3,6 +3,7 @@ from posthog.hogql import ast from posthog.hogql_queries.insights.lifecycle_query_runner import LifecycleQueryRunner +from posthog.hogql_queries.insights.trends.trends_query_runner import TrendsQueryRunner from posthog.hogql_queries.query_runner import QueryRunner, get_query_runner from posthog.models.filters.mixins.utils import cached_property from posthog.schema import InsightActorsQueryOptions, InsightActorsQueryOptionsResponse @@ -23,12 +24,11 @@ def calculate(self) -> InsightActorsQueryOptionsResponse: if isinstance(self.source_runner, LifecycleQueryRunner): lifecycle_runner = cast(LifecycleQueryRunner, self.source_runner) return lifecycle_runner.to_actors_query_options() + elif isinstance(self.source_runner, TrendsQueryRunner): + trends_runner = cast(TrendsQueryRunner, self.source_runner) + return trends_runner.to_actors_query_options() - return InsightActorsQueryOptionsResponse( - day=None, - status=None, - interval=None, - ) + return InsightActorsQueryOptionsResponse(day=None, status=None, interval=None, breakdown=None, series=None) def _is_stale(self, cached_result_package): return True diff --git a/posthog/hogql_queries/insights/insight_actors_query_runner.py b/posthog/hogql_queries/insights/insight_actors_query_runner.py index 2de816aa3d30b..027c5573714a4 100644 --- a/posthog/hogql_queries/insights/insight_actors_query_runner.py +++ b/posthog/hogql_queries/insights/insight_actors_query_runner.py @@ -32,7 +32,12 @@ def to_query(self) -> ast.SelectQuery | ast.SelectUnionQuery: return paths_runner.to_actors_query() elif isinstance(self.source_runner, TrendsQueryRunner): trends_runner = cast(TrendsQueryRunner, self.source_runner) - return trends_runner.to_actors_query(self.query.day) + return trends_runner.to_actors_query( + time_frame=self.query.day, + series_index=self.query.series or 0, + breakdown_value=self.query.breakdown, + compare=self.query.compare, + ) elif isinstance(self.source_runner, RetentionQueryRunner): retention_runner = cast(RetentionQueryRunner, self.source_runner) return retention_runner.to_actors_query(interval=self.query.interval) diff --git a/posthog/hogql_queries/insights/trends/breakdown.py b/posthog/hogql_queries/insights/trends/breakdown.py index d719b4b1ca598..4106d45062d12 100644 --- a/posthog/hogql_queries/insights/trends/breakdown.py +++ b/posthog/hogql_queries/insights/trends/breakdown.py @@ -1,4 +1,4 @@ -from typing import Dict, List, Tuple +from typing import Dict, List, Optional, Tuple from posthog.hogql import ast from posthog.hogql.parser import parse_expr from posthog.hogql.timings import HogQLTimings @@ -28,6 +28,7 @@ class Breakdown: timings: HogQLTimings modifiers: HogQLQueryModifiers events_filter: ast.Expr + breakdown_values_override: Optional[List[str | int]] def __init__( self, @@ -38,6 +39,7 @@ def __init__( timings: HogQLTimings, modifiers: HogQLQueryModifiers, events_filter: ast.Expr, + breakdown_values_override: Optional[List[str | int]] = None, ): self.team = team self.query = query @@ -46,6 +48,7 @@ def __init__( self.timings = timings self.modifiers = modifiers self.events_filter = events_filter + self.breakdown_values_override = breakdown_values_override @cached_property def enabled(self) -> bool: @@ -209,19 +212,20 @@ def _breakdown_values_ast(self) -> ast.Array: @cached_property def _get_breakdown_values(self) -> List[str | int]: + # Used in the actors query + if self.breakdown_values_override is not None: + return self.breakdown_values_override + + if self.query.breakdownFilter is None: + return [] + with self.timings.measure("breakdown_values_query"): breakdown = BreakdownValues( team=self.team, event_name=series_event_name(self.series) or "", - breakdown_field=self.query.breakdownFilter.breakdown, # type: ignore - breakdown_type=self.query.breakdownFilter.breakdown_type, - query_date_range=self.query_date_range, events_filter=self.events_filter, chart_display_type=self._trends_display().display_type, - histogram_bin_count=self.query.breakdownFilter.breakdown_histogram_bin_count, - group_type_index=self.query.breakdownFilter.breakdown_group_type_index, - hide_other_aggregation=self.query.breakdownFilter.breakdown_hide_other_aggregation, - breakdown_limit=self.query.breakdownFilter.breakdown_limit, + breakdown_filter=self.query.breakdownFilter, ) return breakdown.get_breakdown_values() diff --git a/posthog/hogql_queries/insights/trends/breakdown_values.py b/posthog/hogql_queries/insights/trends/breakdown_values.py index 4d0f62b155f37..37f8551d4b276 100644 --- a/posthog/hogql_queries/insights/trends/breakdown_values.py +++ b/posthog/hogql_queries/insights/trends/breakdown_values.py @@ -4,9 +4,8 @@ from posthog.hogql.parser import parse_expr, parse_select from posthog.hogql.query import execute_hogql_query from posthog.hogql_queries.insights.trends.utils import get_properties_chain -from posthog.hogql_queries.utils.query_date_range import QueryDateRange from posthog.models.team.team import Team -from posthog.schema import ChartDisplayType +from posthog.schema import BreakdownFilter, BreakdownType, ChartDisplayType BREAKDOWN_OTHER_STRING_LABEL = "$$_posthog_breakdown_other_$$" BREAKDOWN_OTHER_NUMERIC_LABEL = 9007199254740991 # pow(2, 53) - 1, for JS compatibility @@ -18,8 +17,7 @@ class BreakdownValues: team: Team event_name: str breakdown_field: Union[str, float, List[Union[str, float]]] - breakdown_type: str - query_date_range: QueryDateRange + breakdown_type: BreakdownType events_filter: ast.Expr chart_display_type: ChartDisplayType histogram_bin_count: Optional[int] @@ -31,27 +29,28 @@ def __init__( self, team: Team, event_name: str, - breakdown_field: Union[str, float, List[Union[str, float]]], - query_date_range: QueryDateRange, - breakdown_type: str, events_filter: ast.Expr, chart_display_type: ChartDisplayType, - histogram_bin_count: Optional[int] = None, - group_type_index: Optional[int] = None, - hide_other_aggregation: Optional[bool] = False, - breakdown_limit: Optional[int] = None, + breakdown_filter: BreakdownFilter, ): self.team = team self.event_name = event_name - self.breakdown_field = breakdown_field - self.query_date_range = query_date_range - self.breakdown_type = breakdown_type + self.breakdown_field = breakdown_filter.breakdown # type: ignore + self.breakdown_type = breakdown_filter.breakdown_type # type: ignore self.events_filter = events_filter self.chart_display_type = chart_display_type - self.histogram_bin_count = int(histogram_bin_count) if histogram_bin_count is not None else None - self.group_type_index = int(group_type_index) if group_type_index is not None else None - self.hide_other_aggregation = hide_other_aggregation - self.breakdown_limit = breakdown_limit + self.histogram_bin_count = ( + int(breakdown_filter.breakdown_histogram_bin_count) + if breakdown_filter.breakdown_histogram_bin_count is not None + else None + ) + self.group_type_index = ( + int(breakdown_filter.breakdown_group_type_index) + if breakdown_filter.breakdown_group_type_index is not None + else None + ) + self.hide_other_aggregation = breakdown_filter.breakdown_hide_other_aggregation + self.breakdown_limit = breakdown_filter.breakdown_limit def get_breakdown_values(self) -> List[str | int]: if self.breakdown_type == "cohort": @@ -149,6 +148,8 @@ def get_breakdown_values(self) -> List[str | int]: values = [BREAKDOWN_NULL_STRING_LABEL if value in (None, "") else value for value in values] values.insert(0, BREAKDOWN_OTHER_STRING_LABEL) + breakdown_limit += 1 # Add one to the limit to account for the "other" value + return values[:breakdown_limit] def _to_bucketing_expression(self) -> ast.Expr: diff --git a/posthog/hogql_queries/insights/trends/query_builder.py b/posthog/hogql_queries/insights/trends/query_builder.py index b311d1cd89f34..ffbbff9406876 100644 --- a/posthog/hogql_queries/insights/trends/query_builder.py +++ b/posthog/hogql_queries/insights/trends/query_builder.py @@ -23,8 +23,6 @@ class TrendsQueryBuilder: series: EventsNode | ActionsNode timings: HogQLTimings modifiers: HogQLQueryModifiers - person_query_time_frame: Optional[str | int] - is_person_query: bool def __init__( self, @@ -34,7 +32,6 @@ def __init__( series: EventsNode | ActionsNode, timings: HogQLTimings, modifiers: HogQLQueryModifiers, - person_query_time_frame: Optional[str | int] = None, ): self.query = trends_query self.team = team @@ -42,28 +39,48 @@ def __init__( self.series = series self.timings = timings self.modifiers = modifiers - self.person_query_time_frame = person_query_time_frame - self.is_person_query = person_query_time_frame is not None def build_query(self) -> ast.SelectQuery | ast.SelectUnionQuery: - if self.is_person_query: - return self._get_events_subquery(True) + breakdown = self._breakdown(is_actors_query=False) + + events_query: ast.SelectQuery | ast.SelectUnionQuery if self._trends_display.should_aggregate_values(): - events_query = self._get_events_subquery(False) + events_query = self._get_events_subquery(False, is_actors_query=False, breakdown=breakdown) else: - date_subqueries = self._get_date_subqueries() - event_query = self._get_events_subquery(False) + date_subqueries = self._get_date_subqueries(breakdown=breakdown) + event_query = self._get_events_subquery(False, is_actors_query=False, breakdown=breakdown) events_query = ast.SelectUnionQuery(select_queries=[*date_subqueries, event_query]) - inner_select = self._inner_select_query(events_query) - full_query = self._outer_select_query(inner_select) + inner_select = self._inner_select_query(inner_query=events_query, breakdown=breakdown) + full_query = self._outer_select_query(inner_query=inner_select, breakdown=breakdown) return full_query - def _get_date_subqueries(self, ignore_breakdowns: bool = False) -> List[ast.SelectQuery]: - if not self._breakdown.enabled or ignore_breakdowns: + def build_actors_query( + self, time_frame: Optional[str | int] = None, breakdown_filter: Optional[str | int] = None + ) -> ast.SelectQuery | ast.SelectUnionQuery: + breakdown = self._breakdown(is_actors_query=True, breakdown_values_override=breakdown_filter) + + return parse_select( + """ + SELECT DISTINCT person_id + FROM {subquery} + """, + placeholders={ + "subquery": self._get_events_subquery( + False, + is_actors_query=True, + breakdown=breakdown, + breakdown_values_override=breakdown_filter, + actors_query_time_frame=time_frame, + ) + }, + ) + + def _get_date_subqueries(self, breakdown: Breakdown, ignore_breakdowns: bool = False) -> List[ast.SelectQuery]: + if not breakdown.enabled or ignore_breakdowns: return [ cast( ast.SelectQuery, @@ -127,13 +144,20 @@ def _get_date_subqueries(self, ignore_breakdowns: bool = False) -> List[ast.Sele """, placeholders={ **self.query_date_range.to_placeholders(), - **self._breakdown.placeholders(), + **breakdown.placeholders(), }, ), ) ] - def _get_events_subquery(self, no_modifications: Optional[bool]) -> ast.SelectQuery: + def _get_events_subquery( + self, + no_modifications: Optional[bool], + is_actors_query: bool, + breakdown: Breakdown, + breakdown_values_override: Optional[str | int] = None, + actors_query_time_frame: Optional[str | int] = None, + ) -> ast.SelectQuery: day_start = ast.Alias( alias="day_start", expr=ast.Call( @@ -141,6 +165,14 @@ def _get_events_subquery(self, no_modifications: Optional[bool]) -> ast.SelectQu ), ) + events_filter = self._events_filter( + ignore_breakdowns=False, + breakdown=breakdown, + is_actors_query=is_actors_query, + breakdown_values_override=breakdown_values_override, + actors_query_time_frame=actors_query_time_frame, + ) + default_query = cast( ast.SelectQuery, parse_select( @@ -152,7 +184,7 @@ def _get_events_subquery(self, no_modifications: Optional[bool]) -> ast.SelectQu WHERE {events_filter} """, placeholders={ - "events_filter": self._events_filter(ignore_breakdowns=False), + "events_filter": events_filter, "aggregation_operation": self._aggregation_operation.select_aggregation(), "sample": self._sample_value(), }, @@ -161,72 +193,87 @@ def _get_events_subquery(self, no_modifications: Optional[bool]) -> ast.SelectQu default_query.group_by = [] - if not self._trends_display.should_aggregate_values(): + if not self._trends_display.should_aggregate_values() and not is_actors_query: default_query.select.append(day_start) default_query.group_by.append(ast.Field(chain=["day_start"])) # TODO: Move this logic into the below branches when working on adding breakdown support for the person modal - if self.is_person_query: + if is_actors_query: default_query.select = [ast.Alias(alias="person_id", expr=ast.Field(chain=["e", "person", "id"]))] default_query.distinct = True - default_query.group_by = None + default_query.group_by = [] # No breakdowns and no complex series aggregation if ( - not self._breakdown.enabled + not breakdown.enabled and not self._aggregation_operation.requires_query_orchestration() and not self._aggregation_operation.aggregating_on_session_duration() ) or no_modifications is True: return default_query # Both breakdowns and complex series aggregation - elif self._breakdown.enabled and self._aggregation_operation.requires_query_orchestration(): + elif breakdown.enabled and self._aggregation_operation.requires_query_orchestration(): orchestrator = self._aggregation_operation.get_query_orchestrator( - events_where_clause=self._events_filter(ignore_breakdowns=False), + events_where_clause=events_filter, sample_value=self._sample_value(), ) - orchestrator.events_query_builder.append_select(self._breakdown.column_expr()) - orchestrator.events_query_builder.append_group_by(ast.Field(chain=["breakdown_value"])) + if is_actors_query: + orchestrator.events_query_builder.append_select( + ast.Alias(alias="person_id", expr=ast.Field(chain=["e", "person", "id"])) + ) + orchestrator.inner_select_query_builder.append_select(ast.Field(chain=["person_id"])) + orchestrator.parent_select_query_builder.append_select(ast.Field(chain=["person_id"])) + else: + orchestrator.events_query_builder.append_select(breakdown.column_expr()) + orchestrator.events_query_builder.append_group_by(ast.Field(chain=["breakdown_value"])) + + orchestrator.inner_select_query_builder.append_select(ast.Field(chain=["breakdown_value"])) + orchestrator.inner_select_query_builder.append_group_by(ast.Field(chain=["breakdown_value"])) - orchestrator.inner_select_query_builder.append_select(ast.Field(chain=["breakdown_value"])) - orchestrator.inner_select_query_builder.append_group_by(ast.Field(chain=["breakdown_value"])) + orchestrator.parent_select_query_builder.append_select(ast.Field(chain=["breakdown_value"])) - orchestrator.parent_select_query_builder.append_select(ast.Field(chain=["breakdown_value"])) if ( self._aggregation_operation.should_aggregate_values and not self._aggregation_operation.is_count_per_actor_variant() + and not is_actors_query ): orchestrator.parent_select_query_builder.append_group_by(ast.Field(chain=["breakdown_value"])) return orchestrator.build() # Breakdowns and session duration math property - elif self._breakdown.enabled and self._aggregation_operation.aggregating_on_session_duration(): + elif breakdown.enabled and self._aggregation_operation.aggregating_on_session_duration(): default_query.select = [ ast.Alias( alias="session_duration", expr=ast.Call(name="any", args=[ast.Field(chain=["session", "duration"])]) ), - self._breakdown.column_expr(), + breakdown.column_expr(), ] default_query.group_by.extend([ast.Field(chain=["session", "id"]), ast.Field(chain=["breakdown_value"])]) wrapper = self.session_duration_math_property_wrapper(default_query) + assert wrapper.group_by is not None - if not self._trends_display.should_aggregate_values(): + if not self._trends_display.should_aggregate_values() and not is_actors_query: default_query.select.append(day_start) default_query.group_by.append(ast.Field(chain=["day_start"])) wrapper.select.append(ast.Field(chain=["day_start"])) wrapper.group_by.append(ast.Field(chain=["day_start"])) - wrapper.select.append(ast.Field(chain=["breakdown_value"])) - wrapper.group_by.append(ast.Field(chain=["breakdown_value"])) + if is_actors_query: + default_query.select.append(ast.Alias(alias="person_id", expr=ast.Field(chain=["e", "person", "id"]))) + wrapper.select.append(ast.Field(chain=["person_id"])) + else: + wrapper.select.append(ast.Field(chain=["breakdown_value"])) + wrapper.group_by.append(ast.Field(chain=["breakdown_value"])) return wrapper # Just breakdowns - elif self._breakdown.enabled: - default_query.select.append(self._breakdown.column_expr()) - default_query.group_by.append(ast.Field(chain=["breakdown_value"])) + elif breakdown.enabled: + if not is_actors_query: + default_query.select.append(breakdown.column_expr()) + default_query.group_by.append(ast.Field(chain=["breakdown_value"])) # Just session duration math property elif self._aggregation_operation.aggregating_on_session_duration(): default_query.select = [ @@ -238,24 +285,39 @@ def _get_events_subquery(self, no_modifications: Optional[bool]) -> ast.SelectQu wrapper = self.session_duration_math_property_wrapper(default_query) - if not self._trends_display.should_aggregate_values(): + if not self._trends_display.should_aggregate_values() and not is_actors_query: + assert wrapper.group_by is not None + default_query.select.append(day_start) default_query.group_by.append(ast.Field(chain=["day_start"])) wrapper.select.append(ast.Field(chain=["day_start"])) wrapper.group_by.append(ast.Field(chain=["day_start"])) + if is_actors_query: + default_query.select.append(ast.Alias(alias="person_id", expr=ast.Field(chain=["e", "person", "id"]))) + wrapper.select.append(ast.Field(chain=["person_id"])) + return wrapper # Just complex series aggregation elif self._aggregation_operation.requires_query_orchestration(): - return self._aggregation_operation.get_query_orchestrator( - events_where_clause=self._events_filter(ignore_breakdowns=False), + orchestrator = self._aggregation_operation.get_query_orchestrator( + events_where_clause=events_filter, sample_value=self._sample_value(), - ).build() + ) + + if is_actors_query: + orchestrator.events_query_builder.append_select( + ast.Alias(alias="person_id", expr=ast.Field(chain=["e", "person", "id"])) + ) + orchestrator.inner_select_query_builder.append_select(ast.Field(chain=["person_id"])) + orchestrator.parent_select_query_builder.append_select(ast.Field(chain=["person_id"])) + + return orchestrator.build() return default_query - def _outer_select_query(self, inner_query: ast.SelectQuery) -> ast.SelectQuery: + def _outer_select_query(self, breakdown: Breakdown, inner_query: ast.SelectQuery) -> ast.SelectQuery: query = cast( ast.SelectQuery, parse_select( @@ -272,12 +334,14 @@ def _outer_select_query(self, inner_query: ast.SelectQuery) -> ast.SelectQuery: query = self._trends_display.modify_outer_query( outer_query=query, inner_query=inner_query, - dates_queries=ast.SelectUnionQuery(select_queries=self._get_date_subqueries(ignore_breakdowns=True)), + dates_queries=ast.SelectUnionQuery( + select_queries=self._get_date_subqueries(ignore_breakdowns=True, breakdown=breakdown) + ), ) query.order_by = [ast.OrderExpr(expr=ast.Call(name="sum", args=[ast.Field(chain=["count"])]), order="DESC")] - if self._breakdown.enabled: + if breakdown.enabled: query.select.append( ast.Alias( alias="breakdown_value", @@ -295,7 +359,9 @@ def _outer_select_query(self, inner_query: ast.SelectQuery) -> ast.SelectQuery: return query - def _inner_select_query(self, inner_query: ast.SelectQuery | ast.SelectUnionQuery) -> ast.SelectQuery: + def _inner_select_query( + self, breakdown: Breakdown, inner_query: ast.SelectQuery | ast.SelectUnionQuery + ) -> ast.SelectQuery: query = cast( ast.SelectQuery, parse_select( @@ -344,30 +410,37 @@ def _inner_select_query(self, inner_query: ast.SelectQuery | ast.SelectUnionQuer query.group_by.append(ast.Field(chain=["day_start"])) query.order_by.append(ast.OrderExpr(expr=ast.Field(chain=["day_start"]), order="ASC")) - if self._breakdown.enabled: + if breakdown.enabled: query.select.append(ast.Field(chain=["breakdown_value"])) query.group_by.append(ast.Field(chain=["breakdown_value"])) query.order_by.append(ast.OrderExpr(expr=ast.Field(chain=["breakdown_value"]), order="ASC")) if self._trends_display.should_wrap_inner_query(): - query = self._trends_display.wrap_inner_query(query, self._breakdown.enabled) - if self._breakdown.enabled: + query = self._trends_display.wrap_inner_query(query, breakdown.enabled) + if breakdown.enabled: query.select.append(ast.Field(chain=["breakdown_value"])) return query - def _events_filter(self, ignore_breakdowns: bool = False) -> ast.Expr: + def _events_filter( + self, + is_actors_query: bool, + breakdown: Breakdown | None, + ignore_breakdowns: bool = False, + breakdown_values_override: Optional[str | int] = None, + actors_query_time_frame: Optional[str | int] = None, + ) -> ast.Expr: series = self.series filters: List[ast.Expr] = [] # Dates - if self.is_person_query: + if is_actors_query and actors_query_time_frame is not None: to_start_of_time_frame = f"toStartOf{self.query_date_range.interval_name.capitalize()}" filters.append( ast.CompareOperation( left=ast.Call(name=to_start_of_time_frame, args=[ast.Field(chain=["timestamp"])]), op=ast.CompareOperationOp.Eq, - right=ast.Call(name="toDateTime", args=[ast.Constant(value=self.person_query_time_frame)]), + right=ast.Call(name="toDateTime", args=[ast.Constant(value=actors_query_time_frame)]), ) ) elif not self._aggregation_operation.requires_query_orchestration(): @@ -420,18 +493,16 @@ def _events_filter(self, ignore_breakdowns: bool = False) -> ast.Expr: filters.append(parse_expr("1 = 2")) # Breakdown - if not ignore_breakdowns: - if self._breakdown.enabled and not self._breakdown.is_histogram_breakdown: - breakdown_filter = self._breakdown.events_where_filter() + if not ignore_breakdowns and breakdown is not None: + if breakdown.enabled and not breakdown.is_histogram_breakdown: + breakdown_filter = breakdown.events_where_filter() if breakdown_filter is not None: filters.append(breakdown_filter) if len(filters) == 0: return ast.Constant(value=True) - elif len(filters) == 1: - return filters[0] - else: - return ast.And(exprs=filters) + + return ast.And(exprs=filters) def _sample_value(self) -> ast.RatioExpr: if self.query.samplingFactor is None: @@ -457,8 +528,7 @@ def session_duration_math_property_wrapper(self, default_query: ast.SelectQuery) query.group_by = [] return query - @cached_property - def _breakdown(self): + def _breakdown(self, is_actors_query: bool, breakdown_values_override: Optional[str | int] = None): return Breakdown( team=self.team, query=self.query, @@ -466,7 +536,13 @@ def _breakdown(self): query_date_range=self.query_date_range, timings=self.timings, modifiers=self.modifiers, - events_filter=self._events_filter(ignore_breakdowns=True), + events_filter=self._events_filter( + breakdown=None, # Passing in None because we know we dont actually need it + ignore_breakdowns=True, + is_actors_query=is_actors_query, + breakdown_values_override=breakdown_values_override, + ), + breakdown_values_override=[breakdown_values_override] if breakdown_values_override is not None else None, ) @cached_property diff --git a/posthog/hogql_queries/insights/trends/series_with_extras.py b/posthog/hogql_queries/insights/trends/series_with_extras.py index fb63a205f33d0..5dfafbe260768 100644 --- a/posthog/hogql_queries/insights/trends/series_with_extras.py +++ b/posthog/hogql_queries/insights/trends/series_with_extras.py @@ -4,6 +4,7 @@ class SeriesWithExtras: series: EventsNode | ActionsNode + series_order: int is_previous_period_series: Optional[bool] overriden_query: Optional[TrendsQuery] aggregate_values: Optional[bool] @@ -11,11 +12,13 @@ class SeriesWithExtras: def __init__( self, series: EventsNode | ActionsNode, + series_order: int, is_previous_period_series: Optional[bool], overriden_query: Optional[TrendsQuery], aggregate_values: Optional[bool], ): self.series = series + self.series_order = series_order self.is_previous_period_series = is_previous_period_series self.overriden_query = overriden_query self.aggregate_values = aggregate_values diff --git a/posthog/hogql_queries/insights/trends/test/__snapshots__/test_trends.ambr b/posthog/hogql_queries/insights/trends/test/__snapshots__/test_trends.ambr index aa5870b1810f5..926b7f2d8816f 100644 --- a/posthog/hogql_queries/insights/trends/test/__snapshots__/test_trends.ambr +++ b/posthog/hogql_queries/insights/trends/test/__snapshots__/test_trends.ambr @@ -685,12 +685,12 @@ WHERE equals(person.team_id, 2) GROUP BY person.id HAVING ifNull(equals(argMax(person.is_deleted, person.version), 0), 0))), 0)) SETTINGS optimize_aggregation_in_order=1) AS e__pdi__person ON equals(e__pdi.person_id, e__pdi__person.id) - WHERE and(equals(e.team_id, 2), equals(e.event, '$pageview'), and(or(ifNull(equals(e__pdi__person.properties___name, 'p1'), 0), ifNull(equals(e__pdi__person.properties___name, 'p2'), 0), ifNull(equals(e__pdi__person.properties___name, 'p3'), 0)), ifNull(in(e__pdi.person_id, - (SELECT cohortpeople.person_id AS person_id - FROM cohortpeople - WHERE and(equals(cohortpeople.team_id, 2), equals(cohortpeople.cohort_id, 28)) - GROUP BY cohortpeople.person_id, cohortpeople.cohort_id, cohortpeople.version - HAVING ifNull(greater(sum(cohortpeople.sign), 0), 0))), 0))) + WHERE and(equals(e.team_id, 2), and(equals(e.event, '$pageview'), and(or(ifNull(equals(e__pdi__person.properties___name, 'p1'), 0), ifNull(equals(e__pdi__person.properties___name, 'p2'), 0), ifNull(equals(e__pdi__person.properties___name, 'p3'), 0)), ifNull(in(e__pdi.person_id, + (SELECT cohortpeople.person_id AS person_id + FROM cohortpeople + WHERE and(equals(cohortpeople.team_id, 2), equals(cohortpeople.cohort_id, 28)) + GROUP BY cohortpeople.person_id, cohortpeople.cohort_id, cohortpeople.version + HAVING ifNull(greater(sum(cohortpeople.sign), 0), 0))), 0)))) GROUP BY value ORDER BY count DESC, value DESC LIMIT 25) diff --git a/posthog/hogql_queries/insights/trends/test/test_trends.py b/posthog/hogql_queries/insights/trends/test/test_trends.py index e8800f3c5aac4..648b0fa5bfd49 100644 --- a/posthog/hogql_queries/insights/trends/test/test_trends.py +++ b/posthog/hogql_queries/insights/trends/test/test_trends.py @@ -4679,7 +4679,7 @@ def test_breakdown_filtering_limit(self): ), self.team, ) - self.assertEqual(len(response), 25) # We fetch 25 to see if there are more ethan 20 values + self.assertEqual(len(response), 26) @also_test_with_materialized_columns(event_properties=["order"], person_properties=["name"]) def test_breakdown_with_person_property_filter(self): diff --git a/posthog/hogql_queries/insights/trends/test/test_trends_query_runner.py b/posthog/hogql_queries/insights/trends/test/test_trends_query_runner.py index ab83118a70112..91f6efb16ef7d 100644 --- a/posthog/hogql_queries/insights/trends/test/test_trends_query_runner.py +++ b/posthog/hogql_queries/insights/trends/test/test_trends_query_runner.py @@ -12,10 +12,13 @@ ActionsNode, BaseMathType, BreakdownFilter, + BreakdownItem, BreakdownType, ChartDisplayType, + CompareItem, CountPerActorMathType, DateRange, + DayItem, EventsNode, HogQLQueryModifiers, InCohortVia, @@ -24,12 +27,15 @@ TrendsFilter, TrendsQuery, ) + +from posthog.schema import Series as InsightActorsQuerySeries from posthog.test.base import ( APIBaseTest, ClickhouseTestMixin, _create_event, _create_person, also_test_with_materialized_columns, + flush_persons_and_events, ) @@ -47,7 +53,7 @@ class SeriesTestData: @override_settings(IN_UNIT_TESTING=True) -class TestQuery(ClickhouseTestMixin, APIBaseTest): +class TestTrendsQueryRunner(ClickhouseTestMixin, APIBaseTest): default_date_from = "2020-01-09" default_date_to = "2020-01-19" @@ -59,9 +65,7 @@ def _create_events(self, data: List[SeriesTestData]): for key, value in person.properties.items(): if key not in properties_to_create: - if isinstance(value, str): - type = "String" - elif isinstance(value, bool): + if isinstance(value, bool): type = "Boolean" elif isinstance(value, int): type = "Numeric" @@ -164,10 +168,10 @@ def _create_query_runner( date_to: str, interval: IntervalType, series: Optional[List[EventsNode | ActionsNode]], - trends_filters: Optional[TrendsFilter], - breakdown: Optional[BreakdownFilter], - filter_test_accounts: Optional[bool], - hogql_modifiers: Optional[HogQLQueryModifiers], + trends_filters: Optional[TrendsFilter] = None, + breakdown: Optional[BreakdownFilter] = None, + filter_test_accounts: Optional[bool] = None, + hogql_modifiers: Optional[HogQLQueryModifiers] = None, ) -> TrendsQueryRunner: query_series: List[EventsNode | ActionsNode] = [EventsNode(event="$pageview")] if series is None else series query = TrendsQuery( @@ -1050,7 +1054,17 @@ def test_breakdown_values_limit(self): BreakdownFilter(breakdown="breakdown_value", breakdown_type=BreakdownType.event), ) - assert len(response.results) == 25 + self.assertEqual(len(response.results), 26) + + response = self._run_trends_query( + "2020-01-09", + "2020-01-20", + IntervalType.day, + [EventsNode(event="$pageview")], + TrendsFilter(display=ChartDisplayType.ActionsLineGraph), + BreakdownFilter(breakdown="breakdown_value", breakdown_type=BreakdownType.event, breakdown_limit=10), + ) + self.assertEqual(len(response.results), 11) def test_breakdown_values_world_map_limit(self): PropertyDefinition.objects.create(team=self.team, name="breakdown_value", property_type="String") @@ -1354,3 +1368,233 @@ def test_should_throw_exception(self, patch_sync_execute): str(e.exception), "Error thrown inside thread", ) + + def test_to_actors_query_options(self): + self._create_test_events() + flush_persons_and_events() + + runner = self._create_query_runner( + "2020-01-09", + "2020-01-20", + IntervalType.day, + [EventsNode(event="$pageview")], + None, + None, + ) + response = runner.to_actors_query_options() + + assert response.day == [ + DayItem(label="2020-01-09", value="2020-01-09"), + DayItem(label="2020-01-10", value="2020-01-10"), + DayItem(label="2020-01-11", value="2020-01-11"), + DayItem(label="2020-01-12", value="2020-01-12"), + DayItem(label="2020-01-13", value="2020-01-13"), + DayItem(label="2020-01-14", value="2020-01-14"), + DayItem(label="2020-01-15", value="2020-01-15"), + DayItem(label="2020-01-16", value="2020-01-16"), + DayItem(label="2020-01-17", value="2020-01-17"), + DayItem(label="2020-01-18", value="2020-01-18"), + DayItem(label="2020-01-19", value="2020-01-19"), + DayItem(label="2020-01-20", value="2020-01-20"), + ] + + assert response.breakdown is None + + assert response.series == [InsightActorsQuerySeries(label="$pageview", value=0)] + + assert response.compare is None + + def test_to_actors_query_options_compare(self): + self._create_test_events() + flush_persons_and_events() + + runner = self._create_query_runner( + "2020-01-09", + "2020-01-20", + IntervalType.day, + [EventsNode(event="$pageview")], + TrendsFilter(compare=True), + None, + ) + response = runner.to_actors_query_options() + + assert response.day == [ + DayItem(label="2020-01-09", value="2020-01-09"), + DayItem(label="2020-01-10", value="2020-01-10"), + DayItem(label="2020-01-11", value="2020-01-11"), + DayItem(label="2020-01-12", value="2020-01-12"), + DayItem(label="2020-01-13", value="2020-01-13"), + DayItem(label="2020-01-14", value="2020-01-14"), + DayItem(label="2020-01-15", value="2020-01-15"), + DayItem(label="2020-01-16", value="2020-01-16"), + DayItem(label="2020-01-17", value="2020-01-17"), + DayItem(label="2020-01-18", value="2020-01-18"), + DayItem(label="2020-01-19", value="2020-01-19"), + DayItem(label="2020-01-20", value="2020-01-20"), + ] + + assert response.breakdown is None + + assert response.series == [InsightActorsQuerySeries(label="$pageview", value=0)] + + assert response.compare == [ + CompareItem(label="Current", value="current"), + CompareItem(label="Previous", value="previous"), + ] + + def test_to_actors_query_options_multiple_series(self): + self._create_test_events() + flush_persons_and_events() + + runner = self._create_query_runner( + "2020-01-09", + "2020-01-20", + IntervalType.day, + [EventsNode(event="$pageview"), EventsNode(event="$pageleave")], + None, + None, + ) + response = runner.to_actors_query_options() + + assert response.series == [ + InsightActorsQuerySeries(label="$pageview", value=0), + InsightActorsQuerySeries(label="$pageleave", value=1), + ] + + def test_to_actors_query_options_breakdowns(self): + self._create_test_events() + flush_persons_and_events() + + runner = self._create_query_runner( + "2020-01-09", + "2020-01-20", + IntervalType.day, + [EventsNode(event="$pageview")], + None, + BreakdownFilter(breakdown_type=BreakdownType.event, breakdown="$browser"), + ) + + response = runner.to_actors_query_options() + + assert response.series == [InsightActorsQuerySeries(label="$pageview", value=0)] + + assert response.breakdown == [ + # BreakdownItem(label="Other", value="$$_posthog_breakdown_other_$$"), # TODO: uncomment when "other" shows correct results + BreakdownItem(label="Chrome", value="Chrome"), + BreakdownItem(label="Firefox", value="Firefox"), + BreakdownItem(label="Safari", value="Safari"), + BreakdownItem(label="Edge", value="Edge"), + ] + + def test_to_actors_query_options_breakdowns_boolean(self): + self._create_test_events() + flush_persons_and_events() + + runner = self._create_query_runner( + "2020-01-09", + "2020-01-20", + IntervalType.day, + [EventsNode(event="$pageview")], + None, + BreakdownFilter(breakdown_type=BreakdownType.event, breakdown="bool_field"), + ) + + response = runner.to_actors_query_options() + + assert response.series == [InsightActorsQuerySeries(label="$pageview", value=0)] + + assert response.breakdown == [ + # BreakdownItem(label="Other", value="$$_posthog_breakdown_other_$$"), # TODO: Add when "Other" works + BreakdownItem(label="true", value=1), + BreakdownItem(label="false", value=0), + ] + + def test_to_actors_query_options_breakdowns_histogram(self): + self._create_test_events() + flush_persons_and_events() + + runner = self._create_query_runner( + "2020-01-09", + "2020-01-20", + IntervalType.day, + [EventsNode(event="$pageview")], + None, + BreakdownFilter( + breakdown_type=BreakdownType.event, + breakdown="prop", + breakdown_histogram_bin_count=4, + ), + ) + + response = runner.to_actors_query_options() + + assert response.series == [InsightActorsQuerySeries(label="$pageview", value=0)] + + assert response.breakdown == [ + BreakdownItem(label="[10.0,17.5]", value="[10.0,17.5]"), + BreakdownItem(label="[17.5,25.0]", value="[17.5,25.0]"), + BreakdownItem(label="[25.0,32.5]", value="[25.0,32.5]"), + BreakdownItem(label="[32.5,40.01]", value="[32.5,40.01]"), + BreakdownItem(label='["",""]', value='["",""]'), + ] + + def test_to_actors_query_options_breakdowns_cohort(self): + self._create_test_events() + flush_persons_and_events() + + cohort = Cohort.objects.create( + team=self.team, + groups=[ + { + "properties": [ + { + "key": "name", + "value": "p1", + "type": "person", + } + ] + } + ], + name="cohort", + ) + cohort.calculate_people_ch(pending_version=0) + + runner = self._create_query_runner( + "2020-01-09", + "2020-01-20", + IntervalType.day, + [EventsNode(event="$pageview")], + None, + BreakdownFilter(breakdown_type=BreakdownType.cohort, breakdown=[cohort.pk]), + ) + + response = runner.to_actors_query_options() + + assert response.series == [InsightActorsQuerySeries(label="$pageview", value=0)] + + assert response.breakdown == [BreakdownItem(label="cohort", value=cohort.pk)] + + def test_to_actors_query_options_breakdowns_hogql(self): + self._create_test_events() + flush_persons_and_events() + + runner = self._create_query_runner( + "2020-01-09", + "2020-01-20", + IntervalType.day, + [EventsNode(event="$pageview")], + None, + BreakdownFilter(breakdown_type=BreakdownType.hogql, breakdown="properties.$browser"), + ) + + response = runner.to_actors_query_options() + + assert response.series == [InsightActorsQuerySeries(label="$pageview", value=0)] + + assert response.breakdown == [ + # BreakdownItem(label="Other", value="$$_posthog_breakdown_other_$$"), # TODO: uncomment when "other" shows correct results + BreakdownItem(label="Chrome", value="Chrome"), + BreakdownItem(label="Firefox", value="Firefox"), + BreakdownItem(label="Safari", value="Safari"), + BreakdownItem(label="Edge", value="Edge"), + ] diff --git a/posthog/hogql_queries/insights/trends/trends_query_runner.py b/posthog/hogql_queries/insights/trends/trends_query_runner.py index 227c7386675e8..85d0c36af0d5f 100644 --- a/posthog/hogql_queries/insights/trends/trends_query_runner.py +++ b/posthog/hogql_queries/insights/trends/trends_query_runner.py @@ -5,6 +5,8 @@ from operator import itemgetter import threading from typing import List, Optional, Any, Dict +from dateutil import parser +from dateutil.relativedelta import relativedelta from django.conf import settings from django.utils.timezone import datetime @@ -20,6 +22,8 @@ from posthog.hogql.query import execute_hogql_query from posthog.hogql.timings import HogQLTimings from posthog.hogql_queries.insights.trends.breakdown_values import ( + BREAKDOWN_NULL_NUMERIC_LABEL, + BREAKDOWN_NULL_STRING_LABEL, BREAKDOWN_OTHER_NUMERIC_LABEL, BREAKDOWN_OTHER_STRING_LABEL, ) @@ -39,11 +43,17 @@ from posthog.models.property_definition import PropertyDefinition from posthog.schema import ( ActionsNode, + BreakdownItem, ChartDisplayType, + Compare, + CompareItem, + DayItem, EventsNode, HogQLQueryResponse, InCohortVia, + InsightActorsQueryOptionsResponse, QueryTiming, + Series, TrendsQuery, TrendsQueryResponse, HogQLQueryModifiers, @@ -110,34 +120,118 @@ def to_query(self) -> List[ast.SelectQuery | ast.SelectUnionQuery]: # type: ign return queries - def to_actors_query(self, time_frame: Optional[str | int]) -> ast.SelectQuery | ast.SelectUnionQuery: # type: ignore - queries = [] + def to_actors_query( # type: ignore + self, + time_frame: Optional[str | int], + series_index: int, + breakdown_value: Optional[str | int] = None, + compare: Optional[Compare] = None, + ) -> ast.SelectQuery | ast.SelectUnionQuery: with self.timings.measure("trends_to_actors_query"): - for series in self.series: - if not series.is_previous_period_series: - query_date_range = self.query_date_range - else: - query_date_range = self.query_previous_date_range + series = self.query.series[series_index] - query_builder = TrendsQueryBuilder( - trends_query=series.overriden_query or self.query, - team=self.team, - query_date_range=query_date_range, - series=series.series, - timings=self.timings, - modifiers=self.modifiers, - person_query_time_frame=time_frame, - ) - queries.append(query_builder.build_query()) + if compare == Compare.previous: + query_date_range = self.query_previous_date_range - select_queries: List[ast.SelectQuery] = [] - for query in queries: - if isinstance(query, ast.SelectUnionQuery): - select_queries.extend(query.select_queries) + delta_mappings = self.query_previous_date_range.date_from_delta_mappings() + if delta_mappings is not None and time_frame is not None and isinstance(time_frame, str): + relative_delta = relativedelta(**delta_mappings) # type: ignore + parsed_dt = parser.isoparse(time_frame) + parse_dt_with_relative_delta = parsed_dt - relative_delta + time_frame = parse_dt_with_relative_delta.strftime("%Y-%m-%d") else: - select_queries.append(query) + query_date_range = self.query_date_range + + query_builder = TrendsQueryBuilder( + trends_query=self.query, + team=self.team, + query_date_range=query_date_range, + series=series, + timings=self.timings, + modifiers=self.modifiers, + ) + + query = query_builder.build_actors_query(time_frame=time_frame, breakdown_filter=breakdown_value) - return ast.SelectUnionQuery(select_queries=select_queries) + return query + + def to_actors_query_options(self) -> InsightActorsQueryOptionsResponse: + res_breakdown: List[BreakdownItem] | None = None + res_series: List[Series] = [] + res_compare: List[CompareItem] | None = None + + # Days + res_days: List[DayItem] = [DayItem(label=day, value=day) for day in self.query_date_range.all_values()] + + # Series + for index, series in enumerate(self.query.series): + series_label = self.series_event(series) + res_series.append(Series(label="All events" if series_label is None else series_label, value=index)) + + # Compare + if self.query.trendsFilter is not None and self.query.trendsFilter.compare: + res_compare = [ + CompareItem(label="Current", value="current"), + CompareItem(label="Previous", value="previous"), + ] + + # Breakdowns + for series in self.query.series: + # TODO: Work out if we will have issues only getting breakdown values for + # the "current" period and not "previous" period for when "compare" is turned on + query_date_range = self.query_date_range + + query_builder = TrendsQueryBuilder( + trends_query=self.query, + team=self.team, + query_date_range=query_date_range, + series=series, + timings=self.timings, + modifiers=self.modifiers, + ) + + breakdown = query_builder._breakdown(is_actors_query=False) + if not breakdown.enabled: + break + + is_boolean_breakdown = self._is_breakdown_field_boolean() + is_histogram_breakdown = breakdown.is_histogram_breakdown + breakdown_values: List[str | int] + res_breakdown = [] + + if is_histogram_breakdown: + buckets = breakdown._get_breakdown_histogram_buckets() + breakdown_values = [f"[{t[0]},{t[1]}]" for t in buckets] + breakdown_values.append('["",""]') + else: + breakdown_values = breakdown._get_breakdown_values + + for value in breakdown_values: + if self.query.breakdownFilter is not None and self.query.breakdownFilter.breakdown_type == "cohort": + cohort_name = "all users" if str(value) == "0" else Cohort.objects.get(pk=value).name + label = cohort_name + value = value + elif value == BREAKDOWN_OTHER_STRING_LABEL or value == BREAKDOWN_OTHER_NUMERIC_LABEL: + # label = "Other" + # value = BREAKDOWN_OTHER_STRING_LABEL + continue # TODO: Add support for "other" breakdowns + elif value == BREAKDOWN_NULL_STRING_LABEL or value == BREAKDOWN_NULL_NUMERIC_LABEL: + # label = "Null" + # value = BREAKDOWN_NULL_STRING_LABEL + continue # TODO: Add support for "null" breakdowns + elif is_boolean_breakdown: + label = self._convert_boolean(value) + else: + label = str(value) + + item = BreakdownItem(label=label, value=value) + + if item not in res_breakdown: + res_breakdown.append(item) + + return InsightActorsQueryOptionsResponse( + series=res_series, breakdown=res_breakdown, day=res_days, compare=res_compare + ) def calculate(self): queries = self.to_query() @@ -184,7 +278,7 @@ def run(index: int, query: ast.SelectQuery | ast.SelectUnionQuery, is_parallel: # This exists so that we're not spawning threads during unit tests. We can't do # this right now due to the lack of multithreaded support of Django - if settings.IN_UNIT_TESTING: # type: ignore + if settings.IN_UNIT_TESTING: for index, query in enumerate(queries): run(index, query, False) elif len(queries) == 1: @@ -264,9 +358,10 @@ def get_value(name: str, val: Any): "label": "All events" if series_label is None else series_label, "filter": self._query_to_filter(), "action": { # TODO: Populate missing props in `action` + "days": self.query_date_range.all_values(), "id": series_label, "type": "events", - "order": 0, + "order": series.series_order, "name": series_label or "All events", "custom_name": None, "math": series.series.math, @@ -300,9 +395,10 @@ def get_value(name: str, val: Any): "label": "All events" if series_label is None else series_label, "filter": self._query_to_filter(), "action": { # TODO: Populate missing props in `action` + "days": self.query_date_range.all_values(), "id": series_label, "type": "events", - "order": 0, + "order": series.series_order, "name": series_label or "All events", "custom_name": None, "math": series.series.math, @@ -420,12 +516,13 @@ def update_hogql_modifiers(self) -> None: def setup_series(self) -> List[SeriesWithExtras]: series_with_extras = [ SeriesWithExtras( - series, - None, - None, - self._trends_display.should_aggregate_values(), + series=series, + series_order=index, + is_previous_period_series=None, + overriden_query=None, + aggregate_values=self._trends_display.should_aggregate_values(), ) - for series in self.query.series + for index, series in enumerate(self.query.series) ] if ( @@ -447,6 +544,7 @@ def setup_series(self) -> List[SeriesWithExtras]: updated_series.append( SeriesWithExtras( series=series.series, + series_order=series.series_order, is_previous_period_series=series.is_previous_period_series, overriden_query=copied_query, aggregate_values=self._trends_display.should_aggregate_values(), @@ -460,6 +558,7 @@ def setup_series(self) -> List[SeriesWithExtras]: updated_series.append( SeriesWithExtras( series=series.series, + series_order=series.series_order, is_previous_period_series=False, overriden_query=series.overriden_query, aggregate_values=self._trends_display.should_aggregate_values(), @@ -468,6 +567,7 @@ def setup_series(self) -> List[SeriesWithExtras]: updated_series.append( SeriesWithExtras( series=series.series, + series_order=series.series_order, is_previous_period_series=True, overriden_query=series.overriden_query, aggregate_values=self._trends_display.should_aggregate_values(), @@ -506,6 +606,9 @@ def apply_formula(self, formula: str, results: List[Dict[str, Any]]) -> List[Dic return [new_result] def _is_breakdown_field_boolean(self): + if not self.query.breakdownFilter or not self.query.breakdownFilter.breakdown_type: + return False + if ( self.query.breakdownFilter.breakdown_type == "hogql" or self.query.breakdownFilter.breakdown_type == "cohort" diff --git a/posthog/hogql_queries/utils/query_previous_period_date_range.py b/posthog/hogql_queries/utils/query_previous_period_date_range.py index d4761fb34e855..acd1b648d8731 100644 --- a/posthog/hogql_queries/utils/query_previous_period_date_range.py +++ b/posthog/hogql_queries/utils/query_previous_period_date_range.py @@ -30,14 +30,16 @@ def __init__( def date_from_delta_mappings(self) -> Dict[str, int] | None: if self._date_range and isinstance(self._date_range.date_from, str) and self._date_range.date_from != "all": - delta_mapping = relative_date_parse_with_delta_mapping( - self._date_range.date_from, - self._team.timezone_info, - now=self.now_with_timezone, - )[1] - return delta_mapping + date_from = self._date_range.date_from + else: + date_from = "-7d" - return None + delta_mapping = relative_date_parse_with_delta_mapping( + date_from, + self._team.timezone_info, + now=self.now_with_timezone, + )[1] + return delta_mapping def date_to_delta_mappings(self) -> Dict[str, int] | None: if self._date_range and self._date_range.date_to: diff --git a/posthog/schema.py b/posthog/schema.py index 24ce85784c1a3..de99275188527 100644 --- a/posthog/schema.py +++ b/posthog/schema.py @@ -53,6 +53,10 @@ class BreakdownType(str, Enum): hogql = "hogql" +class BreakdownValueInt(RootModel[int]): + root: int + + class ChartAxis(BaseModel): model_config = ConfigDict( extra="forbid", @@ -319,6 +323,27 @@ class HogQLQueryModifiers(BaseModel): personsOnEventsMode: Optional[PersonsOnEventsMode] = None +class Compare(str, Enum): + current = "current" + previous = "previous" + + +class BreakdownItem(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + label: str + value: Union[str, int] + + +class CompareItem(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + label: str + value: str + + class DayItem(BaseModel): model_config = ConfigDict( extra="forbid", @@ -335,6 +360,14 @@ class IntervalItem(BaseModel): value: int = Field(..., description="An interval selected out of available intervals in source query") +class Series(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + label: str + value: int + + class StatusItem(BaseModel): model_config = ConfigDict( extra="forbid", @@ -347,8 +380,11 @@ class InsightActorsQueryOptionsResponse(BaseModel): model_config = ConfigDict( extra="forbid", ) + breakdown: Optional[List[BreakdownItem]] = None + compare: Optional[List[CompareItem]] = None day: Optional[List[DayItem]] = None interval: Optional[List[IntervalItem]] = None + series: Optional[List[Series]] = None status: Optional[List[StatusItem]] = None @@ -550,8 +586,11 @@ class QueryResponseAlternative5(BaseModel): model_config = ConfigDict( extra="forbid", ) + breakdown: Optional[List[BreakdownItem]] = None + compare: Optional[List[CompareItem]] = None day: Optional[List[DayItem]] = None interval: Optional[List[IntervalItem]] = None + series: Optional[List[Series]] = None status: Optional[List[StatusItem]] = None @@ -2276,12 +2315,15 @@ class InsightActorsQuery(BaseModel): model_config = ConfigDict( extra="forbid", ) + breakdown: Optional[Union[str, int]] = None + compare: Optional[Compare] = None day: Optional[Union[str, int]] = None interval: Optional[int] = Field( default=None, description="An interval selected out of available intervals in source query" ) kind: Literal["InsightActorsQuery"] = "InsightActorsQuery" response: Optional[ActorsQueryResponse] = None + series: Optional[int] = None source: Union[TrendsQuery, FunnelsQuery, RetentionQuery, PathsQuery, StickinessQuery, LifecycleQuery] = Field( ..., discriminator="kind" )