-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Core: allow creating v2 tables through table property #2887
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 3 commits
466fe26
257eaaf
4ba96b3
bff76bb
197fc6a
083dec1
34094f2
aae17e2
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 |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ | |
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
| import java.util.function.Predicate; | ||
| import java.util.stream.Collectors; | ||
| import org.apache.iceberg.exceptions.ValidationException; | ||
| import org.apache.iceberg.io.InputFile; | ||
| import org.apache.iceberg.relocated.com.google.common.base.MoreObjects; | ||
|
|
@@ -57,31 +58,29 @@ public class TableMetadata implements Serializable { | |
|
|
||
| private static final long ONE_MINUTE = TimeUnit.MINUTES.toMillis(1); | ||
|
|
||
| /** | ||
| * @deprecated will be removed in 0.9.0; use newTableMetadata(Schema, PartitionSpec, String, Map) instead. | ||
| */ | ||
| @Deprecated | ||
| public static TableMetadata newTableMetadata(TableOperations ops, | ||
| Schema schema, | ||
| PartitionSpec spec, | ||
| String location, | ||
| Map<String, String> properties) { | ||
| return newTableMetadata(schema, spec, SortOrder.unsorted(), location, properties, DEFAULT_TABLE_FORMAT_VERSION); | ||
| } | ||
|
|
||
| public static TableMetadata newTableMetadata(Schema schema, | ||
| PartitionSpec spec, | ||
| SortOrder sortOrder, | ||
| String location, | ||
| Map<String, String> properties) { | ||
| return newTableMetadata(schema, spec, sortOrder, location, properties, DEFAULT_TABLE_FORMAT_VERSION); | ||
| return newTableMetadata(schema, spec, sortOrder, location, unreservedProperties(properties), | ||
|
jackye1995 marked this conversation as resolved.
Outdated
|
||
| PropertyUtil.propertyAsInt(properties, TableProperties.RESERVED_PROPERTY_FORMAT_VERSION, | ||
| DEFAULT_TABLE_FORMAT_VERSION)); | ||
| } | ||
|
|
||
| public static TableMetadata newTableMetadata(Schema schema, | ||
| PartitionSpec spec, | ||
| String location, | ||
| Map<String, String> properties) { | ||
| return newTableMetadata(schema, spec, SortOrder.unsorted(), location, properties, DEFAULT_TABLE_FORMAT_VERSION); | ||
| return newTableMetadata(schema, spec, SortOrder.unsorted(), location, unreservedProperties(properties), | ||
| PropertyUtil.propertyAsInt(properties, TableProperties.RESERVED_PROPERTY_FORMAT_VERSION, | ||
|
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. nit: I think defining some helper variables will make it a little bit easier to read. I usually try to avoid splitting code over multiple lines if possible. |
||
| DEFAULT_TABLE_FORMAT_VERSION)); | ||
| } | ||
|
|
||
| private static Map<String, String> unreservedProperties(Map<String, String> rawProperties) { | ||
| return rawProperties.entrySet().stream() | ||
| .filter(e -> !TableProperties.RESERVED_PROPERTIES.contains(e.getKey())) | ||
| .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); | ||
| } | ||
|
|
||
| static TableMetadata newTableMetadata(Schema schema, | ||
|
|
@@ -677,12 +676,21 @@ private TableMetadata setCurrentSnapshotTo(Snapshot snapshot) { | |
| newSnapshotLog, addPreviousFile(file, lastUpdatedMillis)); | ||
| } | ||
|
|
||
| public TableMetadata replaceProperties(Map<String, String> newProperties) { | ||
| ValidationException.check(newProperties != null, "Cannot set properties to null"); | ||
| return new TableMetadata(null, formatVersion, uuid, location, | ||
| public TableMetadata replaceProperties(Map<String, String> rawProperties) { | ||
| ValidationException.check(rawProperties != null, "Cannot set properties to null"); | ||
| Map<String, String> newProperties = unreservedProperties(rawProperties); | ||
| TableMetadata metadata = new TableMetadata(null, formatVersion, uuid, location, | ||
| lastSequenceNumber, System.currentTimeMillis(), lastColumnId, currentSchemaId, schemas, defaultSpecId, specs, | ||
| lastAssignedPartitionId, defaultSortOrderId, sortOrders, newProperties, currentSnapshotId, snapshots, | ||
| snapshotLog, addPreviousFile(file, lastUpdatedMillis, newProperties)); | ||
|
|
||
| int newFormatVersion = PropertyUtil.propertyAsInt(rawProperties, | ||
| TableProperties.RESERVED_PROPERTY_FORMAT_VERSION, formatVersion); | ||
| if (formatVersion != newFormatVersion) { | ||
| metadata = metadata.upgradeToFormatVersion(newFormatVersion); | ||
|
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. It seems a bit weird to call another instance method to produce a second |
||
| } | ||
|
|
||
| return metadata; | ||
| } | ||
|
|
||
| public TableMetadata removeSnapshotLogEntries(Set<Long> snapshotIds) { | ||
|
|
@@ -752,9 +760,15 @@ public TableMetadata buildReplacement(Schema updatedSchema, PartitionSpec update | |
| sortOrdersBuilder.add(freshSortOrder); | ||
| } | ||
|
|
||
| Map<String, String> newProperties = Maps.newHashMap(); | ||
| newProperties.putAll(this.properties); | ||
| newProperties.putAll(updatedProperties); | ||
| // prepare new table properties | ||
| Map<String, String> newRawProperties = Maps.newHashMap(); | ||
| newRawProperties.putAll(this.properties); | ||
| newRawProperties.putAll(updatedProperties); | ||
| Map<String, String> newProperties = unreservedProperties(newRawProperties); | ||
|
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 it would be better to construct the properties directly rather than building a map and then rebuilding it. You should be able to use the same
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. You could have |
||
|
|
||
| // check if there is format version override | ||
| int newFormatVersion = PropertyUtil.propertyAsInt(newRawProperties, | ||
| TableProperties.RESERVED_PROPERTY_FORMAT_VERSION, formatVersion); | ||
|
|
||
| // determine the next schema id | ||
| int freshSchemaId = reuseOrCreateNewSchemaId(freshSchema); | ||
|
|
@@ -764,7 +778,7 @@ public TableMetadata buildReplacement(Schema updatedSchema, PartitionSpec update | |
| schemasBuilder.add(new Schema(freshSchemaId, freshSchema.columns(), freshSchema.identifierFieldIds())); | ||
| } | ||
|
|
||
| return new TableMetadata(null, formatVersion, uuid, newLocation, | ||
| return new TableMetadata(null, newFormatVersion, uuid, newLocation, | ||
|
jackye1995 marked this conversation as resolved.
Outdated
|
||
| lastSequenceNumber, System.currentTimeMillis(), newLastColumnId.get(), freshSchemaId, schemasBuilder.build(), | ||
| specId, specListBuilder.build(), Math.max(lastAssignedPartitionId, freshSpec.lastAssignedFieldId()), | ||
| orderId, sortOrdersBuilder.build(), ImmutableMap.copyOf(newProperties), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,11 +19,38 @@ | |
|
|
||
| package org.apache.iceberg; | ||
|
|
||
| import java.util.List; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; | ||
|
|
||
| public class TableProperties { | ||
|
|
||
| private TableProperties() { | ||
| } | ||
|
|
||
| /** | ||
| * Reserved table property for table format version. | ||
| * <p> | ||
| * Iceberg will default a new table's format version to the latest stable and recommended version. | ||
| * This reserved property keyword allows users to override the Iceberg format version of the table metadata. | ||
| * <p> | ||
| * If this table property exists when creating a table, the table will use the specified format version. | ||
| * If a table updates this property, it will try to upgrade to the specified format version. | ||
| * This helps developers to try out new features in an unreleased version or migrate existing tables to a new version. | ||
|
rdblue marked this conversation as resolved.
Outdated
|
||
| * <p> | ||
| * Note: incomplete or unstable versions cannot be selected using this property. | ||
|
rdblue marked this conversation as resolved.
|
||
| */ | ||
| public static final String RESERVED_PROPERTY_FORMAT_VERSION = "format-version"; | ||
|
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. nit: Do we add the Just asking. I am fine either way.
Contributor
Author
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. Yeah that's a good point, I had the prefix because there was no |
||
|
|
||
| /** | ||
| * Reserved Iceberg table properties list. | ||
| * <p> | ||
| * Reserved table properties are only used to control behaviors during the construction of table metadata. | ||
|
rdblue marked this conversation as resolved.
Outdated
|
||
| * The value of these properties are not persisted as a part of the table metadata. | ||
| */ | ||
| public static final List<String> RESERVED_PROPERTIES = ImmutableList.of( | ||
|
jackye1995 marked this conversation as resolved.
Outdated
|
||
| RESERVED_PROPERTY_FORMAT_VERSION | ||
| ); | ||
|
|
||
| public static final String COMMIT_NUM_RETRIES = "commit.retry.num-retries"; | ||
| public static final int COMMIT_NUM_RETRIES_DEFAULT = 4; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -756,4 +756,55 @@ public void testUpdateSchema() { | |
| Assert.assertEquals("Should return expected last column id", | ||
| 6, threeSchemaTable.lastColumnId()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testCreateV2MetadataThroughTableProperty() { | ||
|
Member
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. Should we also add the unit tests in flink/spark/hive module to verify the end-to-end DDL work ? If so, I think we could make it to be a separate PR for this.
Member
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 we could access the format version by using the following code: Table table = ...
TableOperations ops = ((BaseTable) table).operations();
TableMetadata meta = ops.current();
int formatVersion = meta.formatVersion(); |
||
| Schema schema = new Schema( | ||
| Types.NestedField.required(10, "x", Types.StringType.get()) | ||
| ); | ||
|
|
||
| TableMetadata meta = TableMetadata.newTableMetadata(schema, PartitionSpec.unpartitioned(), null, | ||
| ImmutableMap.of(TableProperties.RESERVED_PROPERTY_FORMAT_VERSION, "2", "key", "val")); | ||
|
|
||
| Assert.assertEquals("format version should be configured based on the format-version key", | ||
| 2, meta.formatVersion()); | ||
| Assert.assertEquals("should not contain format-version in properties", | ||
| ImmutableMap.of("key", "val"), meta.properties()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testReplaceV1MetadataToV2ThroughTableProperty() { | ||
| Schema schema = new Schema( | ||
| Types.NestedField.required(10, "x", Types.StringType.get()) | ||
| ); | ||
|
|
||
| TableMetadata meta = TableMetadata.newTableMetadata(schema, PartitionSpec.unpartitioned(), null, | ||
| ImmutableMap.of(TableProperties.RESERVED_PROPERTY_FORMAT_VERSION, "1", "key", "val")); | ||
|
|
||
| meta = meta.buildReplacement(meta.schema(), meta.spec(), meta.sortOrder(), meta.location(), | ||
| ImmutableMap.of(TableProperties.RESERVED_PROPERTY_FORMAT_VERSION, "2", "key2", "val2")); | ||
|
|
||
| Assert.assertEquals("format version should be configured based on the format-version key", | ||
| 2, meta.formatVersion()); | ||
| Assert.assertEquals("should not contain format-version but should contain old and new properties", | ||
| ImmutableMap.of("key", "val", "key2", "val2"), meta.properties()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testUpgradeV1MetadataToV2ThroughTableProperty() { | ||
| Schema schema = new Schema( | ||
| Types.NestedField.required(10, "x", Types.StringType.get()) | ||
| ); | ||
|
|
||
| TableMetadata meta = TableMetadata.newTableMetadata(schema, PartitionSpec.unpartitioned(), null, | ||
| ImmutableMap.of(TableProperties.RESERVED_PROPERTY_FORMAT_VERSION, "1", "key", "val")); | ||
|
|
||
| meta = meta.replaceProperties(ImmutableMap.of(TableProperties.RESERVED_PROPERTY_FORMAT_VERSION, | ||
| "2", "key2", "val2")); | ||
|
|
||
| Assert.assertEquals("format version should be configured based on the format-version key", | ||
| 2, meta.formatVersion()); | ||
| Assert.assertEquals("should not contain format-version but should contain new properties", | ||
| ImmutableMap.of("key2", "val2"), meta.properties()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for removing this!