-
Notifications
You must be signed in to change notification settings - Fork 1.9k
clickhouse dialect implementation #98
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
Conversation
| } | ||
| else if (meta.type.includes("Int") || meta.type.includes("Float")) { | ||
| // convert all numbers into strings | ||
| row[field] = `${value}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why numbers have to be converted to strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lots of the cubejs-schema-compiler tests that deal with measures assert that they are strings.
examples
- https://github.com/statsbotco/cube.js/blob/73db73f874582ad86b3b01d4e93a04c3385e7003/packages/cubejs-schema-compiler/test/DataSchemaCompilerTest.js#L158
- https://github.com/statsbotco/cube.js/blob/73db73f874582ad86b3b01d4e93a04c3385e7003/packages/cubejs-schema-compiler/test/GraphBuilderTest.js#L318
- https://github.com/statsbotco/cube.js/blob/73db73f874582ad86b3b01d4e93a04c3385e7003/packages/cubejs-schema-compiler/test/GraphBuilderTest.js#L365
- https://github.com/statsbotco/cube.js/blob/73db73f874582ad86b3b01d4e93a04c3385e7003/packages/cubejs-schema-compiler/test/GraphBuilderTest.js#L400
- https://github.com/statsbotco/cube.js/blob/73db73f874582ad86b3b01d4e93a04c3385e7003/packages/cubejs-schema-compiler/test/GraphBuilderTest.js#L534
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this is due to the fact that the range of values that the javascript number type supports can overflow. Clickhouse caters for this by returning strings for UInt64,Int64 already. I assume postgres does the same, and assumed that this was the protocol for cube.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand this is only an issue with Int64 and UInt64 data types. How is it handled for Postgres which also has support for 64-bit numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paveltiunov what should we be doing here with numbers vs strings ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably have some variations from driver to driver but as a rule of thumb all numeric types should be returned as strings due to JS can't handle correctly big integers and decimal numbers at all.
Also regarding Timestamp format: driver should always return only local dates in YYYY-DD-MMTHH:MM:SS.sss format as all conversions are done in SQL. There's only one case driver can return UTC: it's UTC timestamp representation in cube.js server local timezone. Otherwise there should be no 'Z' at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like something to be added to driver development guidelines :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inikolaev Yep. Definitely worth adding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re timestamps - I am confused as for example
The following test uses timezone America/Los_Angeles
However the timestamp asserted is UTC (including a Z)
Actually - Every date in that test is formatted the same.
Is there an example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cameronbraid Yep. It's confusing. The thing is Postgres is that exception from the rule: it provides UTC timestamp representation in cube.js server local timezone. Actually it causing problems so we're considering to fix this in the Postgres driver itself.
ac04bf3 to
803c506
Compare
803c506 to
7b880d2
Compare
|
I've re-worked the PR It now no longer changes any existing tests, just adds a copy of them specific to clickhouse. I fixed the timestamp formatting for clickhouse Lots of passing tests : ClickHouse DataSchemaCompiler ClickHouse JoinGraph 35 passing (4s) ✨ Done in 5.06s. |
|
@cameronbraid Looks great overall! I think we're ready to merge. Idea of creating test suite for drivers is great. Need to think about what's the best way to organize it so all tests can be reused. |
Implementation of a
ClickHouseQueryclassIn:
packages/cubejs-schema-compiler/test/DataSchemaCompilerTest.jspackages/cubejs-schema-compiler/test/GraphBuilderTest.js:I made some slight mods with the aim to make the DbRunner based tests plugable. However I ended up cloning these two tests for ClickHouse as there were some other changes required.
I added a docker-compose.yml to provide a local instance of postgres and clickhouse to run the graph builder and schema compiler tests. This required changing DbRunner to specify a user and pass as I couldn't get it working with a default user.
ClickhouseDbRunner
The expected failing tests are marked as skipped