Repository: PostHog/posthog
Pull request: Fetch person property values via Clickhouse
## Changes

- Resolves https://github.com/PostHog/posthog/issues/8364
- Moves person property values autocomplete from postgres to clickhouse
- Looks over only 100k persons (ordered by UUID desc... so either 100k at random or last users, not sure), instead of always looking through all persons like before
- Little bit of flyby cleanup for api/events and some whitespace for the relevant event queries

## How did you test this code?

- Made sure the existing test kept working
- Tried it with various properties in the browser

Problem statement:
PostgreSQL - `properties` query is constantly timing out
## Bug description

The query is semi-constantly failing on cloud as it's taking more than the threshold to execute
```
SELECT ("posthog_person"."properties" -> 'utm_campaign'), COUNT("posthog_person"."id") AS "count" FROM "posthog_person" WHERE ("posthog_person"."team_id" = 1243 AND ("posthog_person"."properties" -> 'utm_campaign') IS NOT NULL) GROUP BY ("posthog_person"."properties" -> 'utm_campaign') ORDER BY "count" DESC LIMIT 50
```

Note: it is scanning **over 20GB** of data. See query plan below 👇 

<details> 

```json
[ { "Plan": { "Node Type": "Limit", "Parallel Aware": false, "Startup Cost": 4046931.50, "Total Cost": 4046931.53, "Plan Rows": 50, "Plan Width": 40, "Actual Startup Time": 27318.768, "Actual Total Time": 27318.781, "Actual Rows": 50, "Actual Loops": 1, "Output": ["((properties -> 'utm_campaign'::text))", "(count(id))"], "Shared Hit Blocks": 851810, "Shared Read Blocks": 2663385, "Shared Dirtied Blocks": 10, "Shared Written Blocks": 32047, "Local Hit Blocks": 0, "Local Read Blocks": 0, "Local Dirtied Blocks": 0, "Local Written Blocks": 0, "Temp Read Blocks": 0, "Temp Written Blocks": 0, "I/O Read Time": 15204.869, "I/O Write Time": 296.256, "Plans": [ { "Node Type": "Sort", "Parent Relationship": "Outer", "Parallel Aware": false, "Startup Cost": 4046931.50, "Total Cost": 4046948.22, "Plan Rows": 33428, "Plan Width": 40, "Actual Startup Time": 27318.767, "Actual Total Time": 27318.775, "Actual Rows": 50, "Actual Loops": 1, "Output": ["((properties -> 'utm_campaign'::text))", "(count(id))"], "Sort Key": ["(count(posthog_person.id)) DESC"], "Sort Method": "top-N heapsort", "Sort Space Used": 32, "Sort Space Type": "Memory", "Shared Hit Blocks": 851810, "Shared Read Blocks": 2663385, "Shared Dirtied Blocks": 10, "Shared Written Blocks": 32047, "Local Hit Blocks": 0, "Local Read Blocks": 0, "Local Dirtied Blocks": 0, "Local Written Blocks": 0, "Temp Read Blocks": 0, "Temp Written Blocks": 0, "I/O Read Time": 15204.869, "I/O Write Time": 296.256, "Plans": [ { "Node Type": "Aggregate", "Strategy": "Hashed", "Partial Mode": "Simple", "Parent Relationship": "Outer", "Parallel Aware": false, "Startup Cost": 4046592.41, "Total Cost": 4046709.41, "Plan Rows": 33428, "Plan Width": 40, "Actual Startup Time": 27317.620, "Actual Total Time": 27318.658, "Actual Rows": 197, "Actual Loops": 1, "Output": ["((properties -> 'utm_campaign'::text))", "count(id)"], "Group Key": ["(posthog_person.properties -> 'utm_campaign'::text)"], "Shared Hit Blocks": 851810, "Shared Read Blocks": 2663385, "Shared Dirtied Blocks": 10, "Shared Written Blocks": 32047, "Local Hit Blocks": 0, "Local Read Blocks": 0, "Local Dirtied Blocks": 0, "Local Written Blocks": 0, "Temp Read Blocks": 0, "Temp Written Blocks": 0, "I/O Read Time": 15204.869, "I/O Write Time": 296.256, "Plans": [ { "Node Type": "Index Scan", "Parent Relationship": "Outer", "Parallel Aware": false, "Scan Direction": "Forward", "Index Name": "posthog_person_team_id_325c1b73", "Relation Name": "posthog_person", "Schema": "public", "Alias": "posthog_person", "Startup Cost": 0.11, "Total Cost": 4043295.80, "Plan Rows": 3296613, "Plan Width": 36, "Actual Startup Time": 0.139, "Actual Total Time": 26661.054, "Actual Rows": 841969, "Actual Loops": 1, "Output": ["(properties -> 'utm_campaign'::text)", "id"], "Index Cond": "(posthog_person.team_id = 1243)", "Rows Removed by Index Recheck": 0, "Filter": "((posthog_person.properties -> 'utm_campaign'::text) IS NOT NULL)", "Rows Removed by Filter": 2541475, "Shared Hit Blocks": 851810, "Shared Read Blocks": 2663385, "Shared Dirtied Blocks": 10, "Shared Written Blocks": 32047, "Local Hit Blocks": 0, "Local Read Blocks": 0, "Local Dirtied Blocks": 0, "Local Written Blocks": 0, "Temp Read Blocks": 0, "Temp Written Blocks": 0, "I/O Read Time": 15204.869, "I/O Write Time": 296.256 } ] } ] } ] }, "Planning Time": 0.201, "Triggers": [ ], "Execution Time": 27319.312 } ]
```

