[GitHub] [iceberg] rdblue commented on a diff in pull request #6524: Spark 3.3: Discard filters that can be pushed down completely

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

rdblue commented on code in PR #6524:
URL: https://github.com/apache/iceberg/pull/6524#discussion_r1064180862


##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/source/SparkScanBuilder.java:
##########
@@ -106,41 +106,41 @@ public SparkScanBuilder caseSensitive(boolean isCaseSensitive) {
   @Override
   public Filter[] pushFilters(Filter[] filters) {
     List<Expression> expressions = Lists.newArrayListWithExpectedSize(filters.length);
-    List<Filter> pushed = Lists.newArrayListWithExpectedSize(filters.length);
+    List<Filter> pushableFilters = Lists.newArrayListWithExpectedSize(filters.length);
+    List<Filter> postScanFilters = Lists.newArrayListWithExpectedSize(filters.length);
 
     for (Filter filter : filters) {
-      Expression expr = null;
       try {
-        expr = SparkFilters.convert(filter);
-      } catch (IllegalArgumentException e) {
-        // converting to Iceberg Expression failed, so this expression cannot be pushed down
-        LOG.info(
-            "Failed to convert filter to Iceberg expression, skipping push down for this expression: {}. {}",
-            filter,
-            e.getMessage());
-      }
+        Expression expr = SparkFilters.convert(filter);
 
-      if (expr != null) {
-        try {
+        if (expr != null) {
+          // try binding the expression to ensure it can be pushed down
           Binder.bind(schema.asStruct(), expr, caseSensitive);
+
           expressions.add(expr);
-          pushed.add(filter);
-        } catch (ValidationException e) {
-          // binding to the table schema failed, so this expression cannot be pushed down
-          LOG.info(
-              "Failed to bind expression to table schema, skipping push down for this expression: {}. {}",
-              filter,
-              e.getMessage());
+          pushableFilters.add(filter);
+        }
+
+        if (expr == null || requiresRecordLevelFiltering(expr)) {
+          postScanFilters.add(filter);
         }
+      } catch (Exception e) {
+        LOG.warn("Failed to check if {} can be pushed down: {}", filter, e.getMessage());

Review Comment:
   I would prefer to be more specific about the error here. Logging the `ValidationError` at INFO is correct because it's okay for some filters to not be convertible -- we want to catch those and support them in the future, but there is no problem for the job. I'm not sure what other errors filter through here now and whether WARN is appropriate for those. But I'd prefer to have them broken out more specifically.



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