refactor: Define CTE-related SQL nodes for emitter#2495
refactor: Define CTE-related SQL nodes for emitter#2495TrevorBergeron wants to merge 28 commits intomainfrom
Conversation
chelsea-lin
left a comment
There was a problem hiding this comment.
It would be great if you can check all generated SQL are executable in the bigframes-dev project.
| INNER JOIN `bfcte_1` | ||
| ON COALESCE(CAST(`bfcol_1` AS STRING), '0') = COALESCE(CAST(`bfcol_5` AS STRING), '0') | ||
| AND COALESCE(CAST(`bfcol_1` AS STRING), '1') = COALESCE(CAST(`bfcol_5` AS STRING), '1') | ||
| ), `bfcte_3` AS ( |
There was a problem hiding this comment.
Can you please double check why this SQL is longer? Both bfcte_2 and bfcte_3 are not useful.
tests/unit/core/compile/sqlglot/snapshots/test_compile_isin/test_compile_isin/out.sql
Outdated
Show resolved
Hide resolved
| FROM ( | ||
| SELECT | ||
| *, | ||
| STRUCT(COALESCE(`bfcol_3`, 0) AS `bfpart1_0`, COALESCE(`bfcol_3`, 1) AS `bfpart2_0`) IN ( |
There was a problem hiding this comment.
The bfpart1_0 and bfpart2_0 looks redundancy. bfpart_0 and bfpart_1 would be better. However, these are local alias so that it may not need to generate a global UID. If so, we can use bfpart1/bfpart or other more readable variable naming.
| values.append(sge.convert(value)) | ||
|
|
||
| if expr.dtype == dtypes.BOOL_DTYPE and must_upcast_bools: | ||
| expr = TypedExpr(sge.cast(expr.expr, "INT64"), dtypes.INT_DTYPE) |
There was a problem hiding this comment.
nit: we don't need to keep expr as TypedExpr because we only use the underlying sqlglot expression below. We can simplify this by defining sg_expr=sge.cast(expr.expr, "INT64") and using that directly instead to replace expr.expr.
| } | ||
| new_root = new_root.remap_refs(downstream_mappings) | ||
| # step 1: defined remappings for each individual unique node | ||
| # step 2: bottom up traversal to apply remappings |
There was a problem hiding this comment.
nit: please update these comments.
|
|
||
|
|
||
| @dataclasses.dataclass(frozen=True) | ||
| class SqlWithCtesNode(nodes.BigFrameNode): |
There was a problem hiding this comment.
Can we extend the SqlSelectNode with CTEs rather than creating a new one?
There was a problem hiding this comment.
we can later, when we can better guarantee that the root will be a SqlSelectNode but not every node is a sql node yet, so can't do this yet
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