Support creating tables with column comment in Delta Lake by ebyhr · Pull Request #12455 · trinodb/trino

ebyhr · 2022-05-18T10:00:20Z

Description

Support creating tables with column comment in Delta Lake

Documentation

( ) 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 column comments during table creation. ({issue}`12455`)

findepi · 2022-05-18T11:27:58Z

in Delta Lake & Kudu

Please split this into two PRs.

ebyhr · 2022-05-19T02:53:38Z

CI hit #12471

ebyhr · 2022-05-24T01:07:08Z

Verified Spark compatibility locally.

trino> CREATE TABLE delta.default.test_comment (c1 int comment 'foo') WITH (location = 's3://presto-ci-test/test_comment');
CREATE TABLE

spark-sql> DESC default.test_comment;
c1	int	foo

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

findinpath · 2022-05-24T14:29:33Z

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

+    private final Optional<String> comment;
+
+    @Deprecated
+    public DeltaLakeColumnHandle(


Can you please remove the usage of this constructor from the current codebase of trino-delta-lake module? I still found 2 usages of it.

Actually, there're a lot of usages more than 2. That is why I marked @Deprecated instead of migrating them. I can migrate all if it's not burden for reviewers.

DLCH is used as key in maps, comment is part of its equality.
We unfortunately need to update all usages appropriately, otherwise we cannot reason about the correctness of the codebase.

I would very much prefer not adding comment here, to the table handle.
It bite us a few times in Iceberg, so i would prefer to see if we can transport the comment to ColumnMetadata somehow else.

plugin/trino-accumulo/src/test/java/io/trino/plugin/accumulo/TestAccumuloConnectorTest.java

...roduct-tests/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeOssCreateTable.java

findepi · 2022-05-26T13:30:36Z

@ebyhr please squash and ping me for a review.

ebyhr · 2022-05-27T09:07:12Z

@findepi Squashed commits.

findepi · 2022-05-27T09:53:17Z

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

+    private final Optional<String> comment;
+
+    @Deprecated
+    public DeltaLakeColumnHandle(


DLCH is used as key in maps, comment is part of its equality.
We unfortunately need to update all usages appropriately, otherwise we cannot reason about the correctness of the codebase.

I would very much prefer not adding comment here, to the table handle.
It bite us a few times in Iceberg, so i would prefer to see if we can transport the comment to ColumnMetadata somehow else.

findepi · 2022-05-27T09:54:39Z

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

+
+    private static Optional<String> getComment(JsonNode node)
+    {
+        return Optional.ofNullable(node.get("metadata"))


Why node.get("metadata") is nullable?

findepi · 2022-05-27T09:56:06Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

        assertUpdate("DROP TABLE " + tableName);
    }

+    @Test


"Deny creating tables with column comment if unsupported" looks solid & only slightly related to the Delta.
Let's separate PR for this.

ebyhr · 2022-05-31T07:06:07Z

@findepi Updated not to touch DeltaLakeColumnHandle.

findepi · 2022-05-31T10:03:59Z

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

        String location = metastore.getTableLocation(tableHandle.getSchemaTableName(), session);
        List<ColumnMetadata> columns = getColumns(tableHandle.getMetadataEntry()).stream()
-                .map(DeltaLakeMetadata::getColumnMetadata)
+                .map(column -> getColumnMetadata(column, getColumnComments(tableHandle.getMetadataEntry())))


getColumnComments(tableHandle.getMetadataEntry()) is performed once for every column, should be only once

findepi · 2022-05-31T10:04:24Z

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

                return metastore.getMetadata(metastore.getSnapshot(table, session), session).stream().map(metadata -> {
                    List<ColumnMetadata> columnMetadata = getColumns(metadata).stream()
-                            .map(DeltaLakeMetadata::getColumnMetadata)
+                            .map(column -> getColumnMetadata(column, getColumnComments(metadata)))


getColumnComments(tableHandle.getMetadataEntry()) is performed once for every column, should be only once

findepi · 2022-05-31T10:06:41Z

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

                .build();
    }

+    public static Map<String, Optional<String>> getColumnComments(MetadataEntry metadataEntry)


Is there a difference between missing entry and entry being Optional.empty()?

maybe we just have a map of non-null comments, passed around as Map<String,String> ?

we would lose ability to validate map contains entries for the columns, but you don't do this anyway (by means of columnComments.getOrDefault...)

findepi · 2022-05-31T10:07:00Z

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

+                .orElseThrow(() -> new IllegalStateException("Serialized schema not found in transaction log for " + metadataEntry.getName()));
+    }
+
+    private static Map<String, Optional<String>> getColumnComment(String json)


getColumnComment -> getColumnComments (plural)

findepi · 2022-05-31T10:07:37Z

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

+        }
+    }
+
+    private static Map.Entry<String, Optional<String>> columnComment(JsonNode node)


ebyhr · 2022-06-01T05:00:59Z

@findepi Addressed comments.

alexjo2144 · 2022-06-01T14:39:55Z