</details>

## Expected behavior
The query should not be failing.


## How to reproduce

1. Run the query

## Environment

- [X] PostHog Cloud
- [ ] self-hosted PostHog (ClickHouse-based), version/commit: _please provide_
- [ ] self-hosted PostHog (Postgres-based, legacy), version/commit: _please provide_

Hints from issue discussion:
cc @alexkim205 @mariusandra @EDsCODE for prioritization. I think this query is triggered from various analytics filters for autocomplete, has to do with data taxonomy
Thanks for the context here, it looks like this query traces back to the `PersonViewSet` GET method  here: https://github.com/PostHog/posthog/blob/7d84ac966bb252da4cc41ba6c183f47b42c59738/posthog/api/person.py#L132-L145

This code path is hit when you navigate to the `https://app.posthog.com/persons` and partially fill out this a query like so:

<img width="840" alt="Screen Shot 2022-02-01 at 9 57 47 AM" src="https://user-images.githubusercontent.com/13460330/152024208-4c0cb244-e41a-451d-ad78-d02713a7c7de.png">

Here we'll try to list out all the possible property values by default which takes a long time since this query runs. It's not blocking a user's ability to query for properties if they know what they're searching for, but I can see how it would slow down discovery if they're going in blind. I would flag this as mid-high priority since it affects perceived performance of the app and the taxonomic filter is used heavily in a lot of areas.
Something like this returns results instantly in metabase:

```sql
SELECT property, count(property) as "count" FROM (
    SELECT 
        ("posthog_person"."properties" -> 'utm_campaign') as "property"
    FROM "posthog_person" 
    WHERE 
        "posthog_person"."team_id" = 1243 AND 
        "posthog_person"."properties" -> 'utm_campaign' IS NOT NULL
    ORDER BY "id" DESC 
    LIMIT 10000
) AS P 
GROUP BY property
ORDER BY "count" DESC
LIMIT 100
```
> Something like this returns results instantly in metabase:

Yes but that's because those are different queries (returning different results) or am I missing something? (maybe it's 👍 for our use case, I'm just commenting at the two queries)
The query above was posted during a call with @alexkim205 , apologies for the lack of context around it. Here's a better analysis.

The issue here is about the person property suggestions being way too slow:

