Core: Support commits with DVs #11495

aokolnychyi · 2024-11-08T18:02:02Z

This PR adds supports for committing operations with DVs.

This work is part of #11122.

aokolnychyi · 2024-11-08T18:05:15Z

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

@@ -769,6 +794,82 @@ protected void validateDataFilesExist(
    }
  }

+  // validates there are no concurrently added DVs for referenced data files
+  protected void validateAddedDVs(


This simply validates there is at most one DV per file. There would be a separate PR to automatically rewrite conflicting DVs during retries. The goal is to make DVs fully usable before optimizing.

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

amogh-jahagirdar

Still need to review tests but the code changes themselves overall look good, just some minor comments. Thanks @aokolnychyi!

amogh-jahagirdar · 2024-11-08T23:34:11Z

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

+  }
+
+  // builds a set of data file locations referenced by new DVs
+  private Set<String> dvRefs() {


Should we call this referencedDataFiles?

nit: it's a private method so name is not a big deal

I think that dvRefs() (or dvReferences()) makes more sense here as it checks if the deleteFile is a DV. So, having DV in the method name makes sense to me.

We need to have dv in the name. If so, I'd prefer a shorter var name to avoid splitting statements on multiple lines.

amogh-jahagirdar · 2024-11-08T23:41:23Z

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

+        .stopOnFailure()
+        .throwFailureWhenFinished()
+        .executeWith(workerPool())
+        .run(m -> validateAddedDVs(m, conflictDetectionFilter, newSnapshotIds, dvRefs));


Nit: Should we use the full word manifest in the lambda?

For clarity I agree with @amogh-jahagirdar (manifest instead of m). I'm fine to use DV instead of DeleteVector but manifest here would help 😃

It was me trying to work-around Spotless formatting for closures. I updated.

jbonofre

I did a first quick pass. I will to another round asap. Thanks @aokolnychyi great work as usual !

jbonofre · 2024-11-09T09:43:04Z

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

@@ -139,6 +139,10 @@ protected void validate(TableMetadata base, Snapshot parent) {
      if (validateNewDeleteFiles) {
        validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
      }
+
+      if (base.formatVersion() >= 3 && addsDeleteFiles()) {


Do we really need the format version here ? Why not just using addsDeleteFiles flag ? (just wondering).

I've restructured this part a bit to compute dv refs as deletes are being added. No longer applies.

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

jbonofre · 2024-11-09T09:46:48Z

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

+          "Must not use DVs for position deletes in V2: %s",
+          ContentFileUtil.dvDesc(file));
+      Preconditions.checkArgument(
+          formatVersion() == 2 || ContentFileUtil.isDV(file),


Same question here, why not just checking if it's DV: we know that if it's DV it's format version 3+ right ?

This method validates input from the user and ensures it is not possible to add a DV to a V2 table.

jbonofre · 2024-11-09T09:47:56Z

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

+        .stopOnFailure()
+        .throwFailureWhenFinished()
+        .executeWith(workerPool())
+        .run(m -> validateAddedDVs(m, conflictDetectionFilter, newSnapshotIds, dvRefs));


For clarity I agree with @amogh-jahagirdar (manifest instead of m). I'm fine to use DV instead of DeleteVector but manifest here would help 😃

jbonofre · 2024-11-09T09:49:52Z

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

+  }
+
+  // builds a set of data file locations referenced by new DVs
+  private Set<String> dvRefs() {


nit: it's a private method so name is not a big deal

I think that dvRefs() (or dvReferences()) makes more sense here as it checks if the deleteFile is a DV. So, having DV in the method name makes sense to me.

nastra · 2024-11-11T09:31:32Z

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

+    return formatVersion >= 3 ? FILE_B_DV : FILE_B_DELETES;
+  }
+
+  protected DeleteFile newDeletes(DataFile dataFile) {


maybe newDeleteFile?

Other newDeleteFile methods below return V2 position deletes, so I wanted to differentiate.
I am OK renaming too.

I believe callers won't care as long as you get a correct DeleteFile for the correct formatVersion, so I was thinking that we most likely don't need to differentiate here with the naming

I have no preference, to be honest. I'll update in the next round.

I went to rename and I remembered why I did it this way. I wanted a way to construct a V2 delete file independently of the format version and to not need to update the other methods that create delete files now.

nastra · 2024-11-11T09:32:30Z

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

@@ -78,7 +78,7 @@ public void testFileSizeSummary() {

  @TestTemplate
  public void testFileSizeSummaryWithDeletes() {
-    assumeThat(formatVersion).isGreaterThan(1);
+    assumeThat(formatVersion).isEqualTo(2);


are you planning to update these tests eventually to run against format version 3?

Not sure. There are new tests for DVs in this class. I think this one needs V2 position deletes, we simply didn't have the validation before. V2 position deletes are not allowed in V3 tables. What do you think, @nastra?

ah sorry I forgot that you already added testFileSizeSummaryWithDVs, so this LGTM

nastra · 2024-11-11T09:34:07Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteManifestsAction.java

@@ -719,7 +719,7 @@ public void testRewriteLargeManifestsEvolvedUnpartitionedV1Table() throws IOExce

  @TestTemplate
  public void testRewriteSmallDeleteManifestsNonPartitionedTable() throws IOException {
-    assumeThat(formatVersion).isGreaterThan(1);
+    assumeThat(formatVersion).isEqualTo(2);


FYI I'm updating this test class in #11485

Awesome, will take a look!

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

danielcweeks · 2024-11-11T18:52:18Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteManifestsAction.java

@@ -956,6 +956,62 @@ public void testRewriteLargeDeleteManifestsPartitionedTable() throws IOException
    assertThat(deleteManifests).hasSizeGreaterThanOrEqualTo(2);
  }

+  @TestTemplate
+  public void testUpgradeToV3() throws IOException {


I feel a little awkward about the scope of this test. It's testing upgrade and manifest rewrite, but there are specific aspects that are included.

I feel like there are more scenarios to cover here (e.g. writing a DV to an existing file that has positional delete after the upgrade.). I would say either refine the name to be more explicit (e.g. testUpgradeToV3WithExistingV2Deletes) so that we can add more test cases for the upgrade path.

I agree the name is too generic, I'll update. I did cover adding a DV to an existing file with positional delete in a previous PR here).

There is also an extra test in TestRowDelta. This one was for the action that rewrites manifests, but I renamed both.

aokolnychyi · 2024-11-11T21:09:37Z

Thanks for reviewing, @nastra @danielcweeks @jbonofre @amogh-jahagirdar!

github-actions bot added spark core labels Nov 8, 2024

aokolnychyi commented Nov 8, 2024

View reviewed changes

Core: Support commits with DVs

52e4898

aokolnychyi force-pushed the commit-dvs branch from e88993e to 52e4898 Compare November 8, 2024 18:27

danielcweeks reviewed Nov 8, 2024

View reviewed changes

core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java Show resolved Hide resolved

Fix tests

21fe5b0

github-actions bot added the data label Nov 8, 2024

amogh-jahagirdar reviewed Nov 9, 2024

View reviewed changes

jbonofre reviewed Nov 9, 2024

View reviewed changes

aokolnychyi added 2 commits November 9, 2024 23:39

Improve and add tests

48c8254

Fix method naming

17bed44

nastra reviewed Nov 11, 2024

View reviewed changes

nastra approved these changes Nov 11, 2024

View reviewed changes

danielcweeks reviewed Nov 11, 2024

View reviewed changes

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

danielcweeks reviewed Nov 11, 2024

View reviewed changes

aokolnychyi added 2 commits November 11, 2024 20:36

Rename test name

10744ef

Rename test name

1d04c0d

danielcweeks approved these changes Nov 11, 2024

View reviewed changes

Use switch for validation

a57f47c

aokolnychyi merged commit af3fbfe into apache:main Nov 11, 2024
49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Support commits with DVs #11495

Core: Support commits with DVs #11495

aokolnychyi commented Nov 8, 2024

aokolnychyi Nov 8, 2024 •

edited

Loading

amogh-jahagirdar left a comment

amogh-jahagirdar Nov 8, 2024

jbonofre Nov 9, 2024

aokolnychyi Nov 10, 2024

amogh-jahagirdar Nov 8, 2024

jbonofre Nov 9, 2024

aokolnychyi Nov 10, 2024

jbonofre left a comment

jbonofre Nov 9, 2024

aokolnychyi Nov 10, 2024

jbonofre Nov 9, 2024

aokolnychyi Nov 10, 2024

jbonofre Nov 9, 2024

jbonofre Nov 9, 2024

nastra Nov 11, 2024

aokolnychyi Nov 11, 2024 •

edited

Loading

nastra Nov 11, 2024

aokolnychyi Nov 11, 2024

aokolnychyi Nov 11, 2024

nastra Nov 11, 2024

aokolnychyi Nov 11, 2024 •

edited

Loading

nastra Nov 11, 2024

nastra Nov 11, 2024

aokolnychyi Nov 11, 2024

danielcweeks Nov 11, 2024

aokolnychyi Nov 11, 2024 •

edited

Loading

aokolnychyi Nov 11, 2024

aokolnychyi commented Nov 11, 2024

Core: Support commits with DVs #11495

Core: Support commits with DVs #11495

Conversation

aokolnychyi commented Nov 8, 2024

aokolnychyi Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

amogh-jahagirdar left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbonofre left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aokolnychyi Nov 11, 2024 • 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

aokolnychyi Nov 11, 2024 • 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

aokolnychyi Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aokolnychyi commented Nov 11, 2024

aokolnychyi Nov 8, 2024 •

edited

Loading

aokolnychyi Nov 11, 2024 •

edited

Loading

aokolnychyi Nov 11, 2024 •

edited

Loading

aokolnychyi Nov 11, 2024 •

edited

Loading