Re: [HACKERS] [PATCH] Generate column names for subquery expressions

2011-09-06 Thread Marti Raudsepp 
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
From 9f987efacfe2cd2af82722775127605e690df976 Mon Sep 17 00:00:00 2001
From: Marti Raudsepp 
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? 
---
- 
+ max  
+--
+ 
 (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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generate column names for subquery expressions

2011-08-31 Thread Tom Lane 
Marti Raudsepp  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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] [PATCH] Generate column names for subquery expressions

2011-08-31 Thread Marti Raudsepp 
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
From c119ba8bf4d72a676aa1fc5a4d42c93f9902efaf Mon Sep 17 00:00:00 2001
From: Marti Raudsepp 
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? 
---
- 
+ max  
+--
+ 
 (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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers