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 <[email protected]> Authored: Wed Feb 24 19:37:21 2016 -0800 Committer: Jason Altekruse <[email protected]> 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" ) );
