public inbox for [email protected]
help / color / mirror / Atom feedFrom: Zhang Mingli <[email protected]>
To: songjinzhou <[email protected]>
To: Japin Li <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: David Rowley <[email protected]>
Subject: Re: Modify an incorrect regression test case in the group by key value elimination function
Date: Tue, 18 Feb 2025 19:09:08 +0800
Message-ID: <c9b3ac67-bb2e-4dd8-ab63-8b343af5ec38@Spark> (raw)
In-Reply-To: <ME0P300MB04455DD194CE6F5A8CAFAD85B6FA2@ME0P300MB0445.AUSP300.PROD.OUTLOOK.COM>
References: <[email protected]>
<ME0P300MB04455DD194CE6F5A8CAFAD85B6FA2@ME0P300MB0445.AUSP300.PROD.OUTLOOK.COM>
On Feb 18, 2025 at 16:17 +0800, Japin Li <[email protected]>, wrote:
> On Tue, 18 Feb 2025 at 15:40, "songjinzhou" <[email protected]> wrote:
> > Dear hackers:
> >
> > Two months ago, we enhanced the group by key value elimination function. This is a very useful function. When I was
> > learning, I found a regression test case that was not suitable, as follows:
> >
> > -- When there are multiple supporting unique indexes and the GROUP BY contains
> > -- columns to cover all of those, ensure we pick the index with the least
> > -- number of columns so that we can remove more columns from the GROUP BY.
> > explain (costs off) select a,b,c from t3 group by a,b,c;
> > QUERY PLAN
> > ----------------------
> > HashAggregate
> > Group Key: c
> > -> Seq Scan on t3
> > (3 rows)
> >
> > -- As above but try ordering the columns differently to ensure we get the
> > -- same result.
> > explain (costs off) select a,b,c from t3 group by c,a,b;
> > QUERY PLAN
> > ----------------------
> > HashAggregate
> > Group Key: c
> > -> Seq Scan on t3
> > (3 rows)
> >
> > Because the table structure of t3 is defined as follows(Its PK is deferrable):
> >
> > postgres=# \d+ t3
> > Table "pg_temp_1.t3"
> > Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
> > --------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
> > a | integer | | not null | | plain | | |
> > b | integer | | not null | | plain | | |
> > c | integer | | not null | | plain | | |
> > Indexes:
> > "t3_pkey" PRIMARY KEY, btree (a, b) DEFERRABLE
> > "t3_c_uidx" UNIQUE, btree (c)
> > Not-null constraints:
> > "t3_a_not_null" NOT NULL "a"
> > "t3_b_not_null" NOT NULL "b"
> > "t3_c_not_null" NOT NULL "c"
> > Access method: heap
> >
> > postgres=#
> >
> > I think this test case does not fully reflect the original meaning. So I made a small change to this and look forward to
> > your feedback. Thanks!
> >
Hi,
Good catch.
>
> Yeah, the primary key of t3 is deferred, so only the t3_c_uidx can be used.
> The test case is inconsistent with comments.
>
> It seems that the t2 does not have a unique index on z, do you forget to
> create it in the patch?
While there isn't an unique index, an unique constraint with a NOT NULL on that column should serve the same purpose.
+alter table t2 alter column z set not null, add constraint t2_z_uidx unique (z);
In patch v2, I made a slight adjustment: creating a unique index on t3 will handle multiple choice scenarios.
Thanks for the patch.
--
Zhang Mingli
HashData
Attachments:
[application/octet-stream] v2-0001-Modify-an-incorrect-regression-test-case-in-the-grou.patch (2.3K, ../c9b3ac67-bb2e-4dd8-ab63-8b343af5ec38@Spark/3-v2-0001-Modify-an-incorrect-regression-test-case-in-the-grou.patch)
download | inline diff:
From c92eba894be3f4d6b238917e6174373f1404a0fc Mon Sep 17 00:00:00 2001
From: songjinzhou <[email protected]>
Date: Tue, 18 Feb 2025 19:02:36 +0800
Subject: [PATCH] Modify an incorrect regression test case in the group by key
value elimination function
---
src/test/regress/expected/aggregates.out | 2 ++
src/test/regress/sql/aggregates.sql | 2 ++
2 files changed, 4 insertions(+)
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index f2fb66388c..ca95ba1343 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1472,6 +1472,7 @@ explain (costs off) select b,c from t3 group by b,c;
-- When there are multiple supporting unique indexes and the GROUP BY contains
-- columns to cover all of those, ensure we pick the index with the least
-- number of columns so that we can remove more columns from the GROUP BY.
+create unique index t3_b_c_uidx on t3(b, c);
explain (costs off) select a,b,c from t3 group by a,b,c;
QUERY PLAN
----------------------
@@ -1490,6 +1491,7 @@ explain (costs off) select a,b,c from t3 group by c,a,b;
-> Seq Scan on t3
(3 rows)
+drop index t3_b_c_uidx;
-- Ensure we don't use a partial index as proof of functional dependency
drop index t3_c_uidx;
create index t3_c_uidx on t3 (c) where c > 0;
diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index 77168bcc74..8acd64d34e 100644
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -520,12 +520,14 @@ explain (costs off) select b,c from t3 group by b,c;
-- When there are multiple supporting unique indexes and the GROUP BY contains
-- columns to cover all of those, ensure we pick the index with the least
-- number of columns so that we can remove more columns from the GROUP BY.
+create unique index t3_b_c_uidx on t3(b, c);
explain (costs off) select a,b,c from t3 group by a,b,c;
-- As above but try ordering the columns differently to ensure we get the
-- same result.
explain (costs off) select a,b,c from t3 group by c,a,b;
+drop index t3_b_c_uidx;
-- Ensure we don't use a partial index as proof of functional dependency
drop index t3_c_uidx;
create index t3_c_uidx on t3 (c) where c > 0;
--
2.34.1
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Modify an incorrect regression test case in the group by key value elimination function
In-Reply-To: <c9b3ac67-bb2e-4dd8-ab63-8b343af5ec38@Spark>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox