rdblue commented on PR #18546:
URL: https://github.com/apache/iceberg/pull/18546#issuecomment-6304733677
The approach this PR takes is to try to unify all cases with a single
conflict detection filter. As I said in my comment above, the goal is to be
able to use a simpler API, like this:
```java
public validator() {
// this would be built just once
return Validator.newValidator()
.io(table.io())
.startingSnapshotId(startId)
.conflictDetectionFilter(filter);
}
public void validate(TableMetadata base, Snapshot parent) {
if (validateNewDataFiles) {
validator.validateAddedDataFiles();
}
if (validateNewDeletes) {
if (deleteByRowFilter) {
// no new delete files for files removed by filter
validator.validateNoNewDeleteFiles();
// no files removed by filter are already deleted
validator.validateDeletedDataFiles();
}
if (!deletedDataFiles.isEmpty()) {
// no new delete files for files removed by deleteFile(...)
validator.validateNoNewDeleteFilesFor(deletedDataFiles);
}
}
}
```
The problem this PR is addressing is inconsistency across different uses of
the API when `conflictDetectionFilter` is not provided. When it is provided, it
is always used so we have just one filter. However, when there is no specific
`conflictDetectionFilter`, some validations use `rowFilter` and some use
`Expressions.alwaysTrue()`:
* If specific files are deleted using `deleteFile(DataFile)` then
`Expressions.alwaysTrue()` is used for both new data and new deletes matching
the file set
* If files are deleted by row filter, then concurrent row-level deletes are
checked using `rowFilter()`
The problem is that these cases are not mutually exclusive, so it isn't
possible to configure a validator with just one filter. I've been thinking
through this a lot today and I don't see a way to unify the filters. I
considered always using `Expressions.alwaysTrue` for
`validator.validateNoNewDeleteFilesFor(deletedDataFiles)` since there is no
correctness impact (it would just index more deletes), but the new data file
validation also uses the `alwaysTrue` filter and that would be a behavior
change that could cause new commit failures.
This PR solves the problem by making `overwriteByRowFilter(Expression)` and
`deleteFile(DataFile)` incompatible with each other. The main argument for this
is that although we tried to make these two uses of the API compatible, they
were always intended for different use cases. Overwriting by an expression is a
stand-alone operation typically used for `df.writeTo(table).overwrite(expr)`,
while `deleteFile` is used for copy-on-write MERGE. I don't know of any use
cases where these calls are needed in the same commit.
The only alternative solution I was able to think of was to detect the
problem and use a second validator that doesn't have a conflict detection
filter configured. Something like this:
```java
if (validateNewDataFiles) {
if (conflictDetectionFilter == null && !deletedDataFiles.isEmpty()) {
validatorWithoutFilter.validateAddedDataFiles();
} else {
validator.validateAddedDataFiles();
}
}
if (validateNewDeletes) {
if (deleteByRowFilter) {
// no new delete files for files removed by filter
validator.validateNoNewDeleteFiles();
// no files removed by filter are already deleted
validator.validateDeletedDataFiles();
}
if (!deletedDataFiles.isEmpty()) {
// no new delete files for files removed by deleteFile(...)
if (conflictDetectionFilter == null && !deletedDataFiles.isEmpty()) {
validatorWithoutFilter.validateNoNewDeleteFilesFor(deletedDataFiles);
} else {
validator.validateNoNewDeleteFilesFor(deletedDataFiles);
}
}
}
```
I think this will work, but means that a second validator has to be
maintained.
--
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]
