[GitHub] [iceberg] clesaec commented on a diff in pull request #6893: Parquet: Make row-group filters cooperate to filter

["clesaec (via GitHub)" <gi...@apache.org>]

clesaec commented on code in PR #6893:
URL: https://github.com/apache/iceberg/pull/6893#discussion_r1113984216


##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetBloomRowGroupFilter.java:
##########
@@ -116,89 +124,99 @@ private boolean eval(
       // If the filter's column set doesn't overlap with any bloom filter columns, exit early with
       // ROWS_MIGHT_MATCH
       if (filterRefs.size() > 0 && Sets.intersection(fieldsWithBloomFilter, filterRefs).isEmpty()) {
-        return ROWS_MIGHT_MATCH;
+        return expr;
       }
 
-      return ExpressionVisitors.visitEvaluator(expr, this);
+      return ExpressionVisitors.visit(expr, this);
     }
 
     @Override
-    public Boolean alwaysTrue() {
-      return ROWS_MIGHT_MATCH; // all rows match
+    public Expression alwaysTrue() {
+      return ROWS_ALL_MATCH; // all rows match
     }
 
     @Override
-    public Boolean alwaysFalse() {
+    public Expression alwaysFalse() {
       return ROWS_CANNOT_MATCH; // all rows fail
     }
 
     @Override
-    public Boolean not(Boolean result) {
+    public Expression not(Expression result) {
       // not() should be rewritten by RewriteNot
       // bloom filter is based on hash and cannot eliminate based on not
       throw new UnsupportedOperationException("This path shouldn't be reached.");
     }
 
     @Override
-    public Boolean and(Boolean leftResult, Boolean rightResult) {
-      return leftResult && rightResult;
+    public Expression and(Supplier<Expression> left, Supplier<Expression> right) {
+      Expression leftResult = left.get();

Review Comment:
   As on methods "and" & "or", you always evaluate, at least left expression, it's like using Suppliers is not relevant here; and just having
   `BloomEvalVisitor extends BoundExpressionVisitor<Boolean>` to 
   `BloomEvalVisitor extends BoundExpressionVisitor<Expression>` would be enough to delay expression evaluation itself.
   (or just have `public abstract static class FindsResidualVisitor extends BoundExpressionVisitor<Expression>`)
   WDYT ?



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