ADBDEV-6314: Fix unexpected output for empty grouping set

[mos65o2]

Prohibiting grouping sets with ungrouped attributes in TargetList

Problem:
Queries with grouping expressions and ungrouped attributes in TL could
produce unexpected output if the group did not have a primary key.

Example with 3 columns where "a" is a primary key:
SELECT a,b,c FROM t GROUP BY GROUPING SETS((a),(b));

Grouping "(a)" had correct output of columns "a", "b", "c". Attribute "b" in
group "(b)" is not a primary key, so it is not unique, so it can be grouped.
In this case, the output of column "c" was undefined. This also applied to
"GROUPING SETS ((a),())", ROLLUP and CUBE (detailed behavior is
described in tests with ungrouped check).
Grouping expressions were introduced in PostgreSQL 9.5 (f3d3118) and
such queries were prohibited. And this patch aligns the behavior with that.

Solution:
Improve parse-stage validation for ungrouped columns when using grouping
expressions.

Changes:
Similar to PostgreSQL, the parsing stage for queries with grouping
expressions reads common attributes for all groups. If any, they are placed in
"groupClauseCommonVars". In case of ungrouped attributes in TL, this list is
used to check for PK in groups.
Compared to the original, the "groupClauses" list uses TargetEntry rather than
an expression. Added function to get common refsortgroupref in grouping
clauses. At the beginning, the function assumes that all expressions are found
in grouping clauses. By running through the groupings, missing expressions
are excluded from the common refs list. The extraction function in the form
"list_intersection_int" taken from f3d3118.
The patch does not change the behavior of regular GROUP BY.

Co-authored-by: anarazel [email protected]

[red1452]

You wrote, that changes are similar to Postgres 9.5, but I did not find the same sources there. Can you give the commit, which is example of your patch? Or can you backport such commit?

[mos65o2]

You wrote, that changes are similar to Postgres 9.5, but I did not find the same sources there. Can you give the commit, which is example of your patch?

f3d3118 introduces support in PG 9.5. Also in GPDB7.

Or can you backport such commit?

I discussed this issue with @Stolb27. I only need to correct the parser behavior without porting. My changes only concern the ungrouped columns check. Whereas the patch in PostgreSQL makes changes to the planner and much more.

[red1452]

When I try the functional_deps solely (with optimizer enabled), it fails on my side.
It appears that it is due to GUC optimizer_enable_table_alias. If I set it to off in the test, the test passes. For ex with sample patch below:

diff --git a/src/test/regress/sql/functional_deps.sql b/src/test/regress/sql/functional_deps.sql
index 878a595259..5026b275f9 100644
--- a/src/test/regress/sql/functional_deps.sql
+++ b/src/test/regress/sql/functional_deps.sql
@@ -2,6 +2,9 @@
 
 -- Enable logs that allow to show GPORCA failed to produce a plan
 SET optimizer_trace_fallback = on;
+-- start_ignore
+SET optimizer_enable_table_alias = off;
+-- end_ignore
 
 CREATE TEMP TABLE articles (
     id int CONSTRAINT articles_pkey PRIMARY KEY,
@@ -429,3 +432,6 @@ DROP TABLE test_table3;
 
 RESET optimizer_trace_fallback;
 RESET optimizer;
+-- start_ignore
+RESET optimizer_enable_table_alias;
+-- end_ignore

I guess that the CI pipeline passes Ok, because some other test disables this GUC, and doesn't reset it back.

It is a separate issue (as it was presented before your patch). But I believe we need to keep tests as low coupled with each other, as possible. So it is better to fix this problem some day soon. Can you please re-check on your side and create a ticket for this problem?

Tests for GPDB6 is run with "optimizer_enable_table_alias=off". You do not need to set GUC in the test.

gpdb/concourse/scripts/common.bash

Line 92 in c69f669

export OPTIMIZER_ENABLE_TABLE_ALIAS=off

[mos65o2]

In the description:

"groping extensions"

groping -> grouping

And I guess not "extensions", but "expressions". Need to update it throughout the text.

Also, it is better to use past tense when describing previous wrong behaviour, and present tense for new/current behaviour.

I fixed the typo and the use of tense. I hope the meaning of "grouping expressions" and expressions (Vars) does not overlap. I will also correct the comment in the code.
Perhaps it would be worth mentioning "grouping expressions" in the title, rather than "grouping sets", since this affects ROLLUP and CUBE.

[mos65o2]

It is necessary to select common attributes among top-level groupings.
This case degenerates into a simple GROUP BY, despite the absence of common attributes among the top-level GROUPING SETS, so these queries work:
create table t (a int primary key, b int, c int, d int);

select a, b, c from t group by GROUPING SETS(a), GROUPING SETS (a,()); ->
select a, b, c from t group by a, GROUPING SETS (a,());

select a, b, c from t group by GROUPING SETS(a), GROUPING SETS (b,()); ->
select a, b, c from t group by a, GROUPING SETS (b,());

It is necessary to take into account nesting. This query also degenerates into a regular GROUP BY and works:
select a, b, c from t group by GROUPING SETS (GROUPING SETS ((a), GROUPING SETS ((grouping sets(a),a)), GROUPING SETS (b,());

This query does not work:
select a, b, c from t group by GROUPING SETS (GROUPING SETS(a), GROUPING SETS (GROUPING SETS(b),a)), GROUPING SETS (b,());

With a composite key it works differently (does not work):
create table t2 (a int, b text, c int, PRIMARY KEY(a,c));
select a, b, c from t2 group by GROUPING SETS (a,c), GROUPING SETS (a,())
All queries are tested in PG 9.5.

[mos65o2]

Followins query:

create table funcdep1(a int primary key, b int, c int, d int);
explain (costs off) select a, b, c from funcdep1 group by GROUPING SETS(a), GROUPING SETS(a, ());

while on your branch it reports an error:

I have changed the behavior. Please check. Pay attention to the tests.

[red1452]

Add note about f3d3118 to the description.

[BenderArenadata]

Allure report https://allure.adsw.io/launch/86899

[BenderArenadata]

Allure report https://allure.adsw.io/launch/87081

[whitehawk]

"is common if it appears in several grouping expressions
* (including nesting) and in a regular GROUP BY (general case)."

Not if

"is common if it appears in all grouping expressions
* (including nesting) or in a regular GROUP BY (general case)."

?

[mos65o2]

Then for the case of GROUPING SETS(a) I will point out that it degenerates into a regular GROUP BY.

[red1452]

extra spaces: if (!grpcl)

[red1452]

do return NIL;, doing return result is better only if it does not empty.

[red1452]

List *result = NIL;

[red1452]

space

[red1452]

If you add return result; here, you may remove the last else with brackets and one tab in each line inside.