-
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): add funnel hogql actors #20472
Conversation
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
Size Change: +686 B (0%) Total Size: 864 kB ℹ️ View Unchanged
|
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! 👍
model_config = ConfigDict( | ||
extra="forbid", | ||
) | ||
includeRecordings: Optional[bool] = None |
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.
I'm actually not sure if this is ever False
. It wasn't when I worked on it on paths. So we might just optimize this away and always return them.
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.
Thanks for the hint, I'll double check this. I think for funnels this might be false for correlation queries or correlation actor queries. Might make sense to remove it from the schema though.
Problem
Funnels need to be converted to HogQL (see the funnels section in PostHog/meta#130).
Changes
Adds frontend side handling of funnel actors and a HogQL version of actors for the regular funnel. The proposed schema for the actors nodes is now as following:
I kept the filter names similar to the existing ones e.g.
funnelStep
, but they could be shortened to e.g.step
or something more expressive likeactorsStep
.Follow-ups
How did you test this code?
Converted existing tests