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 2020/02/04 00:15:54 UTC
[calcite] branch master updated (a6f544e -> c086877)
This is an automated email from the ASF dual-hosted git repository.
jhyde pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git.
from a6f544e [CALCITE-3707] Implement COSH function
new 13728c0 [CALCITE-3764] AggregateCaseToFilterRule handles NULL values incorrectly
new c086877 Switch RelBuilder.Config to an interface, and deprecate RelBuilder.ConfigBuilder
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../rel/rules/AggregateCaseToFilterRule.java | 2 +-
.../java/org/apache/calcite/tools/RelBuilder.java | 106 ++++++++++++---------
.../org/apache/calcite/test/PigRelBuilderTest.java | 12 +++
.../org/apache/calcite/test/RelBuilderTest.java | 45 ++++-----
core/src/test/resources/sql/agg.iq | 36 +++++++
5 files changed, 130 insertions(+), 71 deletions(-)
[calcite] 01/02: [CALCITE-3764] AggregateCaseToFilterRule handles
NULL values incorrectly
Posted by jh...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jhyde pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git
commit 13728c0320dcd2e042e775dcc19b615df5c4a614
Author: Julian Hyde <jh...@apache.org>
AuthorDate: Fri Jan 31 21:35:25 2020 -0800
[CALCITE-3764] AggregateCaseToFilterRule handles NULL values incorrectly
---
.../rel/rules/AggregateCaseToFilterRule.java | 2 +-
core/src/test/resources/sql/agg.iq | 36 ++++++++++++++++++++++
2 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/core/src/main/java/org/apache/calcite/rel/rules/AggregateCaseToFilterRule.java b/core/src/main/java/org/apache/calcite/rel/rules/AggregateCaseToFilterRule.java
index d7876cb..2db096f 100644
--- a/core/src/main/java/org/apache/calcite/rel/rules/AggregateCaseToFilterRule.java
+++ b/core/src/main/java/org/apache/calcite/rel/rules/AggregateCaseToFilterRule.java
@@ -166,7 +166,7 @@ public class AggregateCaseToFilterRule extends RelOptRule {
// Operand 1: Filter
final SqlPostfixOperator op =
- flip ? SqlStdOperatorTable.IS_FALSE : SqlStdOperatorTable.IS_TRUE;
+ flip ? SqlStdOperatorTable.IS_NOT_TRUE : SqlStdOperatorTable.IS_TRUE;
final RexNode filterFromCase =
rexBuilder.makeCall(op, caseCall.operands.get(0));
diff --git a/core/src/test/resources/sql/agg.iq b/core/src/test/resources/sql/agg.iq
index 6eac41b..8b3fd9c 100644
--- a/core/src/test/resources/sql/agg.iq
+++ b/core/src/test/resources/sql/agg.iq
@@ -1307,6 +1307,42 @@ group by deptno;
!ok
+# Convert CASE to FILTER
+select count(case x when 0 then null else -1 end) as c
+from (values 0, null, 0, 1) as t(x);
++---+
+| C |
++---+
+| 2 |
++---+
+(1 row)
+
+!ok
+
+# Same, expressed as FILTER
+select count(*) filter (where (x = 0) is not true) as c
+from (values 0, null, 0, 1) as t(x);
++---+
+| C |
++---+
+| 2 |
++---+
+(1 row)
+
+!ok
+
+# Similar, not quite the same
+select count(*) filter (where (x = 0) is false) as c
+from (values 0, null, 0, 1) as t(x);
++---+
+| C |
++---+
+| 1 |
++---+
+(1 row)
+
+!ok
+
# [CALCITE-1293] Bad code generated when argument to COUNT(DISTINCT) is a
# GROUP BY column
select count(distinct deptno) as cd, count(*) as c
[calcite] 02/02: Switch RelBuilder.Config to an interface,
and deprecate RelBuilder.ConfigBuilder
Posted by jh...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
jhyde pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git
commit c086877a8b7f7b47526d55e9e18dcb3576cdbaab
Author: Julian Hyde <jh...@apache.org>
AuthorDate: Fri Jan 31 16:57:31 2020 -0800
Switch RelBuilder.Config to an interface, and deprecate RelBuilder.ConfigBuilder
---
.../java/org/apache/calcite/tools/RelBuilder.java | 106 ++++++++++++---------
.../org/apache/calcite/test/PigRelBuilderTest.java | 12 +++
.../org/apache/calcite/test/RelBuilderTest.java | 45 ++++-----
3 files changed, 93 insertions(+), 70 deletions(-)
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 7b240e6..b4455a0 100644
--- a/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
+++ b/core/src/main/java/org/apache/calcite/tools/RelBuilder.java
@@ -85,6 +85,7 @@ import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.calcite.sql.type.TableFunctionReturnTypeInference;
import org.apache.calcite.sql.validate.SqlValidatorUtil;
import org.apache.calcite.util.Holder;
+import org.apache.calcite.util.ImmutableBeans;
import org.apache.calcite.util.ImmutableBitSet;
import org.apache.calcite.util.ImmutableIntList;
import org.apache.calcite.util.ImmutableNullableList;
@@ -234,11 +235,8 @@ public class RelBuilder {
private Config getConfig(Context context) {
final Config config =
Util.first(context.unwrap(Config.class), Config.DEFAULT);
- boolean simplify = Hook.REL_BUILDER_SIMPLIFY.get(config.simplify);
- if (simplify == config.simplify) {
- return config;
- }
- return config.toBuilder().withSimplify(simplify).build();
+ boolean simplify = Hook.REL_BUILDER_SIMPLIFY.get(config.simplify());
+ return config.withSimplify(simplify);
}
/** Creates a RelBuilder. */
@@ -1418,7 +1416,7 @@ public class RelBuilder {
}
// Simplify expressions.
- if (config.simplify) {
+ if (config.simplify()) {
for (int i = 0; i < nodeList.size(); i++) {
nodeList.set(i, simplifier.simplifyPreservingType(nodeList.get(i)));
}
@@ -1735,7 +1733,7 @@ public class RelBuilder {
assert groupSet.contains(set);
}
- if (!config.dedupAggregateCalls || Util.isDistinct(aggregateCalls)) {
+ if (!config.dedupAggregateCalls() || Util.isDistinct(aggregateCalls)) {
return aggregate_(groupSet, groupSets, r, aggregateCalls,
registrar.extraNodes, frame.fields);
}
@@ -2038,7 +2036,7 @@ public class RelBuilder {
final RelNode join;
final boolean correlate = variablesSet.size() == 1;
RexNode postCondition = literal(true);
- if (config.simplify) {
+ if (config.simplify()) {
// Normalize expanded versions IS NOT DISTINCT FROM so that simplifier does not
// transform the expression to something unrecognizable
if (condition instanceof RexCall) {
@@ -2751,6 +2749,21 @@ public class RelBuilder {
this.filter = filter;
}
+ @Override public String toString() {
+ final StringBuilder b = new StringBuilder();
+ b.append(aggFunction.getName())
+ .append('(');
+ if (distinct) {
+ b.append("DISTINCT ");
+ }
+ b.append(operands)
+ .append(')');
+ if (filter != null) {
+ b.append(" FILTER (WHERE" + filter + ")");
+ }
+ return b.toString();
+ }
+
public AggCall sort(Iterable<RexNode> orderKeys) {
final ImmutableList<RexNode> orderKeyList =
ImmutableList.copyOf(orderKeys);
@@ -2813,6 +2826,10 @@ public class RelBuilder {
this.aggregateCall = Objects.requireNonNull(aggregateCall);
}
+ @Override public String toString() {
+ return aggregateCall.toString();
+ }
+
public AggCall sort(Iterable<RexNode> orderKeys) {
throw new UnsupportedOperationException();
}
@@ -3001,69 +3018,68 @@ public class RelBuilder {
*
* <p>It is immutable, and all fields are public.
*
- * <p>Use the {@link #DEFAULT} instance,
- * or call {@link #builder()} to create a builder then
- * {@link RelBuilder.ConfigBuilder#build()}. You can also use
- * {@link #toBuilder()} to modify a few properties of an existing Config. */
- public static class Config {
+ * <p>Start with the {@link #DEFAULT} instance,
+ * and call {@code withXxx} methods to set its properties. */
+ public interface Config {
/** Default configuration. */
- public static final Config DEFAULT =
- new Config(true, true);
+ Config DEFAULT = ImmutableBeans.create(Config.class);
+
+ @Deprecated // to be removed before 2.0
+ static ConfigBuilder builder() {
+ return DEFAULT.toBuilder();
+ }
+
+ @Deprecated // to be removed before 2.0
+ default ConfigBuilder toBuilder() {
+ return new ConfigBuilder(this);
+ }
/** Whether {@link RelBuilder#aggregate} should eliminate duplicate
* aggregate calls; default true. */
- public final boolean dedupAggregateCalls;
+ @ImmutableBeans.Property
+ @ImmutableBeans.BooleanDefault(true)
+ boolean dedupAggregateCalls();
- /** Whether to simplify expressions; default true. */
- public final boolean simplify;
+ /** Sets {@link #dedupAggregateCalls}. */
+ Config withDedupAggregateCalls(boolean dedupAggregateCalls);
- // called only from ConfigBuilder and when creating DEFAULT;
- // parameters and fields must be in alphabetical order
- private Config(boolean dedupAggregateCalls,
- boolean simplify) {
- this.dedupAggregateCalls = dedupAggregateCalls;
- this.simplify = simplify;
- }
-
- /** Creates a ConfigBuilder with all properties set to their default
- * values. */
- public static ConfigBuilder builder() {
- return DEFAULT.toBuilder();
- }
+ /** Whether to simplify expressions; default true. */
+ @ImmutableBeans.Property
+ @ImmutableBeans.BooleanDefault(true)
+ boolean simplify();
- /** Creates a ConfigBuilder with properties set to the values in this
- * Config. */
- public ConfigBuilder toBuilder() {
- return new ConfigBuilder()
- .withDedupAggregateCalls(dedupAggregateCalls)
- .withSimplify(simplify);
- }
+ /** Sets {@link #simplify}. */
+ Config withSimplify(boolean simplify);
}
- /** Creates a {@link RelBuilder.Config}. */
+ /** Creates a {@link RelBuilder.Config}.
+ *
+ * @deprecated Use the {@code withXxx} methods in
+ * {@link RelBuilder.Config}. */
+ @Deprecated // to be removed before 2.0
public static class ConfigBuilder {
- private boolean dedupAggregateCalls;
- private boolean simplify;
+ private Config config;
- private ConfigBuilder() {
+ private ConfigBuilder(@Nonnull Config config) {
+ this.config = config;
}
/** Creates a {@link RelBuilder.Config}. */
public Config build() {
- return new Config(dedupAggregateCalls, simplify);
+ return config;
}
/** Sets the value that will become
* {@link org.apache.calcite.tools.RelBuilder.Config#dedupAggregateCalls}. */
public ConfigBuilder withDedupAggregateCalls(boolean dedupAggregateCalls) {
- this.dedupAggregateCalls = dedupAggregateCalls;
+ config = config.withDedupAggregateCalls(dedupAggregateCalls);
return this;
}
/** Sets the value that will become
* {@link org.apache.calcite.tools.RelBuilder.Config#simplify}. */
public ConfigBuilder withSimplify(boolean simplify) {
- this.simplify = simplify;
+ config = config.withSimplify(simplify);
return this;
}
}
diff --git a/core/src/test/java/org/apache/calcite/test/PigRelBuilderTest.java b/core/src/test/java/org/apache/calcite/test/PigRelBuilderTest.java
index ab5e7cd..43b6bbe 100644
--- a/core/src/test/java/org/apache/calcite/test/PigRelBuilderTest.java
+++ b/core/src/test/java/org/apache/calcite/test/PigRelBuilderTest.java
@@ -16,14 +16,18 @@
*/
package org.apache.calcite.test;
+import org.apache.calcite.plan.Contexts;
import org.apache.calcite.plan.RelOptUtil;
import org.apache.calcite.rel.RelNode;
import org.apache.calcite.tools.Frameworks;
import org.apache.calcite.tools.PigRelBuilder;
+import org.apache.calcite.tools.RelBuilder;
import org.apache.calcite.util.Util;
import org.junit.jupiter.api.Test;
+import java.util.function.UnaryOperator;
+
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
@@ -36,6 +40,14 @@ public class PigRelBuilderTest {
return RelBuilderTest.config();
}
+ static PigRelBuilder createBuilder(
+ UnaryOperator<RelBuilder.Config> transform) {
+ final Frameworks.ConfigBuilder configBuilder = config();
+ configBuilder.context(
+ Contexts.of(transform.apply(RelBuilder.Config.DEFAULT)));
+ return PigRelBuilder.create(configBuilder.build());
+ }
+
/** Converts a relational expression to a sting with linux line-endings. */
private String str(RelNode r) {
return Util.toLinux(RelOptUtil.toString(r));
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 c91d3c1..210b398 100644
--- a/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
+++ b/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
@@ -81,8 +81,7 @@ import java.util.List;
import java.util.Locale;
import java.util.NoSuchElementException;
import java.util.TreeSet;
-import java.util.function.Function;
-import javax.annotation.Nonnull;
+import java.util.function.UnaryOperator;
import static org.apache.calcite.test.Matchers.hasHints;
import static org.apache.calcite.test.Matchers.hasTree;
@@ -90,6 +89,7 @@ import static org.apache.calcite.test.Matchers.hasTree;
import static org.hamcrest.CoreMatchers.allOf;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.nullValue;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -164,12 +164,10 @@ public class RelBuilderTest {
return Frameworks.newConfigBuilder().defaultSchema(root);
}
- @Nonnull static RelBuilder createBuilder(
- Function<RelBuilder.ConfigBuilder, RelBuilder.ConfigBuilder> transform) {
+ static RelBuilder createBuilder(UnaryOperator<RelBuilder.Config> transform) {
final Frameworks.ConfigBuilder configBuilder = config();
configBuilder.context(
- Contexts.of(transform.apply(RelBuilder.Config.builder())
- .build()));
+ Contexts.of(transform.apply(RelBuilder.Config.DEFAULT)));
return RelBuilder.create(configBuilder.build());
}
@@ -396,19 +394,18 @@ public class RelBuilderTest {
/** Tests that {@link RelBuilder#project} simplifies expressions if and only if
* {@link RelBuilder.Config#simplify}. */
@Test public void testSimplify() {
- checkSimplify(configBuilder -> configBuilder.withSimplify(true),
+ checkSimplify(c -> c.withSimplify(true),
hasTree("LogicalProject($f0=[true])\n"
+ " LogicalTableScan(table=[[scott, EMP]])\n"));
- checkSimplify(configBuilder -> configBuilder,
+ checkSimplify(c -> c,
hasTree("LogicalProject($f0=[true])\n"
+ " LogicalTableScan(table=[[scott, EMP]])\n"));
- checkSimplify(configBuilder -> configBuilder.withSimplify(false),
+ checkSimplify(c -> c.withSimplify(false),
hasTree("LogicalProject($f0=[IS NOT NULL($0)])\n"
+ " LogicalTableScan(table=[[scott, EMP]])\n"));
}
- private void checkSimplify(
- Function<RelBuilder.ConfigBuilder, RelBuilder.ConfigBuilder> transform,
+ private void checkSimplify(UnaryOperator<RelBuilder.Config> transform,
Matcher<RelNode> matcher) {
final RelBuilder builder = createBuilder(transform);
final RelNode root =
@@ -728,7 +725,7 @@ public class RelBuilderTest {
assertTrue(root instanceof Project);
Project project = (Project) root;
Mappings.TargetMapping mapping = project.getMapping();
- assertTrue(mapping == null);
+ assertThat(mapping, nullValue());
}
private void project1(int value, SqlTypeName sqlTypeName, String message, String expected) {
@@ -999,30 +996,27 @@ public class RelBuilderTest {
/** Tests that {@link RelBuilder#aggregate} eliminates duplicate aggregate
* calls and creates a {@code Project} to compensate. */
@Test public void testAggregateEliminatesDuplicateCalls() {
- final RelBuilder builder =
- createBuilder(configBuilder ->
- configBuilder.withDedupAggregateCalls(true));
final String expected = ""
+ "LogicalProject(S1=[$0], C=[$1], S2=[$2], S1b=[$0])\n"
+ " LogicalAggregate(group=[{}], S1=[SUM($1)], C=[COUNT()], S2=[SUM($2)])\n"
+ " LogicalTableScan(table=[[scott, EMP]])\n";
- assertThat(buildRelWithDuplicateAggregates(builder), hasTree(expected));
+ assertThat(
+ buildRelWithDuplicateAggregates(c -> c.withDedupAggregateCalls(true)),
+ hasTree(expected));
// Now, disable the rewrite
- final RelBuilder builder2 =
- createBuilder(configBuilder ->
- configBuilder.withDedupAggregateCalls(false));
final String expected2 = ""
+ "LogicalAggregate(group=[{}], S1=[SUM($1)], C=[COUNT()], S2=[SUM($2)], S1b=[SUM($1)])\n"
+ " LogicalTableScan(table=[[scott, EMP]])\n";
- assertThat(buildRelWithDuplicateAggregates(builder2), hasTree(expected2));
+ assertThat(
+ buildRelWithDuplicateAggregates(c -> c.withDedupAggregateCalls(false)),
+ hasTree(expected2));
}
/** As {@link #testAggregateEliminatesDuplicateCalls()} but with a
* single-column GROUP BY clause. */
@Test public void testAggregateEliminatesDuplicateCalls2() {
- final RelBuilder builder = RelBuilder.create(config().build());
- RelNode root = buildRelWithDuplicateAggregates(builder, 0);
+ RelNode root = buildRelWithDuplicateAggregates(c -> c, 0);
final String expected = ""
+ "LogicalProject(EMPNO=[$0], S1=[$1], C=[$2], S2=[$3], S1b=[$1])\n"
+ " LogicalAggregate(group=[{0}], S1=[SUM($1)], C=[COUNT()], S2=[SUM($2)])\n"
@@ -1033,8 +1027,7 @@ public class RelBuilderTest {
/** As {@link #testAggregateEliminatesDuplicateCalls()} but with a
* multi-column GROUP BY clause. */
@Test public void testAggregateEliminatesDuplicateCalls3() {
- final RelBuilder builder = RelBuilder.create(config().build());
- RelNode root = buildRelWithDuplicateAggregates(builder, 2, 0, 4, 3);
+ RelNode root = buildRelWithDuplicateAggregates(c -> c, 2, 0, 4, 3);
final String expected = ""
+ "LogicalProject(EMPNO=[$0], JOB=[$1], MGR=[$2], HIREDATE=[$3], S1=[$4], C=[$5], S2=[$6], S1b=[$4])\n"
+ " LogicalAggregate(group=[{0, 2, 3, 4}], S1=[SUM($1)], C=[COUNT()], S2=[SUM($2)])\n"
@@ -1042,8 +1035,10 @@ public class RelBuilderTest {
assertThat(root, hasTree(expected));
}
- private RelNode buildRelWithDuplicateAggregates(RelBuilder builder,
+ private RelNode buildRelWithDuplicateAggregates(
+ UnaryOperator<RelBuilder.Config> transform,
int... groupFieldOrdinals) {
+ final RelBuilder builder = createBuilder(transform);
return builder.scan("EMP")
.aggregate(builder.groupKey(groupFieldOrdinals),
builder.sum(builder.field(1)).as("S1"),