Skip to content

Core, API: BaseRowDelta, BaseOverwrite,BaseReplacePartitions, BaseRewrite to branch Impl #5234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rdblue merged 38 commits into from
Jan 23, 2023
Merged

Core, API: BaseRowDelta, BaseOverwrite,BaseReplacePartitions, BaseRewrite to branch Impl #5234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
9e43889
Append changes to snapshot branch
Jun 1, 2022
e6ea5f6
Adding snapshot to branch Impl
Jun 9, 2022
b43b80b
Adding check to see if it's a branch
Jun 13, 2022
9cb39c2
Creating branch if ref absent
Jun 16, 2022
ba341ec
Building after setting ref
Jun 16, 2022
d1ca432
Setting default branch as main
Jun 21, 2022
0d79ef0
Fetching current when no main ref is set.
Jun 22, 2022
495c0f3
checkStyle fixes
Jun 29, 2022
f7464eb
build fixes
Jun 29, 2022
39de1e4
Validation check for branch
Jul 2, 2022
7e75c68
Merge branch 'master' of https://github.com/apache/iceberg into branc…
Jul 6, 2022
080d76e
Restricting toBranch only for BaseRowDelta
Jul 9, 2022
1dc4f89
removing new line
Jul 10, 2022
41b2f62
Updating BaseRowDelta as per SnapshotProducer
namrathamk Aug 23, 2022
0c18302
Updating BaseRowDelta as per SnapshotProducer
namrathamk Aug 23, 2022
0acca3b
Merge branch 'master' of https://github.com/apache/iceberg into branc…
namrathamk Aug 23, 2022
49b6667
Updating BaseRowDelta as per SnapshotProducer
namrathamk Aug 23, 2022
4ab3414
Merge branch 'master' of https://github.com/apache/iceberg into branc…
namrathamk Aug 24, 2022
fb28c02
Merge branch 'master' of https://github.com/apache/iceberg into branc…
namrathamk Aug 26, 2022
cfc5e67
Updating BaseRowDelta as per SnapshotProducer
namrathamk Aug 28, 2022
70a3cf3
build fixes
namrathamyske Aug 28, 2022
a64b837
Merge branch 'master' of https://github.com/apache/iceberg into branc…
Jan 5, 2023
f9b3d67
fixing tests to take parameters, Snapshot summary fix
Jan 6, 2023
cd5569c
fetching data files and delete files by ref
Jan 6, 2023
3d5659b
spotless fix
namrathamyske Jan 8, 2023
0028bcd
Test case for overwrite and rewrite
namrathamyske Jan 11, 2023
7173dcb
Removing unnecessary testcases
namrathamyske Jan 14, 2023
caf9d59
Merge branch 'master' of https://github.com/apache/iceberg into branc…
namrathamyske Jan 15, 2023
4f64b09
Build fixes
namrathamyske Jan 15, 2023
90f23ab
Core: Add tests for replace partitions with branching, fix check for …
amogh-jahagirdar Jan 17, 2023
4b8ac6d
Merge pull request #20 from amogh-jahagirdar/branch-replace-partitions
namrathamyske Jan 18, 2023
c6df1e7
Pull from master
namrathamyske Jan 18, 2023
defff1d
Merge remote-tracking branch 'origin/branch-checks-final' into branch…
namrathamyske Jan 18, 2023
66e1850
overwrite validation tests
namrathamyske Jan 18, 2023
fc8780c
variable name change
namrathamyske Jan 18, 2023
6b6aefc
Uncomment test.
rdblue Jan 20, 2023
1a98e54
Core: Deprecate validation methods which don't accept parent snapshot…
amogh-jahagirdar Jan 21, 2023
74bab58
Merge pull request #21 from amogh-jahagirdar/row-delta-rev-api
namrathamyske Jan 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .palantir/revapi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,16 @@ acceptedBreaks:
old: "method org.apache.iceberg.Table org.apache.iceberg.BaseMetadataTable::table()"
new: "method org.apache.iceberg.BaseTable org.apache.iceberg.BaseMetadataTable::table()"
justification: "Returning more specific subclass"
- code: "java.method.returnTypeChangedCovariantly"
old: "method ThisT org.apache.iceberg.SnapshotUpdate<ThisT>::toBranch(java.lang.String)\
\ @ org.apache.iceberg.BaseOverwriteFiles"
new: "method org.apache.iceberg.BaseOverwriteFiles org.apache.iceberg.BaseOverwriteFiles::toBranch(java.lang.String)"
justification: "Introducing branch snapshot operations on BaseOverwrite"
- code: "java.method.returnTypeChangedCovariantly"
old: "method ThisT org.apache.iceberg.SnapshotUpdate<ThisT>::toBranch(java.lang.String)\
\ @ org.apache.iceberg.BaseReplacePartitions"
new: "method org.apache.iceberg.BaseReplacePartitions org.apache.iceberg.BaseReplacePartitions::toBranch(java.lang.String)"
justification: "Introducing branch snapshot operations for BaseReplacePartitions"
org.apache.iceberg:iceberg-orc:
- code: "java.method.removed"
old: "method org.apache.iceberg.orc.ORC.WriteBuilder org.apache.iceberg.orc.ORC.WriteBuilder::config(java.lang.String,\
Expand Down
14 changes: 10 additions & 4 deletions core/src/main/java/org/apache/iceberg/BaseOverwriteFiles.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,12 @@ public OverwriteFiles validateNoConflictingDeletes() {
return this;
}

