-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
clickhouse connector #17596
clickhouse connector #17596
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks nice, @ellison840611
a few more comments
presto-clickhouse/pom.xml
Outdated
<groupId>com.facebook.presto</groupId> | ||
<version>0.273-SNAPSHOT</version> | ||
</parent> | ||
<modelVersion>4.0.0</modelVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modelVersion above parent
private static final String TempTableNamePrefix = "tmp_presto_"; | ||
protected final String connectorId; | ||
protected final ConnectionFactory connectionFactory; | ||
protected static final String identifierQuote = "\""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's put static final constants together. put identifierQuote just below TempTableNamePrefix
.put(TIMESTAMP, "timestamp") | ||
.put(TIMESTAMP_WITH_TIME_ZONE, "timestamp with timezone") | ||
.build(); | ||
private static final String TempTableNamePrefix = "tmp_presto_"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/TempTableNamePrefix/tempTableNamePrefix/g
columns.add(new ClickHouseColumnHandle(connectorId, columnName, typeHandle, columnMapping.get().getType(), nullable)); | ||
} | ||
else { | ||
System.out.println("The clickHouse datatype: " + typeHandle.getJdbcTypeName() + " unsupported."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use log.info, instead of System out
public void abortReadConnection(Connection connection) | ||
throws SQLException | ||
{ | ||
// most drivers do not need this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove this comment
skipTestUnless(supportsNotNullColumns()); | ||
|
||
String catalog = getSession().getCatalog().get(); | ||
// String createTableStatement = "CREATE TABLE " + catalog + ".tpch.test_not_null_with_insert (\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove all these commented code
assertQueryFails("INSERT INTO test_not_null_with_insert (column_a) VALUES (date '2012-12-31')", "(?s).*NULL.*column_b.*"); | ||
assertQueryFails("INSERT INTO test_not_null_with_insert (column_a, column_b) VALUES (date '2012-12-31', null)", "(?s).*NULL.*column_b.*"); | ||
|
||
//assertUpdate("ALTER TABLE test_not_null_with_insert ADD COLUMN column_c BIGINT NOT NULL"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented code
assertUpdate("DROP TABLE test_not_null_with_insert"); | ||
} | ||
|
||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented code
==================== | ||
|
||
The ClickHouse connector allows querying tables in an external | ||
`Yandex ClickHouse <https://clickhouse.tech/>`_ server. This can be used to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove Yandex
To connect to a ClickHouse server, you need: | ||
|
||
* ClickHouse version 20.8 or higher. | ||
* Network access from the Trino coordinator and workers to the ClickHouse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Trino/Presto/g
0db1e35
to
5b4f057
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellison840611 mostly good
a few minor things
private String toWriteMapping(Type type) | ||
{ | ||
if (type == BOOLEAN) { | ||
// ClickHouse is no separate type for boolean values. Use UInt8 type, restricted to the values 0 or 1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rephrase the comment to:
ClickHouse uses UInt8 as boolean, restricted values to 0 and 1
return "String"; | ||
} | ||
if (type instanceof VarbinaryType) { | ||
// Strings of an arbitrary length. The length is not limited |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment:
Strings of arbitrary length.
return userCredentialName; | ||
} | ||
|
||
@Config("clickhouse.user-credential-name") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellison840611 ping
STRIPELOG("StripeLog"), | ||
LOG("Log"), | ||
TINYLOG("TinyLog"), | ||
MERGETREE("MergeTree()"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need ()
here?
...to-clickhouse/src/main/java/com/facebook/presto/plugin/clickhouse/ClickhouseDXLKeyWords.java
Show resolved
Hide resolved
} | ||
|
||
@Config("clickhouse.password-credential") | ||
public ClickHouseConfig setPasswordCredentialName(String passwordCredentialName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/setPasswordCredentialName/setPasswordCredential/g
return userCredentialName; | ||
} | ||
|
||
@Config("clickhouse.user-credential-name") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/setUserCredentialName/setUserCredential/g
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhenxiao, I will pass the review given the connector change is pretty much contained. Feel free to merge if it looks good to you.
== RELEASE NOTES ==
General Changes