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

flyrain · 2022-01-18T06:08:11Z

#2123 pushes Iceberg table property values to HMS table properties. It'd be nice to add table uuid to HMS table properties as well. Uuid is critical to identify a table. This change is to support the application that wants to identify a table through its uuid by only access the HMS.

(cherry picked from commit f88366a)

(cherry picked from commit 7d66707)

pvary · 2022-01-18T11:20:07Z

As per our previous discussions, in the long term we would like to keep Iceberg table properties and HMS table properties in sync. With this change and with the Hive 4.0.0 changes in place the UUID will be synchronised back to the Iceberg table properties too. Would this be a problem?

rdblue · 2022-01-18T16:25:22Z

With this change and with the Hive 4.0.0 changes in place the UUID will be synchronised back to the Iceberg table properties too. Would this be a problem?

I don't think that we'd want to do it. Maybe this is okay for just the Iceberg integration since it likely won't be used with Hive 4?

(cherry picked from commit e8b9c435722b079035ba3365aae8ba4326c8ad65)

flyrain · 2022-01-18T17:37:58Z

As per our previous discussions, in the long term we would like to keep Iceberg table properties and HMS table properties in sync. With this change and with the Hive 4.0.0 changes in place the UUID will be synchronised back to the Iceberg table properties too. Would this be a problem?

UUID will be considered as immutable, read-only in HMS. It isn't needed to be synced back. It'd be nice if there is a way to label it read-only in HMS. Not sure if that is possible.

Hi @aokolnychyi, @RussellSpitzer, @szehon-ho, @anuragmantri can you take a look at this PR?

kbendick · 2022-01-18T20:34:20Z

core/src/main/java/org/apache/iceberg/TableProperties.java

+
  /**
   * Reserved Iceberg table properties list.
   * <p>


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

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

Also add a test for that.

flyrain · 2022-01-19T01:34:44Z

flink/v1.14/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java

+    final Map<String, String> expectedProperties = ImmutableMap.of("key", "value");
    table("tl").updateProperties()
-        .set("uuid", uuid)
+        .set("key", "value")


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

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

anuragmantri · 2022-01-23T00:44:48Z

I'm not aware of a way to mark a table property as immutable in HMS. This is not a blocking comment. Just curious. Could you elaborate more on how we would use this UUID from HMS directly?

flyrain · 2022-01-23T04:19:01Z

I'm not aware of a way to mark a table property as immutable in HMS. This is not a blocking comment. Just curious. Could you elaborate more on how we would use this UUID from HMS directly?

An application can identify an Iceberg table by just checking the properties in HMS without reading the metadata.json. It is pretty useful in case that the application doesn't have the permission to access the file system.

RussellSpitzer · 2022-01-24T13:59:33Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

          .map(Snapshot::summary)
          .orElseGet(ImmutableMap::of);
-      setHmsTableParameters(newMetadataLocation, tbl, metadata.properties(), removedProps, hiveEngineEnabled, summary);
+      setHmsTableParameters(newMetadataLocation, tbl, metadata.properties(), removedProps, hiveEngineEnabled, summary,


~~I wonder if we should be passing through the metadata here instead of "tbl, metadata.properties and uuid"~~

Forgot which table this was. I still think we should probably pass through metadata instead of metadata.properties and uuid now that we use both things. Not a big deal though if you want to leave it as separate parameters

Made the change

RussellSpitzer · 2022-01-24T14:04:20Z

mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java


    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


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?

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.

RussellSpitzer

One minor suggestion, but this all looks good to me

RussellSpitzer

Ok this all looks good to me. Going to merge unless I hear any other comments in the next hour or so.

RussellSpitzer · 2022-01-24T19:33:11Z

Ok this all looks good to me. Going to merge unless I hear any other comments in the next hour or so.

@pvary or @rdblue ? You commented on the issue earlier?

anuragmantri · 2022-01-24T19:40:41Z

This looks good to me. Thanks.

rdblue · 2022-01-24T19:40:46Z

My main requirement is that this is a one-way transfer only. UUID should not show up in Iceberg table properties. As long as that's the case, I'm fine with this.

flyrain · 2022-01-24T20:00:05Z

My main requirement is that this is a one-way transfer only. UUID should not show up in Iceberg table properties. As long as that's the case, I'm fine with this.

Yes. One-way. And it won't show up as an Iceberg table property.

RussellSpitzer · 2022-01-24T20:33:54Z

My main requirement is that this is a one-way transfer only. UUID should not show up in Iceberg table properties. As long as that's the case, I'm fine with this.

Yes. One-way. And it won't show up as an Iceberg table property.

This would require any future implementations that automatically try sync Hive properties back into Iceberg properties to ignore reserved properties, which is the behavior I think we are looking for here. Basically, any reserved properties can not be modified through normal property operations.

RussellSpitzer · 2022-01-24T20:42:02Z

Thanks @flyrain and @karuppayya ! Note that this will introduce a slight backwards incompatibility as a user who has previously set UUID as a table property may run into some issues

Karuppayya Rajendran and others added 2 commits January 17, 2022 22:03

Add UUID to metastore table params

99b7555

(cherry picked from commit f88366a)

Refactor

bc1901a

(cherry picked from commit 7d66707)

github-actions bot added core hive labels Jan 18, 2022

Fix the test failure.

a6c30c3

(cherry picked from commit e8b9c435722b079035ba3365aae8ba4326c8ad65)

github-actions bot added the MR label Jan 18, 2022

kbendick reviewed Jan 18, 2022

View reviewed changes

flyrain added 2 commits January 18, 2022 16:16

Not allow to create a table with uuid as a property

368f0cb

Fix the test failure.

c734c24

github-actions bot added the flink label Jan 19, 2022

flyrain commented Jan 19, 2022

View reviewed changes

Fix the test failure.

b092d29

RussellSpitzer reviewed Jan 24, 2022

View reviewed changes

RussellSpitzer approved these changes Jan 24, 2022

View reviewed changes

Resolve the comment.

47fb910

RussellSpitzer approved these changes Jan 24, 2022

View reviewed changes

RussellSpitzer merged commit 2ea62c4 into apache:master Jan 24, 2022

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

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

Uh oh!

Conversation

flyrain commented Jan 18, 2022

Uh oh!

pvary commented Jan 18, 2022

Uh oh!

rdblue commented Jan 18, 2022

Uh oh!

flyrain commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kbendick Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

flyrain Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

flyrain Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

flyrain Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

anuragmantri commented Jan 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flyrain commented Jan 23, 2022

Uh oh!

RussellSpitzer Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

flyrain Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer left a comment

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer left a comment

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer commented Jan 24, 2022

Uh oh!

anuragmantri commented Jan 24, 2022

Uh oh!

rdblue commented Jan 24, 2022

Uh oh!

flyrain commented Jan 24, 2022

Uh oh!

RussellSpitzer commented Jan 24, 2022

Uh oh!

RussellSpitzer commented Jan 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

flyrain commented Jan 18, 2022 •

edited

Loading

flyrain Jan 18, 2022 •

edited

Loading

anuragmantri commented Jan 23, 2022 •

edited

Loading

RussellSpitzer Jan 24, 2022 •

edited

Loading