Add extra_properties to hive table properties #16846

Praveen2112 · 2023-04-03T07:07:08Z

Description

This PR allows us to pass through additional properties to Hive when creating table in Trino.
The additional properties can provided in the following format

extra_properties = map_from_entries(ARRAY[ROW('extra.property.one', 'one'),ROW('extra.property.two', 'two')]

Additional context and related issues

Fixes #954
Extension of #9475

Release notes

(x) Release notes are required, with the following suggested text:

# Section
* Introduce `extra_properties` for adding arbitrary properties to hive table. ({issue}`954`)

Praveen2112 · 2023-04-03T07:10:35Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

+    private static final Set<String> TABLE_PROPERTIES_TO_SKIP = ImmutableSet.of(
+            PRESTO_VERSION_NAME,
+            PRESTO_QUERY_ID_NAME,
+            BUCKETING_VERSION,
+            "EXTERNAL",
+            "numFiles",
+            "totalSize",
+            "last_modified_time",
+            "transient_lastDdlTime",
+            "last_modified_by",
+            "STATS_GENERATED_VIA_STATS_TASK",
+            "COLUMN_STATS_ACCURATE",
+            "numRows",
+            "rawDataSize");


Should we introduce another field in Table which captures the these properties to be filtered ?

Or can we have inject these properties via Guice ? So in future each metastore module can have its own bunch of properties to be filtered.

I'm against the Guice option for now. Let's keep it simple

Just thinking out loud. Wouldn't it be better/safer to whitelist rather than blacklist ?
I mean list properties we allow to read (as we know them and expect what they bring) rather that list properties to skip, but if we forget something or something new would be introduced we will blindly allow it ?

But how would we handle them if an arbitrary properties is added by a different system like hive or spark then we might not be able to list them.

findinpath · 2023-04-03T07:40:47Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveTableProperties.java

+                        false),
+                new PropertyMetadata<>(
+                        EXTRA_PROPERTIES,
+                        "Extra table properties",


Extra -> Arbitrary

I'd argue that arbitrary is a better name than extra.

Something that's arbitrary seems like it's chosen at random instead of following a consistent rule.

I'm not sure that I agree with "arbitrary", because these properties presumably have meaning- just not one that Trino understands. Maybe... "additional_properties"? It's pretty close to "extra_properties", but maybe slightly clearer than "extra". Naming is hard.

How about additional_properties - WDYT @findinpath ?

I find that additional is in the very same category as extra.

these properties presumably have meaning- just not one that Trino understands.

Definitely, that's why actually arbitrary is definitely not the best choice.

From Chat GPT

A possible word for a non-canonical property name could be "unconventional" or "non-standard". Another word that could be used is "ad-hoc", which implies that the property name was created on a temporary or as-needed basis, rather than being part of a predefined standard or convention. Additionally, the term "custom" could also be used to describe a non-canonical property name, as it suggests that the name was specifically tailored to a particular use case or application.

From the answer above, we have a few candidates:

unconventional

ad hoc

custom

I tend to go with ad_hoc_properties - it matches IMO best the purpose.

Based on offliine discussion we finalized the property name to extra_properties

findinpath · 2023-04-03T07:49:26Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

+            return table;
+        }
+
+        public Map<String, String> getUnconsumedProperty()


getUnconsumedProperty -> getUnconsumedProperties

findinpath · 2023-04-04T07:44:51Z

core/trino-main/src/main/java/io/trino/sql/rewrite/ShowQueriesRewrite.java

