-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add support for editing column comments on Iceberg tables #11143
Conversation
@Override | ||
public void updateColumnComment(ConnectorSession session, SchemaTableName schemaTableName, ColumnIdentity columnIdentity, Optional<String> comment) | ||
{ | ||
metastore.commentColumn(schemaTableName.getSchemaName(), schemaTableName.getTableName(), columnIdentity.getName(), comment); |
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.
Do we need to update the comment both in the metastore and via the Iceberg updateColumnDoc
?
If we don't need the metastore's comment set we can move this code up to IcebergMetadata
and make it catalog agnostic.
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.
I inspired myself in the implementation from the updateTableComment
method which does also talk to the metastore.
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.
I think we should remove both, as it leads to inconsistent state if the icebergTable.updateSchema().updateColumnDoc
fails.
let's reconsider with a followup
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java
Show resolved
Hide resolved
1d84209
to
37de02a
Compare
@Override | ||
public void updateColumnComment(ConnectorSession session, SchemaTableName schemaTableName, ColumnIdentity columnIdentity, Optional<String> comment) | ||
{ | ||
metastore.commentColumn(schemaTableName.getSchemaName(), schemaTableName.getTableName(), columnIdentity.getName(), comment); |
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.
I think we should remove both, as it leads to inconsistent state if the icebergTable.updateSchema().updateColumnDoc
fails.
let's reconsider with a followup
@@ -146,7 +146,6 @@ protected QueryRunner createQueryRunner() | |||
protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) | |||
{ | |||
switch (connectorBehavior) { | |||
case SUPPORTS_COMMENT_ON_COLUMN: |
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.
As a follow-up can you please add tests for CREATE TABLE with column comment?
see
trino/testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java
Lines 410 to 413 in dbc25ac
// TODO: comment set when creating a table | |
// assertUpdate("CREATE TABLE " + tableName + "(a integer COMMENT 'new column comment')"); | |
// assertThat(getColumnComment(tableName, "a")).isEqualTo("new column comment"); | |
// assertUpdate("DROP TABLE " + tableName); |
Created #11161 follow-up issue for ditching metastore updates for COMMENT on Iceberg table/column. |
Do we need to document this in the Iceberg connector SQL support section or somewhere? If not .. I dont see any other docs for this. |
@mosabua there is a link towards Schema and table management document within the https://trino.io/docs/current/connector/iceberg.html#sql-support section. I think that in this case (the current change just added a minor missing functionality for the Iceberg connector) there is no need for extra documentation. If there is a need to enrich the docs I can gladly take care of it. |
Thanks for checking that out. I agree @findinpath .. we are good ;-) |
Description
Enable the Trino users to execute commands like the following:
Relevant connector test:
io.trino.testing.AbstractTestDistributedQueries#testCommentColumn
This change is a new feature.
This change affect only the Iceberg connector.
This PR adds the ability to edit comments for Iceberg table columns.
Related issues, pull requests, and links
PRs depending on this change: #11072
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: