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

Add system.metadata.table_comments table #531

Merged
merged 2 commits into from May 7, 2019

Conversation

3 participants
@ebyhr
Copy link
Contributor

commented Mar 24, 2019

Related to #529

@cla-bot cla-bot bot added the cla-signed label Mar 24, 2019

@ebyhr ebyhr force-pushed the ebyhr:system-metadata-table_comments branch 3 times, most recently from 5448c71 to 6e5f4c7 Mar 24, 2019

@electrum
Copy link
Member

left a comment

A few comments, otherwise looks good.

Future enhancement: Since getting metadata is expensive, we might want this to be iterative, so it can return data to the engine as it goes. This could be done with InMemoryRecordSet by passing it an iterator. We'd want a variant of InMemoryRecordSet.Builder#addRow() that constructs a single row object.

public void testInformationSchemaTable()
{
assertUpdate("CREATE TABLE test_comment (c1 bigint) COMMENT 'foo'");
String selectTableComment = format("SELECT comment FROM system.metadata.table_comments WHERE catalog_name = '%s' AND schema_name = '%s' AND table_name = 'test_comment'", getSession().getCatalog().get(), getSession().getSchema().get());

This comment has been minimized.

Copy link
@electrum

electrum Apr 4, 2019

Member

This is a bit long, how about something like (however IntelliJ wraps it)

String selectTableComment = format("" +
    "SELECT comment FROM system.metadata.table_comments " +
    "WHERE catalog_name = '%s' AND schema_name = '%s' AND table_name = 'test_comment'",
    getSession().getCatalog().get(), 
    getSession().getSchema().get());
@@ -1893,6 +1893,16 @@ public void testScaleWriters()
}
}

@Test
public void testInformationSchemaTable()

This comment has been minimized.

Copy link
@electrum

electrum Apr 4, 2019

Member

testTableCommentsTable

Show resolved Hide resolved ...src/main/java/io/prestosql/connector/system/TableCommentSystemTable.java
for (String catalog : filter(listCatalogs(session, metadata, accessControl).keySet(), catalogFilter)) {
QualifiedTablePrefix prefix = FilterUtil.tablePrefix(catalog, schemaFilter, tableFilter);

Set<SchemaTableName> names = new HashSet<>();

This comment has been minimized.

Copy link
@electrum

electrum Apr 4, 2019

Member

This can be ImmutableSet.of()

Builder table = InMemoryRecordSet.builder(COMMENT_TABLE);

for (String catalog : filter(listCatalogs(session, metadata, accessControl).keySet(), catalogFilter)) {
QualifiedTablePrefix prefix = FilterUtil.tablePrefix(catalog, schemaFilter, tableFilter);

This comment has been minimized.

Copy link
@electrum

electrum Apr 4, 2019

Member

Static import

QualifiedObjectName tableName = new QualifiedObjectName(prefix.getCatalogName(), name.getSchemaName(), name.getTableName());
Optional<String> comment = Optional.empty();
try {
Optional<TableHandle> tableHandle = metadata.getTableHandle(session, tableName);

This comment has been minimized.

Copy link
@electrum

electrum Apr 4, 2019

Member

I think this can simplify to

comment = metadata.getTableHandle(session, tableName)
        .map(handle -> metadata.getTableMetadata(session, handle))
        .map(metadata -> metadata.getMetadata().getComment());

@ebyhr ebyhr force-pushed the ebyhr:system-metadata-table_comments branch from 6e5f4c7 to 852367d Apr 5, 2019

@ebyhr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

@electrum Thank you for your reviews. Applied your comments and added NPE at

catch (PrestoException | NullPointerException ignored) {

@electrum

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

@ebyhr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

@electrum Okay. I already sent the PR to fix it (#592). Let me rebase after the PR is merged.

Show resolved Hide resolved ...src/main/java/io/prestosql/connector/system/TableCommentSystemTable.java
catch (PrestoException | NullPointerException ignored) {
// getTableHandle may throw an exception
// Cassandra connector throws PrestoException if the table has case insensitive column names
// Cassandra connector throws NullPointerException if the table has unsupported column types

This comment has been minimized.

Copy link
@kokosing

kokosing Apr 5, 2019

Member

Can we somehow fix this, logic that relies on NPE sounds...

This comment has been minimized.

Copy link
@ebyhr

ebyhr Apr 5, 2019

Author Contributor

I'll fix it in this PR #592.

.map(metadata -> metadata.getMetadata().getComment())
.get();
}
catch (PrestoException | NullPointerException ignored) {

This comment has been minimized.

Copy link
@kokosing

kokosing Apr 5, 2019

Member

log the exception to LOG.debug. Remove usage of NPE.

@electrum electrum assigned electrum and unassigned electrum Apr 22, 2019

@electrum

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

@ebyhr Please rebase and then assign back to me

@ebyhr ebyhr force-pushed the ebyhr:system-metadata-table_comments branch 2 times, most recently from 718a75a to 02020ce Apr 23, 2019

@ebyhr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@electrum Rebased. The travis failed by timed out (https://api.travis-ci.com/v3/job/194801727/log.txt).

(I don't have the permission to change assignees)

@electrum electrum self-assigned this Apr 29, 2019

@electrum
Copy link
Member

left a comment

A few minor comments. Please squash the fixup commit.

{
assertUpdate("CREATE TABLE test_comment (c1 bigint) COMMENT 'foo'");
String selectTableComment = format("" +
"SELECT comment FROM system.metadata.table_comments " +

This comment has been minimized.

Copy link
@electrum

electrum Apr 29, 2019

Member

Nit: this indentation is off (should be an 8 space continuation indent, not aligned). IntelliJ formatter should fix this.

Optional<String> comment = Optional.empty();
try {
comment = metadata.getTableHandle(session, tableName)
.map(handle -> metadata.getTableMetadata(session, handle))

This comment has been minimized.

Copy link
@electrum

electrum Apr 29, 2019

Member

This indentation is also off

// Cassandra connector throws NullPointerException if the table has unsupported column types
catch (PrestoException e) {
// getTableHandle may throw an exception (e.g. Cassandra connector doesn't allow case insensitive column names)
LOG.debug(e, "Getting table handle failed on " + name);

This comment has been minimized.

Copy link
@electrum

electrum Apr 29, 2019

Member

Logging should use string formatting

LOG.debug(e, "Failed to get metadata for table: %s", name);

@electrum electrum removed their assignment Apr 29, 2019

@ebyhr ebyhr force-pushed the ebyhr:system-metadata-table_comments branch from 02020ce to 8ae2ee2 Apr 30, 2019

@ebyhr ebyhr force-pushed the ebyhr:system-metadata-table_comments branch from 8ae2ee2 to 5aeab65 Apr 30, 2019

@ebyhr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@electrum Thank you for your review. Updated.

@electrum electrum merged commit 450aca0 into prestosql:master May 7, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
verification/cla-signed
Details

@electrum electrum added this to the 311 milestone May 7, 2019

@electrum electrum referenced this pull request May 14, 2019

Closed

Release notes for 311 #716

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.