@Override
public BaseOverwriteFiles toBranch(String branch) {
targetBranch(branch);
return this;
}

@Override
protected void validate(TableMetadata base, Snapshot snapshot) {
if (validateAddedFilesMatchOverwriteFilter) {
Expand Down Expand Up @@ -133,19 +139,19 @@ protected void validate(TableMetadata base, Snapshot snapshot) {
}

if (validateNewDataFiles) {
validateAddedDataFiles(base, startingSnapshotId, dataConflictDetectionFilter());
validateAddedDataFiles(base, startingSnapshotId, dataConflictDetectionFilter(), snapshot);
}

if (validateNewDeletes) {
if (rowFilter() != Expressions.alwaysFalse()) {
Expression filter = conflictDetectionFilter != null ? conflictDetectionFilter : rowFilter();
validateNoNewDeleteFiles(base, startingSnapshotId, filter);
validateDeletedDataFiles(base, startingSnapshotId, filter);
validateNoNewDeleteFiles(base, startingSnapshotId, filter, snapshot);
validateDeletedDataFiles(base, startingSnapshotId, filter, snapshot);
}

if (deletedDataFiles.size() > 0) {
validateNoNewDeletesForDataFiles(
base, startingSnapshotId, conflictDetectionFilter, deletedDataFiles);
base, startingSnapshotId, conflictDetectionFilter, deletedDataFiles, snapshot);
}
}
}
Expand Down
21 changes: 15 additions & 6 deletions core/src/main/java/org/apache/iceberg/BaseReplacePartitions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,23 +79,32 @@ public ReplacePartitions validateNoConflictingData() {
return this;
}

@Override
public BaseReplacePartitions toBranch(String branch) {
targetBranch(branch);
return this;
}

