Skip to content

Conversation

@aokolnychyi
Copy link
Contributor

This PR validates concurrently added delete files in BaseRowDelta.

* @param conflictDetectionFilter an expression on rows in the table
* @return this for method chaining
*/
RowDelta conflictDetectionFilter(Expression conflictDetectionFilter);
Copy link
Contributor Author

@aokolnychyi aokolnychyi Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually different from the implementation in PR #3069. I think this one is slightly better as we set the conflict detection filter only once and then enable data/delete file validation.

Copy link
Contributor Author

@aokolnychyi aokolnychyi Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, we would need to reason about what happens if the filters are different, which one to use for validating referenced data files, etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good to me.

@aokolnychyi
Copy link
Contributor Author

return (globalDeletes == null || globalDeletes.length == 0) && sortedDeletesByPartition.isEmpty();
}

public List<DeleteFile> referencedDeleteFiles() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would Iterable<DeleteFile> work instead? That would make this lazy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

protected void validateNoNewDeleteFiles(TableMetadata base, Long startingSnapshotId,
Expression dataFilter, boolean caseSensitive) {
// if there is no current table state, no files have been added
if (base.currentSnapshot() == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the check for base.formatVersion() < 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to both methods. Could you check, @rdblue?

@rdblue
Copy link
Contributor

rdblue commented Sep 28, 2021

Looks good. Thanks for adding that check!

@aokolnychyi
Copy link
Contributor Author

Thanks for reviewing, @rdblue!

@aokolnychyi aokolnychyi merged commit 10dedf7 into apache:master Sep 28, 2021
@rdblue rdblue added this to the Java 0.12.1 Release milestone Oct 26, 2021
kbendick pushed a commit to kbendick/iceberg that referenced this pull request Oct 27, 2021
kbendick pushed a commit to kbendick/iceberg that referenced this pull request Nov 1, 2021
izchen pushed a commit to izchen/iceberg that referenced this pull request Dec 7, 2021
Initial-neko pushed a commit to Initial-neko/iceberg that referenced this pull request Dec 13, 2021
Initial-neko pushed a commit to Initial-neko/iceberg that referenced this pull request Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants