Add extra_properties to Delta Lake table properties #17595

jkylling · 2023-05-22T16:55:17Z

Description

Similar to #17172 this adds support for the extra_properties table property to Delta tables, such that additional table properties can be set on Delta tables.

The additional properties will not be exposed in SHOW CREATE.

It's a bit unclear how we should handle table properties which can be set by other features, and through extra_properties. Currently we take the simplest approach and allow setting properties both through Trino specific table properties, and through extra_properties. For instance, it's possible to set delta.enableChangeDataFeed both implicitly by setting change_data_feed_enabled, and explicitly by setting extra_properties = MAP(ARRAY['delta.enableChangeDataFeed'], ARRAY['true']).

Additional context and related issues

Fixes #17428

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Section
* Introduce `extra_properties` for adding arbitrary properties to Delta tables. ({issue}`17428`)

docs/src/main/sphinx/connector/delta-lake.rst

...rino-delta-lake/src/test/java/io/trino/plugin/deltalake/BaseDeltaLakeConnectorSmokeTest.java

...n/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeTableProperties.java

findinpath · 2023-08-14T10:52:07Z

...n/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeTableProperties.java

+                        .stream()
+                        .map(property -> property.getName())
+                        .collect(Collectors.toSet()),
+                DeltaLakeTableProperties.PROPERTY_NAMES);


If we'll add/remove eventually a new table property, this test will still be 🟢

What do you intend to test here?

If we add a table property, but forget to update the DeltaLakeTableProperties.PROPERTY_NAMES set, then this test will fail.

It's because the list of table properties are created when DeltaLakeTableProperties is constructed, while validateExtraProperties only uses DeltaLakeTableProperties.PROPERTY_NAMES and can be static. We could inject DeltaLakeTableProperties to DeltaLakeMetadata instead and get rid of the PROPERTY_NAMES?

findinpath · 2023-08-14T11:03:05Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeTableProperties.java

+            throw new TrinoException(INVALID_TABLE_PROPERTY, format("Extra table property key cannot be null '%s'", extraProperties));
+        }
+        Set<String> illegalExtraProperties = Sets.intersection(
+                DeltaLakeTableProperties.PROPERTY_NAMES,


Not quite sure whether extra_properties is a Trino reserved property name.

In the Hive connector we disallow using the key extra_properties in the extra_properties

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

Line 1236 in 9d95bff

.addAll(baseProperties.keySet())

Disallowing setting table properties through extra_properties and through ordinary properties reduces some possible user confusion, but also limits the use of extra_properties. For Hive this could make sense, as Trino table property names might overlap with Hive table property names with special meanings. For Delta it might not matter, as none of our current table properties have a special meaning as a Delta table property (special Delta table properties all seem to be prefixed with delta.. Perhaps we should get rid of this validation of illegal extra_property keys, and only check for the special delta. keys?

findinpath · 2023-08-14T11:04:23Z

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeTableProperties.java

+            throw new TrinoException(INVALID_TABLE_PROPERTY, format("Extra table property key cannot be null '%s'", extraProperties));
+        }
+        Set<String> illegalExtraProperties = Sets.intersection(
+                DeltaLakeTableProperties.PROPERTY_NAMES,


Maybe inline PROPERTY_NAMES map declaration (e.g. : reservedTrinoPropertyNames) here in case you won't use it anywhere else.

...rino-delta-lake/src/test/java/io/trino/plugin/deltalake/BaseDeltaLakeConnectorSmokeTest.java

findinpath · 2023-08-14T11:15:04Z

docs/src/main/sphinx/connector/delta-lake.rst

@@ -645,6 +645,10 @@ The following table properties are available for use:
      * ``NONE``

      Defaults to ``NONE``.
+  * - ``extra_properties``


From the description of the ticket:

It's a bit unclear how we should handle table properties which can be set by other features, and through extra_properties.

To avoid such situations we should disallow table property names handled by Trino.

See

trino/plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/transactionlog/MetadataEntry.java

Lines 39 to 43 in df16c38

public static final String DELTA_CHECKPOINT_WRITE_STATS_AS_JSON_PROPERTY = "delta.checkpoint.writeStatsAsJson";

public static final String DELTA_CHECKPOINT_WRITE_STATS_AS_STRUCT_PROPERTY = "delta.checkpoint.writeStatsAsStruct";

public static final String DELTA_CHANGE_DATA_FEED_ENABLED_PROPERTY = "delta.enableChangeDataFeed";

private static final String DELTA_CHECKPOINT_INTERVAL_PROPERTY = "delta.checkpointInterval";

- maybe extract them so that we can ensure the validity of what is being done in DeltaLakeTableProperties.

Collected them in MetadataEntry.RESERVED_TRINO_PROPERTY_NAMES.

There are also a few keys at

trino/plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/transactionlog/DeltaLakeSchemaSupport.java

Lines 86 to 89 in 7b2df4a

public static final String APPEND_ONLY_CONFIGURATION_KEY = "delta.appendOnly";

public static final String COLUMN_MAPPING_MODE_CONFIGURATION_KEY = "delta.columnMapping.mode";

public static final String COLUMN_MAPPING_PHYSICAL_NAME_CONFIGURATION_KEY = "delta.columnMapping.physicalName";

public static final String MAX_COLUMN_ID_CONFIGURATION_KEY = "delta.columnMapping.maxColumnId";

but it does not look like we support modifying them through Trino table properties.

It seems a bit difficult to enforce by other means than convention that reserved table properties are to be defined in MetadataEntry and added to RESERVED_TRINO_PROPERTY_NAMES.

We need to decide how to handle:

Trino table properties of Delta tables

trino/plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeTableProperties.java

Lines 48 to 53 in 7b2df4a

public static final String LOCATION_PROPERTY = "location";

public static final String PARTITIONED_BY_PROPERTY = "partitioned_by";

public static final String CHECKPOINT_INTERVAL_PROPERTY = "checkpoint_interval";

public static final String CHANGE_DATA_FEED_ENABLED_PROPERTY = "change_data_feed_enabled";

public static final String COLUMN_MAPPING_MODE_PROPERTY = "column_mapping_mode";

public static final String EXTRA_PROPERTIES = "extra_properties";

Special Delta table properties which are modified when setting Trino table properties.

Special Delta table properties which influences the way Trino reads or writes a table, but cannot be modified through Trino (e.g., delta.appendOnly).

github-actions · 2024-01-15T17:03:45Z