-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(batch-exports): Support for event property filters #27921
Conversation
Making a draft while I fix some conflicts, some test failures, and address an issue with the UI. |
3ee4a41
to
3a662da
Compare
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
Size Change: 0 B Total Size: 1.16 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
3ea4b04
to
cc6de9c
Compare
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.
Changes look good. Just added some comments to help me understand a few things
posthog/temporal/tests/batch_exports/test_s3_batch_export_workflow.py
Outdated
Show resolved
Hide resolved
ed22b04
to
e56c7c5
Compare
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
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.
LGTM 🚢
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Problem
As described in #26415, users wish to filter batch exports using properties. This is currently not supported by batch exports, so this PR adds support for it. See breakdown of all changes.
Changes
Backend
BatchExportModel
now accepts serialized HogQL event property filters in a new attributefilters
.filters
are now parsed to a clickhouse expression when initializing the record batch producer (as a consequence,Producer.start
is now async).events_batch_export_*
views are no longer in use.Frontend
PropertyFilter
component.exclude_events
andinclude_events
.TODO
Only event properties can be used in filters. A follow-up PR will expand this for person properties too.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
How did you test this code?
Added a new model to S3 tests that has property filters. UI stuff tested manually.