FYI. — Jesús
From: Julian Hyde <[email protected]<mailto:[email protected]>> Date: Tuesday, June 16, 2015 at 9:24 PM To: Jesus Camachorodriguez <[email protected]<mailto:[email protected]>> Subject: Re: incubator-calcite git commit: [CALCITE-753] Aggregate operators may derive row types with duplicate column names Close apache/incubator-calcite#91 The HOWTO contains process information for committers. Search for “for Calcite committers” in http://calcite.incubator.apache.org/docs/howto.html. Modify site/_docs/howto.md and submit a pull request. If really want to preview the site with your edits, see http://calcite.incubator.apache.org/docs/howto.html#publish-the-web-site but it takes about 1/2 hour to install jekyll etc. so it’s not necessary. Github’s https://github.com/apache/incubator-calcite/blob/master/site/_docs/howto.md will give you a reasonable idea whether your markup is valid. Julian PS I think this thread would be useful to other people. If you have no objections, please forward to the dev list. On Jun 16, 2015, at 1:14 PM, Jesus Camachorodriguez <[email protected]<mailto:[email protected]>> wrote: Yes :) I found some useful information that helped me here: https://cwiki.apache.org/confluence/display/REEF/Committer+Guide Maybe I could create a similar page in Calcite website that will help new committers know the process? Which would be the right place for this? Thanks, Jesús From: Julian Hyde <[email protected]<mailto:[email protected]>> Date: Tuesday, June 16, 2015 at 8:54 PM To: Jesus Camachorodriguez <[email protected]<mailto:[email protected]>> Subject: Re: incubator-calcite git commit: [CALCITE-753] Aggregate operators may derive row types with duplicate column names Close apache/incubator-calcite#91 You remembered the “Close apache/…” line. That’s the hard part. (And if you get it wrong Apache won’t accept ‘git push -f’ on master branch.) On Jun 16, 2015, at 11:40 AM, Jesus Camachorodriguez <[email protected]<mailto:[email protected]>> wrote: Thanks Julian! I'm sorry, I was so excited (and careful about not messing up the commit), and I completely forgot about all the rest :) I'll take care of it right now. Thanks for pointing out, Jesús From: Julian Hyde <[email protected]<mailto:[email protected]>> Date: Tuesday, June 16, 2015 at 7:40 PM To: Jesus Camachorodriguez <[email protected]<mailto:[email protected]>> Subject: Fwd: incubator-calcite git commit: [CALCITE-753] Aggregate operators may derive row types with duplicate column names Close apache/incubator-calcite#91 Thanks for checking in this fix. You are now a committer in deed as well as in name! When I receive the commit message from apache (you’ll need to be on the commits list) I go and mark the JIRA case fixed, and set the fix version to the next release (1.4.0-incubating), and add a message “Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/ccdda9b2”<http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/ccdda9b2%E2%80%9D>. Begin forwarded message: From: [email protected]<mailto:[email protected]> Subject: incubator-calcite git commit: [CALCITE-753] Aggregate operators may derive row types with duplicate column names Close apache/incubator-calcite#91 Date: June 16, 2015 at 9:39:33 AM PDT To: [email protected]<mailto:[email protected]> Reply-To: [email protected]<mailto:[email protected]> Repository: incubator-calcite Updated Branches: refs/heads/master f3cae1306 -> ccdda9b20 [CALCITE-753] Aggregate operators may derive row types with duplicate column names Close apache/incubator-calcite#91 Project: http://git-wip-us.apache.org/repos/asf/incubator-calcite/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/ccdda9b2 Tree: http://git-wip-us.apache.org/repos/asf/incubator-calcite/tree/ccdda9b2 Diff: http://git-wip-us.apache.org/repos/asf/incubator-calcite/diff/ccdda9b2 Branch: refs/heads/master Commit: ccdda9b203a9468e439eb2dbffc0359658fb38c3 Parents: f3cae13 Author: Jesus Camacho Rodriguez <[email protected]<mailto:[email protected]>> Authored: Tue Jun 16 17:38:55 2015 +0100 Committer: Jesus Camacho Rodriguez <[email protected]<mailto:[email protected]>> Committed: Tue Jun 16 17:38:55 2015 +0100 ---------------------------------------------------------------------- .../org/apache/calcite/rel/core/Aggregate.java | 40 +++++++++++++++++--- .../calcite/test/SqlToRelConverterTest.java | 8 ++++ .../calcite/test/SqlToRelConverterTest.xml | 13 +++++++ 3 files changed, 56 insertions(+), 5 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-calcite/blob/ccdda9b2/core/src/main/java/org/apache/calcite/rel/core/Aggregate.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/rel/core/Aggregate.java b/core/src/main/java/org/apache/calcite/rel/core/Aggregate.java index b95e44c..3250ae1 100644 --- a/core/src/main/java/org/apache/calcite/rel/core/Aggregate.java +++ b/core/src/main/java/org/apache/calcite/rel/core/Aggregate.java @@ -46,9 +46,11 @@ import org.apache.calcite.util.Util; import com.google.common.base.Preconditions; import com.google.common.base.Predicate; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Sets; import com.google.common.math.IntMath; import java.util.List; +import java.util.Set; /** * Relational operator that eliminates @@ -322,29 +324,57 @@ public abstract class Aggregate extends SingleRel { assert groupList.size() == groupSet.cardinality(); final RelDataTypeFactory.FieldInfoBuilder builder = typeFactory.builder(); final List<RelDataTypeField> fieldList = inputRowType.getFieldList(); + final Set<String> containedNames = Sets.newHashSet(); for (int groupKey : groupList) { - builder.add(fieldList.get(groupKey)); + final RelDataTypeField field = fieldList.get(groupKey); + containedNames.add(field.getName()); + builder.add(field); } if (indicator) { for (int groupKey : groupList) { final RelDataType booleanType = typeFactory.createTypeWithNullability( typeFactory.createSqlType(SqlTypeName.BOOLEAN), false); - builder.add("i$" + fieldList.get(groupKey).getName(), booleanType); + final String base = "i$" + fieldList.get(groupKey).getName(); + String name = base; + int i = 0; + while (containedNames.contains(name)) { + name = base + "_" + i++; + } + containedNames.add(name); + builder.add(name, booleanType); } } for (Ord<AggregateCall> aggCall : Ord.zip(aggCalls)) { - String name; + final String base; if (aggCall.e.name != null) { - name = aggCall.e.name; + base = aggCall.e.name; } else { - name = "$f" + (groupList.size() + aggCall.i); + base = "$f" + (groupList.size() + aggCall.i); + } + String name = base; + int i = 0; + while (containedNames.contains(name)) { + name = base + "_" + i++; } + containedNames.add(name); builder.add(name, aggCall.e.type); } return builder.build(); } + public boolean isValid(boolean fail) { + if (!super.isValid(fail)) { + assert !fail; + return false; + } + if (!Util.isDistinct(getRowType().getFieldNames())) { + assert !fail : getRowType(); + return false; + } + return true; + } + /** * Returns whether the inferred type of an {@link AggregateCall} matches the * type it was given when it was created. http://git-wip-us.apache.org/repos/asf/incubator-calcite/blob/ccdda9b2/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java b/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java index 08a01a9..3d2b48f 100644 --- a/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java +++ b/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java @@ -1115,6 +1115,14 @@ public class SqlToRelConverterTest extends SqlToRelTestBase { .convertsTo("${plan}"); } + /** + * [CALCITE-753] Test aggregate operators do not derive row types with duplicate column names + */ + @Test public void testAggNoDuplicateColumnNames() { + sql("SELECT empno, EXPR$2, COUNT(empno) FROM (SELECT empno, deptno AS EXPR$2 " + + "FROM emp) GROUP BY empno, EXPR$2").convertsTo("${plan}"); + } + @Test public void testAggScalarSubquery() { sql("SELECT SUM(SELECT min(deptno) FROM dept) FROM emp") .convertsTo("${plan}"); http://git-wip-us.apache.org/repos/asf/incubator-calcite/blob/ccdda9b2/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml ---------------------------------------------------------------------- diff --git a/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml b/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml index b3be8ce..58777ab 100644 --- a/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml +++ b/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml @@ -2316,6 +2316,19 @@ LogicalAggregate(group=[{0}]) ]]> </Resource> </TestCase> + <TestCase name="testAggNoDuplicateColumnNames"> + <Resource name="sql"> + <![CDATA[SELECT empno, EXPR$2, COUNT(empno) FROM (SELECT empno, deptno AS EXPR$2 +FROM emp) GROUP BY empno, EXPR$2]]> + </Resource> + <Resource name="plan"> + <![CDATA[ +LogicalAggregate(group=[{0, 1}], EXPR$2=[COUNT()]) + LogicalProject(EMPNO=[$0], EXPR$2=[$7]) + LogicalTableScan(table=[[CATALOG, SALES, EMP]]) +]]> + </Resource> + </TestCase> <TestCase name="testAggCaseSubquery"> <Resource name="sql"> <![CDATA[SELECT SUM(CASE WHEN empno IN (3) THEN 0 ELSE 1 END) FROM emp]]>
