Clickhouse connector #541

briandennis · 2018-12-06T16:27:08Z

This PR adds support for connecting to Clickhouse (https://clickhouse.yandex/) databases.

cc @bpostlethwaite

briandennis · 2018-12-06T16:41:27Z

@nicolaskruchten @tarzzz

Do either of you know what's causing the test failure? (https://circleci.com/gh/plotly/falcon/6819)

I don't think it's related to these changes.

tarzzz · 2018-12-08T00:14:59Z

backend/persistent/datastores/clickhouse.js

+    columnnames: [ 'tablename', 'column_name','data_type' ],
+    rows
+  }))
+}


tarzzz · 2018-12-08T00:19:50Z

package.json

@@ -19,6 +19,8 @@
    "docker:mssql:start": "docker run -e 'ACCEPT_EULA=Y' -e 'SA_PASSWORD=yourStrong(!)Password' -p 1433:1433 -d microsoft/mssql-server-linux:latest",
    "docker:oracle:build": "docker build test/docker/oracle -t falcon-test-oracle --no-cache",
    "docker:oracle:start": "docker run --rm -ti -p 1521:1521 falcon-test-oracle",
+    "docker:clickhouse:build": "docker build test/docker/clickhouse -t falcon-test-clickhouse --no-cache",


Unless there is a good reason for keeping --no-cache, we should remove it. It would just slow things for developers.

tarzzz · 2018-12-08T00:21:33Z

test/docker/clickhouse/README.md

+```sh
+docker run -ti -p 8123:8123 -p 9000:9000 falcon-clickhouse
+```


tarzzz · 2018-12-08T00:26:18Z

package.json

@@ -244,7 +244,7 @@
    "yamljs": "^0.3.0"
  },
  "dependencies": {
-    "@apla/clickhouse": "^1.5.3",
+    "@apla/clickhouse": "1.5.3",


Would be good to mention the reason for locking the dependency version in the commit message. Useful in case we need to upgrade in future.

Good call, updated 👍

briandennis · 2018-12-11T16:07:06Z

Looks like the tests are still breaking due to changes unrelated to this PR. Running test/backend/Datastores.spec.js locally, I'm getting quite a few errors in addition to the one triggering the CI failure.

Independent of these issues, the core Clickhouse functionality here should be ready to merge in. I consider this PR blocked pending #543.

cc @bpostlethwaite

briandennis · 2018-12-12T19:37:26Z

@tarzzz @nicolaskruchten

Everything looks green now ✅ Unless you have any other feedback, this should be good to go!

tarzzz · 2018-12-12T19:40:06Z

Thanks @briandennis ..

I plan on testing it with on-premise some time later today, and then we should be good to merge it.

tarzzz · 2018-12-14T17:06:52Z

After connecting to clickhouse DB, both the "query" and "schedule" tabs seem disabled.

Can you please check the same from your end.

briandennis · 2018-12-17T16:07:00Z

After connecting to clickhouse DB, both the "query" and "schedule" tabs seem disabled.

Seems this is a known issue not specific to this connector (see here and here). The reason it's happening is because the driver was able to connect, but encountered an error at a latter step. In this case, the username was not set so it threw when trying to query.

While updating the general connection error behavior on the FE is probably outside the scope of this PR, we should implicitly set username and database to the default Clickhouse values to avoid this particular issue in the first place. We'll update the PR accordingly

tarzzz · 2018-12-18T16:49:49Z

Noticed another UI related concern while testing. To replicate:

Create a new falcon connection. Set the "max rows to read" to 1000. Wait to connect.
Click on "Query" tab. It gives the following error:

I tried changing the query to 100, but hitting Run does not return anything.

In short: adding any value to "max rows to read" breaks the query-ing UI..

bpostlethwaite · 2018-12-18T17:08:04Z

@tarzzz is that bug report something that only affects Clickhouse connector or does it impact other connector types as well?

tarzzz · 2018-12-18T17:22:28Z

@bpostlethwaite Since we have the "max rows to read" option available only for clickhouse, it is only applicable for this particular connector.

briandennis · 2018-12-18T19:01:02Z

@tarzzz I'm not sure this is a bug.

I don't think the max_rows_to_read setting in Clickhouse is affected by LIMIT clauses. I believe the feature is used to constrain the performance of query execution, not to put a cap on the number of rows returned. For example, if max_rows_to_read = 1000 and LIMIT = 10, querying a table with 10,000 rows would still violate the requirement, since you could possibly have to read more than 1000 rows to find the first 10 that match.

In this instance, if you bump the setting to greater than the exceeded value (168689...), does the query succeed?

tarzzz · 2018-12-18T19:06:25Z

@briandennis Bug in a UI sense that providing a value of limit freezes the "Run" button (It does not do anything). If I leave it blank, everything works.. but as soon as I add a value (any value), the "Run" button freezes (and the data table below is empty).

If I change the value to be greater than the "exceeded value", it seems to work.

nicolaskruchten · 2018-12-18T21:21:20Z

if the max_rows_to_read setting affects the max size of the table that can be read rather than the result size then perhaps we should just indicate that in the connection-config UI?

tarzzz · 2018-12-18T21:30:31Z

if the max_rows_to_read setting affects the max size of the table that can be read rather than the result size then perhaps we should just indicate that in the connection-config UI?

Seems like it. I was wondering whether it is better to skip the option entirely. Having an option incentives setting a value, and users may not understand it or know the number of rows in table they are trying to access. Setting any lesser value will cause the query to error.

nicolaskruchten · 2018-12-18T21:35:27Z

yeah, leaving that option out is maybe a safer choice UX-wise

tarzzz

Tested on on-prem, and connection and scheduling LGTM..

💃 after rebasing and removing the empty commit: 1db6e4c

* Define Codemirror SQL dialect for ClickHouse * fix up `set` function

For running clickhouse tests.

briandennis requested a review from tarzzz December 6, 2018 16:27

tarzzz reviewed Dec 8, 2018

View reviewed changes

backend/persistent/datastores/clickhouse.js

columnnames: [ 'tablename', 'column_name','data_type' ],

rows

}))

}

