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

Refactor IDataType with Generic Type constraint #247

Open
pan3793 opened this issue Dec 11, 2020 · 3 comments · May be fixed by #248
Open

Refactor IDataType with Generic Type constraint #247

pan3793 opened this issue Dec 11, 2020 · 3 comments · May be fixed by #248

Comments

@pan3793
Copy link
Member

pan3793 commented Dec 11, 2020

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
Copy link
Collaborator

Yes i think this is great.

@sbouchex
Copy link
Contributor

Sound good. MySQL is using the same of mechanism.

@pan3793
Copy link
Member Author

pan3793 commented Jan 27, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants