Merge pull request ClickHouse#78637 from Algunenano/fix_attach

Remove query settings during attach

Author: Algunenano

src/Databases/DatabaseOnDisk.cpp

@@ -18,6 +18,7 @@
 #include <Interpreters/Context.h>
 #include <Interpreters/DatabaseCatalog.h>
 #include <Interpreters/InterpreterCreateQuery.h>
+#include <Interpreters/InterpreterSetQuery.h>
 #include <Parsers/ASTCreateQuery.h>
 #include <Parsers/ASTFunction.h>
 #include <Parsers/ParserCreateQuery.h>
@@ -131,18 +132,16 @@ std::pair<String, StoragePtr> createTableFromAST(
         }
     }
 
-    return
-    {
-        ast_create_query.getTable(),
-        StorageFactory::instance().get(
-            ast_create_query,
-            table_data_path_relative,
-            context,
-            context->getGlobalContext(),
-            columns,
-            constraints,
-            mode)
-    };
+    /// Before 24.10 it was possible for query settings to be stored with the .sql definition with some engines, which would ignore them
+    /// Later (breaking) changes to table storages made the engines throw, which now prevents attaching old definitions which include
+    /// those query settings
+    /// In order to ignore them now we call `applySettingsFromQuery` which will move the settings from engine to query level
+    auto ast = std::make_shared<ASTCreateQuery>(std::move(ast_create_query));
+    InterpreterSetQuery::applySettingsFromQuery(ast, context);
+
+    return {
+        ast->getTable(),
+        StorageFactory::instance().get(*ast, table_data_path_relative, context, context->getGlobalContext(), columns, constraints, mode)};
 }
 
 

tests/integration/test_session_settings_table/__init__.py

@@ -0,0 +1,2 @@
+a,b
+1,2

tests/integration/test_session_settings_table/data/clickhouse/part1.csv

@@ -0,0 +1,71 @@
+import logging
+import os
+
+import pytest
+
+from helpers.cluster import ClickHouseCluster
+from helpers.config_cluster import minio_secret_key
+from helpers.mock_servers import start_mock_servers
+from helpers.test_tools import TSV
+
+logging.getLogger().setLevel(logging.INFO)
+logging.getLogger().addHandler(logging.StreamHandler())
+
+SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__))
+S3_DATA = [
+    "data/clickhouse/part1.csv",
+]
+
+
+def create_buckets_s3(cluster):
+    minio = cluster.minio_client
+
+    for file in S3_DATA:
+        minio.fput_object(
+            bucket_name=cluster.minio_bucket,
+            object_name=file,
+            file_path=os.path.join(SCRIPT_DIR, file),
+        )
+    for obj in minio.list_objects(cluster.minio_bucket, recursive=True):
+        print(obj.object_name)
+
+
+@pytest.fixture(scope="module")
+def started_cluster():
+    try:
+        cluster = ClickHouseCluster(__file__)
+        # Until 24.10, query level settings were specified in the .sql file
+        cluster.add_instance(
+            "old_node",
+            image="clickhouse/clickhouse-server",
+            tag="24.9.2.42",
+            with_installed_binary=True,
+            with_minio=True,
+            stay_alive=True,
+        )
+
+        logging.info("Starting cluster...")
+        cluster.start()
+        logging.info("Cluster started")
+
+        create_buckets_s3(cluster)
+
+        yield cluster
+    finally:
+        cluster.shutdown()
+
+
+def test_upgrade_with_query_setting_in_create(started_cluster):
+    node = started_cluster.instances["old_node"]
+    node.query(
+        f"""CREATE TABLE b Engine = S3('http://minio1:9001/root/data/clickhouse/part1.csv', 'minio', '{minio_secret_key}') SETTINGS s3_create_new_file_on_insert = 1;"""
+    )
+
+    try:
+        show_query = "SHOW CREATE TABLE b"
+        node.query(show_query)
+
+        node.restart_with_latest_version()
+        node.query(show_query)
+    finally:
+        node.query("DROP TABLE b")

tests/integration/test_session_settings_table/test.py

@@ -1102,6 +1102,36 @@ def execute_spark_query(query: str):
         ],
     )
 
+    # Do a single check to verify that restarting CH maintains the setting (ATTACH)
+    # We are just interested on the setting working after restart, so no need to run it on all combinations
+    if format_version == "1" and storage_type == "s3" and not is_table_function:
+
+        instance.restart_clickhouse()
+
+        execute_spark_query(
+            f"""
+                ALTER TABLE {TABLE_NAME} RENAME COLUMN e TO z;
+            """
+        )
+
+        check_schema_and_data(
+            instance,
+            table_select_expression,
+            [
+                ["b", "Nullable(Float64)"],
+                ["f", "Nullable(Int64)"],
+                ["c", "Decimal(12, 2)"],
+                ["z", "Nullable(String)"],
+            ],
+            [
+                ["3", "-30", "7.12", "AAA"],
+                ["3", "12", "-9.13", "BBB"],
+                ["3.4", "\\N", "-9.13", "\\N"],
+                ["5", "7", "18.1", "\\N"],
+                ["\\N", "4", "7.12", "\\N"],
+            ],
+        )
+
 
 @pytest.mark.parametrize("format_version", ["1", "2"])
 @pytest.mark.parametrize("storage_type", ["s3", "azure", "local"])