Core, Spark 3.4: Write properties of PositionDeletesTable should respect ones of BaseTable

[jerqi]

What changes were proposed in this pull request?

Make write properties of PositionDeletesTable respect ones of BaseTable.

Why are the changes needed?

When we use PositionDeletesRewriteAction, we will use the properties of PositionDeletesTable, but the properties are empty before this pr. We will use default properties all the time. The write file format of PositionDeletesRewriteAction will be parquet although the table use orc as write format. It's unreasonable. More information you can see #8313 (comment)

Does this PR introduce any user-facing change?

No.

How was this patch tested?

UT

[szehon-ho]

I think this option works for me, @aokolnychyi for any concerns?

[szehon-ho]

@jerqi sorry about this, I was re-thinking about it and not 100% sure it makes sense, as there are some table properties that look weird on all metadata tables.

What do you think about first trying to solve it in [SparkBinPackPositionDeletesRewriter] (using ds write options) (https://github.com/apache/iceberg/blob/master/spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/actions/SparkBinPackPositionDeletesRewriter.java) I think that's a bit less impact.

[jerqi]

@jerqi sorry about this, I was re-thinking about it and not 100% sure it makes sense, as there are some table properties that look weird on all metadata tables.

What do you think about first trying to solve it in [SparkBinPackPositionDeletesRewriter] (using ds write options) (https://github.com/apache/iceberg/blob/master/spark/v3.4/spark/src/main/java/org/apache/iceberg/spark/actions/SparkBinPackPositionDeletesRewriter.java) I think that's a bit less impact.

1. I can raise a this solution pr.

2. And the properties of PositionDeletesTable is empty.
   There are many properties will be useful to write data.

write.delete.orc.compression-codec
write.delete.parquet.row-group-check-max-record-count
write.parquet.bloom-filter-enable
......

We need to set every write option for the data frame. It may be difficult for users to use.
What about my second solution? We only modify the properties of PositionDeletesTable.
Moreover, We can filter out the unreasonable table property. WDYT?

[szehon-ho]

@jerqi ok that sounds good with me, let's go with that approach. Mostly write properties I assume?

Writing position deletes through position_deletes table is not exposed to user, as we dont support that outside rewrite_position_deletes. But I see your point that doing it inside the action makes the code harder as there's quite a few possible properties.

[jerqi]

Mostly write properties

Maybe we should include read properties and commit properties, too, let me see what properties the rewrite_position_deletes used.

[jerqi]

Mostly write properties

Maybe we should include read properties and commit properties, too, let me see what properties the rewrite_position_deletes used.

Yes, you're right. We need mostly write properties.

[jerqi]

I find that all the write properties are needed for our PositionDeletesRewriteAction. So I choose to match the key prefix here instead of copying some specific entries.

[jerqi]

@szehon-ho Could you review this pr again if you have time?

[szehon-ho]

Merged, thanks @jerqi . Can you please update the pr description to make it clearer what problem we are fixing? And do we need to make the fix for other Spark versions?

[jerqi]

Merged, thanks @jerqi . Can you please update the pr description to make it clearer what problem we are fixing? And do we need to make the fix for other Spark versions?

1. ok, I will update the pr description.
2. ok, I will raise another pr to fix Spark 3.5.

[jerqi]

@szehon-ho I have raised a new pr for Spark 3.5. https://github.com/apache/iceberg/pull/8584/files