Skip to content

Commit

Permalink
revert: "fix(insights): HogQL calculation of saved legacy insights" (#…
Browse files Browse the repository at this point in the history 
…21590)

Revert "fix(insights): HogQL calculation of saved legacy insights (#21308)"

This reverts commit 957bc3c.
  • Loading branch information
@timgl
timgl authored Apr 16, 2024
1 parent bf79481 commit a6314c6
Show file tree
Hide file tree
Showing 14 changed files with 218 additions and 133 deletions.
1 change: 1 addition & 0 deletions frontend/src/queries/schema.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ export enum NodeKind {
SavedInsightNode = 'SavedInsightNode',
InsightVizNode = 'InsightVizNode',

// New queries, not yet implemented
TrendsQuery = 'TrendsQuery',
FunnelsQuery = 'FunnelsQuery',
RetentionQuery = 'RetentionQuery',
Expand Down
7 changes: 7 additions & 0 deletions mypy-baseline.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Argument
posthog/hogql_queries/legacy_compatibility/filter_to_query.py:0: error: Dict entry 0 has incompatible type "str": "PathsFilter"; expected "str": "TrendsFilter" [dict-item]
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/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 @@ -349,6 +350,7 @@ posthog/queries/breakdown_props.py:0: error: Argument 1 to "translate_hogql" has
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]
posthog/caching/calculate_results.py:0: error: Argument 3 to "process_query" has incompatible type "bool"; expected "LimitContext | None" [arg-type]
posthog/api/person.py:0: error: Argument 1 to "loads" has incompatible type "str | None"; expected "str | bytes | bytearray" [arg-type]
posthog/api/person.py:0: error: Argument "user" to "log_activity" has incompatible type "User | AnonymousUser"; expected "User | None" [arg-type]
posthog/api/person.py:0: error: Argument "user" to "log_activity" has incompatible type "User | AnonymousUser"; expected "User | None" [arg-type]
Expand Down Expand Up @@ -388,6 +390,11 @@ posthog/hogql_queries/insights/lifecycle_query_runner.py:0: note: Consider using
posthog/hogql_queries/insights/lifecycle_query_runner.py:0: error: Argument 1 to "sorted" has incompatible type "list[Any] | None"; expected "Iterable[Any]" [arg-type]
posthog/hogql_queries/insights/lifecycle_query_runner.py:0: error: Item "SelectUnionQuery" of "SelectQuery | SelectUnionQuery" has no attribute "select_from" [union-attr]
posthog/hogql_queries/insights/lifecycle_query_runner.py:0: error: Item "None" of "JoinExpr | Any | None" has no attribute "sample" [union-attr]
posthog/hogql_queries/legacy_compatibility/process_insight.py:0: error: Incompatible types in assignment (expression has type "PathFilter", variable has type "RetentionFilter") [assignment]
posthog/hogql_queries/legacy_compatibility/process_insight.py:0: error: Incompatible types in assignment (expression has type "StickinessFilter", variable has type "RetentionFilter") [assignment]
posthog/hogql_queries/legacy_compatibility/process_insight.py:0: error: Incompatible types in assignment (expression has type "Filter", variable has type "RetentionFilter") [assignment]
posthog/api/insight.py:0: error: Argument 1 to "is_insight_with_hogql_support" has incompatible type "Insight | DashboardTile"; expected "Insight" [arg-type]
posthog/api/insight.py:0: error: Argument 1 to "process_insight" has incompatible type "Insight | DashboardTile"; expected "Insight" [arg-type]
posthog/api/dashboards/dashboard.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/feature_flag.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/feature_flag.py:0: error: Item "Sequence[Any]" of "Any | Sequence[Any] | None" has no attribute "filters" [union-attr]
Expand Down
45 changes: 15 additions & 30 deletions posthog/api/insight.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@
from posthog.api.tagged_item import TaggedItemSerializerMixin, TaggedItemViewSetMixin
from posthog.api.utils import format_paginated_url
from posthog.auth import SharingAccessTokenAuthentication
from posthog.caching.fetch_from_cache import InsightResult, fetch_cached_insight_result, synchronously_update_cache
from posthog.caching.fetch_from_cache import (
InsightResult,
fetch_cached_insight_result,
synchronously_update_cache,
)
from posthog.caching.insights_api import should_refresh_insight
from posthog.constants import (
BREAKDOWN_VALUES_LIMIT,
Expand All @@ -55,7 +59,7 @@
from posthog.hogql.timings import HogQLTimings
from posthog.hogql_queries.apply_dashboard_filters import DATA_TABLE_LIKE_NODE_KINDS
from posthog.hogql_queries.legacy_compatibility.feature_flag import hogql_insights_enabled
from posthog.hogql_queries.legacy_compatibility.filter_to_query import filter_to_query
from posthog.hogql_queries.legacy_compatibility.process_insight import is_insight_with_hogql_support, process_insight
from posthog.kafka_client.topics import KAFKA_METRICS_TIME_TO_SEE_DATA
from posthog.models import DashboardTile, Filter, Insight, User
from posthog.models.activity_logging.activity_log import (
Expand Down Expand Up @@ -517,34 +521,14 @@ def to_representation(self, instance: Insight):

@lru_cache(maxsize=1)
def insight_result(self, insight: Insight) -> InsightResult:
from posthog.caching.calculate_results import calculate_for_query_based_insight

if insight.query:
try:
return calculate_for_query_based_insight(
insight, refresh_requested=refresh_requested_by_client(self.context["request"])
)
except ExposedHogQLError as e:
raise ValidationError(str(e))

if not self.context["request"].user.is_anonymous and hogql_insights_enabled(
self.context["request"].user, insight.filters.get("insight", schema.InsightType.TRENDS)
):
# TRICKY: As running `filters`-based insights on the HogQL-based engine is a transitional mechanism,
# we fake the insight being properly `query`-based.
# To prevent the lie from accidentally being saved to Postgres, we roll it back in the `finally` branch.
insight.query = filter_to_query(insight.filters).model_dump()
try:
return calculate_for_query_based_insight(
insight, refresh_requested=refresh_requested_by_client(self.context["request"])
)
except ExposedHogQLError as e:
raise ValidationError(str(e))
finally:
insight.query = None

dashboard = self.context.get("dashboard", None)
dashboard_tile = self.dashboard_tile_from_context(insight, dashboard)
target = insight if dashboard is None else dashboard_tile

if hogql_insights_enabled(self.context.get("request", None).user) and is_insight_with_hogql_support(
target or insight
):
return process_insight(target or insight, insight.team)

is_shared = self.context.get("is_shared", False)
refresh_insight_now, refresh_frequency = should_refresh_insight(
Expand All @@ -555,9 +539,10 @@ def insight_result(self, insight: Insight) -> InsightResult:
)
if refresh_insight_now:
INSIGHT_REFRESH_INITIATED_COUNTER.labels(is_shared=is_shared).inc()
return synchronously_update_cache(insight, dashboard, refresh_frequency=refresh_frequency)
return synchronously_update_cache(insight, dashboard, refresh_frequency)

return fetch_cached_insight_result(dashboard_tile or insight, refresh_frequency)
# :TODO: Clear up if tile can be null or not
return fetch_cached_insight_result(target or insight, refresh_frequency)

@lru_cache(maxsize=1) # each serializer instance should only deal with one insight/tile combo
def dashboard_tile_from_context(self, insight: Insight, dashboard: Optional[Dashboard]) -> Optional[DashboardTile]:
Expand Down
2 changes: 0 additions & 2 deletions posthog/api/services/query.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
def process_query(
team: Team,
query_json: dict,
*,
limit_context: Optional[LimitContext] = None,
refresh_requested: Optional[bool] = False,
) -> dict:
Expand All @@ -76,7 +75,6 @@ def process_query(
def process_query_model(
team: Team,
query: BaseModel, # mypy has problems with unions and isinstance
*,
limit_context: Optional[LimitContext] = None,
refresh_requested: Optional[bool] = False,
) -> dict:
Expand Down
20 changes: 3 additions & 17 deletions posthog/api/test/dashboards/test_dashboard.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1232,14 +1232,7 @@ def test_create_from_template_json_cam_provide_query_tile(self) -> None:
"tiles": [
{
"type": "INSIGHT",
"query": {
"kind": "DataTableNode",
"columns": ["person", "id", "created_at", "person.$delete"],
"source": {
"kind": "EventsQuery",
"select": ["*"],
},
},
"query": {"kind": "a datatable"},
"filters": {"date_from": None},
"layouts": {},
}
Expand Down Expand Up @@ -1284,15 +1277,8 @@ def test_create_from_template_json_cam_provide_query_tile(self) -> None:
"name": None,
"next_allowed_client_refresh": None,
"order": None,
"query": {
"kind": "DataTableNode",
"columns": ["person", "id", "created_at", "person.$delete"],
"source": {
"kind": "EventsQuery",
"select": ["*"],
},
},
"result": [],
"query": {"kind": "a datatable"},
"result": None,
"saved": False,
"short_id": ANY,
"tags": [],
Expand Down
17 changes: 10 additions & 7 deletions posthog/api/test/test_insight.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1973,7 +1973,7 @@ def test_get_recently_viewed_insights_excludes_query_based_insights_by_default(s
"*",
"event",
"person",
"coalesce(properties.$current_url, properties.$screen_name)",
"coalesce(properties.$current_url, properties.$screen_name) # Url / Screen",
"properties.$lib",
"timestamp",
],
Expand Down Expand Up @@ -2014,7 +2014,7 @@ def test_get_recently_viewed_insights_can_include_query_based_insights(self) ->
"*",
"event",
"person",
"coalesce(properties.$current_url, properties.$screen_name)",
"coalesce(properties.$current_url, properties.$screen_name) # Url / Screen",
"properties.$lib",
"timestamp",
],
Expand Down Expand Up @@ -2953,19 +2953,22 @@ def test_insight_retention_hogql(self) -> None:
def test_insight_with_filters_via_hogql(self) -> None:
filter_dict = {"insight": "LIFECYCLE", "events": [{"id": "$pageview"}]}

insight = Insight.objects.create(
Insight.objects.create(
filters=Filter(data=filter_dict).to_dict(),
team=self.team,
short_id="xyz123",
)

# fresh response
response = self.client.get(f"/api/projects/{self.team.id}/insights/{insight.id}/?refresh=true")
response = self.client.get(f"/api/projects/{self.team.id}/insights/?short_id=xyz123")
self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertEqual(response.json()["result"][0]["data"], [0, 0, 0, 0, 0, 0, 0, 0])

self.assertEqual(len(response.json()["results"]), 1)
self.assertEqual(response.json()["results"][0]["result"][0]["data"], [0, 0, 0, 0, 0, 0, 0, 0])

# cached response
response = self.client.get(f"/api/projects/{self.team.id}/insights/{insight.id}/?refresh=true")
response = self.client.get(f"/api/projects/{self.team.id}/insights/?short_id=xyz123")
self.assertEqual(response.status_code, status.HTTP_200_OK)

self.assertEqual(response.json()["result"][0]["data"], [0, 0, 0, 0, 0, 0, 0, 0])
self.assertEqual(len(response.json()["results"]), 1)
self.assertEqual(response.json()["results"][0]["result"][0]["data"], [0, 0, 0, 0, 0, 0, 0, 0])
68 changes: 56 additions & 12 deletions posthog/api/test/test_insight_query.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def test_can_save_valid_events_query_to_an_insight(self) -> None:
"*",
"event",
"person",
"coalesce(properties.$current_url, properties.$screen_name)",
"coalesce(properties.$current_url, properties.$screen_name) # Url / Screen",
"properties.$lib",
"timestamp",
],
Expand Down Expand Up @@ -55,7 +55,7 @@ def test_can_save_valid_events_table_query_to_an_insight(self) -> None:
"*",
"event",
"person",
"coalesce(properties.$current_url, properties.$screen_name)",
"coalesce(properties.$current_url, properties.$screen_name) # Url / Screen",
"properties.$lib",
"timestamp",
],
Expand All @@ -82,8 +82,15 @@ def test_can_save_valid_persons_table_query_to_an_insight(self) -> None:
"kind": "DataTableNode",
"columns": ["person", "id", "created_at", "person.$delete"],
"source": {
"kind": "EventsQuery",
"select": ["*"],
"kind": "PersonsNode",
"properties": [
{
"type": "person",
"key": "$browser",
"operator": "exact",
"value": "Chrome",
}
],
},
},
},
Expand All @@ -98,8 +105,15 @@ def test_no_default_filters_on_insight_query(self) -> None:
"kind": "DataTableNode",
"columns": ["person", "id", "created_at", "person.$delete"],
"source": {
"kind": "EventsQuery",
"select": ["*"],
"kind": "PersonsNode",
"properties": [
{
"type": "person",
"key": "$browser",
"operator": "exact",
"value": "Chrome",
}
],
},
},
},
Expand Down Expand Up @@ -164,6 +178,15 @@ def test_can_save_insights_query_to_an_insight(self) -> None:
"name": "$pageview",
"custom_name": "Views",
"event": "$pageview",
"properties": [
{
"type": "event",
"key": "$browser",
"operator": "exact",
"value": "Chrome",
},
{"type": "cohort", "key": "id", "value": 2},
],
"limit": 100,
}
],
Expand All @@ -189,8 +212,15 @@ def test_cannot_save_invalid_persons_table_query_to_an_insight(self) -> None:
"query": {
"kind": "DataTableNode",
"source": {
"kind": "EventsQuery",
"select": ["*"],
"kind": "PersonsNode",
"properties": [
{
"type": "person",
"key": "$browser",
"operator": "exact",
"value": "Chrome",
}
],
},
},
},
Expand All @@ -206,8 +236,15 @@ def test_listing_insights_by_default_does_not_include_those_with_only_queries(se
"kind": "DataTableNode",
"columns": ["person", "id", "created_at", "person.$delete"],
"source": {
"kind": "EventsQuery",
"select": ["*"],
"kind": "PersonsNode",
"properties": [
{
"type": "person",
"key": "$browser",
"operator": "exact",
"value": "Chrome",
}
],
},
},
},
Expand All @@ -229,8 +266,15 @@ def test_can_list_insights_including_those_with_only_queries(self) -> None:
"kind": "DataTableNode",
"columns": ["person", "id", "created_at", "person.$delete"],
"source": {
"kind": "EventsQuery",
"select": ["*"],
"kind": "PersonsNode",
"properties": [
{
"type": "person",
"key": "$browser",
"operator": "exact",
"value": "Chrome",
}
],
},
},
},
Expand Down
Loading

0 comments on commit a6314c6

Please sign in to comment.