[PATCH] Generate column names for subquery expressions
Hi list,
In current PostgreSQL versions, subquery expressions in the SELECT list
always generate columns with name "?column?"
postgres=# select (select 1 as foo);
?column?
--------
1
This patch improves on that:
select (SELECT 1 AS foo) => foo
select exists(SELECT 1) => exists
select array(SELECT 1) => array
The "array" one is now consistent with an array literal: select array[1];
Other subquery types (=ALL(), =ANY() and row comparison) don't change
because they act more like operators.
I guess it's fairly unlikely that users rely on column names being
"?column?", but it does change the name of some expressions, for example:
select (select 1 foo)::int;
select case when true then 1 else (select 1 as foo) end;
Previously these returned column names "int4" and "case", now they would
return "foo". Personally I prefer it this way, but if it is considered a
compatibility problem, lowering the strength of subquery names in
FigureColnameInternal would resort to the old behavior.
How this affects different queries can be seen from the regression diffs.
Does this sound like a good idea?
Should I submit this to the CommitFest?
Regards,
Marti Raudsepp
Attachments:
0001-Generate-column-names-for-subquery-expressions.patchtext/x-patch; charset=US-ASCII; name=0001-Generate-column-names-for-subquery-expressions.patchDownload
From c119ba8bf4d72a676aa1fc5a4d42c93f9902efaf Mon Sep 17 00:00:00 2001
From: Marti Raudsepp <marti@juffo.org>
Date: Wed, 31 Aug 2011 23:53:04 +0300
Subject: [PATCH] Generate column names for subquery expressions
(SELECT 1 AS foo) => foo
exists(SELECT 1) => exists
array(SELECT 1) => array
---
src/backend/parser/parse_target.c | 29 +++++++++++++++++++++++++++++
src/test/regress/expected/aggregates.out | 6 +++---
src/test/regress/expected/subselect.out | 12 ++++++------
src/test/regress/expected/with.out | 4 ++--
4 files changed, 40 insertions(+), 11 deletions(-)
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 9d4e580..378d8ec 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -1585,6 +1585,35 @@ FigureColnameInternal(Node *node, char **name)
return FigureColnameInternal(ind->arg, name);
}
break;
+ case T_SubLink:
+ switch (((SubLink *) node)->subLinkType)
+ {
+ case EXISTS_SUBLINK:
+ *name = "exists";
+ return 2;
+
+ case ARRAY_SUBLINK:
+ *name = "array";
+ return 2;
+
+ case EXPR_SUBLINK:
+ /* Get column name from the subquery's target list */
+ {
+ SubLink *sublink = (SubLink *) node;
+ Query *query = (Query *) sublink->subselect;
+ /* EXPR_SUBLINK always has a single target */
+ TargetEntry *te = (TargetEntry *) linitial(query->targetList);
+
+ /* Subqueries have already been transformed */
+ if(te->resname)
+ {
+ *name = te->resname;
+ return 2;
+ }
+ }
+ break;
+ }
+ break;
case T_FuncCall:
*name = strVal(llast(((FuncCall *) node)->funcname));
return 2;
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 4861006..69926f7 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -300,9 +300,9 @@ LINE 4: where sum(distinct a.four + b.four) = b.four)...
select
(select max((select i.unique2 from tenk1 i where i.unique1 = o.unique1)))
from tenk1 o;
- ?column?
-----------
- 9999
+ max
+------
+ 9999
(1 row)
--
diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
index e638f0a..4ea8211 100644
--- a/src/test/regress/expected/subselect.out
+++ b/src/test/regress/expected/subselect.out
@@ -490,20 +490,20 @@ select view_a from view_a;
(1 row)
select (select view_a) from view_a;
- ?column?
-----------
+ view_a
+--------
(42)
(1 row)
select (select (select view_a)) from view_a;
- ?column?
-----------
+ view_a
+--------
(42)
(1 row)
select (select (a.*)::text) from view_a a;
- ?column?
-----------
+ a
+------
(42)
(1 row)
diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out
index a1b0899..c4b0456 100644
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -1065,7 +1065,7 @@ with cte(foo) as ( select 42 ) select * from ((select foo from cte)) q;
select ( with cte(foo) as ( values(f1) )
select (select foo from cte) )
from int4_tbl;
- ?column?
+ foo
-------------
0
123456
@@ -1077,7 +1077,7 @@ from int4_tbl;
select ( with cte(foo) as ( values(f1) )
values((select foo from cte)) )
from int4_tbl;
- ?column?
+ column1
-------------
0
123456
--
1.7.6.1
Marti Raudsepp <marti@juffo.org> writes:
In current PostgreSQL versions, subquery expressions in the SELECT list
always generate columns with name "?column?"
...
This patch improves on that:
select (SELECT 1 AS foo) => foo
select exists(SELECT 1) => exists
select array(SELECT 1) => array
Does this sound like a good idea?
Seems like a lot of room for bikeshedding here, but we could certainly
consider doing something.
Should I submit this to the CommitFest?
Please.
regards, tom lane
Here's version 2 of the patch, mainly to silence compiler warnings
about missing "case" statements from a switch over SubLinkType enum.
Also expanded commit message a little and changed whitespace to be
more consistent with nearby code.
Regards,
Marti
Attachments:
0001-Generate-column-names-for-subquery-expressions-inste.patchtext/x-patch; charset=US-ASCII; name=0001-Generate-column-names-for-subquery-expressions-inste.patchDownload
From 9f987efacfe2cd2af82722775127605e690df976 Mon Sep 17 00:00:00 2001
From: Marti Raudsepp <marti@juffo.org>
Date: Tue, 6 Sep 2011 21:17:54 +0300
Subject: [PATCH] Generate column names for subquery expressions instead of
"?column?"
Previously these select expressions didn't have column names:
select (SELECT colname FROM tab) => colname
select (SELECT 1 AS foo) => foo
select exists(SELECT 1) => exists
select array(SELECT 1) => array
This change makes the subquery column name take precedence over "weak"
column names. Previously these two would return "int4" and "case", now
they return "foo":
select (select 1 foo)::int
select case when true then 1 else (select 1 as foo) end
---
src/backend/parser/parse_target.c | 33 ++++++++++++++++++++++++++++++
src/test/regress/expected/aggregates.out | 6 ++--
src/test/regress/expected/subselect.out | 12 +++++-----
src/test/regress/expected/with.out | 4 +-
4 files changed, 44 insertions(+), 11 deletions(-)
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 9d4e580..685233c 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -1585,6 +1585,39 @@ FigureColnameInternal(Node *node, char **name)
return FigureColnameInternal(ind->arg, name);
}
break;
+ case T_SubLink:
+ switch (((SubLink *) node)->subLinkType)
+ {
+ case EXISTS_SUBLINK:
+ *name = "exists";
+ return 2;
+ case ARRAY_SUBLINK:
+ *name = "array";
+ return 2;
+ case EXPR_SUBLINK:
+ /* Get column name from the subquery's target list */
+ {
+ SubLink *sublink = (SubLink *) node;
+ Query *query = (Query *) sublink->subselect;
+ /* EXPR_SUBLINK always has a single target */
+ TargetEntry *te = (TargetEntry *) linitial(query->targetList);
+
+ /* Subqueries have already been transformed */
+ if(te->resname)
+ {
+ *name = te->resname;
+ return 2;
+ }
+ }
+ break;
+ /* As with other operator-like nodes, these don't really have names */
+ case ALL_SUBLINK:
+ case ANY_SUBLINK:
+ case ROWCOMPARE_SUBLINK:
+ case CTE_SUBLINK:
+ break;
+ }
+ break;
case T_FuncCall:
*name = strVal(llast(((FuncCall *) node)->funcname));
return 2;
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 4861006..69926f7 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -300,9 +300,9 @@ LINE 4: where sum(distinct a.four + b.four) = b.four)...
select
(select max((select i.unique2 from tenk1 i where i.unique1 = o.unique1)))
from tenk1 o;
- ?column?
-----------
- 9999
+ max
+------
+ 9999
(1 row)
--
diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
index e638f0a..4ea8211 100644
--- a/src/test/regress/expected/subselect.out
+++ b/src/test/regress/expected/subselect.out
@@ -490,20 +490,20 @@ select view_a from view_a;
(1 row)
select (select view_a) from view_a;
- ?column?
-----------
+ view_a
+--------
(42)
(1 row)
select (select (select view_a)) from view_a;
- ?column?
-----------
+ view_a
+--------
(42)
(1 row)
select (select (a.*)::text) from view_a a;
- ?column?
-----------
+ a
+------
(42)
(1 row)
diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out
index a1b0899..c4b0456 100644
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -1065,7 +1065,7 @@ with cte(foo) as ( select 42 ) select * from ((select foo from cte)) q;
select ( with cte(foo) as ( values(f1) )
select (select foo from cte) )
from int4_tbl;
- ?column?
+ foo
-------------
0
123456
@@ -1077,7 +1077,7 @@ from int4_tbl;
select ( with cte(foo) as ( values(f1) )
values((select foo from cte)) )
from int4_tbl;
- ?column?
+ column1
-------------
0
123456
--
1.7.6.1
This is a review for the patch `Generate column names for
subquery expressions'
(https://commitfest.postgresql.org/action/patch_view?id=632)
Summary
====================
Patch format is in context diff format.
This patch applies cleanly on HEAD and make check suceeded.
It seems have no problem to apply.
Documents is needed to modify.
Purpose and function of this patch
====================
This patch intends to name a part of the columns in the outmost
SELECT caluse currently left unnamed - seen as `?column?' - and
fix `unnatural' naming - seen as `int4', `case'.
This patch figures column name after T_SubLink parse nodes
corresponding to EXISTS, ARRAY, and subquery in addition to
currently processed parse node types.
It seems reasonable that (ALL|ANY|ROWCOMPARE|CTE)_SUBLINK is left
unnnamed.
Patch application, regression test
====================
The patch applies cleanly onto HEAD. make check yiels no error.
This patch adds no additional test case and it seems ok.
The coding style in this patch seems according to the convention.
Behavior changes
====================
The behavior of column naming changes as following.
STATEMENT AFTER BEFORE
-----------------------------------------------------
select (select 1 as foo) foo ?column?
select (exists (select 1)) exists ?column?
select (array (select 1 as x)) array ?column?
select (select 1 as aaa)::int aaa int4
select case when true then 1 else (select 1 as foo) end;
foo case
Aboves are same as described. But the following expression
returns somewhat confising outcome.
select case when true then (select 2 as bar) else (select 1 as foo) end;
foo
-----
2
(1 row)
But this patch is not to blame for the behavior. The following is
seen for unpatched pg.
# create table foo (a int, b int, c int);
# insert into foo values (1, 100, -100), (0, 10, -10), (-1, 25, -25);
# select case when a < 0 then b else c end from foo;
c
------
(snipped)
# select case a when -1 then c when 1 then a else b end from foo;
b
-----
(snipped)
Nevertheless this behavior seems a bit unnatural, it is not the
issue for this patch.
Performance
====================
This patch adds no extra load such as loops, recursive calls or
deep calls. Added code runs for exists(), array() and subquery
appear in the column list of select clause. So I think this patch
can put only negligible impact on performance.
Gleaning
====================
This patch assumes node(subLinkType==EXPR_SUBLINK)->subselect is
not null, and it seems that gram.y(c) says the assumption is
correct.
I think this patch needs no documentation, but it is needed to
edit the changed behaviors quoted in document. Maybe only one
change as far as I have seen.
http://www.postgresql.org/docs/9.0/static/sql-expressions.html
4.2.11
..
SELECT ARRAY(SELECT oid FROM pg_proc WHERE proname LIKE 'bytea%');
?column?
-------------------------------------------------------------
{2011,1954,1948,1952,1951,1244,1950,2005,1949,1953,2006,31}
Sincerely,
--
Kyotaro Horiguchi
On Wed, Sep 14, 2011 at 05:26, Kyotaro HORIGUCHI
<horiguchi.kyotaro@oss.ntt.co.jp> wrote:
This is a review for the patch `Generate column names for
subquery expressions'
(https://commitfest.postgresql.org/action/patch_view?id=632)
Thanks for the review. :)
PS: When you send a review, you should add the author's email to the
"To:" line to make sure they see it. I noticed your email only today
because it was in a new thread and not addressed to me directly.
I think this patch needs no documentation, but it is needed to
edit the changed behaviors quoted in document. Maybe only one
change as far as I have seen.http://www.postgresql.org/docs/9.0/static/sql-expressions.html
SELECT ARRAY(SELECT oid FROM pg_proc WHERE proname LIKE 'bytea%');
?column?
-------------------------------------------------------------
{2011,1954,1948,1952,1951,1244,1950,2005,1949,1953,2006,31}
Good catch, a new patch is attached. Apparently the results of this
query have also changed in recent versions, but I didn't touch that.
Regards,
Marti
Attachments:
Generate-column-names-for-subquery-expressions-v3.patchtext/x-patch; charset=US-ASCII; name=Generate-column-names-for-subquery-expressions-v3.patchDownload
From 0defe411697f84d26e6f23970d90c70cd057c298 Mon Sep 17 00:00:00 2001
From: Marti Raudsepp <marti@juffo.org>
Date: Tue, 6 Sep 2011 21:17:54 +0300
Subject: [PATCH] Generate column names for subquery expressions instead of
"?column?"
Previously these select expressions didn't have column names:
select (SELECT colname FROM tab) => colname
select (SELECT 1 AS foo) => foo
select exists(SELECT 1) => exists
select array(SELECT 1) => array
This change makes the subquery column name take precedence over "weak"
column names. Previously these two would return "int4" and "case", now
they return "foo":
select (select 1 foo)::int
select case when true then 1 else (select 1 as foo) end
---
doc/src/sgml/syntax.sgml | 2 +-
src/backend/parser/parse_target.c | 33 ++++++++++++++++++++++++++++++
src/test/regress/expected/aggregates.out | 6 ++--
src/test/regress/expected/subselect.out | 12 +++++-----
src/test/regress/expected/with.out | 4 +-
5 files changed, 45 insertions(+), 12 deletions(-)
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 9119b60..ab81f78 100644
--- a/doc/src/sgml/syntax.sgml
+++ b/doc/src/sgml/syntax.sgml
@@ -2109,7 +2109,7 @@ SELECT ARRAY[]::integer[];
bracketed) subquery. For example:
<programlisting>
SELECT ARRAY(SELECT oid FROM pg_proc WHERE proname LIKE 'bytea%');
- ?column?
+ array
-------------------------------------------------------------
{2011,1954,1948,1952,1951,1244,1950,2005,1949,1953,2006,31}
(1 row)
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 9d4e580..685233c 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -1585,6 +1585,39 @@ FigureColnameInternal(Node *node, char **name)
return FigureColnameInternal(ind->arg, name);
}
break;
+ case T_SubLink:
+ switch (((SubLink *) node)->subLinkType)
+ {
+ case EXISTS_SUBLINK:
+ *name = "exists";
+ return 2;
+ case ARRAY_SUBLINK:
+ *name = "array";
+ return 2;
+ case EXPR_SUBLINK:
+ /* Get column name from the subquery's target list */
+ {
+ SubLink *sublink = (SubLink *) node;
+ Query *query = (Query *) sublink->subselect;
+ /* EXPR_SUBLINK always has a single target */
+ TargetEntry *te = (TargetEntry *) linitial(query->targetList);
+
+ /* Subqueries have already been transformed */
+ if(te->resname)
+ {
+ *name = te->resname;
+ return 2;
+ }
+ }
+ break;
+ /* As with other operator-like nodes, these don't really have names */
+ case ALL_SUBLINK:
+ case ANY_SUBLINK:
+ case ROWCOMPARE_SUBLINK:
+ case CTE_SUBLINK:
+ break;
+ }
+ break;
case T_FuncCall:
*name = strVal(llast(((FuncCall *) node)->funcname));
return 2;
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 4861006..69926f7 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -300,9 +300,9 @@ LINE 4: where sum(distinct a.four + b.four) = b.four)...
select
(select max((select i.unique2 from tenk1 i where i.unique1 = o.unique1)))
from tenk1 o;
- ?column?
-----------
- 9999
+ max
+------
+ 9999
(1 row)
--
diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
index e638f0a..4ea8211 100644
--- a/src/test/regress/expected/subselect.out
+++ b/src/test/regress/expected/subselect.out
@@ -490,20 +490,20 @@ select view_a from view_a;
(1 row)
select (select view_a) from view_a;
- ?column?
-----------
+ view_a
+--------
(42)
(1 row)
select (select (select view_a)) from view_a;
- ?column?
-----------
+ view_a
+--------
(42)
(1 row)
select (select (a.*)::text) from view_a a;
- ?column?
-----------
+ a
+------
(42)
(1 row)
diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out
index a1b0899..c4b0456 100644
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -1065,7 +1065,7 @@ with cte(foo) as ( select 42 ) select * from ((select foo from cte)) q;
select ( with cte(foo) as ( values(f1) )
select (select foo from cte) )
from int4_tbl;
- ?column?
+ foo
-------------
0
123456
@@ -1077,7 +1077,7 @@ from int4_tbl;
select ( with cte(foo) as ( values(f1) )
values((select foo from cte)) )
from int4_tbl;
- ?column?
+ column1
-------------
0
123456
--
1.7.6.1
Hi,
PS: When you send a review, you should add the author's email to the
"To:" line to make sure they see it. I noticed your email only today
because it was in a new thread and not addressed to me directly.
Thanks for the advise. I will do so after this.
Good catch, a new patch is attached. Apparently the results of this
query have also changed in recent versions, but I didn't touch that.
I've comfirmed that is fixed.
I'll send this comitter review.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Marti Raudsepp <marti@juffo.org> writes:
On Wed, Sep 14, 2011 at 05:26, Kyotaro HORIGUCHI
<horiguchi.kyotaro@oss.ntt.co.jp> wrote:This is a review for the patch `Generate column names for
subquery expressions'
(https://commitfest.postgresql.org/action/patch_view?id=632)
Thanks for the review. :)
Applied with minor adjustments.
regards, tom lane