Skip to content

Commit

Permalink
refactor(querying): Consolidate PersonOnEventsMode and `PersonsOnEv…
Browse files Browse the repository at this point in the history
…entsMode` (#21628)

* Consolidate `PersonOnEventsMode` and `PersonsOnEventsMode`

* Fix multi-line imports too

* Fix member casing

* Fix typing

* Fix some more and update baseline

* Fix `PersonsOnEventsMode` member capitalization
  • Loading branch information
Twixes authored Apr 19, 2024
1 parent 688cc36 commit b7add1d
Show file tree
Hide file tree
Showing 35 changed files with 166 additions and 185 deletions.
12 changes: 6 additions & 6 deletions ee/clickhouse/models/test/test_cohort.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from posthog.models.property.util import parse_prop_grouped_clauses
from posthog.models.team import Team
from posthog.queries.util import PersonPropertiesMode
from posthog.schema import PersonsOnEventsMode
from posthog.test.base import (
BaseTest,
ClickhouseTestMixin,
Expand All @@ -25,7 +26,6 @@
snapshot_clickhouse_insert_cohortpeople_queries,
snapshot_clickhouse_queries,
)
from posthog.utils import PersonOnEventsMode


def _create_action(**kwargs):
Expand Down Expand Up @@ -145,7 +145,7 @@ def test_prop_cohort_basic_action(self):
team_id=self.team.pk,
property_group=filter.property_groups,
person_properties_mode=PersonPropertiesMode.USING_SUBQUERY
if self.team.person_on_events_mode == PersonOnEventsMode.DISABLED
if self.team.person_on_events_mode == PersonsOnEventsMode.disabled
else PersonPropertiesMode.DIRECT_ON_EVENTS,
hogql_context=filter.hogql_context,
)
Expand Down Expand Up @@ -200,7 +200,7 @@ def test_prop_cohort_basic_event_days(self):
team_id=self.team.pk,
property_group=filter.property_groups,
person_properties_mode=PersonPropertiesMode.USING_SUBQUERY
if self.team.person_on_events_mode == PersonOnEventsMode.DISABLED
if self.team.person_on_events_mode == PersonsOnEventsMode.disabled
else PersonPropertiesMode.DIRECT_ON_EVENTS,
hogql_context=filter.hogql_context,
)
Expand All @@ -225,7 +225,7 @@ def test_prop_cohort_basic_event_days(self):
team_id=self.team.pk,
property_group=filter.property_groups,
person_properties_mode=PersonPropertiesMode.USING_SUBQUERY
if self.team.person_on_events_mode == PersonOnEventsMode.DISABLED
if self.team.person_on_events_mode == PersonsOnEventsMode.disabled
else PersonPropertiesMode.DIRECT_ON_EVENTS,
hogql_context=filter.hogql_context,
)
Expand Down Expand Up @@ -276,7 +276,7 @@ def test_prop_cohort_basic_action_days(self):
team_id=self.team.pk,
property_group=filter.property_groups,
person_properties_mode=PersonPropertiesMode.USING_SUBQUERY
if self.team.person_on_events_mode == PersonOnEventsMode.DISABLED
if self.team.person_on_events_mode == PersonsOnEventsMode.disabled
else PersonPropertiesMode.DIRECT_ON_EVENTS,
hogql_context=filter.hogql_context,
)
Expand All @@ -297,7 +297,7 @@ def test_prop_cohort_basic_action_days(self):
team_id=self.team.pk,
property_group=filter.property_groups,
person_properties_mode=PersonPropertiesMode.USING_SUBQUERY
if self.team.person_on_events_mode == PersonOnEventsMode.DISABLED
if self.team.person_on_events_mode == PersonsOnEventsMode.disabled
else PersonPropertiesMode.DIRECT_ON_EVENTS,
hogql_context=filter.hogql_context,
)
Expand Down
4 changes: 2 additions & 2 deletions ee/clickhouse/queries/enterprise_cohort_query.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
validate_seq_date_more_recent_than_date,
)
from posthog.queries.util import PersonPropertiesMode
from posthog.utils import PersonOnEventsMode
from posthog.schema import PersonsOnEventsMode


