-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: enable hogql cohorts #30426
feat: enable hogql cohorts #30426
Conversation
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.
PR Summary
This PR implements HogQL-based cohort functionality in PostHog, adding support for advanced property filtering and improving cohort recalculation performance.
- Added IN/NOT_IN operator support in property expressions with proper list value validation in
posthog/hogql/property.py
- Introduced HogQL global settings (
max_ast_elements
,max_expanded_ast_elements
,max_bytes_before_external_group_by
) inposthog/models/cohort/util.py
to prevent performance issues - Added temporary year-to-month conversion (multiplying by 12) in
posthog/hogql_queries/hogql_cohort_query.py
until proper year support is implemented - Enhanced test coverage in
ee/clickhouse/queries/test/test_cohort_query.py
with new cases for long-term filtering and array value handling
4 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
…to aspicer/cohort_switch
…to aspicer/cohort_switch
…to aspicer/cohort_switch
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 to me! One note about test, but nothing blocking.
timestamp=datetime.now() - timedelta(days=1), | ||
) | ||
|
||
# Person 2: Doesn't match - has only 1 "Application Opened" event |
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.
Would it make sense to add a case where the person has 2 "Application Opened" events, but one is outside of the time interval, and therefore doesn't match?
This PR moves cohort calculation to the new HogQL cohorts, behind a feature flag by org.
Checked loki error logs and found just a few cohorts erroring. Fixed an assortment of bugs for these and added tests.
Did a comparison of data on prod and found consistent results.
Rollout plan is to slowly increase the teams using hogql cohorts by default (starting with team 2) over the next week, and complete the switch soon. Will follow with another PR to remove and disentangle the old code.