drill-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From j...@apache.org
Subject [2/4] drill git commit: DRILL-4448: Clean up deserialization of oderings in sorts
Date Wed, 20 Apr 2016 16:08:54 GMT
DRILL-4448: Clean up deserialization of oderings in sorts

Fix sort operator deserialization and validation to respect existing
contract specified in the tests.


Project: http://git-wip-us.apache.org/repos/asf/drill/repo
Commit: http://git-wip-us.apache.org/repos/asf/drill/commit/1d1acc09
Tree: http://git-wip-us.apache.org/repos/asf/drill/tree/1d1acc09
Diff: http://git-wip-us.apache.org/repos/asf/drill/diff/1d1acc09

Branch: refs/heads/master
Commit: 1d1acc09ec30167f0653d99cee6f30c7b1413859
Parents: d24205d
Author: Jason Altekruse <altekrusejason@gmail.com>
Authored: Wed Feb 24 19:37:21 2016 -0800
Committer: Jason Altekruse <altekrusejason@gmail.com>
Committed: Wed Apr 20 08:10:40 2016 -0700

----------------------------------------------------------------------
 .../apache/drill/common/logical/data/Order.java | 107 ++++++++++++++-----
 .../drill/common/logical/data/OrderTest.java    |  14 ++-
 2 files changed, 85 insertions(+), 36 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/drill/blob/1d1acc09/logical/src/main/java/org/apache/drill/common/logical/data/Order.java
----------------------------------------------------------------------
diff --git a/logical/src/main/java/org/apache/drill/common/logical/data/Order.java b/logical/src/main/java/org/apache/drill/common/logical/data/Order.java
index ca3eec4..1bf587d 100644
--- a/logical/src/main/java/org/apache/drill/common/logical/data/Order.java
+++ b/logical/src/main/java/org/apache/drill/common/logical/data/Order.java
@@ -19,7 +19,10 @@ package org.apache.drill.common.logical.data;
 
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 
+import com.google.common.collect.ImmutableMap;
+import org.apache.calcite.util.PartiallyOrderedSet;
 import org.apache.drill.common.exceptions.DrillRuntimeException;
 import org.apache.drill.common.expression.FieldReference;
 import org.apache.drill.common.expression.LogicalExpression;