@Override
public void validate(TableMetadata currentMetadata, Snapshot snapshot) {
if (validateConflictingData) {
if (dataSpec().isUnpartitioned()) {
validateAddedDataFiles(currentMetadata, startingSnapshotId, Expressions.alwaysTrue());
validateAddedDataFiles(
currentMetadata, startingSnapshotId, Expressions.alwaysTrue(), snapshot);
} else {
validateAddedDataFiles(currentMetadata, startingSnapshotId, replacedPartitions);
validateAddedDataFiles(currentMetadata, startingSnapshotId, replacedPartitions, snapshot);
Copy link
Contributor

@rdblue rdblue Jan 20, 2023

Choose a reason for hiding this comment

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

Minor: I think it makes more sense to use parent rather than snapshot for the parent snapshot variable. Not a blocker, but it makes it easier to understand what's going on.

}
}

if (validateConflictingDeletes) {
if (dataSpec().isUnpartitioned()) {
validateDeletedDataFiles(currentMetadata, startingSnapshotId, Expressions.alwaysTrue());
validateNoNewDeleteFiles(currentMetadata, startingSnapshotId, Expressions.alwaysTrue());
validateDeletedDataFiles(
currentMetadata, startingSnapshotId, Expressions.alwaysTrue(), snapshot);
validateNoNewDeleteFiles(
currentMetadata, startingSnapshotId, Expressions.alwaysTrue(), snapshot);
} else {
validateDeletedDataFiles(currentMetadata, startingSnapshotId, replacedPartitions);
validateNoNewDeleteFiles(currentMetadata, startingSnapshotId, replacedPartitions);
validateDeletedDataFiles(currentMetadata, startingSnapshotId, replacedPartitions, snapshot);
validateNoNewDeleteFiles(currentMetadata, startingSnapshotId, replacedPartitions, snapshot);
}
}
}
Expand Down
8 changes: 7 additions & 1 deletion core/src/main/java/org/apache/iceberg/BaseRewriteFiles.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,18 @@ public RewriteFiles validateFromSnapshot(long snapshotId) {
return this;
}

@Override
public BaseRewriteFiles toBranch(String branch) {
targetBranch(branch);
return this;
}

@Override
protected void validate(TableMetadata base, Snapshot snapshot) {
if (replacedDataFiles.size() > 0) {
// if there are replaced data files, there cannot be any new row-level deletes for those data
// files
validateNoNewDeletesForDataFiles(base, startingSnapshotId, replacedDataFiles);
validateNoNewDeletesForDataFiles(base, startingSnapshotId, replacedDataFiles, snapshot);
}
}
}
25 changes: 20 additions & 5 deletions core/src/main/java/org/apache/iceberg/BaseRowDelta.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.apache.iceberg.expressions.Expressions;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.util.CharSequenceSet;
import org.apache.iceberg.util.SnapshotUtil;

class BaseRowDelta extends MergingSnapshotProducer<RowDelta> implements RowDelta {
private Long startingSnapshotId = null; // check all versions by default
Expand Down Expand Up @@ -96,23 +97,37 @@ public RowDelta validateNoConflictingDeleteFiles() {
}

@Override
protected void validate(TableMetadata base, Snapshot snapshot) {
if (base.currentSnapshot() != null) {
public RowDelta toBranch(String branch) {
targetBranch(branch);
return this;
}

@Override
protected void validate(TableMetadata base, Snapshot parent) {
if (parent != null) {
if (startingSnapshotId != null) {
Preconditions.checkArgument(
SnapshotUtil.isAncestorOf(parent.snapshotId(), startingSnapshotId, base::snapshot),
"Snapshot %s is not an ancestor of %s",
Copy link
Contributor

@rdblue rdblue Jan 20, 2023

Choose a reason for hiding this comment

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

I think a bit more context would be helpful if you ever encounter this error. "Cannot validate changes: starting snapshot %s is not an ancestor of parent snapshot %s (branch %s)"

Copy link
Contributor Author

@namrathamyske namrathamyske Jan 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@amogh-jahagirdar Is this ancestor check even required anymore ? We are anyway doing ancestor check in

iceberg/core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java

Line 742 in a1d994c

ValidationException.check(

startingSnapshotId,
parent.snapshotId());
}
Copy link
Contributor

@rdblue rdblue Jan 20, 2023

Choose a reason for hiding this comment

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

Nit: whitespace after this block.

if (!referencedDataFiles.isEmpty()) {
validateDataFilesExist(
base,
startingSnapshotId,
referencedDataFiles,
!validateDeletes,
conflictDetectionFilter);
conflictDetectionFilter,
parent);
}

if (validateNewDataFiles) {
validateAddedDataFiles(base, startingSnapshotId, conflictDetectionFilter);
validateAddedDataFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}

if (validateNewDeleteFiles) {
validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter);
validateNoNewDeleteFiles(base, startingSnapshotId, conflictDetectionFilter, parent);
}
}
}
Expand Down
Loading