Skip to content
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

Add support for setting column type in Clickhouse. #16702

Closed
wants to merge 30 commits into from
Closed
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
b629cdb
Add support for setting column type in Clickhouse.
subkanthi Mar 24, 2023
272419c
Added test to validate MODIFY COLUMN DATA TYPE does not affect modifier.
subkanthi Mar 25, 2023
94a643c
Fixed wildcard imports.
subkanthi Mar 25, 2023
170546a
Removed flag to disble column data type
subkanthi Mar 29, 2023
0d6e541
Reverted back changes to BaseConnectorTest
subkanthi Mar 29, 2023
c36a5cb
Reverted back changes to BaseConnectorTest
subkanthi Mar 29, 2023
21c325d
Override tests in ClickHouseConnector for MODIFY DATA TYPE
subkanthi Apr 8, 2023
e7dd652
Fix Clickhouse MODIFY DATA TYPE tests
subkanthi Apr 8, 2023
a6d518b
Fix Clickhouse MODIFY DATA TYPE tests
subkanthi Apr 8, 2023
98cccdc
Merged from master
subkanthi Apr 8, 2023
b44596a
Fixed missing imports
subkanthi Apr 8, 2023
10300f3
Fix checkstyle errors
subkanthi Apr 9, 2023
967601a
Fix checkstyle imports.
subkanthi Apr 9, 2023
a5e6bcf
Fix order of imports.
subkanthi Apr 9, 2023
bd81ef7
Fix order of imports.
subkanthi Apr 13, 2023
12f5656
Override testSetColumnTypes function to test alter modify column for …
subkanthi Apr 25, 2023
367526f
Pulled in latest changes and added override tests for ClickHouse Conn…
subkanthi Apr 25, 2023
f29f6c3
Fixed checkstyle errors.
subkanthi Apr 26, 2023
d0682ea
Reverted back commented out tests.
subkanthi Apr 27, 2023
fd18ba2
Reverted back changes.
subkanthi Apr 27, 2023
42ac5a6
Add randomNameSuffix to table name
subkanthi Apr 29, 2023
31ce7ad
Merge branch 'master' of github.com:subkanthi/trino into clickhouse-s…
subkanthi Apr 29, 2023
9c18655
ClickHouseConnectorTest - Move logic from setColumnTypesDataProvider …
subkanthi Apr 29, 2023
9033875
Fix checkstyle errors
subkanthi May 1, 2023
3fe83b4
Changed ClickHouseConnectorTest for data types to switch/case
subkanthi May 28, 2023
d7cce90
Reverted unrelated change
subkanthi May 28, 2023
5cdb1cb
Fixed checkstyle errors.
subkanthi May 28, 2023
907f738
Make getColumnType protected so it can be accessed.
subkanthi May 28, 2023
cdb2b35
Fix fallthrough in switch/case of TestClickHouseConnectorTest
subkanthi May 28, 2023
a0aa993
Fix checkstyle violations in TestClickHouseConnectorTest
subkanthi May 28, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,12 @@ public void setColumnComment(ConnectorSession session, JdbcTableHandle handle, J
@Override
public void setColumnType(ConnectorSession session, JdbcTableHandle handle, JdbcColumnHandle column, Type type)
{
throw new TrinoException(NOT_SUPPORTED, "This connector does not support setting column types");
String sql = format(
"ALTER TABLE %s MODIFY COLUMN %s %s",
ebyhr marked this conversation as resolved.
Show resolved Hide resolved
quoted(handle.asPlainTable().getRemoteTableName()),
quoted(column.getColumnName()),
toWriteMapping(session, type).getDataType());
execute(session, sql);
ebyhr marked this conversation as resolved.
Show resolved Hide resolved
}

private static String clickhouseVarcharLiteral(String value)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@
import static io.trino.plugin.jdbc.JdbcMetadataSessionProperties.DOMAIN_COMPACTION_THRESHOLD;
import static io.trino.spi.type.VarcharType.VARCHAR;
import static io.trino.testing.MaterializedResult.resultBuilder;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_CREATE_TABLE_WITH_COLUMN_COMMENT;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_CREATE_TABLE_WITH_DATA;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_INSERT;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_NOT_NULL_CONSTRAINT;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_SET_COLUMN_TYPE;
import static io.trino.testing.TestingNames.randomNameSuffix;
import static java.lang.String.format;
import static org.assertj.core.api.Assertions.assertThat;
Expand Down Expand Up @@ -82,9 +87,6 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
case SUPPORTS_AGGREGATION_PUSHDOWN_COUNT_DISTINCT:
return false;

case SUPPORTS_SET_COLUMN_TYPE:
return false;

case SUPPORTS_ARRAY:
case SUPPORTS_ROW_TYPE:
case SUPPORTS_NEGATIVE_DATE:
Expand Down Expand Up @@ -325,7 +327,7 @@ protected TestTable createTableWithDefaultColumns()
"col_nullable Nullable(Int64)," +
"col_default Nullable(Int64) DEFAULT 43," +
"col_nonnull_default Int64 DEFAULT 42," +
"col_required2 Int64) ENGINE=Log");
"col_required2 Int64) ENGINE=MergeTree() ORDER BY col_required2");
}

