Add support for table rename for delta lake with glue #13707

homar · 2022-08-17T09:09:13Z

Description

Is this change a fix, improvement, new feature, refactoring, or other?

new feature

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

a connector

How would you describe this change to a non-technical end user or system administrator?

ALTER TABLE RENAME TO can be used with glue as a metastore for delta lake

Related issues, pull requests, and links

Fixes: #12985

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Delta Lake
* Add support for `ALTER TABLE ... RENAME TO` statement on Glue metastore. ({issue}`12985`)

alexjo2144

Just to be clear, this will not move any data files?

alexjo2144 · 2022-08-23T16:11:46Z

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

@@ -288,6 +289,8 @@
    private final boolean deleteSchemaLocationsFallback;
    private final boolean useUniqueTableLocation;

+    private final MetastoreTypeConfig metastoreTypeConfig;


Store the type itself, not the config

Suggested change

private final MetastoreTypeConfig metastoreTypeConfig;

private final String metastoreType;

alexjo2144 · 2022-08-23T16:12:34Z

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

@@ -1992,7 +1997,7 @@ public void renameTable(ConnectorSession session, ConnectorTableHandle tableHand
        DeltaLakeTableHandle handle = (DeltaLakeTableHandle) tableHandle;
        Table table = metastore.getTable(handle.getSchemaName(), handle.getTableName())
                .orElseThrow(() -> new TableNotFoundException(handle.getSchemaTableName()));
-        if (table.getTableType().equals(MANAGED_TABLE.name())) {
+        if (table.getTableType().equals(MANAGED_TABLE.name()) && !metastoreTypeConfig.getMetastoreType().equals("glue")) {


equalsIgnoreCase

homar · 2022-08-24T12:11:43Z

This failing suite is not related. It has been failing before on master https://github.com/trinodb/trino/runs/7976696676?check_suite_focus=true

#13825

homar · 2022-08-24T14:30:27Z

Just to be clear, this will not move any data files?

It should not

ebyhr

Can we update io.trino.plugin.hive.metastore.glue.TestHiveGlueMetastore#testRenameTable?

plugin/trino-delta-lake/pom.xml

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

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

...lta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeRenameToWithGlueMetastore.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java

homar · 2022-08-25T09:54:24Z

testRenameTable

Sorry I didn't notice this method

homar · 2022-08-25T14:01:20Z

failure doesn't seem to be related

alexjo2144 · 2022-08-25T15:13:07Z

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

@@ -1996,7 +1999,8 @@ public void renameTable(ConnectorSession session, ConnectorTableHandle tableHand
        DeltaLakeTableHandle handle = (DeltaLakeTableHandle) tableHandle;
        Table table = metastore.getTable(handle.getSchemaName(), handle.getTableName())
                .orElseThrow(() -> new TableNotFoundException(handle.getSchemaTableName()));
-        if (table.getTableType().equals(MANAGED_TABLE.name())) {
+        if (table.getTableType().equals(MANAGED_TABLE.name()) &&
+                !(metastoreType.equalsIgnoreCase("glue") || metastoreType.equalsIgnoreCase("file"))) {


Maybe include the metastore type in the exception msg

alexjo2144 · 2022-08-25T15:15:29Z

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

@@ -98,6 +102,7 @@ public DeltaLakeMetadataFactory(
        this.perTransactionMetastoreCacheMaximumSize = deltaLakeConfig.getPerTransactionMetastoreCacheMaximumSize();
        this.deleteSchemaLocationsFallback = deltaLakeConfig.isDeleteSchemaLocationsFallback();
        this.useUniqueTableLocation = deltaLakeConfig.isUniqueTableLocation();
+        this.metastoreTypeConfig = requireNonNull(metastoreTypeConfig, "metastoreTypeConfig is null");


Reason we do this is that config classes are mutable and the metastore type cannot be changed dynamically. Realistically, this is not usually a problem but better to do it this way.

Suggested change

this.metastoreTypeConfig = requireNonNull(metastoreTypeConfig, "metastoreTypeConfig is null");

requireNonNull(metastoreTypeConfig, "metastoreTypeConfig is null");

this.metastoreType = metastoreTypeConfig.getMetastoreType();

alexjo2144 · 2022-08-25T15:17:53Z

...st/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaLakeRenameToWithGlueMetastore.java

+public class TestDeltaLakeRenameToWithGlueMetastore
+        extends AbstractTestQueryFramework
+{
+    protected static final String SCHEMA = "test_tables_with_custom_location" + randomTableSuffix();


Suggested change

protected static final String SCHEMA = "test_tables_with_custom_location" + randomTableSuffix();

protected static final String SCHEMA = "test_delta_lake_rename_to_with_glue_" + randomTableSuffix();

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java

homar · 2022-08-28T09:19:44Z

@ebyhr I think this is a new test that is failing, it should not be related to my change I think you added it recently could you please take a look ?

ebyhr · 2022-08-28T23:54:58Z

Looking the failure. I will send a PR today.

homar · 2022-08-30T07:26:47Z

@ebyhr can we merge this or do i need more changes?

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

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java

plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/glue/TestHiveGlueMetastore.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java

homar · 2022-08-30T11:39:51Z

Don't we need to copy partition indexes as well? It disappeared as far as I tested.

We never set them anywhere, for Iceberg we don't copy them either, what is more I don't see a way to obtain them. com.amazonaws.services.glue.model.Table doesn't contain them

ebyhr · 2022-08-31T03:29:32Z

We never set them anywhere, for Iceberg we don't copy them either

Even if our connectors don't set partition indexes, users can directly set it in Glue and such index might be used when finding partitions in Hive tables.

what is more I don't see a way to obtain them. com.amazonaws.services.glue.model.Table doesn't contain them

We need to call AWSGlue#getPartitionIndexes to retrieve the information.

Anyway, you added a logic to deny Glue metastore in HiveMetadata.renameTable now. Let me take another look.

homar · 2022-08-31T08:00:27Z

We never set them anywhere, for Iceberg we don't copy them either

Even if our connectors don't set partition indexes, users can directly set it in Glue and such index might be used when finding partitions in Hive tables.

what is more I don't see a way to obtain them. com.amazonaws.services.glue.model.Table doesn't contain them

We need to call AWSGlue#getPartitionIndexes to retrieve the information.

Oh that makes sense, I didn't know that, thanks.

Anyway, you added a logic to deny Glue metastore in HiveMetadata.renameTable now. Let me take another look.

Yes because it was not my intention to turn it on for Hive. I think that it would be hard as for hive it would make sense to actually move data to new location and for DeltaLAke and Iceberg we don't move that so it would be a totally different story

...st/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaLakeRenameToWithGlueMetastore.java

ebyhr · 2022-09-01T02:59:11Z

Merged, thanks!

cla-bot bot added the cla-signed label Aug 17, 2022

github-actions bot added the tests:hive label Aug 17, 2022

ebyhr mentioned this pull request Aug 17, 2022

Test: Add support for table rename for delta lake with glue #13708

Closed

homar force-pushed the homar/delta_lake_with_glue_rename_table_support branch 4 times, most recently from 42928ae to cdd7429 Compare August 23, 2022 10:14

findepi requested review from ebyhr and alexjo2144 August 23, 2022 10:58

alexjo2144 reviewed Aug 23, 2022

View reviewed changes

homar force-pushed the homar/delta_lake_with_glue_rename_table_support branch from cdd7429 to 8c4467b Compare August 23, 2022 23:40

homar marked this pull request as ready for review August 24, 2022 12:11

homar changed the title ~~[WIP] Add support for table rename for delta lake with glue~~ Add support for table rename for delta lake with glue Aug 24, 2022

homar requested a review from alexjo2144 August 24, 2022 14:30

ebyhr reviewed Aug 25, 2022

View reviewed changes

homar force-pushed the homar/delta_lake_with_glue_rename_table_support branch 2 times, most recently from e0e52eb to 4782b6c Compare August 25, 2022 11:01

alexjo2144 reviewed Aug 25, 2022

View reviewed changes

homar force-pushed the homar/delta_lake_with_glue_rename_table_support branch 3 times, most recently from 1f9c62e to a93d827 Compare August 27, 2022 21:31

homar force-pushed the homar/delta_lake_with_glue_rename_table_support branch 3 times, most recently from 4c85271 to 6f47296 Compare August 29, 2022 15:59

ebyhr reviewed Aug 30, 2022

View reviewed changes

homar force-pushed the homar/delta_lake_with_glue_rename_table_support branch 4 times, most recently from bc2e89d to c92f31f Compare August 30, 2022 21:08

ebyhr approved these changes Aug 31, 2022

View reviewed changes

Add RENAME TABLE TO for delta lake external tables with glue

df86c1a

homar force-pushed the homar/delta_lake_with_glue_rename_table_support branch from c92f31f to 30423e8 Compare August 31, 2022 13:58

homar added 2 commits September 1, 2022 07:39

Add RENAME TABLE TO for delta lake managed tables with glue

e9ffcd6

Allow RENAME TABLE TO for FileHiveMetastore

43b174a

ebyhr force-pushed the homar/delta_lake_with_glue_rename_table_support branch from 30423e8 to 43b174a Compare August 31, 2022 22:42

ebyhr merged commit cb9d67d into trinodb:master Sep 1, 2022

ebyhr mentioned this pull request Sep 1, 2022

Release notes for 395 #13913

Closed

github-actions bot added this to the 395 milestone Sep 1, 2022

colebow mentioned this pull request Sep 2, 2022

Add Trino 395 release notes #13975

Merged

findepi mentioned this pull request Sep 8, 2022

Unbind MetastoreTypeConfig in Hive tests #14059

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for table rename for delta lake with glue #13707

Add support for table rename for delta lake with glue #13707

homar commented Aug 17, 2022 •

edited by ebyhr

Loading

alexjo2144 left a comment

alexjo2144 Aug 23, 2022

alexjo2144 Aug 23, 2022

homar commented Aug 24, 2022 •

edited

Loading

homar commented Aug 24, 2022

ebyhr left a comment

homar commented Aug 25, 2022

homar commented Aug 25, 2022

alexjo2144 Aug 25, 2022

alexjo2144 Aug 25, 2022

alexjo2144 Aug 25, 2022

homar commented Aug 28, 2022

ebyhr commented Aug 28, 2022

homar commented Aug 30, 2022

homar commented Aug 30, 2022

ebyhr commented Aug 31, 2022

homar commented Aug 31, 2022

ebyhr commented Sep 1, 2022

	private final MetastoreTypeConfig metastoreTypeConfig;
	private final String metastoreType;

	this.metastoreTypeConfig = requireNonNull(metastoreTypeConfig, "metastoreTypeConfig is null");
	requireNonNull(metastoreTypeConfig, "metastoreTypeConfig is null");
	this.metastoreType = metastoreTypeConfig.getMetastoreType();

	protected static final String SCHEMA = "test_tables_with_custom_location" + randomTableSuffix();
	protected static final String SCHEMA = "test_delta_lake_rename_to_with_glue_" + randomTableSuffix();

Add support for table rename for delta lake with glue #13707

Add support for table rename for delta lake with glue #13707

Conversation

homar commented Aug 17, 2022 • edited by ebyhr Loading

Description

Related issues, pull requests, and links

Documentation

Release notes

alexjo2144 left a comment

Choose a reason for hiding this comment

alexjo2144 Aug 23, 2022

Choose a reason for hiding this comment

alexjo2144 Aug 23, 2022

Choose a reason for hiding this comment

homar commented Aug 24, 2022 • edited Loading

homar commented Aug 24, 2022

ebyhr left a comment

Choose a reason for hiding this comment

homar commented Aug 25, 2022

homar commented Aug 25, 2022

alexjo2144 Aug 25, 2022

Choose a reason for hiding this comment

alexjo2144 Aug 25, 2022

Choose a reason for hiding this comment

alexjo2144 Aug 25, 2022

Choose a reason for hiding this comment

homar commented Aug 28, 2022

ebyhr commented Aug 28, 2022

homar commented Aug 30, 2022

homar commented Aug 30, 2022

ebyhr commented Aug 31, 2022

homar commented Aug 31, 2022

ebyhr commented Sep 1, 2022

homar commented Aug 17, 2022 •

edited by ebyhr

Loading

homar commented Aug 24, 2022 •

edited

Loading