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/01/06 20:52:49 UTC
[5/5] calcite git commit: [CALCITE-1558]
AggregateExpandDistinctAggregatesRule gets field mapping wrong if groupKey is
used in aggregate function (Zhenghua Gao)
[CALCITE-1558] AggregateExpandDistinctAggregatesRule gets field mapping wrong if groupKey is used in aggregate function (Zhenghua Gao)
Add Util.intersects and ImmutableBitSet.asSet. (Julian Hyde)
Close apache/calcite#347
Project: http://git-wip-us.apache.org/repos/asf/calcite/repo
Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/2b966375
Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/2b966375
Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/2b966375
Branch: refs/heads/master
Commit: 2b9663752a0bfffce4641450b01365cc4f2124e1
Parents: 34df5a5
Author: Zhenghua Gao <do...@gmail.com>
Authored: Thu Jan 5 14:42:48 2017 +0800
Committer: Julian Hyde <jh...@apache.org>
Committed: Fri Jan 6 10:52:10 2017 -0800
----------------------------------------------------------------------
.../AggregateExpandDistinctAggregatesRule.java | 33 ++++++++++----
.../apache/calcite/util/ImmutableBitSet.java | 22 ++++++++++
.../main/java/org/apache/calcite/util/Util.java | 10 +++++
.../apache/calcite/test/RelOptRulesTest.java | 46 +++++++++++++++-----
.../calcite/util/ImmutableBitSetTest.java | 32 +++++++++++++-
.../java/org/apache/calcite/util/UtilTest.java | 16 +++++++
.../org/apache/calcite/test/RelOptRulesTest.xml | 45 +++++++++++++++++++
7 files changed, 183 insertions(+), 21 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/calcite/blob/2b966375/core/src/main/java/org/apache/calcite/rel/rules/AggregateExpandDistinctAggregatesRule.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/calcite/rel/rules/AggregateExpandDistinctAggregatesRule.java b/core/src/main/java/org/apache/calcite/rel/rules/AggregateExpandDistinctAggregatesRule.java
index de12e42..553285e 100644
--- a/core/src/main/java/org/apache/calcite/rel/rules/AggregateExpandDistinctAggregatesRule.java
+++ b/core/src/main/java/org/apache/calcite/rel/rules/AggregateExpandDistinctAggregatesRule.java
@@ -266,6 +266,8 @@ public final class AggregateExpandDistinctAggregatesRule extends RelOptRule {
relBuilder.peek().getRowType().getFieldList();
final boolean hasGroupBy = aggregate.getGroupSet().size() > 0;
+ final Set<Integer> groupSet = aggregate.getGroupSet().asSet();
+
// Add the distinct aggregate column(s) to the group-by columns,
// if not already a part of the group-by
newGroupSet.addAll(aggregate.getGroupSet().asList());
@@ -290,26 +292,31 @@ public final class AggregateExpandDistinctAggregatesRule extends RelOptRule {
for (final AggregateCall aggCall : aggCalls) {
if (!aggCall.isDistinct()) {
for (int arg : aggCall.getArgList()) {
- sourceOf.put(arg, projects.size());
+ if (!groupSet.contains(arg)) {
+ sourceOf.put(arg, projects.size());
+ }
}
}
}
int fakeArg0 = 0;
for (final AggregateCall aggCall : aggCalls) {
// We will deal with non-distinct aggregates below
- if (!aggCall.isDistinct()) {
- if (aggCall.getArgList().size() == 0) {
- while (sourceOf.get(fakeArg0) != null) {
- ++fakeArg0;
- }
- fakeArgs.add(fakeArg0);
+ if (!aggCall.isDistinct()
+ && (aggCall.getArgList().isEmpty()
+ || Util.intersects(groupSet, aggCall.getArgList()))) {
+ while (sourceOf.get(fakeArg0) != null) {
+ ++fakeArg0;
}
+ fakeArgs.add(fakeArg0);
+ ++fakeArg0;
}
}
for (final AggregateCall aggCall : aggCalls) {
if (!aggCall.isDistinct()) {
for (int arg : aggCall.getArgList()) {
- sourceOf.remove(arg);
+ if (!groupSet.contains(arg)) {
+ sourceOf.remove(arg);
+ }
}
}
}
@@ -336,6 +343,10 @@ public final class AggregateExpandDistinctAggregatesRule extends RelOptRule {
++fakeArgIdx;
} else {
for (int arg : newCall.getArgList()) {
+ if (groupSet.contains(arg)) {
+ arg = fakeArgs.get(fakeArgIdx++);
+ callArgMap.put(newCall, arg);
+ }
sourceOf.put(arg, projects.size());
projects.add(
Pair.of((RexNode) new RexInputRef(arg, newCall.getType()),
@@ -361,7 +372,11 @@ public final class AggregateExpandDistinctAggregatesRule extends RelOptRule {
final AggregateCall newCall;
for (int j = 0; j < argCount; j++) {
final Integer arg = aggCall.getArgList().get(j);
- newArgs.add(sourceOf.get(arg));
+ if (callArgMap.containsKey(aggCall)) {
+ newArgs.add(sourceOf.get(callArgMap.get(aggCall)));
+ } else {
+ newArgs.add(sourceOf.get(arg));
+ }
}
if (aggCall.isDistinct()) {
newCall =
http://git-wip-us.apache.org/repos/asf/calcite/blob/2b966375/core/src/main/java/org/apache/calcite/util/ImmutableBitSet.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/calcite/util/ImmutableBitSet.java b/core/src/main/java/org/apache/calcite/util/ImmutableBitSet.java
index ff1774c..61b3321 100644
--- a/core/src/main/java/org/apache/calcite/util/ImmutableBitSet.java
+++ b/core/src/main/java/org/apache/calcite/util/ImmutableBitSet.java
@@ -30,6 +30,7 @@ import com.google.common.collect.Ordering;
import java.io.Serializable;
import java.nio.LongBuffer;
import java.util.AbstractList;
+import java.util.AbstractSet;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.BitSet;
@@ -37,6 +38,7 @@ import java.util.Comparator;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
+import java.util.Set;
import java.util.SortedMap;
import java.util.TreeMap;
import javax.annotation.Nonnull;
@@ -625,6 +627,26 @@ public class ImmutableBitSet
};
}
+ /** Creates a view onto this bit set as a set of integers.
+ *
+ * <p>The {@code size} and {@code contains} methods are both O(n), but the
+ * iterator is efficient. */
+ public Set<Integer> asSet() {
+ return new AbstractSet<Integer>() {
+ @Nonnull public Iterator<Integer> iterator() {
+ return ImmutableBitSet.this.iterator();
+ }
+
+ public int size() {
+ return cardinality();
+ }
+
+ @Override public boolean contains(Object o) {
+ return ImmutableBitSet.this.get((Integer) o);
+ }
+ };
+ }
+
/**
* Converts this bit set to an array.
*
http://git-wip-us.apache.org/repos/asf/calcite/blob/2b966375/core/src/main/java/org/apache/calcite/util/Util.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/calcite/util/Util.java b/core/src/main/java/org/apache/calcite/util/Util.java
index 3ab5d4b..4117a8d 100644
--- a/core/src/main/java/org/apache/calcite/util/Util.java
+++ b/core/src/main/java/org/apache/calcite/util/Util.java
@@ -2121,6 +2121,16 @@ public class Util {
return -1;
}
+ /** Returns whether two collections have any elements in common. */
+ public static <E> boolean intersects(Collection<E> c0, Collection<E> c1) {
+ for (E e : c1) {
+ if (c0.contains(e)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
/** Looks for a string within a list of strings, using a given
* case-sensitivity policy, and returns the position at which the first match
* is found, or -1 if there are no matches. */
http://git-wip-us.apache.org/repos/asf/calcite/blob/2b966375/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java b/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
index 1e36724..ac55f8a 100644
--- a/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
+++ b/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
@@ -826,6 +826,42 @@ public class RelOptRulesTest extends RelOptTestBase {
+ " from sales.emp group by rollup(deptno,job)");
}
+ @Test public void testDistinctNonDistinctAggregates() {
+ final String sql = "select emp.empno, count(*), avg(distinct dept.deptno)\n"
+ + "from sales.emp emp inner join sales.dept dept\n"
+ + "on emp.deptno = dept.deptno\n"
+ + "group by emp.empno";
+ final HepProgram program = HepProgram.builder()
+ .addRuleInstance(AggregateExpandDistinctAggregatesRule.JOIN)
+ .build();
+ sql(sql).with(program).check();
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-1558">[CALCITE-1558]
+ * AggregateExpandDistinctAggregatesRule gets field mapping wrong if groupKey
+ * is used in aggregate function</a>. */
+ @Test public void testDistinctNonDistinctAggregatesWithGrouping1() {
+ final String sql = "SELECT deptno,\n"
+ + " SUM(deptno), SUM(DISTINCT sal), MAX(deptno), MAX(comm)\n"
+ + "FROM emp\n"
+ + "GROUP BY deptno";
+ HepProgram program = new HepProgramBuilder()
+ .addRuleInstance(AggregateExpandDistinctAggregatesRule.JOIN)
+ .build();
+ sql(sql).with(program).check();
+ }
+
+ @Test public void testDistinctNonDistinctAggregatesWithGrouping2() {
+ final String sql = "SELECT deptno, COUNT(deptno), SUM(DISTINCT sal)\n"
+ + "FROM emp\n"
+ + "GROUP BY deptno";
+ HepProgram program = new HepProgramBuilder()
+ .addRuleInstance(AggregateExpandDistinctAggregatesRule.JOIN)
+ .build();
+ sql(sql).with(program).check();
+ }
+
@Test public void testPushProjectPastFilter() {
checkPlanning(ProjectFilterTransposeRule.INSTANCE,
"select empno + deptno from emp where sal = 10 * comm "
@@ -3066,16 +3102,6 @@ public class RelOptRulesTest extends RelOptTestBase {
sql(sql).with(program).check();
}
- @Test public void testDistinctNonDistinctAggregates() {
- final String sql = "select emp.empno, count(*), avg(distinct dept.deptno)\n"
- + "from sales.emp emp inner join sales.dept dept\n"
- + "on emp.deptno = dept.deptno\n"
- + "group by emp.empno";
- final HepProgram program = HepProgram.builder()
- .addRuleInstance(AggregateExpandDistinctAggregatesRule.JOIN)
- .build();
- sql(sql).with(program).check();
- }
}
// End RelOptRulesTest.java
http://git-wip-us.apache.org/repos/asf/calcite/blob/2b966375/core/src/test/java/org/apache/calcite/util/ImmutableBitSetTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/calcite/util/ImmutableBitSetTest.java b/core/src/test/java/org/apache/calcite/util/ImmutableBitSetTest.java
index 110f8b0..8894012 100644
--- a/core/src/test/java/org/apache/calcite/util/ImmutableBitSetTest.java
+++ b/core/src/test/java/org/apache/calcite/util/ImmutableBitSetTest.java
@@ -26,7 +26,9 @@ import org.junit.Test;
import java.nio.LongBuffer;
import java.util.Arrays;
import java.util.Collections;
+import java.util.HashSet;
import java.util.List;
+import java.util.Set;
import java.util.SortedMap;
import static org.hamcrest.CoreMatchers.equalTo;
@@ -179,18 +181,44 @@ public class ImmutableBitSetTest {
/**
* Tests the methods
- * {@link org.apache.calcite.util.ImmutableBitSet#toList} and
- * {@link org.apache.calcite.util.ImmutableBitSet#asList}.
+ * {@link org.apache.calcite.util.ImmutableBitSet#toList}, and
+ * {@link org.apache.calcite.util.ImmutableBitSet#asList} and
+ * {@link org.apache.calcite.util.ImmutableBitSet#asSet}.
*/
@Test public void testAsList() {
final List<ImmutableBitSet> list = getSortedList();
+
+ // create a set of integers in and not in the lists
+ final Set<Integer> integers = new HashSet<>();
+ for (ImmutableBitSet set : list) {
+ for (Integer integer : set) {
+ integers.add(integer);
+ integers.add(integer + 1);
+ integers.add(integer + 10);
+ }
+ }
+
for (ImmutableBitSet bitSet : list) {
final List<Integer> list1 = bitSet.toList();
final List<Integer> listView = bitSet.asList();
+ final Set<Integer> setView = bitSet.asSet();
assertThat(list1.size(), equalTo(bitSet.cardinality()));
+ assertThat(listView.size(), equalTo(bitSet.cardinality()));
+ assertThat(setView.size(), equalTo(bitSet.cardinality()));
assertThat(list1.toString(), equalTo(listView.toString()));
+ assertThat(list1.toString(), equalTo(setView.toString()));
assertTrue(list1.equals(listView));
assertThat(list1.hashCode(), equalTo(listView.hashCode()));
+
+ final Set<Integer> set = new HashSet<>(list1);
+ assertThat(setView.hashCode(), is(set.hashCode()));
+ assertThat(setView, equalTo(set));
+
+ for (Integer integer : integers) {
+ final boolean b = list1.contains(integer);
+ assertThat(listView.contains(integer), is(b));
+ assertThat(setView.contains(integer), is(b));
+ }
}
}
http://git-wip-us.apache.org/repos/asf/calcite/blob/2b966375/core/src/test/java/org/apache/calcite/util/UtilTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/calcite/util/UtilTest.java b/core/src/test/java/org/apache/calcite/util/UtilTest.java
index d9c65d8..4b970ce 100644
--- a/core/src/test/java/org/apache/calcite/util/UtilTest.java
+++ b/core/src/test/java/org/apache/calcite/util/UtilTest.java
@@ -1285,6 +1285,22 @@ public class UtilTest {
assertFalse(Util.isDistinct(Arrays.asList("a", null, "b", null)));
}
+ /** Unit test for {@link Util#intersects(Collection, Collection)}. */
+ @Test public void testIntersects() {
+ final List<String> empty = Collections.emptyList();
+ final List<String> listA = Collections.singletonList("a");
+ final List<String> listC = Collections.singletonList("c");
+ final List<String> listD = Collections.singletonList("d");
+ final List<String> listAbc = Arrays.asList("a", "b", "c");
+ assertThat(Util.intersects(empty, listA), is(false));
+ assertThat(Util.intersects(empty, empty), is(false));
+ assertThat(Util.intersects(listA, listAbc), is(true));
+ assertThat(Util.intersects(listAbc, listAbc), is(true));
+ assertThat(Util.intersects(listAbc, listC), is(true));
+ assertThat(Util.intersects(listAbc, listD), is(false));
+ assertThat(Util.intersects(listC, listD), is(false));
+ }
+
/**
* Unit test for {@link org.apache.calcite.util.JsonBuilder}.
*/
http://git-wip-us.apache.org/repos/asf/calcite/blob/2b966375/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml
----------------------------------------------------------------------
diff --git a/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml b/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml
index 1c8cb99..f76ef23 100644
--- a/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml
+++ b/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml
@@ -420,6 +420,51 @@ LogicalProject(DEPTNO=[$0], I0=[$2], I1=[$3])
]]>
</Resource>
</TestCase>
+ <TestCase name="testDistinctNonDistinctAggregatesWithGrouping1">
+ <Resource name="sql">
+ <![CDATA[SELECT deptno,
+ SUM(deptno), SUM(DISTINCT sal), MAX(deptno), MAX(comm)
+FROM emp
+GROUP BY deptno]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalAggregate(group=[{0}], EXPR$1=[SUM($0)], EXPR$2=[SUM(DISTINCT $1)], EXPR$3=[MAX($0)], EXPR$4=[MAX($2)])
+ LogicalProject(DEPTNO=[$7], SAL=[$5], COMM=[$6])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+LogicalAggregate(group=[{0}], EXPR$1=[SUM($2)], EXPR$2=[SUM($1)], EXPR$3=[MAX($3)], EXPR$4=[MAX($4)])
+ LogicalAggregate(group=[{0, 1}], EXPR$1=[SUM($0)], EXPR$3=[MAX($0)], EXPR$4=[MAX($2)])
+ LogicalProject(DEPTNO=[$7], SAL=[$5], COMM=[$6])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testDistinctNonDistinctAggregatesWithGrouping2">
+ <Resource name="sql">
+ <![CDATA[SELECT deptno, COUNT(deptno), SUM(DISTINCT sal)
+FROM emp
+GROUP BY deptno]]>
+ </Resource>
+ <Resource name="planBefore">
+ <![CDATA[
+LogicalAggregate(group=[{0}], EXPR$1=[COUNT()], EXPR$2=[SUM(DISTINCT $1)])
+ LogicalProject(DEPTNO=[$7], SAL=[$5])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ <Resource name="planAfter">
+ <![CDATA[
+LogicalAggregate(group=[{0}], EXPR$1=[SUM($2)], EXPR$2=[SUM($1)])
+ LogicalAggregate(group=[{0, 1}], EXPR$1=[COUNT()])
+ LogicalProject(DEPTNO=[$7], SAL=[$5])
+ LogicalTableScan(table=[[CATALOG, SALES, EMP]])
+]]>
+ </Resource>
+ </TestCase>
<TestCase name="testEmptyAggregate">
<Resource name="sql">
<![CDATA[select sum(empno) from emp where false group by deptno]]>