@@ -76,6 +79,17 @@ public class Order extends SingleInputOperator {
     private final Direction direction;
     /** Net &lt;null ordering> */
     private final NullDirection nullOrdering;
+    /** The values in the plans for ordering specification are ASC, DESC, not the
+     * full words featured in the calcite {@link Direction} Enum, need to map between them.
*/
+    private static ImmutableMap<String, Direction> DRILL_TO_CALCITE_DIR_MAPPING =
+        ImmutableMap.<String, Direction>builder()
+        .put("ASC", Direction.ASCENDING)
+        .put("DESC", Direction.DESCENDING).build();
+    private static ImmutableMap<String, NullDirection> DRILL_TO_CALCITE_NULL_DIR_MAPPING
=
+        ImmutableMap.<String, NullDirection>builder()
+            .put("FIRST", NullDirection.FIRST)
+            .put("LAST", NullDirection.LAST)
+            .put("UNSPECIFIED", NullDirection.UNSPECIFIED).build();
 
     /**
      * Constructs a sort specification.
@@ -91,17 +105,17 @@ public class Order extends SingleInputOperator {
      *             (omitted / {@link NullDirection#UNSPECIFIED}, interpreted later)
      */
     @JsonCreator
-    public Ordering( @JsonProperty("expr") LogicalExpression expr,
-                     @JsonProperty("order") String strOrderingSpec,
+    public Ordering( @JsonProperty("order") String strOrderingSpec,
+                     @JsonProperty("expr") LogicalExpression expr,
                      @JsonProperty("nullDirection") String strNullOrdering ) {
       this.expr = expr;
-      this.direction = getOrderingSpecFromString( strOrderingSpec );
-      this.nullOrdering = getNullOrderingFromString( strNullOrdering );
+      this.direction = getOrderingSpecFromString(strOrderingSpec);
+      this.nullOrdering = getNullOrderingFromString(strNullOrdering);
     }
 
     public Ordering(Direction direction, LogicalExpression e, NullDirection nullOrdering)
{
       this.expr = e;
-      this.direction = direction;
+      this.direction = filterDrillSupportedDirections(direction);
       this.nullOrdering = nullOrdering;
     }
 
@@ -110,41 +124,78 @@ public class Order extends SingleInputOperator {
     }
 
     private static Direction getOrderingSpecFromString( String strDirection ) {
-      final Direction direction;
-      if ( null == strDirection
-          || Direction.ASCENDING.shortString.equals( strDirection ) ) {
-        direction = Direction.ASCENDING;
+      Direction dir = DRILL_TO_CALCITE_DIR_MAPPING.get(strDirection);
+      if (dir != null || strDirection == null) {
+        return filterDrillSupportedDirections(dir);
+      } else {
+        throw new DrillRuntimeException(
+            "Unknown <ordering specification> string (not \"ASC\", \"DESC\", "
+                + "or null): \"" + strDirection + "\"" );
+      }
+    }
+
+    private static NullDirection getNullOrderingFromString( String strNullOrdering ) {
+      NullDirection nullDir = DRILL_TO_CALCITE_NULL_DIR_MAPPING.get(strNullOrdering);
+      if (nullDir != null || strNullOrdering == null) {
+        return filterDrillSupportedNullDirections(nullDir);
+      } else {
+        throw new DrillRuntimeException(
+            "Internal error:  Unknown <null ordering> string (not "
+                + "\"" + NullDirection.FIRST.name() + "\", "
+                + "\"" + NullDirection.LAST.name() + "\", or "
+                + "\"" + NullDirection.UNSPECIFIED.name() + "\" or null): "
+                + "\"" + strNullOrdering + "\"" );
+      }
+    }
+
+    /**
+     * Disallows unsupported values for ordering direction (provided by Calcite but not implemented
by Drill)
+     *
+     * Provides a default value of ASCENDING in the case of a null.
+     *
+     * @param direction
+     * @return - a sanitized direction value
+     */
+    private static Direction filterDrillSupportedDirections(Direction direction) {
+      if (direction == null || direction == Direction.ASCENDING) {
+        return Direction.ASCENDING;
       }
-      else if ( Direction.DESCENDING.shortString.equals( strDirection ) ) {
-        direction = Direction.DESCENDING;
+      else if (Direction.DESCENDING.equals( direction) ) {
+        return direction;
       }
       else {
         throw new DrillRuntimeException(
             "Unknown <ordering specification> string (not \"ASC\", \"DESC\", "
-            + "or null): \"" + strDirection + "\"" );
+            + "or null): \"" + direction + "\"" );
       }
-      return direction;
     }
 
-    private static NullDirection getNullOrderingFromString( String strNullOrdering ) {
-      final RelFieldCollation.NullDirection nullOrdering;
-      if ( null == strNullOrdering ) {
-        nullOrdering = NullDirection.UNSPECIFIED;
+    /**
+     * Disallows unsupported values for null ordering (provided by Calcite but not implemented
by Drill),
+     * currently all values are supported.
+     *
+     * Provides a default value of UNSPECIFIED in the case of a null.
+     *
+     * @param nullDirection
+     * @return - a sanitized direction value
+     */
+    private static NullDirection filterDrillSupportedNullDirections(NullDirection nullDirection)
{
+      if ( null == nullDirection) {
+        return NullDirection.UNSPECIFIED;
       }
-      else {
-        try {
-          nullOrdering = NullDirection.valueOf( strNullOrdering );
-        }
-        catch ( IllegalArgumentException e ) {
+      switch (nullDirection) {
+        case LAST:
+        case FIRST:
+        case UNSPECIFIED:
+          return nullDirection;
+        default:
           throw new DrillRuntimeException(
               "Internal error:  Unknown <null ordering> string (not "
-              + "\"" + NullDirection.FIRST.name() + "\", "
-              + "\"" + NullDirection.LAST.name() + "\", or "
-              + "\"" + NullDirection.UNSPECIFIED.name() + "\" or null): "
-              + "\"" + strNullOrdering + "\"" );
-        }
+                  + "\"" + NullDirection.FIRST.name() + "\", "
+                  + "\"" + NullDirection.LAST.name() + "\", or "
+                  + "\"" + NullDirection.UNSPECIFIED.name() + "\" or null): "
+                  + "\"" + nullDirection + "\"" );
       }
-      return nullOrdering;
    }
 
     @Override

http://git-wip-us.apache.org/repos/asf/drill/blob/1d1acc09/logical/src/test/java/org/apache/drill/common/logical/data/OrderTest.java
----------------------------------------------------------------------
diff --git a/logical/src/test/java/org/apache/drill/common/logical/data/OrderTest.java b/logical/src/test/java/org/apache/drill/common/logical/data/OrderTest.java
index 80b4e3c..9d55742 100644
--- a/logical/src/test/java/org/apache/drill/common/logical/data/OrderTest.java
+++ b/logical/src/test/java/org/apache/drill/common/logical/data/OrderTest.java
@@ -42,7 +42,7 @@ public class OrderTest {
   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()
);
+        new Ordering( src.getDirection(), (LogicalExpression) null, src.getNullDirection()
);
     assertThat( reconstituted.getDirection(), equalTo( RelFieldCollation.Direction.ASCENDING
 ) );
     assertThat( reconstituted.getNullDirection(), equalTo( NullDirection.FIRST  ) );
   }
@@ -51,7 +51,7 @@ public class OrderTest {
   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()
);
+        new Ordering( src.getDirection(), (LogicalExpression) null, src.getNullDirection()
);
     assertThat( reconstituted.getDirection(), equalTo( RelFieldCollation.Direction.DESCENDING
 ) );
     assertThat( reconstituted.getNullDirection(), equalTo( NullDirection.LAST  ) );
   }
@@ -60,22 +60,20 @@ public class OrderTest {
   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()
);
+        new Ordering( src.getDirection(), (LogicalExpression) null, src.getNullDirection()
);
     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 );
+    new Ordering( "AS CE ND IN G", null, null );
   }
 
   @Test( expected = DrillRuntimeException.class )  // (Currently.)
   public void test_Ordering_garbageNullOrderingRejected() {
-    new Ordering( (LogicalExpression) null, (String) null, "HIGH" );
+    new Ordering( null, null, "HIGH" );
   }
 
 
@@ -83,7 +81,7 @@ public class OrderTest {
 
   @Test
   public void testOrdering_nullStrings() {
-    Ordering ordering = new Ordering( (LogicalExpression) null, null, null );
+    Ordering ordering = new Ordering( (String) null, (LogicalExpression) null, null );
     assertThat( ordering.getDirection(), equalTo( RelFieldCollation.Direction.ASCENDING )
);
     assertThat( ordering.getNullDirection(), equalTo( RelFieldCollation.NullDirection.UNSPECIFIED
) );
     assertThat( ordering.getOrder(), equalTo( "ASC" ) );


Mime
View raw message