Add ClickHouse support by edmundito · Pull Request #2467 · grouparoo/grouparoo

edmundito · 2021-10-26T19:57:56Z

Change description

This change adds a plugin and demo generator for the ClickHouse OLAP DB. Supports source and destination, table and query imports. The plugin is based on on the MySQL plugin.

Demo usage:

grouparoo demo --clickhouse

Checklists

Development

Application changes have been tested appropriately

Impact

Code follows company security practices and guidelines
Security impact of change has been considered
Performance impact of change has been considered
Possible migration needs considered (model migrations, config migrations, etc.)

Please explain any security, performance, migration, or other impacts if relevant:

Code review

Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached where applicable.
Relevant tags have been added to the PR (bug, enhancement, internal, etc.)

evantahler

Nice! You've set up the boilerplate.

There are some changes to make the package.json, but otherwise this looks good!

evantahler

I think the next step is to wire up CI. Then there are few TODOs to fixup, and this will be ready!

edmundito · 2021-11-09T22:07:55Z

All the tests are no passing, fixed the template issues and rebased with main. I still need to apply the changes from #2522

* Add custom getColumns function to identify CH types

* Cleanup types in MySQL plugin * Update connection ifaces in plugin to accept type instead of `any`

…d fns

* Update table import to use own implementation of getting data * Implement selects to do type casting based on column types in order to be able to apply the queries * Update `cleanupDataType` to not change case and move to `utils`

…te templates

edmundito · 2021-11-11T16:09:26Z

Applied feedback from the review.

After looking into how to resolve the problems with the mysql query conflicts while running the tests in parallel, I couldn't find a good answer online and I feared that this would be come a problem in production. I saw the same issue occur while running exports from the UI app. So, I implemented a custom connect with the clickhouse npm module, which uses the HTTP interface.

evantahler

🥳

Just lock that dependency, and this is 👍!

edmundito · 2021-11-11T20:56:30Z

Addressed review comments and pushed changes!

krishnaglick · 2021-11-11T19:11:55Z

+import path from "path";
+import fs from "fs-extra";
+import os from "os";
+
+process.env.GROUPAROO_INJECTED_PLUGINS = JSON.stringify({
+  "@grouparoo/clickhouse": { path: path.join(__dirname, "..", "..") },
+});
+
+import { helper } from "@grouparoo/spec-helper";
+import { api } from "actionhero";
+import { Generate } from "@grouparoo/core/src/bin/generate";
+import { Apply } from "@grouparoo/core/src/bin/apply";
+import {
+  beforeData,
+  afterData,
+  appOptions,
+  usersTableName,
+} from "../utils/data";
+
+process.env.GROUPAROO_CONFIG_DIR = `${os.tmpdir()}/test/${
+  process.env.JEST_WORKER_ID
+}/config`;


Feels a little odd that these aren't all imports -> env
Do the later imports require they be set?

@evantahler is this intentional for a reason?

krishnaglick · 2021-11-11T19:13:40Z

+        // .replace(`host: "localhost"`, `host: "${appOptions.host}"`)
+        // .replace(`port: 5432`, `port: "${appOptions.port}"`)


If these are commented out, do we need them?

krishnaglick · 2021-11-11T19:15:51Z

+      // mapping: { id: "userId" },
+      // state: "ready",


krishnaglick · 2021-11-11T19:16:36Z

+        session
+      );
+    expect(bootstrapResponse.error).toBeUndefined();
+    expect(bootstrapResponse.property).toBeTruthy();


If this is a complex object, assuming it doesn't change, could do a snapshot against it.
Or test the object+properties directly if we want that level of detail.

krishnaglick · 2021-11-11T20:41:11Z

