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.

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

Use RemoteTableName in JdbcOutputTableHandle #23090

Merged
merged 1 commit into from
Aug 28, 2024
Merged

Use RemoteTableName in JdbcOutputTableHandle #23090

merged 1 commit into from
Aug 28, 2024

Conversation

chenjian2664
Copy link
Contributor

@chenjian2664 chenjian2664 commented Aug 21, 2024 •
edited
Loading

Description

The catalogName and schemaName members are annotated with nullable, but it's easy to miss the info when use getter. Instead, use RemoteTableName, which encapsulates the catalogName and schemaName within Optional wrapper.

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Aug 21, 2024
@chenjian2664 chenjian2664 mentioned this pull request Aug 21, 2024
Open
@chenjian2664 chenjian2664 force-pushed the jdbc_output branch 3 times, most recently from 42d1791 to 36d463b Compare August 21, 2024 14:32
ebyhr
ebyhr reviewed Aug 21, 2024
View reviewed changes
Copy link
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.

Could you describe the motivation in a PR description?

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcOutputTableHandle.java Show resolved Hide resolved
@chenjian2664 chenjian2664 force-pushed the jdbc_output branch 3 times, most recently from 8c85542 to 72ffecf Compare August 22, 2024 03:02
@chenjian2664 chenjian2664 marked this pull request as ready for review August 22, 2024 03:23
kokosing
kokosing reviewed Aug 25, 2024
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.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java Show resolved Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java Outdated Show resolved Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcOutputTableHandle.java Outdated
@@ -67,24 +63,54 @@ public JdbcOutputTableHandle(
this.pageSinkIdColumnName = requireNonNull(pageSinkIdColumnName, "pageSinkIdColumnName is null");
}

public JdbcOutputTableHandle(
Copy link
Member

Choose a reason for hiding this comment

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

Non deprecated constructor should be first. And one constructor should call the other to avoid the copy-paste code.

Copy link
Member

Choose a reason for hiding this comment

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

@chenjian2664 The 2nd comment about copy-paste isn't addressed. Is it intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ebyhr I missed this part, not intentional.

Copy link
Member

Choose a reason for hiding this comment

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

This constructor still has @JsonCreator & @JsonProperty annotations and a new constructor doesn't have those annotations. That's weird for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean we may let the new added one to call the exists constructor?

Copy link
Contributor Author

@chenjian2664 chenjian2664 Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can the constructor remain solely, shifting JSON annotations to new one?
Unsure how other repositories depend on this constructor.
Or We may need to keep the catalogName , schemaName, tableName fields and add the new constructor based on the exists one.
what do you think

Copy link
Member

Choose a reason for hiding this comment

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

RemoteTableName is already JSON serializable so why do we need new deprecated constructor for just JSON serialization purposes?

@kokosing
Copy link
Member

Could you describe the motivation in a PR description?

This is about using a named abstraction where possible. When doing a review of #23034 where the triple (catalog, schema, table) for remote table name was copied again we noticed that we lost information about the fact that catalog and schema are optional. Also notice that even in this PR we noticed that existing code has issues where we passing nullable values to non-nullable arguments. When using RemoteTableName it is clear what is what as Optional is used for nullable fields.

@chenjian2664 chenjian2664 force-pushed the jdbc_output branch 3 times, most recently from bc08f65 to b6715ed Compare August 26, 2024 12:22
@kokosing
Copy link
Member

@ebyhr or @hashhar Do you want to take a look?

ebyhr
ebyhr reviewed Aug 26, 2024
View reviewed changes
Copy link
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.

The commit body is too long. Please warp at 72 characters: https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#commits-and-pull-requests

plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixMergeSink.java Outdated Show resolved Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcOutputTableHandle.java Outdated
@@ -67,24 +63,54 @@ public JdbcOutputTableHandle(
this.pageSinkIdColumnName = requireNonNull(pageSinkIdColumnName, "pageSinkIdColumnName is null");
}

public JdbcOutputTableHandle(
Copy link
Member

Choose a reason for hiding this comment

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

@chenjian2664 The 2nd comment about copy-paste isn't addressed. Is it intentional?

ebyhr
ebyhr approved these changes Aug 26, 2024
View reviewed changes
Copy link
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.

Looks good except for comments.

@chenjian2664 chenjian2664 force-pushed the jdbc_output branch 2 times, most recently from 2e60945 to f08b198 Compare August 27, 2024 02:12
hashhar
hashhar reviewed Aug 27, 2024
View reviewed changes
Copy link
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.

Looks good % comment about JSON annotations in JdbcOutputTableHandle

@kokosing
Copy link
Member

I would vote to remove the deprecated constructors. The external plugins should be able to adjust their constructors to it as it is a simple change. @ebyhr @hashhar @wdyt?

@ebyhr
Copy link
Member

ebyhr commented Aug 27, 2024

Yeah, I think it would be nice to remove it in this situation.

@chenjian2664
Use RemoteTableName in JdbcOutputTableHandle
5f60fea 
Use RemoteTableName, which wraps `catalogName` and `schemaName` in `Optional` to handle their potential absence more explicitly
@kokosing
Copy link
Member

CI is red

@chenjian2664
Copy link
Contributor Author

Error: Failed to execute goal com.github.eirslett:frontend-maven-plugin:1.15.0:npm (package (webapp)) on project trino-web-ui: Failed to run task: 'npm run package' failed. org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1) -> [Help 1]

�It's unrelated to changes?

@kokosing
Copy link
Member

Restarted

@ebyhr ebyhr merged commit 3b253d1 into trinodb:master Aug 28, 2024
62 checks passed
@github-actions github-actions bot added this to the 455 milestone Aug 28, 2024
@chenjian2664 chenjian2664 deleted the jdbc_output branch August 28, 2024 01:35
@mosabua mosabua mentioned this pull request Aug 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebyhr ebyhr ebyhr approved these changes

@hashhar hashhar hashhar approved these changes

@kokosing kokosing kokosing approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
455
Development

Successfully merging this pull request may close these issues.
4 participants
@chenjian2664 @kokosing @ebyhr @hashhar