You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@drill.apache.org by js...@apache.org on 2016/04/20 18:08:54 UTC
[2/4] drill git commit: DRILL-4448: Clean up deserialization of
oderings in sorts
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 <al...@gmail.com>
Authored: Wed Feb 24 19:37:21 2016 -0800
Committer: Jason Altekruse <al...@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 <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" ) );