Address Review Comments

apache · Aug 7, 2019 · 61b0ef9 · 61b0ef9
1 parent 3180cec
commit 61b0ef9
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 27 deletions.
diff --git a/api/src/main/java/org/apache/iceberg/transforms/Truncate.java b/api/src/main/java/org/apache/iceberg/transforms/Truncate.java
@@ -249,11 +249,6 @@ public UnboundPredicate<CharSequence> project(String name,
         case IS_NULL:
           return Expressions.predicate(predicate.op(), name);
         case STARTS_WITH:
-          if (predicate.literal().value().length() <= width()) {
-            return Expressions.predicate(predicate.op(), name, predicate.literal().value());
-          } else {
-            return ProjectionUtil.truncateArray(name, predicate, this);
-          }
         default:
           return ProjectionUtil.truncateArray(name, predicate, this);
       }
@@ -264,8 +259,10 @@ public UnboundPredicate<CharSequence> projectStrict(String name,
                                                         BoundPredicate<CharSequence> predicate) {
       switch (predicate.op()) {
         case STARTS_WITH:
-          if (predicate.literal().value().length() <= width()) {
+          if (predicate.literal().value().length() < width()) {
             return Expressions.predicate(predicate.op(), name, predicate.literal().value());
+          } else if (predicate.literal().value().length() == width()) {
+            return Expressions.equal(name, predicate.literal().value());
           } else {
             return null;
           }

diff --git a/api/src/test/java/org/apache/iceberg/transforms/TestStartsWith.java b/api/src/test/java/org/apache/iceberg/transforms/TestStartsWith.java
@@ -26,6 +26,7 @@
 import org.apache.iceberg.expressions.BoundPredicate;
 import org.apache.iceberg.expressions.Evaluator;
 import org.apache.iceberg.expressions.Expression;
+import org.apache.iceberg.expressions.False;
 import org.apache.iceberg.expressions.Literal;
 import org.apache.iceberg.expressions.Projections;
 import org.apache.iceberg.expressions.UnboundPredicate;
@@ -36,58 +37,63 @@
 import static org.apache.iceberg.TestHelpers.assertAndUnwrapUnbound;
 import static org.apache.iceberg.expressions.Expressions.startsWith;
 import static org.apache.iceberg.types.Types.NestedField.optional;
-import static org.apache.iceberg.types.Types.NestedField.required;
 
 public class TestStartsWith {
 
-  private static final Schema SCHEMA = new Schema(optional(1, "someStringCol", Types.StringType.get()));
+  private static final String COLUMN = "someStringCol";
+  private static final Schema SCHEMA = new Schema(optional(1, COLUMN, Types.StringType.get()));
 
   @Test
   public void assertTruncateProjections() {
-    PartitionSpec spec = PartitionSpec.builderFor(SCHEMA).truncate("someStringCol", 4).build();
+    PartitionSpec spec = PartitionSpec.builderFor(SCHEMA).truncate(COLUMN, 4).build();
 
-    assertProjectionInclusive(spec, startsWith("someStringCol", "ab"), "ab");
-    assertProjectionInclusive(spec, startsWith("someStringCol", "abab"), "abab");
-    assertProjectionInclusive(spec, startsWith("someStringCol", "ababab"), "abab");
+    assertProjectionInclusive(spec, startsWith(COLUMN, "ab"), "ab", Expression.Operation.STARTS_WITH);
+    assertProjectionInclusive(spec, startsWith(COLUMN, "abab"), "abab", Expression.Operation.STARTS_WITH);
+    assertProjectionInclusive(spec, startsWith(COLUMN, "ababab"), "abab", Expression.Operation.STARTS_WITH);
 
-    assertProjectionStrict(spec, startsWith("someStringCol", "ab"), "ab");
-    assertProjectionStrict(spec, startsWith("someStringCol", "abab"), "abab");
+    assertProjectionStrict(spec, startsWith(COLUMN, "ab"), "ab", Expression.Operation.STARTS_WITH);
+    assertProjectionStrict(spec, startsWith(COLUMN, "abab"), "abab", Expression.Operation.EQ);
+    assertProjectionFalse(spec, startsWith(COLUMN, "ababab"));
   }
 
   @Test
   public void assertTruncateString() {
-    Types.StructType struct = Types.StructType.of(required(0, "s", Types.StringType.get()));
     Truncate<String> trunc = Truncate.get(Types.StringType.get(), 2);
-    Expression expr = startsWith("s", "abcde");
-    BoundPredicate<String> boundExpr = (BoundPredicate<String>) Binder.bind(struct,  expr, false);
+    Expression expr = startsWith(COLUMN, "abcde");
+    BoundPredicate<String> boundExpr = (BoundPredicate<String>) Binder.bind(SCHEMA.asStruct(),  expr, false);
 
-    UnboundPredicate<String> projected = trunc.project("s", boundExpr);
-    Evaluator evaluator = new Evaluator(struct, projected);
+    UnboundPredicate<String> projected = trunc.project(COLUMN, boundExpr);
+    Evaluator evaluator = new Evaluator(SCHEMA.asStruct(), projected);
 
     Assert.assertTrue("startsWith(abcde, truncate(abcde,2))  => true",
         evaluator.eval(TestHelpers.Row.of("abcde")));
   }
 
   private void assertProjectionInclusive(PartitionSpec spec, UnboundPredicate<?> filter,
-                                         String expectedLiteral) {
+                                         String expectedLiteral, Expression.Operation expectedOp) {
     Expression projection = Projections.inclusive(spec).project(filter);
-    assertProjection(spec, expectedLiteral, projection);
+    assertProjection(spec, expectedLiteral, projection, expectedOp);
   }
 
   private void assertProjectionStrict(PartitionSpec spec, UnboundPredicate<?> filter,
-                                         String expectedLiteral) {
+                                         String expectedLiteral, Expression.Operation expectedOp) {
     Expression projection = Projections.strict(spec).project(filter);
-    assertProjection(spec, expectedLiteral, projection);
+    assertProjection(spec, expectedLiteral, projection, expectedOp);
   }
 
-  private void assertProjection(PartitionSpec spec, String expectedLiteral, Expression projection) {
+  private void assertProjection(PartitionSpec spec, String expectedLiteral, Expression projection,
+                                Expression.Operation expectedOp) {
     UnboundPredicate<?> predicate = assertAndUnwrapUnbound(projection);
-
-    Assert.assertEquals(predicate.op(), Expression.Operation.STARTS_WITH);
-
     Literal literal = predicate.literal();
     Truncate<CharSequence> transform = (Truncate<CharSequence>) spec.getFieldsBySourceId(1).get(0).transform();
     String output = transform.toHumanString((String) literal.value());
+
+    Assert.assertEquals(expectedOp, predicate.op());
     Assert.assertEquals(expectedLiteral, output);
   }
+
+  private void assertProjectionFalse(PartitionSpec spec, UnboundPredicate<?> filter) {
+    Expression projection = Projections.strict(spec).project(filter);
+    Assert.assertTrue(projection instanceof False);
+  }
 }