-
Notifications
You must be signed in to change notification settings - Fork 168
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
[FLINK-32068] connector jdbc support clickhouse #49
base: main
Are you sure you want to change the base?
Conversation
WenDing-Y
commented
May 18, 2023
- the clickhouse in batch mode ,as sink and source
- the clickhouse in streaming mode ,as sink
Thanks for opening this pull request! Please check out our contributing guidelines. (https://flink.apache.org/contributing/how-to-contribute.html) |
@WenDing-Y Could you please rebase? |
Okay, I have rebased, but I found that there were quite a few modifications in the testing section. I have already submitted some code, but the unit test cannot pass now. I am still modifying this section. @MartijnVisser |
the pr is ready |
Please help me review, thank you very much @snuyanzin |
flink-connector-jdbc/pom.xml
Outdated
<dependency> | ||
<groupId>org.testcontainers</groupId> | ||
<artifactId>clickhouse</artifactId> | ||
<version>1.18.1</version> |
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.
this is not needed as testcontainers-bom is defined on parent..
if needed should be used ${testcontainers.version} to use same version defined
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.
The testcontainer in the parent pom corresponds to ru.yandex.clickhouse.ClickHouseDriver, but this driver has stopped maintenance, so I have declared a new version of the image here, corresponding to the driver com.clickhouse.jdbc.ClickHouseDriver
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.
I hope you can give me some advice on how to handle it. Thank you very much
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.
we can update the version on parent
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.
If I modify this version number in parent pom.xml, it will affect other testcontainers
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.
the parent pom is
<dependency>
<groupId>org.testcontainers</groupId>
<artifactId>testcontainers-bom</artifactId>
<version>${testcontainers.version}</version>
<type>pom</type>
<scope>import</scope>
</dependency>
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.
we have update master.. if you rebase will be available the last version o testcontainers
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.
ok,I have deleted the specified version @eskabetxe
return fields; | ||
} | ||
|
||
public String getName() { |
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.
already exist a getTableName()
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.
Ok, I have deleted it
getTableName(), | ||
getStreamFields().map(TableField::asString).collect(Collectors.joining(", ")), | ||
"ENGINE = MergeTree", | ||
getStreamFields().map(TableField::getName).findFirst().get()); |
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.
you are ignoring the PKs defined in table definition with this...
return String.format("truncate table %s", getTableName()); | ||
} | ||
|
||
private Stream<TableField> getStreamFields() { |
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.
TableBase already have a getStreamFields()
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.
This method is private, can I change it to protected
return String.format( | ||
"CREATE TABLE %s (%s) %s PRIMARY KEY (%s)", | ||
getTableName(), | ||
getStreamFields().map(TableField::asString).collect(Collectors.joining(", ")), |
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.
as this is also used on TableBase could we create a method for it there?
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.
The syntax for creating a table in the clickhouse like
CREATE TABLE my_first_table ( user_id UInt32, message String, timestamp DateTime, metric Float32 ) ENGINE = MergeTree PRIMARY KEY (user_id, timestamp)
somewhat different from standard SQL, so I think it is reasonable for ClickhouseTableRow to extend TableBase and then Override the getCreateQuery method. If I put it in TableBase, I also need to choose different methods based on the dialect to obtain the statement for creating the table
@@ -322,6 +325,9 @@ void testBatchSink() throws Exception { | |||
|
|||
@Test | |||
void testReadingFromChangelogSource() throws Exception { | |||
if (!getMetadata().supportUpsert()) { |
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.
Here the problem is the deletion no? (I have this same problem in Trino dialect)
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.
If I remove this judgment, this test case will fail. I would like to hear your opinion on how to handle this issue
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.
the approach is fine, I only dont know if the name of method is the correct (supportUpsert)..
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.
Ok, I will modify this issue later
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.
I have updated the method name
@@ -291,4 +291,12 @@ protected <T> T getNullable(ResultSet rs, FunctionWithException<ResultSet, T, SQ | |||
protected <T> T getNullable(ResultSet rs, T value) throws SQLException { | |||
return rs.wasNull() ? null : value; | |||
} | |||
|
|||
public TableField[] getFields() { |
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.
for the use I see you should use getStreamFields()
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.
this is not used, could be removed
compile_and_test (1.18-SNAPSHOT) fail, It doesn't seem like the problem was caused by my submission |
Please help me review, where else can I improve? Thank you very much @eskabetxe |
What other tasks are needed for this PR to merge |
You will need to rebase, because there are a couple of merge commits when you merged |
Ok, I have rebase @MartijnVisser |
I don't think you have, given that there are 4 merge-commits in this PR which shouldn't be there when rebased properly: bb13e1f bced473 9abdfaf 2655b6b |
// Define MAX/MIN precision of TIMESTAMP type according to Mysql docs: | ||
// https://dev.mysql.com/doc/refman/8.0/en/fractional-seconds.html |
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.
Shouldn't we refer here to Clickhouse doc?
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.
Sorry, there was a slight error in my rebase just now, which caused this to happen. Now this has always been resolved
private static final int MIN_TIMESTAMP_PRECISION = 1; | ||
|
||
// Define MAX/MIN precision of DECIMAL type according to Mysql docs: | ||
// https://dev.mysql.com/doc/refman/8.0/en/fixed-point-types.html |
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.
Shouldn't we refer here to Clickhouse doc?
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.
Sorry, there was a slight error in my rebase just now, which caused this to happen. Now this has always been resolved
import org.apache.flink.connector.jdbc.converter.AbstractJdbcRowConverter; | ||
import org.apache.flink.table.types.logical.RowType; | ||
|
||
/** */ |
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.
well it's better to have some description about the class rather than an empty comment
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.
Sorry, there was a slight error in my rebase just now, which caused this to happen. Now this has always been resolved
import org.apache.flink.connector.jdbc.dialect.JdbcDialect; | ||
import org.apache.flink.connector.jdbc.dialect.JdbcDialectFactory; | ||
|
||
/** */ |
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.
well it's better to have some description about the class rather than an empty comment
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.
Sorry, there was a slight error in my rebase just now, which caused this to happen. Now this has always been resolved
import java.util.Optional; | ||
import java.util.Set; | ||
|
||
/** */ |
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.
well it's better to have some description about the class rather than an empty comment
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.
Sorry, there was a slight error in my rebase just now, which caused this to happen. Now this has always been resolved
} else if (jdbcField instanceof Long) { | ||
return ((Long) jdbcField).longValue(); | ||
} | ||
// UINT64 is not supported,the uint64 range exceeds the long range |
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.
can we use BigInteger
for UINT64
?
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.
the clickhouse data types range
UInt8 — [0 : 255]
UInt16 — [0 : 65535]
UInt32 — [0 : 4294967295]
UInt64 — [0 : 18446744073709551615]
UInt128 — [0 : 340282366920938463463374607431768211455]
UInt256 — [0 : 115792089237316195423570985008687907853269984665640564039457584007913129639935]
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.
i still don't see the issue why for UInt64
, UInt128
, UInt256
we can not return BigInteger
?
Sorry i was late here, thanks for your PR @WenDing-Y
and
|
Sorry, it's my problem. I have rebase it again @MartijnVisser |
Sorry, there was a slight error in my rebase just now, which caused this to happen. Now this has always been resolved |
Unfortunately it hasn't been resolved. In case you want to rebase, what you should do is the following (this is assuming that you've added this repository as a remote called
|
It seems like this, my local submit history, the new feature started working after [Flink-31551] |
if you execute this command on your branch
you'll se several merge remote-tracking commits like
and this the issue |
What else do I need to do? I executed it |
how do you recognize that |
b20eb66
to
59b1122
Compare
|
@eskabetxe I guess you might have defined the table incorrectly |
667e72b
to
4facf4c
Compare
@WenDing-Y hello, in our project, we want to import data to clickhouse. Now this PR no longer merges with the main branch? |