Add support for setting column type in Clickhouse. #16702
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java
Show resolved
Hide resolved
plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java
Show resolved
Hide resolved
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
1 similar comment
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
| .matches("VALUES bigint '123'"); | ||
| } | ||
| String tableName = "test_set_column_type"; | ||
| assertUpdate("CREATE TABLE " + tableName + " (a bigint, b double NOT NULL, c varchar(50)) WITH (order_by=ARRAY['b'], engine = 'MergeTree')"); |
There was a problem hiding this comment.
Please don't put ClickHouse specific code to BaseConnectorTest. You can add a new test method to BaseClickHouseConnectorTest.
There was a problem hiding this comment.
Sorry my mistake, I didnt realize the inheritance, will try to override, but in general most of these tests are failing because MODIFY COLUMN is not supported in the default engine type Log. So Im trying to create a table of engine MergeTree.
| assertUpdate("ALTER TABLE " + tableName + " ALTER COLUMN a SET DATA TYPE varchar(50)"); | ||
|
|
||
| assertEquals(getColumnType(tableName, "a"), "varchar"); | ||
| assertThat((String) computeScalar("show create table " + tableName)).contains("CREATE TABLE clickhouse.tpch.test_set_column_type (\n" + |
There was a problem hiding this comment.
nit: Uppercase show create table. I would recommend verifying the table definition on ClikcHouse instead of Trino because the connector doesn't support reading all column definitions.
| String tableName = "test_set_column_type"; | ||
| assertUpdate("CREATE TABLE " + tableName + " (a bigint, b double NOT NULL, c varchar(50)) WITH (order_by=ARRAY['b'], engine = 'MergeTree')"); | ||
|
|
||
| assertUpdate("ALTER TABLE " + tableName + " ALTER COLUMN a SET DATA TYPE varchar(50)"); |
There was a problem hiding this comment.
Column a doesn't have any definition. The new doesn't ensure if SET DATA TYPE preserved a column definition correctly or not because it created the table with table level property. I would recommend creating a table on Clickhouse so that we can specify column definitions (default_expr, codec, TTL).
There was a problem hiding this comment.
I was testing if the NOT NULL is retained, will check for default and others.
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
subkanthi
force-pushed
the
clickhouse-set-column-type
branch
from
5387995
to
170546a
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
Please feel free to ping me in Slack https://trino.io/slack.html if you need a help. |
Hi @ebyhr , the in ClickHouse, not even sure if its supported in CH, the parser fails because of the brackets. it works without Engine as it defaults to Log, but Log does not support MODIFY COLUMN. |
Why the cast doesn't contain value and type? By the way, please set code style configuration in your IDE. |
| .add(new SetColumnTypeSetup("decimal(5,3)", "12.345", "decimal(10,3)")) // short decimal -> short decimal | ||
| .add(new SetColumnTypeSetup("decimal(28,3)", "12.345", "decimal(38,3)")) // long decimal -> long decimal | ||
| .add(new SetColumnTypeSetup("decimal(5,3)", "12.345", "decimal(38,3)")) // short decimal -> long decimal | ||
| .add(new SetColumnTypeSetup("decimal(5,3)", "12.340", "decimal(5,2)")) |
There was a problem hiding this comment.
time, timestamp, addrow were removed since it looks like the translation to clickhouse is not implemented.
...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
Outdated
Show resolved
Hide resolved
...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
Outdated
Show resolved
Hide resolved
...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
Outdated
Show resolved
Hide resolved
...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
Outdated
Show resolved
Hide resolved
...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
Outdated
Show resolved
Hide resolved
| assertUpdate("ALTER TABLE " + tableName + " ALTER COLUMN col SET DATA TYPE integer"); | ||
|
|
||
| assertEquals(getColumnType(tableName, "col"), "integer"); | ||
| assertQuery("SELECT col FROM " + tableName, "VALUES -1"); |
There was a problem hiding this comment.
This looks like a bug. We may need to deny such operations. Could you find a GitHub issue of ClickHouse about this?
There was a problem hiding this comment.
int seems to map to Int32 in ClickHouse, the max value in ClickHouse for Int32 is 2147483647.
https://clickhouse.com/docs/en/sql-reference/data-types/int-uint
Clickhouse version: 21.11.10.1
┌─statement──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ CREATE TABLE tpch.test_set_column_out_of_rangekgarmodrdz
(
`col` Int32,
`col2` Int32
)
ENGINE = MergeTree
ORDER BY col2
SETTINGS index_granularity = 8192 │
└────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
Outdated
Show resolved
Hide resolved
...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
Outdated
Show resolved
Hide resolved
|
|
||
| @DataProvider | ||
| @Override | ||
| public Object[][] setColumnTypesDataProvider() |
There was a problem hiding this comment.
Why do you need to override this method? Please override filterSetColumnTypesDataProvider instead.
There was a problem hiding this comment.
overriding filterSetColumnTypesDataProvider doesnt seem to work.
Because the setColumnTypeSetupData is called from setColumnTypesDataProvider
public Object[][] setColumnTypesDataProvider()
{
return setColumnTypeSetupData().stream()
.map(this::filterSetColumnTypesDataProvider)
.flatMap(Optional::stream)
.collect(toDataProvider());
}
There was a problem hiding this comment.
Please take a look at other connectors. e.g. TestIcebergParquetConnectorTest
...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
Outdated
Show resolved
Hide resolved
…to filterSetColumnTypesDataProvider
|
|
||
| @Test | ||
| @Override | ||
| public void testSetColumnType() |
| public void testSetColumnType() | ||
| { | ||
| String tableName = "test_set_column_type" + randomNameSuffix(); | ||
| assertUpdate("CREATE TABLE " + tableName + " (a bigint, b double NOT NULL, c varchar(50)) WITH (order_by=ARRAY['b'], engine = 'MergeTree')"); |
There was a problem hiding this comment.
Please use TestTable instead. Other places are also the same.
| assertUpdate("CREATE TABLE " + tableName + " (a bigint, b double NOT NULL, c varchar(50)) WITH (order_by=ARRAY['b'], engine = 'MergeTree')"); | ||
| assertUpdate("ALTER TABLE " + tableName + " ALTER COLUMN a SET DATA TYPE varchar(50)"); | ||
| assertEquals(getColumnType(tableName, "a"), "varchar"); | ||
| assertThat((String) computeScalar("show create table " + tableName)).contains("CREATE TABLE " + "clickhouse.tpch." + tableName + " (\n" + |
There was a problem hiding this comment.
I don't understand why this assertion which doesn't exist in the base test method was added. Please extract a test method.
| assertUpdate("CREATE TABLE " + tableName + " (col bigint, col2 int not null) WITH (order_by=ARRAY['col2'], engine = 'MergeTree')"); | ||
| assertUpdate("ALTER TABLE " + tableName + " ALTER COLUMN col SET DATA TYPE integer"); |
There was a problem hiding this comment.
How ClickHouse handle values which can't cast to integer? e.g. test string
Probably, we shouldn't allow changing the type from varchar to numeric types in the connector.
...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
Outdated
Show resolved
Hide resolved
...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
Outdated
Show resolved
Hide resolved
...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| protected Optional<SetColumnTypeSetup> filterSetColumnTypesDataProvider(SetColumnTypeSetup setup) | ||
| { | ||
| if (setup.sourceColumnType().equals("tinyint")) { |
There was a problem hiding this comment.
Please use switch and leave the reason for those change to each block.
...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
Outdated
Show resolved
Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
Outdated
Show resolved
Hide resolved
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
👋 @subkanthi @ebyhr - this PR has become inactive. We hope you are still interested in working on it. Please let us know, and we can try to get reviewers to help with that. We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks. |
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
Added support for setting column type in clickhouse
Relates to #15515
Release notes
(x) Release notes are required, with the following suggested text: