This is an automated email from the ASF dual-hosted git repository.
etudenhoefner pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg.git
The following commit(s) were added to refs/heads/main by this push:
new 42a2c19cec Core: Only write view history when currentVersionId changes
(#9725)
42a2c19cec is described below
commit 42a2c19cec31c626cbff6cc2dfafb86cdf223bd0
Author: Eduard Tudenhoefner <[email protected]>
AuthorDate: Thu Feb 15 11:19:28 2024 +0100
Core: Only write view history when currentVersionId changes (#9725)
---
.../java/org/apache/iceberg/view/ViewMetadata.java | 20 ++-
.../rest/responses/TestLoadViewResponseParser.java | 12 --
.../org/apache/iceberg/view/TestViewMetadata.java | 136 +++++++++++++++++----
.../iceberg/view/TestViewMetadataParser.java | 48 +++++---
4 files changed, 152 insertions(+), 64 deletions(-)
diff --git a/core/src/main/java/org/apache/iceberg/view/ViewMetadata.java
b/core/src/main/java/org/apache/iceberg/view/ViewMetadata.java
index fa75c352f1..68d8c64587 100644
--- a/core/src/main/java/org/apache/iceberg/view/ViewMetadata.java
+++ b/core/src/main/java/org/apache/iceberg/view/ViewMetadata.java
@@ -31,6 +31,7 @@ import javax.annotation.Nullable;
import org.apache.iceberg.MetadataUpdate;
import org.apache.iceberg.Schema;
import org.apache.iceberg.exceptions.ValidationException;
+import
org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
@@ -156,6 +157,7 @@ public interface ViewMetadata extends Serializable {
// internal change tracking
private Integer lastAddedVersionId = null;
private Integer lastAddedSchemaId = null;
+ private ViewHistoryEntry historyEntry = null;
// indexes
private final Map<Integer, ViewVersion> versionsById;
@@ -243,6 +245,12 @@ public interface ViewMetadata extends Serializable {
changes.add(new MetadataUpdate.SetCurrentViewVersion(newVersionId));
}
+ this.historyEntry =
+ ImmutableViewHistoryEntry.builder()
+ .timestampMillis(version.timestampMillis())
+ .versionId(version.versionId())
+ .build();
+
return this;
}
@@ -307,12 +315,6 @@ public interface ViewMetadata extends Serializable {
changes.add(new MetadataUpdate.AddViewVersion(version));
}
- history.add(
- ImmutableViewHistoryEntry.builder()
- .timestampMillis(version.timestampMillis())
- .versionId(version.versionId())
- .build());
-
this.lastAddedVersionId = newVersionId;
return newVersionId;
@@ -438,6 +440,10 @@ public interface ViewMetadata extends Serializable {
metadataLocation == null || changes.isEmpty(),
"Cannot create view metadata with a metadata location and changes");
+ if (null != historyEntry) {
+ history.add(historyEntry);
+ }
+
int historySize =
PropertyUtil.propertyAsInt(
properties,
@@ -479,6 +485,7 @@ public interface ViewMetadata extends Serializable {
metadataLocation);
}
+ @VisibleForTesting
static List<ViewVersion> expireVersions(
Map<Integer, ViewVersion> versionsById, int numVersionsToKeep) {
// version ids are assigned sequentially. keep the latest versions by ID.
@@ -493,6 +500,7 @@ public interface ViewMetadata extends Serializable {
return retainedVersions;
}
+ @VisibleForTesting
static List<ViewHistoryEntry> updateHistory(List<ViewHistoryEntry>
history, Set<Integer> ids) {
List<ViewHistoryEntry> retainedHistory = Lists.newArrayList();
for (ViewHistoryEntry entry : history) {
diff --git
a/core/src/test/java/org/apache/iceberg/rest/responses/TestLoadViewResponseParser.java
b/core/src/test/java/org/apache/iceberg/rest/responses/TestLoadViewResponseParser.java
index d94d035596..f3de08cd29 100644
---
a/core/src/test/java/org/apache/iceberg/rest/responses/TestLoadViewResponseParser.java
+++
b/core/src/test/java/org/apache/iceberg/rest/responses/TestLoadViewResponseParser.java
@@ -137,12 +137,6 @@ public class TestLoadViewResponseParser {
+ " \"representations\" : [ ]\n"
+ " } ],\n"
+ " \"version-log\" : [ {\n"
- + " \"timestamp-ms\" : 23,\n"
- + " \"version-id\" : 1\n"
- + " }, {\n"
- + " \"timestamp-ms\" : 24,\n"
- + " \"version-id\" : 2\n"
- + " }, {\n"
+ " \"timestamp-ms\" : 25,\n"
+ " \"version-id\" : 3\n"
+ " } ]\n"
@@ -235,12 +229,6 @@ public class TestLoadViewResponseParser {
+ " \"representations\" : [ ]\n"
+ " } ],\n"
+ " \"version-log\" : [ {\n"
- + " \"timestamp-ms\" : 23,\n"
- + " \"version-id\" : 1\n"
- + " }, {\n"
- + " \"timestamp-ms\" : 24,\n"
- + " \"version-id\" : 2\n"
- + " }, {\n"
+ " \"timestamp-ms\" : 25,\n"
+ " \"version-id\" : 3\n"
+ " } ]\n"
diff --git a/core/src/test/java/org/apache/iceberg/view/TestViewMetadata.java
b/core/src/test/java/org/apache/iceberg/view/TestViewMetadata.java
index e60fe3b285..70f87e3974 100644
--- a/core/src/test/java/org/apache/iceberg/view/TestViewMetadata.java
+++ b/core/src/test/java/org/apache/iceberg/view/TestViewMetadata.java
@@ -23,7 +23,6 @@ import static
org.assertj.core.api.Assertions.assertThatThrownBy;
import java.util.List;
import java.util.Map;
-import java.util.Set;
import java.util.UUID;
import org.apache.iceberg.MetadataUpdate;
import org.apache.iceberg.Schema;
@@ -63,8 +62,11 @@ public class TestViewMetadata {
ViewVersion v2 = newViewVersion(2, "select count(1) as count from t2");
Map<Integer, ViewVersion> versionsById = ImmutableMap.of(1, v1, 2, v2, 3,
v3);
- List<ViewVersion> retainedVersions =
ViewMetadata.Builder.expireVersions(versionsById, 2);
- assertThat(retainedVersions).hasSameElementsAs(ImmutableList.of(v2, v3));
+ assertThat(ViewMetadata.Builder.expireVersions(versionsById, 3))
+ .containsExactlyInAnyOrder(v1, v2, v3);
+ assertThat(ViewMetadata.Builder.expireVersions(versionsById, 2))
+ .containsExactlyInAnyOrder(v2, v3);
+ assertThat(ViewMetadata.Builder.expireVersions(versionsById,
1)).containsExactly(v3);
}
@Test
@@ -73,26 +75,38 @@ public class TestViewMetadata {
ViewVersion v2 = newViewVersion(2, "select count(1) as count from t2");
ViewVersion v3 = newViewVersion(3, "select count from t1");
- Set<Integer> versionsById = ImmutableSet.of(2, 3);
-
- List<ViewHistoryEntry> history =
- ImmutableList.of(
- ImmutableViewHistoryEntry.builder()
- .versionId(v1.versionId())
- .timestampMillis(v1.timestampMillis())
- .build(),
- ImmutableViewHistoryEntry.builder()
- .versionId(v2.versionId())
- .timestampMillis(v2.timestampMillis())
- .build(),
- ImmutableViewHistoryEntry.builder()
- .versionId(v3.versionId())
- .timestampMillis(v3.timestampMillis())
- .build());
-
- List<ViewHistoryEntry> retainedHistory =
- ViewMetadata.Builder.updateHistory(history, versionsById);
- assertThat(retainedHistory).hasSameElementsAs(history.subList(1, 3));
+ ViewHistoryEntry one =
+ ImmutableViewHistoryEntry.builder()
+ .versionId(v1.versionId())
+ .timestampMillis(v1.timestampMillis())
+ .build();
+ ViewHistoryEntry two =
+ ImmutableViewHistoryEntry.builder()
+ .versionId(v2.versionId())
+ .timestampMillis(v2.timestampMillis())
+ .build();
+ ViewHistoryEntry three =
+ ImmutableViewHistoryEntry.builder()
+ .versionId(v3.versionId())
+ .timestampMillis(v3.timestampMillis())
+ .build();
+
+ assertThat(
+ ViewMetadata.Builder.updateHistory(
+ ImmutableList.of(one, two, three), ImmutableSet.of(1, 2, 3)))
+ .containsExactly(one, two, three);
+
+ // one was an invalid entry in the history, so all previous elements are
removed
+ assertThat(
+ ViewMetadata.Builder.updateHistory(
+ ImmutableList.of(three, two, one, two, three),
ImmutableSet.of(2, 3)))
+ .containsExactly(two, three);
+
+ // two was an invalid entry in the history, so all previous elements are
removed
+ assertThat(
+ ViewMetadata.Builder.updateHistory(
+ ImmutableList.of(one, two, three, one, three),
ImmutableSet.of(1, 3)))
+ .containsExactly(three, one, three);
}
@Test
@@ -233,7 +247,7 @@ public class TestViewMetadata {
}
@Test
- public void viewHistoryNormalization() {
+ public void viewVersionHistoryNormalization() {
Map<String, String> properties =
ImmutableMap.of(ViewProperties.VERSION_HISTORY_SIZE, "2");
ViewVersion viewVersionOne = newViewVersion(1, "select * from ns.tbl");
ViewVersion viewVersionTwo = newViewVersion(2, "select count(*) from
ns.tbl");
@@ -252,12 +266,12 @@ public class TestViewMetadata {
// the first build will not expire versions that were added in the builder
assertThat(originalViewMetadata.versions()).hasSize(3);
- assertThat(originalViewMetadata.history()).hasSize(3);
+ assertThat(originalViewMetadata.history()).hasSize(1);
// rebuild the metadata to expire older versions
ViewMetadata viewMetadata =
ViewMetadata.buildFrom(originalViewMetadata).build();
assertThat(viewMetadata.versions()).hasSize(2);
- assertThat(viewMetadata.history()).hasSize(2);
+ assertThat(viewMetadata.history()).hasSize(1);
// make sure that metadata changes reflect the current state after the
history was adjusted,
// meaning that the first view version shouldn't be included
@@ -314,6 +328,74 @@ public class TestViewMetadata {
.isEqualTo(-1);
}
+ @Test
+ public void viewVersionHistoryIsCorrectlyRetained() {
+ Map<String, String> properties =
ImmutableMap.of(ViewProperties.VERSION_HISTORY_SIZE, "2");
+ ViewVersion viewVersionOne = newViewVersion(1, "select * from ns.tbl");
+ ViewVersion viewVersionTwo = newViewVersion(2, "select count(*) from
ns.tbl");
+ ViewVersion viewVersionThree = newViewVersion(3, "select count(*) as count
from ns.tbl");
+
+ ViewMetadata originalViewMetadata =
+ ViewMetadata.builder()
+ .setProperties(properties)
+ .setLocation("location")
+ .addSchema(new Schema(Types.NestedField.required(1, "x",
Types.LongType.get())))
+ .addVersion(viewVersionOne)
+ .addVersion(viewVersionTwo)
+ .addVersion(viewVersionThree)
+ .setCurrentVersionId(3)
+ .build();
+
+ assertThat(originalViewMetadata.versions())
+ .hasSize(3)
+ .containsExactlyInAnyOrder(viewVersionOne, viewVersionTwo,
viewVersionThree);
+ assertThat(originalViewMetadata.history())
+ .hasSize(1)
+ .first()
+ .extracting(ViewHistoryEntry::versionId)
+ .isEqualTo(3);
+
+ // rebuild the metadata to expire older versions
+ ViewMetadata viewMetadata =
ViewMetadata.buildFrom(originalViewMetadata).build();
+ assertThat(viewMetadata.versions())
+ .hasSize(2)
+ // there is no requirement about the order of versions
+ .containsExactlyInAnyOrder(viewVersionThree, viewVersionTwo);
+ assertThat(viewMetadata.history())
+ .hasSize(1)
+ .first()
+ .extracting(ViewHistoryEntry::versionId)
+ .isEqualTo(3);
+
+ ViewMetadata updated =
ViewMetadata.buildFrom(viewMetadata).setCurrentVersionId(2).build();
+ assertThat(updated.versions())
+ .hasSize(2)
+ .containsExactlyInAnyOrder(viewVersionTwo, viewVersionThree);
+ assertThat(updated.history())
+ .hasSize(2)
+ .element(0)
+ .extracting(ViewHistoryEntry::versionId)
+ .isEqualTo(3);
+
assertThat(updated.history()).element(1).extracting(ViewHistoryEntry::versionId).isEqualTo(2);
+
+ ViewMetadata view =
ViewMetadata.buildFrom(updated).setCurrentVersionId(3).build();
+ assertThat(view.versions())
+ .hasSize(2)
+ .containsExactlyInAnyOrder(viewVersionTwo, viewVersionThree);
+ assertThat(view.history())
+ .hasSize(3)
+ .element(0)
+ .extracting(ViewHistoryEntry::versionId)
+ .isEqualTo(3);
+
assertThat(view.history()).element(1).extracting(ViewHistoryEntry::versionId).isEqualTo(2);
+
assertThat(view.history()).element(2).extracting(ViewHistoryEntry::versionId).isEqualTo(3);
+
+ // viewVersionId 1 has been removed from versions, so this should fail
+ assertThatThrownBy(() ->
ViewMetadata.buildFrom(view).setCurrentVersionId(1).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Cannot set current version to unknown version: 1");
+ }
+
@Test
public void viewMetadataAndMetadataChanges() {
Map<String, String> properties = ImmutableMap.of("key1", "prop1", "key2",
"prop2");
@@ -340,7 +422,7 @@ public class TestViewMetadata {
assertThat(viewMetadata.versions())
.hasSize(3)
.containsExactly(viewVersionOne, viewVersionTwo, viewVersionThree);
- assertThat(viewMetadata.history()).hasSize(3);
+ assertThat(viewMetadata.history()).hasSize(1);
assertThat(viewMetadata.currentVersionId()).isEqualTo(3);
assertThat(viewMetadata.currentVersion()).isEqualTo(viewVersionThree);
assertThat(viewMetadata.formatVersion()).isEqualTo(ViewMetadata.DEFAULT_VIEW_FORMAT_VERSION);
diff --git
a/core/src/test/java/org/apache/iceberg/view/TestViewMetadataParser.java
b/core/src/test/java/org/apache/iceberg/view/TestViewMetadataParser.java
index d237236372..7784fdc4ed 100644
--- a/core/src/test/java/org/apache/iceberg/view/TestViewMetadataParser.java
+++ b/core/src/test/java/org/apache/iceberg/view/TestViewMetadataParser.java
@@ -101,16 +101,20 @@ public class TestViewMetadataParser {
String json =
readViewMetadataInputFile("org/apache/iceberg/view/ValidViewMetadata.json");
ViewMetadata expectedViewMetadata =
- ViewMetadata.builder()
- .assignUUID("fa6506c3-7681-40c8-86dc-e36561f83385")
- .addSchema(TEST_SCHEMA)
- .addVersion(version1)
- .addVersion(version2)
- .setLocation("s3://bucket/test/location")
- .setProperties(
- ImmutableMap.of("some-key", "some-value",
ViewProperties.COMMENT, "some-comment"))
+ ViewMetadata.buildFrom(
+ ViewMetadata.builder()
+ .assignUUID("fa6506c3-7681-40c8-86dc-e36561f83385")
+ .addSchema(TEST_SCHEMA)
+ .addVersion(version1)
+ .addVersion(version2)
+ .setLocation("s3://bucket/test/location")
+ .setProperties(
+ ImmutableMap.of(
+ "some-key", "some-value", ViewProperties.COMMENT,
"some-comment"))
+ .setCurrentVersionId(1)
+ .upgradeFormatVersion(1)
+ .build())
.setCurrentVersionId(2)
- .upgradeFormatVersion(1)
.build();
ViewMetadata actual = ViewMetadataParser.fromJson(json);
@@ -213,17 +217,23 @@ public class TestViewMetadataParser {
String metadataLocation =
"s3://bucket/test/location/metadata/v1.metadata.json";
ViewMetadata expectedViewMetadata =
ViewMetadata.buildFrom(
- ViewMetadata.builder()
- .assignUUID("fa6506c3-7681-40c8-86dc-e36561f83385")
- .addSchema(TEST_SCHEMA)
- .addVersion(version1)
- .addVersion(version2)
- .setLocation("s3://bucket/test/location")
- .setProperties(
- ImmutableMap.of(
- "some-key", "some-value", ViewProperties.COMMENT,
"some-comment"))
+ ViewMetadata.buildFrom(
+ ViewMetadata.builder()
+ .assignUUID("fa6506c3-7681-40c8-86dc-e36561f83385")
+ .addSchema(TEST_SCHEMA)
+ .addVersion(version1)
+ .addVersion(version2)
+ .setLocation("s3://bucket/test/location")
+ .setProperties(
+ ImmutableMap.of(
+ "some-key",
+ "some-value",
+ ViewProperties.COMMENT,
+ "some-comment"))
+ .setCurrentVersionId(1)
+ .upgradeFormatVersion(1)
+ .build())
.setCurrentVersionId(2)
- .upgradeFormatVersion(1)
.build())
.setMetadataLocation(metadataLocation)
.build();
