From 61b0ef92387868732944796dd09e44dafe84a0ac Mon Sep 17 00:00:00 2001 From: Sujith Jay Nair Date: Wed, 7 Aug 2019 20:18:22 +0200 Subject: [PATCH] Address Review Comments --- .../apache/iceberg/transforms/Truncate.java | 9 ++-- .../iceberg/transforms/TestStartsWith.java | 48 +++++++++++-------- 2 files changed, 30 insertions(+), 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 index 9947ca92e4bd..bac6277f14cc 100644 --- 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 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 projectStrict(String name, BoundPredicate 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 index 1ab4fd932adb..ca7de5c9efb1 100644 --- 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 trunc = Truncate.get(Types.StringType.get(), 2); - Expression expr = startsWith("s", "abcde"); - BoundPredicate boundExpr = (BoundPredicate) Binder.bind(struct, expr, false); + Expression expr = startsWith(COLUMN, "abcde"); + BoundPredicate boundExpr = (BoundPredicate) Binder.bind(SCHEMA.asStruct(), expr, false); - UnboundPredicate projected = trunc.project("s", boundExpr); - Evaluator evaluator = new Evaluator(struct, projected); + UnboundPredicate 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 transform = (Truncate) 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); + } }