Do these properties belong here now?

The plan currently returns the jobs in order in a CloseableIterable.
We can push the ordering to the user, but this seems something we can reuse between Spark and Flink, so I decided to keep it here.

Your thoughts?

probably RewritePositionDeletesPlanner is more accurate

Renamed to BinPackRewritePositionDeletePlanner

I know this is refactored from existing code. this config name doesn't seem the most intuitive. delete-file-threshold could create an impression of delete file size threshold (in bytes). I think it meant deleted-rows-threshold. But it is probably too late to change it for compatibility reason.

While I agree with you, I also agree with you that we don't want to touch this in this PR (and probably not even later)

some of the methods in this class look similar to the code in the BinPackRewriteFilePlanner. can the code be pushed to the base class? e.g. plan(), most of planFileGroups, etc.

In this specific method, I guess PartitionUtil.coercePartition is probably different

I had the same feeling, and I tried it several times, but there are nuances which makes them different, and hard to generalize:

• The input tasks are different
• The partition handling is different (data file compaction handles partition evolution)
• The file filtering logic is different
• The group filterig logic is different
• Result groups are different

I see the partition handling as the main issue.
When compacting data files we have this additional logic:

      // If a task uses an incompatible partition spec the data inside could contain values
      // which belong to multiple partitions in the current spec. Treating all such files as
      // un-partitioned and grouping them together helps to minimize new files made.

When compacting position delete files just uses coercePartition to update the task partition to the current one.

Based on these, I decided against the generalization.

sounds good.