Add support for storing metadata to metastore in Delta Lake #21463

ebyhr · 2024-04-09T06:46:44Z

Description

This PR adds a new delta.metastore.store-table-metadata config property (true by default) and stores the last transaction number and schema string to table parameter on the metastore when the property is enabled.

Conditions we update the metastore:

Add these table parameters during CREATE (OR REPLACE) TABLE (AS) statements. This is non-async operation because we have to create or replace metastore tables either.
Repalce metastore table in getTableHandle asynchronously if the transaction number doesn't exist in metastore or the number is older than _delta_log directory
Repalce metastore table in other DDL or DML asynchronously

The cached information is used when listing table comments or columns.
Test result with 100 tables having 7 transaction logs:

SELECT * FROM system.metadata.table_comments WHERE schema_name = 'xxx';
3:09 → 1:44 (55%)

SELECT * FROM information_schema.columns WHERE table_schema = 'xxx';
3:30 → 1:42 (48%)

We could improve more once we use Glue specific API call (AWSGlue#getTables). This can be handled in follow-up.

Release notes

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

# Delta Lake
* Add support for caching table metadata to metastore. ({issue}`issuenumber`)

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

findinpath · 2024-04-11T21:01:13Z

Pls rebase to deal with code conflicts.

findinpath

Looks very promissing !!!

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

...rc/test/java/io/trino/plugin/deltalake/metastore/TestDeltaLakeMetastoreAccessOperations.java

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

...n/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/metastore/DeltaLakeMetastore.java

...rc/test/java/io/trino/plugin/deltalake/metastore/TestDeltaLakeMetastoreAccessOperations.java

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

findinpath · 2024-04-11T21:54:41Z

...n/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/metastore/DeltaLakeMetastore.java

@@ -42,4 +42,6 @@ public interface DeltaLakeMetastore
    void dropTable(SchemaTableName schemaTableName, String tableLocation, boolean deleteData);

    void renameTable(SchemaTableName from, SchemaTableName to);
+


We need compatibility tests with Delta Lake OSS where Spark writes to the table and Trino is constrained to update the metadata caching properties on any read/write operation.

ebyhr · 2024-04-12T10:06:35Z

Addressed comments partially. I will add more tests to TestDeltaLakeMetastoreAccessOperations.

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

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

...src/main/java/io/trino/plugin/deltalake/metastore/HiveMetastoreBackedDeltaLakeMetastore.java

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

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

...rc/test/java/io/trino/plugin/deltalake/metastore/TestDeltaLakeMetastoreAccessOperations.java

...src/main/java/io/trino/plugin/deltalake/metastore/HiveMetastoreBackedDeltaLakeMetastore.java

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

findinpath · 2024-04-19T13:11:07Z

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

+        }
+        tableComment.ifPresent(comment -> builder.put(TABLE_COMMENT, comment));
+        builder.put(TRINO_LAST_TRANSACTION_VERSION, Long.toString(version));
+        builder.put(TRINO_METADATA_SCHEMA_STRING, schemaString);


Have you taken into consideration base64(compress(schema))?

Good question.
Regardless of whether we compress or not, we should have a check on length.

trino/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/GlueIcebergUtil.java

Lines 90 to 92 in d60b4d0

boolean canPersistComment = (comment == null || comment.length() <= GLUE_TABLE_PARAMETER_LENGTH_LIMIT);

boolean canPersistColumnInfo = glueColumns.isPresent();

boolean canPersistMetadata = canPersistComment && canPersistColumnInfo;

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

...in/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeFileOperations.java

findinpath · 2024-04-26T15:54:50Z

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

@@ -626,6 +650,9 @@ public LocatedTableHandle getTableHandle(
            return null;
        }
        verifySupportedColumnMapping(getColumnMappingMode(metadataEntry, protocolEntry));
