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

introduce JDBCTypeDescriptor.getDefaultSqlTypeCode() #4105

Merged
merged 4 commits into from Jul 28, 2021

Conversation

gavinking
Copy link
Member

The goal of this PR is to cleanly separate usages of the JDBC type code in interactions with the JDBC API, with usages of the JDBC type code to identify a SQL column type in schema export.

This should fix a bug I introduced a while back.

Also rename sqlType -> jdbcTypeCodes in Type hierarchy for consistency.

See #4088

cc @beikov @sebersole

@gavinking gavinking force-pushed the sql-type-code branch 2 times, most recently from 2a36190 to 190d4e5 Compare July 23, 2021 19:50
@hibernate hibernate deleted a comment from hibernate-github-bot bot Jul 23, 2021
@dreab8 dreab8 added the 6.0 label Jul 26, 2021
@@ -108,7 +108,7 @@
*
* @throws MappingException Generally indicates an issue accessing the passed mapping object.
*/
int[] sqlTypes(Mapping mapping) throws MappingException;
int[] getSqlTypeCodes(Mapping mapping) throws MappingException;
Copy link
Contributor

Choose a reason for hiding this comment

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

As much as I like this change, I think this is a backwards compatibility concern. Not sure if that is something we are ok with, but maybe revert the renaming here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Depends what @sebersole thinks.

Copy link
Member

Choose a reason for hiding this comment

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

I do think it would be nice to minimize changes as much as we can.

Here though, I think we should just make this change. This contract is changing already wrt read-from-jdbc , so implementors and consumers are going to have to change. If we do the whole deprecation paradigm, we are just making them make changes again later if/when we remove the deprecated form)

@@ -20,7 +20,7 @@
*/
public interface SingleColumnType<T> extends Type {

int sqlType();
int getJdbcTypeCode();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same backwards compatibility concern here as for Type.

Copy link
Contributor

@beikov beikov left a comment

Choose a reason for hiding this comment

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

The PR looks good so far, but I am missing some changes to JdbcTypeDescriptorRegistry. IMO the default key for registration should be the sqlTypeCode to be able to have e.g. descriptors for both GIS types and JSON types which both map to the Jdbc type code Types.OTHER

@gavinking
Copy link
Member Author

Yeah, I guess that's right.

@hibernate hibernate deleted a comment from hibernate-github-bot bot Jul 27, 2021
@gavinking
Copy link
Member Author

IMO the default key for registration should be the sqlTypeCode to be able to have

Yeah, I guess that's right.

Well, perhaps not, actually, that's unclear.

Currently the only type we have were the SQL and JDBC types differ is DoubleTypeDescriptor. And for DoubleTypeDescriptor the current implementation does what we want.

Now, naturally we can special-case DoubleTypeDescriptor in JdbcTypeDescriptorBaseline, but it just seems a bit suspicious that in the one case where there's a difference, the correct rule is opposite of the one you're suggesting.

@hibernate hibernate deleted a comment from hibernate-github-bot bot Jul 27, 2021
@sebersole
Copy link
Member

IMO the default key for registration should be the sqlTypeCode to be able to have e.g. descriptors for both GIS types and JSON types which both map to the Jdbc type code Types.OTHER

Nothing says the "conflicts" have to register under the key for OTHER. In fact, the point of the dynamic registry is that they wouldn't.

@sebersole
Copy link
Member

Just pushed a fix to a breakage I caused in the build. Sorry. Can you rebase this from upstream wip/6.0 and push again?

@beikov beikov merged commit 8dd0ed7 into hibernate:wip/6.0 Jul 28, 2021
@gavinking gavinking deleted the sql-type-code branch June 15, 2023 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants