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

Refactor to always use JdbcColumnHandle.Builder instead of a constructor #18718

Closed
wants to merge 1 commit into from
Closed

Refactor to always use JdbcColumnHandle.Builder instead of a constructor #18718

wants to merge 1 commit into from

Conversation

dominikzalewski
Copy link
Member

Description

Refactor to always use JdbcColumnHandle.Builder instead of a constructor. This is a prerequisite change for a bugfix.

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:

@cla-bot cla-bot bot added the cla-signed label Aug 17, 2023
@dominikzalewski dominikzalewski force-pushed the jdbc_column_handle_builder_refactor branch from e5c5282 to 29b4c74 Compare August 17, 2023 14:01
@dominikzalewski dominikzalewski force-pushed the jdbc_column_handle_builder_refactor branch from 29b4c74 to 80d1dee Compare August 17, 2023 14:16
wendigo
wendigo approved these changes Aug 17, 2023
View reviewed changes
Copy link
Contributor

@wendigo wendigo left a comment

Choose a reason for hiding this comment

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

LGTM % method arguments wrapping

@dominikzalewski dominikzalewski force-pushed the jdbc_column_handle_builder_refactor branch from 80d1dee to 6df5803 Compare August 18, 2023 09:42
@dominikzalewski
Copy link
Member Author

LGTM % method arguments wrapping

Thanks for the review. I believe I have addressed every comment you made.

hashhar
hashhar reviewed Aug 18, 2023
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.

No opinions on this one. I don't see apparent benefits since we trade a constructor for a static method with same number of arguments.

LGTM % how do we prevent new usages of the constructor from appearing?

@wendigo
Copy link
Contributor

wendigo commented Aug 18, 2023 •
edited
Loading

@hashhar do you think that we should make constructor privateprotected right away?

@hashhar
Copy link
Member

hashhar commented Aug 18, 2023

I don't see the benefit of doing that - i.e. why is this change useful? There will always be code which needs to create JdbcColumnHandle. Whether it uses a builder or a constructor or a static method doesn't make any difference. The constructor/static is actually safer since there are no defaults being assumed like when using the Builder.

@wendigo
Copy link
Contributor

wendigo commented Aug 18, 2023

@hashhar this is required for the upcoming change that will introduce new field (optional) on the JdbcColumnHandle. Hence the builder usage.

@hashhar
Copy link
Member

hashhar commented Aug 18, 2023

@hashhar do you think that we should make constructor privateprotected right away?

It'll be needed to be done if we want to avoid the bug we want to fix from being reintroduced without noticing.

@ebyhr
Copy link
Member

ebyhr commented Aug 23, 2023

This is a prerequisite change for a bugfix.

Can you share the link of the bug?

@hashhar
Copy link
Member

hashhar commented Aug 24, 2023

@ebyhr See #18642

kokosing
kokosing reviewed Aug 24, 2023
View reviewed changes
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcColumnHandle.java
@@ -152,6 +152,15 @@ public long getRetainedSizeInBytes()
+ jdbcTypeHandle.getRetainedSizeInBytes();
}

public static JdbcColumnHandle createJdbcColumnHandle(String columnName, JdbcTypeHandle jdbcTypeHandle, Type columnType)
Copy link
Member

Choose a reason for hiding this comment

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

Refactor to always use JdbcColumnHandle.Builder instead of a constructor

createJdbcColumnHandle method defeats the purpose of this commit. What is the difference between factory method and constructor? I would inline this method.

@dominikzalewski dominikzalewski mentioned this pull request Aug 29, 2023
Closed
@ebyhr
Copy link
Member

ebyhr commented Sep 15, 2023

#18924 is already merged. Do we still need this PR?

@dominikzalewski
Copy link
Member Author

Sorry, we don't need it any more. My fault leaving the trash behind. Closed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wendigo wendigo wendigo approved these changes

@kokosing kokosing kokosing left review comments

@hashhar hashhar hashhar left review comments

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dominikzalewski @wendigo @hashhar @ebyhr @kokosing