test: Test aliases with BigQuery CTEs. #11439
Conversation
DiegoAlbertoTorres
force-pushed
the
fix-bq-cte-aliasing
branch
from
128077c to
bee0d2c
Compare
DiegoAlbertoTorres
force-pushed
the
fix-bq-cte-aliasing
branch
2 times, most recently
from
eb1524f to
57dd49a
Compare
cpcloud
force-pushed
the
fix-bq-cte-aliasing
branch
from
57dd49a to
5f28a20
Compare
ibis/backends/sql/compilers/base.py
Outdated
| sg.exp.CTE( | ||
| alias=sg.to_identifier(name, quoted=self.quoted), | ||
| this=compiled_ibis_expr, | ||
| ), |
There was a problem hiding this comment.
It looks like this is the main change in this part of the code.
Is the issue that in the previous implementation compiled_query might reference compiled_ibis_expr or its CTEs, which would break topological ordering of CTE definition?
There was a problem hiding this comment.
I guess this change and the one in translate are about the same, just one happens during compilation and this one is during schema validation.
Really the main source of the issue is that sqlglot's with_ produces code with different semantics in some backends (including BQ) in these two cases: (1) with_select_statement.with_(alias, query) and (2) select_statement.with_(alias, query). In (1), sqlglot appends query as the last CTE in with_select_statement, which means that in backends with top-to-bottom CTE evaluation the alias cannot be used by any of the other CTEs. In (2), sqlglot will turn the vanilla select statement into a with-select, and make query into a CTE that is always evaluated before the select's body.
My change ensures case (2) is always used by popping the top-level with arg from all the sqlglot expressions, then re-assembling them into a single with-select statement with all relevant CTEs in the right order and the body of the user query as the select part of the statement.
There was a problem hiding this comment.
Hm, not sure I'm following why (1) is a problem.
It seems like (1) is a scenario that is tough to get into. I realize that your tests are probably covering this, so just bear with me.
It sounds like maybe you're suggesting that it's possible for with_select_statement to already be referencing alias before its with_ method is called, which seems super weird.
Do you have an example of that scenario, or can you point me to which covers this case?
There was a problem hiding this comment.
I think the var names I picked above are a bit confusing, so just to make sure we have the right ordering in our head let's use test_embedded_cte_with_alias_simple as the example: expr is the user Ibis expression with an alias, and the user calls .sql(<with statement>) on it, where the with statement contains a reference to the alias.
Without this PR, Ibis will call parsed_user_query.with_(parsed_ibis_expr), which is how you end up with the weird case where with_select_statement already references the alias.
ibis/backends/sql/compilers/base.py
Outdated
| this = results[cte] | ||
| if "alias" in this.args: | ||
| this = this.this | ||
| modified_cte = sg.exp.CTE( |
There was a problem hiding this comment.
| modified_cte = sg.exp.CTE( | |
| modified_cte = sge.CTE( |
ibis/backends/sql/compilers/base.py
Outdated
|
|
||
| ctes = [ | ||
| *compiled_ibis_expr.ctes, | ||
| sg.exp.CTE( |
There was a problem hiding this comment.
| sg.exp.CTE( | |
| sge.CTE( |
DiegoAlbertoTorres
force-pushed
the
fix-bq-cte-aliasing
branch
from
425796c to
7837754
Compare
Description of changes
Introduces a test to validate use of expression aliasing with CTEs in BigQuery. This functionality is currently broken (and I hope I can fix it, creating this PR first to show the test in the issue).
Issues closed
Resolves #11440