-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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): support join USING #21916
Changes from 5 commits
d461452
e47a42b
28595ed
3c98359
9d19eee
121c1aa
21dc28a
50e6b3f
3377624
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -275,12 +275,17 @@ | |
return response | ||
|
||
if isinstance(node.table, ast.Field): | ||
node = cast(ast.JoinExpr, clone_expr(node)) | ||
if node.constraint and node.constraint.constraint_type == "USING": | ||
# visit USING constraint before adding the table to avoid ambiguous names | ||
node.constraint = self.visit_join_constraint(node.constraint) | ||
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.
|
||
|
||
table_name = node.table.chain[0] | ||
Check failure on line 283 in posthog/hogql/resolver.py GitHub Actions / Python code quality checks
Check failure on line 283 in posthog/hogql/resolver.py GitHub Actions / Python code quality checks
|
||
table_alias = node.alias or table_name | ||
if table_alias in scope.tables: | ||
raise QueryError(f'Already have joined a table called "{table_alias}". Can\'t redefine.') | ||
|
||
database_table = self.database.get_table(table_name) | ||
|
||
if isinstance(database_table, SavedQuery): | ||
self.current_view_depth += 1 | ||
|
@@ -293,7 +298,7 @@ | |
if isinstance(node.table, ast.SelectQuery): | ||
node.table.view_name = database_table.name | ||
|
||
node.alias = table_alias or database_table.name | ||
node = self.visit(node) | ||
|
||
self.current_view_depth -= 1 | ||
|
@@ -307,13 +312,13 @@ | |
# Always add an alias for function call tables. This way `select table.* from table` is replaced with | ||
# `select table.* from something() as table`, and not with `select something().* from something()`. | ||
if table_alias != table_name or isinstance(database_table, FunctionCallTable): | ||
node_type = ast.TableAliasType(alias=table_alias, table_type=node_table_type) | ||
else: | ||
node_type = node_table_type | ||
|
||
scope.tables[table_alias] = node_type | ||
Check failure on line 319 in posthog/hogql/resolver.py GitHub Actions / Python code quality checks
|
||
|
||
# :TRICKY: Make sure to clone and visit _all_ JoinExpr fields/nodes. | ||
node = cast(ast.JoinExpr, clone_expr(node)) | ||
node.type = node_type | ||
node.table = cast(ast.Field, clone_expr(node.table)) | ||
node.table.type = node_table_type | ||
|
@@ -330,7 +335,8 @@ | |
if is_global: | ||
node.next_join.join_type = "GLOBAL JOIN" | ||
|
||
node.constraint = self.visit(node.constraint) | ||
if node.constraint and node.constraint.constraint_type == "ON": | ||
node.constraint = self.visit_join_constraint(node.constraint) | ||
node.sample = self.visit(node.sample) | ||
|
||
# In case we had a function call table, and had to add an alias where none was present, mark it here | ||
|
@@ -341,6 +347,9 @@ | |
|
||
elif isinstance(node.table, ast.SelectQuery) or isinstance(node.table, ast.SelectUnionQuery): | ||
node = cast(ast.JoinExpr, clone_expr(node)) | ||
if node.constraint and node.constraint.constraint_type == "USING": | ||
# visit USING constraint before adding the table to avoid ambiguous names | ||
node.constraint = self.visit_join_constraint(node.constraint) | ||
|
||
node.table = super().visit(node.table) | ||
if isinstance(node.table, ast.SelectQuery) and node.table.view_name is not None and node.alias is not None: | ||
|
@@ -365,7 +374,8 @@ | |
|
||
# :TRICKY: Make sure to clone and visit _all_ JoinExpr fields/nodes. | ||
node.next_join = self.visit(node.next_join) | ||
node.constraint = self.visit(node.constraint) | ||
if node.constraint and node.constraint.constraint_type == "ON": | ||
node.constraint = self.visit_join_constraint(node.constraint) | ||
node.sample = self.visit(node.sample) | ||
|
||
return node | ||
|
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.
IMO it doesn't make sense and will reduce readability if I asserted in tests that these fields are not None, but I could do it.
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.
Yup that's fine. I agree that additional asserts wouldn't help code quality.