jackye1995 commented on a change in pull request #3883:
URL: https://github.com/apache/iceberg/pull/3883#discussion_r789414852
##########
File path: api/src/main/java/org/apache/iceberg/Table.java
##########
@@ -138,6 +138,21 @@ default String name() {
*/
List<HistoryEntry> history();
+ /**
+ * Get the snapshot references of this table.
Review comment:
worth pointing out that there is always a snapshot ref for the main
branch. If there is no snapshot in the table, main branch points to -1
##########
File path: api/src/main/java/org/apache/iceberg/Table.java
##########
@@ -138,6 +138,21 @@ default String name() {
*/
List<HistoryEntry> history();
+ /**
+ * Get the snapshot references of this table.
+ *
+ * @return a map with ref name as key, {@link SnapshotRef} as value
+ */
+ Map<String, SnapshotRef> refs();
+
+ /**
+ * Get the snapshot reference with the given name
+ *
+ * @param refName snapshot reference name
+ * @return the SnapShot ref with the given name if it exists, null otherwise.
+ */
+ SnapshotRef ref(String refName);
Review comment:
nit: prefer simple variable name, `name` instead of `refName` since
`ref` is implied from the method name.
##########
File path: core/src/test/java/org/apache/iceberg/TestUpdateSnapshotRefs.java
##########
@@ -0,0 +1,255 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg;
+
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class TestUpdateSnapshotRefs extends TableTestBase {
+
+ private static final String TEST_BRANCH = "testBranch";
+ private static final String TEST_TAG = "testTag";
+
+ @Parameterized.Parameters(name = "formatVersion = {0}")
+ public static Object[] parameters() {
+ return new Object[] { 2 };
+ }
+
+ public TestUpdateSnapshotRefs(int formatVersion) {
+ super(formatVersion);
+ }
+
+ @Before
+ public void setupTable() throws Exception {
+ super.setupTable();
+
+ TableMetadata base = table.ops().current();
+
+ Map<String, SnapshotRef> refs = ImmutableMap.of(
+ TEST_BRANCH,
SnapshotRef.branchBuilder(123).maxSnapshotAgeMs(1L).minSnapshotsToKeep(1).build(),
+ TEST_TAG, SnapshotRef.tagBuilder(456).maxRefAgeMs(1L).build());
+
+ List<Snapshot> snapshots = Lists.newArrayList(
+ new BaseSnapshot(table.ops().io(), 789, null,
"file:/tmp/manifest1.avro"),
+ new BaseSnapshot(table.ops().io(), 456, null,
"file:/tmp/manifest1.avro"),
+ new BaseSnapshot(table.ops().io(), 123, null,
"file:/tmp/manifest1.avro"));
+
+ List<HistoryEntry> snapshotLogs = snapshots.stream()
+ .map(snapshot -> new
TableMetadata.SnapshotLogEntry(snapshot.snapshotId(), snapshot.snapshotId()))
+ .collect(Collectors.toList());
+
+ TableMetadata newTableMetadata = new TableMetadata(
+ metadataDir.getAbsolutePath(), base.formatVersion(), base.uuid(),
base.location(),
+ base.lastSequenceNumber(), base.lastUpdatedMillis(),
base.lastColumnId(), base.currentSchemaId(),
+ base.schemas(), base.defaultSpecId(), base.specs(),
base.lastAssignedPartitionId(),
+ base.defaultSortOrderId(), base.sortOrders(), base.properties(), 123,
snapshots,
+ snapshotLogs, base.previousFiles(), refs, ImmutableList.of());
+
+ table.ops().commit(base, newTableMetadata);
+ table.refresh();
+ }
+
+ @Test
+ public void testCreateTag() {
+ String tag = "newTag";
+ table.updateRefs().tag(tag, 789).commit();
+ table.refresh();
+ Assert.assertTrue("Should contain new tag", table.refs().containsKey(tag));
+ SnapshotRef ref = table.refs().get(tag);
+ Assert.assertEquals("Should create tag type ref", SnapshotRefType.TAG,
ref.type());
+ Assert.assertEquals("Should create tag for the given snapshot", 789,
ref.snapshotId());
+ }
+
+ @Test
+ public void testCreateTagNameInvalid() {
+ AssertHelpers.assertThrows("Should fail when tag name is null",
+ IllegalArgumentException.class,
+ "Tag name must not be null",
+ () -> table.updateRefs().tag(null, 789));
+
+ AssertHelpers.assertThrows("Should fail when tag name already exists as a
tag",
+ IllegalArgumentException.class,
+ "Cannot tag snapshot, ref already exists: testTag",
+ () -> table.updateRefs().tag("testTag", 789));
+
+ AssertHelpers.assertThrows("Should fail when tag name already exists as a
branch",
+ IllegalArgumentException.class,
+ "Cannot tag snapshot, ref already exists: testBranch",
+ () -> table.updateRefs().tag("testBranch", 789));
+ }
+
+ @Test
+ public void testCreateBranch() {
+ String branch = "newBranch";
+ table.updateRefs().branch(branch, 789).commit();
+ table.refresh();
+ Assert.assertTrue("Should contain new branch",
table.refs().containsKey(branch));
+ SnapshotRef ref = table.refs().get(branch);
+ Assert.assertEquals("Should create branch type ref",
SnapshotRefType.BRANCH, ref.type());
+ Assert.assertEquals("Should create branch for the given snapshot", 789,
ref.snapshotId());
+ }
+
+ @Test
+ public void testCreateBranchNameInvalid() {
+ AssertHelpers.assertThrows("Should fail when branch name is null",
+ IllegalArgumentException.class,
+ "Branch name must not be null",
+ () -> table.updateRefs().branch(null, 789));
+
+ AssertHelpers.assertThrows("Should fail when branch name already exists as
a tag",
+ IllegalArgumentException.class,
+ "Cannot create branch, ref already exists: testTag",
+ () -> table.updateRefs().branch(TEST_TAG, 789));
+
+ AssertHelpers.assertThrows("Should fail when branch name already exists as
a branch",
+ IllegalArgumentException.class,
+ "Cannot create branch, ref already exists: testBranch",
+ () -> table.updateRefs().branch(TEST_BRANCH, 789));
+ }
+
+ @Test
+ public void testCreateRefSnapshotNotExist() {
+ AssertHelpers.assertThrows("Should fail when snapshot to create tag does
not exist",
+ IllegalArgumentException.class,
+ "Cannot find snapshot with ID: 233",
+ () -> table.updateRefs().tag("tag", 233));
+
+ AssertHelpers.assertThrows("Should fail when snapshot to create branch
does not exist",
+ IllegalArgumentException.class,
+ "Cannot find snapshot with ID: 233",
+ () -> table.updateRefs().branch("branch", 233));
+ }
+
+ @Test
+ public void testSetRefLifetime() {
+ table.updateRefs()
+ .setLifetime(TEST_BRANCH, 10000)
+ .setLifetime(TEST_TAG, 20000)
+ .commit();
+ table.refresh();
+ Assert.assertEquals("Should have latest lifetime config for branch",
+ 10000, (long) table.refs().get(TEST_BRANCH).maxRefAgeMs());
+ Assert.assertEquals("Should have latest lifetime config for tag",
+ 20000, (long) table.refs().get(TEST_TAG).maxRefAgeMs());
+ }
+
+ @Test
+ public void testSetMainBranchLifetimeShouldFail() {
+ AssertHelpers.assertThrows("Should not be able to set lifetime for main
branch",
+ IllegalArgumentException.class,
+ "Main branch is retained forever",
+ () -> table.updateRefs().setLifetime(SnapshotRef.MAIN_BRANCH, 100));
+ }
+
+ @Test
+ public void testSetBranchSnapshotLifetime() {
+ table.updateRefs()
+ .setBranchSnapshotLifetime(TEST_BRANCH, 10000)
+ .commit();
+ table.refresh();
+ Assert.assertEquals("Should have latest snapshot lifetime config for
branch",
+ 10000, (long) table.refs().get(TEST_BRANCH).maxSnapshotAgeMs());
+ }
+
+ @Test
+ public void testSetMinSnapshotsInBranch() {
+ table.updateRefs()
+ .setMinSnapshotsInBranch(TEST_BRANCH, 10000)
+ .commit();
+ table.refresh();
+ Assert.assertEquals("Should have latest snapshot in branch config for
branch",
+ 10000, (int) table.refs().get(TEST_BRANCH).minSnapshotsToKeep());
+ }
+
+ @Test
+ public void testRenameTag() {
+ SnapshotRef tagRef = table.refs().get(TEST_TAG);
+ table.updateRefs().rename(TEST_TAG, "tag2").commit();
+ Assert.assertEquals("Renamed tag should have the same config", tagRef,
table.refs().get("tag2"));
+ }
+
+ @Test
+ public void testRenameBranch() {
+ SnapshotRef tagRef = table.refs().get(TEST_BRANCH);
+ table.updateRefs().rename(TEST_BRANCH, "branch2").commit();
+ Assert.assertEquals("Renamed branch should have the same config", tagRef,
table.refs().get("branch2"));
+ }
+
+ @Test
+ public void testInvalidRename() {
+ AssertHelpers.assertThrows("Should not have null from name",
+ IllegalArgumentException.class,
+ "Names must not be null",
+ () -> table.updateRefs().rename(null, "to"));
+
+ AssertHelpers.assertThrows("Should not have null to name",
+ IllegalArgumentException.class,
+ "Names must not be null",
+ () -> table.updateRefs().rename("from", null));
+
+ AssertHelpers.assertThrows("From ref must exist",
+ IllegalArgumentException.class,
+ "Cannot find ref to rename from: tag",
+ () -> table.updateRefs().rename("tag", "tag2"));
+
+ AssertHelpers.assertThrows("To ref must not exist",
+ IllegalArgumentException.class,
+ "Cannot rename to an existing ref: " + TEST_BRANCH,
+ () -> table.updateRefs().rename(TEST_TAG, TEST_BRANCH));
+ }
+
+ @Test
+ public void testRemoveTag() {
+ table.updateRefs().remove(TEST_TAG).commit();
+ Assert.assertNull("Removed tag should not exist",
table.refs().get(TEST_TAG));
+ }
+
+ @Test
+ public void testRemoveBranch() {
+ table.updateRefs().remove(TEST_BRANCH).commit();
+ Assert.assertNull("Removed branch should not exist",
table.refs().get(TEST_BRANCH));
+ }
+
+ @Test
+ public void testRemoveMainBranchShouldFail() {
+ AssertHelpers.assertThrows("Should fail when removing main branch",
+ IllegalArgumentException.class,
+ "Main branch must not be removed",
+ () -> table.updateRefs().remove(SnapshotRef.MAIN_BRANCH));
+ }
+
+ @Test
+ public void testRemoveRefNotExist() {
+ AssertHelpers.assertThrows("Should fail when removing ref not exist",
+ IllegalArgumentException.class,
+ "Cannot find ref to remove",
+ () -> table.updateRefs().remove("tag"));
+ }
Review comment:
I think we are missing a test for chaining multiple operations for
tag/branch, like
```
table.updateRefs()
.branch("b", 123)
.setMinSnapshotsInBranch("b", 10000)
.build()
```
to make sure chaining works as expected when creating new ref and set values
at the same time.
##########
File path: core/src/main/java/org/apache/iceberg/BaseTransaction.java
##########
@@ -587,6 +595,16 @@ public Snapshot snapshot(long snapshotId) {
return current.snapshotLog();
}
+ @Override
+ public Map<String, SnapshotRef> refs() {
+ return current.refs();
+ }
+
+ @Override
+ public SnapshotRef ref(String refName) {
Review comment:
nit: `name` as suggested before
##########
File path: api/src/main/java/org/apache/iceberg/UpdateSnapshotRefs.java
##########
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg;
+
+import java.util.Map;
+import org.apache.iceberg.exceptions.CommitFailedException;
+
+/**
+ * API for snapshot reference evolution.
+ * <p>
+ * When committing, these changes will be applied to the current table
metadata. Commit conflicts
Review comment:
nit: prefer full sentence in javadoc, change to a newline after `.`
##########
File path: core/src/main/java/org/apache/iceberg/SerializableTable.java
##########
@@ -262,6 +262,16 @@ public Snapshot snapshot(long snapshotId) {
return lazyTable().history();
}
+ @Override
+ public Map<String, SnapshotRef> refs() {
+ return lazyTable().refs();
+ }
+
+ @Override
+ public SnapshotRef ref(String refName) {
Review comment:
nit: `name` as suggested before
##########
File path: core/src/main/java/org/apache/iceberg/BaseTable.java
##########
@@ -124,6 +124,16 @@ public Snapshot snapshot(long snapshotId) {
return ops.current().snapshotLog();
}
+ @Override
+ public Map<String, SnapshotRef> refs() {
+ return ops.current().refs();
+ }
+
+ @Override
+ public SnapshotRef ref(String refName) {
Review comment:
nit: `name` as suggested before
##########
File path: api/src/main/java/org/apache/iceberg/Table.java
##########
@@ -138,6 +138,21 @@ default String name() {
*/
List<HistoryEntry> history();
+ /**
+ * Get the snapshot references of this table.
+ *
+ * @return a map with ref name as key, {@link SnapshotRef} as value
+ */
+ Map<String, SnapshotRef> refs();
+
+ /**
+ * Get the snapshot reference with the given name
+ *
+ * @param refName snapshot reference name
+ * @return the SnapShot ref with the given name if it exists, null otherwise.
Review comment:
nit: lower case snapshot
##########
File path: api/src/main/java/org/apache/iceberg/SnapshotRef.java
##########
@@ -0,0 +1,176 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg;
+
+import java.io.Serializable;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+public class SnapshotRef implements Serializable {
+
+ public static final String MAIN_BRANCH = "main";
+
+ private final long snapshotId;
+ private final SnapshotRefType type;
+ private final Integer minSnapshotsToKeep;
+ private final Long maxSnapshotAgeMs;
+ private final Long maxRefAgeMs;
+
+ private SnapshotRef(
+ long snapshotId,
+ SnapshotRefType type,
+ Integer minSnapshotsToKeep,
+ Long maxSnapshotAgeMs,
+ Long maxRefAgeMs) {
+ this.snapshotId = snapshotId;
+ this.type = type;
+ this.minSnapshotsToKeep = minSnapshotsToKeep;
+ this.maxSnapshotAgeMs = maxSnapshotAgeMs;
+ this.maxRefAgeMs = maxRefAgeMs;
+ }
+
+ public long snapshotId() {
+ return snapshotId;
+ }
+
+ public SnapshotRefType type() {
+ return type;
+ }
+
+ public Integer minSnapshotsToKeep() {
+ return minSnapshotsToKeep;
+ }
+
+ public Long maxSnapshotAgeMs() {
+ return maxSnapshotAgeMs;
+ }
+
+ public Long maxRefAgeMs() {
+ return maxRefAgeMs;
+ }
+
+ public static Builder tagBuilder(long snapshotId) {
+ return builderFor(snapshotId, SnapshotRefType.TAG);
+ }
+
+ public static Builder branchBuilder(long snapshotId) {
+ return builderFor(snapshotId, SnapshotRefType.BRANCH);
+ }
+
+ public static Builder builderFrom(SnapshotRef ref) {
+ return new Builder(ref.type(), ref.snapshotId())
+ .minSnapshotsToKeep(ref.minSnapshotsToKeep())
+ .maxSnapshotAgeMs(ref.maxSnapshotAgeMs())
+ .maxRefAgeMs(ref.maxRefAgeMs());
+ }
+ /**
+ * Creates a ref builder from the given ref and its properties but the ref
will now point to the given snapshotId.
+ * @param ref Ref to build from
Review comment:
nit: newline after javadoc description
##########
File path: core/src/main/java/org/apache/iceberg/SnapshotReferenceParser.java
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg;
+
+import com.fasterxml.jackson.core.JsonGenerator;
+import com.fasterxml.jackson.databind.JsonNode;
+import java.io.IOException;
+import java.io.StringWriter;
+import java.io.UncheckedIOException;
+import java.util.Locale;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.util.JsonUtil;
+
+public class SnapshotReferenceParser {
+
+ private SnapshotReferenceParser() {
+ }
+
+ private static final String SNAPSHOT_ID = "snapshot-id";
+ private static final String TYPE = "type";
+ private static final String MIN_SNAPSHOTS_TO_KEEP = "min-snapshots-to-keep";
+ private static final String MAX_SNAPSHOT_AGE_MS = "max-snapshot-age-ms";
+ private static final String MAX_REF_AGE_MS = "max-ref-age-ms";
+
+ public static String toJson(SnapshotRef ref) {
+ return toJson(ref, false);
+ }
+
+ public static String toJson(SnapshotRef ref, boolean pretty) {
+ try {
+ StringWriter writer = new StringWriter();
+ JsonGenerator generator = JsonUtil.factory().createGenerator(writer);
+ if (pretty) {
+ generator.useDefaultPrettyPrinter();
+ }
+
+ toJson(ref, generator);
+ generator.flush();
+ return writer.toString();
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ }
+
+ public static void toJson(SnapshotRef ref, JsonGenerator generator) throws
IOException {
+ generator.writeStartObject();
+ generator.writeNumberField(SNAPSHOT_ID, ref.snapshotId());
+ generator.writeStringField(TYPE,
ref.type().name().toLowerCase(Locale.ENGLISH));
+ JsonUtil.writeIntegerIfExists(MIN_SNAPSHOTS_TO_KEEP,
ref.minSnapshotsToKeep(), generator);
+ JsonUtil.writeLongIfExists(MAX_SNAPSHOT_AGE_MS, ref.maxSnapshotAgeMs(),
generator);
+ JsonUtil.writeLongIfExists(MAX_REF_AGE_MS, ref.maxRefAgeMs(), generator);
+ generator.writeEndObject();
+ }
+
+ public static SnapshotRef fromJson(String json) {
+ try {
+ return fromJson(JsonUtil.mapper().readValue(json, JsonNode.class));
+ } catch (IOException e) {
+ throw new UncheckedIOException("Failed to parse snapshot ref: " + json,
e);
+ }
+ }
+
+ public static SnapshotRef fromJson(JsonNode node) {
+ Preconditions.checkArgument(node.isObject(), "Cannot parse snapshot
reference from a non-object: %s", node);
+ long snapshotId = JsonUtil.getLong(SNAPSHOT_ID, node);
+ SnapshotRefType type = SnapshotRefType.valueOf(
+ JsonUtil.getString(TYPE, node).toUpperCase(Locale.ENGLISH));
Review comment:
does this need to be on the newline?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
