Skip to content

Metabase fixes#247

Merged
serebrserg merged 3 commits intoClickHouse:metabase-fixesfrom
ei-grad:metabase-fixes
Dec 28, 2018
Merged

Metabase fixes#247
serebrserg merged 3 commits intoClickHouse:metabase-fixesfrom
ei-grad:metabase-fixes

Conversation

@ei-grad
Copy link
Copy Markdown
Contributor

@ei-grad ei-grad commented Oct 19, 2018 •
edited
Loading

I came to this changes into clickhouse-jdbc for metabase/metabase#8722.

@ei-grad
Memory tables are just tables
f041258 
Why they should be excluded from the metadata?
@ei-grad ei-grad mentioned this pull request Oct 19, 2018
Closed
@ei-grad ei-grad force-pushed the metabase-fixes branch 3 times, most recently from a7a703c to 1adaeef Compare October 19, 2018 12:46
@enqueue
Copy link
Copy Markdown
Contributor

enqueue commented Oct 20, 2018 •
edited
Loading

@ei-grad I think this will not fix the issue because the ClickHouse server does not understand "raw" hyphens or dashes in schema names.

enqueue :) create database foo-bar;

Syntax error: failed at position 20:

create database foo-bar;

Expected one of: ENGINE, storage definition, INTO OUTFILE, FORMAT, ON

Escaped:

fmueller :) create database `foo-bar`;

CREATE DATABASE `foo-bar`

Ok.

I think it might be worthwhile adjusting the driver in Metabase to use proper escaping or translate "-" into "_". I do not know any Clojure, but I am going to give it a go, based on your branch in PR metabase/metabase#8722

Is there anything else I can help with in the JDBC part? I am eager to get the combination ClickHouse / Metabase going, too.

@ei-grad
Copy link
Copy Markdown
Contributor Author

ei-grad commented Oct 20, 2018 •
edited
Loading

It's wrong, database name in ClickHouse is a FS directory name, and it looks like it could consist of any characters. Just use backtick quotes around the identifier as the ANSI SQL says for names escaping.

create database `test-data`

It works.

@enqueue
Copy link
Copy Markdown
Contributor

enqueue commented Oct 20, 2018

You are right, escaping works fine, but the Metabase driver will need to know. Sorry for writing in this JDBC driver PR. I just checked it out, ran the Metabase test suite and encountered this error (using prostgres escaping mode).

@ei-grad
Copy link
Copy Markdown
Contributor Author

ei-grad commented Oct 20, 2018

No problems, you are welcome :).

@enqueue enqueue mentioned this pull request Oct 26, 2018
Closed
@enqueue enqueue mentioned this pull request Nov 24, 2018
Closed
serebrserg
serebrserg approved these changes Nov 25, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@serebrserg serebrserg left a comment

Choose a reason for hiding this comment

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

Is this still work in progress or can it be merged? I don't see a problem merging it now.

src/main/java/ru/yandex/clickhouse/ClickHouseDatabaseMetadata.java
if (e == "View" || e == "MaterializedView" || e == "Merge" || e == "Distributed" || e == "Null") {
type = "VIEW"; // some kind of view
} else if (e == "Memory" || e == "Set" || e == "Join" || e == "Buffer") {
} else if (e == "Set" || e == "Join" || e == "Buffer") {
Copy link
Copy Markdown
Contributor

@serebrserg serebrserg Nov 25, 2018

Choose a reason for hiding this comment

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

Could you explain the case for this change?
This is really pretty uncertain where to place Memory engine. I agree that it may be closer to TABLE, but would like to know the specific case.

Copy link
Copy Markdown
Contributor Author

@ei-grad ei-grad Nov 26, 2018

Choose a reason for hiding this comment

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

Memory engine tables could be used in tests.

Copy link
Copy Markdown
Contributor Author

@ei-grad ei-grad Nov 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Or like a more-table-like alternative to the buffer tables in data pipeline (where you really don't want to see the buffer tables in the list, but would like to see the memory ones).

@ei-grad
Copy link
Copy Markdown
Contributor Author

ei-grad commented Nov 26, 2018

Is this still work in progress or can it be merged? I don't see a problem merging it now.

Nope, it would be great to merge it I think.

@ei-grad ei-grad changed the title Metabase fixes [WIP] Metabase fixes Nov 26, 2018
@serebrserg serebrserg changed the base branch from master to metabase-fixes December 28, 2018 10:00
@serebrserg serebrserg merged commit 674bed3 into ClickHouse:metabase-fixes Dec 28, 2018
@serebrserg
Copy link
Copy Markdown
Contributor

serebrserg commented Dec 28, 2018

Sorry, I think I have broken your branch while resolving conflicts.
Resolved here #285

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@serebrserg serebrserg serebrserg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ei-grad @enqueue @serebrserg