Repository: calcite Updated Branches: refs/heads/master a0928af70 -> f17c376b6
[CALCITE-2323] Apply "defaultNullCollation" configuration parameter when translating ORDER BY inside OVER (John Fang) Close apache/calcite#697 Project: http://git-wip-us.apache.org/repos/asf/calcite/repo Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/f17c376b Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/f17c376b Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/f17c376b Branch: refs/heads/master Commit: f17c376b6f8441d4f029a450588179341d90208f Parents: a0928af Author: xiaojian.fxj <[email protected]> Authored: Thu May 24 00:04:13 2018 +0800 Committer: Julian Hyde <[email protected]> Committed: Thu May 24 21:40:44 2018 -0700 ---------------------------------------------------------------------- .../calcite/sql2rel/SqlToRelConverter.java | 55 ++++++++++++++++---- .../calcite/test/SqlToRelConverterTest.java | 26 +++++++++ .../apache/calcite/test/SqlToRelTestBase.java | 5 ++ .../calcite/test/SqlToRelConverterTest.xml | 16 ++++++ 4 files changed, 91 insertions(+), 11 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/calcite/blob/f17c376b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java index d07927e..9da95a5 100644 --- a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java +++ b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java @@ -1878,14 +1878,16 @@ public class SqlToRelConverter { "Relation should have sort key for implicit ORDER BY"); } } + final ImmutableList.Builder<RexFieldCollation> orderKeys = ImmutableList.builder(); - final Set<SqlKind> flags = EnumSet.noneOf(SqlKind.class); for (SqlNode order : orderList) { - flags.clear(); - RexNode e = bb.convertSortExpression(order, flags); - orderKeys.add(new RexFieldCollation(e, flags)); + orderKeys.add( + bb.convertSortExpression(order, + RelFieldCollation.Direction.ASCENDING, + RelFieldCollation.NullDirection.UNSPECIFIED)); } + try { Preconditions.checkArgument(bb.window == null, "already in window agg mode"); @@ -4554,19 +4556,50 @@ public class SqlToRelConverter { } /** - * Converts an item in an ORDER BY clause, extracting DESC, NULLS LAST - * and NULLS FIRST flags first. + * Converts an item in an ORDER BY clause inside a window (OVER) clause, + * extracting DESC, NULLS LAST and NULLS FIRST flags first. */ - public RexNode convertSortExpression(SqlNode expr, Set<SqlKind> flags) { + public RexFieldCollation convertSortExpression(SqlNode expr, + RelFieldCollation.Direction direction, + RelFieldCollation.NullDirection nullDirection) { switch (expr.getKind()) { case DESCENDING: + return convertSortExpression(((SqlCall) expr).operand(0), + RelFieldCollation.Direction.DESCENDING, nullDirection); case NULLS_LAST: + return convertSortExpression(((SqlCall) expr).operand(0), + direction, RelFieldCollation.NullDirection.LAST); case NULLS_FIRST: - flags.add(expr.getKind()); - final SqlNode operand = ((SqlCall) expr).operand(0); - return convertSortExpression(operand, flags); + return convertSortExpression(((SqlCall) expr).operand(0), + direction, RelFieldCollation.NullDirection.FIRST); default: - return convertExpression(expr); + final Set<SqlKind> flags = EnumSet.noneOf(SqlKind.class); + switch (direction) { + case DESCENDING: + flags.add(SqlKind.DESCENDING); + } + switch (nullDirection) { + case UNSPECIFIED: + final RelFieldCollation.NullDirection nullDefaultDirection = + validator.getDefaultNullCollation().last(desc(direction)) + ? RelFieldCollation.NullDirection.LAST + : RelFieldCollation.NullDirection.FIRST; + if (nullDefaultDirection != direction.defaultNullDirection()) { + SqlKind nullDirectionSqlKind = + validator.getDefaultNullCollation().last(desc(direction)) + ? SqlKind.NULLS_LAST + : SqlKind.NULLS_FIRST; + flags.add(nullDirectionSqlKind); + } + break; + case FIRST: + flags.add(SqlKind.NULLS_FIRST); + break; + case LAST: + flags.add(SqlKind.NULLS_LAST); + break; + } + return new RexFieldCollation(convertExpression(expr), flags); } } http://git-wip-us.apache.org/repos/asf/calcite/blob/f17c376b/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java b/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java index 844313e..bc6d6bd 100644 --- a/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java +++ b/core/src/test/java/org/apache/calcite/test/SqlToRelConverterTest.java @@ -16,6 +16,10 @@ */ package org.apache.calcite.test; +import org.apache.calcite.config.CalciteConnectionConfigImpl; +import org.apache.calcite.config.CalciteConnectionProperty; +import org.apache.calcite.config.NullCollation; +import org.apache.calcite.plan.Contexts; import org.apache.calcite.plan.RelOptUtil; import org.apache.calcite.prepare.Prepare; import org.apache.calcite.rel.RelNode; @@ -45,6 +49,7 @@ import java.io.PrintWriter; import java.io.StringWriter; import java.util.ArrayDeque; import java.util.Deque; +import java.util.Properties; import java.util.Set; import static org.hamcrest.CoreMatchers.notNullValue; @@ -2813,6 +2818,27 @@ public class SqlToRelConverterTest extends SqlToRelTestBase { sql(sql).ok(); } + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-2323";>[CALCITE-2323] + * Validator should allow alternative nullCollations for ORDER BY in + * OVER</a>. */ + @Test public void testUserDefinedOrderByOver() { + String sql = "select deptno,\n" + + " rank() over(partition by empno order by deptno)\n" + + "from emp\n" + + "order by row_number() over(partition by empno order by deptno)"; + Properties properties = new Properties(); + properties.setProperty( + CalciteConnectionProperty.DEFAULT_NULL_COLLATION.camelName(), + NullCollation.LOW.name()); + CalciteConnectionConfigImpl connectionConfig = + new CalciteConnectionConfigImpl(properties); + TesterImpl tester = new TesterImpl(getDiffRepos(), false, false, true, false, + null, null, SqlToRelConverter.Config.DEFAULT, + SqlConformanceEnum.DEFAULT, Contexts.of(connectionConfig)); + sql(sql).with(tester).ok(); + } + /** * Visitor that checks that every {@link RelNode} in a tree is valid. * http://git-wip-us.apache.org/repos/asf/calcite/blob/f17c376b/core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java b/core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java index 5d627b8..c1b42d0 100644 --- a/core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java +++ b/core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java @@ -16,6 +16,7 @@ */ package org.apache.calcite.test; +import org.apache.calcite.config.CalciteConnectionConfig; import org.apache.calcite.linq4j.tree.Expression; import org.apache.calcite.plan.Context; import org.apache.calcite.plan.Contexts; @@ -573,6 +574,10 @@ public abstract class SqlToRelTestBase { final SqlValidator validator = createValidator( catalogReader, typeFactory); + final CalciteConnectionConfig calciteConfig = context.unwrap(CalciteConnectionConfig.class); + if (calciteConfig != null) { + validator.setDefaultNullCollation(calciteConfig.defaultNullCollation()); + } if (config == SqlToRelConverter.Config.DEFAULT) { localConfig = SqlToRelConverter.configBuilder() .withTrimUnusedFields(true).withExpand(enableExpand).build(); http://git-wip-us.apache.org/repos/asf/calcite/blob/f17c376b/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml ---------------------------------------------------------------------- diff --git a/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml b/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml index c55ad57..160792a 100644 --- a/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml +++ b/core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml @@ -5192,6 +5192,22 @@ LogicalProject(EMPNO=[$0]) ]]> </Resource> </TestCase> + <TestCase name="testUserDefinedOrderByOver"> + <Resource name="sql"> + <![CDATA[select deptno, + rank() over(partition by empno order by deptno) +from emp +order by row_number() over(partition by empno order by deptno)]]> + </Resource> + <Resource name="plan"> + <![CDATA[ +LogicalProject(DEPTNO=[$0], EXPR$1=[$1]) + LogicalSort(sort0=[$2], dir0=[ASC-nulls-first]) + LogicalProject(DEPTNO=[$7], EXPR$1=[RANK() OVER (PARTITION BY $0 ORDER BY $7 NULLS FIRST RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)], EXPR$2=[ROW_NUMBER() OVER (PARTITION BY $0 ORDER BY $7 NULLS FIRST ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW)]) + LogicalTableScan(table=[[CATALOG, SALES, EMP]]) +]]> + </Resource> + </TestCase> <TestCase name="testWindowOnDynamicStar"> <Resource name="sql"> <