[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>. *
