Spark 3.4: Push down system functions by V2 filters for rewriting DataFiles and PositionDeleteFiles #8560

dramaticlly · 2023-09-14T06:28:53Z

Inspired by #7886, try to push down v2 filters in where clause used by both rewriteDataFiles and rewritePositionDeleteFiles

make sure the bucket transform works in the action
Make sure the conversion works in the procedure

CC @RussellSpitzer @aokolnychyi @ConeyLiu @rdblue

Expect following query to work after this change

spark.sql(
"""
CALL iceberg.system.rewrite_data_files(
  table => 'foo.bar', 
  where => 'iceberg.system.bucket(4, url) = 0')
"""
)

…aFiles and PositionDeleteFiles

.../v3.4/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteDataFilesProcedure.java

ConeyLiu · 2023-09-14T13:38:41Z

...ark/src/main/scala/org/apache/spark/sql/execution/datasources/SparkExpressionConverter.scala


 object SparkExpressionConverter {

  def convertToIcebergExpression(sparkExpression: Expression): org.apache.iceberg.expressions.Expression = {
    // Currently, it is a double conversion as we are converting Spark expression to Spark filter
    // and then converting Spark filter to Iceberg expression.
    // But these two conversions already exist and well tested. So, we are going with this approach.
-    SparkFilters.convert(DataSourceStrategy.translateFilter(sparkExpression, supportNestedPredicatePushdown = true).get)
+    SparkV2Filters.convert(DataSourceV2Strategy.translateFilterV2(sparkExpression).get)


Not related to this PR either. The get here indeed is not a good idea, because the expression could fail to translate and the error message is not valuable. I have opened #8394 for it.

Is there anything we have to worry about here in moving from SparkFilters to SparkV2Filters?

thank you @ConeyLiu , I will rebase my change if 8394 gets merged first.
Also added some comments where we are now convert Spark catalyst expression to Predicate instead of spark source filter. I ran all the unit tests to make sure old filter are working as expected.

ConeyLiu · 2023-09-14T13:41:14Z

spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteDataFilesAction.java

@@ -223,6 +223,31 @@ public void testBinPackWithFilter() {
    assertEquals("Rows must match", expectedRecords, actualRecords);
  }

+  @Test
+  public void testBinPackWithFilterOnBucketExpression() {
+    PartitionSpec spec = PartitionSpec.builderFor(SCHEMA).bucket("c3", 2).build();


Is it possible to use the test utils class SystemFunctionPushDownHelper to build the table and data?

Thank you Coney, your test utils class is super helpful. However I realized this SparkAction tests was assumed to use hadoop catalog so the table creation is a bit different as it's by table location https://github.com/apache/iceberg/blob/master/spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteDataFilesAction.java#L1563. But I opted to use your SystemFunctionPushDownHelper in TestRewriteDataFilesProcedure.

ConeyLiu · 2023-09-14T13:45:23Z

...ensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java

@@ -480,7 +511,10 @@ public void testRewriteDataFilesWithAllPossibleFilters() {
    sql(
        "CALL %s.system.rewrite_data_files(table => '%s'," + " where => 'c2 like \"%s\"')",
        catalogName, tableIdent, "car%");
-
+    // StringStartsWith


What's the purpose here? It seems to just copy from L510-L513.

Is this meant to be a bucket transform call?

yeah good catch. Initially I was about to add filter here for bucket transform (forgot to change) but I end up create a new method to test all V2Filters can be evaluated without exception.

...ensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java

ConeyLiu · 2023-09-14T13:50:52Z

Thanks @dramaticlly for this contribution. This is on my working list but happy to see it finished in advance. I have some minor comments

...ensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java

spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteDataFilesAction.java

RussellSpitzer

I just have some minor questions, this looks solid to me though. Thanks for adding all the tests as well!

spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteDataFilesAction.java

ConeyLiu

+1

...ensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java

RussellSpitzer · 2023-09-18T20:16:27Z

Thanks @dramaticlly for the PR, and thank you @ConeyLiu for the Review! Merged

Spark 3.4: Push down system functions by V2 filters for rewriting Dat…

f791682

…aFiles and PositionDeleteFiles

github-actions bot added the spark label Sep 14, 2023

dramaticlly mentioned this pull request Sep 14, 2023

Spark: Rule for converting StaticInvoke to ApplyFunctionExpression for V2 filter push down #8088

Merged

ConeyLiu reviewed Sep 14, 2023

View reviewed changes

.../v3.4/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteDataFilesProcedure.java Show resolved Hide resolved

ConeyLiu reviewed Sep 14, 2023

View reviewed changes

.../v3.4/spark/src/main/java/org/apache/iceberg/spark/procedures/RewriteDataFilesProcedure.java Show resolved Hide resolved

ConeyLiu reviewed Sep 14, 2023

View reviewed changes

...ensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java Show resolved Hide resolved

RussellSpitzer reviewed Sep 14, 2023

View reviewed changes

...ensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Sep 14, 2023

View reviewed changes

spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteDataFilesAction.java Show resolved Hide resolved

RussellSpitzer reviewed Sep 14, 2023

View reviewed changes

spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteDataFilesAction.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Sep 14, 2023

View reviewed changes

Address review feedback

802dfd1

nastra reviewed Sep 15, 2023

View reviewed changes

spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteDataFilesAction.java Outdated Show resolved Hide resolved

ConeyLiu mentioned this pull request Sep 15, 2023

Spark 3.4: Rewrite procedure throw better exception when filter expression cannot translate #8394

Merged

ConeyLiu approved these changes Sep 15, 2023

View reviewed changes

Migrate junit4 assertion to assertJ

e8ca42f

advancedxy reviewed Sep 18, 2023

View reviewed changes

...ensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewriteDataFilesProcedure.java Show resolved Hide resolved

RussellSpitzer approved these changes Sep 18, 2023

View reviewed changes

RussellSpitzer merged commit 2817dd4 into apache:master Sep 18, 2023
31 checks passed

dramaticlly mentioned this pull request Sep 18, 2023

Switch to using ProcedureInput for rewriteDataFiles #8582

Closed

dramaticlly deleted the sparkFilterV2 branch April 3, 2024 21:18

manuzhang mentioned this pull request Sep 12, 2024

Spark 3.5: Support filtering with buckets in RewriteDataFilesProcedure #9396

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spark 3.4: Push down system functions by V2 filters for rewriting DataFiles and PositionDeleteFiles #8560

Spark 3.4: Push down system functions by V2 filters for rewriting DataFiles and PositionDeleteFiles #8560

dramaticlly commented Sep 14, 2023 •

edited

Loading

ConeyLiu Sep 14, 2023

RussellSpitzer Sep 14, 2023

dramaticlly Sep 14, 2023

ConeyLiu Sep 14, 2023

dramaticlly Sep 14, 2023

ConeyLiu Sep 14, 2023

RussellSpitzer Sep 14, 2023

dramaticlly Sep 14, 2023

ConeyLiu commented Sep 14, 2023

RussellSpitzer left a comment

ConeyLiu left a comment

RussellSpitzer commented Sep 18, 2023

Spark 3.4: Push down system functions by V2 filters for rewriting DataFiles and PositionDeleteFiles #8560

Spark 3.4: Push down system functions by V2 filters for rewriting DataFiles and PositionDeleteFiles #8560

Conversation

dramaticlly commented Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ConeyLiu commented Sep 14, 2023

RussellSpitzer left a comment

Choose a reason for hiding this comment

ConeyLiu left a comment

Choose a reason for hiding this comment

RussellSpitzer commented Sep 18, 2023

dramaticlly commented Sep 14, 2023 •

edited

Loading