[GitHub] [iceberg] cccs-eric commented on a change in pull request #2062: [CORE] Add in a NOT_STARTS_WITH operator with filter pushdown

[GitBox <gi...@apache.org>]

cccs-eric commented on a change in pull request #2062:
URL: https://github.com/apache/iceberg/pull/2062#discussion_r762639059



##########
File path: api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java
##########
@@ -220,6 +222,27 @@ public void testNotEqual() {
 
   }
 
+  @Test
+  public void testStartsWith() {
+    StructType struct = StructType.of(required(24, "s", Types.StringType.get()));
+    Evaluator evaluator = new Evaluator(struct, startsWith("s", "abc"));
+    Assert.assertTrue("abc startsWith abc should be true", evaluator.eval(TestHelpers.Row.of("abc")));

Review comment:
       Should we add a test with the pattern present, but not a the start?
   ```suggestion
       Assert.assertTrue("abc startsWith abc should be true", evaluator.eval(TestHelpers.Row.of("abc")));
       Assert.assertFalse("xabc startsWith abc should be false", evaluator.eval(TestHelpers.Row.of("xabc")));
   ```

##########
File path: api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java
##########
@@ -220,6 +222,27 @@ public void testNotEqual() {
 
   }
 
+  @Test
+  public void testStartsWith() {
+    StructType struct = StructType.of(required(24, "s", Types.StringType.get()));
+    Evaluator evaluator = new Evaluator(struct, startsWith("s", "abc"));
+    Assert.assertTrue("abc startsWith abc should be true", evaluator.eval(TestHelpers.Row.of("abc")));
+    Assert.assertFalse("Abc startsWith abc should be false", evaluator.eval(TestHelpers.Row.of("Abc")));
+    Assert.assertFalse("a startsWith abc should be false", evaluator.eval(TestHelpers.Row.of("a")));
+    Assert.assertTrue("abcd startsWith abc should be true", evaluator.eval(TestHelpers.Row.of("abcd")));
+  }
+
+  @Test
+  public void testNotStartsWith() {
+    StructType struct = StructType.of(required(24, "s", Types.StringType.get()));
+    Evaluator evaluator = new Evaluator(struct, notStartsWith("s", "abc"));
+    Assert.assertFalse("abc notStartsWith abc should be false", evaluator.eval(TestHelpers.Row.of("abc")));

Review comment:
       Should we add a test with the pattern present, but not a the start?
   ```suggestion
       Assert.assertFalse("abc notStartsWith abc should be false", evaluator.eval(TestHelpers.Row.of("abc")));
       Assert.assertTrue("xabc notStartsWith abc should be true", evaluator.eval(TestHelpers.Row.of("xabc")));
   ```

##########
File path: api/src/main/java/org/apache/iceberg/expressions/ExpressionVisitors.java
##########
@@ -242,6 +248,10 @@ public R or(R leftResult, R rightResult) {
       throw new UnsupportedOperationException("Unsupported operation.");
     }
 
+    public <T> R notStartsWith(Bound<T> expr, Literal<T> lit) {
+      throw new UnsupportedOperationException("Unsupported operation.");

Review comment:
       I think the exception message should specify which operation is not supported.   `startsWith` should also be modified.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org