-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Support creating tables with table comment in Delta Lake #12452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5ede924
41a1361
e127800
126232f
71dcb66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,8 +87,8 @@ public void testReadMetadataEntry() | |
| .isEqualTo( | ||
| new MetadataEntry( | ||
| "b6aeffad-da73-4dde-b68e-937e468b1fde", | ||
| "", | ||
| "", | ||
| null, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is "name" a table name? shouldn't it be present?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| null, | ||
| new MetadataEntry.Format("parquet", Map.of()), | ||
| "{\"type\":\"struct\",\"fields\":[" + | ||
| "{\"name\":\"name\",\"type\":\"string\",\"nullable\":true,\"metadata\":{}}," + | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 themThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not new support. It returned an empty character before this change.
TestCheckpointEntryIteratorcovers both non-empty and null cases.Added
checkArgumentto those three methods.