feat: implemented getColumnClassName #2113
Conversation
|
|
|
@livk-cloud the build failed: |
|
@chernser Hey, bro。I've tried to fix the above and remove the API introduced in JDK9，Thanks for the review |
| case Enum16: | ||
| case IPv4: | ||
| case IPv6: | ||
| case JSON: |
There was a problem hiding this comment.
JSON - it depends on query settings com.clickhouse.client.api.internal.ServerSettings#OUTPUT_FORMAT_BINARY_WRITE_JSON_AS_STRING.
Object - is deprecated. We should use OTHER .
| break; | ||
| case Enum8: | ||
| case Enum16: | ||
| case IPv4: |
There was a problem hiding this comment.
Technically IP addresses are stored as series of numbers, but we can return them either way. If we put VARCHAR for it - what will be Spring behavior?
| typeName = "ARRAY"; | ||
| break; | ||
| case Enum8: | ||
| case Enum16: |
There was a problem hiding this comment.
Enum s are SMALLINT as they are stored as int8, int16.
What will be Spring behavior in this case?
| typeName = "VARCHAR"; | ||
| break; | ||
| default: | ||
| typeName = "BINARY"; |
There was a problem hiding this comment.
it cannot be BINARY by default. Is there options like VARIANT or OTHER?
| } | ||
|
|
||
| @Override | ||
| public int toSqlType(ClickHouseColumn column, Map<String, Class<?>> typeMap) { |
There was a problem hiding this comment.
we have this mapping already com.clickhouse.jdbc.internal.JdbcUtils#CLICKHOUSE_TO_SQL_TYPE_MAP
| /** | ||
| * Inner class for static initialization. | ||
| */ | ||
| static final class InstanceHolder { |
There was a problem hiding this comment.
Why do we need an instance holder? can we have just simple static block in JdbcTypeMapping class?
We are trying to have classes with some meaningful functions that can be reused in different places. Current class is private and just holds static values.
| * @param sqlType generic SQL types defined in JDBC | ||
| * @return non-null ClickHouse data type | ||
| */ | ||
| protected ClickHouseDataType getDataType(int sqlType) { |
There was a problem hiding this comment.
we have this mapping in com.clickhouse.jdbc.internal.JdbcUtils#SQL_TYPE_TO_CLASS_MAP
| */ | ||
| static final class InstanceHolder { | ||
| private static final JdbcTypeMapping defaultMapping = ClickHouseUtils | ||
| .getService(JdbcTypeMapping.class, JdbcTypeMapping::new); |
There was a problem hiding this comment.
who else will implement JdbcTypeMapping ?
Is there a glue code in application for that?
|
There is good idea of customizable mappings. With different dialects and variety of tools should help. |
I'm coming up with a New Year's holiday in the coming days, and I probably don't have much time to code. Considering the multiple dialects, I think there should be a common interface that uses SPI for loading processing, what do you think about this? |
|
@livk-cloud I think we can do with simple properties. Besides not always it is possible to manipulate a service loader when only URL is available to setup. |
After some trying, I found that the getColumnClassName method returns Null and works fine with these frameworks, what do you think? |
|
Hi @livk-cloud ! |
|
@chernser Of course, the easiest way to do this is to return Null. But I don't know if this has a bad effect on other ORM frameworks |
Implement the getColumnClassName method to accommodate Spring Jdbc and Mybatis