search

housepower / ClickHouse-Native-JDBC

ClickHouse Native Protocol JDBC implementation
https://housepower.github.io/ClickHouse-Native-JDBC/
Apache License 2.0
529 stars 145 forks source link

Refactor IDataType with Generic Type constraint #247

Open pan3793 opened 3 years ago

pan3793 commented 3 years ago

Background

IDataType is the bridge between ClickHouse type system and JDBC type system, and we find there are some limitations in current implementation, the most important one I think is the lack of Generic Type constraint. Without this constraint, serializeBinary can only accept Object, and deserializeBinary can only return Object, which causes all static type checks lose effectiveness.

The root cause here is that JDBC type and ClickHouse type are not a 1:1 mapping, and we mixed the UIntX and IntX in one DataTypeIntX make things worse. Because Java Generic Type system not support MixIn(or Hybrid) Type constraint, Object is the only option.

Another thing is that we found some JDBC-GUI Tools like DataGrip will call ResultSet#getString to render the value and display it. The temporary solution in codebase is roughly return object#toString.

Based on above, I think we need to refactor IDataType.

Proposal

  1. Decouple IDataType type and JDBC type;
    • One ClickHouse type mapping one IDataType with the property Java type, i.e. DateTime map to DataTypeDateTime(ZonedDateTime);
    • Introduce a converter layer between IDataType and JDBC interface type, thus DataTypeDateTime(ZonedDateTime) can map to java.sql.Date, Timestamp, Calendar;
      1. Split DataTypeIntX into DataTypeUIntX and DataTypeIntX;
      2. Add Generic Type constraint on IDataType to get more benefits of static type checks;
      3. Add capability of render value to property String for display in IDataType.

After this, it will be easier to implement features like #265.

Consider this change is a bit large, it will not included in release v2.5.0.

@sundy-li @sbouchex Please let me know what you guys thinking of this proposal.

Updated

This proposal has been almost implemented in #295, and the followup PR will coming soon.

andy1xx8 commented 3 years ago

Yes i think this is great.

sbouchex commented 3 years ago

Sound good. MySQL is using the same of mechanism.

pan3793 commented 3 years ago

Updated

We found significant performance downgrade at least in JDBC interface, and I will do some profile in next couple days to determine the most expensive time cost process.

An alternative approach is abandon single root Generic Type constraint on IDataType but spilt it into several primitive interfaces to avoid box/unbox overhead, I will open another GH-ISSUE to talk this.