From 1972dbc742bde07d6d028a535ce403c7a9bce1d2 Mon Sep 17 00:00:00 2001 From: Andrey Zvonov <32552679+zvonand@users.noreply.github.com> Date: Mon, 14 Sep 2026 17:18:31 +0200 Subject: [PATCH 1/2] Cherry-pick of https://github.com/Altinity/ClickHouse/pull/2272 with unresolved conflict markers (resolution in next commit) --- Original cherry-pick message follows: Merge pull request #2272 from Altinity/2085-glue-datalakecatalog-uses-stale-schema-after-alter-add-column Antalya 26.6 - Fixed refreshing of glue metadata after ALTER calls # Conflicts: # src/Databases/DataLake/GlueCatalog.cpp # tests/integration/test_database_glue/test.py --- src/Databases/DataLake/GlueCatalog.cpp | 201 ++++++++++++++++--- src/Databases/DataLake/GlueCatalog.h | 13 ++ tests/integration/test_database_glue/test.py | 50 +++++ 3 files changed, 232 insertions(+), 32 deletions(-) diff --git a/src/Databases/DataLake/GlueCatalog.cpp b/src/Databases/DataLake/GlueCatalog.cpp index 060d8fdd5742..8e35a2173736 100644 --- a/src/Databases/DataLake/GlueCatalog.cpp +++ b/src/Databases/DataLake/GlueCatalog.cpp @@ -51,6 +51,7 @@ #include #include #include +#include #include #include #include @@ -73,6 +74,7 @@ namespace DB::FailPoints namespace DB::Setting { + extern const SettingsBool allow_experimental_geo_types_in_iceberg; extern const SettingsUInt64 s3_max_connections; extern const SettingsUInt64 s3_max_redirects; extern const SettingsUInt64 s3_retry_attempts; @@ -112,6 +114,87 @@ namespace CurrentMetrics extern const Metric MarkCacheFiles; } +namespace +{ + +/// Convert an Iceberg JSON type (string or object) to the Glue type string. +String icebergTypeToGlueType(const Poco::Dynamic::Var & type_value) +{ + if (type_value.isString()) + { + String type = type_value.toString(); + if (type == "timestamptz") + return "timestamp"; + if (type == "timestamp_ns" || type == "timestamptz_ns") + return "timestamp_nano"; + return type; + } + + auto obj = type_value.extract(); + String complex_type = obj->getValue(DB::Iceberg::f_type); + + if (complex_type == DB::Iceberg::f_list) + { + return "array<" + icebergTypeToGlueType(obj->get(DB::Iceberg::f_element)) + ">"; + } + if (complex_type == DB::Iceberg::f_map) + { + return "map<" + icebergTypeToGlueType(obj->get(DB::Iceberg::f_key)) + ", " + + icebergTypeToGlueType(obj->get(DB::Iceberg::f_value)) + ">"; + } + if (complex_type == DB::Iceberg::f_struct) + { + auto fields = obj->getArray(DB::Iceberg::f_fields); + String result = "struct<"; + for (UInt32 i = 0; i < fields->size(); ++i) + { + if (i > 0) + result += ", "; + auto field = fields->getObject(i); + result += field->getValue(DB::Iceberg::f_name) + ":" + icebergTypeToGlueType(field->get(DB::Iceberg::f_type)); + } + result += ">"; + return result; + } + + throw DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "Unknown Iceberg complex type: {}", complex_type); +} + +/// Build Glue Column objects from an Iceberg schema JSON object +/// (which has "type": "struct", "fields": [...]). +std::vector icebergSchemaToGlueColumns(const Poco::JSON::Object::Ptr & schema) +{ + std::vector glue_columns; + auto fields = schema->getArray(DB::Iceberg::f_fields); + + for (UInt32 i = 0; i < fields->size(); ++i) + { + auto field = fields->getObject(i); + Aws::Glue::Model::Column col; + col.SetName(field->getValue(DB::Iceberg::f_name)); + col.SetType(icebergTypeToGlueType(field->get(DB::Iceberg::f_type))); + + Aws::Map params; + params["iceberg.field.id"] = std::to_string(field->getValue(DB::Iceberg::f_id)); + params["iceberg.field.optional"] = field->getValue(DB::Iceberg::f_required) ? "false" : "true"; + params["iceberg.field.current"] = "true"; + col.SetParameters(params); + + glue_columns.push_back(std::move(col)); + } + + return glue_columns; +} + +/// Extract the current schema object from full Iceberg metadata JSON. +Poco::JSON::Object::Ptr getCurrentSchemaFromMetadata(const Poco::JSON::Object::Ptr & metadata) +{ + auto [schema, _] = DB::Iceberg::parseTableSchemaV2Method(metadata); + return schema; +} + +} + namespace DataLake { @@ -372,7 +455,7 @@ bool GlueCatalog::existsTable(const std::string & database_name, const std::stri bool GlueCatalog::tryGetTableMetadata( const std::string & database_name, const std::string & table_name, - DB::ContextPtr /* context_ */, + DB::ContextPtr context_, TableMetadata & result) const { if (!isNamespaceAllowed(database_name)) @@ -454,26 +537,50 @@ bool GlueCatalog::tryGetTableMetadata( { DB::NamesAndTypesList schema; auto columns = table_outcome.GetStorageDescriptor().GetColumns(); - for (const auto & column : columns) + if (!columns.empty()) { - const auto column_params = column.GetParameters(); - bool can_be_nullable = column_params.contains("iceberg.field.optional") && column_params.at("iceberg.field.optional") == "true"; - - /// Skip field if it's not "current" (for example Renamed). No idea how someone can utilize "non current fields" but for some reason - /// they are returned by Glue API. So if you do "RENAME COLUMN a to new_a" glue will return two fields: a and new_a. - /// And a will be marked as "non current" field. - if (column_params.contains("iceberg.field.current") && column_params.at("iceberg.field.current") == "false") - continue; - - String column_type = column.GetType(); - if (column_type == "timestamp" || column_type == "timestamp_nano") + for (const auto & column : columns) { - if (!result.requiresDataLakeSpecificProperties()) - setup_specific_properties(); - column_type = getActualTimestampType(column.GetName(), result, column_type); + const auto column_params = column.GetParameters(); + bool can_be_nullable = column_params.contains("iceberg.field.optional") && column_params.at("iceberg.field.optional") == "true"; + + /// Skip field if it's not "current" (for example Renamed). No idea how someone can utilize "non current fields" but for some reason + /// they are returned by Glue API. So if you do "RENAME COLUMN a to new_a" glue will return two fields: a and new_a. + /// And a will be marked as "non current" field. + if (column_params.contains("iceberg.field.current") && column_params.at("iceberg.field.current") == "false") + continue; + + String column_type = column.GetType(); + if (column_type == "timestamp" || column_type == "timestamp_nano") + { + if (!result.requiresDataLakeSpecificProperties()) + setup_specific_properties(); + column_type = getActualTimestampType(column.GetName(), result, column_type); + } + + schema.push_back({column.GetName(), getType(column_type, can_be_nullable, getContext())}); + } + } + else + { + /// StorageDescriptor has no columns (e.g. table was created via ClickHouse DDL + /// which only sets metadata_location). Fall back to parsing the Iceberg metadata + /// file directly, same approach as the REST catalog. + if (!result.requiresDataLakeSpecificProperties()) + setup_specific_properties(); + + auto table_specific_properties = result.getDataLakeSpecificProperties(); + if (table_specific_properties.has_value() && !table_specific_properties->iceberg_metadata_file_location.empty()) + { + auto metadata_object = getOrFetchMetadataObject(table_specific_properties->iceberg_metadata_file_location, result); + const bool allow_geo_parser + = getContext()->getSettingsRef()[DB::Setting::allow_experimental_geo_types_in_iceberg].value; + auto schema_processor = DB::Iceberg::IcebergSchemaProcessor(context_, allow_geo_parser); + auto id = DB::IcebergMetadata::parseTableSchema(metadata_object, schema_processor, context_, log); + auto parsed_schema = schema_processor.getClickhouseTableSchemaById(id); + if (parsed_schema) + schema = *parsed_schema; } - - schema.push_back({column.GetName(), getType(column_type, can_be_nullable, getContext())}); } result.setSchema(schema); } @@ -556,24 +663,26 @@ bool GlueCatalog::empty() const return true; } -String GlueCatalog::getActualTimestampType(const String & column_name, const TableMetadata & table_metadata, const String & glue_column_type) const +Poco::JSON::Object::Ptr GlueCatalog::getOrFetchMetadataObject(const String & metadata_uri, const TableMetadata & table_metadata) const { - auto table_specific_properties = table_metadata.getDataLakeSpecificProperties(); - if (!table_specific_properties.has_value()) - throw DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "Failed to read table metadata, reason why table is unreadable: {}", table_metadata.getReasonWhyTableIsUnreadable()); - - const String & metadata_uri = table_specific_properties->iceberg_metadata_file_location; - - if (!metadata_objects.get(metadata_uri)) + auto [value, _] = metadata_objects.getOrSet(metadata_uri, [&]() { auto [object_storage, bucket_name, metadata_path] = createObjectStorageForEarlyTableAccess(metadata_uri, table_metadata); auto compression_method = DB::Iceberg::getCompressionMethodFromMetadataFile(metadata_uri); auto metadata_object = DB::Iceberg::getMetadataJSONObject( metadata_path, object_storage, nullptr, getContext(), log, compression_method, std::nullopt); - metadata_objects.set(metadata_uri, std::make_shared(metadata_object)); - } + return std::make_shared(metadata_object); + }); + return *value; +} + +String GlueCatalog::getActualTimestampType(const String & column_name, const TableMetadata & table_metadata, const String & glue_column_type) const +{ + auto table_specific_properties = table_metadata.getDataLakeSpecificProperties(); + if (!table_specific_properties.has_value()) + throw DB::Exception(DB::ErrorCodes::BAD_ARGUMENTS, "Failed to read table metadata, reason why table is unreadable: {}", table_metadata.getReasonWhyTableIsUnreadable()); - auto metadata_object = *metadata_objects.get(metadata_uri); + auto metadata_object = getOrFetchMetadataObject(table_specific_properties->iceberg_metadata_file_location, table_metadata); return resolveTimestampTypeFromMetadata(metadata_object, column_name, glue_column_type); } @@ -696,7 +805,7 @@ void GlueCatalog::createNamespaceIfNotExists(const String & namespace_name, cons } } -void GlueCatalog::createTable(const String & namespace_name, const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr /*metadata_content*/) const +void GlueCatalog::createTable(const String & namespace_name, const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr metadata_content) const { if (!isNamespaceAllowed(namespace_name)) throw DB::Exception(DB::ErrorCodes::CATALOG_NAMESPACE_DISABLED, @@ -717,6 +826,12 @@ void GlueCatalog::createTable(const String & namespace_name, const String & tabl sd.SetLocation(grandparent.c_str()); + if (metadata_content) + { + auto schema = getCurrentSchemaFromMetadata(metadata_content); + sd.SetColumns(icebergSchemaToGlueColumns(schema)); + } + table_input.SetStorageDescriptor(sd); table_input.SetTableType("ICEBERG"); @@ -740,7 +855,11 @@ void GlueCatalog::createTable(const String & namespace_name, const String & tabl throw DB::Exception(DB::ErrorCodes::DATALAKE_DATABASE_ERROR, "Can not create metadata in glue catalog: {}", response.GetError().GetMessage()); } -bool GlueCatalog::updateMetadata(const String & namespace_name, const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr /*new_snapshot*/) const +bool GlueCatalog::updateTableInGlue( + const String & namespace_name, + const String & table_name, + const String & new_metadata_path, + const std::vector & columns) const { Aws::Glue::Model::UpdateTableRequest request; request.SetDatabaseName(namespace_name); @@ -758,6 +877,9 @@ bool GlueCatalog::updateMetadata(const String & namespace_name, const String & t /// We should drop `metadata/v-metadata.json` suffix to get location. sd.SetLocation(grandparent.c_str()); + if (!columns.empty()) + sd.SetColumns(columns); + table_input.SetStorageDescriptor(sd); table_input.SetTableType("ICEBERG"); @@ -783,14 +905,29 @@ bool GlueCatalog::updateMetadata(const String & namespace_name, const String & t return true; } +bool GlueCatalog::updateMetadata(const String & namespace_name, const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr /*new_snapshot*/) const +{ + return updateTableInGlue(namespace_name, table_name, new_metadata_path); +} + bool GlueCatalog::updateSchema( const String & namespace_name, const String & table_name, const String & new_metadata_path, +<<<<<<< HEAD Poco::JSON::Object::Ptr /*new_schema*/, Int32 /*previous_schema_id*/) const +======= + Poco::JSON::Object::Ptr new_schema, + Int32 /*previous_schema_id*/, + Int32 /*new_last_column_id*/, + Poco::JSON::Object::Ptr /*metadata*/) const +>>>>>>> 81817b6f1c3 (Merge pull request #2272 from Altinity/2085-glue-datalakecatalog-uses-stale-schema-after-alter-add-column) { - return updateMetadata(namespace_name, table_name, new_metadata_path, nullptr); + std::vector columns; + if (new_schema) + columns = icebergSchemaToGlueColumns(new_schema); + return updateTableInGlue(namespace_name, table_name, new_metadata_path, columns); } void GlueCatalog::dropTable(const String & namespace_name, const String & table_name, bool /*delete_data*/) const diff --git a/src/Databases/DataLake/GlueCatalog.h b/src/Databases/DataLake/GlueCatalog.h index 3277e1a5f1cb..0ccea87a0cfe 100644 --- a/src/Databases/DataLake/GlueCatalog.h +++ b/src/Databases/DataLake/GlueCatalog.h @@ -4,6 +4,7 @@ #if USE_AWS_S3 && USE_AVRO #include +#include #include #include #include @@ -130,6 +131,18 @@ class GlueCatalog final : public ICatalog, private DB::WithContext ObjectStorageWithPath createObjectStorageForEarlyTableAccess(const String & s3_location, const TableMetadata & table_metadata) const; + /// Fetches and caches the parsed Iceberg metadata JSON for `metadata_uri`. + /// Returns the cached object on subsequent calls for the same URI. + Poco::JSON::Object::Ptr getOrFetchMetadataObject(const String & metadata_uri, const TableMetadata & table_metadata) const; + + /// Shared implementation for updateMetadata / updateSchema that optionally + /// sets StorageDescriptor columns in the Glue UpdateTable call. + bool updateTableInGlue( + const String & namespace_name, + const String & table_name, + const String & new_metadata_path, + const std::vector & columns = {}) const; + mutable DB::CacheBase metadata_objects; }; diff --git a/tests/integration/test_database_glue/test.py b/tests/integration/test_database_glue/test.py index e0d32c159cda..49f37c457525 100644 --- a/tests/integration/test_database_glue/test.py +++ b/tests/integration/test_database_glue/test.py @@ -844,6 +844,7 @@ def test_create(started_cluster): assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") == "AAPL\n" +<<<<<<< HEAD def test_create_gzip_metadata(started_cluster): # Regression for issue #109801: a catalog-backed CREATE TABLE from ClickHouse # with gzip metadata compression exercises IcebergMetadata::createInitial and @@ -895,6 +896,55 @@ def test_create_gzip_metadata(started_cluster): # Reopen through the catalog (fresh database) and confirm read still works. create_clickhouse_glue_database(started_cluster, node, CATALOG_NAME) assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") == "AAPL\n" +======= +def test_schema_evolution_show_create_and_drop(started_cluster): + """SHOW CREATE TABLE must reflect columns added/dropped via ALTER. + + Reproducer for the bug where GlueCatalog::updateSchema only updated + metadata_location but not StorageDescriptor.Columns, causing + SHOW CREATE TABLE to return a stale schema and DROP COLUMN to fail + with NOT_FOUND_COLUMN_IN_BLOCK. + """ + node = started_cluster.instances["node1"] + + test_ref = f"test_show_create_drop_{uuid.uuid4()}" + table_name = f"{test_ref}_table" + root_namespace = f"{test_ref}_namespace" + table_ref = f"{CATALOG_NAME}.`{root_namespace}.{table_name}`" + write_settings = {"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1} + + create_clickhouse_glue_database(started_cluster, node, CATALOG_NAME) + create_clickhouse_glue_table(started_cluster, node, root_namespace, table_name, "(name Nullable(String))") + + node.query(f"INSERT INTO {table_ref} VALUES ('Alice');", settings=write_settings) + + node.query(f"ALTER TABLE {table_ref} ADD COLUMN column_a Nullable(String);", settings=write_settings) + node.query(f"ALTER TABLE {table_ref} ADD COLUMN column_b Nullable(Int64);", settings=write_settings) + + assert node.query(f"SELECT * FROM {table_ref}") == "Alice\t\\N\t\\N\n" + + show_create = node.query(f"SHOW CREATE TABLE {table_ref}") + assert "column_a" in show_create, f"column_a missing from SHOW CREATE:\n{show_create}" + assert "column_b" in show_create, f"column_b missing from SHOW CREATE:\n{show_create}" + + node.query(f"ALTER TABLE {table_ref} DROP COLUMN column_a;", settings=write_settings) + + show_create = node.query(f"SHOW CREATE TABLE {table_ref}") + assert "column_a" not in show_create, f"column_a still in SHOW CREATE after DROP:\n{show_create}" + assert "column_b" in show_create, f"column_b missing from SHOW CREATE after DROP:\n{show_create}" + + assert node.query(f"SELECT name, column_b FROM {table_ref}") == "Alice\t\\N\n" + + node.query(f"ALTER TABLE {table_ref} ADD COLUMN column_c Nullable(String);", settings=write_settings) + node.query(f"INSERT INTO {table_ref} (name, column_b, column_c) VALUES ('Bob', 42, 'hello');", settings=write_settings) + + show_create = node.query(f"SHOW CREATE TABLE {table_ref}") + assert "column_c" in show_create, f"column_c missing from SHOW CREATE:\n{show_create}" + assert "column_a" not in show_create, f"column_a reappeared in SHOW CREATE:\n{show_create}" + + result = node.query(f"SELECT name, column_b, column_c FROM {table_ref} ORDER BY name") + assert result == "Alice\t\\N\t\\N\nBob\t42\thello\n" +>>>>>>> 81817b6f1c3 (Merge pull request #2272 from Altinity/2085-glue-datalakecatalog-uses-stale-schema-after-alter-add-column) def test_schema_evolution(started_cluster): From bef921e08526e1cc3d0b75483fb9f7d7738e346f Mon Sep 17 00:00:00 2001 From: Andrey Zvonov <32552679+zvonand@users.noreply.github.com> Date: Thu, 1 Oct 2026 15:34:27 +0200 Subject: [PATCH 2/2] Resolve conflicts in cherry-pick of #2272 Kept the antalya-26.8 five-parameter `GlueCatalog::updateSchema` signature (matching `ICatalog::updateSchema`) and only un-commented `new_schema` as in the source PR. Kept both `test_create_gzip_metadata` and the new `test_schema_evolution_show_create_and_drop`. --- src/Databases/DataLake/GlueCatalog.cpp | 11 ++--------- tests/integration/test_database_glue/test.py | 5 ++--- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/src/Databases/DataLake/GlueCatalog.cpp b/src/Databases/DataLake/GlueCatalog.cpp index 8e35a2173736..1aac6014c9fe 100644 --- a/src/Databases/DataLake/GlueCatalog.cpp +++ b/src/Databases/DataLake/GlueCatalog.cpp @@ -577,7 +577,7 @@ bool GlueCatalog::tryGetTableMetadata( = getContext()->getSettingsRef()[DB::Setting::allow_experimental_geo_types_in_iceberg].value; auto schema_processor = DB::Iceberg::IcebergSchemaProcessor(context_, allow_geo_parser); auto id = DB::IcebergMetadata::parseTableSchema(metadata_object, schema_processor, context_, log); - auto parsed_schema = schema_processor.getClickhouseTableSchemaById(id); + auto parsed_schema = schema_processor.getClickHouseTableSchemaById(id); if (parsed_schema) schema = *parsed_schema; } @@ -914,15 +914,8 @@ bool GlueCatalog::updateSchema( const String & namespace_name, const String & table_name, const String & new_metadata_path, -<<<<<<< HEAD - Poco::JSON::Object::Ptr /*new_schema*/, - Int32 /*previous_schema_id*/) const -======= Poco::JSON::Object::Ptr new_schema, - Int32 /*previous_schema_id*/, - Int32 /*new_last_column_id*/, - Poco::JSON::Object::Ptr /*metadata*/) const ->>>>>>> 81817b6f1c3 (Merge pull request #2272 from Altinity/2085-glue-datalakecatalog-uses-stale-schema-after-alter-add-column) + Int32 /*previous_schema_id*/) const { std::vector columns; if (new_schema) diff --git a/tests/integration/test_database_glue/test.py b/tests/integration/test_database_glue/test.py index 49f37c457525..8c6d4f1a3787 100644 --- a/tests/integration/test_database_glue/test.py +++ b/tests/integration/test_database_glue/test.py @@ -844,7 +844,6 @@ def test_create(started_cluster): assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") == "AAPL\n" -<<<<<<< HEAD def test_create_gzip_metadata(started_cluster): # Regression for issue #109801: a catalog-backed CREATE TABLE from ClickHouse # with gzip metadata compression exercises IcebergMetadata::createInitial and @@ -896,7 +895,8 @@ def test_create_gzip_metadata(started_cluster): # Reopen through the catalog (fresh database) and confirm read still works. create_clickhouse_glue_database(started_cluster, node, CATALOG_NAME) assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") == "AAPL\n" -======= + + def test_schema_evolution_show_create_and_drop(started_cluster): """SHOW CREATE TABLE must reflect columns added/dropped via ALTER. @@ -944,7 +944,6 @@ def test_schema_evolution_show_create_and_drop(started_cluster): result = node.query(f"SELECT name, column_b, column_c FROM {table_ref} ORDER BY name") assert result == "Alice\t\\N\t\\N\nBob\t42\thello\n" ->>>>>>> 81817b6f1c3 (Merge pull request #2272 from Altinity/2085-glue-datalakecatalog-uses-stale-schema-after-alter-add-column) def test_schema_evolution(started_cluster):