Skip to content
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

Jump to bottom

Show column comment in JDBC connector #1840

Merged
merged 1 commit into from Nov 19, 2019
Merged

Show column comment in JDBC connector #1840

merged 1 commit into from Nov 19, 2019

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Oct 23, 2019 •
edited

Fixes #1837

@cla-bot cla-bot bot added the cla-signed label Oct 23, 2019
kokosing
kokosing reviewed Oct 28, 2019
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just two comments

presto-mysql/src/test/java/io/prestosql/plugin/mysql/TestMySqlIntegrationSmokeTest.java Outdated Show resolved Hide resolved
...tgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlIntegrationSmokeTest.java Show resolved Hide resolved
@ebyhr ebyhr force-pushed the jdbc-column-comment branch 2 times, most recently from 0fe37cc to 8c0d92d Compare October 29, 2019 14:34
@ebyhr ebyhr requested a review from findepi October 29, 2019 14:36
findepi
findepi reviewed Oct 30, 2019
View reviewed changes
presto-base-jdbc/src/test/java/io/prestosql/plugin/jdbc/TestJdbcClient.java Outdated Show resolved Hide resolved
...tgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlIntegrationSmokeTest.java Outdated Show resolved Hide resolved
...tgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlIntegrationSmokeTest.java Outdated

assertQuery(
"SELECT column_name, comment FROM information_schema.columns WHERE table_schema = 'tpch' AND table_name = 'test_column_comment'",
"VALUES ('col1', 'test comment'), ('col2', null), ('col3', null)");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why comment for col2 is null and not ''?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In PostgreSQL, COMMENT ON object IS '' is the same as COMMENT ON object IS NULL (remove the existing comment). I couldn't find the document about the empty character, but even pg_catalog.pg_description table doesn't hold the empty comment. It means this is not driver's issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, so i suggest: #1840 (comment)

presto-mysql/src/main/java/io/prestosql/plugin/mysql/MySqlClient.java Outdated
@@ -155,6 +160,43 @@ protected String getTableSchemaName(ResultSet resultSet)
return resultSet.getString("TABLE_CAT");
}

@Override
public List<JdbcColumnHandle> getColumns(ConnectorSession session, JdbcTableHandle tableHandle)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't copy.
Just add

// Note: some databases (e.g. SQL Server) do not return column remarks/comment here.
Optional<String> comment = Optional.ofNullable(emptyToNull(resultSet.getString("REMARKS")));

in BaseJdbcClient's method

presto-mysql/src/test/java/io/prestosql/plugin/mysql/TestMySqlIntegrationSmokeTest.java Show resolved Hide resolved
findepi
findepi reviewed Oct 30, 2019
View reviewed changes
...tgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlIntegrationSmokeTest.java Outdated
{
execute("CREATE TABLE tpch.test_column_comment (col1 bigint, col2 bigint, col3 bigint)");
execute("COMMENT ON COLUMN tpch.test_column_comment.col1 IS 'test comment'");
execute("COMMENT ON COLUMN tpch.test_column_comment.col2 IS ''");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
execute("COMMENT ON COLUMN tpch.test_column_comment.col2 IS ''");
execute("COMMENT ON COLUMN tpch.test_column_comment.col2 IS ''"); // it will be NULL, PostgreSQL doesn't store empty comment

@ebyhr ebyhr requested a review from findepi November 13, 2019 13:09
@ebyhr ebyhr merged commit 744c74c into trinodb:master Nov 19, 2019
@ebyhr ebyhr mentioned this pull request Nov 19, 2019
Closed
7 tasks
@ebyhr ebyhr deleted the jdbc-column-comment branch November 19, 2019 13:21
@martint martint added this to the 326 milestone Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kokosing kokosing kokosing left review comments

@findepi findepi findepi approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
326
Development

Successfully merging this pull request may close these issues.

Add support for showing column comment in JDBC connector
4 participants
@ebyhr @findepi @kokosing @martint