-
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): Allow a placeholder to be used in place of a select statement #19767
Changes from all commits
40353c3
a5a24e8
ef0363e
8be900d
ce16fb9
abdc430
7c1008e
6d4e707
89624d5
f525bb3
e0d1406
bc41be7
fd443c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,7 @@ | |
|
||
setup( | ||
name="hogql_parser", | ||
version="1.0.2", | ||
version="1.0.3", | ||
url="https://github.com/PostHog/posthog/tree/master/hogql_parser", | ||
author="PostHog Inc.", | ||
author_email="[email protected]", | ||
|
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -172,14 +172,16 @@ def visitSelectUnionStmt(self, ctx: HogQLParser.SelectUnionStmtContext): | |
flattened_queries.append(query) | ||
elif isinstance(query, ast.SelectUnionQuery): | ||
flattened_queries.extend(query.select_queries) | ||
elif isinstance(query, ast.Placeholder): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can simplify this case by folding it into the first condition: just replace |
||
flattened_queries.append(query) | ||
else: | ||
raise Exception(f"Unexpected query node type {type(query).__name__}") | ||
if len(flattened_queries) == 1: | ||
return flattened_queries[0] | ||
return ast.SelectUnionQuery(select_queries=flattened_queries) | ||
|
||
def visitSelectStmtWithParens(self, ctx: HogQLParser.SelectStmtWithParensContext): | ||
return self.visit(ctx.selectStmt() or ctx.selectUnionStmt()) | ||
return self.visit(ctx.selectStmt() or ctx.selectUnionStmt() or ctx.placeholder()) | ||
|
||
def visitSelectStmt(self, ctx: HogQLParser.SelectStmtContext): | ||
select_query = ast.SelectQuery( | ||
|
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.
Same as in the Python version, this would be more readable if we did
|| is_ast_node_instance(query, "Placeholder")
in the firstif ()
branch. (I know readability is not the C++ code's strong suit, but that makes reining it in all the more important.)BTW
is_ast_node_instance(query, "SelectQuery");
is called twice in this function, which is an oversight on my side. It's intentionally assigned tois_select_query
so that we can check against the error value (-1
), which is great for perfect error handling. However, two lines later repeat that call without the check…These operations are very low risk, so I think in this case I suggest we remove lines 341 and 342. Because if were to run the same error checks for all branches in an
else if
fashion, we'd have to do quite someif
nesting…