Core: Interface changes for separating rewrite planner and runner by pvary · Pull Request #12306 · apache/iceberg

pvary · 2025-02-18T10:08:30Z

Compaction interface changes which separates the planning and the execution phase of the compaction.
This enables moving the common planning part of the compaction from Spark code to the core, and Flink (or another execution engines) could implement their own rewrite executor to do the actual compaction.
See discussion on: https://lists.apache.org/thread/6lj2jn3dbvqjscc96w0mc32bhxq0qfqv

stevenzwu · 2025-02-18T17:48:43Z

core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java

+ * @param <T> the Java type of the tasks to read the files which are rewritten
+ * @param <F> the Java type of the content files which are rewritten
+ */
+public abstract class FileRewriteGroup<I, T extends ContentScanTask<F>, F extends ContentFile<F>> {


it is confusing to introduce FileRewriteGroup name with another existing class named RewriteFileGroup.

We can probably call this BaseFileGroup or BaseRewriteFileGroup.

Names staring with Base are reserved for the immutable interfaces for the actions themselves.

I would like to keep the FileRewriteGroup/FileRewritePlan/FileRewritePlanner/FileRewriteRunner similar and/or grouped together.

Maybe create a new package (org.apache.iceberg.actions.rewrite)? Maybe a name starting with the same prefix (RewriteGroup/RewritePlan/RewritePlanner/RewriteRunner)?

Names staring with Base are reserved for the immutable interfaces for the actions themselves.

Is Base* reserved only for actions in that package? Personally, I don't see an issue of adding classes like BaseRewriteFileGroup, BaseRewriteFilePlan etc.

Currently only action base classes are there with name starting with base.

RewriteFilePlan will be the final class. I don't plan to add different plans for different compaction methods. They will only differ in the RewriteFileGroups.

What about the planner/runner interfaces? They are definitely not base

core/src/main/java/org/apache/iceberg/actions/FileRewriteExecutor.java

stevenzwu · 2025-02-18T23:00:42Z

core/src/main/java/org/apache/iceberg/actions/FileRewriteExecutor.java

+ * @param <G> the Java type of the planned groups
+ */
+public interface FileRewriteExecutor<
+    FGI,


Right now, it seems that {@link RewriteDataFiles.FileGroupInfo} and {@link RewritePositionDeleteFiles.FileGroupInfo} interfaces are identical. wondering if we should start without generics for FGI? it can affect future flexibility though.

I didn't change this, because the info objects are coming from the immutable base infrastructure for the actions. I decided against touching those to limit the scope of the PR

stevenzwu · 2025-02-18T23:01:39Z

core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java

+/**
+ * Container class representing a set of files to be rewritten by a {@link FileRewriteExecutor}.
+ *
+ * @param <I> the Java type of the plan info member like {@link RewriteDataFiles.FileGroupInfo} or


for consistency with the class above, should this generic type also be called FGI?

This checkstyle module prevents using FGI for classes:

<module name="ClassTypeParameterName">  <property name="format" value="(^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*[T]$)"/> </module>

We don't have similar for interfaces, like:

<module name="InterfaceTypeParameterName">  <property name="format" value="(^[A-Z][0-9]?)$|([A-Z][a-zA-Z0-9]*[T]$)"/> </module>

Since it was not highlighted by checkstyle, I forgot to update the interfaces. I will move from FGI to I as well.

stevenzwu · 2025-02-18T23:46:25Z

core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java

+  }
+
+  /** Accumulated size for the input files. */
+  public long sizeInBytes() {


inputFilesSizeInBytes?

This method exists in the current API as RewriteFileGroup.sizeInBytes() and RewritePositionDeletesGroup.rewrittenBytes() and the new method name will be inherited from the FileRewriteGroup

Minimally one of them needs to be deprecated, renamed. Do you think we should change both?

core/src/main/java/org/apache/iceberg/actions/FileRewriteGroup.java

core/src/main/java/org/apache/iceberg/actions/FileRewritePlan.java

core/src/main/java/org/apache/iceberg/actions/FileRewritePlanner.java

stevenzwu · 2025-02-19T18:25:30Z

@pvary regarding the inheritance, I am thinking it might still be valuable to rename/clarify them in the new base class. We can still keep and deprecate the existing methods and just forward the calls to the base class. some of the method names aren't as clear, like info, numFiles. We can use this refactoring opportunity to improve them.

pvary · 2025-02-19T20:13:54Z

@pvary regarding the inheritance, I am thinking it might still be valuable to rename/clarify them in the new base class. We can still keep and deprecate the existing methods and just forward the calls to the base class. some of the method names aren't as clear, like info, numFiles. We can use this refactoring opportunity to improve them.

Ok. Will do. I agree but wanted to start from the minimal changeset.

stevenzwu

LGTM

RussellSpitzer · 2025-02-20T15:59:26Z

core/src/main/java/org/apache/iceberg/actions/FileRewritePlan.java

+/**
+ * Result of the file rewrite planning as generated by the {@link FileRewritePlanner#plan()}.
+ *
+ * <p>The plan contains an iterable for the planned groups and statistics about the number of the


The doc here is a bit confusing to me. I think it has a bit too many implementation details.

Probably something like

a Container class holding a the output of a FileRewritePlanner.plan call. Contains a list of "RewriteFileGroups" see ... link which are used by engines to rewrite a particular set of files. ?

RussellSpitzer · 2025-02-20T16:08:36Z

core/src/main/java/org/apache/iceberg/actions/FileRewritePlanner.java

+import org.apache.iceberg.ContentScanTask;
+
+/**
+ * A class for planning content file rewrites.


Planning needs more defining here imho.

The notes below are not clear to me. They seem to be for the entire Operation. I think if we were going to write that there it should be something like

A Rewrite Operation is performed by several classes working in conjunction.

A FileRewritePlanner - (this class) which scans the files in the table and determines which files should be rewritten and how they should be grouped. This produces a FileRewritePlan.

FileRewritePlan - This immutable output of the planner contains a set of Groups, each containing a set of files that are ment to be compacted together by the FileRewriteRunner.

The FileRewrite Runner is the engine specific implementation that can take a FileRewritePlan and perform a rewrite on each of the groups within. This is the actual implementation of the rewrite which generates new files.

?

Fixed, please take a look

core/src/main/java/org/apache/iceberg/actions/FileRewriteRunner.java

RussellSpitzer · 2025-02-20T16:26:31Z

core/src/main/java/org/apache/iceberg/actions/RewriteGroupBase.java

+  }
+
+  /**
+   * While we create tasks that should all be smaller than our target size, there is a chance that


This doc is specific to the planner not the Group

Reworded to highlight the relevant part for the runners. Please take a look.

RussellSpitzer · 2025-02-20T16:29:28Z

core/src/main/java/org/apache/iceberg/actions/RewriteGroupBase.java

+  }
+
+  /**
+   * Determines the reader split size as the input size divided by the desired number of output


Also specific to the planner. I'm also not sure if split size is a good name here.

This should be something like

The amount of bytes of data the FileRewriteRunner should pull from a single group in a single read task (?)

Reworded. Please take a look.

core/src/main/java/org/apache/iceberg/actions/RewriteGroupBase.java

RussellSpitzer · 2025-02-26T16:34:28Z

Ok I'm on board, I think the interfaces are good and Java Doc is solid for now, if @stevenzwu is good with the changes I say we merge and start the rest of the refactor.

stevenzwu · 2025-02-27T01:22:42Z

yes, current state looks good to me. let me merge it.

pvary · 2025-02-27T14:53:35Z

Thanks @stevenzwu and @RussellSpitzer!
I will be OOO next week, so the next PR will come after that

github-actions bot added the core label Feb 18, 2025

pvary force-pushed the rewrite_api branch 2 times, most recently from ced2bee to 33827d1 Compare February 18, 2025 10:41

[core] Interface changes for separating rewrite planner and executor

47ab886

pvary force-pushed the rewrite_api branch from 33827d1 to 47ab886 Compare February 18, 2025 11:02

stevenzwu reviewed Feb 18, 2025

View reviewed changes

core/src/main/java/org/apache/iceberg/actions/FileRewriteExecutor.java Outdated Show resolved Hide resolved

stevenzwu reviewed Feb 18, 2025

View reviewed changes

pvary force-pushed the rewrite_api branch 3 times, most recently from 43c75d0 to a70ee75 Compare February 19, 2025 10:50

Steven's comments first round

9060f8f

pvary force-pushed the rewrite_api branch from a70ee75 to 9060f8f Compare February 19, 2025 11:40

Changing/deprecating old method names in groups

365ab4f

pvary changed the title ~~Core: Interface changes for separating rewrite planner and executor~~ Core: Interface changes for separating rewrite planner and runner Feb 19, 2025

stevenzwu approved these changes Feb 19, 2025

View reviewed changes

RussellSpitzer reviewed Feb 20, 2025

View reviewed changes

core/src/main/java/org/apache/iceberg/actions/FileRewriteRunner.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Feb 20, 2025

View reviewed changes

core/src/main/java/org/apache/iceberg/actions/RewriteGroupBase.java Outdated Show resolved Hide resolved

Javadoc comments

c423b42

pvary force-pushed the rewrite_api branch from c10a7fd to c423b42 Compare February 21, 2025 10:07

pvary mentioned this pull request Feb 26, 2025

Flink Table Maintenance #10264

Open

6 tasks

RussellSpitzer approved these changes Feb 26, 2025

View reviewed changes

stevenzwu merged commit a50ec92 into apache:main Feb 27, 2025
42 checks passed

pvary mentioned this pull request Mar 10, 2025

Core: FileRewritePlanner implementation #12493

Merged

pvary mentioned this pull request Apr 7, 2025

Spark: support rewrite on specified target branch #12257

Closed

pvary mentioned this pull request Apr 14, 2025

Flink: Maintenance - RewriteDataFiles #11497

Merged

Conversation

pvary commented Feb 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevenzwu commented Feb 19, 2025

Uh oh!

pvary commented Feb 19, 2025

Uh oh!

stevenzwu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RussellSpitzer Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RussellSpitzer commented Feb 26, 2025

Uh oh!

stevenzwu commented Feb 27, 2025

Uh oh!

Uh oh!

pvary commented Feb 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

RussellSpitzer Feb 20, 2025 •

edited

Loading

RussellSpitzer Feb 20, 2025 •

edited

Loading

RussellSpitzer Feb 20, 2025 •

edited

Loading