From f27a1f7e7e07ef7355dd53a887916a29628be1fa Mon Sep 17 00:00:00 2001 From: Denis Hirn Date: Thu, 11 Jul 2024 10:05:52 +0200 Subject: [PATCH] Fix several CTE related issues Materialized CTEs did not work properly with INSERT statements TransformRecursiveCTE was too restrictive Materialized CTEs were sometimes inlined and thereby duplicated Fix PIVOT statements after materialized CTE fixes --- .../transform/helpers/transform_cte.cpp | 9 ++-- .../transform/statement/transform_insert.cpp | 1 - .../statement/transform_pivot_stmt.cpp | 2 +- src/planner/binder.cpp | 2 +- .../binder/tableref/bind_basetableref.cpp | 9 ++++ .../materialized/test_cte_materialized.test | 4 +- .../materialized/test_materialized_cte.test | 9 ++++ test/sql/cte/test_cte.test | 41 +++++++++++++++++++ 8 files changed, 65 insertions(+), 12 deletions(-) diff --git a/src/parser/transform/helpers/transform_cte.cpp b/src/parser/transform/helpers/transform_cte.cpp index a481dac2483b..544442f40ba3 100644 --- a/src/parser/transform/helpers/transform_cte.cpp +++ b/src/parser/transform/helpers/transform_cte.cpp @@ -100,9 +100,7 @@ unique_ptr Transformer::TransformRecursiveCTE(duckdb_libpgquery unique_ptr select; switch (stmt.op) { - case duckdb_libpgquery::PG_SETOP_UNION: - case duckdb_libpgquery::PG_SETOP_EXCEPT: - case duckdb_libpgquery::PG_SETOP_INTERSECT: { + case duckdb_libpgquery::PG_SETOP_UNION: { select = make_uniq(); select->node = make_uniq_base(); auto &result = select->node->Cast(); @@ -111,11 +109,10 @@ unique_ptr Transformer::TransformRecursiveCTE(duckdb_libpgquery result.left = TransformSelectNode(*PGPointerCast(stmt.larg)); result.right = TransformSelectNode(*PGPointerCast(stmt.rarg)); result.aliases = info.aliases; - if (stmt.op != duckdb_libpgquery::PG_SETOP_UNION) { - throw ParserException("Unsupported setop type for recursive CTE: only UNION or UNION ALL are supported"); - } break; } + case duckdb_libpgquery::PG_SETOP_EXCEPT: + case duckdb_libpgquery::PG_SETOP_INTERSECT: default: // This CTE is not recursive. Fallback to regular query transformation. return TransformSelect(*PGPointerCast(cte.ctequery)); diff --git a/src/parser/transform/statement/transform_insert.cpp b/src/parser/transform/statement/transform_insert.cpp index 491632bbd758..dfa3c25012c9 100644 --- a/src/parser/transform/statement/transform_insert.cpp +++ b/src/parser/transform/statement/transform_insert.cpp @@ -42,7 +42,6 @@ unique_ptr Transformer::TransformInsert(duckdb_libpgquery::PGIn } if (stmt.selectStmt) { result->select_statement = TransformSelect(stmt.selectStmt, false); - result->select_statement->node = TransformMaterializedCTE(std::move(result->select_statement->node)); } else { result->default_values = true; } diff --git a/src/parser/transform/statement/transform_pivot_stmt.cpp b/src/parser/transform/statement/transform_pivot_stmt.cpp index 4bfb1c46d3f9..fee41aada399 100644 --- a/src/parser/transform/statement/transform_pivot_stmt.cpp +++ b/src/parser/transform/statement/transform_pivot_stmt.cpp @@ -94,7 +94,7 @@ unique_ptr Transformer::GenerateCreateEnumStmt(unique_ptr(); - select->node = std::move(subselect); + select->node = TransformMaterializedCTE(std::move(subselect)); info->query = std::move(select); info->type = LogicalType::INVALID; diff --git a/src/planner/binder.cpp b/src/planner/binder.cpp index 7de9d98bd894..ef41465f71fd 100644 --- a/src/planner/binder.cpp +++ b/src/planner/binder.cpp @@ -146,7 +146,7 @@ BoundStatement Binder::Bind(SQLStatement &statement) { case StatementType::SELECT_STATEMENT: return Bind(statement.Cast()); case StatementType::INSERT_STATEMENT: - return Bind(statement.Cast()); + return BindWithCTE(statement.Cast()); case StatementType::COPY_STATEMENT: return Bind(statement.Cast(), CopyToType::COPY_TO_FILE); case StatementType::DELETE_STATEMENT: diff --git a/src/planner/binder/tableref/bind_basetableref.cpp b/src/planner/binder/tableref/bind_basetableref.cpp index 5455a53a9645..2f7b09866c00 100644 --- a/src/planner/binder/tableref/bind_basetableref.cpp +++ b/src/planner/binder/tableref/bind_basetableref.cpp @@ -129,6 +129,15 @@ unique_ptr Binder::Bind(BaseTableRef &ref) { // retry with next candidate CTE continue; } + + // If we have found a materialized CTE, but no corresponding CTE binding, + // something is wrong. + if (cte.materialized == CTEMaterialize::CTE_MATERIALIZE_ALWAYS) { + throw BinderException( + "There is a WITH item named \"%s\", but it cannot be references from this part of the query.", + ref.table_name); + } + // Move CTE to subquery and bind recursively SubqueryRef subquery(unique_ptr_cast(cte.query->Copy())); subquery.alias = ref.alias.empty() ? ref.table_name : ref.alias; diff --git a/test/sql/cte/materialized/test_cte_materialized.test b/test/sql/cte/materialized/test_cte_materialized.test index 9506ce22f836..7cccdc813a1b 100644 --- a/test/sql/cte/materialized/test_cte_materialized.test +++ b/test/sql/cte/materialized/test_cte_materialized.test @@ -48,11 +48,9 @@ with cte1 as MATERIALIZED (select 42), cte1 as MATERIALIZED (select 42) select * ---- # reference to CTE before its actually defined -query I +statement error with cte3 as MATERIALIZED (select ref2.j as i from cte1 as ref2), cte1 as MATERIALIZED (Select i as j from a), cte2 as MATERIALIZED (select ref.j+1 as k from cte1 as ref) select * from cte2 union all select * FROM cte3; ---- -43 -42 # multiple uses of same CTE query II diff --git a/test/sql/cte/materialized/test_materialized_cte.test b/test/sql/cte/materialized/test_materialized_cte.test index 193c32971a69..3895d16cdf94 100644 --- a/test/sql/cte/materialized/test_materialized_cte.test +++ b/test/sql/cte/materialized/test_materialized_cte.test @@ -75,3 +75,12 @@ WITH t(x) AS MATERIALIZED (SELECT 1), ---- 2 1 + +statement error +WITH t0(x) AS MATERIALIZED ( + SELECT x FROM t1 +), t1(x) AS MATERIALIZED ( + SELECT 1 +) +SELECT * FROM t0; +---- \ No newline at end of file diff --git a/test/sql/cte/test_cte.test b/test/sql/cte/test_cte.test index f68d6d9c6e8d..d240ef4e1323 100644 --- a/test/sql/cte/test_cte.test +++ b/test/sql/cte/test_cte.test @@ -130,3 +130,44 @@ from cte where alias2 > 0; ---- 1 1 + +# recursive CTE and a non-recursive CTE with except +query I +WITH RECURSIVE t AS ( + SELECT 1 AS a + UNION ALL + SELECT a+1 + FROM t + WHERE a < 10 +), s AS ( + (VALUES (5), (6), (7), (8), (9), (10), (11), (12), (13), (42)) + EXCEPT + TABLE t +) +SELECT * FROM s AS _(x) ORDER BY x; +---- +11 +12 +13 +42 + +# recursive CTE with except in recursive part (but not as a recursive anchor) +query I +WITH RECURSIVE t AS ( + select 1 as a + union all + (select a+1 + from t + where a < 10 + except + SELECT 4) +), s AS ( + (values (5), (6), (7)) + except + table t +) +SELECT * FROM s AS _(x) ORDER BY x; +---- +5 +6 +7