Re: [HACKERS] GSets: Getting collation related error when GSets has text column

2015-07-26 Thread Andres Freund
Hi,

On 2015-07-17 18:55:52 +0530, Jeevan Chalke wrote:
 Attached patch which attempts to fix this issue. However I am not sure
 whether it is correct. But it does not add any new issues as such.
 Added few test in the patch as well.

Pushed the fix. Thanks for the report and fix.

- Andres


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


[HACKERS] GSets: Getting collation related error when GSets has text column

2015-07-17 Thread Jeevan Chalke
Hi,

When we have text column in the GROUPING SETS (and with some specific
order of columns), we are getting error saying
could not determine which collation to use for string comparison

Here is the example:

postgres=# select sum(ten) from onek group by rollup(four::text), two
order by 1;
ERROR:  could not determine which collation to use for string comparison
HINT:  Use the COLLATE clause to set the collation explicitly.

However if we have sql like;
select sum(ten) from onek group by two, rollup(four::text)
order by 1;

It is not failing.

After spending enough time, If I understood the code correctly, we are
re-ordering the sort columns but we are not remapping the grpColIdx
correctly.

Attached patch which attempts to fix this issue. However I am not sure
whether it is correct. But it does not add any new issues as such.
Added few test in the patch as well.

Please have a look.

Thanks

-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index a6ce96e..96def6b 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -2402,13 +2402,8 @@ build_grouping_chain(PlannerInfo *root,
 	 * Prepare the grpColIdx for the real Agg node first, because we may need
 	 * it for sorting
 	 */
-	if (list_length(rollup_groupclauses)  1)
-	{
-		Assert(rollup_lists  llast(rollup_lists));
-
-		top_grpColIdx =
-			remap_groupColIdx(root, llast(rollup_groupclauses));
-	}
+	if (parse-groupingSets)
+		top_grpColIdx = remap_groupColIdx(root, llast(rollup_groupclauses));
 
 	/*
 	 * If we need a Sort operation on the input, generate that.
@@ -2429,11 +2424,14 @@ build_grouping_chain(PlannerInfo *root,
 	while (list_length(rollup_groupclauses)  1)
 	{
 		List	   *groupClause = linitial(rollup_groupclauses);
-		List	   *gsets = linitial(rollup_lists);
+		List	   *gsets;
 		AttrNumber *new_grpColIdx;
 		Plan	   *sort_plan;
 		Plan	   *agg_plan;
 
+		Assert(rollup_lists  linitial(rollup_lists));
+		gsets = linitial(rollup_lists);
+
 		Assert(groupClause);
 		Assert(gsets);
 
diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index 5c47717..4ff5963 100644
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -755,4 +755,27 @@ select array(select row(v.a,s1.*) from (select two,four, count(*) from onek grou
  {(2,0,0,250),(2,0,2,250),(2,0,,500),(2,1,1,250),(2,1,3,250),(2,1,,500),(2,,0,250),(2,,1,250),(2,,2,250),(2,,3,250),(2,,,1000)}
 (2 rows)
 
+-- Grouping on text columns
+select sum(ten) from onek group by two, rollup(four::text) order by 1;
+ sum  
+--
+ 1000
+ 1000
+ 1250
+ 1250
+ 2000
+ 2500
+(6 rows)
+
+select sum(ten) from onek group by rollup(four::text), two order by 1;
+ sum  
+--
+ 1000
+ 1000
+ 1250
+ 1250
+ 2000
+ 2500
+(6 rows)
+
 -- end
diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql
index e478d34..cc5b89a 100644
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -205,4 +205,8 @@ group by rollup(ten);
 select * from (values (1),(2)) v(a) left join lateral (select v.a, four, ten, count(*) from onek group by cube(four,ten)) s on true order by v.a,four,ten;
 select array(select row(v.a,s1.*) from (select two,four, count(*) from onek group by cube(two,four) order by two,four) s1) from (values (1),(2)) v(a);
 
+-- Grouping on text columns
+select sum(ten) from onek group by two, rollup(four::text) order by 1;
+select sum(ten) from onek group by rollup(four::text), two order by 1;
+
 -- end

-- 
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] GSets: Getting collation related error when GSets has text column

2015-07-17 Thread Andrew Gierth
 Jeevan == Jeevan Chalke jeevan.cha...@enterprisedb.com writes:

 Jeevan Hi,

 Jeevan When we have text column in the GROUPING SETS (and with some specific
 Jeevan order of columns), we are getting error saying
 Jeevan could not determine which collation to use for string comparison

Good catch.

 Jeevan After spending enough time, If I understood the code correctly,
 Jeevan we are re-ordering the sort columns but we are not remapping
 Jeevan the grpColIdx correctly.

Yup.

 Jeevan Attached patch which attempts to fix this issue. However I am
 Jeevan not sure whether it is correct. But it does not add any new
 Jeevan issues as such.  Added few test in the patch as well.

I wouldn't have bothered moving the Assert; it can be removed. Otherwise
it looks correct.

-- 
Andrew (irc:RhodiumToad)


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