cube-js / cube

📊 Cube — Universal semantic layer platform for AI, BI, spreadsheets, and embedded analytics
https://cube.dev
Other
17.97k stars 1.78k forks source link

Postgres Driver SET TIMEZONE #3633

Open webfrank opened 3 years ago

webfrank commented 3 years ago

Describe the bug I'm using CrateDB with Postgres wire protocol. I've found the current implementation of prepareConnection inside PostgresDriver uses this line: await conn.query(SET TIME ZONE '${this.config.storeTimezone || 'UTC'}'); which for sure works for Postgres but not with compatible ones which needs this one: await conn.query(SET TIMEZONE = '${this.config.storeTimezone || 'UTC'}');

This latest syntax also works on Postgres and it seems the standard one (from my research).

I propose to just change this line or create a new driver, CrateDBDriver, copy of PostgresDriver with only this line changed.

To Reproduce Steps to reproduce the behavior:

  1. Connect to a CrateDB Instance
  2. Try to open Schema from playground
  3. See error

Expected behavior As CrateDB uses Postgres wire protocol and high compatibility for syntax it should work out of the box

Version: 0.28.52

github-actions[bot] commented 3 years ago

If you are interested in working on this issue, please leave a comment below and we will be happy to assign the issue to you. If this is the first time you are contributing a Pull Request to Cube.js, please check our contribution guidelines. You can also post any questions while contributing in the #contributors channel in the Cube.js Slack.

webfrank commented 3 years ago

Hi, I will try to create the new driver for CrateDB, I got another issue inside BaseDriver which tries to create a new schema but on CrateDB this is not necessary so it should be override inside the driver.

ovr commented 3 years ago

Hello @webfrank ,

Looks like it's better to do a new driver, and there is a good example:

https://github.com/cube-js/cube.js/tree/master/packages/cubejs-redshift-driver

Redshift driver is based on top of PostgreSQL driver. You should use the same approach for the CrateDB driver, and you will be able to override prepareConnection.

https://github.com/cube-js/cube.js/blob/v0.28.53/packages/cubejs-postgres-driver/src/PostgresDriver.ts#L157

Why is it better to do a new driver instead of hacking Postgres driver?

  1. In the feature it will be possible to implement unload (Export table using COPY TO) for CrateDB driver for speeding up pre-aggregations.
  2. Any changes in PostgresDriver can break drivers that use it as a base, for example, CrateDB.

Thanks

rongfengliang commented 3 years ago

@webfrank i had write one cratedb driver you can try it https://github.com/rongfengliang/cubejs-cratedb-driver