sezruby commented on code in PR #18603:
URL: https://github.com/apache/iceberg/pull/18603#discussion_r4445748914
##########
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) {
Review Comment:
@huaxingao Thanks, you're right. I tried the example on the 4.2 tests (MoR
UPDATE, with the concurrent change committed right before the UPDATE commits,
same setup as the tests in this PR). With the current check the UPDATE fails;
with this PR it commits.
Without a subquery on the same table, the UPDATE's result depends only on
rows that match its condition. All of those are in data files it modifies, so
the narrowed check still catches every concurrent delete that changes the
result. Concurrent inserts are still checked by
`validateNoConflictingDataFiles()` under SERIALIZABLE. The difference is only
for UPDATEs whose subquery reads other rows of the same table, and the write
can't tell those apart: it only gets the command and the target scan's filter.
For those UPDATEs, the current check covers only some cases. The conflict
filter is the target scan's filter (`id = 1`), not the subquery's (`parent =
1`). The example fails today because DVs and position deletes have no stats on
data columns, so they match any filter in the same partition. Related cases
already commit with the current check:
| Case | Current check | This PR |
|---|---|---|
| `EXISTS` + concurrent DV/position delete of the child, unpartitioned |
fails | commits |
| Same, child in another partition (`PARTITIONED BY (dep)`) | commits |
commits |
| Same, concurrent equality delete of the child | commits* | commits* |
| `NOT EXISTS` + concurrent insert of a child | commits* | commits* |
\* Parquet and ORC. With Avro (no column stats), both fail.
Two options:
1. Keep the narrowed check under both isolation levels. Only UPDATEs with a
subquery on the same table become less strict, and for those the current check
already misses the cases above.
2. Use the narrowed check only under SNAPSHOT isolation, and keep
`validateNoConflictingDeleteFiles()` under SERIALIZABLE, so the default doesn't
change.
I'd prefer option 1, but I'm happy to switch to option 2. Let me know which
you prefer.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