@Override
Expand Down Expand Up @@ -817,4 +819,168 @@ private Map<String, String> getTableProperties(String schemaName, String tableNa
return properties.buildOrThrow();
}
}

@Test
@Override
public void testSetColumnType()
Copy link
Member

Choose a reason for hiding this comment

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

Please leave code comments why these test are overridden.

Copy link
Member

Choose a reason for hiding this comment

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

Reminder.

{
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')");
Copy link
Member

Choose a reason for hiding this comment

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

Please use TestTable instead. Other places are also the same.

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" +
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this assertion which doesn't exist in the base test method was added. Please extract a test method.

" a varchar NOT NULL,\n" +
" b double NOT NULL,\n" +
" c varchar\n" +
")\n" +
"WITH (\n" +
" engine = 'MERGETREE',\n" +
" order_by = ARRAY['b'],\n" +
" primary_key = ARRAY['b']\n" +
")");
}

@Test
@Override
public void testSetColumnIncompatibleType()
{
skipTestUnless(hasBehavior(SUPPORTS_SET_COLUMN_TYPE) && hasBehavior(SUPPORTS_CREATE_TABLE_WITH_DATA));

String tableName = "test_set_column_incompatible_type" + randomNameSuffix();

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");
Comment on lines +849 to +850
Copy link
Member

Choose a reason for hiding this comment

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

The base test changes varchar type to integer. This overridden test is different from the base test purpose.

Copy link
Member

Choose a reason for hiding this comment

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

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.

assertEquals(getColumnType(tableName, "col"), "integer");
}

@Test
@Override
public void testSetColumnOutOfRangeType()
{
skipTestUnless(hasBehavior(SUPPORTS_SET_COLUMN_TYPE) && hasBehavior(SUPPORTS_CREATE_TABLE_WITH_DATA));
subkanthi marked this conversation as resolved.
Show resolved Hide resolved

String tableName = "test_set_column_out_of_range" + randomNameSuffix();
assertUpdate("CREATE TABLE " + tableName + " (col bigint, col2 int not null) WITH (order_by=ARRAY['col2'], engine = 'MergeTree')");
query("insert into " + tableName + " values(9223372036854775807, 22)");
assertUpdate("ALTER TABLE " + tableName + " ALTER COLUMN col SET DATA TYPE integer");
assertEquals(getColumnType(tableName, "col"), "integer");
assertQuery("SELECT col FROM " + tableName, "VALUES -1");
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a bug. We may need to deny such operations. Could you find a GitHub issue of ClickHouse about this?

Copy link
Member Author

@subkanthi subkanthi Apr 29, 2023

Choose a reason for hiding this comment

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

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 │
└────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘


Copy link
Member

Choose a reason for hiding this comment

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

Let's deny changing such types.

}

