feat: added the final set of aggregation functions #18268
Conversation
Gilbert09
changed the title
|
Hey @Gilbert09! 👋 |
| if self.series.math_property == "$time": | ||
| return ast.Call( | ||
| name=method, | ||
| args=[ | ||
| ast.Call( | ||
| name="toUnixTimestamp", | ||
| args=[ast.Field(chain=["properties", "$time"])], | ||
| ) | ||
| ], | ||
| ) |
There was a problem hiding this comment.
This is interesting – I thought the $time property was deprecated, in which case we probably don't need this?
There was a problem hiding this comment.
It is - but, it looks like there's a frontend bug that means the property default gets set to $time - one to fix in a follow-up
| return parse_expr(self.series.math_hogql) | ||
| elif self.series.math == "total": | ||
| return parse_expr("count(e.uuid)") | ||
| elif self.series.math == "dau": | ||
| return parse_expr("count(DISTINCT e.person_id)") | ||
| elif self.series.math == "weekly_active": | ||
| return ast.Field(chain=["counts"]) | ||
| return ast.Field(chain=["counts"]) # This gets replaced when doing query orchestration |
There was a problem hiding this comment.
"This gets replaced" sounds a lot like ast.Placeholder 😄 I'm curious why this scheme instead of placeholders?
There was a problem hiding this comment.
Either is fine - no AST parsing/resolving is actually run before the replacement gets done, we just need something to populate the field. In my mind, Field is the simplest AST node we could pick here
There was a problem hiding this comment.
If they both function the same way here, I think it'd be better to use Placeholder, clearer in terms of intent. If that'd need more complex replacement logic, then Field is fine
Yep - I know I keep saying this, but they are coming. I don't think there's much value in adding unit tests to most of these classes (the majority of them being fairly trivial abstractions). I do think there's a lot more value in doing "integration" style testing of the whole query builder and trend runner though, and am gonna do some of those very soon |
Problem
Changes
How did you test this code?