![2022-02-03 09 27 40](https://user-images.githubusercontent.com/53387/152306977-399b4cbb-260c-4690-ab2b-51561fa62741.gif)

While suggestions for event properties are quite fast:

![2022-02-03 09 28 36](https://user-images.githubusercontent.com/53387/152307155-540814c0-0c57-4d34-b9b0-05bb55fcc9f0.gif)

That's because the event property query has an `and` clause with `timestamp > 7 days ago`... and is done on ClickHouse, but the person property query looks through every person, and is done on Postgres.

The query posted above is a (draft for a) quick win to give us some breathing room while we're figuring this out. Moving the query to clickhouse will surely be another win, but I'd still implement some kind of limit on the number of rows scanned.

So indeed that query is not doing the same as what we have now, but I think it's good enough, considering how its compatriot events query works.
Make sense, thank you for the additional context!

Commit message: no point making it too flat now

Patch to review:
diff --git a/ee/clickhouse/models/property.py b/ee/clickhouse/models/property.py
index 5804e02de05d4..06a3627aea1c8 100644
--- a/ee/clickhouse/models/property.py
+++ b/ee/clickhouse/models/property.py
@@ -29,6 +29,8 @@
     GET_DISTINCT_IDS_BY_PERSON_ID_FILTER,
     GET_DISTINCT_IDS_BY_PROPERTY_SQL,
     GET_TEAM_PERSON_DISTINCT_IDS,
+    SELECT_PERSON_PROP_VALUES_SQL,
+    SELECT_PERSON_PROP_VALUES_SQL_WITH_FILTER,
 )
 from posthog.models.cohort import Cohort
 from posthog.models.event import Selector
@@ -419,6 +421,14 @@ def get_property_values_for_key(key: str, team: Team, value: Optional[str] = Non
     )
 
 
+def get_person_property_values_for_key(key: str, team: Team, value: Optional[str] = None):
+    if value:
+        return sync_execute(
+            SELECT_PERSON_PROP_VALUES_SQL_WITH_FILTER, {"team_id": team.pk, "key": key, "value": "%{}%".format(value)},
+        )
+    return sync_execute(SELECT_PERSON_PROP_VALUES_SQL, {"team_id": team.pk, "key": key},)
+
+
 def filter_element(filters: Dict, *, operator: Optional[OperatorType] = None, prepend: str = "") -> Tuple[str, Dict]:
     if not operator:
         operator = "exact"
diff --git a/ee/clickhouse/sql/events.py b/ee/clickhouse/sql/events.py
index c7f58f81263dd..64fb283790a06 100644
--- a/ee/clickhouse/sql/events.py
+++ b/ee/clickhouse/sql/events.py
@@ -114,11 +114,29 @@
 """
 
 SELECT_PROP_VALUES_SQL = """
-SELECT DISTINCT trim(BOTH '\"' FROM JSONExtractRaw(properties, %(key)s)) FROM events where JSONHas(properties, %(key)s) AND team_id = %(team_id)s {parsed_date_from} {parsed_date_to} LIMIT 10
+SELECT 
+    DISTINCT trim(BOTH '\"' FROM JSONExtractRaw(properties, %(key)s)) 
+FROM 
+    events 
+WHERE 
+    team_id = %(team_id)s AND
+    JSONHas(properties, %(key)s) 
+    {parsed_date_from} 
+    {parsed_date_to} 
+LIMIT 10
 """
 
 SELECT_PROP_VALUES_SQL_WITH_FILTER = """
-SELECT DISTINCT trim(BOTH '\"' FROM JSONExtractRaw(properties, %(key)s)) FROM events where team_id = %(team_id)s AND trim(BOTH '\"' FROM JSONExtractRaw(properties, %(key)s)) ILIKE %(value)s {parsed_date_from} {parsed_date_to} LIMIT 10
+SELECT 
+    DISTINCT trim(BOTH '\"' FROM JSONExtractRaw(properties, %(key)s)) 
+FROM
+    events 
+WHERE 
+    team_id = %(team_id)s AND 
+    trim(BOTH '\"' FROM JSONExtractRaw(properties, %(key)s)) ILIKE %(value)s 
+    {parsed_date_from} 
+    {parsed_date_to} 
+LIMIT 10
 """
 
 SELECT_EVENT_BY_TEAM_AND_CONDITIONS_SQL = """
diff --git a/ee/clickhouse/sql/person.py b/ee/clickhouse/sql/person.py
index de225bd79a0cf..2cc3c47b0bf01 100644
--- a/ee/clickhouse/sql/person.py
+++ b/ee/clickhouse/sql/person.py
@@ -384,3 +384,44 @@
 COMMENT_DISTINCT_ID_COLUMN_SQL = (
     lambda: f"ALTER TABLE person_distinct_id ON CLUSTER {CLICKHOUSE_CLUSTER} COMMENT COLUMN distinct_id 'skip_0003_fill_person_distinct_id2'"
 )
+
+
+SELECT_PERSON_PROP_VALUES_SQL = """
+SELECT 
+    value, 
+    count(value) 
+FROM (
+    SELECT 
+        trim(BOTH '\"' FROM JSONExtractRaw(properties, %(key)s)) as value
+    FROM 
+        person 
+    WHERE 
+        team_id = %(team_id)s AND
+        JSONHas(properties, %(key)s) 
+    ORDER BY id DESC
+    LIMIT 100000
+)
+GROUP BY value
+ORDER BY count(value) DESC
+LIMIT 20
+"""
+
+SELECT_PERSON_PROP_VALUES_SQL_WITH_FILTER = """
+SELECT 
+    value, 
+    count(value) 
+FROM (
+    SELECT 
+        trim(BOTH '\"' FROM JSONExtractRaw(properties, %(key)s)) as value
+    FROM 
+        person 
+    WHERE 
+        team_id = %(team_id)s AND 
+        trim(BOTH '\"' FROM JSONExtractRaw(properties, %(key)s)) ILIKE %(value)s 
+    ORDER BY id DESC
+    LIMIT 100000
+)
+GROUP BY value
+ORDER BY count(value) DESC
+LIMIT 20
+"""
diff --git a/posthog/api/event.py b/posthog/api/event.py
index 4193ea49263ba..1be99c9dd23d2 100644
--- a/posthog/api/event.py
+++ b/posthog/api/event.py
@@ -221,7 +221,6 @@ def retrieve(
     def values(self, request: request.Request, **kwargs) -> response.Response:
         key = request.GET.get("key")
         team = self.team
-        result = []
         flattened = []
         if key == "custom_event":
             events = sync_execute(GET_CUSTOM_EVENTS, {"team_id": team.pk})
@@ -234,7 +233,7 @@ def values(self, request: request.Request, **kwargs) -> response.Response:
                     flattened.append(json.loads(value[0]))
                 except json.decoder.JSONDecodeError:
                     flattened.append(value[0])
-        return response.Response([{"name": convert_property_value(value)} for value in flatten(flattened)])
+        return response.Response([{"name": convert_property_value(value)} for value in flattened])
 
 
 class LegacyEventViewSet(EventViewSet):
diff --git a/posthog/api/person.py b/posthog/api/person.py
index 9a97db7113d1f..79f0e02695f18 100644
--- a/posthog/api/person.py
+++ b/posthog/api/person.py
@@ -24,6 +24,7 @@
 
 from ee.clickhouse.client import sync_execute
 from ee.clickhouse.models.person import delete_person
+from ee.clickhouse.models.property import get_person_property_values_for_key
 from ee.clickhouse.queries.funnels import ClickhouseFunnelActors, ClickhouseFunnelTrendsActors
 from ee.clickhouse.queries.funnels.base import ClickhouseFunnelBase
 from ee.clickhouse.queries.funnels.funnel_correlation_persons import FunnelCorrelationActors
@@ -52,11 +53,14 @@
 from posthog.models.filters.stickiness_filter import StickinessFilter
 from posthog.permissions import ProjectMembershipNecessaryPermissions, TeamMemberAccessPermission
 from posthog.queries.base import filter_persons
-from posthog.queries.lifecycle import LifecycleTrend
-from posthog.queries.retention import Retention
-from posthog.queries.stickiness import Stickiness
 from posthog.tasks.split_person import split_person
-from posthog.utils import convert_property_value, format_query_params_absolute_url, is_anonymous_id, relative_date_parse
+from posthog.utils import (
+    convert_property_value,
+    flatten,
+    format_query_params_absolute_url,
+    is_anonymous_id,
+    relative_date_parse,
+)
 
 
 class PersonCursorPagination(CursorPagination):
@@ -320,17 +324,20 @@ def calculate_path_persons(self, request: request.Request) -> Dict[str, Tuple[li
 
     @action(methods=["GET"], detail=False)
     def values(self, request: request.Request, **kwargs) -> response.Response:
-        people = self.get_queryset()
-        key = "properties__{}".format(request.GET.get("key"))
-        people = people.values(key).annotate(count=Count("id")).filter(**{f"{key}__isnull": False}).order_by("-count")
-
-        if request.GET.get("value"):
-            people = people.extra(
-                where=["properties ->> %s LIKE %s"], params=[request.GET["key"], "%{}%".format(request.GET["value"])],
-            )
-
+        key = request.GET.get("key")
+        value = request.GET.get("value")
+        team = self.team
+        flattened = []
+        if key:
+            result = get_person_property_values_for_key(key, team, value)
+            for (value, count) in result:
+                try:
+                    # Try loading as json for dicts or arrays
+                    flattened.append((json.loads(value), count))  # type: ignore
+                except json.decoder.JSONDecodeError:
+                    flattened.append((value, count))
         return response.Response(
-            [{"name": convert_property_value(event[key]), "count": event["count"]} for event in people[:50]]
+            [{"name": convert_property_value(value), "count": count} for (value, count) in flattened]
         )
 
     @action(methods=["POST"], detail=True)


Code review comments:
diff_hunk: @@ -320,17 +324,20 @@ def calculate_path_persons(self, request: request.Request) -> Dict[str, Tuple[li
 
     @action(methods=["GET"], detail=False)
     def values(self, request: request.Request, **kwargs) -> response.Response:
-        people = self.get_queryset()
-        key = "properties__{}".format(request.GET.get("key"))
-        people = people.values(key).annotate(count=Count("id")).filter(**{f"{key}__isnull": False}).order_by("-count")
-
-        if request.GET.get("value"):
-            people = people.extra(
-                where=["properties ->> %s LIKE %s"], params=[request.GET["key"], "%{}%".format(request.GET["value"])],
-            )
-
+        key = request.GET.get("key")
+        value = request.GET.get("value")
+        team = self.team
+        flattened = []
+        if key:
+            result = get_person_property_values_for_key(key, team, value)
path: posthog/api/person.py
text: @user1:
1. should we add a try/catch?
2. can we add some telemetry to this call? (metric for success/failure calls, timer for success/failure calls) 


@author:
Thanks for the review! 

1. Genuinely curious what would this add? I think it's better to bubble any exception up the stack, eventually reaching sentry, and to let the frontend handle non-200 responses.
2. I'd be happy to add telemetry, but the code I adapted doesn't come with any. What's the latest canonical way to do this?

Going on a bit of a tangent, but...

Whenever I'm in part of a codebase I don't regularly interact with, I try to copy prior art. Here I was just adapting what the equivalent events api does (without the `"custom_event"` nonsense):

https://github.com/PostHog/posthog/blob/9bb9025a90aa8312fdcc6700cd90ab3b2b6ca25e/posthog/api/event.py#L220-L236

If there are any guidelines I should follow, other than what's in the very early [coding conventions](https://posthog.com/docs/contribute/coding-conventions) doc, I don't know of them... and it would be great to add them. The small teams offsites might be a good time to collectively think of this...

@user2:
Some middleware that sits and pushes path and responses status code to statsd or equivalent would be super cool

@user1:
> Genuinely curious what would this add? I think it's better to bubble any exception up the stack, eventually reaching sentry, and to let the frontend handle non-200 responses.

That's a fair question! We can definitely leave everything as it is and let Sentry and the frontend handle the non-200 response. My suggestion was to explicitly handle this try/except here so that we can customise the behaviour and do like:
 
- add a metric on query/timing failure
- eventually wrap the error message body with something custom
- etc... 

> I'd be happy to add telemetry, but the code I adapted doesn't come with any. What's the latest canonical way to do this?

I don't think we have a canonical way (yet) but here's an example of it https://github.com/PostHog/posthog/blob/0464a51d7d0733cc5ebb35406884be1abb81fae4/posthog/tasks/update_cache.py#L52-L81

> If there are any guidelines I should follow, other than what's in the very early [coding conventions](https://posthog.com/docs/contribute/coding-conventions) doc, I don't know of them... and it would be great to add them. The small teams offsites might be a good time to collectively think of this...

Unfortunately there aren't yet. 100% to follow up and discuss about those collectively at the offsite!