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

ebyhr · 2022-05-18T07:51:37Z

Description

Support creating tables with table comment in Delta Lake. Verified the Delta Lake's compatibility with Spark locally.

Documentation

(x) No documentation is needed.

Release notes

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

# Delta Lake
* Support storing a table comment when creating new tables. ({issue}`12452`)

findepi · 2022-05-18T11:29:44Z

Support creating tables with table comment in Delta Lake & Kudu. Verified the Delta Lake's compatibility with Spark locally.

Remove Kudu from the description ?

ebyhr · 2022-05-19T02:44:25Z

CI hit #12413

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

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

ebyhr · 2022-05-24T00:50:56Z

Rebased on upstream to resolve conflicts.

ebyhr · 2022-05-25T05:25:11Z

Added a product test. Ready for review now.

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

ebyhr · 2022-05-26T04:18:37Z

CI hit #12300

findepi · 2022-05-26T13:33:20Z

...c/main/java/io/trino/plugin/deltalake/transactionlog/checkpoint/CheckpointEntryIterator.java

    {
+        if (block.isNull(position)) {
+            return null;
+        }


Does it add support for reading checkpoint files we didn't read before?
Should we have a test?

Also, should we have this for getLong, getInt, getByte methods?
If there are supposed to be called on non-null values only, we should probably
have checkArgument(block.isNull(position))` in them

This is not new support. It returned an empty character before this change.
TestCheckpointEntryIterator covers both non-empty and null cases.

Added checkArgument to those three methods.

findepi · 2022-05-26T13:34:01Z

...st/java/io/trino/plugin/deltalake/transactionlog/checkpoint/TestCheckpointEntryIterator.java

                                "b6aeffad-da73-4dde-b68e-937e468b1fde",
-                                "",
-                                "",
+                                null,


is "name" a table name? shouldn't it be present?

It's optional "User-provided identifier for this table" according to https://github.com/delta-io/delta/blob/master/PROTOCOL.md#change-metadata.

I can send another PR if you agree on setting the field at https://github.com/trinodb/trino/blob/master/plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java#L1004

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

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

+    @JsonProperty
+    public Optional<String> getComment()
+    {
+        return metadataEntry.map(MetadataEntry::getDescription);


i think metadata should be present:

Optional.ofNullable(getMetadataEntry().getDescription())

remove it from table handle class, let the caller do this

findepi · 2022-05-26T13:42:49Z

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

+            assertEquals(getTableCommentOnDelta("default", tableName), "test comment");
+        }
+        finally {
+            onDelta().executeQuery("DROP TABLE default." + tableName);


table was created on trino, so drop on trino as well

findepi · 2022-05-26T13:43:14Z

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

+        }
+    }
+
    private void testInsert(String tableName, List<QueryAssert.Row> existingRows)


this method is a helper method for the test above; put your new test below, so that the test and its helper remain grouped

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

ebyhr force-pushed the ebi/delta-table-comment branch from 376f02b to b15fb16 Compare May 18, 2022 10:29

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

ebyhr force-pushed the ebi/delta-table-comment branch 2 times, most recently from 0c3745b to 3876d2d Compare May 19, 2022 01:40

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

ebyhr requested review from findepi, findinpath and hashhar May 20, 2022 06:00

findinpath reviewed May 20, 2022

View reviewed changes

...lta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeOssDeltaLakeConnectorTest.java Outdated Show resolved Hide resolved

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

ebyhr force-pushed the ebi/delta-table-comment branch 2 times, most recently from 3e946a3 to f3b6a8e Compare May 23, 2022 00:39

ebyhr requested a review from findinpath May 23, 2022 02:34

ebyhr force-pushed the ebi/delta-table-comment branch from f3b6a8e to c8c05be Compare May 24, 2022 00:50

ebyhr force-pushed the ebi/delta-table-comment branch from c8c05be to 4dbbff8 Compare May 25, 2022 02:53

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

findinpath approved these changes 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-table-comment branch from 4dbbff8 to 554fac3 Compare May 26, 2022 02:18

findepi approved these changes May 26, 2022

View reviewed changes

ebyhr added 5 commits May 27, 2022 09:57

Remove unused config property

5ede924

Fix string null entry in CheckpointEntryIterator

41a1361

Check nullability in CheckpointEntryIterator

e127800

Support creating tables with table comment in Delta Lake

126232f

Deny creating tables with table comment if unsupported

71dcb66

ebyhr force-pushed the ebi/delta-table-comment branch from 554fac3 to 71dcb66 Compare May 27, 2022 01:25

ebyhr merged commit bb0d814 into master May 27, 2022

ebyhr deleted the ebi/delta-table-comment branch May 27, 2022 05:31

ebyhr mentioned this pull request May 27, 2022

Release notes for 383 #12556

Closed

github-actions bot added this to the 383 milestone May 27, 2022

colebow mentioned this pull request May 27, 2022

Add Trino 383 release notes #12565

Merged

This was referenced May 28, 2022

Support table and column comments in Delta Lake #12424

Closed

Deny CREATE TABLE with comment if the connector doesn't support storing comments #11348

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!

Uh oh!

Uh oh!

ebyhr commented May 24, 2022

Uh oh!

ebyhr commented May 25, 2022

Uh oh!

Uh oh!

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

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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

ebyhr commented May 18, 2022 •

edited

Loading