You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by jh...@apache.org on 2017/10/02 21:00:07 UTC
[07/15] calcite git commit: [CALCITE-1980] RelBuilder.aggregate
should rename underlying fields if groupKey contains alias
[CALCITE-1980] RelBuilder.aggregate should rename underlying fields if groupKey contains alias
Test case by Pavel Gubin, in the following PR; did not use the rest of
the PR.
Close apache/calcite#535
Project: http://git-wip-us.apache.org/repos/asf/calcite/repo
Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/2773c484
Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/2773c484
Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/2773c484
Branch: refs/heads/master
Commit: 2773c4846a67360de8301680e375779ce3b1304b
Parents: 43fa8e9
Author: Julian Hyde <jh...@apache.org>
Authored: Wed Sep 13 12:00:35 2017 -0700
Committer: Julian Hyde <jh...@apache.org>
Committed: Mon Oct 2 11:13:42 2017 -0700
----------------------------------------------------------------------
.../org/apache/calcite/tools/RelBuilder.java | 92 ++++++++++++--------
.../org/apache/calcite/test/RelBuilderTest.java | 44 ++++++++++
2 files changed, 102 insertions(+), 34 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/calcite/blob/2773c484/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/calcite/tools/RelBuilder.java b/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
index fe822f0..0a726c7 100644
--- a/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
+++ b/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
@@ -1049,10 +1049,12 @@ public class RelBuilder {
/** Creates an {@link org.apache.calcite.rel.core.Aggregate} with a list of
* calls. */
public RelBuilder aggregate(GroupKey groupKey, Iterable<AggCall> aggCalls) {
- final List<RexNode> extraNodes = new ArrayList<>(fields());
+ final Registrar registrar = new Registrar();
+ registrar.extraNodes.addAll(fields());
+ registrar.names.addAll(peek().getRowType().getFieldNames());
final GroupKeyImpl groupKey_ = (GroupKeyImpl) groupKey;
final ImmutableBitSet groupSet =
- ImmutableBitSet.of(registerExpressions(extraNodes, groupKey_.nodes));
+ ImmutableBitSet.of(registrar.registerExpressions(groupKey_.nodes));
label:
if (Iterables.isEmpty(aggCalls) && !groupKey_.indicator) {
final RelMetadataQuery mq = peek().getCluster().getMetadataQuery();
@@ -1064,10 +1066,12 @@ public class RelBuilder {
break label;
}
}
- final Boolean unique = mq.areColumnsUnique(peek(), groupSet);
- if (unique != null && unique) {
- // Rel is already unique. Nothing to do.
- return this;
+ if (registrar.extraNodes.size() == fields().size()) {
+ final Boolean unique = mq.areColumnsUnique(peek(), groupSet);
+ if (unique != null && unique) {
+ // Rel is already unique. Nothing to do.
+ return this;
+ }
}
final Double maxRowCount = mq.getMaxRowCount(peek());
if (maxRowCount != null && maxRowCount <= 1D) {
@@ -1077,12 +1081,12 @@ public class RelBuilder {
}
final ImmutableList<ImmutableBitSet> groupSets;
if (groupKey_.nodeLists != null) {
- final int sizeBefore = extraNodes.size();
+ final int sizeBefore = registrar.extraNodes.size();
final SortedSet<ImmutableBitSet> groupSetSet =
new TreeSet<>(ImmutableBitSet.ORDERING);
for (ImmutableList<RexNode> nodeList : groupKey_.nodeLists) {
final ImmutableBitSet groupSet2 =
- ImmutableBitSet.of(registerExpressions(extraNodes, nodeList));
+ ImmutableBitSet.of(registrar.registerExpressions(nodeList));
if (!groupSet.contains(groupSet2)) {
throw new IllegalArgumentException("group set element " + nodeList
+ " must be a subset of group key");
@@ -1090,10 +1094,11 @@ public class RelBuilder {
groupSetSet.add(groupSet2);
}
groupSets = ImmutableList.copyOf(groupSetSet);
- if (extraNodes.size() > sizeBefore) {
+ if (registrar.extraNodes.size() > sizeBefore) {
throw new IllegalArgumentException(
"group sets contained expressions not in group key: "
- + extraNodes.subList(sizeBefore, extraNodes.size()));
+ + registrar.extraNodes.subList(sizeBefore,
+ registrar.extraNodes.size()));
}
} else {
groupSets = ImmutableList.of(groupSet);
@@ -1101,13 +1106,14 @@ public class RelBuilder {
for (AggCall aggCall : aggCalls) {
if (aggCall instanceof AggCallImpl) {
final AggCallImpl aggCall1 = (AggCallImpl) aggCall;
- registerExpressions(extraNodes, aggCall1.operands);
+ registrar.registerExpressions(aggCall1.operands);
if (aggCall1.filter != null) {
- registerExpression(extraNodes, aggCall1.filter);
+ registrar.registerExpression(aggCall1.filter);
}
}
}
- project(extraNodes);
+ project(registrar.extraNodes);
+ rename(registrar.names);
final Frame frame = stack.pop();
final RelNode r = frame.rel;
final List<AggregateCall> aggregateCalls = new ArrayList<>();
@@ -1115,9 +1121,10 @@ public class RelBuilder {
final AggregateCall aggregateCall;
if (aggCall instanceof AggCallImpl) {
final AggCallImpl aggCall1 = (AggCallImpl) aggCall;
- final List<Integer> args = registerExpressions(extraNodes, aggCall1.operands);
+ final List<Integer> args =
+ registrar.registerExpressions(aggCall1.operands);
final int filterArg = aggCall1.filter == null ? -1
- : registerExpression(extraNodes, aggCall1.filter);
+ : registrar.registerExpression(aggCall1.filter);
if (aggCall1.distinct && !aggCall1.aggFunction.isQuantifierAllowed()) {
throw new IllegalArgumentException("DISTINCT not allowed");
}
@@ -1147,7 +1154,7 @@ public class RelBuilder {
int i = 0;
// first, group fields
for (Integer groupField : groupSet.asList()) {
- RexNode node = extraNodes.get(groupField);
+ RexNode node = registrar.extraNodes.get(groupField);
final SqlKind kind = node.getKind();
switch (kind) {
case INPUT_REF:
@@ -1184,24 +1191,6 @@ public class RelBuilder {
return this;
}
- private static int registerExpression(List<RexNode> exprList, RexNode node) {
- int i = exprList.indexOf(node);
- if (i < 0) {
- i = exprList.size();
- exprList.add(node);
- }
- return i;
- }
-
- private static List<Integer> registerExpressions(List<RexNode> extraNodes,
- Iterable<? extends RexNode> nodes) {
- final List<Integer> builder = new ArrayList<>();
- for (RexNode node : nodes) {
- builder.add(registerExpression(extraNodes, node));
- }
- return builder;
- }
-
private RelBuilder setOp(boolean all, SqlKind kind, int n) {
List<RelNode> inputs = new LinkedList<>();
for (int i = 0; i < n; i++) {
@@ -1800,6 +1789,41 @@ public class RelBuilder {
}
}
+ /** Collects the extra expressions needed for {@link #aggregate}.
+ *
+ * <p>The extra expressions come from the group key and as arguments to
+ * aggregate calls, and later there will be a {@link #project} or a
+ * {@link #rename(List)} if necessary. */
+ private static class Registrar {
+ final List<RexNode> extraNodes = new ArrayList<>();
+ final List<String> names = new ArrayList<>();
+
+ int registerExpression(RexNode node) {
+ switch (node.getKind()) {
+ case AS:
+ final List<RexNode> operands = ((RexCall) node).operands;
+ int i = registerExpression(operands.get(0));
+ names.set(i, RexLiteral.stringValue(operands.get(1)));
+ return i;
+ }
+ int i = extraNodes.indexOf(node);
+ if (i < 0) {
+ i = extraNodes.size();
+ extraNodes.add(node);
+ names.add(null);
+ }
+ return i;
+ }
+
+ List<Integer> registerExpressions(Iterable<? extends RexNode> nodes) {
+ final List<Integer> builder = new ArrayList<>();
+ for (RexNode node : nodes) {
+ builder.add(registerExpression(node));
+ }
+ return builder;
+ }
+ }
+
/** Builder stack frame.
*
* <p>Describes a previously created relational expression and
http://git-wip-us.apache.org/repos/asf/calcite/blob/2773c484/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java b/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
index 8a337cc..0f66db3 100644
--- a/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
+++ b/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
@@ -702,6 +702,50 @@ public class RelBuilderTest {
assertThat(str(root), is(expected));
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1980">[CALCITE-1980]
+ * RelBuilder gives NPE if groupKey contains alias</a>.
+ *
+ * <p>Now, the alias does not cause a new expression to be added to the input,
+ * but causes the referenced fields to be renamed. */
+ @Test public void testAggregateProjectWithAliases() {
+ final RelBuilder builder = RelBuilder.create(config().build());
+ RelNode root =
+ builder.scan("EMP")
+ .project(builder.field("DEPTNO"))
+ .aggregate(
+ builder.groupKey(
+ builder.alias(builder.field("DEPTNO"), "departmentNo")))
+ .build();
+ final String expected = ""
+ + "LogicalAggregate(group=[{0}])\n"
+ + " LogicalProject(departmentNo=[$0])\n"
+ + " LogicalProject(DEPTNO=[$7])\n"
+ + " LogicalTableScan(table=[[scott, EMP]])\n";
+ assertThat(str(root), is(expected));
+ }
+
+ @Test public void testAggregateProjectWithExpression() {
+ final RelBuilder builder = RelBuilder.create(config().build());
+ RelNode root =
+ builder.scan("EMP")
+ .project(builder.field("DEPTNO"))
+ .aggregate(
+ builder.groupKey(
+ builder.alias(
+ builder.call(SqlStdOperatorTable.PLUS,
+ builder.field("DEPTNO"), builder.literal(3)),
+ "d3")))
+ .build();
+ final String expected = ""
+ + "LogicalAggregate(group=[{1}])\n"
+ + " LogicalProject(DEPTNO=[$0], d3=[$1])\n"
+ + " LogicalProject(DEPTNO=[$0], $f1=[+($0, 3)])\n"
+ + " LogicalProject(DEPTNO=[$7])\n"
+ + " LogicalTableScan(table=[[scott, EMP]])\n";
+ assertThat(str(root), is(expected));
+ }
+
@Test public void testAggregateGroupingKeyOutOfRangeFails() {
final RelBuilder builder = RelBuilder.create(config().build());
try {