+        if (cacheTableMetadata && endVersion.isEmpty() && !isCachedVersionSameAsLastTransaction(metastoreTable.get(), tableSnapshot)) {


In case of referencing multiple times the same table in a query, is this check supposed to avoid calling metastore table replace repeatedly for the same table?

I don't follow where we ensure that the call is made only once.

I'm thinking that this should be ensured with the help of a ConcurrentMap like in io.trino.plugin.deltalake.DeltaLakeMetadata#getSnapshot(io.trino.spi.connector.ConnectorSession, io.trino.spi.connector.SchemaTableName, java.lang.String, java.util.Optional<java.lang.Long>)

I don't follow where we ensure that the call is made only once.

good point. i don't think we do

we should maintain a sort of a queue of pending updates
and scheduling new update logically remove previous entry

the ordering of updates doesn't matter (we can deliver them out of order, as long as we deduplicate), and we want to deduplicate, so we only need a Map<SchemaTableName, /* update info */>

Moreover, the update should be propagated to the metastore after transaction is committed.

Thus

DeltaLakeMetadata should collect stuff to update (there shouldn't be any duplicates here, as DeltaLakeMetadata should not observe two different versions of one table, unless AS OF is used0)

on commit, it should move the collected stuff to update to update manager singleton, to perform actual update

the update manager should deduplicate (updates coming from different transactions) and perform actual updates

findepi

(skimmed)

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

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

findepi · 2024-04-29T12:27:28Z

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

@@ -626,6 +650,9 @@ public LocatedTableHandle getTableHandle(
            return null;
        }
        verifySupportedColumnMapping(getColumnMappingMode(metadataEntry, protocolEntry));
+        if (cacheTableMetadata && endVersion.isEmpty() && !isCachedVersionSameAsLastTransaction(metastoreTable.get(), tableSnapshot)) {


I don't follow where we ensure that the call is made only once.

good point. i don't think we do

we should maintain a sort of a queue of pending updates
and scheduling new update logically remove previous entry

the ordering of updates doesn't matter (we can deliver them out of order, as long as we deduplicate), and we want to deduplicate, so we only need a Map<SchemaTableName, /* update info */>

Moreover, the update should be propagated to the metastore after transaction is committed.

Thus

DeltaLakeMetadata should collect stuff to update (there shouldn't be any duplicates here, as DeltaLakeMetadata should not observe two different versions of one table, unless AS OF is used0)

on commit, it should move the collected stuff to update to update manager singleton, to perform actual update

the update manager should deduplicate (updates coming from different transactions) and perform actual updates

findepi · 2024-04-29T12:34:10Z

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

+
+        TrinoFileSystem fileSystem = fileSystemFactory.create(session);
+
+        Stream<RelationCommentMetadata> tables = streamTables(session, schemaName)


This looks like efficient operation, but it's not good API at least for Glue. Getting table names is as expensive as getting all table information (because that's what's happening under the covers)
We should add something like HiveMetastore.streamTables that would return full table objects in an iterative manner. I happen to have something like that implemented, so I can share the code later. For now, we can just use listTables, without extracting new streamTables method.

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

...rc/test/java/io/trino/plugin/deltalake/metastore/TestDeltaLakeMetastoreAccessOperations.java

findepi · 2024-04-29T13:01:56Z

...rc/test/java/io/trino/plugin/deltalake/metastore/TestDeltaLakeMetastoreAccessOperations.java

+                            .add(GET_TABLE)
+                            .add(REPLACE_TABLE)
+                            .build());


And without cache, we do only GET_TABLE, right?

would it be possible to have "do cache metadata" as a boolean variable in the test
so that the expected can be expressed as a conditional expression based on "do cache metadata"? this would make the test tell the story more directly

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

ebyhr · 2024-05-08T04:31:40Z

Just rebased on master to resolve conflicts. (No change)

...ava/io/trino/plugin/deltalake/metastore/glue/v1/DeltaLakeGlueV1MetastoreTableOperations.java

ebyhr · 2024-07-23T10:51:23Z

Rebased on master to resolve logical conflicts.

ebyhr · 2024-07-26T01:32:30Z

Rebased on master to resolve conflicts.

SemionPar · 2024-07-29T15:31:51Z

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

@@ -53,6 +53,11 @@ public void commit(ConnectorTransactionHandle transaction)
    {
        MemoizedMetadata deltaLakeMetadata = transactions.remove(transaction);
        checkArgument(deltaLakeMetadata != null, "no such transaction: %s", transaction);
+        deltaLakeMetadata.optionalGet().ifPresent(metadata -> {
+            try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(getClass().getClassLoader())) {
+                metadata.commit();


unrelated question: Why is ThreadContextClassLoader required for metadata.commit()?

probably to ensure that metadata.commit() call will be executed in the context of the delta lake plugin classloader

ebyhr · 2024-07-30T02:44:23Z

@wendigo Please take another look.

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

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

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

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

...in/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeFileOperations.java

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

wendigo

LGTM % fixups

This is preparatory change for the next commit that stores the table definition to metastore.

cla-bot bot added the cla-signed label Apr 9, 2024

github-actions bot added docs delta-lake Delta Lake connector labels Apr 9, 2024

ebyhr force-pushed the ebi/delta-metastore-cache branch from 29de71c to 374a418 Compare April 9, 2024 07:34

findinpath reviewed Apr 9, 2024

View reviewed changes

docs/src/main/sphinx/connector/delta-lake.md Outdated Show resolved Hide resolved

findinpath reviewed Apr 11, 2024

View reviewed changes

ebyhr force-pushed the ebi/delta-metastore-cache branch 2 times, most recently from 8785fd6 to 0fc669a Compare April 12, 2024 09:59

findinpath reviewed Apr 12, 2024

View reviewed changes

plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/DeltaLakeQueryRunner.java Outdated Show resolved Hide resolved

findinpath reviewed Apr 12, 2024

View reviewed changes

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated Show resolved Hide resolved

findinpath reviewed Apr 12, 2024

View reviewed changes

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

ebyhr force-pushed the ebi/delta-metastore-cache branch 2 times, most recently from 3cb75c9 to 0d7fdfd Compare April 15, 2024 02:36

ebyhr marked this pull request as ready for review April 15, 2024 11:23

findinpath requested review from alexjo2144 and pajaks April 16, 2024 20:05

pajaks reviewed Apr 19, 2024

View reviewed changes

findinpath reviewed Apr 19, 2024

View reviewed changes

ebyhr force-pushed the ebi/delta-metastore-cache branch 3 times, most recently from 303cf68 to 050edfa Compare April 26, 2024 02:44

findinpath reviewed Apr 26, 2024

View reviewed changes

findepi reviewed Apr 29, 2024

View reviewed changes

ebyhr force-pushed the ebi/delta-metastore-cache branch from 050edfa to 7114f75 Compare May 8, 2024 04:31

ebyhr force-pushed the ebi/delta-metastore-cache branch 2 times, most recently from 810d795 to 57d174d Compare May 9, 2024 08:33

pajaks approved these changes May 13, 2024

View reviewed changes

wendigo reviewed Jul 23, 2024

View reviewed changes

...ava/io/trino/plugin/deltalake/metastore/glue/v1/DeltaLakeGlueV1MetastoreTableOperations.java Outdated Show resolved Hide resolved

...ava/io/trino/plugin/deltalake/metastore/glue/v1/DeltaLakeGlueV1MetastoreTableOperations.java Show resolved Hide resolved

ebyhr force-pushed the ebi/delta-metastore-cache branch from 65c0f8e to e1e35b9 Compare July 23, 2024 10:51

ebyhr force-pushed the ebi/delta-metastore-cache branch 3 times, most recently from 71390ac to 69b1e5a Compare July 25, 2024 09:11

ebyhr requested a review from wendigo July 25, 2024 09:13

ebyhr force-pushed the ebi/delta-metastore-cache branch from 69b1e5a to ecaa8e7 Compare July 26, 2024 01:32

SemionPar reviewed Jul 29, 2024

View reviewed changes

wendigo reviewed Aug 1, 2024

View reviewed changes

ebyhr force-pushed the ebi/delta-metastore-cache branch 2 times, most recently from 8295cc1 to 537164b Compare August 1, 2024 11:52

wendigo reviewed Aug 1, 2024

View reviewed changes

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated Show resolved Hide resolved

wendigo approved these changes Aug 1, 2024

View reviewed changes

ebyhr added 4 commits August 5, 2024 09:06

Extract a method to convert metastore table

1974315

Extract a helper method in MetastoreInvocations

3a8a012

Use raw metastore table in Delta getTableHandle

8895830

This is preparatory change for the next commit that stores the table definition to metastore.

Move method to GlueInputConverter

d7c22b2

ebyhr force-pushed the ebi/delta-metastore-cache branch 2 times, most recently from 3afd97a to e010a3d Compare August 5, 2024 01:22

ebyhr added 2 commits August 5, 2024 11:27

Add support for storing metadata to metastore in Delta Lake

c39e539

Use table definition of metastore in Delta Lake

33f8454

ebyhr force-pushed the ebi/delta-metastore-cache branch from e010a3d to 33f8454 Compare August 5, 2024 02:35

ebyhr merged commit 27f86ca into trinodb:master Aug 5, 2024
15 of 37 checks passed

ebyhr deleted the ebi/delta-metastore-cache branch August 5, 2024 02:41

github-actions bot added this to the 454 milestone Aug 5, 2024

findinpath mentioned this pull request Aug 5, 2024

Store all table metadata to the metastore in the Delta lake connector #17519

Closed

colebow mentioned this pull request Aug 7, 2024

Add Trino 454 release notes #22977

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for storing metadata to metastore in Delta Lake #21463

Add support for storing metadata to metastore in Delta Lake #21463

ebyhr commented Apr 9, 2024 •

edited

Loading

findinpath commented Apr 11, 2024

findinpath left a comment

findinpath Apr 11, 2024

ebyhr commented Apr 12, 2024

findinpath Apr 19, 2024

findepi Apr 29, 2024

findinpath Apr 26, 2024

findepi Apr 29, 2024

findepi left a comment

findepi Apr 29, 2024

findepi Apr 29, 2024

findepi Apr 29, 2024

ebyhr commented May 8, 2024 •

edited

Loading

ebyhr commented Jul 23, 2024

ebyhr commented Jul 26, 2024

SemionPar Jul 29, 2024

wendigo Aug 1, 2024

ebyhr commented Jul 30, 2024

wendigo left a comment

		@@ -42,4 +42,6 @@ public interface DeltaLakeMetastore
		void dropTable(SchemaTableName schemaTableName, String tableLocation, boolean deleteData);

		void renameTable(SchemaTableName from, SchemaTableName to);

	boolean canPersistComment = (comment == null \|\| comment.length() <= GLUE_TABLE_PARAMETER_LENGTH_LIMIT);
	boolean canPersistColumnInfo = glueColumns.isPresent();
	boolean canPersistMetadata = canPersistComment && canPersistColumnInfo;


		TrinoFileSystem fileSystem = fileSystemFactory.create(session);

		Stream<RelationCommentMetadata> tables = streamTables(session, schemaName)

Add support for storing metadata to metastore in Delta Lake #21463

Add support for storing metadata to metastore in Delta Lake #21463

Conversation

ebyhr commented Apr 9, 2024 • edited Loading

Description

Release notes

findinpath commented Apr 11, 2024

findinpath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Apr 12, 2024

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

findepi left a 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

ebyhr commented May 8, 2024 • edited Loading

ebyhr commented Jul 23, 2024

ebyhr commented Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Jul 30, 2024

wendigo left a comment

Choose a reason for hiding this comment

ebyhr commented Apr 9, 2024 •

edited

Loading

ebyhr commented May 8, 2024 •

edited

Loading