This is an automated email from the ASF dual-hosted git repository. yjhjstz pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/cloudberry.git
commit a8729fc9f36d71f5ce2e5caec8e270c52096487c Author: Zhenghua Lyu <[email protected]> AuthorDate: Wed Mar 23 14:09:53 2022 +0800 Fix banning window agg in recursive queries. Greenplum does not support window agg in the target list of recursive part of recursive CTE. Current implementation of recursive queries is based on Postgres' Worktable Scan and RecursiveUnion, this forces the two nodes must be in the same slice, means no motion node is allowed between them. The locus of worktable scan is determined by the non-recurisve part of path thus it is hard to avoid motion over worktable scan when there are window aggs. This should be the reason why Greenplum does not support this feature. However, previous code only checks the direct raw expression in parse tree's targetlist, this is not enough, since it might be a compound expression that involving window aggs, like a type cast expression. We should use raw_expression_tree_walker to search window functions. This commit fixes Issue: https://github.com/greenplum-db/gpdb/issues/13299, more details can be found there. For more discussion on this feature and furture plan, please refer to https://groups.google.com/a/greenplum.org/g/gpdb-dev/c/GIYw6t-uX7s. --- src/backend/parser/parse_cte.c | 67 +++++++++++++++------- src/test/regress/expected/with.out | 2 +- src/test/regress/expected/with_clause.out | 38 +++++++++++- .../regress/expected/with_clause_optimizer.out | 36 ++++++++++++ src/test/regress/expected/with_optimizer.out | 2 +- src/test/regress/sql/with_clause.sql | 31 ++++++++++ src/test/singlenode_regress/expected/with.out | 2 +- 7 files changed, 153 insertions(+), 25 deletions(-) diff --git a/src/backend/parser/parse_cte.c b/src/backend/parser/parse_cte.c index cd098c1c03..ecc27df766 100644 --- a/src/backend/parser/parse_cte.c +++ b/src/backend/parser/parse_cte.c @@ -1249,30 +1249,57 @@ checkSelfRefInRangeSubSelect(SelectStmt *stmt, CteState *cstate) } /* - * Check if the recursive term of a recursive cte contains a window function. - * This is currently not supported and is checked for in the parsing stage + * GPDB: + * Check if the recursive term of a recursive cte contains a window function + * or ordered set aggregate function (special aggs). This is currently not + * supported and is checked for in the parsing stage. Refer to dicussion: + * https://groups.google.com/a/greenplum.org/g/gpdb-dev/c/GIYw6t-uX7s */ -static void -checkWindowFuncInRecursiveTerm(SelectStmt *stmt, CteState *cstate) +typedef struct CTEWindowAggSearchContext { - ListCell *lc; - foreach(lc, stmt->targetList) + bool found; /* flag to show if we have found */ + FuncCall *func; /* if found is true, this field is the Agg */ +} CTEWindowAggSearchContext; + +static bool +cte_window_agg_walker(Node *node, CTEWindowAggSearchContext *context) +{ + if (node == NULL) + return false; + + if (IsA(node, FuncCall)) { - if (IsA((Node *) lfirst(lc), ResTarget)) + FuncCall *fc = (FuncCall *) node; + if (fc->over != NULL) { - ResTarget *rt = (ResTarget *) lfirst(lc); - if (IsA(rt->val, FuncCall)) - { - FuncCall *fc = (FuncCall *) rt->val; - if (fc->over != NULL) - { - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("window functions in a recursive query is not implemented"), - parser_errposition(cstate->pstate, - exprLocation((Node *) fc)))); - } - } + context->found = true; + context->func = fc; + + return true; } } + + return raw_expression_tree_walker(node, cte_window_agg_walker, context); +} + +static void +checkWindowFuncInRecursiveTerm(SelectStmt *stmt, CteState *cstate) +{ + CTEWindowAggSearchContext context; + + context.found = false; + context.func = NULL; + + (void) raw_expression_tree_walker((Node *) stmt->targetList, + cte_window_agg_walker, + (void *) &context); + + if (context.found) + { + ereport(ERROR, + (errcode(ERRCODE_GP_FEATURE_NOT_YET), + errmsg("window functions in the target list of a recursive query is not supported"), + parser_errposition(cstate->pstate, + exprLocation((Node *) context.func)))); + } } diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out index ac33dc53a8..14944834c8 100644 --- a/src/test/regress/expected/with.out +++ b/src/test/regress/expected/with.out @@ -1857,7 +1857,7 @@ WITH RECURSIVE x(n) AS ( UNION ALL SELECT level+1, row_number() over() FROM x, bar) SELECT * FROM x LIMIT 10; -ERROR: window functions in a recursive query is not implemented +ERROR: window functions in the target list of a recursive query is not supported LINE 4: SELECT level+1, row_number() over() FROM x, bar) ^ CREATE TEMPORARY TABLE y (a INTEGER) DISTRIBUTED RANDOMLY; diff --git a/src/test/regress/expected/with_clause.out b/src/test/regress/expected/with_clause.out index dbed613c81..018235254d 100644 --- a/src/test/regress/expected/with_clause.out +++ b/src/test/regress/expected/with_clause.out @@ -1,8 +1,6 @@ drop table if exists with_test1 cascade; NOTICE: table "with_test1" does not exist, skipping create table with_test1 (i int, t text, value int) distributed by (i); -NOTICE: Table doesn't have 'DISTRIBUTED BY' clause -- Using column named 'i' as the Cloudberry Database data distribution key for this table. -HINT: The 'DISTRIBUTED BY' clause determines the distribution of data. Make sure column(s) chosen are the optimal data distribution key to minimize skew. insert into with_test1 select i%10, 'text' || i%20, i%30 from generate_series(0, 99) i; analyze with_test1; drop table if exists with_test2 cascade; @@ -2195,6 +2193,42 @@ WITH RECURSIVE r1 AS ( ) SELECT * FROM r1 LIMIT 1; ERROR: joining nested RECURSIVE clauses is not supported +-- GPDB +-- Greenplum does not support window functions in recursive part's target list +-- See issue https://github.com/greenplum-db/gpdb/issues/13299 for details. +-- Previously the following SQL will PANIC or Assert Fail if compiled with assert. +create table t_window_ordered_set_agg_rte(a bigint, b bigint, c bigint); +NOTICE: Table doesn't have 'DISTRIBUTED BY' clause -- Using column named 'a' as the Greenplum Database data distribution key for this table. +HINT: The 'DISTRIBUTED BY' clause determines the distribution of data. Make sure column(s) chosen are the optimal data distribution key to minimize skew. +insert into t_window_ordered_set_agg_rte select i,i,i from generate_series(1, 10)i; +-- should error out during parse-analyze +with recursive rcte(x,y) as +( + select a, b from t_window_ordered_set_agg_rte + union all + select (first_value(c) over (partition by b))::int, a+x + from rcte, + t_window_ordered_set_agg_rte as t + where t.b = x +) +select * from rcte limit 10; +ERROR: window functions in the target list of a recursive query is not supported +LINE 5: select (first_value(c) over (partition by b))::int, a+x + ^ +-- should error out during parse-analyze +with recursive rcte(x,y) as +( + select a, b from t_window_ordered_set_agg_rte + union all + select first_value(c) over (partition by b), a+x + from rcte, + t_window_ordered_set_agg_rte as t + where t.b = x +) +select * from rcte limit 10; +ERROR: window functions in the target list of a recursive query is not supported +LINE 5: select first_value(c) over (partition by b), a+x + ^ -- This used to deadlock, before the IPC between ShareInputScans across -- slices was rewritten. set gp_cte_sharing=on; diff --git a/src/test/regress/expected/with_clause_optimizer.out b/src/test/regress/expected/with_clause_optimizer.out index fe1e4e41ea..2e50a1f283 100644 --- a/src/test/regress/expected/with_clause_optimizer.out +++ b/src/test/regress/expected/with_clause_optimizer.out @@ -2196,6 +2196,42 @@ WITH RECURSIVE r1 AS ( ) SELECT * FROM r1 LIMIT 1; ERROR: joining nested RECURSIVE clauses is not supported +-- GPDB +-- Greenplum does not support window functions in recursive part's target list +-- See issue https://github.com/greenplum-db/gpdb/issues/13299 for details. +-- Previously the following SQL will PANIC or Assert Fail if compiled with assert. +create table t_window_ordered_set_agg_rte(a bigint, b bigint, c bigint); +NOTICE: Table doesn't have 'DISTRIBUTED BY' clause -- Using column named 'a' as the Greenplum Database data distribution key for this table. +HINT: The 'DISTRIBUTED BY' clause determines the distribution of data. Make sure column(s) chosen are the optimal data distribution key to minimize skew. +insert into t_window_ordered_set_agg_rte select i,i,i from generate_series(1, 10)i; +-- should error out during parse-analyze +with recursive rcte(x,y) as +( + select a, b from t_window_ordered_set_agg_rte + union all + select (first_value(c) over (partition by b))::int, a+x + from rcte, + t_window_ordered_set_agg_rte as t + where t.b = x +) +select * from rcte limit 10; +ERROR: window functions in the target list of a recursive query is not supported +LINE 5: select (first_value(c) over (partition by b))::int, a+x + ^ +-- should error out during parse-analyze +with recursive rcte(x,y) as +( + select a, b from t_window_ordered_set_agg_rte + union all + select first_value(c) over (partition by b), a+x + from rcte, + t_window_ordered_set_agg_rte as t + where t.b = x +) +select * from rcte limit 10; +ERROR: window functions in the target list of a recursive query is not supported +LINE 5: select first_value(c) over (partition by b), a+x + ^ -- This used to deadlock, before the IPC between ShareInputScans across -- slices was rewritten. set gp_cte_sharing=on; diff --git a/src/test/regress/expected/with_optimizer.out b/src/test/regress/expected/with_optimizer.out index 19eeacf4eb..fa150c6aa5 100644 --- a/src/test/regress/expected/with_optimizer.out +++ b/src/test/regress/expected/with_optimizer.out @@ -1866,7 +1866,7 @@ WITH RECURSIVE x(n) AS ( UNION ALL SELECT level+1, row_number() over() FROM x, bar) SELECT * FROM x LIMIT 10; -ERROR: window functions in a recursive query is not implemented +ERROR: window functions in the target list of a recursive query is not supported LINE 4: SELECT level+1, row_number() over() FROM x, bar) ^ CREATE TEMPORARY TABLE y (a INTEGER) DISTRIBUTED RANDOMLY; diff --git a/src/test/regress/sql/with_clause.sql b/src/test/regress/sql/with_clause.sql index 16fb7f8d8a..0ef209415a 100644 --- a/src/test/regress/sql/with_clause.sql +++ b/src/test/regress/sql/with_clause.sql @@ -369,6 +369,37 @@ WITH RECURSIVE r1 AS ( ) SELECT * FROM r1 LIMIT 1; +-- GPDB +-- Greenplum does not support window functions in recursive part's target list +-- See issue https://github.com/greenplum-db/gpdb/issues/13299 for details. +-- Previously the following SQL will PANIC or Assert Fail if compiled with assert. +create table t_window_ordered_set_agg_rte(a bigint, b bigint, c bigint); + +insert into t_window_ordered_set_agg_rte select i,i,i from generate_series(1, 10)i; + +-- should error out during parse-analyze +with recursive rcte(x,y) as +( + select a, b from t_window_ordered_set_agg_rte + union all + select (first_value(c) over (partition by b))::int, a+x + from rcte, + t_window_ordered_set_agg_rte as t + where t.b = x +) +select * from rcte limit 10; + +-- should error out during parse-analyze +with recursive rcte(x,y) as +( + select a, b from t_window_ordered_set_agg_rte + union all + select first_value(c) over (partition by b), a+x + from rcte, + t_window_ordered_set_agg_rte as t + where t.b = x +) +select * from rcte limit 10; -- This used to deadlock, before the IPC between ShareInputScans across -- slices was rewritten. diff --git a/src/test/singlenode_regress/expected/with.out b/src/test/singlenode_regress/expected/with.out index da0602e655..886674bcf5 100644 --- a/src/test/singlenode_regress/expected/with.out +++ b/src/test/singlenode_regress/expected/with.out @@ -1850,7 +1850,7 @@ WITH RECURSIVE x(n) AS ( UNION ALL SELECT level+1, row_number() over() FROM x, bar) SELECT * FROM x LIMIT 10; -ERROR: window functions in a recursive query is not implemented +ERROR: window functions in the target list of a recursive query is not supported LINE 4: SELECT level+1, row_number() over() FROM x, bar) ^ CREATE TEMPORARY TABLE y (a INTEGER); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