@Test
@Override
public void testSetColumnTypeWithComment()
{
skipTestUnless(hasBehavior(SUPPORTS_SET_COLUMN_TYPE) && hasBehavior(SUPPORTS_CREATE_TABLE_WITH_COLUMN_COMMENT));
Copy link
Member

Choose a reason for hiding this comment

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

Remove.

String tableName = "test_set_column_with_comment" + randomNameSuffix();

assertUpdate("CREATE TABLE " + tableName + " (col bigint COMMENT 'test comment', col2 int not null) WITH (order_by=ARRAY['col2'], engine = 'MergeTree')");
assertEquals(getColumnComment(tableName, "col"), "test comment");

assertUpdate("ALTER TABLE " + tableName + " ALTER COLUMN col SET DATA TYPE bigint");
assertEquals(getColumnComment(tableName, "col"), "test comment");
}

@Test
@Override
public void testSetColumnTypeWithDefaultColumn()
{
skipTestUnless(hasBehavior(SUPPORTS_SET_COLUMN_TYPE) && hasBehavior(SUPPORTS_INSERT));
subkanthi marked this conversation as resolved.
Show resolved Hide resolved
try (TestTable table = createTableWithDefaultColumns()) {
// col_default column inserts 43 by default
assertUpdate("ALTER TABLE " + table.getName() + " ALTER COLUMN col_default SET DATA TYPE bigint");
assertUpdate("INSERT INTO " + table.getName() + " (col_required, col_required2) VALUES (1, 10)", 1);
assertQuery("SELECT col_default FROM " + table.getName(), "VALUES 43");
}
}

@Test
@Override
public void testSetColumnTypeWithNotNull()
{
skipTestUnless(hasBehavior(SUPPORTS_SET_COLUMN_TYPE) && hasBehavior(SUPPORTS_NOT_NULL_CONSTRAINT));
subkanthi marked this conversation as resolved.
Show resolved Hide resolved

String tableName = "test_set_column_with_not_null" + randomNameSuffix();

assertUpdate("CREATE TABLE " + tableName + " (col bigint not null, col2 int not null) WITH (order_by=ARRAY['col2'], engine = 'MergeTree')");
assertFalse(columnIsNullable(tableName, "col"));

assertUpdate("ALTER TABLE " + tableName + " ALTER COLUMN col SET DATA TYPE bigint");
assertFalse(columnIsNullable(tableName, "col"));
}

@Override
protected Optional<SetColumnTypeSetup> filterSetColumnTypesDataProvider(SetColumnTypeSetup setup)
{
if (setup.sourceColumnType().equals("tinyint")) {
Copy link
Member

Choose a reason for hiding this comment

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

Please use switch and leave the reason for those change to each block.

return Optional.of(new SetColumnTypeSetup("tinyint", "TINYINT '127'", "smallint"));
}
else if (setup.sourceColumnType().equals("smallint")) {
return Optional.of(new SetColumnTypeSetup("smallint", "SMALLINT '32767'", "integer"));
}
else if (setup.sourceColumnType().equals("integer")) {
return Optional.of(new SetColumnTypeSetup("integer", "2147483647", "bigint"));
}
else if (setup.sourceColumnType().equals("bigint")) {
return Optional.of(new SetColumnTypeSetup("bigint", "BIGINT '-2147483648'", "integer"));
}
else if (setup.sourceColumnType().equals("real")) {
return Optional.of(new SetColumnTypeSetup("real", "REAL '10.3'", "double"));
}
else if (setup.sourceColumnType().equals("real")) {
return Optional.of(new SetColumnTypeSetup("real", "REAL 'NaN'", "double"));
}
else if (setup.sourceColumnType().equals("decimal(5,3)")) {
return Optional.of(new SetColumnTypeSetup("decimal(5,3)", "12.345", "decimal(10,3)"));
}
else if (setup.sourceColumnType().equals("decimal(28,3)")) {
return Optional.of(new SetColumnTypeSetup("decimal(28,3)", "12.345", "decimal(38,3)"));
}
else if (setup.sourceColumnType().equals("decimal(5,3)")) {
return Optional.of(new SetColumnTypeSetup("decimal(5,3)", "12.345", "decimal(38,3)"));
}
else if (setup.sourceColumnType().equals("decimal(5,3)")) {
return Optional.of(new SetColumnTypeSetup("decimal(5,3)", "12.340", "decimal(5,2)"));
}
else if (setup.sourceColumnType().equals("decimal(5,3)")) {
return Optional.of(new SetColumnTypeSetup("decimal(5,3)", "12.35", "decimal(5,2)"));
}
else if (setup.sourceColumnType().equals("varchar(100)")) {
return Optional.of(new SetColumnTypeSetup("varchar(100)", "'shorten-varchar'", "varchar"));
}
else if (setup.sourceColumnType().equals("char(25)")) {
return Optional.of(new SetColumnTypeSetup("char(25)", "'shorten-char'", "varchar"));
}
else if (setup.sourceColumnType().equals("char(20)")) {
return Optional.of(new SetColumnTypeSetup("char(20)", "'char-to-varchar'", "varchar"));
}
else if (setup.sourceColumnType().equals("varchar")) {
return Optional.of(new SetColumnTypeSetup("varchar", "'varchar-to-char'", "varchar"));
}

return Optional.empty();
}

