You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "sezruby (via GitHub)" <gi...@apache.org> on 2026/10/21 21:20:22 UTC
[PR] Core, Spark: Limit UPDATE delete file conflicts to modified data files [iceberg]
sezruby opened a new pull request, #18603:
URL: https://github.com/apache/iceberg/pull/18603
Part of #18020.
An UPDATE fails on any concurrent delete file matching its conflict detection filter, even a deletion vector for a data file it does not modify, so on an unpartitioned table any concurrent DELETE aborts a concurrent UPDATE. This adds `RowDelta#validateNoConflictingDeleteFilesForModifiedDataFiles`, which ignores concurrent position deletes and DVs that apply to a single data file the commit does not modify, and uses it for Spark UPDATE. It assumes that a data file the UPDATE does not modify has no rows matching its condition; as with DELETE today, a concurrent delete of a row read only by a subquery in the condition is not detected. MERGE is unchanged. On v2 tables this applies only to file-scoped position deletes.
Test plan: in `TestRowDelta`, a concurrent delete for another data file in the same partition commits; a concurrent delete for a modified data file, a concurrent equality delete and a concurrent partition-scoped position delete still conflict; a commit adding a partition-scoped position delete still validates every concurrent delete; a delete for a removed data file still conflicts. `TestMergeOnReadUpdate` passes unchanged.
---
**AI Disclosure**
- Model: Claude Opus 5.5
- Platform/Tool: Claude Code
- Human Oversight: [unknown - human to fill in]
- Prompt Summary: Narrow UPDATE's concurrent delete-file validation to the data files it modifies; the implementation, tests and this description are generated.
--
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, Spark: Limit UPDATE delete file conflicts to modified data files [iceberg]
Posted by "sezruby (via GitHub)" <gi...@apache.org>.
sezruby commented on code in PR #18603:
URL: https://github.com/apache/iceberg/pull/18603#discussion_r4385013130
##########
spark/v4.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMergeOnReadUpdate.java:
##########
@@ -52,6 +54,29 @@ protected Map<String, String> extraTableProperties() {
TableProperties.UPDATE_MODE, RowLevelOperationMode.MERGE_ON_READ.modeName());
}
+ @TestTemplate
+ void updateCommitsWithConcurrentDeleteOfAnotherDataFile() throws IOException {
+ createAndInitTable("id INT, dep STRING");
+ append(tableName, "{ \"id\": 1, \"dep\": \"hr\" }");
+ Table table = validationCatalog.loadTable(tableIdent);
+ DataFile dataFile =
+ Iterables.getOnlyElement(
+ SnapshotChanges.builderFor(table)
+ .snapshot(table.currentSnapshot())
+ .build()
+ .addedDataFiles());
+ createBranchIfNeeded();
+ append(commitTarget(), "{ \"id\": 2, \"dep\": \"hr\" }");
+
+ sql("UPDATE %s SET dep = 'it' WHERE id = 2", tableWithConcurrentDelete(dataFile, 0L));
Review Comment:
Added `updateConflictsWithConcurrentDeleteOfUpdatedRow` in 62e61d05: it runs `UPDATE ... WHERE id = 1` against `tableWithConcurrentDelete(dataFile, 0L)` and asserts the "Found new conflicting delete files" failure. Removing the `validateNoConflictingDeleteFilesForModifiedDataFiles` call for UPDATE now fails 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] Core, Spark: Limit UPDATE delete file conflicts to modified data files [iceberg]
Posted by "sezruby (via GitHub)" <gi...@apache.org>.
sezruby commented on PR #18603:
URL: https://github.com/apache/iceberg/pull/18603#issuecomment-6433865499
Pushed 45fed199: removed data files are no longer added to the data files this check looks at. `validateNoNewDeletesForDataFiles` already fails on any concurrent delete that applies to a removed data file, and it runs first on the same delete files, so the outcome doesn't change; `concurrentDeleteForRemovedDataFileConflictsWithModifiedDataFiles` still fails with that message.
--
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, Spark: Limit UPDATE delete file conflicts to modified data files [iceberg]
Posted by "sezruby (via GitHub)" <gi...@apache.org>.
sezruby commented on code in PR #18603:
URL: https://github.com/apache/iceberg/pull/18603#discussion_r4381293948
##########
core/src/test/java/org/apache/iceberg/TestRowDelta.java:
##########
@@ -1324,6 +1324,163 @@ public void testConcurrentConflictingRowDelta() {
.hasMessageStartingWith("Found new conflicting delete files");
}
+ private RowDelta updateOfFileA(long startingSnapshotId, DeleteFile fileADeletes) {
+ return table
+ .newRowDelta()
+ .toBranch(branch)
+ .addRows(FILE_B)
+ .addDeletes(fileADeletes)
+ .validateFromSnapshot(startingSnapshotId)
+ .conflictDetectionFilter(Expressions.alwaysTrue())
+ .validateDataFilesExist(ImmutableList.of(FILE_A.location()))
Review Comment:
Changed the modified set instead of the helper: the files passed to validateDataFilesExist are no longer included, so it is now the commit's delete targets and removed files, as the Javadoc already says. Spark passes the same delete targets to validateDataFilesExist, so this only removed a duplicate. The tests now depend on the targets: leaving them out of the set fails concurrentDeleteForModifiedDataFileConflicts.
--
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, Spark: Limit UPDATE delete file conflicts to modified data files [iceberg]
Posted by "sezruby (via GitHub)" <gi...@apache.org>.
sezruby commented on PR #18603:
URL: https://github.com/apache/iceberg/pull/18603#issuecomment-6453491465
@amogh-jahagirdar could you take a look at this when you have a chance? It narrows UPDATE's check for concurrent delete files in `BaseRowDelta` and `MergingSnapshotProducer` to the data files the UPDATE modifies, so a concurrent delete on another data file no longer fails the UPDATE. You reviewed the related DV validation changes in #15653 and #17764.
--
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, Spark: Limit UPDATE delete file conflicts to modified data files [iceberg]
Posted by "sezruby (via GitHub)" <gi...@apache.org>.
sezruby commented on code in PR #18603:
URL: https://github.com/apache/iceberg/pull/18603#discussion_r4381284968
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -605,11 +605,71 @@ protected void validateNoNewDeleteFiles(
locations);
}
+ /**
+ * Validates that no delete files that may apply to the given data files, among those matching a
+ * filter, have been added to the table since a starting snapshot.
+ *
+ * <p>Position deletes and deletion vectors that apply to a single data file not in {@code
+ * dataFiles} are ignored. Equality deletes and position deletes that may apply to more than one
+ * data file are always considered conflicting.
+ *
+ * @param base table metadata to validate
+ * @param startingSnapshotId id of the snapshot current at the start of the operation
+ * @param dataFilter an expression used to find new conflicting delete files
+ * @param dataFiles locations of the data files to validate have no new row deletes
+ * @param parent ending snapshot on the branch being validated
+ */
+ @SuppressWarnings("CollectionUndefinedEquality")
+ protected void validateNoNewDeleteFiles(
+ TableMetadata base,
+ Long startingSnapshotId,
+ Expression dataFilter,
+ CharSequenceSet dataFiles,
+ Snapshot parent) {
+ Set<String> locations =
+ referencedDeleteFileLocations(
+ addedDeleteFilesIndexedPerSnapshot(base, startingSnapshotId, dataFilter, null, parent),
+ deleteFile -> {
+ CharSequence referencedDataFile = ContentFileUtil.referencedDataFile(deleteFile);
+ return referencedDataFile == null || dataFiles.contains(referencedDataFile);
+ });
+ ValidationException.check(
+ locations.isEmpty(),
+ "Found new conflicting delete files that can apply to records matching %s: %s",
+ dataFilter,
+ locations);
+ }
+
+ /**
+ * Returns the locations of the data files that the delete files added by this operation apply to,
+ * or null if one of them may apply to more than one data file.
+ */
+ protected CharSequenceSet referencedDataFilesOfAddedDeletes() {
Review Comment:
Made both package-private. Thanks!
--
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, Spark: Limit UPDATE delete file conflicts to modified data files [iceberg]
Posted by "sezruby (via GitHub)" <gi...@apache.org>.
sezruby commented on code in PR #18603:
URL: https://github.com/apache/iceberg/pull/18603#discussion_r4381308699
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/source/SparkPositionDeltaWrite.java:
##########
@@ -285,7 +285,11 @@ public void commit(WriterCommitMessage[] messages, WriteSummary summary) {
rowDelta.validateFromSnapshot(scan.snapshotId());
}
- if (command == UPDATE || command == MERGE) {
+ if (command == UPDATE) {
+ // data files that the UPDATE does not modify have no rows matching its condition
+ rowDelta.validateDeletedFiles();
+ rowDelta.validateNoConflictingDeleteFilesForModifiedDataFiles();
Review Comment:
Added updateCommitsWithConcurrentDeleteOfAnotherDataFile to TestMergeOnReadUpdate and mergeConflictsWithConcurrentDeleteOfAnotherDataFile to TestMergeOnReadMerge. A shared helper in SparkRowLevelOperationsTestBase spies on the row delta and commits a delete of another data file right before the real commit. Reverting the UPDATE branch fails the first test, and moving MERGE to the new method fails the second.
--
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, Spark: Limit UPDATE delete file conflicts to modified data files [iceberg]
Posted by "sezruby (via GitHub)" <gi...@apache.org>.
sezruby commented on PR #18603:
URL: https://github.com/apache/iceberg/pull/18603#issuecomment-6424961546
@wombatu-kun thanks for the review. All four comments are addressed in b1cb5b05. Could you take another look?
@huaxingao could you review this when you have a chance? It narrows UPDATE's check for concurrent delete files to the data files the UPDATE modifies, so conflicts are decided per data file rather than per conflict filter. #17754 is related: it merges concurrently added DVs for DELETE commits on the same data file. The two PRs don't depend 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, Spark: Limit UPDATE delete file conflicts to modified data files [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #18603:
URL: https://github.com/apache/iceberg/pull/18603#discussion_r4384536276
##########
spark/v4.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestMergeOnReadUpdate.java:
##########
@@ -52,6 +54,29 @@ protected Map<String, String> extraTableProperties() {
TableProperties.UPDATE_MODE, RowLevelOperationMode.MERGE_ON_READ.modeName());
}
+ @TestTemplate
+ void updateCommitsWithConcurrentDeleteOfAnotherDataFile() throws IOException {
+ createAndInitTable("id INT, dep STRING");
+ append(tableName, "{ \"id\": 1, \"dep\": \"hr\" }");
+ Table table = validationCatalog.loadTable(tableIdent);
+ DataFile dataFile =
+ Iterables.getOnlyElement(
+ SnapshotChanges.builderFor(table)
+ .snapshot(table.currentSnapshot())
+ .build()
+ .addedDataFiles());
+ createBranchIfNeeded();
+ append(commitTarget(), "{ \"id\": 2, \"dep\": \"hr\" }");
+
+ sql("UPDATE %s SET dep = 'it' WHERE id = 2", tableWithConcurrentDelete(dataFile, 0L));
Review Comment:
This pins only the commit side: removing the validateNoConflictingDeleteFilesForModifiedDataFiles call for UPDATE in SparkPositionDeltaWrite still leaves every test green. Could a second test run UPDATE ... WHERE id = 1 against tableWithConcurrentDelete(dataFile, 0L), so the concurrent delete hits the row being updated, and assert the "Found new conflicting delete files" failure?
--
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, Spark: Limit UPDATE delete file conflicts to modified data files [iceberg]
Posted by "wombatu-kun (via GitHub)" <gi...@apache.org>.
wombatu-kun commented on code in PR #18603:
URL: https://github.com/apache/iceberg/pull/18603#discussion_r4375571806
##########
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java:
##########
@@ -605,11 +605,71 @@ protected void validateNoNewDeleteFiles(
locations);
}
+ /**
+ * Validates that no delete files that may apply to the given data files, among those matching a
+ * filter, have been added to the table since a starting snapshot.
+ *
+ * <p>Position deletes and deletion vectors that apply to a single data file not in {@code
+ * dataFiles} are ignored. Equality deletes and position deletes that may apply to more than one
+ * data file are always considered conflicting.
+ *
+ * @param base table metadata to validate
+ * @param startingSnapshotId id of the snapshot current at the start of the operation
+ * @param dataFilter an expression used to find new conflicting delete files
+ * @param dataFiles locations of the data files to validate have no new row deletes
+ * @param parent ending snapshot on the branch being validated
+ */
+ @SuppressWarnings("CollectionUndefinedEquality")
+ protected void validateNoNewDeleteFiles(
+ TableMetadata base,
+ Long startingSnapshotId,
+ Expression dataFilter,
+ CharSequenceSet dataFiles,
+ Snapshot parent) {
+ Set<String> locations =
+ referencedDeleteFileLocations(
+ addedDeleteFilesIndexedPerSnapshot(base, startingSnapshotId, dataFilter, null, parent),
+ deleteFile -> {
+ CharSequence referencedDataFile = ContentFileUtil.referencedDataFile(deleteFile);
+ return referencedDataFile == null || dataFiles.contains(referencedDataFile);
+ });
+ ValidationException.check(
+ locations.isEmpty(),
+ "Found new conflicting delete files that can apply to records matching %s: %s",
+ dataFilter,
+ locations);
+ }
+
+ /**
+ * Returns the locations of the data files that the delete files added by this operation apply to,
+ * or null if one of them may apply to more than one data file.
+ */
+ protected CharSequenceSet referencedDataFilesOfAddedDeletes() {
Review Comment:
referencedDataFilesOfAddedDeletes and the new validateNoNewDeleteFiles overload are protected, so revapi tracks them on every public MergingSnapshotProducer subclass. Can they be package-private, since BaseRowDelta is their only caller and lives in the same package?
##########
spark/v4.2/spark/src/main/java/org/apache/iceberg/spark/source/SparkPositionDeltaWrite.java:
##########
@@ -285,7 +285,11 @@ public void commit(WriterCommitMessage[] messages, WriteSummary summary) {
rowDelta.validateFromSnapshot(scan.snapshotId());
}
- if (command == UPDATE || command == MERGE) {
+ if (command == UPDATE) {
+ // data files that the UPDATE does not modify have no rows matching its condition
+ rowDelta.validateDeletedFiles();
+ rowDelta.validateNoConflictingDeleteFilesForModifiedDataFiles();
Review Comment:
No Spark test covers this branch: reverting it to validateNoConflictingDeleteFiles, or moving MERGE to the new method, leaves every test green. Can TestMergeOnReadUpdate add a case where a DELETE of another file in the same partition commits between the UPDATE's scan and commit (a spied RowDelta commit, as in TestMergeOnReadDelete.testCommitUnknownException, can inject it), asserting the UPDATE succeeds?
##########
api/src/main/java/org/apache/iceberg/RowDelta.java:
##########
@@ -161,4 +161,31 @@ default RowDelta removeDeletes(DeleteFile deletes) {
* @return this for method chaining
*/
RowDelta validateNoConflictingDeleteFiles();
+
+ /**
+ * Enables validation that delete files added concurrently do not conflict with this commit's
+ * operation, like {@link #validateNoConflictingDeleteFiles()} but limited to the data files this
+ * commit modifies: those its position deletes and deletion vectors apply to, and those it
+ * removes.
+ *
+ * <p>Concurrent position deletes and deletion vectors that apply to a single other data file do
+ * not conflict. Concurrent equality deletes and position deletes that may apply to more than one
+ * data file still conflict, and so does every concurrent delete file if this commit adds such a
+ * delete file itself.
+ *
+ * <p>This method can be called instead of {@link #validateNoConflictingDeleteFiles()} for UPDATE
Review Comment:
validateNoConflictingDeleteFiles still says it must be called for UPDATE operations, which this paragraph now contradicts. Can that javadoc point UPDATE callers to this method, and can this one state that a concurrent delete of a row the UPDATE only reads through a subquery can go undetected?
##########
core/src/test/java/org/apache/iceberg/TestRowDelta.java:
##########
@@ -1324,6 +1324,163 @@ public void testConcurrentConflictingRowDelta() {
.hasMessageStartingWith("Found new conflicting delete files");
}
+ private RowDelta updateOfFileA(long startingSnapshotId, DeleteFile fileADeletes) {
+ return table
+ .newRowDelta()
+ .toBranch(branch)
+ .addRows(FILE_B)
+ .addDeletes(fileADeletes)
+ .validateFromSnapshot(startingSnapshotId)
+ .conflictDetectionFilter(Expressions.alwaysTrue())
+ .validateDataFilesExist(ImmutableList.of(FILE_A.location()))
Review Comment:
updateOfFileA always passes FILE_A to validateDataFilesExist, so FILE_A reaches the modified set even when referencedDataFilesOfAddedDeletes contributes nothing, and none of these tests fail if the commit's own DV or position-delete targets are left out. Can concurrentDeleteForModifiedDataFileConflicts (or the helper) skip validateDataFilesExist so it depends on those targets?
--
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, Spark: Limit UPDATE delete file conflicts to modified data files [iceberg]
Posted by "sezruby (via GitHub)" <gi...@apache.org>.
sezruby commented on code in PR #18603:
URL: https://github.com/apache/iceberg/pull/18603#discussion_r4381276186
##########
api/src/main/java/org/apache/iceberg/RowDelta.java:
##########
@@ -161,4 +161,31 @@ default RowDelta removeDeletes(DeleteFile deletes) {
* @return this for method chaining
*/
RowDelta validateNoConflictingDeleteFiles();
+
+ /**
+ * Enables validation that delete files added concurrently do not conflict with this commit's
+ * operation, like {@link #validateNoConflictingDeleteFiles()} but limited to the data files this
+ * commit modifies: those its position deletes and deletion vectors apply to, and those it
+ * removes.
+ *
+ * <p>Concurrent position deletes and deletion vectors that apply to a single other data file do
+ * not conflict. Concurrent equality deletes and position deletes that may apply to more than one
+ * data file still conflict, and so does every concurrent delete file if this commit adds such a
+ * delete file itself.
+ *
+ * <p>This method can be called instead of {@link #validateNoConflictingDeleteFiles()} for UPDATE
Review Comment:
Updated validateNoConflictingDeleteFiles to say an UPDATE can call validateNoConflictingDeleteFilesForModifiedDataFiles instead, and added to the new method's Javadoc that a concurrent delete of a row the UPDATE only reads, such as through a subquery in its condition, is not detected.
--
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