Core: Validate concurrently added delete files in OverwriteFiles by aokolnychyi · Pull Request #3199 · apache/iceberg

aokolnychyi · 2021-09-28T20:13:29Z

This PR validates concurrently added delete files in BaseOvewriteFiles.

RussellSpitzer

This looks good to me, I couldn't think of other conflicts to test but maybe someone else can :)

rdblue · 2021-09-29T22:53:20Z

@@ -145,4 +151,50 @@ public interface OverwriteFiles extends SnapshotUpdate<OverwriteFiles> {
 */
 @Deprecated
 OverwriteFiles validateNoConflictingAppends(Long readSnapshotId, Expression conflictDetectionFilter);


Can you remove this as well? Looks like it was supposed to be removed already.

I'll clean up all interfaces in the api module in a separate PR.

szehon-ho · 2021-09-29T22:33:41Z

+ if (validateNewDeleteFiles && base.currentSnapshot() != null) {
+ if (rowFilter() != Expressions.alwaysFalse()) {
+ validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter(), caseSensitive);
+ } else if (deletedDataFiles.size() > 0) {


Can we use deletedDataFiles.isEmpty for consistency with below?

I am not a big fan of negation inside conditions but it is purely a personal thing :)

OK fair enough :)

Yeah, I think we all have some of these. Personally, I don't use ++ at all. When reviewing I ignore it unless the return value is used, which I think makes code harder to understand.

szehon-ho · 2021-09-29T22:35:20Z

+ * Calling this method with a correct conflict detection filter is required to maintain
+ * serializable isolation for overwrite operations.
+ * <p>
+ * Validation uses the conflict detection filter passed to {@link #conflictDetectionFilter(Expression)} and


Am I right to understand that we use rowFilter to do the detection, if conflictDetectionFilter is not set? Should we mention it?

If it is not set, the validation is tricky. I added a few sentences. Let me know if that makes sense, @szehon-ho.

Yea its a lot better.

szehon-ho · 2021-09-29T23:06:23Z

+
+ if (validateNewDeleteFiles && base.currentSnapshot() != null) {
+ if (rowFilter() != Expressions.alwaysFalse()) {
+ validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter(), caseSensitive);


Takes a bit of time to understand this if - else, especially as conflictDetectionFilter() itself returns conditionally. Wonder if it can be simplified any way.
Tracing through the many cases,I was wondering some cases, eg:
If (rowFilter != false && !deletedDataFiles.isEmpty && conflictDetectionFilter == true), seems we will call validateNoNewDeleteFiles with conflictDetectionFilter, is it intended?

Changed the logic to behave a bit differently now.

Great, this looks a lot better and easier to understand now

szehon-ho

Not sure how to resolve the conversations, but looks good for me now, thanks @aokolnychyi

rdblue · 2021-10-01T15:39:39Z

- overwriteFiles.validateNoConflictingAppends(conflictDetectionFilter);
+ overwriteFiles.conflictDetectionFilter(conflictDetectionFilter);
+ overwriteFiles.validateNoConflictingData();
+ overwriteFiles.validateNoConflictingDeletes();


@aokolnychyi, shouldn't this check whether the operation is a delete? If this is invoked by DELETE FROM then we don't need to validate conflicting deletes.

I think that can only be done for merge-on-read. If I delete file_A with copy-on-write and overwrite it with file_B, I should still check no deletes happened for file_A, otherwise I'll undelete records.

rdblue

I have a couple remaining questions, but overall I think this is correct and good to go!

aokolnychyi · 2021-10-01T18:49:13Z

Thanks for reviewing, @RussellSpitzer @rdblue @szehon-ho!

…he#3199)

…he#3199(merged)

github-actionsBot added API core spark labels Sep 28, 2021

Core: Validate concurrently added delete files in OvewriteFiles
778c1e8

aokolnychyi force-pushed the validate-overwrite-files branch from 4eb4063 to 778c1e8Compare September 28, 2021 20:28

RussellSpitzer reviewed Sep 29, 2021
View reviewed changes

Comment threadapi/src/main/java/org/apache/iceberg/OverwriteFiles.java Outdated

RussellSpitzer approved these changes Sep 29, 2021
View reviewed changes