@Test(dataProvider = "setColumnTypesDataProvider")
@Override
public void testSetColumnTypes(SetColumnTypeSetup setup)
{
skipTestUnless(hasBehavior(SUPPORTS_SET_COLUMN_TYPE) && hasBehavior(SUPPORTS_CREATE_TABLE_WITH_DATA));
subkanthi marked this conversation as resolved.
Show resolved Hide resolved

String tableName = "test_set_column_type_" + System.currentTimeMillis();
assertUpdate("CREATE TABLE " + tableName + "(col " + setup.sourceColumnType() + ", col2 int not null) WITH (engine='mergetree', order_by=ARRAY['col2'])");
query("insert into " + tableName + "(col, col2) values(CAST(" + setup.sourceValueLiteral() + " AS " + setup.sourceColumnType() + "), 2)");
Runnable setColumnType = () -> assertUpdate("ALTER TABLE " + tableName + " ALTER COLUMN col SET DATA TYPE " + setup.newColumnType());
if (setup.unsupportedType()) {
assertThatThrownBy(setColumnType::run)
.satisfies(this::verifySetColumnTypeFailurePermissible);
return;
}
setColumnType.run();

assertEquals(getColumnType(tableName, "col"), setup.newColumnType());
assertThat(query("SELECT col FROM " + tableName))
.skippingTypesCheck()
.matches("SELECT " + setup.newValueLiteral());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2890,7 +2890,7 @@ protected void verifySetColumnTypeFailurePermissible(Throwable e)
throw new AssertionError("Unexpected set column type failure", e);
}

private String getColumnType(String tableName, String columnName)
protected String getColumnType(String tableName, String columnName)
{
return (String) computeScalar(format("SELECT data_type FROM information_schema.columns WHERE table_schema = CURRENT_SCHEMA AND table_name = '%s' AND column_name = '%s'",
tableName,
Expand Down Expand Up @@ -5864,7 +5864,7 @@ public void testMergeAllColumnsReversed()
assertUpdate("DROP TABLE " + targetTable);
}

private void verifyUnsupportedTypeException(Throwable exception, String trinoTypeName)
protected void verifyUnsupportedTypeException(Throwable exception, String trinoTypeName)
subkanthi marked this conversation as resolved.
Show resolved Hide resolved
{
String typeNameBase = trinoTypeName.replaceFirst("\\(.*", "");
String expectedMessagePart = format("(%1$s.*not (yet )?supported)|((?i)unsupported.*%1$s)|((?i)not supported.*%1$s)", Pattern.quote(typeNameBase));
Expand Down