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 2018/04/11 07:56:02 UTC
[2/2] calcite git commit: [CALCITE-2206] JDBC adapter incorrectly
pushes windowed aggregates down to HSQLDB (Pavel Gubin)
[CALCITE-2206] JDBC adapter incorrectly pushes windowed aggregates down to HSQLDB (Pavel Gubin)
Close apache/calcite#646
Project: http://git-wip-us.apache.org/repos/asf/calcite/repo
Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/502c1084
Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/502c1084
Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/502c1084
Branch: refs/heads/master
Commit: 502c108473e535a76cebc5e09bd6be5efcc24531
Parents: 000622d
Author: pavelgubin <pa...@contiamo.com>
Authored: Wed Mar 7 14:10:54 2018 +0100
Committer: Julian Hyde <jh...@apache.org>
Committed: Tue Apr 10 23:26:19 2018 -0700
----------------------------------------------------------------------
.../apache/calcite/adapter/jdbc/JdbcRules.java | 152 +++++++++++++++----
.../java/org/apache/calcite/sql/SqlDialect.java | 5 +
.../calcite/sql/dialect/AccessSqlDialect.java | 4 +
.../calcite/sql/dialect/H2SqlDialect.java | 4 +
.../calcite/sql/dialect/HsqldbSqlDialect.java | 4 +
.../sql/dialect/InfobrightSqlDialect.java | 5 +
.../apache/calcite/test/JdbcAdapterTest.java | 24 +++
7 files changed, 165 insertions(+), 33 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/calcite/blob/502c1084/core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java b/core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java
index 8c07fef..dd0c461 100644
--- a/core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java
+++ b/core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java
@@ -58,14 +58,19 @@ import org.apache.calcite.rex.RexInputRef;
import org.apache.calcite.rex.RexLiteral;
import org.apache.calcite.rex.RexMultisetUtil;
import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexOver;
import org.apache.calcite.rex.RexProgram;
+import org.apache.calcite.runtime.PredicateImpl;
import org.apache.calcite.schema.ModifiableTable;
import org.apache.calcite.sql.SqlAggFunction;
import org.apache.calcite.sql.SqlDialect;
+import org.apache.calcite.tools.RelBuilderFactory;
import org.apache.calcite.util.ImmutableBitSet;
import org.apache.calcite.util.Util;
import org.apache.calcite.util.trace.CalciteTrace;
+import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
import com.google.common.collect.ImmutableList;
import org.slf4j.Logger;
@@ -73,6 +78,7 @@ import org.slf4j.Logger;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
+import javax.annotation.Nullable;
/**
* Rules and relational operators for
@@ -86,37 +92,57 @@ public class JdbcRules {
protected static final Logger LOGGER = CalciteTrace.getPlannerTracer();
public static List<RelOptRule> rules(JdbcConvention out) {
+ return rules(out, RelFactories.LOGICAL_BUILDER);
+ }
+
+ public static List<RelOptRule> rules(JdbcConvention out,
+ RelBuilderFactory relBuilderFactory) {
return ImmutableList.<RelOptRule>of(
- new JdbcToEnumerableConverterRule(out, RelFactories.LOGICAL_BUILDER),
- new JdbcJoinRule(out),
- new JdbcCalcRule(out),
- new JdbcProjectRule(out),
- new JdbcFilterRule(out),
- new JdbcAggregateRule(out),
- new JdbcSortRule(out),
- new JdbcUnionRule(out),
- new JdbcIntersectRule(out),
- new JdbcMinusRule(out),
- new JdbcTableModificationRule(out),
- new JdbcValuesRule(out));
+ new JdbcToEnumerableConverterRule(out, relBuilderFactory),
+ new JdbcJoinRule(out, relBuilderFactory),
+ new JdbcCalcRule(out, relBuilderFactory),
+ new JdbcProjectRule(out, relBuilderFactory),
+ new JdbcFilterRule(out, relBuilderFactory),
+ new JdbcAggregateRule(out, relBuilderFactory),
+ new JdbcSortRule(out, relBuilderFactory),
+ new JdbcUnionRule(out, relBuilderFactory),
+ new JdbcIntersectRule(out, relBuilderFactory),
+ new JdbcMinusRule(out, relBuilderFactory),
+ new JdbcTableModificationRule(out, relBuilderFactory),
+ new JdbcValuesRule(out, relBuilderFactory));
}
/** Abstract base class for rule that converts to JDBC. */
abstract static class JdbcConverterRule extends ConverterRule {
protected final JdbcConvention out;
+ @Deprecated // to be removed before 2.0
JdbcConverterRule(Class<? extends RelNode> clazz, RelTrait in,
JdbcConvention out, String description) {
- super(clazz, in, out, description);
+ this(clazz, Predicates.<RelNode>alwaysTrue(), in, out,
+ RelFactories.LOGICAL_BUILDER, description);
+ }
+
+ <R extends RelNode> JdbcConverterRule(Class<R> clazz,
+ Predicate<? super R> predicate, RelTrait in, JdbcConvention out,
+ RelBuilderFactory relBuilderFactory, String description) {
+ super(clazz, predicate, in, out, relBuilderFactory, description);
this.out = out;
}
}
/** Rule that converts a join to JDBC. */
public static class JdbcJoinRule extends JdbcConverterRule {
- /** Creates a JdbcJoinRule. */
+ @Deprecated // to be removed before 2.0
public JdbcJoinRule(JdbcConvention out) {
- super(Join.class, Convention.NONE, out, "JdbcJoinRule");
+ this(out, RelFactories.LOGICAL_BUILDER);
+ }
+
+ /** Creates a JdbcJoinRule. */
+ public JdbcJoinRule(JdbcConvention out,
+ RelBuilderFactory relBuilderFactory) {
+ super(Join.class, Predicates.<RelNode>alwaysTrue(), Convention.NONE,
+ out, relBuilderFactory, "JdbcJoinRule");
}
@Override public RelNode convert(RelNode rel) {
@@ -267,8 +293,11 @@ public class JdbcRules {
* {@link org.apache.calcite.adapter.jdbc.JdbcRules.JdbcCalc}.
*/
private static class JdbcCalcRule extends JdbcConverterRule {
- private JdbcCalcRule(JdbcConvention out) {
- super(Calc.class, Convention.NONE, out, "JdbcCalcRule");
+ /** Creates a JdbcCalcRule. */
+ private JdbcCalcRule(JdbcConvention out,
+ RelBuilderFactory relBuilderFactory) {
+ super(Calc.class, Predicates.<RelNode>alwaysTrue(), Convention.NONE,
+ out, relBuilderFactory, "JdbcCalcRule");
}
public RelNode convert(RelNode rel) {
@@ -340,8 +369,23 @@ public class JdbcRules {
* an {@link org.apache.calcite.adapter.jdbc.JdbcRules.JdbcProject}.
*/
public static class JdbcProjectRule extends JdbcConverterRule {
- public JdbcProjectRule(JdbcConvention out) {
- super(Project.class, Convention.NONE, out, "JdbcProjectRule");
+ @Deprecated // to be removed before 2.0
+ public JdbcProjectRule(final JdbcConvention out) {
+ this(out, RelFactories.LOGICAL_BUILDER);
+ }
+
+ /** Creates a JdbcProjectRule. */
+ public JdbcProjectRule(final JdbcConvention out,
+ RelBuilderFactory relBuilderFactory) {
+ super(Project.class,
+ new PredicateImpl<Project>() {
+ public boolean test(@Nullable Project project) {
+ assert project != null;
+ return out.dialect.supportsWindowFunctions()
+ && !RexOver.containsOver(project.getProjects(), null);
+ }
+ },
+ Convention.NONE, out, relBuilderFactory, "JdbcProjectRule");
}
public RelNode convert(RelNode rel) {
@@ -401,8 +445,16 @@ public class JdbcRules {
* an {@link org.apache.calcite.adapter.jdbc.JdbcRules.JdbcFilter}.
*/
public static class JdbcFilterRule extends JdbcConverterRule {
+ @Deprecated // to be removed before 2.0
public JdbcFilterRule(JdbcConvention out) {
- super(Filter.class, Convention.NONE, out, "JdbcFilterRule");
+ this(out, RelFactories.LOGICAL_BUILDER);
+ }
+
+ /** Creates a JdbcFilterRule. */
+ public JdbcFilterRule(JdbcConvention out,
+ RelBuilderFactory relBuilderFactory) {
+ super(Filter.class, Predicates.<RelNode>alwaysTrue(), Convention.NONE,
+ out, relBuilderFactory, "JdbcFilterRule");
}
public RelNode convert(RelNode rel) {
@@ -444,8 +496,16 @@ public class JdbcRules {
* to a {@link org.apache.calcite.adapter.jdbc.JdbcRules.JdbcAggregate}.
*/
public static class JdbcAggregateRule extends JdbcConverterRule {
+ @Deprecated // to be removed before 2.0
public JdbcAggregateRule(JdbcConvention out) {
- super(Aggregate.class, Convention.NONE, out, "JdbcAggregateRule");
+ this(out, RelFactories.LOGICAL_BUILDER);
+ }
+
+ /** Creates a JdbcAggregateRule. */
+ public JdbcAggregateRule(JdbcConvention out,
+ RelBuilderFactory relBuilderFactory) {
+ super(Aggregate.class, Predicates.<RelNode>alwaysTrue(), Convention.NONE,
+ out, relBuilderFactory, "JdbcAggregateRule");
}
public RelNode convert(RelNode rel) {
@@ -521,9 +581,16 @@ public class JdbcRules {
* {@link org.apache.calcite.adapter.jdbc.JdbcRules.JdbcSort}.
*/
public static class JdbcSortRule extends JdbcConverterRule {
- /** Creates a JdbcSortRule. */
+ @Deprecated // to be removed before 2.0
public JdbcSortRule(JdbcConvention out) {
- super(Sort.class, Convention.NONE, out, "JdbcSortRule");
+ this(out, RelFactories.LOGICAL_BUILDER);
+ }
+
+ /** Creates a JdbcSortRule. */
+ public JdbcSortRule(JdbcConvention out,
+ RelBuilderFactory relBuilderFactory) {
+ super(Sort.class, Predicates.<RelNode>alwaysTrue(), Convention.NONE, out,
+ relBuilderFactory, "JdbcSortRule");
}
public RelNode convert(RelNode rel) {
@@ -585,8 +652,16 @@ public class JdbcRules {
* {@link org.apache.calcite.adapter.jdbc.JdbcRules.JdbcUnion}.
*/
public static class JdbcUnionRule extends JdbcConverterRule {
+ @Deprecated // to be removed before 2.0
public JdbcUnionRule(JdbcConvention out) {
- super(Union.class, Convention.NONE, out, "JdbcUnionRule");
+ this(out, RelFactories.LOGICAL_BUILDER);
+ }
+
+ /** Creates a JdbcUnionRule. */
+ public JdbcUnionRule(JdbcConvention out,
+ RelBuilderFactory relBuilderFactory) {
+ super(Union.class, Predicates.<RelNode>alwaysTrue(), Convention.NONE, out,
+ relBuilderFactory, "JdbcUnionRule");
}
public RelNode convert(RelNode rel) {
@@ -628,8 +703,11 @@ public class JdbcRules {
* to a {@link org.apache.calcite.adapter.jdbc.JdbcRules.JdbcIntersect}.
*/
public static class JdbcIntersectRule extends JdbcConverterRule {
- private JdbcIntersectRule(JdbcConvention out) {
- super(Intersect.class, Convention.NONE, out, "JdbcIntersectRule");
+ /** Creates a JdbcIntersectRule. */
+ private JdbcIntersectRule(JdbcConvention out,
+ RelBuilderFactory relBuilderFactory) {
+ super(Intersect.class, Predicates.<RelNode>alwaysTrue(), Convention.NONE,
+ out, relBuilderFactory, "JdbcIntersectRule");
}
public RelNode convert(RelNode rel) {
@@ -672,8 +750,11 @@ public class JdbcRules {
* {@link org.apache.calcite.adapter.jdbc.JdbcRules.JdbcMinus}.
*/
public static class JdbcMinusRule extends JdbcConverterRule {
- private JdbcMinusRule(JdbcConvention out) {
- super(Minus.class, Convention.NONE, out, "JdbcMinusRule");
+ /** Creates a JdbcMinusRule. */
+ private JdbcMinusRule(JdbcConvention out,
+ RelBuilderFactory relBuilderFactory) {
+ super(Minus.class, Predicates.<RelNode>alwaysTrue(), Convention.NONE, out,
+ relBuilderFactory, "JdbcMinusRule");
}
public RelNode convert(RelNode rel) {
@@ -708,9 +789,11 @@ public class JdbcRules {
/** Rule that converts a table-modification to JDBC. */
public static class JdbcTableModificationRule extends JdbcConverterRule {
- private JdbcTableModificationRule(JdbcConvention out) {
- super(TableModify.class, Convention.NONE, out,
- "JdbcTableModificationRule");
+ /** Creates a JdbcTableModificationRule. */
+ private JdbcTableModificationRule(JdbcConvention out,
+ RelBuilderFactory relBuilderFactory) {
+ super(TableModify.class, Predicates.<RelNode>alwaysTrue(),
+ Convention.NONE, out, relBuilderFactory, "JdbcTableModificationRule");
}
@Override public RelNode convert(RelNode rel) {
@@ -782,8 +865,11 @@ public class JdbcRules {
/** Rule that converts a values operator to JDBC. */
public static class JdbcValuesRule extends JdbcConverterRule {
- private JdbcValuesRule(JdbcConvention out) {
- super(Values.class, Convention.NONE, out, "JdbcValuesRule");
+ /** Creates a JdbcValuesRule. */
+ private JdbcValuesRule(JdbcConvention out,
+ RelBuilderFactory relBuilderFactory) {
+ super(Values.class, Predicates.<RelNode>alwaysTrue(), Convention.NONE,
+ out, relBuilderFactory, "JdbcValuesRule");
}
@Override public RelNode convert(RelNode rel) {
http://git-wip-us.apache.org/repos/asf/calcite/blob/502c1084/core/src/main/java/org/apache/calcite/sql/SqlDialect.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/calcite/sql/SqlDialect.java b/core/src/main/java/org/apache/calcite/sql/SqlDialect.java
index c0ebbb9..3e679d3 100644
--- a/core/src/main/java/org/apache/calcite/sql/SqlDialect.java
+++ b/core/src/main/java/org/apache/calcite/sql/SqlDialect.java
@@ -582,6 +582,11 @@ public class SqlDialect {
return false;
}
+ /** Returns whether this dialect supports window functions (OVER clause). */
+ public boolean supportsWindowFunctions() {
+ return true;
+ }
+
/** Returns whether this dialect supports a given function or operator. */
public boolean supportsFunction(SqlOperator operator, RelDataType type,
List<RelDataType> paramTypes) {
http://git-wip-us.apache.org/repos/asf/calcite/blob/502c1084/core/src/main/java/org/apache/calcite/sql/dialect/AccessSqlDialect.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/calcite/sql/dialect/AccessSqlDialect.java b/core/src/main/java/org/apache/calcite/sql/dialect/AccessSqlDialect.java
index 1c2861f..91d69e7 100644
--- a/core/src/main/java/org/apache/calcite/sql/dialect/AccessSqlDialect.java
+++ b/core/src/main/java/org/apache/calcite/sql/dialect/AccessSqlDialect.java
@@ -31,6 +31,10 @@ public class AccessSqlDialect extends SqlDialect {
public AccessSqlDialect(Context context) {
super(context);
}
+
+ @Override public boolean supportsWindowFunctions() {
+ return false;
+ }
}
// End AccessSqlDialect.java
http://git-wip-us.apache.org/repos/asf/calcite/blob/502c1084/core/src/main/java/org/apache/calcite/sql/dialect/H2SqlDialect.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/calcite/sql/dialect/H2SqlDialect.java b/core/src/main/java/org/apache/calcite/sql/dialect/H2SqlDialect.java
index 005482a..a30c3f0 100644
--- a/core/src/main/java/org/apache/calcite/sql/dialect/H2SqlDialect.java
+++ b/core/src/main/java/org/apache/calcite/sql/dialect/H2SqlDialect.java
@@ -35,6 +35,10 @@ public class H2SqlDialect extends SqlDialect {
@Override public boolean supportsCharSet() {
return false;
}
+
+ @Override public boolean supportsWindowFunctions() {
+ return false;
+ }
}
// End H2SqlDialect.java
http://git-wip-us.apache.org/repos/asf/calcite/blob/502c1084/core/src/main/java/org/apache/calcite/sql/dialect/HsqldbSqlDialect.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/calcite/sql/dialect/HsqldbSqlDialect.java b/core/src/main/java/org/apache/calcite/sql/dialect/HsqldbSqlDialect.java
index 70dd55d..1cbd56c 100644
--- a/core/src/main/java/org/apache/calcite/sql/dialect/HsqldbSqlDialect.java
+++ b/core/src/main/java/org/apache/calcite/sql/dialect/HsqldbSqlDialect.java
@@ -46,6 +46,10 @@ public class HsqldbSqlDialect extends SqlDialect {
return false;
}
+ @Override public boolean supportsWindowFunctions() {
+ return false;
+ }
+
@Override public void unparseCall(SqlWriter writer, SqlCall call,
int leftPrec, int rightPrec) {
switch (call.getKind()) {
http://git-wip-us.apache.org/repos/asf/calcite/blob/502c1084/core/src/main/java/org/apache/calcite/sql/dialect/InfobrightSqlDialect.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/calcite/sql/dialect/InfobrightSqlDialect.java b/core/src/main/java/org/apache/calcite/sql/dialect/InfobrightSqlDialect.java
index 78ba060..80f8baf 100644
--- a/core/src/main/java/org/apache/calcite/sql/dialect/InfobrightSqlDialect.java
+++ b/core/src/main/java/org/apache/calcite/sql/dialect/InfobrightSqlDialect.java
@@ -31,6 +31,11 @@ public class InfobrightSqlDialect extends SqlDialect {
public InfobrightSqlDialect(Context context) {
super(context);
}
+
+ @Override public boolean supportsWindowFunctions() {
+ return false;
+ }
+
}
// End InfobrightSqlDialect.java
http://git-wip-us.apache.org/repos/asf/calcite/blob/502c1084/core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java b/core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java
index c35ef8a..a4db644 100644
--- a/core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java
+++ b/core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java
@@ -419,6 +419,30 @@ public class JdbcAdapterTest {
}
/** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-2206">[CALCITE-2206]
+ * JDBC adapter incorrectly pushes windowed aggregates down to HSQLDB</a>. */
+ @Test public void testOverNonSupportedDialect() {
+ final String sql = "select \"store_id\", \"account_id\", \"exp_date\",\n"
+ + " \"time_id\", \"category_id\", \"currency_id\", \"amount\",\n"
+ + " last_value(\"time_id\") over () as \"last_version\"\n"
+ + "from \"expense_fact\"";
+ final String explain = "PLAN="
+ + "EnumerableWindow(window#0=[window(partition {} "
+ + "order by [] range between UNBOUNDED PRECEDING and "
+ + "UNBOUNDED FOLLOWING aggs [LAST_VALUE($3)])])\n"
+ + " JdbcToEnumerableConverter\n"
+ + " JdbcTableScan(table=[[foodmart, expense_fact]])\n";
+ CalciteAssert
+ .model(JdbcTest.FOODMART_MODEL)
+ .enable(CalciteAssert.DB == DatabaseInstance.HSQLDB)
+ .query(sql)
+ .explainContains(explain)
+ .runs()
+ .planHasSql("SELECT *\n"
+ + "FROM \"foodmart\".\"expense_fact\"");
+ }
+
+ /** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-1506">[CALCITE-1506]
* Push OVER Clause to underlying SQL via JDBC adapter</a>.
*