Skip to content

Remove redundant table metadata calls #17269

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.

Sign up for GitHub

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
Merged
findepi merged 2 commits into from
Apr 28, 2023
Merged

Remove redundant table metadata calls #17269

findepi merged 2 commits into from
Apr 28, 2023

Conversation

@findepi
Copy link
Member

@findepi findepi commented Apr 27, 2023

This retrofits existing ConnectorMetadata.getSchemaTableName. The method remains deprecated, because a new name, getTableName is better.

As a benefit of this change, the ConnectorMetadata's getTableMetadata and getTableSchema no longer need to support synthetic table handles. This fixes a bug in connectors that support advanced pushdown. For example, HiveMetadata.getTableMetadata called for HiveTableHandle having projectedColumns should return column metadata for the projected columns, but it did not do that.

@findepi findepi added the no-release-notes This pull request does not require release notes entry label Apr 27, 2023
@cla-bot cla-bot bot added the cla-signed label Apr 27, 2023
This was referenced Apr 27, 2023
Closed
Merged
krvikash
krvikash reviewed Apr 27, 2023
View reviewed changes
@findepi findepi force-pushed the findepi/remove-redundant-table-metadata-calls-170b62 branch from 87f38f0 to 5c9c4c5 Compare April 27, 2023 14:54
@findepi
Copy link
Member Author

findepi commented Apr 27, 2023

thanks @krvikash , updated

@github-actions github-actions bot added bigquery BigQuery connector delta-lake Delta Lake connector hive Hive connector iceberg Iceberg connector mongodb MongoDB connector tests:hive labels Apr 27, 2023
@krvikash
Copy link
Contributor

krvikash commented Apr 28, 2023

Delta CI is failing.

Error:  io.trino.plugin.deltalake.TestDeltaLakePerTransactionMetastoreCache.testPerTransactionHiveMetastoreCachingDisabled  Time elapsed: 1.076 s  <<< FAILURE!
java.lang.AssertionError: 
Expected: 
		6 more occurrences of GET_TABLE

@findepi findepi force-pushed the findepi/remove-redundant-table-metadata-calls-170b62 branch from 5c9c4c5 to d7ae703 Compare April 28, 2023 10:49
@findepi findepi requested a review from krvikash April 28, 2023 10:52
findinpath
findinpath reviewed Apr 28, 2023
View reviewed changes
core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java Outdated
Copy link
Contributor

@findinpath findinpath Apr 28, 2023

Choose a reason for hiding this comment

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

Why is the new method calling a deprecated method?

Copy link
Member Author

@findepi findepi Apr 28, 2023

Choose a reason for hiding this comment

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

because getSchemaTableName already existed and can be implemented by a connector

this PR is effectively rename (from ConnectorMetadata perspective) and this is how you handle a rename in a backwards-compatible way

kokosing
kokosing reviewed Apr 28, 2023
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.

% comments

core/trino-main/src/main/java/io/trino/execution/TableInfo.java Outdated
Copy link
Member

@kokosing kokosing Apr 28, 2023

Choose a reason for hiding this comment

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

Do we still nee getTableSchema? It was supposed to be faster than getting table metadata and it is still slow anyway?

Copy link
Member Author

@findepi findepi Apr 28, 2023

Choose a reason for hiding this comment

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

good question. io.trino.metadata.Metadata#getTableSchema is still used & not going away

yes, io.trino.metadata.Metadata#getTableSchema is faster than getTableMetadata, but getTableName is even faster (usually no-op, since table handles typically contain the name)

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java Outdated
Copy link
Member

@kokosing kokosing Apr 28, 2023

Choose a reason for hiding this comment

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

do we need to do this refactor (rename)? Maybe separate commit?

Copy link
Member Author

@findepi findepi Apr 28, 2023

Choose a reason for hiding this comment

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

i did this because this method was deprecated and almost removed, i didn't want ot add new usages for a method that's already known not to be implemented
would it help if we do #17267 first?

kokosing
kokosing approved these changes Apr 28, 2023
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.

% comments

@findepi
Copy link
Member Author

findepi commented Apr 28, 2023

thank you @kokosing for your review. answered

findepi added 2 commits April 28, 2023 21:58
@findepi
Simplify query pass-through handle creation
dd2a2eb
@findepi
Remove redundant table metadata calls
220afce 
This retrofits existing `ConnectorMetadata.getSchemaTableName`. The
method remains deprecated, because a new name, `getTableName` is better.

As a benefit of this change, the `ConnectorMetadata`'s
`getTableMetadata` and `getTableSchema` no longer need to support
synthetic table handles. This fixes a bug in connectors that support
advanced pushdown. For example, `HiveMetadata.getTableMetadata` called
for `HiveTableHandle` having `projectedColumns` should return column
metadata for the projected columns, but it did not do that.
@findepi findepi force-pushed the findepi/remove-redundant-table-metadata-calls-170b62 branch from d7ae703 to 220afce Compare April 28, 2023 19:59
@findepi findepi merged commit 4fcf76f into trinodb:master Apr 28, 2023
@findepi findepi deleted the findepi/remove-redundant-table-metadata-calls-170b62 branch April 28, 2023 19:59
@github-actions github-actions bot added this to the 416 milestone Apr 28, 2023
@colebow colebow mentioned this pull request May 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kokosing kokosing kokosing approved these changes

@electrum electrum Awaiting requested review from electrum

@martint martint Awaiting requested review from martint

@losipiuk losipiuk Awaiting requested review from losipiuk

@arhimondr arhimondr Awaiting requested review from arhimondr

@ebyhr ebyhr Awaiting requested review from ebyhr

@Praveen2112 Praveen2112 Awaiting requested review from Praveen2112

+2 more reviewers

@findinpath findinpath findinpath left review comments

@krvikash krvikash krvikash approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bigquery BigQuery connector cla-signed delta-lake Delta Lake connector hive Hive connector iceberg Iceberg connector mongodb MongoDB connector no-release-notes This pull request does not require release notes entry

Milestone

416

Development

Successfully merging this pull request may close these issues.

4 participants

@findepi @krvikash @kokosing @findinpath