DRILL-1062: Implemented null ordering (NULLS FIRST/NULLS LAST).

Primary: - Split "compare_to" function templates (for sorting) into "compare_to_nulls_high" and "compare_to_nulls_low" versions. - Added tests to verify ORDER BY ordering. - Added tests to verify merge join order correctness. - Implemented java.sql.DatabaseMetaData.nullsAreSortedHigh(), etc. Secondary: - Eliminated DateInterfaceFunctions.java template (merged into other). - Renamed comparison-related template data objects and file names. - Eliminated unused template macros, function template classes. - Overhauled Order.Ordering; added unit test. - Regularized some generated-class names. Miscellaneous: - Added toString() to ExpressionPosition, Order.Ordering, JoinStatus. - Fixed some typos. - Fixed some comment syntax.
apache · Feb 23, 2015 · 5efc7e6 · 5efc7e6
1 parent 3c85bd8
commit 5efc7e6
Show file tree

Hide file tree

Showing 50 changed files with 3,101 additions and 1,836 deletions.
diff --git a/.gitignore b/.gitignore
@@ -4,6 +4,7 @@
 .checkstyle
 .settings/
 .idea/
+TAGS
 *.log
 *.lck
 *.iml

diff --git a/common/src/main/java/org/apache/drill/common/expression/ExpressionPosition.java b/common/src/main/java/org/apache/drill/common/expression/ExpressionPosition.java
@@ -31,6 +31,12 @@ public ExpressionPosition(String expression, int charIndex) {
     this.charIndex = charIndex;
   }
 
+  @Override
+  public String toString() {
+    return super.toString()
+           + "[charIndex = " + charIndex + ", expression = " + expression + "]";
+  }
+
   public String getExpression() {
     return expression;
   }

