-
Notifications
You must be signed in to change notification settings - Fork 3k
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 all 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 |
|---|---|---|
|
|
@@ -84,6 +84,7 @@ public void accept(String file) { | |
| private Consumer<String> deleteFunc = defaultDelete; | ||
|
|
||
namrathamyske marked this conversation as resolved.
Show resolved
Hide resolved
namrathamyske marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| private ExecutorService workerPool = ThreadPools.getWorkerPool(); | ||
| private String targetBranch = SnapshotRef.MAIN_BRANCH; | ||
|
|
||
| protected SnapshotProducer(TableOperations ops) { | ||
| this.ops = ops; | ||
|
|
@@ -113,6 +114,20 @@ public ThisT scanManifestsWith(ExecutorService executorService) { | |
| return self(); | ||
| } | ||
|
|
||
| /** | ||
| * * 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 +165,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. | ||
| * Apply the update's changes to the given metadata and snapshot. Return the new manifest list. | ||
| * | ||
| * @param metadataToUpdate the base table metadata to apply changes to | ||
| * @param snapshot snapshot to apply the changes to | ||
| * @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 +361,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.