+            if (value instanceof Map) {
+                Map<?, ?> map = (Map<?, ?>) value;


Suggested change

if (value instanceof Map) {

Map<?, ?> map = (Map<?, ?>) value;

if (value instanceof Map<?, ?> map) {

findinpath · 2023-04-04T08:39:32Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java

+    @Test
+    public void testExtraProperties()
+    {
+        String tableName = format("%s.%s.test_extra_properties", getSession().getCatalog().get(), getSession().getSchema().get());


Is support for setting extra properties a follow-up?

alter table test3 set properties extra_properties = map_from_entries(ARRAY[ROW('extra2', 'hello'),ROW('extra1', 'world')]);

But hive doesn't support altering table properties right ? i.e HiveMetadata doesn't implement setTableProperties yet. Or did I miss something here

As far as i remember we can only set properties during table create.

Yes. It would be as a follow-up once we have this functionality for hive connector.

findinpath · 2023-04-04T08:43:19Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

@@ -1157,6 +1181,14 @@ else if (avroSchemaLiteral != null) {
        tableProperties.put("numFiles", "-1");
        tableProperties.put("totalSize", "-1");

+        // Extra properties
+        Map<String, String> extraProperties = getExtraProperties(tableMetadata.getProperties());
+        if (extraProperties != null) {


Let's check as well if the map is not empty.

findinpath · 2023-04-04T08:45:08Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

+        // Extra properties
+        Map<String, String> extraProperties = getExtraProperties(tableMetadata.getProperties());
+        if (extraProperties != null) {
+            for (Map.Entry<String, String> extraProperty : extraProperties.entrySet()) {


trino:default> create table test4(x integer) with (extra_properties= MAP(ARRAY['auto.purge'], ARRAY['true'])); Query 20230404_084054_00019_fr53z failed: Multiple entries with same key: auto.purge=true and auto.purge=false java.lang.IllegalArgumentException: Multiple entries with same key: auto.purge=true and auto.purge=false at com.google.common.collect.ImmutableMap.conflictException(ImmutableMap.java:377) at com.google.common.collect.ImmutableMap.checkNoConflict(ImmutableMap.java:371) at com.google.common.collect.RegularImmutableMap.checkNoConflictInKeyBucket(RegularImmutableMap.java:241) at com.google.common.collect.RegularImmutableMap.fromEntryArrayCheckingBucketOverflow(RegularImmutableMap.java:132) at com.google.common.collect.RegularImmutableMap.fromEntryArray(RegularImmutableMap.java:94) at com.google.common.collect.ImmutableMap$Builder.build(ImmutableMap.java:573) at com.google.common.collect.ImmutableMap$Builder.buildOrThrow(ImmutableMap.java:601) at io.trino.plugin.hive.HiveMetadata.getEmptyTableProperties(HiveMetadata.java:1198)

We could improve the error message to signal to the user that the statement is making use of an illegal parameter.
Pls add a corresponding test

findinpath · 2023-04-04T09:02:37Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java

+        assertEquals(getOnlyElement(actualResult.getOnlyColumnAsSet()), expectedShowCreateTableSql);
+        assertUpdate("DROP TABLE " + tableName);
+    }
+


trino:default> create table test10( -> name varchar(10), -> comment varchar(10), -> nationkey bigint, -> regionkey bigint, -> short_name varchar(100) -> with (partition_projection_type = 'enum', partition_projection_values = array['PL1', 'CZ1']) -> ) -> with ( -> partitioned_by = array['short_name'], -> partition_projection_enabled = true, -> extra_properties = map_from_entries(ARRAY[ROW('extra2', 'hello'),ROW('extra1', 'world')])); CREATE TABLE trino:default> show create table test10; Create Table ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- CREATE TABLE hive.default.test10 ( name varchar(10), comment varchar(10), nationkey bigint, regionkey bigint, short_name varchar(100) ) WITH ( extra_properties = map_from_entries(ARRAY[ROW('extra2', 'hello'),ROW('projection.short_name.type', 'enum'),ROW('extra1', 'world'),ROW('projection.short_name.values', 'PL1,CZ1')]), format = 'ORC', partition_projection_enabled = true, partitioned_by = ARRAY['short_name'] ) (1 row)

Is this intended to have the partition projection related properties displayed this way ?

cc @aczajkowski

I would be very cautious here as it was a conscious decision to not blindly present all properties as table properties.
As in case of partition projection in Hive some properties are presented as table properties even they are pointing to columns. Which creates a lot of potential issues (e.g dropping column, keeping property etc..).
Thats why we decided to keep them / present as table column properties.
So seems there should be no problem, as they will be just additionally presented in extra_properties but it might bring confusion here as they will be named differently as well.
CC: @findepi

follow-up show create table issue related to partition projection #16873

So seems there should be no problem, as they will be just additionally presented in extra_properties

they shouldn't be double-presented. if these show up as column properties, they shouldn't show up as table properties.

For now partition projection related properties displayed as extra properties - handling them for now would be a bit tricky - It will be resolved when we fix #16873 - as the partition projection property will be fetched from TableParameterProvider and it tracks the list of consumed properties.

What is your opinion @findinpath

Is fixing #16873 something that you're considering doing @Praveen2112 ?
I'm thinking it is worth taking a stab at it just to get more knowledge of eventual corner cases which are not yet easily visible and are relevant for this feature.

skrzypo987 · 2023-04-04T09:10:25Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/aws/athena/PartitionProjectionService.java

+            String targetPropertyKey,
+            Function<I, V> valueMapper)
+    {
+        Optional.ofNullable(sourcePropertyProvider.apply(sourcePropertyKey))


nit: I believe making optional to simply check if something is not null should be replaced with a simple if for readability. But this has not been changed here, so it's ok for me to leave it

skrzypo987 · 2023-04-04T09:24:27Z

core/trino-main/src/main/java/io/trino/sql/rewrite/ShowQueriesRewrite.java

+            if (value instanceof Map) {
+                Map<?, ?> map = (Map<?, ?>) value;
+                return new FunctionCall(QualifiedName.of("map_from_entries"), ImmutableList.of(new Array(map.entrySet().stream()
+                        .map(entry -> new Row(asList(toExpression(entry.getKey()), toExpression(entry.getValue()))))


asList -> ImmutableList.of

skrzypo987 · 2023-04-04T09:26:58Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

+    private static final Set<String> TABLE_PROPERTIES_TO_SKIP = ImmutableSet.of(
+            PRESTO_VERSION_NAME,
+            PRESTO_QUERY_ID_NAME,
+            BUCKETING_VERSION,
+            "EXTERNAL",
+            "numFiles",
+            "totalSize",
+            "last_modified_time",
+            "transient_lastDdlTime",
+            "last_modified_by",
+            "STATS_GENERATED_VIA_STATS_TASK",
+            "COLUMN_STATS_ACCURATE",
+            "numRows",
+            "rawDataSize");


I'm against the Guice option for now. Let's keep it simple

skrzypo987 · 2023-04-04T09:32:12Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

    {
-        return getSerdeProperty(table, key).map(csvSerdeProperty -> csvSerdeProperty.substring(0, 1));
+        return getSerdeProperty(tableParameterProvider, key).map(csvSerdeProperty -> csvSerdeProperty.substring(0, 1));


Are we certain than csvSerdeProperty is not empty? If it is we are having an exception here

skrzypo987 · 2023-04-04T09:34:10Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

+    public static class TableParameterProvider
+    {
+        private final Table table;
+        private final Set<String> consumedParameters = new HashSet<>(TABLE_PROPERTIES_TO_SKIP);


ImmutableSet?

This set is per-instance and mutated as part of getParameter(String key)

skrzypo987 · 2023-04-04T09:37:35Z

...rino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveSparkCompatibility.java

-                                ")",
-                        trinoTableName,
-                        expectedTrinoTableFormat)));
+        Assertions.assertThat((String) onTrino().executeQuery("SHOW CREATE TABLE " + trinoTableName).getOnlyValue())


static import assertThat

There is alternative QueryAssert.assertThat used in the PTs.

pettyjamesm · 2023-04-05T15:29:34Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/aws/athena/PartitionProjectionService.java

+        rewriteProperty(sourceProperties::get, targetPropertiesBuilder, sourcePropertyKey, targetPropertyKey, valueMapper);
+    }
+
+    private <I, V> void rewriteProperty(


Nit: this method (and the pre-existing method this is overloading) could both be static.

pettyjamesm · 2023-04-05T17:43:28Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

+    public static class TableParameterProvider
+    {
+        private final Table table;
+        private final Set<String> consumedParameters = new HashSet<>(TABLE_PROPERTIES_TO_SKIP);


This set is per-instance and mutated as part of getParameter(String key)

pettyjamesm · 2023-04-05T17:45:23Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

+
+        public Map<String, String> getUnconsumedProperty()
+        {
+            return table.getParameters().entrySet().stream()


This could be tracked incrementally as part of getParameter(String key) by taking an initial snapshot of table.getProperties() in the constructor, removing each property that gets consumed, and just returning the remaining map entries at this point.

But if we need to use the same properties at multiple places then we might not be able to handle them.

pettyjamesm · 2023-04-05T17:46:54Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveTableProperties.java

+                        false),
+                new PropertyMetadata<>(
+                        EXTRA_PROPERTIES,
+                        "Extra table properties",


I'm not sure that I agree with "arbitrary", because these properties presumably have meaning- just not one that Trino understands. Maybe... "additional_properties"? It's pretty close to "extra_properties", but maybe slightly clearer than "extra". Naming is hard.

- Remove unused methods - Make rewriteProperty static - Extract rewriteProperty to consume `Function<String, I>`

Praveen2112 · 2023-04-20T14:32:04Z

@findinpath / @pettyjamesm AC

findinpath · 2023-04-21T13:12:04Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

+            "COLUMN_STATS_ACCURATE",
+            "numRows",
+            "rawDataSize",
+            "numFilesErasureCoded");


How does this property get produced ? I'm not aware of it.

findinpath · 2023-04-21T13:14:24Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

@@ -3846,4 +3892,33 @@ public boolean supportsReportingWrittenBytes(ConnectorSession session, SchemaTab
    {
        return true;
    }
+
+    public static class TableParameterProvider


Let's extract this class from HiveMetadata.

findinpath · 2023-04-21T13:15:50Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

+            return table;
+        }
+
+        public Map<String, String> getUnconsumedProperties()


getUnconsumedProperties -> getExtraProperties

findinpath · 2023-04-21T13:18:59Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

+        public Map<String, String> getUnconsumedProperties()
+        {
+            return table.getParameters().entrySet().stream()
+                    .filter(entry -> !consumedParameters.contains(entry.getKey()))


Should we lowercase the values entered in and also the values used for checking here just to be on the safe side?

findinpath · 2023-04-21T13:25:22Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

+            return table.getParameters().get(key);
+        }
+
+        public Table getTable()


This class should likely include the logic of io.trino.plugin.hive.HiveMetadata#getSerdeProperty and should not expose getTable()

findinpath · 2023-04-21T13:32:55Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/aws/athena/PartitionProjectionService.java

@@ -90,13 +91,12 @@ public PartitionProjectionService(
        this.projectionFactories = ImmutableMap.copyOf(requireNonNull(projectionFactories, "projectionFactories is null"));
    }

-    public Map<String, Object> getPartitionProjectionTrinoTableProperties(Table table)
+    public Map<String, Object> getPartitionProjectionTrinoTableProperties(HiveMetadata.TableParameterProvider tableParameterProvider)


This can be a static method.

I'd argue that we should bring this method in HiveMetadata to have all TableParameterProvider handling in one place.

findinpath · 2023-04-21T13:33:40Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveTableProperties.java

@@ -173,7 +177,25 @@ public HiveTableProperties(
                        PARTITION_PROJECTION_LOCATION_TEMPLATE,
                        "Partition projection location template",
                        null,
-                        false));
+                        false),
+                new PropertyMetadata<>(


Add docs to hive.rst or consider creating a follow-up docs issue for this.

findinpath · 2023-04-21T13:36:23Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java

+        assertEquals(getOnlyElement(actualResult.getOnlyColumnAsSet()), expectedShowCreateTableSql);
+        assertUpdate("DROP TABLE " + tableName);
+    }
+


Is fixing #16873 something that you're considering doing @Praveen2112 ?
I'm thinking it is worth taking a stab at it just to get more knowledge of eventual corner cases which are not yet easily visible and are relevant for this feature.

Make TableNameSplitResult as a record

ee49d32

Praveen2112 requested review from findepi, kokosing, findinpath and skrzypo987 April 3, 2023 07:07

cla-bot bot added the cla-signed label Apr 3, 2023

Praveen2112 commented Apr 3, 2023

View reviewed changes

github-actions bot added hive Hive connector tests:hive labels Apr 3, 2023

Praveen2112 requested a review from huberty89 April 4, 2023 06:22

findinpath reviewed Apr 4, 2023

View reviewed changes

Praveen2112 changed the title ~~Add extra_properties to hive table properties~~ Add arbitrary_properties to hive table properties Apr 4, 2023

findinpath reviewed Apr 4, 2023

View reviewed changes

skrzypo987 reviewed Apr 4, 2023

View reviewed changes

pettyjamesm reviewed Apr 5, 2023

View reviewed changes

Praveen2112 changed the title ~~Add arbitrary_properties to hive table properties~~ Add extra_properties to hive table properties Apr 18, 2023

posulliv added 2 commits April 20, 2023 19:56

Minor cleanup in PartitionProjectionService

c38d269

- Remove unused methods - Make rewriteProperty static - Extract rewriteProperty to consume `Function<String, I>`

Add extra_properties to hive table properties

b57a6ee

Praveen2112 force-pushed the praveen/hive-extra-table-properties-2 branch from dbdb0ae to b57a6ee Compare April 20, 2023 14:27

findinpath reviewed Apr 21, 2023

View reviewed changes

Praveen2112 closed this Apr 21, 2023

findinpath mentioned this pull request Apr 26, 2023

Add extra_properties to hive table properties #17172

Merged

		if (value instanceof Map) {
		Map<?, ?> map = (Map<?, ?>) value;

	if (value instanceof Map) {
	Map<?, ?> map = (Map<?, ?>) value;
	if (value instanceof Map<?, ?> map) {

Add extra_properties to hive table properties #16846

Add extra_properties to hive table properties #16846

Conversation

Praveen2112 commented Apr 3, 2023

Description

Additional context and related issues

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findinpath Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Praveen2112 commented Apr 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findinpath Apr 4, 2023 •

edited

Loading