refactor(sql): remove temporary table creation when using inline sql #8149

cpcloud · 2024-01-30T11:51:30Z

This PR fixes a long-standing annoyance with our .sql methods. Previously we pooped a bunch of temporary tables or views (depending on the backend), but after this PR the various .sql methods replace this hack with much more vanilla CTEs.

cpcloud · 2024-01-30T12:10:29Z

@ssanderson I was finally able to clean this up ... years later 😂

ibis/expr/operations/relations.py

kszucs · 2024-01-30T12:47:14Z

ibis/backends/base/sqlglot/rewrites.py

@@ -157,7 +157,9 @@ def extract_ctes(node):

    g = Graph.from_bfs(node, filter=(ops.Relation, ops.Subquery, ops.JoinLink))
    for node, dependents in g.invert().items():
-        if len(dependents) > 1 and isinstance(node, cte_types):
+        if isinstance(node, ops.View) or (


Could we avoid having this special case here? IIC it is only to force turning a View into a CTE.

Without this the generated SQL would be a subselect, right?

Without this change a cte won't be created, and View must compile to a cte so it's visible by any subsequent query.

Consider the case of joining two aliased ibis expressions. The aliases need to be visible at the top level for a subsequent join that uses them. An aliased subselect would hide all aliases except the immediate parent. AFAIK only CTEs allow defining multiple named relations with the same scope.

This code still confuses me, you add View here to the list of the relations which should be wrapped with a CTE, but you exclude the same type from the dict comprehension below.

I'm adding it to the list to be able to generate CTEs later, the ibis CTE object is only used in a call to replace, the dict of which filters out the View objects. There's are a few separate components of that process, that are independent of this PR:

The extraction of things that should become CTEs

The replacement of existing nodes with those CTEs

The construction of sqlglot CTEs based on step 1

I am reusing steps 1 and 3 for this PR, but operating on Views.

So basically we want to append the ops.View objects to the list of CTEs. That should be done by ctest + node.find(ops.View) instead, and outside of this function but rather at the compiler.

The list needs to be assembled in topological order, we can't assemble the View CTEs separately from the others.

ibis/backends/duckdb/__init__.py

ibis/expr/types/relations.py

ibis/backends/base/sqlglot/compiler.py

ibis/backends/base/sqlglot/__init__.py

…ss special case-y

…bis-project#8149) This PR fixes a long-standing annoyance with our `.sql` methods. Previously we pooped a bunch of temporary tables or views (depending on the backend), but after this PR the various `.sql` methods replace this hack with much more vanilla CTEs. --------- Co-authored-by: Krisztián Szűcs <szucs.krisztian@gmail.com>

…8149) This PR fixes a long-standing annoyance with our `.sql` methods. Previously we pooped a bunch of temporary tables or views (depending on the backend), but after this PR the various `.sql` methods replace this hack with much more vanilla CTEs. --------- Co-authored-by: Krisztián Szűcs <szucs.krisztian@gmail.com>

…bis-project#8149) This PR fixes a long-standing annoyance with our `.sql` methods. Previously we pooped a bunch of temporary tables or views (depending on the backend), but after this PR the various `.sql` methods replace this hack with much more vanilla CTEs. --------- Co-authored-by: Krisztián Szűcs <szucs.krisztian@gmail.com>

…8149) This PR fixes a long-standing annoyance with our `.sql` methods. Previously we pooped a bunch of temporary tables or views (depending on the backend), but after this PR the various `.sql` methods replace this hack with much more vanilla CTEs. --------- Co-authored-by: Krisztián Szűcs <szucs.krisztian@gmail.com>

…bis-project#8149) This PR fixes a long-standing annoyance with our `.sql` methods. Previously we pooped a bunch of temporary tables or views (depending on the backend), but after this PR the various `.sql` methods replace this hack with much more vanilla CTEs. --------- Co-authored-by: Krisztián Szűcs <szucs.krisztian@gmail.com>

…8149) This PR fixes a long-standing annoyance with our `.sql` methods. Previously we pooped a bunch of temporary tables or views (depending on the backend), but after this PR the various `.sql` methods replace this hack with much more vanilla CTEs. --------- Co-authored-by: Krisztián Szűcs <szucs.krisztian@gmail.com>

…bis-project#8149) This PR fixes a long-standing annoyance with our `.sql` methods. Previously we pooped a bunch of temporary tables or views (depending on the backend), but after this PR the various `.sql` methods replace this hack with much more vanilla CTEs. --------- Co-authored-by: Krisztián Szűcs <szucs.krisztian@gmail.com>

cpcloud added refactor Issues or PRs related to refactoring the codebase ux User experience related issues sql Backends that generate SQL tes-required-for-release Things that must be addressed before *release* of `main` after merging in `the-epic-split` labels Jan 30, 2024

cpcloud requested a review from kszucs January 30, 2024 11:51

cpcloud force-pushed the clean-up-temp-table-poo branch 5 times, most recently from bbfee93 to 6041191 Compare January 30, 2024 12:09