-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(hogql): PersonsQuery #17764
feat(hogql): PersonsQuery #17764
Conversation
All right, I think this is ready for a review. The description has been updated with what is still in scope of this PR, and what will follow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from what I can tell.
If we're not intending to ever cache the persons query, we should probably implement a variant of the query runner without caching. I initially missed that we use QUERY_WITH_RUNNER_NO_CACHE
to call calculate
instead of run.
return self.to_query() | ||
|
||
def _is_stale(self, cached_result_package): | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we intending to cache this query at some point? Otherwise we should properly implement a query runner without caching, so that we don't make unnecessary requests to Redis and Prometheus.
Thanks for the review! Regarding caching... I'm not sure. It might make sense to cache some of the results, so e.g. the flow of persons -> person -> back is fast even if we make another request. To be continued... :) |
Problem
Splitting out work from #17470
Changes
PersonsQuery
, similar toEventsQuery
PersonsNode
, this query uses ClickHouse instead of Postgres/persons
page and on the cohort edit pages/persons/:UUID
vs/person/:distinct_id
) to avoid making a join we don't need at this point.How did you test this code?
Wrote tests for the backend, clicked around a lot in the interface for the frontend.
Out of scope
source
field toPersonsQuery
, and hooking it to theLifecycleQuery
orTrendsQuery
.person_display_name_properties
field onTeam
. This field contains 7 properties by default (email
,Email
,name
,Name
,username
,Username
,UserName
). I didn't want to put all of them into the filter, as property extraction is a heavy operation. Doing it 5-7 times for properties a team doesn't have (and which isn't likely materialized) will make the query really slow. So I kept the current behaviour.raw_persons
table.