...n/java/io/trino/tests/product/deltalake/TestDeltaLakeDatabricksCreateTableCompatibility.java

    }
+
+    @Test(groups = {DELTA_LAKE_DATABRICKS, PROFILE_SPECIFIC_TESTS})
+    public void testCreateTableWithColumnComment()


Do we have a test that goes the other way, creating the comment in Delta and reading from Trino?

alexjo2144 · 2022-06-01T14:43:52Z

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

                    handle.getMetadataEntry().getId(),
                    columnsBuilder.build(),
                    partitionColumns,
+                    ImmutableMap.of(),


Can you add a test that creates a table with comments and then adds a column? Make sure this doesn't wipe the existing comments

alexjo2144 · 2022-06-01T14:44:53Z

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

                    randomUUID().toString(),
                    handle.getInputColumns(),
                    handle.getPartitionedBy(),
+                    ImmutableMap.of(),


Should this match what we do in createTable?

I think it's not needed as column comment is unsupported in CTAS syntax level.

Yep, you're totally right. Thanks

alexjo2144

Do you want to support alter table add column with a comment here, or create a separate PR?

ebyhr · 2022-06-02T14:22:25Z

I want to separate PR for ADD COLUMN with a comment.

ebyhr · 2022-06-07T01:23:52Z

Rebased on upstream to resolve conflicts.

cla-bot bot added the cla-signed label May 18, 2022

ebyhr changed the title ~~Support creating tables with column comment in Delta Lake & Kudu~~ Support creating tables with column comment in Delta Lake May 18, 2022

ebyhr force-pushed the ebi/delta-column-comment branch 2 times, most recently from 12c206c to 495369a Compare May 19, 2022 01:20

ebyhr marked this pull request as ready for review May 19, 2022 02:54

ebyhr force-pushed the ebi/delta-column-comment branch from 495369a to 7847baa Compare May 24, 2022 00:54

ebyhr requested review from findepi, findinpath and hashhar May 24, 2022 01:07

findinpath reviewed May 24, 2022

View reviewed changes

ebyhr force-pushed the ebi/delta-column-comment branch from 7847baa to 18df43c Compare May 25, 2022 01:54

github-actions bot added the tests:hive label May 25, 2022

ebyhr commented May 25, 2022

View reviewed changes

...roduct-tests/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeOssCreateTable.java Outdated Show resolved Hide resolved

ebyhr force-pushed the ebi/delta-column-comment branch from 18df43c to 0072698 Compare May 26, 2022 05:08

ebyhr force-pushed the ebi/delta-column-comment branch from ee31f4e to 417d9cd Compare May 27, 2022 05:43

findepi reviewed May 27, 2022

View reviewed changes

ebyhr force-pushed the ebi/delta-column-comment branch 2 times, most recently from 7d63473 to 183d16e Compare May 31, 2022 05:26

findepi reviewed May 31, 2022

View reviewed changes

ebyhr force-pushed the ebi/delta-column-comment branch 2 times, most recently from a11f6d9 to dcf4f0d Compare June 1, 2022 03:05

ebyhr mentioned this pull request Jun 1, 2022

Support table and column comments in Delta Lake #12424

Closed

3 tasks

alexjo2144 reviewed Jun 1, 2022

View reviewed changes

ebyhr force-pushed the ebi/delta-column-comment branch from dcf4f0d to 04d0d3f Compare June 2, 2022 01:32

alexjo2144 approved these changes Jun 2, 2022

View reviewed changes

ebyhr requested a review from findepi June 3, 2022 00:44

findepi approved these changes Jun 6, 2022

View reviewed changes

Support creating table with column comment in Delta Lake

5442cfa

ebyhr force-pushed the ebi/delta-column-comment branch from 04d0d3f to 5442cfa Compare June 7, 2022 01:23

ebyhr merged commit fff9862 into master Jun 7, 2022

ebyhr deleted the ebi/delta-column-comment branch June 7, 2022 03:34

ebyhr mentioned this pull request Jun 7, 2022

Release notes for 385 #12686

Closed

github-actions bot added this to the 385 milestone Jun 7, 2022

colebow mentioned this pull request Jun 7, 2022

Add Trino 385 release notes #12703

Closed

Conversation

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

Uh oh!

Description

Documentation

Release notes

Uh oh!

findepi commented May 18, 2022

Uh oh!

ebyhr commented May 19, 2022

Uh oh!

ebyhr commented May 24, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebyhr May 25, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

findepi commented May 26, 2022

Uh oh!

ebyhr commented May 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebyhr commented May 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebyhr commented Jun 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebyhr Jun 2, 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!

Choose a reason for hiding this comment

Uh oh!

alexjo2144 left a comment

Choose a reason for hiding this comment

Uh oh!

ebyhr commented Jun 2, 2022

Uh oh!

ebyhr commented Jun 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

ebyhr commented May 18, 2022 •

edited

Loading

ebyhr May 25, 2022 •

edited

Loading

ebyhr Jun 2, 2022 •

edited

Loading