You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "aokolnychyi (via GitHub)" <gi...@apache.org> on 2024/11/08 18:02:05 UTC
[PR] Core: Support commits with DVs [iceberg]
aokolnychyi opened a new pull request, #11495:
URL: https://github.com/apache/iceberg/pull/11495
This PR adds supports for committing operations with DVs.
This work is part of #11122.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1837117968
##########
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 {
Review Comment:
There is also an extra test in `TestRowDelta`. This one was for the action that rewrites manifests, but I renamed both.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1836207152
##########
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);
Review Comment:
are you planning to update these tests eventually to run against format version 3?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1837127586
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -268,9 +274,32 @@ private void add(PendingDeleteFile file) {
if (deleteFiles.add(file)) {
addedFilesSummary.addedFile(spec, file);
hasNewDeleteFiles = true;
+ if (ContentFileUtil.isDV(file)) {
+ newDVRefs.add(file.referencedDataFile());
+ }
+ }
+ }
+
+ protected void validateNewDeleteFile(DeleteFile file) {
+ Preconditions.checkNotNull(file, "Invalid delete file: null");
+ Preconditions.checkArgument(formatVersion() >= 2, "Deletes are supported in V2 and above");
+ if (file.content() == FileContent.POSITION_DELETES) {
+ Preconditions.checkArgument(
+ formatVersion() != 2 || !ContentFileUtil.isDV(file),
Review Comment:
Yeah, you're right. I feel like these are hard to read because of the way the checks stack on each other.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1836681746
##########
core/src/test/java/org/apache/iceberg/TestBase.java:
##########
@@ -643,6 +666,22 @@ protected DataFile newDataFile(String partitionPath) {
.build();
}
+ protected DeleteFile fileADeletes() {
+ return formatVersion >= 3 ? FILE_A_DV : FILE_A_DELETES;
+ }
+
+ protected DeleteFile fileBDeletes() {
+ return formatVersion >= 3 ? FILE_B_DV : FILE_B_DELETES;
+ }
+
+ protected DeleteFile newDeletes(DataFile dataFile) {
Review Comment:
Other `newDeleteFile` methods below return V2 position deletes, so I wanted to differentiate. I am OK renaming too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1836209280
##########
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);
Review Comment:
FYI I'm updating this test class in https://github.com/apache/iceberg/pull/11485
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1836685282
##########
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);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835144928
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -245,13 +250,13 @@ private PartitionSpec spec(int specId) {
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
add(new PendingDeleteFile(file));
}
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file, long dataSequenceNumber) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
Review Comment:
Yep, it is [here](https://github.com/apache/iceberg/pull/11495/files#diff-410ff1b47d9a44a2fd5dbd103cad9463d82c8f4f51aa1be63b8b403123ab6e0eR1571). Let me know if that's what you meant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1837051720
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -268,9 +274,32 @@ private void add(PendingDeleteFile file) {
if (deleteFiles.add(file)) {
addedFilesSummary.addedFile(spec, file);
hasNewDeleteFiles = true;
+ if (ContentFileUtil.isDV(file)) {
+ newDVRefs.add(file.referencedDataFile());
+ }
+ }
+ }
+
+ protected void validateNewDeleteFile(DeleteFile file) {
+ Preconditions.checkNotNull(file, "Invalid delete file: null");
+ Preconditions.checkArgument(formatVersion() >= 2, "Deletes are supported in V2 and above");
+ if (file.content() == FileContent.POSITION_DELETES) {
+ Preconditions.checkArgument(
+ formatVersion() != 2 || !ContentFileUtil.isDV(file),
Review Comment:
This is a little more clear if we invert the check:
```suggestion
formatVersion() == 2 && ContentFileUtil.isDV(file),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1834857507
##########
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(
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835114428
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -245,13 +250,13 @@ private PartitionSpec spec(int specId) {
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
add(new PendingDeleteFile(file));
}
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file, long dataSequenceNumber) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
Review Comment:
This only affects new delete files that are added to the table. I had a test in the prototype PR that verified that automatic manifest compaction after upgrading to V3 didn't have issues. Let me check if I added it here too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835147603
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -245,13 +250,13 @@ private PartitionSpec spec(int specId) {
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
add(new PendingDeleteFile(file));
}
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file, long dataSequenceNumber) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
Review Comment:
That said, there are some test failures in Spark that may be related. Let me explore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835614371
##########
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(
+ TableMetadata base,
+ Long startingSnapshotId,
+ Expression conflictDetectionFilter,
+ Snapshot parent) {
+ // skip if there is no current table state or table format doesn't support DVs
+ if (parent == null || base.formatVersion() < 3) {
+ return;
+ }
+
+ // skip if this operation doesn't add new DVs
+ Set<String> dvRefs = dvRefs();
+ if (dvRefs.isEmpty()) {
+ return;
+ }
+
+ Pair<List<ManifestFile>, Set<Long>> history =
+ validationHistory(
+ base,
+ startingSnapshotId,
+ VALIDATE_ADDED_DVS_OPERATIONS,
+ ManifestContent.DELETES,
+ parent);
+ List<ManifestFile> newDeleteManifests = history.first();
+ Set<Long> newSnapshotIds = history.second();
+
+ Tasks.foreach(newDeleteManifests)
+ .stopOnFailure()
+ .throwFailureWhenFinished()
+ .executeWith(workerPool())
+ .run(m -> validateAddedDVs(m, conflictDetectionFilter, newSnapshotIds, dvRefs));
+ }
+
+ private void validateAddedDVs(
+ ManifestFile manifest,
+ Expression conflictDetectionFilter,
+ Set<Long> newSnapshotIds,
+ Set<String> dvRefs) {
+ try (CloseableIterable<ManifestEntry<DeleteFile>> entries =
+ ManifestFiles.readDeleteManifest(manifest, ops.io(), ops.current().specsById())
+ .filterRows(conflictDetectionFilter)
+ .caseSensitive(caseSensitive)
+ .liveEntries()) {
+
+ for (ManifestEntry<DeleteFile> entry : entries) {
+ DeleteFile file = entry.file();
+ if (newSnapshotIds.contains(entry.snapshotId()) && ContentFileUtil.isDV(file)) {
+ ValidationException.check(
+ !dvRefs.contains(file.referencedDataFile()),
+ "Found concurrently added DV for %s: %s",
+ file.referencedDataFile(),
+ ContentFileUtil.dvDesc(file));
+ }
+ }
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ }
+
+ // builds a set of data file locations referenced by new DVs
+ private Set<String> dvRefs() {
Review Comment:
We need to have `dv` in the name. If so, I'd prefer a shorter var name to avoid splitting statements on multiple lines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835152614
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -245,13 +250,13 @@ private PartitionSpec spec(int specId) {
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
add(new PendingDeleteFile(file));
}
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file, long dataSequenceNumber) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
Review Comment:
It was another issue, should be OK, but I'll add more Spark tests around that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835155795
##########
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(
+ TableMetadata base,
+ Long startingSnapshotId,
+ Expression conflictDetectionFilter,
+ Snapshot parent) {
+ // skip if there is no current table state or table format doesn't support DVs
+ if (parent == null || base.formatVersion() < 3) {
+ return;
+ }
+
+ // skip if this operation doesn't add new DVs
+ Set<String> dvRefs = dvRefs();
+ if (dvRefs.isEmpty()) {
+ return;
+ }
+
+ Pair<List<ManifestFile>, Set<Long>> history =
+ validationHistory(
+ base,
+ startingSnapshotId,
+ VALIDATE_ADDED_DVS_OPERATIONS,
+ ManifestContent.DELETES,
+ parent);
+ List<ManifestFile> newDeleteManifests = history.first();
+ Set<Long> newSnapshotIds = history.second();
+
+ Tasks.foreach(newDeleteManifests)
+ .stopOnFailure()
+ .throwFailureWhenFinished()
+ .executeWith(workerPool())
+ .run(m -> validateAddedDVs(m, conflictDetectionFilter, newSnapshotIds, dvRefs));
+ }
+
+ private void validateAddedDVs(
+ ManifestFile manifest,
+ Expression conflictDetectionFilter,
+ Set<Long> newSnapshotIds,
+ Set<String> dvRefs) {
+ try (CloseableIterable<ManifestEntry<DeleteFile>> entries =
+ ManifestFiles.readDeleteManifest(manifest, ops.io(), ops.current().specsById())
+ .filterRows(conflictDetectionFilter)
+ .caseSensitive(caseSensitive)
+ .liveEntries()) {
+
+ for (ManifestEntry<DeleteFile> entry : entries) {
+ DeleteFile file = entry.file();
+ if (newSnapshotIds.contains(entry.snapshotId()) && ContentFileUtil.isDV(file)) {
+ ValidationException.check(
+ !dvRefs.contains(file.referencedDataFile()),
+ "Found concurrently added DV for %s: %s",
+ file.referencedDataFile(),
+ ContentFileUtil.dvDesc(file));
+ }
+ }
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ }
+
+ // builds a set of data file locations referenced by new DVs
+ private Set<String> dvRefs() {
Review Comment:
Should we call this `referencedDataFiles`?
##########
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(
+ TableMetadata base,
+ Long startingSnapshotId,
+ Expression conflictDetectionFilter,
+ Snapshot parent) {
+ // skip if there is no current table state or table format doesn't support DVs
+ if (parent == null || base.formatVersion() < 3) {
+ return;
+ }
+
+ // skip if this operation doesn't add new DVs
+ Set<String> dvRefs = dvRefs();
+ if (dvRefs.isEmpty()) {
+ return;
+ }
+
+ Pair<List<ManifestFile>, Set<Long>> history =
+ validationHistory(
+ base,
+ startingSnapshotId,
+ VALIDATE_ADDED_DVS_OPERATIONS,
+ ManifestContent.DELETES,
+ parent);
+ List<ManifestFile> newDeleteManifests = history.first();
+ Set<Long> newSnapshotIds = history.second();
+
+ Tasks.foreach(newDeleteManifests)
+ .stopOnFailure()
+ .throwFailureWhenFinished()
+ .executeWith(workerPool())
+ .run(m -> validateAddedDVs(m, conflictDetectionFilter, newSnapshotIds, dvRefs));
Review Comment:
Nit: Should we use the full word `manifest` in the lambda?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1837076611
##########
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 {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1836685282
##########
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);
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi merged PR #11495:
URL: https://github.com/apache/iceberg/pull/11495
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835613879
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -245,13 +250,13 @@ private PartitionSpec spec(int specId) {
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
add(new PendingDeleteFile(file));
}
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file, long dataSequenceNumber) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
Review Comment:
I added more testing around manifest rewrites is Spark.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1834857507
##########
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(
Review Comment:
This simply validates there is at most 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1836681746
##########
core/src/test/java/org/apache/iceberg/TestBase.java:
##########
@@ -643,6 +666,22 @@ protected DataFile newDataFile(String partitionPath) {
.build();
}
+ protected DeleteFile fileADeletes() {
+ return formatVersion >= 3 ? FILE_A_DV : FILE_A_DELETES;
+ }
+
+ protected DeleteFile fileBDeletes() {
+ return formatVersion >= 3 ? FILE_B_DV : FILE_B_DELETES;
+ }
+
+ protected DeleteFile newDeletes(DataFile dataFile) {
Review Comment:
Other `newDeleteFile` methods below return V2 position deletes, so I wanted to differentiate.
I am OK renaming too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1836701207
##########
core/src/test/java/org/apache/iceberg/TestBase.java:
##########
@@ -643,6 +666,22 @@ protected DataFile newDataFile(String partitionPath) {
.build();
}
+ protected DeleteFile fileADeletes() {
+ return formatVersion >= 3 ? FILE_A_DV : FILE_A_DELETES;
+ }
+
+ protected DeleteFile fileBDeletes() {
+ return formatVersion >= 3 ? FILE_B_DV : FILE_B_DELETES;
+ }
+
+ protected DeleteFile newDeletes(DataFile dataFile) {
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1837104323
##########
core/src/test/java/org/apache/iceberg/TestBase.java:
##########
@@ -643,6 +666,22 @@ protected DataFile newDataFile(String partitionPath) {
.build();
}
+ protected DeleteFile fileADeletes() {
+ return formatVersion >= 3 ? FILE_A_DV : FILE_A_DELETES;
+ }
+
+ protected DeleteFile fileBDeletes() {
+ return formatVersion >= 3 ? FILE_B_DV : FILE_B_DELETES;
+ }
+
+ protected DeleteFile newDeletes(DataFile dataFile) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835329015
##########
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()) {
Review Comment:
Do we really need the format version here ? Why not just using `addsDeleteFiles` flag ? (just wondering).
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -245,13 +250,13 @@ private PartitionSpec spec(int specId) {
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
add(new PendingDeleteFile(file));
}
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file, long dataSequenceNumber) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
Review Comment:
Thanks @aokolnychyi for the background. It wasn't clear to me the purpose of `validateNewDeleteFile()`during `add`.
##########
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(
+ TableMetadata base,
+ Long startingSnapshotId,
+ Expression conflictDetectionFilter,
+ Snapshot parent) {
+ // skip if there is no current table state or table format doesn't support DVs
+ if (parent == null || base.formatVersion() < 3) {
+ return;
+ }
+
+ // skip if this operation doesn't add new DVs
+ Set<String> dvRefs = dvRefs();
+ if (dvRefs.isEmpty()) {
+ return;
+ }
+
+ Pair<List<ManifestFile>, Set<Long>> history =
+ validationHistory(
+ base,
+ startingSnapshotId,
+ VALIDATE_ADDED_DVS_OPERATIONS,
+ ManifestContent.DELETES,
+ parent);
+ List<ManifestFile> newDeleteManifests = history.first();
+ Set<Long> newSnapshotIds = history.second();
+
+ Tasks.foreach(newDeleteManifests)
+ .stopOnFailure()
+ .throwFailureWhenFinished()
+ .executeWith(workerPool())
+ .run(m -> validateAddedDVs(m, conflictDetectionFilter, newSnapshotIds, dvRefs));
Review Comment:
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 😃
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -271,6 +276,26 @@ private void add(PendingDeleteFile file) {
}
}
+ protected void validateNewDeleteFile(DeleteFile file) {
+ Preconditions.checkNotNull(file, "Invalid delete file: null");
+ Preconditions.checkArgument(formatVersion() >= 2, "Deletes are supported in V2 and above");
+ if (file.content() == FileContent.POSITION_DELETES) {
+ Preconditions.checkArgument(
+ formatVersion() != 2 || !ContentFileUtil.isDV(file),
+ "Must not use DVs for position deletes in V2: %s",
+ ContentFileUtil.dvDesc(file));
+ Preconditions.checkArgument(
+ formatVersion() == 2 || ContentFileUtil.isDV(file),
Review Comment:
Same question here, why not just checking if it's DV: we know that if it's DV it's format version 3+ right ?
##########
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(
+ TableMetadata base,
+ Long startingSnapshotId,
+ Expression conflictDetectionFilter,
+ Snapshot parent) {
+ // skip if there is no current table state or table format doesn't support DVs
+ if (parent == null || base.formatVersion() < 3) {
+ return;
+ }
+
+ // skip if this operation doesn't add new DVs
+ Set<String> dvRefs = dvRefs();
+ if (dvRefs.isEmpty()) {
+ return;
+ }
+
+ Pair<List<ManifestFile>, Set<Long>> history =
+ validationHistory(
+ base,
+ startingSnapshotId,
+ VALIDATE_ADDED_DVS_OPERATIONS,
+ ManifestContent.DELETES,
+ parent);
+ List<ManifestFile> newDeleteManifests = history.first();
+ Set<Long> newSnapshotIds = history.second();
+
+ Tasks.foreach(newDeleteManifests)
+ .stopOnFailure()
+ .throwFailureWhenFinished()
+ .executeWith(workerPool())
+ .run(m -> validateAddedDVs(m, conflictDetectionFilter, newSnapshotIds, dvRefs));
+ }
+
+ private void validateAddedDVs(
+ ManifestFile manifest,
+ Expression conflictDetectionFilter,
+ Set<Long> newSnapshotIds,
+ Set<String> dvRefs) {
+ try (CloseableIterable<ManifestEntry<DeleteFile>> entries =
+ ManifestFiles.readDeleteManifest(manifest, ops.io(), ops.current().specsById())
+ .filterRows(conflictDetectionFilter)
+ .caseSensitive(caseSensitive)
+ .liveEntries()) {
+
+ for (ManifestEntry<DeleteFile> entry : entries) {
+ DeleteFile file = entry.file();
+ if (newSnapshotIds.contains(entry.snapshotId()) && ContentFileUtil.isDV(file)) {
+ ValidationException.check(
+ !dvRefs.contains(file.referencedDataFile()),
+ "Found concurrently added DV for %s: %s",
+ file.referencedDataFile(),
+ ContentFileUtil.dvDesc(file));
+ }
+ }
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ }
+
+ // builds a set of data file locations referenced by new DVs
+ private Set<String> dvRefs() {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835613811
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -271,6 +276,26 @@ private void add(PendingDeleteFile file) {
}
}
+ protected void validateNewDeleteFile(DeleteFile file) {
+ Preconditions.checkNotNull(file, "Invalid delete file: null");
+ Preconditions.checkArgument(formatVersion() >= 2, "Deletes are supported in V2 and above");
+ if (file.content() == FileContent.POSITION_DELETES) {
+ Preconditions.checkArgument(
+ formatVersion() != 2 || !ContentFileUtil.isDV(file),
+ "Must not use DVs for position deletes in V2: %s",
+ ContentFileUtil.dvDesc(file));
+ Preconditions.checkArgument(
+ formatVersion() == 2 || ContentFileUtil.isDV(file),
Review Comment:
This method validates input from the user and ensures it is not possible to add a DV to a V2 table.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835152614
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -245,13 +250,13 @@ private PartitionSpec spec(int specId) {
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
add(new PendingDeleteFile(file));
}
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file, long dataSequenceNumber) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
Review Comment:
It was another issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1836687170
##########
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);
Review Comment:
Awesome, will take a look!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835613879
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -245,13 +250,13 @@ private PartitionSpec spec(int specId) {
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
add(new PendingDeleteFile(file));
}
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file, long dataSequenceNumber) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
Review Comment:
I added more testing around manifest rewrites in Spark.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1837091098
##########
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 {
Review Comment:
I agree the name is too generic. I did cover adding a DV to an existing file with positional delete in a previous PR [here](https://github.com/apache/iceberg/pull/11467/files#diff-f7cf300e48afd586e158fba64c058d30d14d37b3e96739d6cca23758ee444593R633)).
##########
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 {
Review Comment:
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](https://github.com/apache/iceberg/pull/11467/files#diff-f7cf300e48afd586e158fba64c058d30d14d37b3e96739d6cca23758ee444593R633)).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1837092881
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -268,9 +274,32 @@ private void add(PendingDeleteFile file) {
if (deleteFiles.add(file)) {
addedFilesSummary.addedFile(spec, file);
hasNewDeleteFiles = true;
+ if (ContentFileUtil.isDV(file)) {
+ newDVRefs.add(file.referencedDataFile());
+ }
+ }
+ }
+
+ protected void validateNewDeleteFile(DeleteFile file) {
+ Preconditions.checkNotNull(file, "Invalid delete file: null");
+ Preconditions.checkArgument(formatVersion() >= 2, "Deletes are supported in V2 and above");
+ if (file.content() == FileContent.POSITION_DELETES) {
+ Preconditions.checkArgument(
+ formatVersion() != 2 || !ContentFileUtil.isDV(file),
Review Comment:
Wouldn't it fail for V3 tables, though?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835114428
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -245,13 +250,13 @@ private PartitionSpec spec(int specId) {
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
add(new PendingDeleteFile(file));
}
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file, long dataSequenceNumber) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
Review Comment:
This only affects new delete files that are added to the table. I had a test in the prototype PR that verified that automatic manifest compaction after upgrading to V3 didn't have issues. Let me check if I added that test to this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1836698961
##########
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);
Review Comment:
ah sorry I forgot that you already added `testFileSizeSummaryWithDVs`, so this LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1836738741
##########
core/src/test/java/org/apache/iceberg/TestBase.java:
##########
@@ -643,6 +666,22 @@ protected DataFile newDataFile(String partitionPath) {
.build();
}
+ protected DeleteFile fileADeletes() {
+ return formatVersion >= 3 ? FILE_A_DV : FILE_A_DELETES;
+ }
+
+ protected DeleteFile fileBDeletes() {
+ return formatVersion >= 3 ? FILE_B_DV : FILE_B_DELETES;
+ }
+
+ protected DeleteFile newDeletes(DataFile dataFile) {
Review Comment:
I have no preference, to be honest. I'll update in the next round.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835152614
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -245,13 +250,13 @@ private PartitionSpec spec(int specId) {
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
add(new PendingDeleteFile(file));
}
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file, long dataSequenceNumber) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
Review Comment:
It was another issue, should be OK, but I'll add more Spark tests around that to be safe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835104546
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -245,13 +250,13 @@ private PartitionSpec spec(int specId) {
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
add(new PendingDeleteFile(file));
}
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file, long dataSequenceNumber) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
Review Comment:
I'm not quite clear if there's a case if a manifest rewrite could cause an issue here. Is it possible that just rewriting delete manifests as part of incremental maintenance would hit the table version check? I don't feel like we can require delete file rewrite as part of that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1836205922
##########
core/src/test/java/org/apache/iceberg/TestBase.java:
##########
@@ -643,6 +666,22 @@ protected DataFile newDataFile(String partitionPath) {
.build();
}
+ protected DeleteFile fileADeletes() {
+ return formatVersion >= 3 ? FILE_A_DV : FILE_A_DELETES;
+ }
+
+ protected DeleteFile fileBDeletes() {
+ return formatVersion >= 3 ? FILE_B_DV : FILE_B_DELETES;
+ }
+
+ protected DeleteFile newDeletes(DataFile dataFile) {
Review Comment:
maybe `newDeleteFile`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#issuecomment-2469024997
Thanks for reviewing, @nastra @danielcweeks @jbonofre @amogh-jahagirdar!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835144928
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -245,13 +250,13 @@ private PartitionSpec spec(int specId) {
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
add(new PendingDeleteFile(file));
}
/** Add a delete file to the new snapshot. */
protected void add(DeleteFile file, long dataSequenceNumber) {
- Preconditions.checkNotNull(file, "Invalid delete file: null");
+ validateNewDeleteFile(file);
Review Comment:
Yep, it is [here](https://github.com/apache/iceberg/pull/11495/files#diff-410ff1b47d9a44a2fd5dbd103cad9463d82c8f4f51aa1be63b8b403123ab6e0eR1571) (`testUpgradeToV3` in `TestRowDelta`). Let me know if that's what you meant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835614576
##########
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(
+ TableMetadata base,
+ Long startingSnapshotId,
+ Expression conflictDetectionFilter,
+ Snapshot parent) {
+ // skip if there is no current table state or table format doesn't support DVs
+ if (parent == null || base.formatVersion() < 3) {
+ return;
+ }
+
+ // skip if this operation doesn't add new DVs
+ Set<String> dvRefs = dvRefs();
+ if (dvRefs.isEmpty()) {
+ return;
+ }
+
+ Pair<List<ManifestFile>, Set<Long>> history =
+ validationHistory(
+ base,
+ startingSnapshotId,
+ VALIDATE_ADDED_DVS_OPERATIONS,
+ ManifestContent.DELETES,
+ parent);
+ List<ManifestFile> newDeleteManifests = history.first();
+ Set<Long> newSnapshotIds = history.second();
+
+ Tasks.foreach(newDeleteManifests)
+ .stopOnFailure()
+ .throwFailureWhenFinished()
+ .executeWith(workerPool())
+ .run(m -> validateAddedDVs(m, conflictDetectionFilter, newSnapshotIds, dvRefs));
Review Comment:
It was me trying to work-around Spotless formatting for closures. I updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Support commits with DVs [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11495:
URL: https://github.com/apache/iceberg/pull/11495#discussion_r1835614940
##########
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()) {
Review Comment:
I've restructured this part a bit to compute dv refs as deletes are being added. No longer applies.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org