-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Core, API: Performing operations on a snapshot branch ref #4926
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.
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
Changes from 9 commits
9e43889
0e2f690
bcfb7a8
8d80888
637b2e8
c87df9a
9e98792
9c1a2d2
4772047
790613d
6948eac
3a60283
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,4 +59,11 @@ public interface SnapshotUpdate<ThisT> extends PendingUpdate<Snapshot> { | |
| * @return this for method chaining | ||
| */ | ||
| ThisT scanManifestsWith(ExecutorService executorService); | ||
|
|
||
| /** | ||
| * Perform operations on a particular branch | ||
| * | ||
| * @param branch which is name of SnapshotRef of type branch. | ||
| */ | ||
| ThisT toBranch(String branch); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs a default that throws |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,6 +84,7 @@ public void accept(String file) { | |
| private Consumer<String> deleteFunc = defaultDelete; | ||
|
|
||
|
namrathamyske marked this conversation as resolved.
namrathamyske marked this conversation as resolved.
|
||
| private ExecutorService workerPool = ThreadPools.getWorkerPool(); | ||
| private String targetBranch = SnapshotRef.MAIN_BRANCH; | ||
|
|
||
| protected SnapshotProducer(TableOperations ops) { | ||
| this.ops = ops; | ||
|
|
@@ -113,6 +114,28 @@ public ThisT scanManifestsWith(ExecutorService executorService) { | |
| return self(); | ||
| } | ||
|
|
||
| @Override | ||
| public ThisT toBranch(String branch) { | ||
| throw new UnsupportedOperationException( | ||
| String.format( | ||
| "Cannot commit to branch %s: %s does not support branch commits", | ||
| branch, this.getClass().getName())); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be in the interface, not here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point, fixed |
||
|
|
||
| /** | ||
| * * A setter for the target branch on which snapshot producer operation should be performed | ||
| * | ||
| * @param branch to set as target branch | ||
| */ | ||
| protected void targetBranch(String branch) { | ||
| Preconditions.checkArgument(branch != null, "Invalid branch name: null"); | ||
| boolean refExists = base.ref(branch) != null; | ||
| Preconditions.checkArgument( | ||
| !refExists || base.ref(branch).isBranch(), | ||
| "%s is a tag, not a branch. Tags cannot be targets for producing snapshots"); | ||
| this.targetBranch = branch; | ||
| } | ||
|
|
||
| protected ExecutorService workerPool() { | ||
| return this.workerPool; | ||
| } | ||
|
|
@@ -150,28 +173,37 @@ public ThisT deleteWith(Consumer<String> deleteCallback) { | |
| * <p>Child operations can override this to add custom validation. | ||
| * | ||
| * @param currentMetadata current table metadata to validate | ||
| * @param snapshot ending snapshot on the lineage which is being validated | ||
| */ | ||
| protected void validate(TableMetadata currentMetadata) {} | ||
| protected void validate(TableMetadata currentMetadata, Snapshot snapshot) {} | ||
|
|
||
| /** | ||
| * Apply the update's changes to the base table metadata and return the new manifest list. | ||
| * | ||
| * @param metadataToUpdate the base table metadata to apply changes to | ||
| * @param snapshot parent snapshot passed to child operations. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah thanks for adding the commenting. Just a nit, I think this should say something like "snapshot to apply the changes to". Child operations doesn't make too much sense to me imo. I think we just need to say "Apply the changes with the given metadata to the snapshot". |
||
| * @return a manifest list for the new snapshot. | ||
| */ | ||
| protected abstract List<ManifestFile> apply(TableMetadata metadataToUpdate); | ||
| protected abstract List<ManifestFile> apply(TableMetadata metadataToUpdate, Snapshot snapshot); | ||
|
|
||
| @Override | ||
| public Snapshot apply() { | ||
| refresh(); | ||
| Long parentSnapshotId = | ||
| base.currentSnapshot() != null ? base.currentSnapshot().snapshotId() : null; | ||
| long sequenceNumber = base.nextSequenceNumber(); | ||
| Snapshot parentSnapshot = base.currentSnapshot(); | ||
| if (targetBranch != null) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also considering targetBranch will always be set (we have it default to Main, we don't need this check). |
||
| SnapshotRef branch = base.ref(targetBranch); | ||
| if (branch != null) { | ||
| parentSnapshot = base.snapshot(branch.snapshotId()); | ||
| } else if (base.currentSnapshot() != null) { | ||
| parentSnapshot = base.currentSnapshot(); | ||
| } | ||
| } | ||
|
|
||
| // run validations from the child operation | ||
| validate(base); | ||
| long sequenceNumber = base.nextSequenceNumber(); | ||
| Long parentSnapshotId = parentSnapshot == null ? null : parentSnapshot.snapshotId(); | ||
|
|
||
| List<ManifestFile> manifests = apply(base); | ||
| validate(base, parentSnapshot); | ||
| List<ManifestFile> manifests = apply(base, parentSnapshot); | ||
|
|
||
| if (base.formatVersion() > 1 | ||
| || base.propertyAsBoolean(MANIFEST_LISTS_ENABLED, MANIFEST_LISTS_ENABLED_DEFAULT)) { | ||
|
|
@@ -337,11 +369,11 @@ public void commit() { | |
| TableMetadata.Builder update = TableMetadata.buildFrom(base); | ||
| if (base.snapshot(newSnapshot.snapshotId()) != null) { | ||
| // this is a rollback operation | ||
| update.setBranchSnapshot(newSnapshot.snapshotId(), SnapshotRef.MAIN_BRANCH); | ||
| update.setBranchSnapshot(newSnapshot.snapshotId(), targetBranch); | ||
| } else if (stageOnly) { | ||
| update.addSnapshot(newSnapshot); | ||
| } else { | ||
| update.setBranchSnapshot(newSnapshot, SnapshotRef.MAIN_BRANCH); | ||
| update.setBranchSnapshot(newSnapshot, targetBranch); | ||
| } | ||
|
|
||
| TableMetadata updated = update.build(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -487,4 +487,54 @@ public void testIncludedPartitionSummaryLimit() { | |
| table.currentSnapshot().summary().get(SnapshotSummary.CHANGED_PARTITION_COUNT_PROP); | ||
| Assert.assertEquals("Should set changed partition count", "2", changedPartitions); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAppendToExistingBranch() { | ||
| table.newFastAppend().appendFile(FILE_A).commit(); | ||
| table.manageSnapshots().createBranch("branch", table.currentSnapshot().snapshotId()).commit(); | ||
| table.newFastAppend().appendFile(FILE_B).toBranch("branch").commit(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just realized I just hardcoded branch throughout the tests, probably cleaner to just define a variable in the tests and use that.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is okay. |
||
| int branchSnapshot = 2; | ||
|
|
||
| Assert.assertEquals(table.currentSnapshot().snapshotId(), 1); | ||
| Assert.assertEquals(table.ops().current().ref("branch").snapshotId(), branchSnapshot); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAppendCreatesBranchIfNeeded() { | ||
| table.newFastAppend().appendFile(FILE_A).commit(); | ||
| table.newFastAppend().appendFile(FILE_B).toBranch("branch").commit(); | ||
| int branchSnapshot = 2; | ||
|
|
||
| Assert.assertEquals(table.currentSnapshot().snapshotId(), 1); | ||
| Assert.assertNotNull(table.ops().current().ref("branch")); | ||
| Assert.assertEquals(table.ops().current().ref("branch").snapshotId(), branchSnapshot); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAppendToBranchEmptyTable() { | ||
| table.newFastAppend().appendFile(FILE_B).toBranch("branch").commit(); | ||
| int branchSnapshot = 1; | ||
|
|
||
| Assert.assertNull(table.currentSnapshot()); | ||
| Assert.assertNotNull(table.ops().current().ref("branch")); | ||
| Assert.assertEquals(table.ops().current().ref("branch").snapshotId(), branchSnapshot); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAppendToNullBranchFails() { | ||
| AssertHelpers.assertThrows( | ||
| "Invalid branch", | ||
| IllegalArgumentException.class, | ||
| () -> table.newFastAppend().appendFile(FILE_A).toBranch(null)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAppendToTagFails() { | ||
| table.newFastAppend().appendFile(FILE_A).commit(); | ||
| table.manageSnapshots().createTag("some-tag", table.currentSnapshot().snapshotId()).commit(); | ||
| AssertHelpers.assertThrows( | ||
| "Invalid branch", | ||
| IllegalArgumentException.class, | ||
| () -> table.newFastAppend().appendFile(FILE_A).toBranch("some-tag").commit()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -300,4 +300,17 @@ public void testValidatedOverwriteWithAppendSuccess() { | |
| Assert.assertEquals( | ||
| "Should not create a new snapshot", baseId, table.currentSnapshot().snapshotId()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testOverwriteToBranchUnsupported() { | ||
| AssertHelpers.assertThrows( | ||
| "Cannot commit to branch someBranch: org.apache.iceberg.BaseOverwriteFiles does not support branch commits", | ||
| UnsupportedOperationException.class, | ||
| () -> | ||
| table | ||
| .newOverwrite() | ||
| .overwriteByRowFilter(and(equal("date", "2018-06-09"), lessThan("id", 20))) | ||
| .addFile(FILE_10_TO_14) | ||
| .toBranch("someBranch")); | ||
|
Comment on lines
+306
to
+314
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should fix this assertion so it actually checks the expected error message; I think the assertThrows method with 3 params just surfaces this message in case the test fails, it doesn't actually check the expected error message. Like what's being done in TestReplacePartitions.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is minor because it is going to be replaced very soon. |
||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.