You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "RussellSpitzer (via GitHub)" <gi...@apache.org> on 2025/04/21 19:39:52 UTC
[PR] API, Core: Add deleteFile to RowDelta API [iceberg]
RussellSpitzer opened a new pull request, #12861:
URL: https://github.com/apache/iceberg/pull/12861
(no comment)
--
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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#discussion_r2054449713
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -140,6 +149,11 @@ protected void validate(TableMetadata base, Snapshot parent) {
validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
+ if (!deletedDataFiles.isEmpty()) {
+ validateNoNewDeletesForDataFiles(
+ base, startingSnapshotId, conflictDetectionFilter, deletedDataFiles, parent);
+ }
Review Comment:
[doubt] Should we add the deletedDataFiles for existence checks implicitly ? as if these files which are deleted need to be in table or are we expecting the caller of this API to re-use `validateDataFilesExist` to catch this case ?
```java
@TestTemplate
public void testValidateFileDeleteAndRowDeleteWithNonExistentFileRemoved() {
commit(table, table.newAppend().appendFile(FILE_A).appendFile(FILE_B), branch);
long initialCommit = latestSnapshot(table, branch).snapshotId();
// compaction re-wrote this file.
commit(table, table.newRewrite().addFile(FILE_C).deleteFile(FILE_B), branch);
commit(
table,
table
.newRowDelta()
.addDeletes(fileADeletes())
.deleteFile(FILE_B)
.validateFromSnapshot(initialCommit)
// Note: we are not explicitly checking the FILE_B existence
// so this pass.
.validateDataFilesExist(ImmutableList.of(FILE_A.location())),
branch);
}
```
OR
```java
@TestTemplate
public void testValidateFileDeleteAndRowDeleteWithNonExistentFileRemoved() {
commit(table, table.newAppend().appendFile(FILE_A).appendFile(FILE_B), branch);
long initialCommit = latestSnapshot(table, branch).snapshotId();
// compaction re-wrote this file.
commit(table, table.newRewrite().addFile(FILE_C).deleteFile(FILE_B), branch);
assertThatThrownBy(
() -> {
commit(
table,
table
.newRowDelta()
.addDeletes(fileADeletes())
.deleteFile(FILE_B)
.validateFromSnapshot(initialCommit)
// Note: we are not explicitly checking the FILE_B existence
// so this pass.
.validateDataFilesExist(ImmutableList.of(FILE_A.location(), FILE_B.location())),
branch);
})
.isInstanceOf(ValidationException.class)
.hasMessageContaining("Cannot commit, missing data files: [" + FILE_B.location() + "]");
}
```
--
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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#discussion_r2054449713
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -140,6 +149,11 @@ protected void validate(TableMetadata base, Snapshot parent) {
validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
+ if (!deletedDataFiles.isEmpty()) {
+ validateNoNewDeletesForDataFiles(
+ base, startingSnapshotId, conflictDetectionFilter, deletedDataFiles, parent);
+ }
Review Comment:
[doubt] Should we add the deletedDataFiles for existence checks implicitly ? as if these files which are deleted need to be in table or are we expecting the caller of this API to re-use `validateDataFilesExist` to catch this case ?
```java
@TestTemplate
public void testValidateFileDeleteAndRowDeleteWithNonExistentFileRemoved() {
commit(table, table.newAppend().appendFile(FILE_A).appendFile(FILE_B), branch);
long initialCommit = latestSnapshot(table, branch).snapshotId();
// compaction re-wrote this file.
commit(table, table.newRewrite().addFile(FILE_C).deleteFile(FILE_B), branch);
commit(
table,
table
.newRowDelta()
.addDeletes(fileADeletes())
.deleteFile(FILE_B)
.validateFromSnapshot(initialCommit)
// Note: we are not explicitly checking the FILE_B existence
// so this pass.
.validateDataFilesExist(ImmutableList.of(FILE_A.location())),
branch);
}
```
OR
```java
@TestTemplate
public void testValidateFileDeleteAndRowDeleteWithNonExistentFileRemoved() {
commit(table, table.newAppend().appendFile(FILE_A).appendFile(FILE_B), branch);
long initialCommit = latestSnapshot(table, branch).snapshotId();
// compaction re-wrote this file.
commit(table, table.newRewrite().addFile(FILE_C).deleteFile(FILE_B), branch);
assertThatThrownBy(
() -> {
commit(
table,
table
.newRowDelta()
.addDeletes(fileADeletes())
.deleteFile(FILE_B)
.validateFromSnapshot(initialCommit)
// Note: we are explicitly checking the FILE_B existence
// so this pass as it expected to throw.
.validateDataFilesExist(ImmutableList.of(FILE_A.location(), FILE_B.location())),
branch);
})
.isInstanceOf(ValidationException.class)
.hasMessageContaining("Cannot commit, missing data files: [" + FILE_B.location() + "]");
}
```
--
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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#discussion_r2054683442
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -140,6 +149,11 @@ protected void validate(TableMetadata base, Snapshot parent) {
validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
+ if (!deletedDataFiles.isEmpty()) {
+ validateNoNewDeletesForDataFiles(
+ base, startingSnapshotId, conflictDetectionFilter, deletedDataFiles, parent);
+ }
Review Comment:
I changed this so now you can either use
.validateDeletedFiles() // Checks that paths exist for all deleted files
or
.validateDataFilesExist() // Explictly checking for
I also added a validation that you are not simultaneously deleting a file while also adding a DeleteFile for that 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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#discussion_r2074185204
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -132,15 +144,44 @@ protected void validate(TableMetadata base, Snapshot parent) {
parent);
}
+ if (validateDeletes) {
+ failMissingDeletePaths();
+ }
+
if (validateNewDataFiles) {
validateAddedDataFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
if (validateNewDeleteFiles) {
+ // validate that explicitly deleted files have not had added deletes
+ if (!deletedDataFiles.isEmpty()) {
+ validateNoNewDeletesForDataFiles(
+ base, startingSnapshotId, conflictDetectionFilter, deletedDataFiles, parent);
+ }
+
+ // validate that previous deletes do not conflict with added deletes
validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
+ validateNoSimultaneousForDeletedFiles();
+
validateAddedDVs(base, startingSnapshotId, conflictDetectionFilter, parent);
}
}
+
+ /** Validates that the data files removed in this commit do not overlap with delete files added */
Review Comment:
I am OK keeping it. Maybe rename the method? I think it only covers concurrent position deletes.
--
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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#discussion_r2054449713
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -140,6 +149,11 @@ protected void validate(TableMetadata base, Snapshot parent) {
validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
+ if (!deletedDataFiles.isEmpty()) {
+ validateNoNewDeletesForDataFiles(
+ base, startingSnapshotId, conflictDetectionFilter, deletedDataFiles, parent);
+ }
Review Comment:
[doubt] Should we add the deletedDataFiles for existence checks implicitly ? as if these files which are deleted need to be in table or are we expecting the caller of this API to re-use `validateDataFilesExist` to catch this case ?
```java
@TestTemplate
public void testValidateFileDeleteAndRowDeleteWithNonExistentFileRemoved() {
commit(table, table.newAppend().appendFile(FILE_A).appendFile(FILE_B), branch);
long initialCommit = latestSnapshot(table, branch).snapshotId();
// compaction re-wrote this file.
commit(table, table.newRewrite().addFile(FILE_C).deleteFile(FILE_B), branch);
commit(
table,
table
.newRowDelta()
.addDeletes(fileADeletes())
.deleteFile(FILE_B)
.validateFromSnapshot(initialCommit)
// Note: we are not explicitly checking the FILE_B existence
// so this pass, so we should do implicit existence check then ?
.validateDataFilesExist(ImmutableList.of(FILE_A.location())),
branch);
}
```
OR
```java
@TestTemplate
public void testValidateFileDeleteAndRowDeleteWithNonExistentFileRemoved() {
commit(table, table.newAppend().appendFile(FILE_A).appendFile(FILE_B), branch);
long initialCommit = latestSnapshot(table, branch).snapshotId();
// compaction re-wrote this file.
commit(table, table.newRewrite().addFile(FILE_C).deleteFile(FILE_B), branch);
assertThatThrownBy(
() -> {
commit(
table,
table
.newRowDelta()
.addDeletes(fileADeletes())
.deleteFile(FILE_B)
.validateFromSnapshot(initialCommit)
// Note: we are explicitly checking the FILE_B existence
// so this pass as it expected to throw.
.validateDataFilesExist(ImmutableList.of(FILE_A.location(), FILE_B.location())),
branch);
})
.isInstanceOf(ValidationException.class)
.hasMessageContaining("Cannot commit, missing data files: [" + FILE_B.location() + "]");
}
```
--
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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#discussion_r2074114731
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -132,15 +144,44 @@ protected void validate(TableMetadata base, Snapshot parent) {
parent);
}
+ if (validateDeletes) {
+ failMissingDeletePaths();
+ }
+
if (validateNewDataFiles) {
validateAddedDataFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
if (validateNewDeleteFiles) {
+ // validate that explicitly deleted files have not had added deletes
+ if (!deletedDataFiles.isEmpty()) {
Review Comment:
Yes, our file delete is essentially again being treated as a "position delete" that effects the entire 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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#discussion_r2074100873
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -132,15 +144,44 @@ protected void validate(TableMetadata base, Snapshot parent) {
parent);
}
+ if (validateDeletes) {
+ failMissingDeletePaths();
+ }
+
if (validateNewDataFiles) {
validateAddedDataFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
if (validateNewDeleteFiles) {
+ // validate that explicitly deleted files have not had added deletes
+ if (!deletedDataFiles.isEmpty()) {
+ validateNoNewDeletesForDataFiles(
+ base, startingSnapshotId, conflictDetectionFilter, deletedDataFiles, parent);
+ }
+
+ // validate that previous deletes do not conflict with added deletes
validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
+ validateNoSimultaneousForDeletedFiles();
+
validateAddedDVs(base, startingSnapshotId, conflictDetectionFilter, parent);
}
}
+
+ /** Validates that the data files removed in this commit do not overlap with delete files added */
Review Comment:
I am not sure I understand here. What's the use case?
--
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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#discussion_r2074113475
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -132,15 +144,44 @@ protected void validate(TableMetadata base, Snapshot parent) {
parent);
}
+ if (validateDeletes) {
Review Comment:
Yes, the doc is in RowDelta (although under validateDeletedFiles)
```
/**
* Enable validation that referenced data files passed to {@link
* #validateDataFilesExist(Iterable)} have not been removed by a delete operation.
*
* <p>If a data file has a row deleted using a position delete file, rewriting or overwriting the
* data file concurrently would un-delete the row. Deleting the data file is normally allowed, but
* a delete may be part of a transaction that reads and re-appends a row. This method is used to
* validate deletes for the transaction case.
*
* @return this for method chaining
*/
```
This usage is similar to adding a position delete for a data file but instead of removing a singular row we are attempting to remove every row in the file. The way I have been thinking of this is point deletes and file deletes. Everything we do for the point deletes we must also do for the file deletes.
--
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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#discussion_r2054449713
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -140,6 +149,11 @@ protected void validate(TableMetadata base, Snapshot parent) {
validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
+ if (!deletedDataFiles.isEmpty()) {
+ validateNoNewDeletesForDataFiles(
+ base, startingSnapshotId, conflictDetectionFilter, deletedDataFiles, parent);
+ }
Review Comment:
[doubt] Should we add the deletedDataFiles for existence checks ? as if these files which are deleted need to be in table or are we expecting the caller of this API to re-use `validateDataFilesExist` to catch this case ?
```java
@TestTemplate
public void testValidateFileDeleteAndRowDeleteWithNonExistentFileRemoved() {
commit(table, table.newAppend().appendFile(FILE_A).appendFile(FILE_B), branch);
long initialCommit = latestSnapshot(table, branch).snapshotId();
// compaction re-wrote this file.
commit(table, table.newRewrite().addFile(FILE_C).deleteFile(FILE_B), branch);
commit(
table,
table
.newRowDelta()
.addDeletes(fileADeletes())
.deleteFile(FILE_B)
.validateFromSnapshot(initialCommit)
// Note: we are not explicitly checking the FILE_B existence
// so this pass.
.validateDataFilesExist(ImmutableList.of(FILE_A.location())),
branch);
}
```
OR
```java
@TestTemplate
public void testValidateFileDeleteAndRowDeleteWithNonExistentFileRemoved() {
commit(table, table.newAppend().appendFile(FILE_A).appendFile(FILE_B), branch);
long initialCommit = latestSnapshot(table, branch).snapshotId();
// compaction re-wrote this file.
commit(table, table.newRewrite().addFile(FILE_C).deleteFile(FILE_B), branch);
assertThatThrownBy(
() -> {
commit(
table,
table
.newRowDelta()
.addDeletes(fileADeletes())
.deleteFile(FILE_B)
.validateFromSnapshot(initialCommit)
// Note: we are not explicitly checking the FILE_B existence
// so this pass.
.validateDataFilesExist(ImmutableList.of(FILE_A.location(), FILE_B.location())),
branch);
})
.isInstanceOf(ValidationException.class)
.hasMessageContaining("Cannot commit, missing data files: [" + FILE_B.location() + "]");
}
```
--
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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer merged PR #12861:
URL: https://github.com/apache/iceberg/pull/12861
--
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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#issuecomment-2828568406
@aokolnychyi Please 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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#discussion_r2074117005
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -132,15 +144,44 @@ protected void validate(TableMetadata base, Snapshot parent) {
parent);
}
+ if (validateDeletes) {
+ failMissingDeletePaths();
+ }
+
if (validateNewDataFiles) {
validateAddedDataFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
if (validateNewDeleteFiles) {
+ // validate that explicitly deleted files have not had added deletes
+ if (!deletedDataFiles.isEmpty()) {
+ validateNoNewDeletesForDataFiles(
+ base, startingSnapshotId, conflictDetectionFilter, deletedDataFiles, parent);
+ }
+
+ // validate that previous deletes do not conflict with added deletes
validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
+ validateNoSimultaneousForDeletedFiles();
+
validateAddedDVs(base, startingSnapshotId, conflictDetectionFilter, parent);
}
}
+
+ /** Validates that the data files removed in this commit do not overlap with delete files added */
Review Comment:
This was really just a sanity check. Forbids a commit where you attempt to add a position delete/equality delete for a file which you are also completely removing. I can remove this but I am assuming that an implementation is doing something wrong if it attempts to delete FILEA and also add a DV for FILEA at the same time.
--
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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#issuecomment-2855151885
Merging, Thanks for the review @sn2479 , @singhpk234 and @aokolnychyi !
--
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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#discussion_r2054463539
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -140,6 +149,11 @@ protected void validate(TableMetadata base, Snapshot parent) {
validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
+ if (!deletedDataFiles.isEmpty()) {
+ validateNoNewDeletesForDataFiles(
+ base, startingSnapshotId, conflictDetectionFilter, deletedDataFiles, parent);
+ }
Review Comment:
Hmmm I think it should be implicit here because the removal of files should be according to the conflict detection filter. I think it should look just like OverwriteFIles
--
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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#discussion_r2074096240
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -132,15 +144,44 @@ protected void validate(TableMetadata base, Snapshot parent) {
parent);
}
+ if (validateDeletes) {
+ failMissingDeletePaths();
+ }
+
if (validateNewDataFiles) {
validateAddedDataFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
if (validateNewDeleteFiles) {
+ // validate that explicitly deleted files have not had added deletes
+ if (!deletedDataFiles.isEmpty()) {
Review Comment:
This block simply ensures no concurrent transactions added new DVs for files we are trying to remove?
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -132,15 +144,44 @@ protected void validate(TableMetadata base, Snapshot parent) {
parent);
}
+ if (validateDeletes) {
+ failMissingDeletePaths();
+ }
+
if (validateNewDataFiles) {
validateAddedDataFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
if (validateNewDeleteFiles) {
+ // validate that explicitly deleted files have not had added deletes
+ if (!deletedDataFiles.isEmpty()) {
Review Comment:
This block simply ensures no concurrent transactions added new deletes for files we are trying to remove?
--
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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#discussion_r2074093747
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -132,15 +144,44 @@ protected void validate(TableMetadata base, Snapshot parent) {
parent);
}
+ if (validateDeletes) {
Review Comment:
If I remember correctly, `validateDeletes` controls whether we check concurrent `delete` snapshots to see if any of them removed data files that we include in `referencedDataFiles`. Then `failMissingDeletePaths` must ensure that whatever we are trying to remove is actually still in the table. In other words, this blocks means if we do UPDATE or MERGE, make sure that data files that we are trying to remove entirely are still there.
Correct?
--
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] API, Core: Add deleteFile to RowDelta API [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12861:
URL: https://github.com/apache/iceberg/pull/12861#discussion_r2074205131
##########
core/src/main/java/org/apache/iceberg/BaseRowDelta.java:
##########
@@ -132,15 +144,44 @@ protected void validate(TableMetadata base, Snapshot parent) {
parent);
}
+ if (validateDeletes) {
+ failMissingDeletePaths();
+ }
+
if (validateNewDataFiles) {
validateAddedDataFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
if (validateNewDeleteFiles) {
+ // validate that explicitly deleted files have not had added deletes
+ if (!deletedDataFiles.isEmpty()) {
+ validateNoNewDeletesForDataFiles(
+ base, startingSnapshotId, conflictDetectionFilter, deletedDataFiles, parent);
+ }
+
+ // validate that previous deletes do not conflict with added deletes
validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
+ validateNoSimultaneousForDeletedFiles();
+
validateAddedDVs(base, startingSnapshotId, conflictDetectionFilter, parent);
}
}
+
+ /** Validates that the data files removed in this commit do not overlap with delete files added */
Review Comment:
ah that's true, validating for equality deletes would obviously be much more of a hassle.
--
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