Clickhouse connector

[briandennis]

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

cc @bpostlethwaite

[briandennis]

@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.

[briandennis]

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 https://github.com/plotly/falcon/pull/543.

cc @bpostlethwaite

[briandennis]

@tarzzz @nicolaskruchten

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

[tarzzz]

Thanks @briandennis ..

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

[tarzzz]

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

Can you please check the same from your end.

[briandennis]

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]

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]

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

[tarzzz]

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

[briandennis]

@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]

@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]

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]

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]

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