[#3888] Metadata Support ClickHouse #5819
Conversation
# Conflicts: # gradle.properties # web/web/src/lib/utils/initial.js
flaming-archer
changed the title
|
@shaofengshi @xunliu Pls help to review this pr. |
|
|
||
| @Override | ||
| public String fromGravitino(Type type) { | ||
| if (type instanceof Types.ByteType) { |
There was a problem hiding this comment.
I'd do typeName = type.getTypeName() and use a switch statement for the name.
The if ... else if is a headache to me.
There was a problem hiding this comment.
My gut feeling is that this conversion between type and type names can be generalized
and then put to JdbcTypeConverter.
We can then maintain the peculiarities for ClickHouse here.
There was a problem hiding this comment.
I'd do
typeName = type.getTypeName()and use aswitchstatement for the name. Theif ... else ifis a headache to me.
MySql, Doris,OceanBase, psql ，it's all like this. Perhaps maintaining this would be better？Should we use one issue to make the changes uniformly in the future.
There was a problem hiding this comment.
My gut feeling is that this conversion between type and type names can be generalized and then put to
JdbcTypeConverter. We can then maintain the peculiarities for ClickHouse here.
I thinl, in fact, it is simply taking its own type from the gravitino type, processing mostly the types of its own engine, most of which are different. Unless the type of gravitino is separated and a callback is written to return its own type. But the code is also quite obscure.
There was a problem hiding this comment.
My suggestion was to promote this to the JDBC level from ClickHouse level.
I was not suggesting to promote this to the Gravitino level.
...ain/java/org/apache/gravitino/catalog/clickhouse/operation/ClickHouseDatabaseOperations.java
Outdated
Show resolved
Hide resolved
Among the 5000 lines, most of them are ck code, and most of them are ck test code. As a new catlog, it may still be quite normal. |
|
Hi @flaming-archer thanks a lot for your contribution. Since this is a big feature, would you please create a epic issue and break down things into small tasks, also it would be nice to have a design doc beforehand. You can check and follow what OceanBase did (#4848). Thanks a lot. |
|
Hi @flaming-archer thanks a lot for your contribution, can we just break down into small subtasks, and start with a design doc? @mchades from the community will help you to get everything merged. Thanks a lot. |
…ullable (dataType)
Clickhouse 0.7 Fix alter table operation bug
OK |
OK |
flaming-archer
force-pushed
the
clickhouse_0.7_forpr
branch
from
22a4d78 to
a2cbba3
Compare
...ain/java/org/apache/gravitino/catalog/clickhouse/operation/ClickHouseDatabaseOperations.java
Outdated
Show resolved
Hide resolved
| try (Statement statement = connection.createStatement()) { | ||
| try (ResultSet resultSet = statement.executeQuery("SHOW DATABASES")) { |
There was a problem hiding this comment.
I'm not sure what we gain from these three levels of try blocks ...
There was a problem hiding this comment.
I'm not sure what we gain from these three levels of
tryblocks ...
fixed
| private static String quote(String name) { | ||
| return BACK_QUOTE + name + BACK_QUOTE; | ||
| } | ||
| } |
There was a problem hiding this comment.
Sir. This file is not trivial. Please consider move it out to a separate PR.
There was a problem hiding this comment.
Sir. This file is not trivial. Please consider move it out to a separate PR.
There are indeed many unique logic behind it
Title
What changes were proposed in this pull request?
Support clickhouse as a metadata.
Why are the changes needed?
No new api.
Because ck is a new metadata, there has been a change in page classification.