From d1d76dc8b5ae0b1843b02beb05c1f6bcacb301cb Mon Sep 17 00:00:00 2001 From: Kevin Michel Date: Fri, 29 Dec 2023 12:56:19 +0100 Subject: [PATCH] clickhouse: don't SET merge_tree_allow_nullable_key Setting it was unnecessary and did not help. The previous change https://github.com/Aiven-Open/astacus/pull/172 was motivated by an incorrect diagnosis of the problem: This flag is a merge tree setting and not a global setting, so it's part of the table definition and does not need to be set on the session. The real problem was https://github.com/ClickHouse/ClickHouse/issues/54814 which was introduced in ClickHouse 23.7 and fixed in ClickHouse >23.9 and is specific to projections inside a table: https://github.com/ClickHouse/ClickHouse/pull/52361/commits/781674bdc5b7920c4881614ceb6aa011357e53b7 https://github.com/ClickHouse/ClickHouse/pull/54895/commits/973cd5e972192a2139c20191bf26c75c8a03183f Another problem is that checkProperties (or checkKeyExpression) is not called when doing: `ALTER TABLE ... MODIFY SETTING allow_nullable_key=false` on a table that relied on a nullable key, but that requires an upstream fix. --- astacus/coordinator/plugins/clickhouse/steps.py | 1 - tests/integration/coordinator/plugins/clickhouse/test_plugin.py | 1 + tests/unit/coordinator/plugins/clickhouse/test_steps.py | 2 -- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/astacus/coordinator/plugins/clickhouse/steps.py b/astacus/coordinator/plugins/clickhouse/steps.py index 0d2d414e..8a233b71 100644 --- a/astacus/coordinator/plugins/clickhouse/steps.py +++ b/astacus/coordinator/plugins/clickhouse/steps.py @@ -516,7 +516,6 @@ def _create_dbs(client: ClickHouseClient) -> Iterator[Awaitable[None]]: b"SET allow_experimental_object_type=true", b"SET allow_suspicious_codecs=true", b"SET allow_suspicious_low_cardinality_types=true", - b"SET merge_tree_allow_nullable_key=true", # If a table was created with flatten_nested=0, we must be careful to not re-create the # table with flatten_nested=1, since this would recreate the table with a different schema. # If a table was created with flatten_nested=1, the query in system.tables.create_table_query diff --git a/tests/integration/coordinator/plugins/clickhouse/test_plugin.py b/tests/integration/coordinator/plugins/clickhouse/test_plugin.py index 6f8d4bce..a154cfb1 100644 --- a/tests/integration/coordinator/plugins/clickhouse/test_plugin.py +++ b/tests/integration/coordinator/plugins/clickhouse/test_plugin.py @@ -147,6 +147,7 @@ async def setup_cluster_content(clients: List[HttpClickHouseClient], use_named_c b"SETTINGS allow_experimental_object_type=1" ) # test that we can re-create a table requiring custom merge tree settings. + # Unlike other cases, we don't have to SET a global settings because it's merge tree setting. await clients[0].execute( b"CREATE TABLE default.with_nullable_key (thekey Nullable(UInt32), thedata String) " b"ENGINE = ReplicatedMergeTree ORDER BY (thekey) " diff --git a/tests/unit/coordinator/plugins/clickhouse/test_steps.py b/tests/unit/coordinator/plugins/clickhouse/test_steps.py index 3de26df7..765319c0 100644 --- a/tests/unit/coordinator/plugins/clickhouse/test_steps.py +++ b/tests/unit/coordinator/plugins/clickhouse/test_steps.py @@ -788,7 +788,6 @@ async def test_creates_all_replicated_databases_and_tables_in_manifest() -> None b"SET allow_experimental_object_type=true", b"SET allow_suspicious_codecs=true", b"SET allow_suspicious_low_cardinality_types=true", - b"SET merge_tree_allow_nullable_key=true", b"SET flatten_nested=0", b"CREATE TABLE db-one.table-uno ...", b"CREATE TABLE db-one.table-dos ...", @@ -847,7 +846,6 @@ async def test_creates_all_replicated_databases_and_tables_in_manifest_with_cust b"SET allow_experimental_object_type=true", b"SET allow_suspicious_codecs=true", b"SET allow_suspicious_low_cardinality_types=true", - b"SET merge_tree_allow_nullable_key=true", b"SET flatten_nested=0", b"CREATE TABLE db-one.table-uno ...", b"CREATE TABLE db-one.table-dos ...",