-
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: added support for daily and weekly active user aggregations #18101
Conversation
from posthog.schema import ActionsNode, EventsNode | ||
|
||
|
||
class QueryModifier: |
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.
Perhaps we can rename this to something else? We already have the system of "hogql query modifiers" inside modifier.py
, which sets things like PoE on/off, cohorts via join/subquery, etc. To avoid confusing people, can we find something else for this? QueryAlternator
🙈?
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.
Aha, yep! I can change this - shame you can't scope classes to just the file they're defined in
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.
Some more comments, but overall I think this works. Happy to merge and go on.
def appendSelect(self, expr: ast.Expr) -> None: | ||
self._selects.append(expr) | ||
|
||
def appendGroupBy(self, expr: ast.Expr) -> None: | ||
self._group_bys.append(expr) | ||
|
||
def replaceSelectFrom(self, join_expr: ast.JoinExpr) -> None: | ||
self._select_from = join_expr | ||
|
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.
Our adherence to this is a mixed bag... but normally python prefers snake_case
method names
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.
Ahh, that's me falling into JS habits 🤦 Not intentional - will change
from posthog.schema import ActionsNode, EventsNode | ||
|
||
|
||
class QueryAlternator: |
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.
Might be good to leave a line of prose for the future traveller, as to what this class does, without having to go through all the code.
…8101) * Added support for daily and weekly active user aggregations * Removed unused join * Fixed tests and applied Sample placeholders * Fixed formula running on an empty string * Updated the name of QueryModifier * Comments and fixed name casing
…stHog#18101) * Added support for daily and weekly active user aggregations * Removed unused join * Fixed tests and applied Sample placeholders * Fixed formula running on an empty string * Updated the name of QueryModifier * Comments and fixed name casing
Problem
Changes
QueryBuilder
, making it the orchestrator of putting the query together, with other classes returning AST nodesAggregationOperations
class to organize the extra joins needed when doing the likes ofweekly active users
QueryModifier
class. More are to come for the aggregation stuff soonHow did you test this code?