Copy link

Contributor

tarzzz Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

tarzzz reviewed Dec 8, 2018

View reviewed changes

test/docker/clickhouse/README.md

```sh

docker run -ti -p 8123:8123 -p 9000:9000 falcon-clickhouse

```

Copy link

Contributor

tarzzz Dec 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

tarzzz reviewed Dec 8, 2018

View reviewed changes

briandennis force-pushed the clickhouse branch from 97cefec to bdb7fb9 Compare December 11, 2018 15:02

tarzzz mentioned this pull request Dec 12, 2018

Add back S3 and Apache drill tests #545

Open

tarzzz force-pushed the clickhouse branch 2 times, most recently from 6fd2f38 to 9d4e6ce Compare December 12, 2018 15:50

tarzzz approved these changes Dec 19, 2018

View reviewed changes

Sean Rabaut and others added 6 commits December 19, 2018 11:59

ClickHouse integration

789cbff

Add more config options

c1937d1

setting changes

dbc237e

Tests for ClickHouse

701c2e5

finish README

5170b77

Define Codemirror SQL dialect for ClickHouse (#3)

8772c1a

* Define Codemirror SQL dialect for ClickHouse * fix up `set` function

Sean Rabaut and others added 15 commits December 19, 2018 11:59

Update option constants

58120be

Lock Clickhouse driver to prevent minor version breakages

e082392

add invalid query test

834e0bf

change to exists

f9d880c

Fix readonly config

83048e3

remove console.log

755d419

add keywords to Clickhouse codemirror script

a948929

linting fixes

d3b7852

Run clickhouse container on circle.

d4044ac

For running clickhouse tests.

allow build caching for docker Clickhouse

47ccac0

add testURL to jest config

08f2ecf

Fix DialectSelector test

e4192ea

use default username

b5561e3

update clickhouse test

c942cc8

remove max_rows_to_read

7123c8e

briandennis force-pushed the clickhouse branch from 7384f6f to 7123c8e Compare December 19, 2018 17:02

briandennis merged commit b208c6e into master Dec 19, 2018

briandennis deleted the clickhouse branch December 19, 2018 17:39

babybear1992 mentioned this pull request May 22, 2020

After connecting to clickhouse DB, both the "query" and "schedule" tabs seem disabled. #587

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clickhouse connector #541

Clickhouse connector #541

briandennis commented Dec 6, 2018

briandennis commented Dec 6, 2018

tarzzz Dec 8, 2018

tarzzz Dec 8, 2018

tarzzz Dec 8, 2018

tarzzz Dec 8, 2018

briandennis Dec 11, 2018

briandennis commented Dec 11, 2018

briandennis commented Dec 12, 2018

tarzzz commented Dec 12, 2018

tarzzz commented Dec 14, 2018

briandennis commented Dec 17, 2018

tarzzz commented Dec 18, 2018

bpostlethwaite commented Dec 18, 2018

tarzzz commented Dec 18, 2018

briandennis commented Dec 18, 2018

tarzzz commented Dec 18, 2018 •

edited

Loading

nicolaskruchten commented Dec 18, 2018 •

edited

Loading

tarzzz commented Dec 18, 2018 •

edited

Loading

nicolaskruchten commented Dec 18, 2018

tarzzz left a comment •

edited

Loading

Clickhouse connector #541

Clickhouse connector #541

Conversation

briandennis commented Dec 6, 2018

briandennis commented Dec 6, 2018

tarzzz Dec 8, 2018

Choose a reason for hiding this comment

tarzzz Dec 8, 2018

Choose a reason for hiding this comment

tarzzz Dec 8, 2018

Choose a reason for hiding this comment

tarzzz Dec 8, 2018

Choose a reason for hiding this comment

briandennis Dec 11, 2018

Choose a reason for hiding this comment

briandennis commented Dec 11, 2018

briandennis commented Dec 12, 2018

tarzzz commented Dec 12, 2018

tarzzz commented Dec 14, 2018

briandennis commented Dec 17, 2018

tarzzz commented Dec 18, 2018

bpostlethwaite commented Dec 18, 2018

tarzzz commented Dec 18, 2018

briandennis commented Dec 18, 2018

tarzzz commented Dec 18, 2018 • edited Loading

nicolaskruchten commented Dec 18, 2018 • edited Loading

tarzzz commented Dec 18, 2018 • edited Loading

nicolaskruchten commented Dec 18, 2018

tarzzz left a comment • edited Loading

Choose a reason for hiding this comment

tarzzz commented Dec 18, 2018 •

edited

Loading

nicolaskruchten commented Dec 18, 2018 •

edited

Loading

tarzzz commented Dec 18, 2018 •

edited

Loading

tarzzz left a comment •

edited

Loading