feat(hogql): Add funnels to paths insight #21044
Conversation
|
Size Change: -32 B (0%) Total Size: 824 kB ℹ️ View Unchanged
|
|
Feels ready and live compare shows nice green. |
There was a problem hiding this comment.
Looks solid overall, great to finally get the last piece missing in the HogQL conversion :)
There are some CI failures and I left a couple of comments inline. Also do we have existing tests for this? I usually found that converting the existing tests for the new implementation helped catch unexpected bugs.
frontend/src/queries/nodes/InsightQuery/utils/filtersToQueryNode.ts
Outdated
Show resolved
Hide resolved
| and self.query.pathsFilter.funnelActorsQuery.funnelStep | ||
| and self.query.pathsFilter.funnelActorsQuery.funnelStep < 0 | ||
| ): | ||
| default_case += f" + INTERVAL {interval} {interval_unit}" |
There was a problem hiding this comment.
Just the other day I noticed an issue with HogQL and adding intervals. Basically ClickHouse requires the date to be in UTC, otherwise it will have an extra/missing hour for periods spanning a DST transition. See my fix and associated tests here: #21042. The solution for this is to wrap the relevant date in toTimeZone(mydate, 'UTC')
|
A note on the status here. I want to check both things suggested by @thmsobrmlr but didn't get to it before my vacation.
|
Problem
We left out funnels when working on the paths query.
Changes
How did you test this code?