feat(hogql): Add paths query runner #19761
Conversation
|
Size Change: +44 B (0%) Total Size: 2 MB ℹ️ View Unchanged
|
|
@mariusandra I was starting on the tests but got heavily sidetracked with Celery work. Maybe you can take a look at this here already, and we can merge it as first step? |
There was a problem hiding this comment.
Great work porting this query. I think we can merge it in... after just one category of fixes: I'd remove as many f-string usages as possible in parse_expr calls, and replace them with placeholders. It's generally safer. f-strings might be "faster", and if you inject SQL inside a HogQL query... then it's the same as sending garbage under the "SQL" tab, so there's no extra danger. However there will eventually be someone who manages to get unescaped quotes or other bad symbols into here, causing the query to just fail.
| @@ -431,6 +431,7 @@ const handleQuerySourceUpdateSideEffects = ( | |||
| * Date range change side effects. | |||
| */ | |||
| if ( | |||
| !isPathsQuery(currentState) && // TODO: Apply side logic more elegantly | |||
There was a problem hiding this comment.
It added an interval even if there was none. And it's not supported by paths.
| if len(node.exprs) == 1: | ||
| return self.visit(node.exprs[0]) | ||
| return f"and({', '.join([self.visit(expr) for expr in node.exprs])})" | ||
|
|
||
| def visit_or(self, node: ast.Or): | ||
| if len(node.exprs) == 1: | ||
| return self.visit(node.exprs[0]) |
|
Also, it's now a race between this PR and this one. I'm not sure which should go in first, and who should fix the other 😅 |
# Conflicts: # mypy-baseline.txt
Co-authored-by: Marius Andra <marius.andra@gmail.com>
# Conflicts: # posthog/schema.py
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Problem
We want to convert the paths query to HogQL as well. See #19641
Changes
How did you test this code?