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

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

kbendick commented on a change in pull request #2062:
URL: https://github.com/apache/iceberg/pull/2062#discussion_r762686988



##########
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 generally we try to touch as few lines as needed in some cases (or avoid unnecessary changes), as that makes it easier on people who maintain forks.
   
   I don't disagree with you that the error message should be more descriptive, but given what I know about the API, I don't think these specific base class for `BoundVisitor` actually gets called. Notice how many of them return `null` as well.
   
   Here, it's just to be consistent with the existing one. If other's feel like it should be changed, I'm happy to do so, but I think given the size of the PR it's best to keep unrelated changes (particularly updating `startsWith`) in a separate PR.




-- 
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