API, Core: Make RewriteFiles flexible #7501

aokolnychyi · 2023-05-02T20:50:00Z

This PR makes RewriteFiles more flexible and aligned with other APIs like OverwriteFiles. One valid use case is discussed in #7389, where we want to replace one set of delete files with another one. Instead of adding another overloaded method, we should make the interface flexible enough to support various combinations.

aokolnychyi · 2023-05-02T20:51:57Z

api/src/main/java/org/apache/iceberg/RewriteFiles.java

@@ -34,13 +34,57 @@
 * will throw a {@link ValidationException}.
 */
 public interface RewriteFiles extends SnapshotUpdate<RewriteFiles> {
+  /**
+   * Delete a data file whose content was rewritten.


Using the word delete is a bit questionable in this context but that's what the Javadoc says. It is also similar to the naming used in OverwriteFiles and other APIs. I can see this being called rewriteFile or something too.

Our terminology always throw me a bit off here. We struggle with "delete" = physically remove and "delete" mark as logically removed. Both are used in various places and sometimes we also use "remove".

Here I think "originalFile" maybe is more of the logical description, "original" => "rewritten" instead of "add" and "delete" which are really the implementation details.

That said i'm fine with just "add" and "delete" since we use them in many other places.

Yeah, it is unfortunate we overlooked the distinction between physical delete and logical remove. Let me double check other APIs to see whether there are also any good examples.

Does anyone else have any preferences?

@jackye1995 @rdblue, any thoughts?

aokolnychyi · 2023-05-02T20:53:04Z

api/src/main/java/org/apache/iceberg/RewriteFiles.java

+   * @param sequenceNumber a data sequence number
+   * @return this for method chaining
+   */
+  RewriteFiles dataSequenceNumber(long sequenceNumber);


We can only set a data sequence number for data files. Whenever we rewrite delete files, we have to rely on sequence numbers of source files. I can see this being called newDataFilesDataSequenceNumber but that's too long.

aokolnychyi · 2023-05-02T20:54:46Z

core/src/test/java/org/apache/iceberg/TestRewriteFiles.java

@@ -86,7 +86,7 @@ public void testEmptyTable() {
                    .rewriteFiles(
                        ImmutableSet.of(),
                        ImmutableSet.of(FILE_A_DELETES),
-                        ImmutableSet.of(FILE_A),


Was it ever a valid use case?

This would be removing a delete file and adding a data file and a new delete file? Seems impossible in rewrite

Exactly, this is testing an error message so it was, probably, overlooked.

aokolnychyi · 2023-05-02T20:57:35Z

cc @nastra @jackye1995 @amogh-jahagirdar @szehon-ho @RussellSpitzer @rdblue @flyrain

szehon-ho · 2023-05-02T21:58:02Z

api/src/main/java/org/apache/iceberg/RewriteFiles.java

+  }
+
+  /**
+   * Delete a delete file whose content was rewritten.


Question: will this be not entirely accurate for RewritePositionDelete? We may end up not rewriting invalid deletes. Or is 'rewritten' more nuanced?

Do you mean cases when we rewrite a position delete file but discard no longer valid deletes?

Yea the way I read it, it may indicate that all the content will be rewritten. But its likely in my case that it is just dropped without being rewritten. Not sure if its just worth calling out here 'whose content was rewritten'? Maybe we can do the other way around, and add the javadoc on addFile(): 'Add a DeleteFile rewriting content of a deleted DeleteFile'.

RussellSpitzer · 2023-05-02T22:12:54Z

api/src/main/java/org/apache/iceberg/RewriteFiles.java

+   * @param deleteFile a new delete file
+   * @return this for method chaining
+   */
+  default RewriteFiles addFile(DeleteFile deleteFile) {


Do we want to specify that adding the same delete file/data file twice will have no effect? We required set
s before

That's a good point, I'll add. That said, neither DataFile nor DeleteFile implementations provide proper equality at the moment so Set was kind of useless.

True, Sounds like we can drop it, although may want to fix that in the future since we don't actually support multiple instances of the same file within a table.

RussellSpitzer · 2023-05-02T22:17:15Z

core/src/main/java/org/apache/iceberg/BaseRewriteFiles.java

@@ -26,6 +26,9 @@
 class BaseRewriteFiles extends MergingSnapshotProducer<RewriteFiles> implements RewriteFiles {
  private final Set<DataFile> replacedDataFiles = Sets.newHashSet();
  private Long startingSnapshotId = null;
+  private boolean replacesDeleteFiles = false;


rather than fields, can we check whether "repalcesDataFiles" or other data structures have contents?

Do you mean checking the underlying ManifestFilterManager, List<DataFile> and the map of delete files? I thought about this but was initially reluctant to touch ManifestFilterManager (4 different ways to delete files).

That said, these boolean flags are a bit odd. Now I am thinking it makes sense to spend a bit more time and check the underlying structures. @RussellSpitzer, is this what you meant?

Yep, sorry if I was unclear. It's just weird to keep this as state flags when they are just proxy variables for the actual state of containers we have access to.

RussellSpitzer

I think this makes more sense as an api than continuing to expand the single method rewrite(lots of args)

aokolnychyi · 2023-05-03T15:06:53Z

CI failed because of the API checks. I'll fix them together with addressing the feedback.

aokolnychyi · 2023-05-08T19:33:08Z

I double checked a few places that use this API as well as the underlying implementation methods. We do use delete in most places, so I'll go ahead and merge this PR. If there are any comments later, we can still change this before the release.

github-actions bot added API core labels May 2, 2023

aokolnychyi commented May 2, 2023

View reviewed changes

aokolnychyi force-pushed the refactor-rewrite-files branch from e9d4b8f to ca2d545 Compare May 2, 2023 20:56

aokolnychyi force-pushed the refactor-rewrite-files branch from ca2d545 to 7d16caa Compare May 2, 2023 21:38

szehon-ho reviewed May 2, 2023

View reviewed changes

RussellSpitzer reviewed May 2, 2023

View reviewed changes

RussellSpitzer approved these changes May 2, 2023

View reviewed changes

aokolnychyi force-pushed the refactor-rewrite-files branch from 7d16caa to cc7d2d5 Compare May 5, 2023 01:28

API, Core: Make RewriteFiles flexible

1a5d11d

aokolnychyi force-pushed the refactor-rewrite-files branch from cc7d2d5 to 1a5d11d Compare May 5, 2023 01:34

szehon-ho approved these changes May 5, 2023

View reviewed changes

aokolnychyi merged commit 174c4ca into apache:master May 8, 2023

jimjag pushed a commit to jimjag/iceberg that referenced this pull request May 8, 2023

API, Core: Make RewriteFiles flexible (apache#7501)

23419b4

jimjag pushed a commit to jimjag/iceberg that referenced this pull request May 8, 2023

API, Core: Make RewriteFiles flexible (apache#7501)

f7b1b72

jimjag pushed a commit to jimjag/iceberg that referenced this pull request May 8, 2023

API, Core: Make RewriteFiles flexible (apache#7501)

39bbb98

takezoe mentioned this pull request May 21, 2024

Avoid using deprecated method in IcebergMetadata trinodb/trino#22058

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API, Core: Make RewriteFiles flexible #7501

API, Core: Make RewriteFiles flexible #7501

aokolnychyi commented May 2, 2023

aokolnychyi May 2, 2023

RussellSpitzer May 2, 2023

aokolnychyi May 3, 2023

aokolnychyi May 5, 2023

aokolnychyi May 5, 2023

aokolnychyi May 2, 2023 •

edited

Loading

aokolnychyi May 2, 2023

RussellSpitzer May 2, 2023

aokolnychyi May 3, 2023

aokolnychyi commented May 2, 2023

szehon-ho May 2, 2023

aokolnychyi May 3, 2023

szehon-ho May 3, 2023

RussellSpitzer May 2, 2023 •

edited

Loading

aokolnychyi May 3, 2023

RussellSpitzer May 3, 2023

RussellSpitzer May 2, 2023

aokolnychyi May 3, 2023

RussellSpitzer May 3, 2023

RussellSpitzer left a comment

aokolnychyi commented May 3, 2023

aokolnychyi commented May 8, 2023

API, Core: Make RewriteFiles flexible #7501

API, Core: Make RewriteFiles flexible #7501

Conversation

aokolnychyi commented May 2, 2023

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

aokolnychyi May 2, 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

aokolnychyi commented May 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussellSpitzer May 2, 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

RussellSpitzer left a comment

Choose a reason for hiding this comment

aokolnychyi commented May 3, 2023

aokolnychyi commented May 8, 2023

aokolnychyi May 2, 2023 •

edited

Loading

RussellSpitzer May 2, 2023 •

edited

Loading