You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "adawrapub (via GitHub)" <gi...@apache.org> on 2025/09/30 22:06:29 UTC
[PR] Preserve DV-specific fields for deletion vectors [iceberg]
adawrapub opened a new pull request, #14226:
URL: https://github.com/apache/iceberg/pull/14226
This is to fix problem reported in https://github.com/apache/iceberg/issues/13671 by preserving DV specific fields
cc @anuragmantri
--
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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14226:
URL: https://github.com/apache/iceberg/pull/14226#discussion_r2394727722
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -449,12 +449,25 @@ private static RewriteResult<DeleteFile> writeDeleteFileEntry(
String targetDeleteFilePath = newPath(file.location(), sourcePrefix, targetPrefix);
Metrics metricsWithTargetPath =
ContentFileUtil.replacePathBounds(file, sourcePrefix, targetPrefix);
- DeleteFile movedFile =
+ FileMetadata.Builder builder =
FileMetadata.deleteFileBuilder(spec)
.copy(file)
.withPath(targetDeleteFilePath)
- .withMetrics(metricsWithTargetPath)
- .build();
+ .withMetrics(metricsWithTargetPath);
+
+ // Preserve DV-specific fields for deletion vectors
+ if (file.referencedDataFile() != null) {
Review Comment:
+1, copy should preserve the referenced data file, the offset and size in bytes.
--
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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #14226:
URL: https://github.com/apache/iceberg/pull/14226#issuecomment-3354917294
Ideally we want to be able to test this as well by running the tests that @huaxingao mentioned with v2 and v3, which effectively means we'll need to solve #13671. See also my [comment](https://github.com/apache/iceberg/issues/13671#issuecomment-3130889106) about the other issue you'll be running into when executing the tests with v3 that we need to fix as part of this
--
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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14226:
URL: https://github.com/apache/iceberg/pull/14226#discussion_r2409729868
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java:
##########
@@ -354,8 +382,10 @@ public void testManifestRewriteAndIncrementalCopy() throws Exception {
checkFileNum(1, 1, addedManifest, 3, postReweiteResult);
}
- @Test
- public void testDeleteDataFile() throws Exception {
+ @ParameterizedTest
Review Comment:
rather than parameterizing every single method, we typically parameterize at the class level by using `@ExtendWith(ParameterizedTestExtension.class)`. You can take at a few other test classes that already do this, such as `TestRewritePositionDeleteFilesAction`
--
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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14226:
URL: https://github.com/apache/iceberg/pull/14226#discussion_r2409732885
##########
core/src/main/java/org/apache/iceberg/FileMetadata.java:
##########
@@ -109,6 +109,11 @@ public Builder copy(DeleteFile toCopy) {
this.keyMetadata =
toCopy.keyMetadata() == null ? null : ByteBuffers.copy(toCopy.keyMetadata());
this.sortOrderId = toCopy.sortOrderId();
+ this.splitOffsets = toCopy.splitOffsets();
+ // Preserve DV-specific fields for deletion vectors
+ this.referencedDataFile = toCopy.referencedDataFile();
+ this.contentOffset = toCopy.contentOffset();
+ this.contentSizeInBytes = toCopy.contentSizeInBytes();
Review Comment:
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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14226:
URL: https://github.com/apache/iceberg/pull/14226#discussion_r2409715269
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java:
##########
@@ -111,10 +114,23 @@ protected ActionsProvider actions() {
@BeforeEach
public void setupTableLocation() {
this.tableLocation = tableDir.toFile().toURI().toString();
- this.table = createATableWith2Snapshots(tableLocation);
createNameSpaces();
}
+ protected void setupTable() {
+ this.table = createATableWith2Snapshots(tableLocation);
+ }
+
+ protected void setupTableWithFormatVersion(int version) {
Review Comment:
these methods aren't needed because there's already a way to pass the properties when creating a table. Personally I would remove `createATableWith2Snapshots()` and use `createTableWithSnapshots()`, which takes a properties map. Therefore you could just pass the properties in
--
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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #14226:
URL: https://github.com/apache/iceberg/pull/14226#discussion_r2394727722
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -449,12 +449,25 @@ private static RewriteResult<DeleteFile> writeDeleteFileEntry(
String targetDeleteFilePath = newPath(file.location(), sourcePrefix, targetPrefix);
Metrics metricsWithTargetPath =
ContentFileUtil.replacePathBounds(file, sourcePrefix, targetPrefix);
- DeleteFile movedFile =
+ FileMetadata.Builder builder =
FileMetadata.deleteFileBuilder(spec)
.copy(file)
.withPath(targetDeleteFilePath)
- .withMetrics(metricsWithTargetPath)
- .build();
+ .withMetrics(metricsWithTargetPath);
+
+ // Preserve DV-specific fields for deletion vectors
+ if (file.referencedDataFile() != null) {
Review Comment:
+1, copy should preserve the referenced data 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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14226:
URL: https://github.com/apache/iceberg/pull/14226#discussion_r2393562390
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -449,12 +449,25 @@ private static RewriteResult<DeleteFile> writeDeleteFileEntry(
String targetDeleteFilePath = newPath(file.location(), sourcePrefix, targetPrefix);
Metrics metricsWithTargetPath =
ContentFileUtil.replacePathBounds(file, sourcePrefix, targetPrefix);
- DeleteFile movedFile =
+ FileMetadata.Builder builder =
FileMetadata.deleteFileBuilder(spec)
.copy(file)
.withPath(targetDeleteFilePath)
- .withMetrics(metricsWithTargetPath)
- .build();
+ .withMetrics(metricsWithTargetPath);
+
+ // Preserve DV-specific fields for deletion vectors
+ if (file.referencedDataFile() != null) {
Review Comment:
instead of doing this here a better approach would be to carry this over in the `copy()` method. See also https://github.com/apache/iceberg/issues/13671
--
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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14226:
URL: https://github.com/apache/iceberg/pull/14226#discussion_r2409718738
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java:
##########
@@ -159,8 +175,10 @@ private void dropNameSpaces() {
sql("DROP DATABASE IF EXISTS %s CASCADE", backupNs);
}
- @Test
- public void testRewritePath() throws Exception {
+ @ParameterizedTest
+ @ValueSource(ints = {2, 3})
+ public void testRewritePath(int version) throws Exception {
+ setupTableWithFormatVersion(version);
Review Comment:
you can just call `createTableWithSnapshots(...)` here as there is a version that takes a properties map
--
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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #14226:
URL: https://github.com/apache/iceberg/pull/14226#issuecomment-3354764454
@adawrapub Thanks for the PR! To verify the fix, could you parameterize `TestRewriteTablePathsAction` to run against multiple table format versions (2 and 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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14226:
URL: https://github.com/apache/iceberg/pull/14226#discussion_r2409705850
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java:
##########
@@ -111,10 +114,23 @@ protected ActionsProvider actions() {
@BeforeEach
public void setupTableLocation() {
this.tableLocation = tableDir.toFile().toURI().toString();
- this.table = createATableWith2Snapshots(tableLocation);
createNameSpaces();
}
+ protected void setupTable() {
+ this.table = createATableWith2Snapshots(tableLocation);
+ }
+
+ protected void setupTableWithFormatVersion(int version) {
+ this.table = createTableWithSnapshots(tableLocation, 2, getPropertiesMap(version));
+ }
+
+ private @NotNull Map<String, String> getPropertiesMap(int version) {
Review Comment:
```suggestion
private Map<String, String> properties(int formatVersion) {
```
--
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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "adawrapub (via GitHub)" <gi...@apache.org>.
adawrapub closed pull request #14226: Preserve DV-specific fields for deletion vectors
URL: https://github.com/apache/iceberg/pull/14226
--
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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14226:
URL: https://github.com/apache/iceberg/pull/14226#discussion_r2409732014
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -449,12 +449,13 @@ private static RewriteResult<DeleteFile> writeDeleteFileEntry(
String targetDeleteFilePath = newPath(file.location(), sourcePrefix, targetPrefix);
Metrics metricsWithTargetPath =
ContentFileUtil.replacePathBounds(file, sourcePrefix, targetPrefix);
- DeleteFile movedFile =
+ FileMetadata.Builder builder =
Review Comment:
I don't think these changes are necessary here, so these can be reverted
--
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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "adawrapub (via GitHub)" <gi...@apache.org>.
adawrapub commented on code in PR #14226:
URL: https://github.com/apache/iceberg/pull/14226#discussion_r2407284218
##########
core/src/main/java/org/apache/iceberg/RewriteTablePathUtil.java:
##########
@@ -449,12 +449,25 @@ private static RewriteResult<DeleteFile> writeDeleteFileEntry(
String targetDeleteFilePath = newPath(file.location(), sourcePrefix, targetPrefix);
Metrics metricsWithTargetPath =
ContentFileUtil.replacePathBounds(file, sourcePrefix, targetPrefix);
- DeleteFile movedFile =
+ FileMetadata.Builder builder =
FileMetadata.deleteFileBuilder(spec)
.copy(file)
.withPath(targetDeleteFilePath)
- .withMetrics(metricsWithTargetPath)
- .build();
+ .withMetrics(metricsWithTargetPath);
+
+ // Preserve DV-specific fields for deletion vectors
+ if (file.referencedDataFile() != null) {
Review Comment:
Thanks @nastra and @amogh-jahagirdar. Moved changes to copy method. Please let me know what you think
--
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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "adawrapub (via GitHub)" <gi...@apache.org>.
adawrapub commented on PR #14226:
URL: https://github.com/apache/iceberg/pull/14226#issuecomment-3372604220
> @adawrapub Thanks for the PR! To verify the fix, could you parameterize `TestRewriteTablePathsAction` to run against multiple table format versions (2 and 3)?
Thanks @huaxingao and @ebyhr for the review. Added test cases. Please let me know if it looks okay to you or have suggestions on how to improve it.
--
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] Preserve DV-specific fields for deletion vectors [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #14226:
URL: https://github.com/apache/iceberg/pull/14226#discussion_r2409702521
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewriteTablePathsAction.java:
##########
@@ -78,10 +78,13 @@
import org.apache.spark.storage.BlockInfoManager;
import org.apache.spark.storage.BlockManager;
import org.apache.spark.storage.BroadcastBlockId;
+import org.jetbrains.annotations.NotNull;
Review Comment:
this import shouldn't be allowed/available anymore once you rebase
--
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