Skip to content

Remove deprecated methods from JdbcTableHandle#13654

Merged
hashhar merged 7 commits intotrinodb:masterfrom
tangjiangling:remove-deprecated-methods-from-JdbcTableHandle
Sep 13, 2022
Merged

Remove deprecated methods from JdbcTableHandle#13654
hashhar merged 7 commits intotrinodb:masterfrom
tangjiangling:remove-deprecated-methods-from-JdbcTableHandle

Conversation

@tangjiangling
Copy link
Copy Markdown
Member

@tangjiangling tangjiangling commented Aug 12, 2022

Related issues, pull requests, and links

Fixes #6797

Documentation

(x) No documentation is needed.

Release notes

(x) No release notes entries required.

@cla-bot cla-bot bot added the cla-signed label Aug 12, 2022
@tangjiangling tangjiangling changed the title Remove deprecated methods from jdbc table handle Remove deprecated methods from JdbcTableHandle Aug 12, 2022
@tangjiangling tangjiangling force-pushed the remove-deprecated-methods-from-JdbcTableHandle branch from edb917f to 0a41656 Compare August 12, 2022 18:37
@tangjiangling tangjiangling marked this pull request as ready for review August 12, 2022 18:43
@tangjiangling tangjiangling added release-notes maintenance Project maintenance task labels Aug 12, 2022
@tangjiangling tangjiangling requested review from ebyhr, findepi and hashhar and removed request for findepi and hashhar August 12, 2022 18:43
@tangjiangling tangjiangling added no-release-notes This pull request does not require release notes entry and removed release-notes labels Aug 12, 2022
Copy link
Copy Markdown
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Please check CI failures.

@tangjiangling tangjiangling force-pushed the remove-deprecated-methods-from-JdbcTableHandle branch from 3ae02bf to e668c5b Compare August 13, 2022 06:44
@tangjiangling
Copy link
Copy Markdown
Member Author

Squashed commits and fixed CI.

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Aug 16, 2022

How did you decide on when to use asPlainTable vs getRequiredNamedRelation?

@tangjiangling tangjiangling force-pushed the remove-deprecated-methods-from-JdbcTableHandle branch from e668c5b to ddc2879 Compare August 18, 2022 13:27
@tangjiangling
Copy link
Copy Markdown
Member Author

How did you decide on when to use asPlainTable vs getRequiredNamedRelation?

This is a really good question, and I'm probably not the best person to answer it (CC: @findepi).

But I can share my understanding:

  • If a table is not synthetic, like in JdbcClient#getTableHandle, JdbcClient#renameTable,JdbcClient#addColumn,JdbcClient#dropColumn and other such method calls, it is obvious that "Table" will simply mean which table under which database, and will not contain other information (such as filter push-down, limit push-down, etc.), I would use JdbcTableHandle#asPlainTable (as in the naming description, it just describes which table under which database)
  • If a table is synthetic (such as the optimizer's various push-downs: filter push-down, limit push-down, etc., which carry additional information than just a plain table), I would use JdbcTableHandle#getRequiredNamedRelation

To summarize, you need to know what state your table is in at the time of use (synthetic or non-synthetic) before you can choose whether to use getRequiredNamedRelation or asPlainTable.

Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % ci.

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Aug 18, 2022

I agree with your understanding of the usages @tangjiangling . However to me this seems like the API around JdbcTableHandle should be revisited at some point. The table handle today is basically a single class which can represent two different types of objects - actual tables as they exist in remote database or something represented by a PreparedQuery - maybe they should each be their own class and that way we can have the type-system guide us what kind of handle is expected where.

We'll discuss with Piotr once he's back and follow-up accordingly.

This PR looks good to me.

@tangjiangling tangjiangling force-pushed the remove-deprecated-methods-from-JdbcTableHandle branch from ddc2879 to adb77cf Compare August 18, 2022 14:58
@tangjiangling
Copy link
Copy Markdown
Member Author

(Fixed CI)

@tangjiangling
Copy link
Copy Markdown
Member Author

CI hit #13362

@tangjiangling tangjiangling force-pushed the remove-deprecated-methods-from-JdbcTableHandle branch from 20520f2 to 436f23d Compare August 26, 2022 04:26
@tangjiangling
Copy link
Copy Markdown
Member Author

(Cherry-pick to resolve conflicts)

@hashhar hashhar self-requested a review August 26, 2022 06:00
@tangjiangling tangjiangling force-pushed the remove-deprecated-methods-from-JdbcTableHandle branch from 436f23d to 582a766 Compare September 8, 2022 17:05
@tangjiangling
Copy link
Copy Markdown
Member Author

(Cherry-pick to resolve conflicts again)

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Sep 10, 2022

Thanks. I did check that all the deprecations have been there for at-least 2 years now (almost since the Trino rename).

@hashhar hashhar merged commit d7faf84 into trinodb:master Sep 13, 2022
@github-actions github-actions bot added this to the 396 milestone Sep 13, 2022
@tangjiangling tangjiangling deleted the remove-deprecated-methods-from-JdbcTableHandle branch September 13, 2022 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed maintenance Project maintenance task no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

Remove usages of deprecated JdbcTableHandle accessors

3 participants