def check_negation_clause(prop: PropertyGroup) -> Tuple[bool, bool]:
Expand Down Expand Up @@ -319,7 +319,7 @@ def _get_sequence_query(self) -> Tuple[str, Dict[str, Any], str]:

event_param_name = f"{self._cohort_pk}_event_ids"

if self.should_pushdown_persons and self._person_on_events_mode != PersonOnEventsMode.DISABLED:
if self.should_pushdown_persons and self._person_on_events_mode != PersonsOnEventsMode.disabled:
person_prop_query, person_prop_params = self._get_prop_groups(
self._inner_property_groups,
person_properties_mode=PersonPropertiesMode.DIRECT_ON_EVENTS,
Expand Down
4 changes: 2 additions & 2 deletions ee/clickhouse/queries/event_query.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
from posthog.models.property import PropertyName
from posthog.models.team import Team
from posthog.queries.event_query.event_query import EventQuery
from posthog.utils import PersonOnEventsMode
from posthog.schema import PersonsOnEventsMode


class EnterpriseEventQuery(EventQuery):
Expand All @@ -37,7 +37,7 @@ def __init__(
extra_event_properties: List[PropertyName] = [],
extra_person_fields: List[ColumnName] = [],
override_aggregate_users_by_distinct_id: Optional[bool] = None,
person_on_events_mode: PersonOnEventsMode = PersonOnEventsMode.DISABLED,
person_on_events_mode: PersonsOnEventsMode = PersonsOnEventsMode.disabled,
**kwargs,
) -> None:
super().__init__(
Expand Down
17 changes: 9 additions & 8 deletions ee/clickhouse/queries/funnels/funnel_correlation.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@
from posthog.queries.person_distinct_id_query import get_team_distinct_ids_query
from posthog.queries.person_query import PersonQuery
from posthog.queries.util import correct_result_for_sampling
from posthog.utils import PersonOnEventsMode, generate_short_id
from posthog.schema import PersonsOnEventsMode
from posthog.utils import generate_short_id


class EventDefinition(TypedDict):
Expand Down Expand Up @@ -155,7 +156,7 @@ def __init__(
def properties_to_include(self) -> List[str]:
props_to_include = []
if (
self._team.person_on_events_mode != PersonOnEventsMode.DISABLED
self._team.person_on_events_mode != PersonsOnEventsMode.disabled
and self._filter.correlation_type == FunnelCorrelationType.PROPERTIES
):
# When dealing with properties, make sure funnel response comes with properties
Expand Down Expand Up @@ -435,7 +436,7 @@ def get_properties_query(self) -> Tuple[str, Dict[str, Any]]:
return query, params

def _get_aggregation_target_join_query(self) -> str:
if self._team.person_on_events_mode == PersonOnEventsMode.PERSON_ID_NO_OVERRIDE_PROPERTIES_ON_EVENTS:
if self._team.person_on_events_mode == PersonsOnEventsMode.person_id_no_override_properties_on_events:
aggregation_person_join = f"""
JOIN funnel_actors as actors
ON event.person_id = actors.actor_id
Expand Down Expand Up @@ -502,7 +503,7 @@ def _get_events_join_query(self) -> str:

def _get_aggregation_join_query(self):
if self._filter.aggregation_group_type_index is None:
if self._team.person_on_events_mode != PersonOnEventsMode.DISABLED and groups_on_events_querying_enabled():
if self._team.person_on_events_mode != PersonsOnEventsMode.disabled and groups_on_events_querying_enabled():
return "", {}

person_query, person_query_params = PersonQuery(
Expand All @@ -522,7 +523,7 @@ def _get_aggregation_join_query(self):
return GroupsJoinQuery(self._filter, self._team.pk, join_key="funnel_actors.actor_id").get_join_query()

def _get_properties_prop_clause(self):
if self._team.person_on_events_mode != PersonOnEventsMode.DISABLED and groups_on_events_querying_enabled():
if self._team.person_on_events_mode != PersonsOnEventsMode.disabled and groups_on_events_querying_enabled():
group_properties_field = f"group{self._filter.aggregation_group_type_index}_properties"
aggregation_properties_alias = (
"person_properties" if self._filter.aggregation_group_type_index is None else group_properties_field
Expand All @@ -549,20 +550,20 @@ def _get_properties_prop_clause(self):
param_name = f"property_name_{index}"
if self._filter.aggregation_group_type_index is not None:
expression, _ = get_property_string_expr(
"groups" if self._team.person_on_events_mode == PersonOnEventsMode.DISABLED else "events",
"groups" if self._team.person_on_events_mode == PersonsOnEventsMode.disabled else "events",
property_name,
f"%({param_name})s",
aggregation_properties_alias,
materialised_table_column=aggregation_properties_alias,
)
else:
expression, _ = get_property_string_expr(
"person" if self._team.person_on_events_mode == PersonOnEventsMode.DISABLED else "events",
"person" if self._team.person_on_events_mode == PersonsOnEventsMode.disabled else "events",
property_name,
f"%({param_name})s",
aggregation_properties_alias,
materialised_table_column=aggregation_properties_alias
if self._team.person_on_events_mode != PersonOnEventsMode.DISABLED
if self._team.person_on_events_mode != PersonsOnEventsMode.disabled
else "properties",
)
person_property_params[param_name] = property_name
Expand Down
6 changes: 3 additions & 3 deletions ee/clickhouse/queries/groups_join_query.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from posthog.models.property.util import parse_prop_grouped_clauses
from posthog.models.team.team import groups_on_events_querying_enabled
from posthog.queries.util import PersonPropertiesMode
from posthog.utils import PersonOnEventsMode
from posthog.schema import PersonsOnEventsMode


class GroupsJoinQuery:
Expand All @@ -27,7 +27,7 @@ def __init__(
team_id: int,
column_optimizer: Optional[EnterpriseColumnOptimizer] = None,
join_key: Optional[str] = None,
person_on_events_mode: PersonOnEventsMode = PersonOnEventsMode.DISABLED,
person_on_events_mode: PersonsOnEventsMode = PersonsOnEventsMode.disabled,
) -> None:
self._filter = filter
self._team_id = team_id
Expand All @@ -38,7 +38,7 @@ def __init__(
def get_join_query(self) -> Tuple[str, Dict]:
join_queries, params = [], {}

if self._person_on_events_mode != PersonOnEventsMode.DISABLED and groups_on_events_querying_enabled():
if self._person_on_events_mode != PersonsOnEventsMode.disabled and groups_on_events_querying_enabled():
return "", {}

for group_type_index in self._column_optimizer.group_types_to_query:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from posthog.clickhouse.client import sync_execute
from posthog.models import Person
from posthog.models.filters import SessionRecordingsFilter
from posthog.schema import PersonsOnEventsMode
from posthog.session_recordings.queries.session_recording_list_from_replay_summary import (
SessionRecordingListFromReplaySummary,
)
Expand All @@ -24,7 +25,6 @@
snapshot_clickhouse_queries,
_create_event,
)
from posthog.utils import PersonOnEventsMode


@freeze_time("2021-01-01T13:46:23")
Expand Down Expand Up @@ -63,7 +63,7 @@ def create_event(
True,
False,
False,
PersonOnEventsMode.PERSON_ID_NO_OVERRIDE_PROPERTIES_ON_EVENTS,
PersonsOnEventsMode.person_id_no_override_properties_on_events,
{
"kperson_filter_pre__0": "rgInternal",
"kpersonquery_person_filter_fin__0": "rgInternal",
Expand All @@ -79,7 +79,7 @@ def create_event(
False,
False,
False,
PersonOnEventsMode.DISABLED,
PersonsOnEventsMode.disabled,
{
"kperson_filter_pre__0": "rgInternal",
"kpersonquery_person_filter_fin__0": "rgInternal",
Expand All @@ -95,7 +95,7 @@ def create_event(
False,
True,
False,
PersonOnEventsMode.PERSON_ID_OVERRIDE_PROPERTIES_ON_EVENTS,
PersonsOnEventsMode.person_id_override_properties_on_events,
{
"event_names": [],
"event_start_time": mock.ANY,
Expand All @@ -111,7 +111,7 @@ def create_event(
False,
True,
True,
PersonOnEventsMode.PERSON_ID_OVERRIDE_PROPERTIES_ON_EVENTS,
PersonsOnEventsMode.person_id_override_properties_on_events,
{
"event_end_time": mock.ANY,
"event_names": [],
Expand All @@ -130,7 +130,7 @@ def test_effect_of_poe_settings_on_query_generated(
poe_v1: bool,
poe_v2: bool,
allow_denormalized_props: bool,
expected_poe_mode: PersonOnEventsMode,
expected_poe_mode: PersonsOnEventsMode,
expected_query_params: Dict,
unmaterialized_person_column_used: bool,
materialized_event_column_used: bool,
Expand Down
14 changes: 0 additions & 14 deletions mypy-baseline.txt
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@ posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Dict ent
posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Dict entry 0 has incompatible type "str": "LifecycleFilter"; expected "str": "TrendsFilter" [dict-item]
posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Dict entry 0 has incompatible type "str": "StickinessFilter"; expected "str": "TrendsFilter" [dict-item]
posthog/hogql_queries/legacy_compatibility/feature_flag.py:0: error: Item "AnonymousUser" of "User | AnonymousUser" has no attribute "email" [union-attr]
posthog/hogql/modifiers.py:0: error: Incompatible types in assignment (expression has type "PersonOnEventsMode", variable has type "PersonsOnEventsMode | None") [assignment]
posthog/api/utils.py:0: error: Incompatible types in assignment (expression has type "type[EventDefinition]", variable has type "type[EnterpriseEventDefinition]") [assignment]
posthog/api/utils.py:0: error: Argument 1 to "UUID" has incompatible type "int | str"; expected "str | None" [arg-type]
ee/billing/quota_limiting.py:0: error: List comprehension has incompatible type List[int]; expected List[str] [misc]
Expand Down Expand Up @@ -276,16 +275,9 @@ posthog/hogql/printer.py:0: error: Name "args" already defined on line 0 [no-re
posthog/hogql/printer.py:0: error: Name "args" already defined on line 0 [no-redef]
posthog/hogql/printer.py:0: error: Argument 1 to "lookup_field_by_name" has incompatible type "SelectQueryType | None"; expected "SelectQueryType" [arg-type]
posthog/hogql/printer.py:0: error: Item "TableType" of "TableType | TableAliasType" has no attribute "alias" [union-attr]
posthog/hogql/printer.py:0: error: Non-overlapping equality check (left operand type: "PersonsOnEventsMode | None", right operand type: "Literal[PersonOnEventsMode.DISABLED]") [comparison-overlap]
posthog/hogql/printer.py:0: error: Statement is unreachable [unreachable]
posthog/hogql/printer.py:0: error: "FieldOrTable" has no attribute "name" [attr-defined]
posthog/hogql/printer.py:0: error: Non-overlapping equality check (left operand type: "PersonsOnEventsMode | None", right operand type: "Literal[PersonOnEventsMode.DISABLED]") [comparison-overlap]
posthog/hogql/printer.py:0: error: Statement is unreachable [unreachable]
posthog/hogql/printer.py:0: error: "FieldOrTable" has no attribute "name" [attr-defined]
posthog/hogql/printer.py:0: error: Argument 2 to "_get_materialized_column" of "_Printer" has incompatible type "str | int"; expected "str" [arg-type]
posthog/hogql/printer.py:0: error: Non-overlapping equality check (left operand type: "PersonsOnEventsMode | None", right operand type: "Literal[PersonOnEventsMode.DISABLED]") [comparison-overlap]
posthog/hogql/printer.py:0: error: Argument 2 to "_get_materialized_column" of "_Printer" has incompatible type "str | int"; expected "str" [arg-type]
posthog/hogql/printer.py:0: error: Statement is unreachable [unreachable]
posthog/hogql/printer.py:0: error: Argument 1 to "_print_identifier" of "_Printer" has incompatible type "str | None"; expected "str" [arg-type]
posthog/api/organization.py:0: error: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases [misc]
posthog/api/organization.py:0: error: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases [misc]
Expand All @@ -309,7 +301,6 @@ posthog/hogql/filters.py:0: note: PEP 484 prohibits implicit Optional. According
posthog/hogql/filters.py:0: note: Use https://github.com/hauntsaninja/no_implicit_optional to automatically upgrade your codebase
posthog/hogql/query.py:0: error: Incompatible types in assignment (expression has type "None", variable has type "str | SelectQuery | SelectUnionQuery") [assignment]
posthog/hogql/query.py:0: error: Incompatible types in assignment (expression has type "Expr", variable has type "SelectQuery | SelectUnionQuery") [assignment]
posthog/hogql/query.py:0: error: Incompatible types in assignment (expression has type "Expr", variable has type "SelectQuery | SelectUnionQuery") [assignment]
posthog/hogql/query.py:0: error: Argument 1 to "get_default_limit_for_context" has incompatible type "LimitContext | None"; expected "LimitContext" [arg-type]
posthog/hogql/query.py:0: error: "SelectQuery" has no attribute "select_queries" [attr-defined]
posthog/hogql/query.py:0: error: Subclass of "SelectQuery" and "SelectUnionQuery" cannot exist: would have incompatible method signatures [unreachable]
Expand All @@ -330,12 +321,9 @@ posthog/hogql_queries/insights/trends/breakdown.py:0: error: Item "None" of "Bre
posthog/hogql_queries/insights/trends/breakdown.py:0: error: Argument "breakdown_field" to "get_properties_chain" has incompatible type "str | float | list[str | float] | Any | None"; expected "str" [arg-type]
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/hogql_query_runner.py:0: error: Statement is unreachable [unreachable]
posthog/hogql_queries/hogql_query_runner.py:0: error: Argument "placeholders" to "parse_select" has incompatible type "dict[str, Constant] | None"; expected "dict[str, Expr] | None" [arg-type]
posthog/hogql_queries/hogql_query_runner.py:0: error: Incompatible types in assignment (expression has type "Expr", variable has type "SelectQuery | SelectUnionQuery") [assignment]
posthog/hogql_queries/hogql_query_runner.py:0: error: Incompatible return value type (got "SelectQuery | SelectUnionQuery", expected "SelectQuery") [return-value]
posthog/hogql_queries/events_query_runner.py:0: error: Statement is unreachable [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/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]
Expand Down Expand Up @@ -422,8 +410,6 @@ posthog/session_recordings/queries/session_recording_list_from_replay_summary.py
posthog/session_recordings/queries/session_recording_list_from_replay_summary.py:0: note: If the method is meant to be abstract, use @abc.abstractmethod
posthog/session_recordings/queries/session_recording_list_from_replay_summary.py:0: error: Missing return statement [empty-body]
posthog/session_recordings/queries/session_recording_list_from_replay_summary.py:0: note: If the method is meant to be abstract, use @abc.abstractmethod
posthog/session_recordings/queries/session_recording_list_from_replay_summary.py:0: error: Incompatible types in assignment (expression has type "PersonOnEventsMode", variable has type "PersonsOnEventsMode | None") [assignment]
posthog/session_recordings/queries/session_recording_list_from_replay_summary.py:0: error: Incompatible types in assignment (expression has type "PersonOnEventsMode", variable has type "PersonsOnEventsMode | None") [assignment]
posthog/hogql_queries/test/test_query_runner.py:0: error: Variable "TestQueryRunner" is not valid as a type [valid-type]
posthog/hogql_queries/test/test_query_runner.py:0: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
posthog/hogql_queries/test/test_query_runner.py:0: error: Invalid base class "TestQueryRunner" [misc]
Expand Down
Loading

0 comments on commit b7add1d

Please sign in to comment.