diff --git a/common/src/main/java/org/apache/drill/common/expression/fn/CastFunctions.java b/common/src/main/java/org/apache/drill/common/expression/fn/CastFunctions.java
@@ -27,9 +27,13 @@
 public class CastFunctions {
 
   private static Map<MinorType, String> TYPE2FUNC = new HashMap<>();
-  private static Set<String> CAST_FUNC_REPLACEMENT_NEEDED = new HashSet<>();  // The cast fucntions which are needed to be replaced (if "drill.exec.functions.cast_empty_string_to_null"" is set as true)
+  /** The cast functions that need to be replaced (if
-  private static Map<String, String> CAST_FUNC_REPLACEMENT_FROM_NONNULLABLE = new HashMap<>(); // Map from the replaced functions to the new ones (for non-nullable varchar)
+   * "drill.exec.functions.cast_empty_string_to_null" is set to true). */
-  private static Map<String, String> CAST_FUNC_REPLACEMENT_FROM_NULLABLE = new HashMap<>();    // Map from the replaced functions to the new ones (for nullable varchar)
+  private static Set<String> CAST_FUNC_REPLACEMENT_NEEDED = new HashSet<>();
+  /** Map from the replaced functions to the new ones (for non-nullable VARCHAR). */
+  private static Map<String, String> CAST_FUNC_REPLACEMENT_FROM_NONNULLABLE = new HashMap<>();
+  /** Map from the replaced functions to the new ones (for nullable VARCHAR). */
+  private static Map<String, String> CAST_FUNC_REPLACEMENT_FROM_NULLABLE = new HashMap<>();
 
   static {
     TYPE2FUNC.put(MinorType.BIGINT, "castBIGINT");

diff --git a/common/src/main/java/org/apache/drill/common/logical/data/Order.java b/common/src/main/java/org/apache/drill/common/logical/data/Order.java
@@ -20,6 +20,7 @@
 import java.util.Iterator;
 import java.util.List;
 
+import org.apache.drill.common.exceptions.DrillRuntimeException;
 import org.apache.drill.common.expression.FieldReference;
 import org.apache.drill.common.expression.LogicalExpression;
 import org.apache.drill.common.logical.data.visitors.LogicalVisitor;
@@ -65,29 +66,98 @@ public Iterator<LogicalOperator> iterator() {
     }
 
 
-    public static class Ordering {
+  /**
+   * Representation of a SQL &lt;sort specification>.
+   */
+  public static class Ordering {
 
-    private final RelFieldCollation.Direction direction;
     private final LogicalExpression expr;
-    private final RelFieldCollation.NullDirection nulls;
+    /** Net &lt;ordering specification>. */
-
+    private final Direction direction;
+    /** Net &lt;null ordering> */
+    private final NullDirection nullOrdering;
+
+    /**
+     * Constructs a sort specification.
+     * @param  expr  ...
+     * @param  strOrderingSpec  the &lt;ordering specification> as string;
+     *             allowed values: {@code "ASC"}, {@code "DESC"}, {@code null};
+     *             null specifies default &lt;ordering specification>
+     *                   ({@code "ASC"} / {@link Direction#ASCENDING})
+     * @param  strNullOrdering   the &lt;null ordering> as string;
+     *             allowed values: {@code "FIRST"}, {@code "LAST"},
+     *             {@code "UNSPECIFIED"}, {@code null};
+     *             null specifies default &lt;null ordering>
+     *             (omitted / {@link NullDirection#UNSPECIFIED}, interpreted later)
+     */
     @JsonCreator
-    public Ordering(@JsonProperty("order") String strOrder, @JsonProperty("expr") LogicalExpression expr, @JsonProperty("nullDirection") String nullCollation) {
+    public Ordering( @JsonProperty("expr") LogicalExpression expr,
+                     @JsonProperty("order") String strOrderingSpec,
+                     @JsonProperty("nullDirection") String strNullOrdering ) {
       this.expr = expr;
-      this.nulls = NullDirection.LAST.name().equalsIgnoreCase(nullCollation) ? NullDirection.LAST :  NullDirection.FIRST; // default first
+      this.direction = getOrderingSpecFromString( strOrderingSpec );
-      this.direction = Order.getDirectionFromString(strOrder);
+      this.nullOrdering = getNullOrderingFromString( strNullOrdering );
     }
 
-    public Ordering(Direction direction, LogicalExpression e, NullDirection nullCollation) {
+    public Ordering(Direction direction, LogicalExpression e, NullDirection nullOrdering) {
       this.expr = e;
-      this.nulls = nullCollation;
       this.direction = direction;
+      this.nullOrdering = nullOrdering;
     }
 
     public Ordering(Direction direction, LogicalExpression e) {
       this(direction, e, NullDirection.FIRST);
     }
 
+    private static Direction getOrderingSpecFromString( String strDirection ) {
+      final Direction direction;
+      if ( null == strDirection
+          || Direction.ASCENDING.shortString.equals( strDirection ) ) {
+        direction = Direction.ASCENDING;
+      }
+      else if ( Direction.DESCENDING.shortString.equals( strDirection ) ) {
+        direction = Direction.DESCENDING;
+      }
+      else {
+        throw new DrillRuntimeException(
+            "Unknown <ordering specification> string (not \"ASC\", \"DESC\", "
+            + "or null): \"" + strDirection + "\"" );
+      }
+      return direction;
+    }
+
+    private static NullDirection getNullOrderingFromString( String strNullOrdering ) {
+      final RelFieldCollation.NullDirection nullOrdering;
+      if ( null == strNullOrdering ) {
+        nullOrdering = NullDirection.UNSPECIFIED;
+      }
+      else {
+        try {
+          nullOrdering = NullDirection.valueOf( strNullOrdering );
+        }
+        catch ( IllegalArgumentException e ) {
+          throw new DrillRuntimeException(
+              "Internal error:  Unknown <null ordering> string (not "
+              + "\"" + NullDirection.FIRST.name() + "\", "
+              + "\"" + NullDirection.LAST.name() + "\", or "
+              + "\"" + NullDirection.UNSPECIFIED.name() + "\" or null): "
+              + "\"" + strNullOrdering + "\"" );
+        }
+      }
+      return nullOrdering;
+   }
+
+    @Override
+    public String toString() {
+      return
+          super.toString()
+          + "[ "
+          + " expr = " + expr
+          + ", direction = " + direction
+          + ", nullOrdering = " + nullOrdering
+          + "] ";
+    }
+
     @JsonIgnore
     public Direction getDirection() {
       return direction;
@@ -98,18 +168,59 @@ public LogicalExpression getExpr() {
     }
 
     public String getOrder() {
-
+      switch (direction) {
-      switch(direction){
+      case ASCENDING:
-      case DESCENDING: return "DESC";
+        return Direction.ASCENDING.shortString;
-      default: return "ASC";
+      case DESCENDING:
+        return Direction.DESCENDING.shortString;
+      default:
+        throw new DrillRuntimeException(
+            "Unexpected " + Direction.class.getName() + " value other than "
+            + Direction.ASCENDING + " or " + Direction.DESCENDING + ": "
+            + direction );
       }
     }
 
     public NullDirection getNullDirection() {
-      return nulls;
+      return nullOrdering;
     }
 
+    /**
+     * Reports whether NULL sorts high or low in this ordering.
+     *
+     * @return
+     * {@code true}  if NULL sorts higher than any other value;
+     * {@code false} if NULL sorts lower  than any other value
+     */
+    public boolean nullsSortHigh() {
+      final boolean nullsHigh;
+
+      switch (nullOrdering) {
+
+      case UNSPECIFIED:
+        // Default:  NULL sorts high: like NULLS LAST if ASC, FIRST if DESC.
+        nullsHigh = true;
+        break;
+
+      case FIRST:
+        // FIRST: NULL sorts low with ASC, high with DESC.
+        nullsHigh = Direction.DESCENDING == getDirection();
+        break;
+
+      case LAST:
+        // LAST: NULL sorts high with ASC, low with DESC.
+        nullsHigh = Direction.ASCENDING == getDirection();
+        break;
+
+      default:
+        throw new DrillRuntimeException(
+            "Unexpected " + NullDirection.class.getName() + " value other than "
+            + NullDirection.FIRST + ", " + NullDirection.LAST + " or " + NullDirection.UNSPECIFIED + ": "
+            + nullOrdering );
+      }
 
+      return nullsHigh;
+    }
 
   }
 
@@ -138,12 +249,4 @@ public Order internalBuild() {
 
 
   }
-
-  public static Direction getDirectionFromString(String direction){
-    return "DESC".equalsIgnoreCase(direction) ? Direction.DESCENDING : Direction.ASCENDING;
-  }
-
-  public static String getStringFromDirection(Direction direction){
-    return direction == Direction.DESCENDING ? "DESC" : "ASC";
-  }
 }
diff --git a/common/src/test/java/org/apache/drill/common/logical/data/OrderTest.java b/common/src/test/java/org/apache/drill/common/logical/data/OrderTest.java
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.common.logical.data;
+
+import static org.junit.Assert.*;
+
+import java.sql.SQLException;
+
+import org.apache.drill.common.exceptions.DrillRuntimeException;
+import org.apache.drill.common.expression.LogicalExpression;
+import org.apache.drill.common.logical.data.Order.Ordering;
+import org.eigenbase.rel.RelFieldCollation;
+import org.eigenbase.rel.RelFieldCollation.Direction;
+import org.eigenbase.rel.RelFieldCollation.NullDirection;
+import org.junit.Test;
+
+import static org.hamcrest.CoreMatchers.*;
+
+public class OrderTest {
+
+  //////////
+  // Order.Ordering tests:
+
+  // "Round trip" tests that strings from output work as input:
+
+  @Test
+  public void test_Ordering_roundTripAscAndNullsFirst() {
+    Ordering src = new Ordering( Direction.ASCENDING, null, NullDirection.FIRST);
+    Ordering reconstituted =
+        new Ordering( (LogicalExpression) null, src.getOrder(), src.getNullDirection().toString() );
+    assertThat( reconstituted.getDirection(), equalTo( RelFieldCollation.Direction.ASCENDING  ) );
+    assertThat( reconstituted.getNullDirection(), equalTo( NullDirection.FIRST  ) );
+  }
+
+  @Test
+  public void test_Ordering_roundTripDescAndNullsLast() {
+    Ordering src = new Ordering( Direction.DESCENDING, null, NullDirection.LAST);
+    Ordering reconstituted =
+        new Ordering( (LogicalExpression) null, src.getOrder(), src.getNullDirection().toString() );
+    assertThat( reconstituted.getDirection(), equalTo( RelFieldCollation.Direction.DESCENDING  ) );
+    assertThat( reconstituted.getNullDirection(), equalTo( NullDirection.LAST  ) );
+  }
+
+  @Test
+  public void test_Ordering_roundTripDescAndNullsUnspecified() {
+    Ordering src = new Ordering( Direction.DESCENDING, null, NullDirection.UNSPECIFIED);
+    Ordering reconstituted =
+        new Ordering( (LogicalExpression) null, src.getOrder(), src.getNullDirection().toString() );
+    assertThat( reconstituted.getDirection(), equalTo( RelFieldCollation.Direction.DESCENDING  ) );
+    assertThat( reconstituted.getNullDirection(), equalTo( NullDirection.UNSPECIFIED  ) );
+  }
+
+
+  // Basic input validation:
+
+  @Test( expected = DrillRuntimeException.class )  // (Currently.)
+  public void test_Ordering_garbageOrderRejected() {
+    new Ordering( (LogicalExpression) null, "AS CE ND IN G", (String) null );
+  }
+
+  @Test( expected = DrillRuntimeException.class )  // (Currently.)
+  public void test_Ordering_garbageNullOrderingRejected() {
+    new Ordering( (LogicalExpression) null, (String) null, "HIGH" );
+  }
+
+
+  // Defaults-value/null-strings test:
+
+  @Test
+  public void testOrdering_nullStrings() {
+    Ordering ordering = new Ordering( (LogicalExpression) null, null, null );
+    assertThat( ordering.getDirection(), equalTo( RelFieldCollation.Direction.ASCENDING ) );
+    assertThat( ordering.getNullDirection(), equalTo( RelFieldCollation.NullDirection.UNSPECIFIED ) );
+    assertThat( ordering.getOrder(), equalTo( "ASC" ) );
+  }
+
+
+}
diff --git a/contrib/storage-hive/core/src/test/java/org/apache/drill/exec/TestHiveProjectPushDown.java b/contrib/storage-hive/core/src/test/java/org/apache/drill/exec/TestHiveProjectPushDown.java
@@ -27,7 +27,7 @@ private void testHelper(String query, String expectedColNamesInPlan, int expecte
     testPhysicalPlan(query, expectedColNamesInPlan);
 
     int actualRecordCount = testSql(query);
-    assertEquals(String.format("Received unexepcted number of rows in output: expected=%d, received=%s",
+    assertEquals(String.format("Received unexpected number of rows in output: expected=%d, received=%s",
         expectedRecordCount, actualRecordCount), expectedRecordCount, actualRecordCount);
   }
 

diff --git a/exec/java-exec/src/main/codegen/config.fmpp b/exec/java-exec/src/main/codegen/config.fmpp
@@ -15,29 +15,34 @@
 # limitations under the License.
 
 data: {
-    vv: tdd(../data/ValueVectorTypes.tdd),
+    vv:                       tdd(../data/ValueVectorTypes.tdd),
-    compare: tdd(../data/CompareTypes.tdd),
+
-    cast: tdd(../data/Casts.tdd),
+    # Most types for comparison functions (for templates/ComparisonFunctions.java).
-    MathFunctionTypes: tdd(../data/MathFunctionTypes.tdd),
+    comparisonTypesMain:      tdd(../data/ComparisonTypesMain.tdd),
-    mathFunc:tdd(../data/MathFunc.tdd),
+
-    aggrtypes1: tdd(../data/AggrTypes1.tdd),
+    # Decimal types for comparison function (for
-    decimalaggrtypes1: tdd(../data/DecimalAggrTypes1.tdd),
+    # templates/DecimalFunctions/ComparisonFunctions.java).
-    decimalaggrtypes2: tdd(../data/DecimalAggrTypes2.tdd),
+    comparisonTypesDecimal:   tdd(../data/ComparisonTypesDecimal.tdd),
-    aggrtypes2: tdd(../data/AggrTypes2.tdd),
+
-    aggrtypes3: tdd(../data/AggrTypes3.tdd),
+    cast:                     tdd(../data/Casts.tdd),
-    covarTypes: tdd(../data/CovarTypes.tdd),
+    MathFunctionTypes:        tdd(../data/MathFunctionTypes.tdd),
-    corrTypes: tdd(../data/CorrelationTypes.tdd),
+    mathFunc:                 tdd(../data/MathFunc.tdd),
-    logicalTypes: tdd(../data/AggrBitwiseLogicalTypes.tdd),
+    aggrtypes1:               tdd(../data/AggrTypes1.tdd),
-    date: tdd(../data/DateTypes.tdd),
+    decimalaggrtypes1:        tdd(../data/DecimalAggrTypes1.tdd),
-    extract: tdd(../data/ExtractTypes.tdd),
+    decimalaggrtypes2:        tdd(../data/DecimalAggrTypes2.tdd),
-    parser: tdd(../data/Parser.tdd),
+    aggrtypes2:               tdd(../data/AggrTypes2.tdd),
-    decimal: tdd(../data/DecimalTypes.tdd),
+    aggrtypes3:               tdd(../data/AggrTypes3.tdd),
-    dateIntervalFunc: tdd(../data/DateIntervalFunc.tdd),
+    covarTypes:               tdd(../data/CovarTypes.tdd),
-    intervalNumericTypes: tdd(../data/IntervalNumericTypes.tdd),
+    corrTypes:                tdd(../data/CorrelationTypes.tdd),
-    extract: tdd(../data/ExtractTypes.tdd),
+    logicalTypes:             tdd(../data/AggrBitwiseLogicalTypes.tdd),
-    sumzero: tdd(../data/SumZero.tdd),
+    extract:                  tdd(../data/ExtractTypes.tdd),
-    numericTypes: tdd(../data/NumericTypes.tdd),
+    parser:                   tdd(../data/Parser.tdd),
-    casthigh: tdd(../data/CastHigh.tdd)
+    dateIntervalFunc:         tdd(../data/DateIntervalFunc.tdd),
+    intervalNumericTypes:     tdd(../data/IntervalNumericTypes.tdd),
+    extract:                  tdd(../data/ExtractTypes.tdd),
+    sumzero:                  tdd(../data/SumZero.tdd),
+    numericTypes:             tdd(../data/NumericTypes.tdd),
+    casthigh:                 tdd(../data/CastHigh.tdd)
 }
 freemarkerLinks: {
     includes: includes/