Core: Support rewriting delete files. #2294

openinx · 2021-03-04T08:17:28Z

In iceberg format v2, we've introduced the row-level delete feature, which would introduce many delete files ( include positional delete files and insert data files). Currently the batch RewriteAction only works for iceberg format v2, say compacting data files only. Before expose the format v2 to end users, we need to check whether the common features are OK, we should provide the ability to rewrite both data files and delete files.

In this PR, I'm trying to extend the RewriteFiles API to work for both data files and delete files, so that we could implement the format v2's RewriteFiles Action.

chenjunjiedada · 2021-03-04T08:30:43Z

I wrote an API in #2216 you may be interested.

openinx · 2021-03-04T08:49:44Z

I wrote an API in #2216 you may be interested.

Thanks for the great work, I'd like to review that patch.

openinx · 2021-03-08T07:39:20Z

Ping @rdblue @aokolnychyi @chenjunjiedada for reviewing, extending the RewriteFiles API is the first step for further format v2's rewrite actions (See the linked issues), thanks.

chenjunjiedada · 2021-03-08T09:17:51Z

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

   * @return this for method chaining
   */
-  RewriteFiles rewriteFiles(Set<DataFile> filesToDelete, Set<DataFile> filesToAdd);
+  default RewriteFiles rewriteFiles(Set<DataFile> filesToDelete, Set<DataFile> filesToAdd) {


How about adding RewriteFiles rewriteDeletes(Set<DeleteFile> fileToDelete, Set<DeleteFile> filesToAdd for rewrite deletes?

I think you want to expose this API for replacing equality deletions with positional deletions, for me that seems like an internal usage we may don't have to define such a specific API for end users. I prefer to expose the following common API to end users, for our internal usage we could rewrite files based on that one.

openinx · 2021-03-09T07:25:17Z

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

+  public RewriteFiles rewriteFiles(Set<DataFile> dataFilesToDelete, Set<DeleteFile> deleteFilesToDelete,
+                                   Set<DataFile> dataFilesToAdd, Set<DeleteFile> deleteFilesToAdd) {
+    checkFilesToDelete(dataFilesToDelete, deleteFilesToDelete);
+    checkFilesToAdd(dataFilesToAdd, deleteFilesToAdd);


The files to add seems could be empty since we have introduced the row-level deletes. For example, we insert <1, 'aaa'>. then delete <1, 'aaa'>, finally do the rewrite files action. In the end, there should be no file to add when rewriting.

yyanyy

LGTM, mostly minor comments

yyanyy · 2021-03-18T02:27:49Z

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

+
+    Assert.assertEquals("Should contain 3 manifest", 3, pending.allManifests().size());
+    Assert.assertFalse("Should not contain manifest from initial write",
+        pending.allManifests().containsAll(initialManifests));


I think we want to check if any manifest matches initial manifests by .allManifests.stream().anyMatch(initialManifests::contains) ?

yyanyy · 2021-03-18T02:32:41Z

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

+    TableMetadata metadata = readMetadata();
+    List<ManifestFile> committedManifests = Lists.newArrayList(manifest1, manifest2, manifest3);
+    Assert.assertTrue("Should committed the manifests",
+        metadata.currentSnapshot().allManifests().containsAll(committedManifests));


nit: could do metadata.currentSnapshot().allManifests().equals(committedManifests)

yyanyy · 2021-03-18T02:35:39Z

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

+    ManifestFile manifest3 = pending.allManifests().get(2);
+
+    validateManifestEntries(manifest1,
+        ids(baseSnapshotId),


nit: should we verify that sequence number for this is 1?

I think we'd better to add the seq number verification after we have reached agreement on this issue: #2308.

yyanyy · 2021-03-18T02:36:13Z

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

+    metadata = readMetadata();
+    List<ManifestFile> committedManifests = Lists.newArrayList(manifest1, manifest2, manifest3);
+    Assert.assertTrue("Should committed the manifests",
+        metadata.currentSnapshot().allManifests().containsAll(committedManifests));


nit: also equals here

yyanyy · 2021-03-18T02:43:09Z

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

+    if (deleteFilesToDelete == null || deleteFilesToDelete.isEmpty()) {
+      // When there is no delete files in the rewrite action, data files to add cannot be null or empty.
+      Preconditions.checkArgument(dataFilesToAdd != null && dataFilesToAdd.size() > 0,
+          "Data files to add can not be null or empty because there's no delete file to rewrite");


Nit: might want to update to something like "Data files to add can not be null or empty when there is no delete file to be rewritten", similar applies to L61

yyanyy · 2021-03-18T02:47:13Z

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

+  }
+
+  /**
+   * Add a rewrite that replaces one set of files with another set that contains the same data (format v2).


nit: probably don't need to mention format v2 specifically

yyanyy · 2021-03-18T02:50:11Z

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

-    Preconditions.checkArgument(filesToDelete != null && !filesToDelete.isEmpty(),
-        "Files to delete cannot be null or empty");
-    Preconditions.checkArgument(filesToAdd != null && !filesToAdd.isEmpty(),


Looks like we are changing the logic now to not enforce input sets to be non-nullable? I think for the new code we can do a precondition check on the four input sets to ensure they are all non-null, to save all the null check everywhere

Looks like we are changing the logic now to not enforce input sets to be non-nullable?

Yes, in current version we are required to pass non-empty and non-null Set because we must ensure the input files and output files have some rows to rewrite & generate. After introducing the format v2, the data input files can be empty (if we plan to convert eq-deletes to pos-deletes), then I think we also don't have to require the user to pass a non-null empty set. Here passing a null or ImmutableMap.of() , both of them looks good to me.

I agree that they can be empty, I guess my major point is that since it is the new API we introduced here that allows empty input, we can enforce inputs to be not null by adding a precondition check at the beginning of the method to fail if null is passed in, so that we don't have to do all the != null everywhere to make the code a bit more clean, while still allowing ImmutableSet.of() which should logically be equivalent to null.

OK, I think it's good to simplify this null check, just updated this patch.

openinx · 2021-03-26T07:29:11Z

Filed an issue to address the flakey unit test #2384, will retry the travis CI.

openinx · 2021-03-29T03:10:50Z

Got this merged, thanks @yyanyy and @chenjunjiedada for reviewing.

openinx requested a review from rdblue March 4, 2021 08:17

github-actions bot added API core labels Mar 4, 2021

openinx linked an issue Mar 5, 2021 that may be closed by this pull request

Add an action to rewrite equality deletes as position deletes #1026

Closed

openinx mentioned this pull request Mar 5, 2021

Add an action to rewrite equality deletes as position deletes #1026

Closed

openinx removed a link to an issue Mar 8, 2021

Add an action to rewrite equality deletes as position deletes #1026

Closed

openinx linked an issue Mar 8, 2021 that may be closed by this pull request

Add an action to rewrite data files and remove deleted rows #1028

Closed

chenjunjiedada reviewed Mar 8, 2021

View reviewed changes

openinx mentioned this pull request Mar 8, 2021

Core: Remove all delete files in RewriteFiles action. #2303

Closed

openinx commented Mar 9, 2021

View reviewed changes

yyanyy reviewed Mar 18, 2021

View reviewed changes

chenjunjiedada mentioned this pull request Mar 24, 2021

Spark: Add an action to rewrite equality deletes #2364

Closed

openinx added 6 commits March 24, 2021 16:05

Core: Support rewriting delete files.

370a365

Add unit tests.

06492ad

Refactor the RewriteFiles API

0f4edf3

Minior fixes.

3475382

Addressing comments

6f7ede4

Fix broken unit tests

fe98e01

yyanyy approved these changes Mar 25, 2021

View reviewed changes

Address the nullable comments.

e84022e

openinx closed this Mar 26, 2021

openinx reopened this Mar 26, 2021

openinx merged commit 7a0954f into apache:master Mar 29, 2021

openinx deleted the rewrite-v2 branch March 29, 2021 03:11

coolderli pushed a commit to coolderli/iceberg that referenced this pull request Apr 26, 2021

Core: Support rewriting delete files. (apache#2294)

2db7f9f

stevenzwu pushed a commit to stevenzwu/iceberg that referenced this pull request Jul 28, 2021

Core: Support rewriting delete files. (apache#2294)

2cbfa37

Core: Support rewriting delete files. #2294

Core: Support rewriting delete files. #2294

Uh oh!

Conversation

openinx commented Mar 4, 2021

Uh oh!

chenjunjiedada commented Mar 4, 2021

Uh oh!

openinx commented Mar 4, 2021

Uh oh!

openinx commented Mar 8, 2021

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!

yyanyy 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!

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!

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!

openinx commented Mar 26, 2021

Uh oh!

openinx commented Mar 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants