feat(hogql): Allow a placeholder to be used in place of a select statement #19767
Conversation
|
Hey @Gilbert09! 👋 |
|
It looks like the code of |
Gilbert09
temporarily deployed
to
pypi-hogql-parser
GitHub Actions
Inactive
posthog/hogql/grammar/HogQLParser.g4
Outdated
| selectStmtWithParens: selectStmtWithPlaceholder | LPAREN selectUnionStmt RPAREN; | ||
| selectStmtWithPlaceholder: selectStmt | placeholder; |
There was a problem hiding this comment.
Is there a benefit to splitting selectStmtWithPlaceholder out as another node in the hierarchy? Because we could also do selectStmtWithParens: selectStmt | LPAREN selectUnionStmt RPAREN | placeholder;
There was a problem hiding this comment.
Great question, no real reason. Happy to change if we think this is important?
There was a problem hiding this comment.
I don't think it's critical (this works after all), but i'd definitely be nice. Fewer new nodes means fewer potential issues 🤷.
There was a problem hiding this comment.
Okay, this is done now ✅
| @@ -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.
You can simplify this case by folding it into the first condition: just replace if isinstance(query, ast.SelectQuery): with if isinstance(query, (ast.SelectQuery, ast.Placeholder)):
| @@ -344,6 +350,9 @@ class HogQLParseTreeConverter : public HogQLParserBaseVisitor { | |||
| int extend_code = X_PyList_Extend(flattened_queries, sub_select_queries); | |||
| if (extend_code == -1) goto select_queries_loop_py_error; | |||
| Py_DECREF(sub_select_queries); | |||
| } else if (is_ast_node_instance(query, "Placeholder")) { | |||
There was a problem hiding this comment.
Same as in the Python version, this would be more readable if we did || is_ast_node_instance(query, "Placeholder") in the first if () 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 to is_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 some if nesting…
Problem
We weren't able to use a
{placeholder}in place of aSELECTstatement. We only added support for use in table expressions which sadly doesn't cover things like aSELECTin aUNION ALL. The workaround was to useSELECT * FROM {placeholder}, where{placeholder}was anotherSELECT ...statement, causing unnecessary SQL wrappingChanges
selectStmtWithPlaceholderwhich is used to unionselectStmtandplaceholderHow did you test this code?