+  groupForeignKey = groupForeignKey?.toString();
+  groupColumnName = groupColumnName?.toString();
+
+  if (Object.keys(newRecordProperties).length === 0) {


Suggested change

if (Object.keys(newRecordProperties).length === 0) {

if (!Object.keys(newRecordProperties).length) {

Purely stylistic choice.

krishnaglick · 2021-11-11T20:48:17Z

+              oldRecordProperties[k] !== undefined)
+        );
+
+        if (columnsToErase.length > 0) {


Suggested change

if (columnsToErase.length > 0) {

if (columnsToErase.length) {

Minor nitpick!

krishnaglick · 2021-11-11T20:52:03Z

+    error = e;
+  } finally {
+    if (error) {
+      throw error;
+    }


Why not just let this throw? Or is error getting grabbed at a higher level?

I'm not sure but this is how it's implemented in all the SQL-based plugins.

krishnaglick · 2021-11-11T20:54:05Z

+  const out: DataResponseRow[] = [];
+  const rows = await connection.asyncQuery(query, params);
+  rows.forEach((row) => out.push(row));
+  return out;


Why not just return await connection.asyncQuery(query, params)?

Not sure either, this was a copy and paste from mysql and there may be a reason why copying the values into anew array ([...out]) ?

…clickhouse-plugin

edmundito · 2021-11-12T14:32:04Z

Pushed one more change to address @krishnaglick's feedback and update the version to match main. Ready to merge.

krishnaglick · 2021-11-12T15:06:44Z

+    const { host, port, database, user, password } = appOptions;
+
+    let url = String(host);
+    if (url && !url.match(/^localhost/i) && !url.match(/^https?/)) {


It might be good to combine these match calls.

edmundito added the enhancement New feature or request label Oct 26, 2021

edmundito requested a review from evantahler October 26, 2021 19:58

evantahler reviewed Oct 26, 2021

View reviewed changes

Comment thread plugins/@grouparoo/clickhouse/package.json

Comment thread plugins/@grouparoo/clickhouse/package.json Outdated

Comment thread plugins/@grouparoo/clickhouse/package.json Outdated

edmundito commented Oct 28, 2021

View reviewed changes

Comment thread plugins/@grouparoo/clickhouse/src/initializers/plugin.ts Outdated

evantahler reviewed Oct 28, 2021

View reviewed changes

Comment thread plugins/@grouparoo/clickhouse/README.md

evantahler reviewed Nov 9, 2021

View reviewed changes

Comment thread core/src/classes/plugin.ts Outdated

Comment thread plugins/@grouparoo/clickhouse/src/lib/export/exportRecord.ts Outdated

Comment thread plugins/@grouparoo/clickhouse/src/lib/table-import/getPropertyValues.ts

edmundito force-pushed the 180005482-clickhouse-plugin branch from 60bb6ed to be90b6d Compare November 9, 2021 21:58

edmundito changed the title ~~Add ClickHouse plugin~~ Add ClickHouse plugin and demo generator Nov 10, 2021

edmundito marked this pull request as ready for review November 10, 2021 22:33

evantahler reviewed Nov 10, 2021

View reviewed changes

edmundito and others added 19 commits November 11, 2021 09:10

Add clickhouse plugin project setup

28795c4

Updating things automatically

27ed9ff

Update grouparoo dep versions in clickhouse plugin package

2bc35b3

Update clickhouse to use the mysql plugin as the app connect command

3c835f3

Add clickhouse demo generator

407dffb

Add precision to decimal type in clickhouse demo

2e6caf2

Add support for Sources in ClickHouse

5f52bc0

* Add custom getColumns function to identify CH types

Add clickhouse plugin to apps

8787ae9

Update mysql to clickhouse in ch plugin init

e97f534

Add support for destination and export in ClickHouse

acb441e

* Cleanup types in MySQL plugin * Update connection ifaces in plugin to accept type instead of `any`

Fix oldRecordProperties nil check in mysql and clickhouse exportRecor…

40c9a5e

…d fns

Update clickhouse exportRecord to omit pk on updates

f038c2b

Improve DataType cleanup

2422f8b

Fix typo in db plugin tests

4977cb9

Update plugin classes to specify Connection class

e37e693

Specify connection type in MySQL plugin types

9e18975

Add ClickHouse tests for propertyOptions

e79ad54

Fix ClickHouse DateTime64 casting to add precision and add record tests

496f2c8

edmundito added 4 commits November 11, 2021 09:15

Add Connection type generic to AppQueryMethod iface

8ae4fbf

Update ClickHouse plugin logo and add attribution

0a97b55

Replace clickhouse logo with svg logo

27f447a

Update clickhouse connection configs to support multiple apps

f0f2a72

edmundito force-pushed the 180005482-clickhouse-plugin branch from 208c4ec to f0f2a72 Compare November 11, 2021 14:21

Add ClickHouse connect function to connect to HTTP interface and upda…

b3f7084

…te templates

edmundito added 3 commits November 11, 2021 11:18

Add mysql dep directly to clickhouse

456c2e7

Revert clickhouse logo to png

cf6ca15

Add default user back to appOptions

0db2143

edmundito requested review from bleonard and krishnaglick November 11, 2021 18:04

evantahler approved these changes Nov 11, 2021

View reviewed changes

Comment thread plugins/@grouparoo/clickhouse/package.json Outdated

Comment thread plugins/@grouparoo/clickhouse/public/templates/app/{{{id}}}.js.template

Comment thread plugins/@grouparoo/clickhouse/src/lib/connect.ts

Comment thread plugins/@grouparoo/clickhouse/src/lib/connect.ts Outdated

edmundito added 2 commits November 11, 2021 15:54

Lock clickhouse dependency

0a7bdae

Update getMajorVersion return type

c4bddab

krishnaglick approved these changes Nov 11, 2021

View reviewed changes

edmundito added 2 commits November 12, 2021 09:01

Cleanup clickhouse plugin code

808df7e

Merge branch 'main' of github.com:grouparoo/grouparoo into 180005482-…

0ef8dac

…clickhouse-plugin

edmundito added 2 commits November 12, 2021 09:55

Match jest version in clickhouse plugin

e9b9bb5

Revert string casting on clickhouse connect

ec71e07

krishnaglick approved these changes Nov 12, 2021

View reviewed changes

Combine clickhouse connect localhost | https? check

868c627

krishnaglick approved these changes Nov 12, 2021

View reviewed changes

edmundito merged commit 49f5734 into main Nov 12, 2021

edmundito deleted the 180005482-clickhouse-plugin branch November 12, 2021 17:07

evantahler mentioned this pull request Nov 12, 2021

Feature Request: Clickhouse Source #2337

Closed

evantahler linked an issue Nov 12, 2021 that may be closed by this pull request

Feature Request: Clickhouse Source #2337

Closed

bleonard changed the title ~~Add ClickHouse plugin and demo generator~~ Add ClickHouse support Nov 12, 2021

		// .replace(`host: "localhost"`, `host: "${appOptions.host}"`)
		// .replace(`port: 5432`, `port: "${appOptions.port}"`)

	if (Object.keys(newRecordProperties).length === 0) {
	if (!Object.keys(newRecordProperties).length) {

	if (columnsToErase.length > 0) {
	if (columnsToErase.length) {

Conversation

edmundito commented Oct 26, 2021 • edited by evantahler Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change description

Checklists

Development

Impact

Code review

Uh oh!

evantahler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evantahler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edmundito commented Nov 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edmundito commented Nov 11, 2021

Uh oh!

evantahler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edmundito commented Nov 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edmundito commented Nov 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

edmundito commented Oct 26, 2021 •

edited by evantahler

Loading