Skip to content

Hive: Add table UUID to HMS table properties. #3914

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.

Sign up for GitHub

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

Merged
RussellSpitzer merged 7 commits into from
Jan 24, 2022
Merged

Hive: Add table UUID to HMS table properties. #3914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
99b7555
Add UUID to metastore table params
Oct 27, 2021
bc1901a
Refactor
Jan 18, 2022
a6c30c3
Fix the test failure.
Jan 18, 2022
368f0cb
Not allow to create a table with uuid as a property
flyrain Jan 19, 2022
c734c24
Fix the test failure.
flyrain Jan 19, 2022
b092d29
Fix the test failure.
flyrain Jan 19, 2022
47fb910
Resolve the comment.
flyrain Jan 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion core/src/main/java/org/apache/iceberg/TableProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,22 @@ private TableProperties() {
*/
public static final String FORMAT_VERSION = "format-version";

/**
* Reserved table property for UUID.
* <p>
* This reserved property is used to store the UUID of the table.
*/
public static final String UUID = "uuid";

/**
* Reserved Iceberg table properties list.
* <p>
Copy link
Contributor

@kbendick kbendick Jan 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might need to add the new constant UUID to this set of reserved properties just below this line.

Copy link
Contributor Author

@flyrain flyrain Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. We should NOT allow user to add a uuid as a table property.

Copy link
Contributor Author

@flyrain flyrain Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add a test for that.

* Reserved table properties are only used to control behaviors when creating or updating a table.
* The value of these properties are not persisted as a part of the table metadata.
*/
public static final Set<String> RESERVED_PROPERTIES = ImmutableSet.of(
FORMAT_VERSION
FORMAT_VERSION,
UUID
);

public static final String COMMIT_NUM_RETRIES = "commit.retry.num-retries";
Expand Down
6 changes: 6 additions & 0 deletions core/src/test/java/org/apache/iceberg/TestTableMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -884,5 +884,11 @@ public void testNoReservedPropertyForTableMetadataCreation() {
"Table properties should not contain reserved properties, but got {format-version=1}",
() -> TableMetadata.newTableMetadata(schema, PartitionSpec.unpartitioned(), null, "/tmp",
ImmutableMap.of(TableProperties.FORMAT_VERSION, "1"), 1));

AssertHelpers.assertThrows("should not allow reserved table property when creating table metadata",
IllegalArgumentException.class,
"Table properties should not contain reserved properties, but got {uuid=uuid}",
() -> TableMetadata.newTableMetadata(schema, PartitionSpec.unpartitioned(), null, "/tmp",
ImmutableMap.of(TableProperties.UUID, "uuid"), 1));
}
}
6 changes: 2 additions & 4 deletions flink/v1.12/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;
import org.apache.flink.table.api.DataTypes;
Expand Down Expand Up @@ -178,10 +177,9 @@ public void testCreateTableIfNotExists() {
sql("CREATE TABLE IF NOT EXISTS tl(id BIGINT)");
Assert.assertEquals(Maps.newHashMap(), table("tl").properties());

final String uuid = UUID.randomUUID().toString();
final Map<String, String> expectedProperties = ImmutableMap.of("uuid", uuid);
final Map<String, String> expectedProperties = ImmutableMap.of("key", "value");
table("tl").updateProperties()
.set("uuid", uuid)
.set("key", "value")
.commit();
Assert.assertEquals(expectedProperties, table("tl").properties());

Expand Down
6 changes: 2 additions & 4 deletions flink/v1.13/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;
import org.apache.flink.table.api.DataTypes;
Expand Down Expand Up @@ -179,10 +178,9 @@ public void testCreateTableIfNotExists() {
sql("CREATE TABLE IF NOT EXISTS tl(id BIGINT)");
Assert.assertEquals(Maps.newHashMap(), table("tl").properties());

final String uuid = UUID.randomUUID().toString();
final Map<String, String> expectedProperties = ImmutableMap.of("uuid", uuid);
final Map<String, String> expectedProperties = ImmutableMap.of("key", "value");
table("tl").updateProperties()
.set("uuid", uuid)
.set("key", "value")
.commit();
Assert.assertEquals(expectedProperties, table("tl").properties());

Expand Down
6 changes: 2 additions & 4 deletions flink/v1.14/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;
import org.apache.flink.table.api.DataTypes;
Expand Down Expand Up @@ -179,10 +178,9 @@ public void testCreateTableIfNotExists() {
sql("CREATE TABLE IF NOT EXISTS tl(id BIGINT)");
Assert.assertEquals(Maps.newHashMap(), table("tl").properties());

final String uuid = UUID.randomUUID().toString();
final Map<String, String> expectedProperties = ImmutableMap.of("uuid", uuid);
final Map<String, String> expectedProperties = ImmutableMap.of("key", "value");
table("tl").updateProperties()
.set("uuid", uuid)
.set("key", "value")
Copy link
Contributor Author

@flyrain flyrain Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make the same change for the same test in v1.12 and v1.13?

Copy link
Contributor Author

@flyrain flyrain Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI said yes :-( . Made the change.

.commit();
Assert.assertEquals(expectedProperties, table("tl").properties());

Expand Down
9 changes: 6 additions & 3 deletions hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
Map<String, String> summary = Optional.ofNullable(metadata.currentSnapshot())
.map(Snapshot::summary)
.orElseGet(ImmutableMap::of);
setHmsTableParameters(newMetadataLocation, tbl, metadata.properties(), removedProps, hiveEngineEnabled, summary);
setHmsTableParameters(newMetadataLocation, tbl, metadata, removedProps, hiveEngineEnabled, summary);

if (!keepHiveStats) {
tbl.getParameters().remove(StatsSetupConst.COLUMN_STATS_ACCURATE);
Expand Down Expand Up @@ -352,18 +352,21 @@ private Table newHmsTable() {
return newTable;
}

private void setHmsTableParameters(String newMetadataLocation, Table tbl, Map<String, String> icebergTableProps,
private void setHmsTableParameters(String newMetadataLocation, Table tbl, TableMetadata metadata,
Set<String> obsoleteProps, boolean hiveEngineEnabled,
Map<String, String> summary) {
Map<String, String> parameters = Optional.ofNullable(tbl.getParameters())
.orElseGet(Maps::newHashMap);

// push all Iceberg table properties into HMS
icebergTableProps.forEach((key, value) -> {
metadata.properties().forEach((key, value) -> {
// translate key names between Iceberg and HMS where needed
String hmsKey = ICEBERG_TO_HMS_TRANSLATION.getOrDefault(key, key);
parameters.put(hmsKey, value);
});
if (metadata.uuid() != null) {
parameters.put(TableProperties.UUID, metadata.uuid());
}

// remove any props from HMS that are no longer present in Iceberg table props
obsoleteProps.forEach(parameters::remove);
Expand Down
26 changes: 26 additions & 0 deletions hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.iceberg.Schema;
import org.apache.iceberg.SortOrder;
import org.apache.iceberg.Table;
import org.apache.iceberg.TableProperties;
import org.apache.iceberg.Transaction;
import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.Namespace;
Expand Down Expand Up @@ -442,4 +443,29 @@ private String defaultUri(Namespace namespace) throws TException {
"hive.metastore.warehouse.dir", "") + "/" + namespace.level(0) + ".db";
}

@Test
public void testUUIDinTableProperties() throws Exception {
Schema schema = new Schema(
required(1, "id", Types.IntegerType.get(), "unique ID"),
required(2, "data", Types.StringType.get())
);
TableIdentifier tableIdentifier = TableIdentifier.of(DB_NAME, "tbl");
String location = temp.newFolder("tbl").toString();

try {
catalog.buildTable(tableIdentifier, schema)
.withLocation(location)
.create();

String tableName = tableIdentifier.name();
org.apache.hadoop.hive.metastore.api.Table hmsTable =
metastoreClient.getTable(tableIdentifier.namespace().level(0), tableName);

// check parameters are in expected state
Map<String, String> parameters = hmsTable.getParameters();
Assert.assertNotNull(parameters.get(TableProperties.UUID));
} finally {
catalog.dropTable(tableIdentifier);
}
}
}
4 changes: 2 additions & 2 deletions mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ public void testIcebergAndHmsTableProperties() throws Exception {
Assert.assertEquals(expectedIcebergProperties, icebergTable.properties());

if (Catalogs.hiveCatalog(shell.getHiveConf(), tableProperties)) {
Assert.assertEquals(10, hmsParams.size());
Assert.assertEquals(11, hmsParams.size());
Assert.assertEquals("initial_val", hmsParams.get("custom_property"));
Assert.assertEquals("TRUE", hmsParams.get(InputFormatConfig.EXTERNAL_TABLE_PURGE));
Assert.assertEquals("TRUE", hmsParams.get("EXTERNAL"));
Expand Down Expand Up @@ -662,7 +662,7 @@ public void testIcebergAndHmsTableProperties() throws Exception {
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));

if (Catalogs.hiveCatalog(shell.getHiveConf(), tableProperties)) {
Assert.assertEquals(13, hmsParams.size()); // 2 newly-added properties + previous_metadata_location prop
Assert.assertEquals(14, hmsParams.size()); // 2 newly-added properties + previous_metadata_location prop
Copy link
Member

@RussellSpitzer RussellSpitzer Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Could we convert 14 -> an expression that calculates the expected number of properties rather than having to manually change this whenever we add new properties?

Copy link
Contributor Author

@flyrain flyrain Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to avoid magic number. Here, I'd just leave it as is. Adding a reserved table property doesn't happen frequently. I'd avoid a smart solution to calculate the property number, which add complexity to the unit test. But I'm also open to any nice solution.

Assert.assertEquals("true", hmsParams.get("new_prop_1"));
Assert.assertEquals("false", hmsParams.get("new_prop_2"));
Assert.assertEquals("new_val", hmsParams.get("custom_property"));
Expand Down