From e93ce1c26f38826880f5f2cd3d1dfedd5f51e174 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Tue, 4 Aug 2026 00:51:39 +0200 Subject: [PATCH 01/18] Fix alter operations for iceberg --- src/Common/FailPoint.cpp | 3 + src/Databases/DataLake/GlueCatalog.cpp | 3 +- src/Databases/DataLake/GlueCatalog.h | 3 +- src/Databases/DataLake/ICatalog.cpp | 3 +- src/Databases/DataLake/ICatalog.h | 3 +- src/Databases/DataLake/RestCatalog.cpp | 433 ++++++++++++++---- src/Databases/DataLake/RestCatalog.h | 13 +- .../gtest_rest_catalog_update_metadata.cpp | 186 ++++++++ .../DataLakes/DataLakeConfiguration.h | 10 +- .../DataLakes/Iceberg/Compaction.cpp | 5 +- .../DataLakes/Iceberg/MetadataGenerator.cpp | 49 +- .../DataLakes/Iceberg/MetadataGenerator.h | 3 +- .../DataLakes/Iceberg/Mutations.cpp | 78 +++- .../ObjectStorage/DataLakes/Iceberg/Utils.cpp | 12 +- .../ObjectStorage/DataLakes/Iceberg/Utils.h | 3 +- .../integration/test_database_iceberg/test.py | 391 +++++++++++++++- .../test_writes_add_column.py | 72 +++ .../test_writes_drop_column.py | 62 +++ .../test_writes_modify_column.py | 70 +++ 19 files changed, 1272 insertions(+), 130 deletions(-) create mode 100644 src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp create mode 100644 tests/integration/test_storage_iceberg_no_spark/test_writes_add_column.py create mode 100644 tests/integration/test_storage_iceberg_no_spark/test_writes_drop_column.py create mode 100644 tests/integration/test_storage_iceberg_no_spark/test_writes_modify_column.py diff --git a/src/Common/FailPoint.cpp b/src/Common/FailPoint.cpp index 71b6b731d871..e885c3134462 100644 --- a/src/Common/FailPoint.cpp +++ b/src/Common/FailPoint.cpp @@ -162,6 +162,9 @@ static struct InitFiu ONCE(write_file_operation_fail_on_read) \ REGULAR(slowdown_parallel_replicas_local_plan_read) \ ONCE(iceberg_writes_cleanup) \ + ONCE(iceberg_alter_catalog_update_metadata_fail) \ + REGULAR(iceberg_alter_orphan_metadata_cleanup_fail) \ + REGULAR(datalake_iceberg_metadata_create_fail) \ REGULAR(storage_cluster_read_sleep) \ ONCE(backup_add_empty_memory_table) \ PAUSEABLE_ONCE(backup_pause_on_start) \ diff --git a/src/Databases/DataLake/GlueCatalog.cpp b/src/Databases/DataLake/GlueCatalog.cpp index 53ac171c79ff..966fd334a521 100644 --- a/src/Databases/DataLake/GlueCatalog.cpp +++ b/src/Databases/DataLake/GlueCatalog.cpp @@ -679,7 +679,8 @@ bool GlueCatalog::updateSchema( const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr /*new_schema*/, - Int32 /*previous_schema_id*/) const + Int32 /*previous_schema_id*/, + Poco::JSON::Object::Ptr /*full_metadata*/) const { return updateMetadata(namespace_name, table_name, new_metadata_path, nullptr); } diff --git a/src/Databases/DataLake/GlueCatalog.h b/src/Databases/DataLake/GlueCatalog.h index 919b13a5669f..d5b566050469 100644 --- a/src/Databases/DataLake/GlueCatalog.h +++ b/src/Databases/DataLake/GlueCatalog.h @@ -73,7 +73,8 @@ class GlueCatalog final : public ICatalog, private DB::WithContext const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr new_schema, - Int32 previous_schema_id) const override; + Int32 previous_schema_id, + Poco::JSON::Object::Ptr full_metadata = nullptr) const override; void dropTable(const String & namespace_name, const String & table_name) const override; diff --git a/src/Databases/DataLake/ICatalog.cpp b/src/Databases/DataLake/ICatalog.cpp index 432b4d8b61c5..70eccb5fc113 100644 --- a/src/Databases/DataLake/ICatalog.cpp +++ b/src/Databases/DataLake/ICatalog.cpp @@ -325,7 +325,8 @@ bool ICatalog::updateSchema( const String & /*table_name*/, const String & /*new_metadata_path*/, Poco::JSON::Object::Ptr /*new_schema*/, - Int32 /*previous_schema_id*/) const + Int32 /*previous_schema_id*/, + Poco::JSON::Object::Ptr /*full_metadata*/) const { throw DB::Exception(DB::ErrorCodes::NOT_IMPLEMENTED, "updateSchema is not implemented"); } diff --git a/src/Databases/DataLake/ICatalog.h b/src/Databases/DataLake/ICatalog.h index e14b00ac3732..8fd5ba85666c 100644 --- a/src/Databases/DataLake/ICatalog.h +++ b/src/Databases/DataLake/ICatalog.h @@ -196,7 +196,8 @@ class ICatalog const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr new_schema, - Int32 previous_schema_id) const; + Int32 previous_schema_id, + Poco::JSON::Object::Ptr full_metadata = nullptr) const; /// Drop table from catalog. virtual void dropTable(const String & namespace_name, const String & table_name) const; diff --git a/src/Databases/DataLake/RestCatalog.cpp b/src/Databases/DataLake/RestCatalog.cpp index 28c1195082e4..3d0a34885a99 100644 --- a/src/Databases/DataLake/RestCatalog.cpp +++ b/src/Databases/DataLake/RestCatalog.cpp @@ -8,6 +8,8 @@ #include #include #include +#include +#include #include #include #include "config.h" @@ -34,6 +36,7 @@ #include #include +#include #include #include #include @@ -66,6 +69,7 @@ namespace DB::Setting namespace DB::FailPoints { extern const char check_database_datalake_negative[]; + extern const char iceberg_alter_catalog_update_metadata_fail[]; } namespace DataLake @@ -149,6 +153,305 @@ std::unordered_set getAllowedBigLakeMetadataServiceHosts( } +namespace +{ + +Poco::JSON::Object::Ptr cloneJsonObject(const Poco::JSON::Object::Ptr & obj) +{ + std::ostringstream oss; // STYLE_CHECK_ALLOW_STD_STRING_STREAM + obj->stringify(oss); + Poco::JSON::Parser parser; + return parser.parse(oss.str()).extract(); +} + +bool icebergJsonValueEquals(const Poco::Dynamic::Var & lhs, const Poco::Dynamic::Var & rhs); + +bool icebergJsonObjectEquals(const Poco::JSON::Object::Ptr & lhs, const Poco::JSON::Object::Ptr & rhs) +{ + if (lhs.isNull() || rhs.isNull()) + return lhs.isNull() && rhs.isNull(); + if (lhs->size() != rhs->size()) + return false; + for (auto it = lhs->begin(); it != lhs->end(); ++it) + { + if (!rhs->has(it->first)) + return false; + if (!icebergJsonValueEquals(it->second, rhs->get(it->first))) + return false; + } + return true; +} + +bool icebergJsonArrayEquals(const Poco::JSON::Array::Ptr & lhs, const Poco::JSON::Array::Ptr & rhs) +{ + if (lhs.isNull() || rhs.isNull()) + return lhs.isNull() && rhs.isNull(); + if (lhs->size() != rhs->size()) + return false; + for (UInt32 i = 0; i < lhs->size(); ++i) + if (!icebergJsonValueEquals(lhs->get(i), rhs->get(i))) + return false; + return true; +} + +/// Structural, key-order-independent comparison of two parsed JSON values. +bool icebergJsonValueEquals(const Poco::Dynamic::Var & lhs, const Poco::Dynamic::Var & rhs) +{ + const bool lhs_is_object = lhs.type() == typeid(Poco::JSON::Object::Ptr); + const bool rhs_is_object = rhs.type() == typeid(Poco::JSON::Object::Ptr); + if (lhs_is_object || rhs_is_object) + { + if (!(lhs_is_object && rhs_is_object)) + return false; + return icebergJsonObjectEquals(lhs.extract(), rhs.extract()); + } + const bool lhs_is_array = lhs.type() == typeid(Poco::JSON::Array::Ptr); + const bool rhs_is_array = rhs.type() == typeid(Poco::JSON::Array::Ptr); + if (lhs_is_array || rhs_is_array) + { + if (!(lhs_is_array && rhs_is_array)) + return false; + return icebergJsonArrayEquals(lhs.extract(), rhs.extract()); + } + return lhs.toString() == rhs.toString(); +} + +/// Two Iceberg schemas are equivalent when they differ only by their `schema-id`. +bool schemasEquivalentIgnoringId(const Poco::JSON::Object::Ptr & lhs, const Poco::JSON::Object::Ptr & rhs) +{ + Poco::JSON::Object::Ptr lhs_copy = cloneJsonObject(lhs); + Poco::JSON::Object::Ptr rhs_copy = cloneJsonObject(rhs); + lhs_copy->remove(DB::Iceberg::f_schema_id); + rhs_copy->remove(DB::Iceberg::f_schema_id); + return icebergJsonObjectEquals(lhs_copy, rhs_copy); +} + +void collectSchemaFieldIdsFromFields(const Poco::JSON::Array::Ptr & fields, std::unordered_set & ids) +{ + for (UInt32 i = 0; i < fields->size(); ++i) + { + auto field = fields->getObject(i); + if (field->has(DB::Iceberg::f_id)) + ids.insert(field->getValue(DB::Iceberg::f_id)); + } +} + +/// Returns true when the default sort order references field ids that are absent +/// from the new schema (i.e. the sort order became incompatible after a column drop). +bool sortOrderIncompatibleWithSchema( + const Poco::JSON::Object::Ptr & metadata_obj, + const Poco::JSON::Object::Ptr & new_schema_obj) +{ + if (!metadata_obj->has(DB::Iceberg::f_sort_orders) || !metadata_obj->has(DB::Iceberg::f_default_sort_order_id)) + return false; + + const Int64 default_sort_order_id = metadata_obj->getValue(DB::Iceberg::f_default_sort_order_id); + if (default_sort_order_id == 0) + return false; + + auto sort_orders = metadata_obj->getArray(DB::Iceberg::f_sort_orders); + Poco::JSON::Object::Ptr default_sort_order; + for (UInt32 i = 0; i < sort_orders->size(); ++i) + { + auto sort_order = sort_orders->getObject(i); + if (sort_order->getValue(DB::Iceberg::f_order_id) == default_sort_order_id) + { + default_sort_order = sort_order; + break; + } + } + + if (!default_sort_order || !default_sort_order->has(DB::Iceberg::f_fields)) + return false; + + auto sort_fields = default_sort_order->getArray(DB::Iceberg::f_fields); + if (sort_fields->size() == 0) + return false; + + std::unordered_set new_schema_field_ids; + if (new_schema_obj->has(DB::Iceberg::f_fields)) + collectSchemaFieldIdsFromFields(new_schema_obj->getArray(DB::Iceberg::f_fields), new_schema_field_ids); + + for (UInt32 i = 0; i < sort_fields->size(); ++i) + { + auto field = sort_fields->getObject(i); + if (!field->has(DB::Iceberg::f_source_id)) + continue; + + const Int32 source_id = field->getValue(DB::Iceberg::f_source_id); + if (!new_schema_field_ids.contains(source_id)) + return true; + } + + return false; +} + +} + +Poco::JSON::Object::Ptr buildUpdateMetadataRequestBody( + const String & namespace_name, const String & table_name, Poco::JSON::Object::Ptr new_snapshot) +{ + if (!new_snapshot) + return nullptr; + + Poco::JSON::Object::Ptr request_body = new Poco::JSON::Object; + { + Poco::JSON::Object::Ptr identifier = new Poco::JSON::Object; + identifier->set("name", table_name); + Poco::JSON::Array::Ptr namespaces = new Poco::JSON::Array; + namespaces->add(namespace_name); + identifier->set("namespace", namespaces); + + request_body->set("identifier", identifier); + } + + if (new_snapshot->has(DB::Iceberg::f_schemas)) + { + if (!new_snapshot->has(DB::Iceberg::f_current_schema_id)) + throw DB::Exception( + DB::ErrorCodes::DATALAKE_DATABASE_ERROR, + "Iceberg update-metadata for {}.{} is missing '{}' field", + namespace_name, table_name, DB::Iceberg::f_current_schema_id); + + const Int32 new_schema_id = new_snapshot->getValue(DB::Iceberg::f_current_schema_id); + const Int32 old_schema_id = new_schema_id - 1; + + Poco::JSON::Object::Ptr new_schema_obj; + auto schemas = new_snapshot->getArray(DB::Iceberg::f_schemas); + for (UInt32 i = 0; i < schemas->size(); ++i) + { + auto s = schemas->getObject(i); + if (s->getValue(DB::Iceberg::f_schema_id) == new_schema_id) + { + new_schema_obj = s; + break; + } + } + if (!new_schema_obj) + throw DB::Exception( + DB::ErrorCodes::DATALAKE_DATABASE_ERROR, + "Iceberg update-metadata for {}.{}: no schema object matching current-schema-id={}", + namespace_name, table_name, new_schema_id); + + Poco::JSON::Object::Ptr schema_for_rest = cloneJsonObject(new_schema_obj); + if (!schema_for_rest->has("identifier-field-ids")) + { + Poco::JSON::Array::Ptr empty_identifier_field_ids = new Poco::JSON::Array; + schema_for_rest->set("identifier-field-ids", empty_identifier_field_ids); + } + + if (old_schema_id >= 0) + { + Poco::JSON::Object::Ptr requirement = new Poco::JSON::Object; + requirement->set("type", "assert-current-schema-id"); + requirement->set("current-schema-id", old_schema_id); + + Poco::JSON::Array::Ptr requirements = new Poco::JSON::Array; + requirements->add(requirement); + request_body->set("requirements", requirements); + } + + /// The target schema may be identical to a schema already present in the table's + /// schema history. The Iceberg catalog deduplicates identical schemas, so an + /// `add-schema` update becomes a no-op and a subsequent `set-current-schema: -1` + /// is rejected. In that case we point `set-current-schema` at the existing id. + std::optional existing_equivalent_schema_id; + for (UInt32 i = 0; i < schemas->size(); ++i) + { + auto existing_schema = schemas->getObject(i); + if (existing_schema->getValue(DB::Iceberg::f_schema_id) == new_schema_id) + continue; + if (schemasEquivalentIgnoringId(existing_schema, new_schema_obj)) + { + existing_equivalent_schema_id = existing_schema->getValue(DB::Iceberg::f_schema_id); + break; + } + } + + Poco::JSON::Array::Ptr updates = new Poco::JSON::Array; + if (existing_equivalent_schema_id.has_value()) + { + Poco::JSON::Object::Ptr set_current_schema = new Poco::JSON::Object; + set_current_schema->set("action", "set-current-schema"); + set_current_schema->set("schema-id", *existing_equivalent_schema_id); + updates->add(set_current_schema); + } + else + { + { + Poco::JSON::Object::Ptr add_schema = new Poco::JSON::Object; + add_schema->set("action", "add-schema"); + add_schema->set("schema", schema_for_rest); + if (new_snapshot->has(DB::Iceberg::f_last_column_id)) + add_schema->set("last-column-id", new_snapshot->getValue(DB::Iceberg::f_last_column_id)); + updates->add(add_schema); + } + { + Poco::JSON::Object::Ptr set_current_schema = new Poco::JSON::Object; + set_current_schema->set("action", "set-current-schema"); + set_current_schema->set("schema-id", -1); + updates->add(set_current_schema); + } + } + + if (sortOrderIncompatibleWithSchema(new_snapshot, new_schema_obj)) + { + Poco::JSON::Object::Ptr unsorted_sort_order = new Poco::JSON::Object; + unsorted_sort_order->set(DB::Iceberg::f_order_id, 0); + unsorted_sort_order->set(DB::Iceberg::f_fields, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); + + Poco::JSON::Object::Ptr add_sort_order = new Poco::JSON::Object; + add_sort_order->set("action", "add-sort-order"); + add_sort_order->set("sort-order", unsorted_sort_order); + updates->add(add_sort_order); + + Poco::JSON::Object::Ptr set_default_sort_order = new Poco::JSON::Object; + set_default_sort_order->set("action", "set-default-sort-order"); + set_default_sort_order->set("sort-order-id", -1); + updates->add(set_default_sort_order); + } + + request_body->set("updates", updates); + } + else + { + if (new_snapshot->has("parent-snapshot-id")) + { + auto parent_snapshot_id = new_snapshot->getValue("parent-snapshot-id"); + if (parent_snapshot_id != -1) + { + Poco::JSON::Object::Ptr requirement = new Poco::JSON::Object; + requirement->set("type", "assert-ref-snapshot-id"); + requirement->set("ref", "main"); + requirement->set("snapshot-id", parent_snapshot_id); + + Poco::JSON::Array::Ptr requirements = new Poco::JSON::Array; + requirements->add(requirement); + request_body->set("requirements", requirements); + } + } + + Poco::JSON::Array::Ptr updates = new Poco::JSON::Array; + { + Poco::JSON::Object::Ptr add_snapshot = new Poco::JSON::Object; + add_snapshot->set("action", "add-snapshot"); + add_snapshot->set("snapshot", new_snapshot); + updates->add(add_snapshot); + } + { + Poco::JSON::Object::Ptr set_snapshot = new Poco::JSON::Object; + set_snapshot->set("action", "set-snapshot-ref"); + set_snapshot->set("ref-name", "main"); + set_snapshot->set("type", "branch"); + set_snapshot->set("snapshot-id", new_snapshot->getValue("snapshot-id")); + updates->add(set_snapshot); + } + request_body->set("updates", updates); + } + + return request_body; +} + std::string RestCatalog::Config::toString() const { DB::WriteBufferFromOwnString wb; @@ -1233,57 +1536,13 @@ void RestCatalog::createTable(const String & namespace_name, const String & tabl bool RestCatalog::updateMetadata(const String & namespace_name, const String & table_name, const String & /*new_metadata_path*/, Poco::JSON::Object::Ptr new_snapshot) const { - const std::string endpoint = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables" / table_name).generic_string(); - - Poco::JSON::Object::Ptr request_body = new Poco::JSON::Object; - { - Poco::JSON::Object::Ptr identifier = new Poco::JSON::Object; - identifier->set("name", table_name); - Poco::JSON::Array::Ptr namespaces = new Poco::JSON::Array; - namespaces->add(namespace_name); - identifier->set("namespace", namespaces); - - request_body->set("identifier", identifier); - } - - if (new_snapshot->has("parent-snapshot-id")) - { - auto parent_snapshot_id = new_snapshot->getValue("parent-snapshot-id"); - if (parent_snapshot_id != -1) - { - Poco::JSON::Object::Ptr requirement = new Poco::JSON::Object; - requirement->set("type", "assert-ref-snapshot-id"); - requirement->set("ref", "main"); - requirement->set("snapshot-id", parent_snapshot_id); - - Poco::JSON::Array::Ptr requirements = new Poco::JSON::Array; - requirements->add(requirement); + fiu_do_on(DB::FailPoints::iceberg_alter_catalog_update_metadata_fail, { return false; }); - request_body->set("requirements", requirements); - } - } - - { - Poco::JSON::Array::Ptr updates = new Poco::JSON::Array; - - { - Poco::JSON::Object::Ptr add_snapshot = new Poco::JSON::Object; - add_snapshot->set("action", "add-snapshot"); - add_snapshot->set("snapshot", new_snapshot); - updates->add(add_snapshot); - } - - { - Poco::JSON::Object::Ptr set_snapshot = new Poco::JSON::Object; - set_snapshot->set("action", "set-snapshot-ref"); - set_snapshot->set("ref-name", "main"); - set_snapshot->set("type", "branch"); - set_snapshot->set("snapshot-id", new_snapshot->getValue("snapshot-id")); + const std::string endpoint = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables" / table_name).generic_string(); - updates->add(set_snapshot); - } - request_body->set("updates", updates); - } + auto request_body = buildUpdateMetadataRequestBody(namespace_name, table_name, new_snapshot); + if (!request_body) + return true; try { @@ -1291,7 +1550,8 @@ bool RestCatalog::updateMetadata(const String & namespace_name, const String & t } catch (const DB::HTTPException & ex) { - LOG_TRACE(log, "Unsucceeded request {}", ex.what()); + LOG_WARNING(log, "Iceberg REST updateMetadata for {}.{} failed: {}", + namespace_name, table_name, ex.displayText()); return false; } return true; @@ -1302,49 +1562,59 @@ bool RestCatalog::updateSchema( const String & table_name, const String & /*new_metadata_path*/, Poco::JSON::Object::Ptr new_schema, - Int32 previous_schema_id) const + Int32 previous_schema_id, + Poco::JSON::Object::Ptr full_metadata) const { const std::string endpoint = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables" / table_name).generic_string(); - Poco::JSON::Object::Ptr request_body = new Poco::JSON::Object; - { - Poco::JSON::Object::Ptr identifier = new Poco::JSON::Object; - identifier->set("name", table_name); - Poco::JSON::Array::Ptr namespaces = new Poco::JSON::Array; - namespaces->add(namespace_name); - identifier->set("namespace", namespaces); - - request_body->set("identifier", identifier); - } + Poco::JSON::Object::Ptr request_body; + /// When full metadata is available, use the richer builder which handles + /// equivalent-schema dedup and sort-order reset. + if (full_metadata) { - Poco::JSON::Object::Ptr requirement = new Poco::JSON::Object; - requirement->set("type", "assert-current-schema-id"); - requirement->set("current-schema-id", previous_schema_id); - - Poco::JSON::Array::Ptr requirements = new Poco::JSON::Array; - requirements->add(requirement); - request_body->set("requirements", requirements); + request_body = buildUpdateMetadataRequestBody(namespace_name, table_name, full_metadata); + if (!request_body) + return true; } - + else { - Poco::JSON::Array::Ptr updates = new Poco::JSON::Array; - + request_body = new Poco::JSON::Object; { - Poco::JSON::Object::Ptr add_schema = new Poco::JSON::Object; - add_schema->set("action", "add-schema"); - add_schema->set("schema", new_schema); - updates->add(add_schema); + Poco::JSON::Object::Ptr identifier = new Poco::JSON::Object; + identifier->set("name", table_name); + Poco::JSON::Array::Ptr namespaces = new Poco::JSON::Array; + namespaces->add(namespace_name); + identifier->set("namespace", namespaces); + request_body->set("identifier", identifier); } { - Poco::JSON::Object::Ptr set_current_schema = new Poco::JSON::Object; - set_current_schema->set("action", "set-current-schema"); - set_current_schema->set("schema-id", -1); - updates->add(set_current_schema); + Poco::JSON::Object::Ptr requirement = new Poco::JSON::Object; + requirement->set("type", "assert-current-schema-id"); + requirement->set("current-schema-id", previous_schema_id); + + Poco::JSON::Array::Ptr requirements = new Poco::JSON::Array; + requirements->add(requirement); + request_body->set("requirements", requirements); } - request_body->set("updates", updates); + { + Poco::JSON::Array::Ptr updates = new Poco::JSON::Array; + { + Poco::JSON::Object::Ptr add_schema = new Poco::JSON::Object; + add_schema->set("action", "add-schema"); + add_schema->set("schema", new_schema); + updates->add(add_schema); + } + { + Poco::JSON::Object::Ptr set_current_schema = new Poco::JSON::Object; + set_current_schema->set("action", "set-current-schema"); + set_current_schema->set("schema-id", -1); + updates->add(set_current_schema); + } + request_body->set("updates", updates); + } } try @@ -1353,7 +1623,8 @@ bool RestCatalog::updateSchema( } catch (const DB::HTTPException & ex) { - LOG_TRACE(log, "Unsucceeded request {}", ex.what()); + LOG_WARNING(log, "Iceberg REST updateSchema for {}.{} failed: {}", + namespace_name, table_name, ex.displayText()); return false; } return true; diff --git a/src/Databases/DataLake/RestCatalog.h b/src/Databases/DataLake/RestCatalog.h index 982475ee2c96..00799d781142 100644 --- a/src/Databases/DataLake/RestCatalog.h +++ b/src/Databases/DataLake/RestCatalog.h @@ -79,7 +79,8 @@ class RestCatalog : public ICatalog, public DB::WithContext const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr new_schema, - Int32 previous_schema_id) const override; + Int32 previous_schema_id, + Poco::JSON::Object::Ptr full_metadata = nullptr) const override; bool isTransactional() const override { return true; } @@ -243,6 +244,16 @@ class BigLakeCatalog : public RestCatalog AccessToken retrieveGoogleCloudAccessTokenFromRefreshToken() const; }; +/// Builds the JSON body for `POST .../namespaces/{ns}/tables/{table}` (Iceberg REST update). +/// +/// Returns `nullptr` when `new_snapshot` is null (nothing to commit). Throws +/// `DB::Exception(DATALAKE_DATABASE_ERROR)` with a specific message when the metadata +/// blob is malformed (e.g. missing `current-schema-id`, no schema object matching it). +Poco::JSON::Object::Ptr buildUpdateMetadataRequestBody( + const String & namespace_name, + const String & table_name, + Poco::JSON::Object::Ptr new_snapshot); + } #endif diff --git a/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp b/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp new file mode 100644 index 000000000000..11fcc085990c --- /dev/null +++ b/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp @@ -0,0 +1,186 @@ +#include "config.h" + +#if USE_AVRO + +#include +#include +#include +#include +#include +#include +#include +#include + +using namespace DB; + +namespace +{ +Poco::JSON::Object::Ptr findUpdateByAction(const Poco::JSON::Array::Ptr & updates, const std::string & action) +{ + for (unsigned int i = 0; i < updates->size(); ++i) + { + auto o = updates->getObject(i); + if (o->getValue("action") == action) + return o; + } + return nullptr; +} +} + +TEST(RestCatalogUpdateMetadataBody, NullSnapshotReturnsNull) +{ + auto body = DataLake::buildUpdateMetadataRequestBody("ns", "t", nullptr); + EXPECT_FALSE(body); +} + +TEST(RestCatalogUpdateMetadataBody, SchemaUpdateValid) +{ + Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; + Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; + Poco::JSON::Object::Ptr schema = new Poco::JSON::Object; + schema->set(Iceberg::f_schema_id, 1); + schema->set(Iceberg::f_type, "struct"); + schema->set(Iceberg::f_fields, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); + schemas->add(schema); + snapshot->set(Iceberg::f_schemas, schemas); + snapshot->set(Iceberg::f_current_schema_id, 1); + snapshot->set(Iceberg::f_last_column_id, 3); + + auto body = DataLake::buildUpdateMetadataRequestBody("my.ns", "tbl", snapshot); + ASSERT_TRUE(body); + + auto id = body->getObject("identifier"); + EXPECT_EQ(id->getValue("name"), "tbl"); + auto ns = id->getArray("namespace"); + ASSERT_EQ(ns->size(), 1u); + EXPECT_EQ(ns->getElement(0), "my.ns"); + + ASSERT_TRUE(body->has("requirements")); + auto req = body->getArray("requirements")->getObject(0); + EXPECT_EQ(req->getValue("type"), "assert-current-schema-id"); + EXPECT_EQ(req->getValue("current-schema-id"), 0); + + auto updates = body->getArray("updates"); + auto add_schema = findUpdateByAction(updates, "add-schema"); + ASSERT_TRUE(add_schema); + EXPECT_TRUE(add_schema->has("schema")); + EXPECT_EQ(add_schema->getValue("last-column-id"), 3); + + auto set_schema = findUpdateByAction(updates, "set-current-schema"); + ASSERT_TRUE(set_schema); + EXPECT_EQ(set_schema->getValue("schema-id"), -1); +} + +TEST(RestCatalogUpdateMetadataBody, SchemaUpdateCurrentIdZeroNoRequirement) +{ + Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; + Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; + Poco::JSON::Object::Ptr schema = new Poco::JSON::Object; + schema->set(Iceberg::f_schema_id, 0); + schema->set(Iceberg::f_type, "struct"); + schema->set(Iceberg::f_fields, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); + schemas->add(schema); + snapshot->set(Iceberg::f_schemas, schemas); + snapshot->set(Iceberg::f_current_schema_id, 0); + + auto body = DataLake::buildUpdateMetadataRequestBody("ns", "t", snapshot); + ASSERT_TRUE(body); + EXPECT_FALSE(body->has("requirements")); +} + +TEST(RestCatalogUpdateMetadataBody, SchemaUpdateBodyIsStringifiable) +{ + Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; + Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; + Poco::JSON::Object::Ptr schema = new Poco::JSON::Object; + schema->set(Iceberg::f_schema_id, 1); + schema->set(Iceberg::f_type, "struct"); + schema->set(Iceberg::f_fields, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); + schemas->add(schema); + snapshot->set(Iceberg::f_schemas, schemas); + snapshot->set(Iceberg::f_current_schema_id, 1); + + auto body = DataLake::buildUpdateMetadataRequestBody("ns", "t", snapshot); + ASSERT_TRUE(body); + + std::ostringstream oss; // STYLE_CHECK_ALLOW_STD_STRING_STREAM + ASSERT_NO_THROW(body->stringify(oss)); + EXPECT_NE(oss.str().find("\"identifier-field-ids\""), std::string::npos); + EXPECT_NE(oss.str().find("\"add-schema\""), std::string::npos); +} + +TEST(RestCatalogUpdateMetadataBody, SchemaUpdateMissingCurrentSchemaIdThrows) +{ + Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; + snapshot->set(Iceberg::f_schemas, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); + + EXPECT_THROW(DataLake::buildUpdateMetadataRequestBody("ns", "t", snapshot), DB::Exception); +} + +TEST(RestCatalogUpdateMetadataBody, SchemaUpdateNoMatchingSchemaIdThrows) +{ + Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; + Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; + Poco::JSON::Object::Ptr schema = new Poco::JSON::Object; + schema->set(Iceberg::f_schema_id, 1); + schema->set(Iceberg::f_type, "struct"); + schema->set(Iceberg::f_fields, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); + schemas->add(schema); + snapshot->set(Iceberg::f_schemas, schemas); + snapshot->set(Iceberg::f_current_schema_id, 99); + + EXPECT_THROW(DataLake::buildUpdateMetadataRequestBody("ns", "t", snapshot), DB::Exception); +} + +TEST(RestCatalogUpdateMetadataBody, SnapshotUpdateWithParent) +{ + Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; + snapshot->set("snapshot-id", static_cast(12345)); + snapshot->set("parent-snapshot-id", static_cast(12344)); + snapshot->set(Iceberg::f_timestamp_ms, static_cast(1700000000000LL)); + + auto body = DataLake::buildUpdateMetadataRequestBody("ns", "t", snapshot); + ASSERT_TRUE(body); + + ASSERT_TRUE(body->has("requirements")); + auto req = body->getArray("requirements")->getObject(0); + EXPECT_EQ(req->getValue("type"), "assert-ref-snapshot-id"); + EXPECT_EQ(req->getValue("ref"), "main"); + EXPECT_EQ(req->getValue("snapshot-id"), 12344); + + auto updates = body->getArray("updates"); + auto add_snap = findUpdateByAction(updates, "add-snapshot"); + ASSERT_TRUE(add_snap); + EXPECT_EQ(add_snap->getObject("snapshot")->getValue("snapshot-id"), 12345); + + auto set_ref = findUpdateByAction(updates, "set-snapshot-ref"); + ASSERT_TRUE(set_ref); + EXPECT_EQ(set_ref->getValue("snapshot-id"), 12345); +} + +TEST(RestCatalogUpdateMetadataBody, SnapshotUpdateWithoutParent) +{ + Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; + snapshot->set("snapshot-id", static_cast(999)); + + auto body = DataLake::buildUpdateMetadataRequestBody("ns", "t", snapshot); + ASSERT_TRUE(body); + EXPECT_FALSE(body->has("requirements")); + + auto updates = body->getArray("updates"); + ASSERT_TRUE(findUpdateByAction(updates, "add-snapshot")); + ASSERT_TRUE(findUpdateByAction(updates, "set-snapshot-ref")); +} + +TEST(RestCatalogUpdateMetadataBody, SnapshotUpdateParentMinusOneNoRequirement) +{ + Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; + snapshot->set("snapshot-id", static_cast(1)); + snapshot->set("parent-snapshot-id", static_cast(-1)); + + auto body = DataLake::buildUpdateMetadataRequestBody("ns", "t", snapshot); + ASSERT_TRUE(body); + EXPECT_FALSE(body->has("requirements")); +} + +#endif diff --git a/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h b/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h index f22078d2799c..bdc5e173addc 100644 --- a/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h +++ b/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -48,9 +49,15 @@ namespace ErrorCodes { extern const int BAD_ARGUMENTS; extern const int LOGICAL_ERROR; + extern const int NOT_INITIALIZED; extern const int PATH_ACCESS_DENIED; } +namespace FailPoints +{ + extern const char datalake_iceberg_metadata_create_fail[]; +} + namespace DataLakeStorageSetting { extern DataLakeStorageSettingsDatabaseDataLakeCatalogType storage_catalog_type; @@ -122,6 +129,7 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl { if (current_metadata != nullptr) return; + fiu_do_on(FailPoints::datalake_iceberg_metadata_create_fail, { return; }); BaseStorageConfiguration::update(object_storage, local_context); assertLocalPathCorrect(object_storage, local_context); current_metadata = DataLakeMetadata::create(object_storage, weak_from_this(), local_context); @@ -424,7 +432,7 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl void assertInitialized() const { if (!current_metadata) - throw Exception(ErrorCodes::LOGICAL_ERROR, "Metadata is not initialized"); + throw Exception(ErrorCodes::NOT_INITIALIZED, "Metadata is not initialized"); } ReadFromFormatInfo prepareReadingFromFormat( diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp index 66f07c521b27..fcc188dddcb6 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp @@ -138,7 +138,10 @@ static Plan getPlan( context, log.get(), persistent_table_components.table_uuid, - persistent_table_components.metadata_compression_method); + persistent_table_components.metadata_compression_method, + /* force_fetch_latest_metadata */ true, + /* ignore_explicit_metadata_file_path */ false, + /* select_by_table_uuid */ true); Poco::JSON::Object::Ptr initial_metadata_object = getMetadataJSONObject(metadata_file_path, object_storage, persistent_table_components.metadata_cache, context, log, compression_method, persistent_table_components.table_uuid); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp index 85f5127c21c4..f85b14764067 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp @@ -54,6 +54,7 @@ bool checkValidSchemaEvolution(Poco::Dynamic::Var old_type, Poco::Dynamic::Var n return true; } + if (!old_type.isString() && !new_type.isString()) { auto old_complex_type = old_type.extract(); auto new_complex_type = new_type.extract(); @@ -69,6 +70,23 @@ bool checkValidSchemaEvolution(Poco::Dynamic::Var old_type, Poco::Dynamic::Var n return false; } +bool icebergTypesEqual(Poco::Dynamic::Var old_type, Poco::Dynamic::Var new_type) +{ + if (old_type.isString() && new_type.isString()) + return old_type.extract() == new_type.extract(); + + if (!old_type.isString() && !new_type.isString()) + { + std::ostringstream oss_old; // STYLE_CHECK_ALLOW_STD_STRING_STREAM + std::ostringstream oss_new; // STYLE_CHECK_ALLOW_STD_STRING_STREAM + old_type.extract()->stringify(oss_old); + new_type.extract()->stringify(oss_new); + return oss_old.str() == oss_new.str(); + } + + return false; +} + } MetadataGenerator::MetadataGenerator(Poco::JSON::Object::Ptr metadata_object_) @@ -317,10 +335,9 @@ void MetadataGenerator::generateAddColumnMetadata(const String & column_name, Da metadata_object->getArray(Iceberg::f_schemas)->add(current_schema); } -void MetadataGenerator::generateModifyColumnMetadata(const String & column_name, DataTypePtr type) +bool MetadataGenerator::generateModifyColumnMetadata(const String & column_name, DataTypePtr type) { auto current_schema_id = metadata_object->getValue(Iceberg::f_current_schema_id); - metadata_object->set(Iceberg::f_current_schema_id, current_schema_id + 1); Poco::JSON::Object::Ptr current_schema; auto schemas = metadata_object->getArray(Iceberg::f_schemas); @@ -335,37 +352,41 @@ void MetadataGenerator::generateModifyColumnMetadata(const String & column_name, if (!current_schema) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Not found schema with id {}", current_schema_id); - current_schema = deepCopy(current_schema); - auto last_column_id = metadata_object->getValue(Iceberg::f_last_column_id); + auto last_column_id = metadata_object->getValue(Iceberg::f_last_column_id); auto new_type = Iceberg::getIcebergType(type, last_column_id); auto schema_fields = current_schema->getArray(Iceberg::f_fields); - bool found = false; for (UInt32 i = 0; i < schema_fields->size(); ++i) { auto current_field = schema_fields->getObject(i); if (current_field->getValue(Iceberg::f_name) == column_name) { + if (current_field->getValue(Iceberg::f_required) == new_type.second + && icebergTypesEqual(current_field->get(Iceberg::f_type), new_type.first)) + return false; + if (!checkValidSchemaEvolution(current_field->get(Iceberg::f_type), new_type.first)) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Iceberg spec doesn't allow schema evolution to type {}", type->getPrettyName()); - auto old_type = deepCopy(current_field); - current_field->set(Iceberg::f_type, new_type.first); if (!current_field->getValue(Iceberg::f_required) && !type->isNullable()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Iceberg spec doesn't allow change type from nullable to non-nullable {}", type->getPrettyName()); + current_schema = deepCopy(current_schema); + schema_fields = current_schema->getArray(Iceberg::f_fields); + current_field = schema_fields->getObject(i); + + current_field->set(Iceberg::f_type, new_type.first); current_field->set(Iceberg::f_required, new_type.second); - found = true; - break; + + metadata_object->set(Iceberg::f_current_schema_id, current_schema_id + 1); + current_schema->set(Iceberg::f_schema_id, current_schema_id + 1); + metadata_object->getArray(Iceberg::f_schemas)->add(current_schema); + return true; } } - if (!found) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Not found column {}", column_name); - - current_schema->set(Iceberg::f_schema_id, current_schema_id + 1); - metadata_object->getArray(Iceberg::f_schemas)->add(current_schema); + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Column {} not found in schema", column_name); } void MetadataGenerator::generateRenameColumnMetadata(const String & column_name, const String & new_column_name) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h index de7cbc86d99f..676185c4ae63 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h @@ -43,7 +43,8 @@ class MetadataGenerator void generateAddColumnMetadata(const String & column_name, DataTypePtr type); void generateDropColumnMetadata(const String & column_name); - void generateModifyColumnMetadata(const String & column_name, DataTypePtr type); + /// Returns false when the column already has the requested type (no metadata change). + bool generateModifyColumnMetadata(const String & column_name, DataTypePtr type); void generateRenameColumnMetadata(const String & column_name, const String & new_column_name); private: diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp index 8a04b5c83b3e..02e698c82170 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp @@ -39,6 +39,7 @@ namespace DB::ErrorCodes { extern const int BAD_ARGUMENTS; +extern const int DATALAKE_DATABASE_ERROR; extern const int LOGICAL_ERROR; extern const int LIMIT_EXCEEDED; } @@ -52,6 +53,7 @@ extern const DataLakeStorageSettingsString iceberg_metadata_file_path; namespace DB::FailPoints { extern const char iceberg_writes_cleanup[]; +extern const char iceberg_alter_orphan_metadata_cleanup_fail[]; } namespace DB::Iceberg @@ -590,7 +592,8 @@ void mutate( persistent_table_components.table_uuid, persistent_table_components.metadata_compression_method, /* force_fetch_latest_metadata */ true, - /* ignore_explicit_metadata_file_path */ true); + /* ignore_explicit_metadata_file_path */ true, + /* select_by_table_uuid */ true); FileNamesGenerator filename_generator(persistent_table_components.path_resolver.getTableLocation(), false, CompressionMethod::None, write_format); filename_generator.setVersion(last_version + 1); @@ -720,11 +723,14 @@ void alter( std::shared_ptr catalog) { if (params.size() != 1) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Params with size 1 is not supported"); + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Iceberg alter supports exactly one command at a time, got {}", params.size()); - size_t i = 0; - bool succeeded = false; - while (i < MAX_TRANSACTION_RETRIES) + /// The command was marked as a no-op by AlterCommands::prepare (e.g. RENAME/DROP COLUMN IF EXISTS + /// for a missing column, or ADD COLUMN IF NOT EXISTS for an existing one). + if (params[0].ignore) + return; + + for (size_t i = 0; i < MAX_TRANSACTION_RETRIES; ++i) { auto log = getLogger("IcebergMutations"); @@ -743,7 +749,8 @@ void alter( persistent_table_components.table_uuid, persistent_table_components.metadata_compression_method, /* force_fetch_latest_metadata */ true, - /* ignore_explicit_metadata_file_path */ true); + /* ignore_explicit_metadata_file_path */ true, + /* select_by_table_uuid */ true); last_version = last_version_info.version; metadata_path = last_version_info.path; compression_method = last_version_info.compression_method; @@ -776,13 +783,18 @@ void alter( persistent_table_components.table_uuid, persistent_table_components.metadata_compression_method, /* force_fetch_latest_metadata */ true, - /* ignore_explicit_metadata_file_path */ false); + /* ignore_explicit_metadata_file_path */ false, + /* select_by_table_uuid */ true); last_version = last_version_info.version; metadata_path = last_version_info.path; compression_method = last_version_info.compression_method; } - FileNamesGenerator filename_generator(persistent_table_components.path_resolver.getTableLocation(), false, CompressionMethod::None, write_format); + FileNamesGenerator filename_generator( + persistent_table_components.path_resolver.getTableLocation(), + catalog && catalog->isTransactional(), + CompressionMethod::None, + write_format); filename_generator.setVersion(last_version + 1); filename_generator.setCompressionMethod(compression_method); @@ -803,7 +815,8 @@ void alter( metadata_json_generator.generateDropColumnMetadata(params[0].column_name); break; case AlterCommand::Type::MODIFY_COLUMN: - metadata_json_generator.generateModifyColumnMetadata(params[0].column_name, params[0].data_type); + if (!metadata_json_generator.generateModifyColumnMetadata(params[0].column_name, params[0].data_type)) + return; break; case AlterCommand::Type::RENAME_COLUMN: metadata_json_generator.generateRenameColumnMetadata(params[0].column_name, params[0].rename_to); @@ -843,7 +856,7 @@ void alter( context, data_lake_settings[DataLakeStorageSetting::iceberg_use_version_hint])) { - ++i; + LOG_WARNING(log, "Iceberg alter: failed to write metadata (attempt {}), retrying", i + 1); continue; } @@ -851,24 +864,45 @@ void alter( { auto catalog_filename = persistent_table_components.path_resolver.resolveForCatalog(metadata_info.path); const auto & [namespace_name, table_name] = DataLake::parseTableName(storage_id.getTableName()); - if (!catalog->updateSchema(namespace_name, table_name, catalog_filename, new_schema, previous_schema_id)) + if (!catalog->updateSchema(namespace_name, table_name, catalog_filename, new_schema, previous_schema_id, metadata)) { - ++i; - continue; + auto storage_metadata_name = persistent_table_components.path_resolver.resolve(metadata_info.path); + String orphan_cleanup_error; + try + { + fiu_do_on(FailPoints::iceberg_alter_orphan_metadata_cleanup_fail, + { + throw Exception(ErrorCodes::DATALAKE_DATABASE_ERROR, "Failpoint: orphan metadata cleanup failed"); + }); + object_storage->removeObjectIfExists(StoredObject(storage_metadata_name)); + } + catch (...) + { + orphan_cleanup_error = getCurrentExceptionMessage(false); + tryLogCurrentException(log, "Iceberg alter: failed to remove orphan metadata file after catalog commit failure"); + } + if (orphan_cleanup_error.empty()) + { + throw Exception( + ErrorCodes::DATALAKE_DATABASE_ERROR, + "Iceberg alter: catalog commit failed for '{}' after metadata file was written successfully", + catalog_filename); + } + throw Exception( + ErrorCodes::DATALAKE_DATABASE_ERROR, + "Iceberg alter: catalog commit failed for '{}' after metadata file was written successfully. " + "Failed to remove orphan metadata file '{}': {}", + catalog_filename, + storage_metadata_name, + orphan_cleanup_error); } } - succeeded = true; - break; + persistent_table_components.invalidateMetadataCache(); + return; } - if (!succeeded) - throw Exception(ErrorCodes::LIMIT_EXCEEDED, "Too many unsuccessed retries to alter iceberg table"); - - /// Invalidate the metadata files cache so that subsequent operations on this table see the - /// schema we just wrote. See `PersistentTableComponents::invalidateMetadataCache` for the - /// rationale. - persistent_table_components.invalidateMetadataCache(); + throw Exception(ErrorCodes::LIMIT_EXCEEDED, "Too many unsuccessful retries to alter iceberg table"); } #endif diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp index f7a3f164ac1d..4c39350ebb19 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp @@ -1210,7 +1210,8 @@ MetadataFileWithInfo getLatestOrExplicitMetadataFileAndVersion( const std::optional & table_uuid, CompressionMethod known_compression_method, bool force_fetch_latest_metadata, - bool ignore_explicit_metadata_file_path) + bool ignore_explicit_metadata_file_path, + bool select_by_table_uuid) { if (data_lake_settings[DataLakeStorageSetting::iceberg_metadata_file_path].changed && !ignore_explicit_metadata_file_path) { @@ -1270,7 +1271,14 @@ MetadataFileWithInfo getLatestOrExplicitMetadataFileAndVersion( { return getLatestMetadataFileAndVersion( - object_storage, table_path, data_lake_settings, metadata_cache, local_context, table_uuid, false, force_fetch_latest_metadata); + object_storage, + table_path, + data_lake_settings, + metadata_cache, + local_context, + table_uuid, + select_by_table_uuid && table_uuid.has_value(), + force_fetch_latest_metadata); } } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.h index 43d2c040ad59..8fb909f328a9 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.h @@ -94,7 +94,8 @@ MetadataFileWithInfo getLatestOrExplicitMetadataFileAndVersion( const std::optional & table_uuid, CompressionMethod known_compression_method, bool force_fetch_latest_metadata = true, - bool ignore_explicit_metadata_file_path = false); + bool ignore_explicit_metadata_file_path = false, + bool select_by_table_uuid = false); std::pair parseTableSchemaV1Method(const Poco::JSON::Object::Ptr & metadata_object); std::pair parseTableSchemaV2Method(const Poco::JSON::Object::Ptr & metadata_object); diff --git a/tests/integration/test_database_iceberg/test.py b/tests/integration/test_database_iceberg/test.py index 31ec8882a357..3c167b73816f 100644 --- a/tests/integration/test_database_iceberg/test.py +++ b/tests/integration/test_database_iceberg/test.py @@ -13,10 +13,13 @@ from pyiceberg.catalog import load_catalog from pyiceberg.partitioning import PartitionField, PartitionSpec from pyiceberg.schema import Schema -from pyiceberg.table.sorting import SortField, SortOrder +from pyiceberg.table.sorting import SortField, SortOrder, UNSORTED_SORT_ORDER from pyiceberg.transforms import DayTransform, IdentityTransform from pyiceberg.types import ( DoubleType, + IntegerType, + LongType, + FloatType, NestedField, StringType, StructType, @@ -24,9 +27,11 @@ TimestamptzType ) +from minio import Minio from helpers.cluster import ClickHouseCluster from helpers.config_cluster import minio_secret_key, minio_access_key from helpers.client import QueryRuntimeException +from helpers.s3_tools import list_s3_objects BASE_URL = "http://rest:8181/v1" @@ -95,11 +100,12 @@ def create_table( schema=DEFAULT_SCHEMA, partition_spec=DEFAULT_PARTITION_SPEC, sort_order=DEFAULT_SORT_ORDER, + location="s3://warehouse-rest/data", ): return catalog.create_table( identifier=f"{namespace}.{table}", schema=schema, - location="s3://warehouse-rest/data", + location=location, partition_spec=partition_spec, sort_order=sort_order, ) @@ -1238,3 +1244,384 @@ def test_iceberg_file_progress_callback(started_cluster): f"`IcebergIterator::next` did not invoke the file-progress callback " f"(regression of PR #105413 wiring)." ) + + +def test_alter_drop_column_without_reload(started_cluster): + node = started_cluster.instances["node1"] + + test_ref = f"test_alter_drop_column_{uuid.uuid4()}" + table_name = f"{test_ref}_table" + root_namespace = f"{test_ref}_namespace" + + schema = Schema( + NestedField(field_id=1, name="x", field_type=StringType(), required=False), + NestedField(field_id=2, name="y", field_type=StringType(), required=False), + ) + + catalog = load_catalog_impl(started_cluster) + catalog.create_namespace(root_namespace) + create_table( + catalog, + root_namespace, + table_name, + schema, + PartitionSpec(), + DEFAULT_SORT_ORDER, + location=f"s3://warehouse-rest/data/{root_namespace}/{table_name}", + ) + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + node.query( + f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_name}` VALUES ('a', 'b');", + settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, + ) + assert ( + node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") + == "a\tb\n" + ) + + node.query( + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` DROP COLUMN y;", + settings={"allow_insert_into_iceberg": 1}, + ) + assert ( + node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") + == "a\n" + ) + assert "`y`" not in node.query( + f"SHOW CREATE TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}`" + ) + + +def test_alter_modify_column_rest_catalog(started_cluster): + node = started_cluster.instances["node1"] + + test_ref = f"test_alter_modify_column_{uuid.uuid4()}" + table_name = f"{test_ref}_table" + root_namespace = f"{test_ref}_namespace" + + schema = Schema( + NestedField(field_id=1, name="id", field_type=IntegerType(), required=False), + NestedField(field_id=2, name="value", field_type=StringType(), required=False), + ) + + catalog = load_catalog_impl(started_cluster) + catalog.create_namespace(root_namespace) + create_table( + catalog, + root_namespace, + table_name, + schema, + PartitionSpec(), + DEFAULT_SORT_ORDER, + location=f"s3://warehouse-rest/data/{root_namespace}/{table_name}", + ) + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + node.query( + f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_name}` VALUES (1, 'hello'), (2, 'world');", + settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, + ) + assert ( + node.query(f"SELECT id, value FROM {CATALOG_NAME}.`{root_namespace}.{table_name}` ORDER BY id") + == "1\thello\n2\tworld\n" + ) + + node.query( + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` ADD COLUMN newCol Nullable(Int64);", + settings={"allow_insert_into_iceberg": 1}, + ) + + node.query( + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` MODIFY COLUMN newCol Nullable(Int64);", + settings={"allow_insert_into_iceberg": 1}, + ) + + node.query( + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` MODIFY COLUMN id Nullable(Int64);", + settings={"allow_insert_into_iceberg": 1}, + ) + + assert ( + node.query(f"SELECT id, value FROM {CATALOG_NAME}.`{root_namespace}.{table_name}` ORDER BY id") + == "1\thello\n2\tworld\n" + ) + + node.query( + f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_name}` VALUES (3000000000, 'foo', NULL);", + settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, + ) + assert ( + node.query( + f"SELECT id, value, newCol FROM {CATALOG_NAME}.`{root_namespace}.{table_name}` ORDER BY id" + ) + == "1\thello\t\\N\n2\tworld\t\\N\n3000000000\tfoo\t\\N\n" + ) + + iceberg_table = catalog.load_table(f"{root_namespace}.{table_name}") + current_schema = iceberg_table.schema() + assert isinstance(current_schema.find_field("id").field_type, LongType) + assert isinstance(current_schema.find_field("newCol").field_type, LongType) + + +def test_alter_orphan_metadata_cleanup_on_catalog_failure(started_cluster): + node = started_cluster.instances["node1"] + + test_ref = f"test_alter_orphan_cleanup_{uuid.uuid4()}" + table_name = f"{test_ref}_table" + root_namespace = f"{test_ref}_namespace" + + schema = Schema( + NestedField(field_id=1, name="x", field_type=StringType(), required=False), + NestedField(field_id=2, name="y", field_type=StringType(), required=False), + ) + + catalog = load_catalog_impl(started_cluster) + catalog.create_namespace(root_namespace) + create_table( + catalog, + root_namespace, + table_name, + schema, + PartitionSpec(), + DEFAULT_SORT_ORDER, + location=f"s3://warehouse-rest/data/{root_namespace}/{table_name}", + ) + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + node.query( + f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_name}` VALUES ('a', 'b');", + settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, + ) + + iceberg_table = catalog.load_table(f"{root_namespace}.{table_name}") + metadata_location_before = iceberg_table.metadata_location + metadata_prefix = metadata_location_before.replace("s3://warehouse-rest/", "").rsplit("/", 1)[0] + "/" + + minio_client = Minio( + f"{started_cluster.get_instance_ip('minio')}:9000", + access_key=minio_access_key, + secret_key=minio_secret_key, + secure=False, + ) + + def count_metadata_files(): + return len( + [ + f + for f in list_s3_objects(minio_client, "warehouse-rest", prefix=metadata_prefix) + if f.endswith(".metadata.json") + ] + ) + + metadata_files_before = count_metadata_files() + + node.query("SYSTEM ENABLE FAILPOINT iceberg_alter_catalog_update_metadata_fail") + try: + with pytest.raises(QueryRuntimeException, match="catalog commit failed"): + node.query( + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` DROP COLUMN y;", + settings={"allow_insert_into_iceberg": 1}, + ) + finally: + node.query("SYSTEM DISABLE FAILPOINT iceberg_alter_catalog_update_metadata_fail") + + assert count_metadata_files() == metadata_files_before + catalog.load_table(f"{root_namespace}.{table_name}") + assert catalog.load_table(f"{root_namespace}.{table_name}").metadata_location == metadata_location_before + assert ( + node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") + == "a\tb\n" + ) + + +def test_alter_fails_when_metadata_not_initialized(started_cluster): + node = started_cluster.instances["node1"] + + test_ref = f"test_alter_uninit_metadata_{uuid.uuid4()}" + table_name = f"{test_ref}_table" + root_namespace = f"{test_ref}_namespace" + + schema = Schema( + NestedField(field_id=1, name="x", field_type=StringType(), required=False), + NestedField(field_id=2, name="y", field_type=StringType(), required=False), + ) + + catalog = load_catalog_impl(started_cluster) + catalog.create_namespace(root_namespace) + create_table( + catalog, + root_namespace, + table_name, + schema, + PartitionSpec(), + DEFAULT_SORT_ORDER, + location=f"s3://warehouse-rest/data/{root_namespace}/{table_name}", + ) + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + + node.query("SYSTEM ENABLE FAILPOINT datalake_iceberg_metadata_create_fail") + try: + node.query(f"DROP DATABASE IF EXISTS {CATALOG_NAME}") + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + + with pytest.raises(QueryRuntimeException, match="Metadata is not initialized"): + node.query( + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` DROP COLUMN y;", + settings={"allow_insert_into_iceberg": 1}, + ) + finally: + node.query("SYSTEM DISABLE FAILPOINT datalake_iceberg_metadata_create_fail") + node.query(f"DROP DATABASE IF EXISTS {CATALOG_NAME}") + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + + +def test_alter_orphan_cleanup_failure_reported(started_cluster): + node = started_cluster.instances["node1"] + + test_ref = f"test_alter_orphan_cleanup_fail_{uuid.uuid4()}" + table_name = f"{test_ref}_table" + root_namespace = f"{test_ref}_namespace" + + schema = Schema( + NestedField(field_id=1, name="x", field_type=StringType(), required=False), + NestedField(field_id=2, name="y", field_type=StringType(), required=False), + ) + + catalog = load_catalog_impl(started_cluster) + catalog.create_namespace(root_namespace) + create_table( + catalog, + root_namespace, + table_name, + schema, + PartitionSpec(), + DEFAULT_SORT_ORDER, + location=f"s3://warehouse-rest/data/{root_namespace}/{table_name}", + ) + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + node.query( + f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_name}` VALUES ('a', 'b');", + settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, + ) + + node.query("SYSTEM ENABLE FAILPOINT iceberg_alter_catalog_update_metadata_fail") + node.query("SYSTEM ENABLE FAILPOINT iceberg_alter_orphan_metadata_cleanup_fail") + try: + with pytest.raises(QueryRuntimeException) as exc_info: + node.query( + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` DROP COLUMN y;", + settings={"allow_insert_into_iceberg": 1}, + ) + error = exc_info.value.args[0].lower() + assert "catalog commit failed" in error + assert "failed to remove orphan metadata file" in error + finally: + node.query("SYSTEM DISABLE FAILPOINT iceberg_alter_orphan_metadata_cleanup_fail") + node.query("SYSTEM DISABLE FAILPOINT iceberg_alter_catalog_update_metadata_fail") + + +def test_alter_sequential_add_drop_shared_location(started_cluster): + """ + Two Iceberg tables share the same storage location, so their metadata files + land in the same folder. Running a sequence of ALTER ADD/DROP COLUMN + statements on one table must keep selecting that table's own metadata + (by table-uuid) instead of the globally highest-version metadata file. + """ + node = started_cluster.instances["node1"] + + test_ref = f"test_alter_sequential_{uuid.uuid4()}" + table_a = f"{test_ref}_table_a" + table_b = f"{test_ref}_table_b" + root_namespace = f"{test_ref}_namespace" + + schema = Schema( + NestedField(field_id=1, name="x", field_type=StringType(), required=False), + ) + + shared_location = f"s3://warehouse-rest/data/{root_namespace}/shared" + + catalog = load_catalog_impl(started_cluster) + catalog.create_namespace(root_namespace) + create_table( + catalog, + root_namespace, + table_a, + schema, + PartitionSpec(), + UNSORTED_SORT_ORDER, + location=shared_location, + ) + create_table( + catalog, + root_namespace, + table_b, + schema, + PartitionSpec(), + UNSORTED_SORT_ORDER, + location=shared_location, + ) + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + + node.query( + f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_a}` VALUES ('a');", + settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, + ) + node.query( + f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_b}` VALUES ('b');", + settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, + ) + + table_b_columns = ["b_col1", "b_col2", "b_col3", "b_col4", "b_col5", "b_col6"] + for column in table_b_columns: + node.query( + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_b}` ADD COLUMN IF NOT EXISTS {column} Nullable(String);", + settings={"allow_insert_into_iceberg": 1}, + ) + + alter_statements = [ + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` ADD COLUMN IF NOT EXISTS name Nullable(String);", + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` ADD COLUMN IF NOT EXISTS age Nullable(UInt64);", + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` ADD COLUMN IF NOT EXISTS email Nullable(String);", + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` ADD COLUMN IF NOT EXISTS `double` Nullable(Float64);", + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` ADD COLUMN IF NOT EXISTS `integer` Nullable(UInt64);", + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` DROP COLUMN IF EXISTS name;", + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` DROP COLUMN IF EXISTS age;", + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` DROP COLUMN IF EXISTS email;", + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` DROP COLUMN IF EXISTS `double`;", + f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` DROP COLUMN IF EXISTS `integer`;", + ] + for i, statement in enumerate(alter_statements): + if i > 0: + time.sleep(2) + node.query(statement, settings={"allow_insert_into_iceberg": 1}) + + show_create_a = node.query( + f"SHOW CREATE TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}`" + ) + assert "`x`" in show_create_a + for column in ["name", "age", "email", "double", "integer"]: + assert f"`{column}`" not in show_create_a + + assert ( + node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_a}`") == "a\n" + ) + + schema_a = catalog.load_table(f"{root_namespace}.{table_a}").schema() + assert [field.name for field in schema_a.fields] == ["x"] + + show_create_b = node.query( + f"SHOW CREATE TABLE {CATALOG_NAME}.`{root_namespace}.{table_b}`" + ) + for column in table_b_columns: + assert f"`{column}`" in show_create_b + + assert ( + node.query(f"SELECT x FROM {CATALOG_NAME}.`{root_namespace}.{table_b}`") == "b\n" + ) + + schema_b = catalog.load_table(f"{root_namespace}.{table_b}").schema() + assert [field.name for field in schema_b.fields] == ["x"] + table_b_columns diff --git a/tests/integration/test_storage_iceberg_no_spark/test_writes_add_column.py b/tests/integration/test_storage_iceberg_no_spark/test_writes_add_column.py new file mode 100644 index 000000000000..630cafdf3a06 --- /dev/null +++ b/tests/integration/test_storage_iceberg_no_spark/test_writes_add_column.py @@ -0,0 +1,72 @@ +import pytest + +from helpers.iceberg_utils import ( + create_iceberg_table, + get_uuid_str, +) + +INSERT_SETTINGS = {"allow_insert_into_iceberg": 1} + + +@pytest.mark.parametrize("format_version", [1, 2]) +@pytest.mark.parametrize("storage_type", ["local", "s3"]) +def test_add_column_basic(started_cluster_iceberg_no_spark, format_version, storage_type): + """ADD COLUMN (nullable): existing rows read with NULL in the new column; new inserts can set it.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + TABLE_NAME = "test_add_column_basic_" + storage_type + "_" + get_uuid_str() + + create_iceberg_table( + storage_type, + instance, + TABLE_NAME, + started_cluster_iceberg_no_spark, + "(id Int32, value Nullable(String))", + format_version, + ) + + instance.query(f"INSERT INTO {TABLE_NAME} VALUES (1, 'hello'), (2, 'world');", settings=INSERT_SETTINGS) + assert instance.query(f"SELECT id, value FROM {TABLE_NAME} ORDER BY id") == "1\thello\n2\tworld\n" + + instance.query(f"ALTER TABLE {TABLE_NAME} ADD COLUMN extra Nullable(Int32);", settings=INSERT_SETTINGS) + + assert instance.query(f"SELECT id, value, extra FROM {TABLE_NAME} ORDER BY id") == ( + "1\thello\t\\N\n2\tworld\t\\N\n" + ) + + instance.query(f"INSERT INTO {TABLE_NAME} VALUES (3, 'foo', 7);", settings=INSERT_SETTINGS) + assert instance.query(f"SELECT id, value, extra FROM {TABLE_NAME} ORDER BY id") == ( + "1\thello\t\\N\n2\tworld\t\\N\n3\tfoo\t7\n" + ) + + +@pytest.mark.parametrize("format_version", [1, 2]) +@pytest.mark.parametrize("storage_type", ["local", "s3"]) +def test_add_column_errors(started_cluster_iceberg_no_spark, format_version, storage_type): + """Non-nullable ADD COLUMN and duplicate name must fail; schema unchanged.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + TABLE_NAME = "test_add_column_errors_" + storage_type + "_" + get_uuid_str() + + create_iceberg_table( + storage_type, + instance, + TABLE_NAME, + started_cluster_iceberg_no_spark, + "(id Int32, value Nullable(String))", + format_version, + ) + + error = instance.query_and_get_error( + f"ALTER TABLE {TABLE_NAME} ADD COLUMN bad Int32;", + settings=INSERT_SETTINGS, + ) + assert "non-nullable" in error.lower() or "doesn't allow" in error.lower() + + error = instance.query_and_get_error( + f"ALTER TABLE {TABLE_NAME} ADD COLUMN value Nullable(Int32);", + settings=INSERT_SETTINGS, + ) + assert "DUPLICATE_COLUMN" in error or "already exists" in error + + assert instance.query( + f"SELECT name FROM system.columns WHERE database = currentDatabase() AND table = '{TABLE_NAME}' ORDER BY name" + ) == "id\nvalue\n" diff --git a/tests/integration/test_storage_iceberg_no_spark/test_writes_drop_column.py b/tests/integration/test_storage_iceberg_no_spark/test_writes_drop_column.py new file mode 100644 index 000000000000..f29f8903e2c6 --- /dev/null +++ b/tests/integration/test_storage_iceberg_no_spark/test_writes_drop_column.py @@ -0,0 +1,62 @@ +import pytest + +from helpers.iceberg_utils import ( + create_iceberg_table, + get_uuid_str, +) + +INSERT_SETTINGS = {"allow_insert_into_iceberg": 1} + + +@pytest.mark.parametrize("format_version", [1, 2]) +@pytest.mark.parametrize("storage_type", ["local", "s3"]) +def test_drop_column_basic(started_cluster_iceberg_no_spark, format_version, storage_type): + """DROP COLUMN removes the column from reads and inserts; remaining columns unchanged.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + TABLE_NAME = "test_drop_column_basic_" + storage_type + "_" + get_uuid_str() + + create_iceberg_table( + storage_type, + instance, + TABLE_NAME, + started_cluster_iceberg_no_spark, + "(id Int32, value Nullable(String))", + format_version, + ) + + instance.query(f"INSERT INTO {TABLE_NAME} VALUES (1, 'hello'), (2, 'world');", settings=INSERT_SETTINGS) + assert instance.query(f"SELECT id, value FROM {TABLE_NAME} ORDER BY id") == "1\thello\n2\tworld\n" + + instance.query(f"ALTER TABLE {TABLE_NAME} DROP COLUMN value;", settings=INSERT_SETTINGS) + + assert instance.query(f"SELECT id FROM {TABLE_NAME} ORDER BY id") == "1\n2\n" + + instance.query(f"INSERT INTO {TABLE_NAME} VALUES (3);", settings=INSERT_SETTINGS) + assert instance.query(f"SELECT id FROM {TABLE_NAME} ORDER BY id") == "1\n2\n3\n" + + +@pytest.mark.parametrize("format_version", [1, 2]) +@pytest.mark.parametrize("storage_type", ["local", "s3"]) +def test_drop_column_errors(started_cluster_iceberg_no_spark, format_version, storage_type): + """Dropping a non-existent column must fail; table structure unchanged.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + TABLE_NAME = "test_drop_column_errors_" + storage_type + "_" + get_uuid_str() + + create_iceberg_table( + storage_type, + instance, + TABLE_NAME, + started_cluster_iceberg_no_spark, + "(id Int32, value Nullable(String))", + format_version, + ) + + error = instance.query_and_get_error( + f"ALTER TABLE {TABLE_NAME} DROP COLUMN nonexistent;", + settings=INSERT_SETTINGS, + ) + assert "nonexistent" in error + + assert instance.query( + f"SELECT name FROM system.columns WHERE database = currentDatabase() AND table = '{TABLE_NAME}' ORDER BY name" + ) == "id\nvalue\n" diff --git a/tests/integration/test_storage_iceberg_no_spark/test_writes_modify_column.py b/tests/integration/test_storage_iceberg_no_spark/test_writes_modify_column.py new file mode 100644 index 000000000000..314927cd586b --- /dev/null +++ b/tests/integration/test_storage_iceberg_no_spark/test_writes_modify_column.py @@ -0,0 +1,70 @@ +import pytest + +from helpers.iceberg_utils import ( + create_iceberg_table, + get_uuid_str, +) + +INSERT_SETTINGS = {"allow_insert_into_iceberg": 1} + + +@pytest.mark.parametrize("format_version", [1, 2]) +@pytest.mark.parametrize("storage_type", ["local", "s3"]) +def test_modify_column_basic(started_cluster_iceberg_no_spark, format_version, storage_type): + """Widen Int32 to Int64 (Iceberg int→long); existing and new rows read correctly.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + TABLE_NAME = "test_modify_column_basic_" + storage_type + "_" + get_uuid_str() + + create_iceberg_table( + storage_type, + instance, + TABLE_NAME, + started_cluster_iceberg_no_spark, + "(id Int32, value Nullable(String))", + format_version, + ) + + instance.query(f"INSERT INTO {TABLE_NAME} VALUES (1, 'hello'), (2, 'world');", settings=INSERT_SETTINGS) + assert instance.query(f"SELECT id, value FROM {TABLE_NAME} ORDER BY id") == "1\thello\n2\tworld\n" + + instance.query(f"ALTER TABLE {TABLE_NAME} MODIFY COLUMN id Int64;", settings=INSERT_SETTINGS) + + assert instance.query(f"SELECT id, value FROM {TABLE_NAME} ORDER BY id") == "1\thello\n2\tworld\n" + + instance.query(f"INSERT INTO {TABLE_NAME} VALUES (3000000000, 'foo');", settings=INSERT_SETTINGS) + assert instance.query(f"SELECT id, value FROM {TABLE_NAME} ORDER BY id") == "1\thello\n2\tworld\n3000000000\tfoo\n" + + +@pytest.mark.parametrize("format_version", [1, 2]) +@pytest.mark.parametrize("storage_type", ["local", "s3"]) +def test_modify_column_errors(started_cluster_iceberg_no_spark, format_version, storage_type): + """Invalid schema evolution (e.g. String→Int64) must fail; columns unchanged.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + TABLE_NAME = "test_modify_column_errors_" + storage_type + "_" + get_uuid_str() + + create_iceberg_table( + storage_type, + instance, + TABLE_NAME, + started_cluster_iceberg_no_spark, + "(id Int32, value Nullable(String))", + format_version, + ) + + error = instance.query_and_get_error( + f"ALTER TABLE {TABLE_NAME} MODIFY COLUMN value Int64;", + settings=INSERT_SETTINGS, + ) + el = error.lower() + # String→integer: mismatched Poco::Var kinds in checkValidSchemaEvolution → BadCastException + assert ( + "bad cast" in el + or "can not convert" in el + or "cannot convert" in el + or "schema evolution" in el + or "doesn't allow" in el + ) + + assert instance.query( + f"SELECT name FROM system.columns WHERE database = currentDatabase() AND table = '{TABLE_NAME}' ORDER BY name" + ) == "id\nvalue\n" From 0eb31fc3534628cf1eb82186269cfe8e19179f67 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Thu, 6 Aug 2026 05:37:42 +0200 Subject: [PATCH 02/18] fix medium defects with retries --- .../DataLakes/Iceberg/Mutations.cpp | 41 +++++++------------ .../integration/test_database_iceberg/test.py | 7 +--- 2 files changed, 17 insertions(+), 31 deletions(-) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp index 02e698c82170..274757c2adc9 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp @@ -866,35 +866,24 @@ void alter( const auto & [namespace_name, table_name] = DataLake::parseTableName(storage_id.getTableName()); if (!catalog->updateSchema(namespace_name, table_name, catalog_filename, new_schema, previous_schema_id, metadata)) { - auto storage_metadata_name = persistent_table_components.path_resolver.resolve(metadata_info.path); - String orphan_cleanup_error; - try + if (!catalog_writes_metadata_file) { - fiu_do_on(FailPoints::iceberg_alter_orphan_metadata_cleanup_fail, + try { - throw Exception(ErrorCodes::DATALAKE_DATABASE_ERROR, "Failpoint: orphan metadata cleanup failed"); - }); - object_storage->removeObjectIfExists(StoredObject(storage_metadata_name)); - } - catch (...) - { - orphan_cleanup_error = getCurrentExceptionMessage(false); - tryLogCurrentException(log, "Iceberg alter: failed to remove orphan metadata file after catalog commit failure"); - } - if (orphan_cleanup_error.empty()) - { - throw Exception( - ErrorCodes::DATALAKE_DATABASE_ERROR, - "Iceberg alter: catalog commit failed for '{}' after metadata file was written successfully", - catalog_filename); + fiu_do_on(FailPoints::iceberg_alter_orphan_metadata_cleanup_fail, + { + throw Exception(ErrorCodes::DATALAKE_DATABASE_ERROR, "Failpoint: orphan metadata cleanup failed"); + }); + auto storage_metadata_name = persistent_table_components.path_resolver.resolve(metadata_info.path); + object_storage->removeObjectIfExists(StoredObject(storage_metadata_name)); + } + catch (...) + { + tryLogCurrentException(log, "Iceberg alter: failed to remove orphan metadata file after catalog commit failure"); + } } - throw Exception( - ErrorCodes::DATALAKE_DATABASE_ERROR, - "Iceberg alter: catalog commit failed for '{}' after metadata file was written successfully. " - "Failed to remove orphan metadata file '{}': {}", - catalog_filename, - storage_metadata_name, - orphan_cleanup_error); + LOG_WARNING(log, "Iceberg alter: catalog commit failed (attempt {}), retrying", i + 1); + continue; } } diff --git a/tests/integration/test_database_iceberg/test.py b/tests/integration/test_database_iceberg/test.py index 0e501fe8deb9..ebb43069f96c 100644 --- a/tests/integration/test_database_iceberg/test.py +++ b/tests/integration/test_database_iceberg/test.py @@ -1419,7 +1419,7 @@ def count_metadata_files(): node.query("SYSTEM ENABLE FAILPOINT iceberg_alter_catalog_update_metadata_fail") try: - with pytest.raises(QueryRuntimeException, match="catalog commit failed"): + with pytest.raises(QueryRuntimeException, match="unsuccessful retries"): node.query( f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` DROP COLUMN y;", settings={"allow_insert_into_iceberg": 1}, @@ -1511,14 +1511,11 @@ def test_alter_orphan_cleanup_failure_reported(started_cluster): node.query("SYSTEM ENABLE FAILPOINT iceberg_alter_catalog_update_metadata_fail") node.query("SYSTEM ENABLE FAILPOINT iceberg_alter_orphan_metadata_cleanup_fail") try: - with pytest.raises(QueryRuntimeException) as exc_info: + with pytest.raises(QueryRuntimeException, match="unsuccessful retries"): node.query( f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` DROP COLUMN y;", settings={"allow_insert_into_iceberg": 1}, ) - error = exc_info.value.args[0].lower() - assert "catalog commit failed" in error - assert "failed to remove orphan metadata file" in error finally: node.query("SYSTEM DISABLE FAILPOINT iceberg_alter_orphan_metadata_cleanup_fail") node.query("SYSTEM DISABLE FAILPOINT iceberg_alter_catalog_update_metadata_fail") From f76fd1ddf412edc5cac24929c78fd9a6ee42e69b Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Thu, 6 Aug 2026 06:25:41 +0200 Subject: [PATCH 03/18] Added support for boolean and decimal --- .../DataLakes/Iceberg/Mutations.cpp | 65 +-- .../ObjectStorage/DataLakes/Iceberg/Utils.cpp | 20 + .../tests/gtest_iceberg_type_mapping.cpp | 128 +++++ .../integration/test_database_iceberg/test.py | 487 +----------------- .../test_writes_add_column.py | 34 ++ 5 files changed, 207 insertions(+), 527 deletions(-) create mode 100644 src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_type_mapping.cpp diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp index 274757c2adc9..8a04b5c83b3e 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp @@ -39,7 +39,6 @@ namespace DB::ErrorCodes { extern const int BAD_ARGUMENTS; -extern const int DATALAKE_DATABASE_ERROR; extern const int LOGICAL_ERROR; extern const int LIMIT_EXCEEDED; } @@ -53,7 +52,6 @@ extern const DataLakeStorageSettingsString iceberg_metadata_file_path; namespace DB::FailPoints { extern const char iceberg_writes_cleanup[]; -extern const char iceberg_alter_orphan_metadata_cleanup_fail[]; } namespace DB::Iceberg @@ -592,8 +590,7 @@ void mutate( persistent_table_components.table_uuid, persistent_table_components.metadata_compression_method, /* force_fetch_latest_metadata */ true, - /* ignore_explicit_metadata_file_path */ true, - /* select_by_table_uuid */ true); + /* ignore_explicit_metadata_file_path */ true); FileNamesGenerator filename_generator(persistent_table_components.path_resolver.getTableLocation(), false, CompressionMethod::None, write_format); filename_generator.setVersion(last_version + 1); @@ -723,14 +720,11 @@ void alter( std::shared_ptr catalog) { if (params.size() != 1) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Iceberg alter supports exactly one command at a time, got {}", params.size()); + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Params with size 1 is not supported"); - /// The command was marked as a no-op by AlterCommands::prepare (e.g. RENAME/DROP COLUMN IF EXISTS - /// for a missing column, or ADD COLUMN IF NOT EXISTS for an existing one). - if (params[0].ignore) - return; - - for (size_t i = 0; i < MAX_TRANSACTION_RETRIES; ++i) + size_t i = 0; + bool succeeded = false; + while (i < MAX_TRANSACTION_RETRIES) { auto log = getLogger("IcebergMutations"); @@ -749,8 +743,7 @@ void alter( persistent_table_components.table_uuid, persistent_table_components.metadata_compression_method, /* force_fetch_latest_metadata */ true, - /* ignore_explicit_metadata_file_path */ true, - /* select_by_table_uuid */ true); + /* ignore_explicit_metadata_file_path */ true); last_version = last_version_info.version; metadata_path = last_version_info.path; compression_method = last_version_info.compression_method; @@ -783,18 +776,13 @@ void alter( persistent_table_components.table_uuid, persistent_table_components.metadata_compression_method, /* force_fetch_latest_metadata */ true, - /* ignore_explicit_metadata_file_path */ false, - /* select_by_table_uuid */ true); + /* ignore_explicit_metadata_file_path */ false); last_version = last_version_info.version; metadata_path = last_version_info.path; compression_method = last_version_info.compression_method; } - FileNamesGenerator filename_generator( - persistent_table_components.path_resolver.getTableLocation(), - catalog && catalog->isTransactional(), - CompressionMethod::None, - write_format); + FileNamesGenerator filename_generator(persistent_table_components.path_resolver.getTableLocation(), false, CompressionMethod::None, write_format); filename_generator.setVersion(last_version + 1); filename_generator.setCompressionMethod(compression_method); @@ -815,8 +803,7 @@ void alter( metadata_json_generator.generateDropColumnMetadata(params[0].column_name); break; case AlterCommand::Type::MODIFY_COLUMN: - if (!metadata_json_generator.generateModifyColumnMetadata(params[0].column_name, params[0].data_type)) - return; + metadata_json_generator.generateModifyColumnMetadata(params[0].column_name, params[0].data_type); break; case AlterCommand::Type::RENAME_COLUMN: metadata_json_generator.generateRenameColumnMetadata(params[0].column_name, params[0].rename_to); @@ -856,7 +843,7 @@ void alter( context, data_lake_settings[DataLakeStorageSetting::iceberg_use_version_hint])) { - LOG_WARNING(log, "Iceberg alter: failed to write metadata (attempt {}), retrying", i + 1); + ++i; continue; } @@ -864,34 +851,24 @@ void alter( { auto catalog_filename = persistent_table_components.path_resolver.resolveForCatalog(metadata_info.path); const auto & [namespace_name, table_name] = DataLake::parseTableName(storage_id.getTableName()); - if (!catalog->updateSchema(namespace_name, table_name, catalog_filename, new_schema, previous_schema_id, metadata)) + if (!catalog->updateSchema(namespace_name, table_name, catalog_filename, new_schema, previous_schema_id)) { - if (!catalog_writes_metadata_file) - { - try - { - fiu_do_on(FailPoints::iceberg_alter_orphan_metadata_cleanup_fail, - { - throw Exception(ErrorCodes::DATALAKE_DATABASE_ERROR, "Failpoint: orphan metadata cleanup failed"); - }); - auto storage_metadata_name = persistent_table_components.path_resolver.resolve(metadata_info.path); - object_storage->removeObjectIfExists(StoredObject(storage_metadata_name)); - } - catch (...) - { - tryLogCurrentException(log, "Iceberg alter: failed to remove orphan metadata file after catalog commit failure"); - } - } - LOG_WARNING(log, "Iceberg alter: catalog commit failed (attempt {}), retrying", i + 1); + ++i; continue; } } - persistent_table_components.invalidateMetadataCache(); - return; + succeeded = true; + break; } - throw Exception(ErrorCodes::LIMIT_EXCEEDED, "Too many unsuccessful retries to alter iceberg table"); + if (!succeeded) + throw Exception(ErrorCodes::LIMIT_EXCEEDED, "Too many unsuccessed retries to alter iceberg table"); + + /// Invalidate the metadata files cache so that subsequent operations on this table see the + /// schema we just wrote. See `PersistentTableComponents::invalidateMetadataCache` for the + /// rationale. + persistent_table_components.invalidateMetadataCache(); } #endif diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp index 2e02a002b237..4bd1c52b5ab6 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp @@ -510,6 +510,15 @@ std::pair getIcebergType(DataTypePtr type, Int32 & ite { switch (type->getTypeId()) { + case TypeIndex::UInt8: + { + if (isBool(type)) + return {"boolean", true}; + return {"int", true}; + } + case TypeIndex::Int8: + case TypeIndex::UInt16: + case TypeIndex::Int16: case TypeIndex::UInt32: case TypeIndex::Int32: return {"int", true}; @@ -536,6 +545,17 @@ std::pair getIcebergType(DataTypePtr type, Int32 & ite return {"string", true}; case TypeIndex::UUID: return {"uuid", true}; + case TypeIndex::Decimal32: + case TypeIndex::Decimal64: + case TypeIndex::Decimal128: + case TypeIndex::Decimal256: + { + Poco::JSON::Object::Ptr result = new Poco::JSON::Object; + result->set("type", "decimal"); + result->set("precision", static_cast(getDecimalPrecision(*type))); + result->set("scale", static_cast(getDecimalScale(*type))); + return {result, true}; + } case TypeIndex::Tuple: { auto type_tuple = std::static_pointer_cast(type); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_type_mapping.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_type_mapping.cpp new file mode 100644 index 000000000000..3f323df48811 --- /dev/null +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_type_mapping.cpp @@ -0,0 +1,128 @@ +#include "config.h" + +#if USE_AVRO + +#include + +#include +#include +#include +#include +#include +#include + +using namespace DB; +using namespace DB::Iceberg; + +TEST(IcebergTypeMapping, BoolMapsToBoolean) +{ + auto bool_type = DataTypeFactory::instance().get("Bool"); + Int32 iter = 0; + auto [iceberg_type, required] = getIcebergType(bool_type, iter); + ASSERT_TRUE(iceberg_type.isString()); + EXPECT_EQ(iceberg_type.extract(), "boolean"); + EXPECT_TRUE(required); +} + +TEST(IcebergTypeMapping, NullableBoolMapsToBoolean) +{ + auto bool_type = makeNullable(DataTypeFactory::instance().get("Bool")); + Int32 iter = 0; + auto [iceberg_type, required] = getIcebergType(bool_type, iter); + ASSERT_TRUE(iceberg_type.isString()); + EXPECT_EQ(iceberg_type.extract(), "boolean"); + EXPECT_FALSE(required); +} + +TEST(IcebergTypeMapping, UInt8MapsToInt) +{ + auto type = std::make_shared(); + Int32 iter = 0; + auto [iceberg_type, required] = getIcebergType(type, iter); + ASSERT_TRUE(iceberg_type.isString()); + EXPECT_EQ(iceberg_type.extract(), "int"); + EXPECT_TRUE(required); +} + +TEST(IcebergTypeMapping, Int8MapsToInt) +{ + auto type = std::make_shared(); + Int32 iter = 0; + auto [iceberg_type, required] = getIcebergType(type, iter); + ASSERT_TRUE(iceberg_type.isString()); + EXPECT_EQ(iceberg_type.extract(), "int"); +} + +TEST(IcebergTypeMapping, UInt16MapsToInt) +{ + auto type = std::make_shared(); + Int32 iter = 0; + auto [iceberg_type, required] = getIcebergType(type, iter); + ASSERT_TRUE(iceberg_type.isString()); + EXPECT_EQ(iceberg_type.extract(), "int"); +} + +TEST(IcebergTypeMapping, Int16MapsToInt) +{ + auto type = std::make_shared(); + Int32 iter = 0; + auto [iceberg_type, required] = getIcebergType(type, iter); + ASSERT_TRUE(iceberg_type.isString()); + EXPECT_EQ(iceberg_type.extract(), "int"); +} + +TEST(IcebergTypeMapping, Decimal32MapsToDecimal) +{ + auto type = std::make_shared>(9, 2); + Int32 iter = 0; + auto [iceberg_type, required] = getIcebergType(type, iter); + ASSERT_FALSE(iceberg_type.isString()); + auto obj = iceberg_type.extract(); + ASSERT_TRUE(obj); + EXPECT_EQ(obj->getValue("type"), "decimal"); + EXPECT_EQ(obj->getValue("precision"), 9); + EXPECT_EQ(obj->getValue("scale"), 2); + EXPECT_TRUE(required); +} + +TEST(IcebergTypeMapping, Decimal64MapsToDecimal) +{ + auto type = std::make_shared>(18, 5); + Int32 iter = 0; + auto [iceberg_type, required] = getIcebergType(type, iter); + ASSERT_FALSE(iceberg_type.isString()); + auto obj = iceberg_type.extract(); + ASSERT_TRUE(obj); + EXPECT_EQ(obj->getValue("type"), "decimal"); + EXPECT_EQ(obj->getValue("precision"), 18); + EXPECT_EQ(obj->getValue("scale"), 5); +} + +TEST(IcebergTypeMapping, Decimal128MapsToDecimal) +{ + auto type = std::make_shared>(38, 10); + Int32 iter = 0; + auto [iceberg_type, required] = getIcebergType(type, iter); + ASSERT_FALSE(iceberg_type.isString()); + auto obj = iceberg_type.extract(); + ASSERT_TRUE(obj); + EXPECT_EQ(obj->getValue("type"), "decimal"); + EXPECT_EQ(obj->getValue("precision"), 38); + EXPECT_EQ(obj->getValue("scale"), 10); +} + +TEST(IcebergTypeMapping, NullableDecimalMapsToDecimalNotRequired) +{ + auto type = makeNullable(std::make_shared>(7, 3)); + Int32 iter = 0; + auto [iceberg_type, required] = getIcebergType(type, iter); + ASSERT_FALSE(iceberg_type.isString()); + auto obj = iceberg_type.extract(); + ASSERT_TRUE(obj); + EXPECT_EQ(obj->getValue("type"), "decimal"); + EXPECT_EQ(obj->getValue("precision"), 7); + EXPECT_EQ(obj->getValue("scale"), 3); + EXPECT_FALSE(required); +} + +#endif diff --git a/tests/integration/test_database_iceberg/test.py b/tests/integration/test_database_iceberg/test.py index ebb43069f96c..31ec8882a357 100644 --- a/tests/integration/test_database_iceberg/test.py +++ b/tests/integration/test_database_iceberg/test.py @@ -4,7 +4,7 @@ import time import uuid from concurrent.futures import ThreadPoolExecutor -from datetime import datetime, time as dtime +from datetime import datetime import pyarrow as pa import pytest @@ -13,26 +13,20 @@ from pyiceberg.catalog import load_catalog from pyiceberg.partitioning import PartitionField, PartitionSpec from pyiceberg.schema import Schema -from pyiceberg.table.sorting import SortField, SortOrder, UNSORTED_SORT_ORDER +from pyiceberg.table.sorting import SortField, SortOrder from pyiceberg.transforms import DayTransform, IdentityTransform from pyiceberg.types import ( DoubleType, - IntegerType, - LongType, - FloatType, NestedField, StringType, StructType, TimestampType, - TimestamptzType, - TimeType, + TimestamptzType ) -from minio import Minio from helpers.cluster import ClickHouseCluster from helpers.config_cluster import minio_secret_key, minio_access_key from helpers.client import QueryRuntimeException -from helpers.s3_tools import list_s3_objects BASE_URL = "http://rest:8181/v1" @@ -101,12 +95,11 @@ def create_table( schema=DEFAULT_SCHEMA, partition_spec=DEFAULT_PARTITION_SPEC, sort_order=DEFAULT_SORT_ORDER, - location="s3://warehouse-rest/data", ): return catalog.create_table( identifier=f"{namespace}.{table}", schema=schema, - location=location, + location="s3://warehouse-rest/data", partition_spec=partition_spec, sort_order=sort_order, ) @@ -1245,475 +1238,3 @@ def test_iceberg_file_progress_callback(started_cluster): f"`IcebergIterator::next` did not invoke the file-progress callback " f"(regression of PR #105413 wiring)." ) - - -def test_alter_drop_column_without_reload(started_cluster): - node = started_cluster.instances["node1"] - - test_ref = f"test_alter_drop_column_{uuid.uuid4()}" - table_name = f"{test_ref}_table" - root_namespace = f"{test_ref}_namespace" - - schema = Schema( - NestedField(field_id=1, name="x", field_type=StringType(), required=False), - NestedField(field_id=2, name="y", field_type=StringType(), required=False), - ) - - catalog = load_catalog_impl(started_cluster) - catalog.create_namespace(root_namespace) - create_table( - catalog, - root_namespace, - table_name, - schema, - PartitionSpec(), - DEFAULT_SORT_ORDER, - location=f"s3://warehouse-rest/data/{root_namespace}/{table_name}", - ) - - create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) - node.query( - f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_name}` VALUES ('a', 'b');", - settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, - ) - assert ( - node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") - == "a\tb\n" - ) - - node.query( - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` DROP COLUMN y;", - settings={"allow_insert_into_iceberg": 1}, - ) - assert ( - node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") - == "a\n" - ) - assert "`y`" not in node.query( - f"SHOW CREATE TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}`" - ) - - -def test_alter_modify_column_rest_catalog(started_cluster): - node = started_cluster.instances["node1"] - - test_ref = f"test_alter_modify_column_{uuid.uuid4()}" - table_name = f"{test_ref}_table" - root_namespace = f"{test_ref}_namespace" - - schema = Schema( - NestedField(field_id=1, name="id", field_type=IntegerType(), required=False), - NestedField(field_id=2, name="value", field_type=StringType(), required=False), - ) - - catalog = load_catalog_impl(started_cluster) - catalog.create_namespace(root_namespace) - create_table( - catalog, - root_namespace, - table_name, - schema, - PartitionSpec(), - DEFAULT_SORT_ORDER, - location=f"s3://warehouse-rest/data/{root_namespace}/{table_name}", - ) - - create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) - node.query( - f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_name}` VALUES (1, 'hello'), (2, 'world');", - settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, - ) - assert ( - node.query(f"SELECT id, value FROM {CATALOG_NAME}.`{root_namespace}.{table_name}` ORDER BY id") - == "1\thello\n2\tworld\n" - ) - - node.query( - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` ADD COLUMN newCol Nullable(Int64);", - settings={"allow_insert_into_iceberg": 1}, - ) - - node.query( - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` MODIFY COLUMN newCol Nullable(Int64);", - settings={"allow_insert_into_iceberg": 1}, - ) - - node.query( - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` MODIFY COLUMN id Nullable(Int64);", - settings={"allow_insert_into_iceberg": 1}, - ) - - assert ( - node.query(f"SELECT id, value FROM {CATALOG_NAME}.`{root_namespace}.{table_name}` ORDER BY id") - == "1\thello\n2\tworld\n" - ) - - node.query( - f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_name}` VALUES (3000000000, 'foo', NULL);", - settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, - ) - assert ( - node.query( - f"SELECT id, value, newCol FROM {CATALOG_NAME}.`{root_namespace}.{table_name}` ORDER BY id" - ) - == "1\thello\t\\N\n2\tworld\t\\N\n3000000000\tfoo\t\\N\n" - ) - - iceberg_table = catalog.load_table(f"{root_namespace}.{table_name}") - current_schema = iceberg_table.schema() - assert isinstance(current_schema.find_field("id").field_type, LongType) - assert isinstance(current_schema.find_field("newCol").field_type, LongType) - - -def test_alter_orphan_metadata_cleanup_on_catalog_failure(started_cluster): - node = started_cluster.instances["node1"] - - test_ref = f"test_alter_orphan_cleanup_{uuid.uuid4()}" - table_name = f"{test_ref}_table" - root_namespace = f"{test_ref}_namespace" - - schema = Schema( - NestedField(field_id=1, name="x", field_type=StringType(), required=False), - NestedField(field_id=2, name="y", field_type=StringType(), required=False), - ) - - catalog = load_catalog_impl(started_cluster) - catalog.create_namespace(root_namespace) - create_table( - catalog, - root_namespace, - table_name, - schema, - PartitionSpec(), - DEFAULT_SORT_ORDER, - location=f"s3://warehouse-rest/data/{root_namespace}/{table_name}", - ) - - create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) - node.query( - f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_name}` VALUES ('a', 'b');", - settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, - ) - - iceberg_table = catalog.load_table(f"{root_namespace}.{table_name}") - metadata_location_before = iceberg_table.metadata_location - metadata_prefix = metadata_location_before.replace("s3://warehouse-rest/", "").rsplit("/", 1)[0] + "/" - - minio_client = Minio( - f"{started_cluster.get_instance_ip('minio')}:9000", - access_key=minio_access_key, - secret_key=minio_secret_key, - secure=False, - ) - - def count_metadata_files(): - return len( - [ - f - for f in list_s3_objects(minio_client, "warehouse-rest", prefix=metadata_prefix) - if f.endswith(".metadata.json") - ] - ) - - metadata_files_before = count_metadata_files() - - node.query("SYSTEM ENABLE FAILPOINT iceberg_alter_catalog_update_metadata_fail") - try: - with pytest.raises(QueryRuntimeException, match="unsuccessful retries"): - node.query( - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` DROP COLUMN y;", - settings={"allow_insert_into_iceberg": 1}, - ) - finally: - node.query("SYSTEM DISABLE FAILPOINT iceberg_alter_catalog_update_metadata_fail") - - assert count_metadata_files() == metadata_files_before - catalog.load_table(f"{root_namespace}.{table_name}") - assert catalog.load_table(f"{root_namespace}.{table_name}").metadata_location == metadata_location_before - assert ( - node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") - == "a\tb\n" - ) - - -def test_alter_fails_when_metadata_not_initialized(started_cluster): - node = started_cluster.instances["node1"] - - test_ref = f"test_alter_uninit_metadata_{uuid.uuid4()}" - table_name = f"{test_ref}_table" - root_namespace = f"{test_ref}_namespace" - - schema = Schema( - NestedField(field_id=1, name="x", field_type=StringType(), required=False), - NestedField(field_id=2, name="y", field_type=StringType(), required=False), - ) - - catalog = load_catalog_impl(started_cluster) - catalog.create_namespace(root_namespace) - create_table( - catalog, - root_namespace, - table_name, - schema, - PartitionSpec(), - DEFAULT_SORT_ORDER, - location=f"s3://warehouse-rest/data/{root_namespace}/{table_name}", - ) - - create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) - - node.query("SYSTEM ENABLE FAILPOINT datalake_iceberg_metadata_create_fail") - try: - node.query(f"DROP DATABASE IF EXISTS {CATALOG_NAME}") - create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) - - with pytest.raises(QueryRuntimeException, match="Metadata is not initialized"): - node.query( - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` DROP COLUMN y;", - settings={"allow_insert_into_iceberg": 1}, - ) - finally: - node.query("SYSTEM DISABLE FAILPOINT datalake_iceberg_metadata_create_fail") - node.query(f"DROP DATABASE IF EXISTS {CATALOG_NAME}") - create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) - - -def test_alter_orphan_cleanup_failure_reported(started_cluster): - node = started_cluster.instances["node1"] - - test_ref = f"test_alter_orphan_cleanup_fail_{uuid.uuid4()}" - table_name = f"{test_ref}_table" - root_namespace = f"{test_ref}_namespace" - - schema = Schema( - NestedField(field_id=1, name="x", field_type=StringType(), required=False), - NestedField(field_id=2, name="y", field_type=StringType(), required=False), - ) - - catalog = load_catalog_impl(started_cluster) - catalog.create_namespace(root_namespace) - create_table( - catalog, - root_namespace, - table_name, - schema, - PartitionSpec(), - DEFAULT_SORT_ORDER, - location=f"s3://warehouse-rest/data/{root_namespace}/{table_name}", - ) - - create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) - node.query( - f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_name}` VALUES ('a', 'b');", - settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, - ) - - node.query("SYSTEM ENABLE FAILPOINT iceberg_alter_catalog_update_metadata_fail") - node.query("SYSTEM ENABLE FAILPOINT iceberg_alter_orphan_metadata_cleanup_fail") - try: - with pytest.raises(QueryRuntimeException, match="unsuccessful retries"): - node.query( - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` DROP COLUMN y;", - settings={"allow_insert_into_iceberg": 1}, - ) - finally: - node.query("SYSTEM DISABLE FAILPOINT iceberg_alter_orphan_metadata_cleanup_fail") - node.query("SYSTEM DISABLE FAILPOINT iceberg_alter_catalog_update_metadata_fail") - - -def test_alter_sequential_add_drop_shared_location(started_cluster): - """ - Two Iceberg tables share the same storage location, so their metadata files - land in the same folder. Running a sequence of ALTER ADD/DROP COLUMN - statements on one table must keep selecting that table's own metadata - (by table-uuid) instead of the globally highest-version metadata file. - """ - node = started_cluster.instances["node1"] - - test_ref = f"test_alter_sequential_{uuid.uuid4()}" - table_a = f"{test_ref}_table_a" - table_b = f"{test_ref}_table_b" - root_namespace = f"{test_ref}_namespace" - - schema = Schema( - NestedField(field_id=1, name="x", field_type=StringType(), required=False), - ) - - shared_location = f"s3://warehouse-rest/data/{root_namespace}/shared" - - catalog = load_catalog_impl(started_cluster) - catalog.create_namespace(root_namespace) - create_table( - catalog, - root_namespace, - table_a, - schema, - PartitionSpec(), - UNSORTED_SORT_ORDER, - location=shared_location, - ) - create_table( - catalog, - root_namespace, - table_b, - schema, - PartitionSpec(), - UNSORTED_SORT_ORDER, - location=shared_location, - ) - - create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) - - node.query( - f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_a}` VALUES ('a');", - settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, - ) - node.query( - f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_b}` VALUES ('b');", - settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, - ) - - table_b_columns = ["b_col1", "b_col2", "b_col3", "b_col4", "b_col5", "b_col6"] - for column in table_b_columns: - node.query( - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_b}` ADD COLUMN IF NOT EXISTS {column} Nullable(String);", - settings={"allow_insert_into_iceberg": 1}, - ) - - alter_statements = [ - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` ADD COLUMN IF NOT EXISTS name Nullable(String);", - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` ADD COLUMN IF NOT EXISTS age Nullable(UInt64);", - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` ADD COLUMN IF NOT EXISTS email Nullable(String);", - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` ADD COLUMN IF NOT EXISTS `double` Nullable(Float64);", - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` ADD COLUMN IF NOT EXISTS `integer` Nullable(UInt64);", - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` DROP COLUMN IF EXISTS name;", - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` DROP COLUMN IF EXISTS age;", - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` DROP COLUMN IF EXISTS email;", - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` DROP COLUMN IF EXISTS `double`;", - f"ALTER TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}` DROP COLUMN IF EXISTS `integer`;", - ] - for i, statement in enumerate(alter_statements): - if i > 0: - time.sleep(2) - node.query(statement, settings={"allow_insert_into_iceberg": 1}) - - show_create_a = node.query( - f"SHOW CREATE TABLE {CATALOG_NAME}.`{root_namespace}.{table_a}`" - ) - assert "`x`" in show_create_a - for column in ["name", "age", "email", "double", "integer"]: - assert f"`{column}`" not in show_create_a - - assert ( - node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_a}`") == "a\n" - ) - - schema_a = catalog.load_table(f"{root_namespace}.{table_a}").schema() - assert [field.name for field in schema_a.fields] == ["x"] - - show_create_b = node.query( - f"SHOW CREATE TABLE {CATALOG_NAME}.`{root_namespace}.{table_b}`" - ) - for column in table_b_columns: - assert f"`{column}`" in show_create_b - - assert ( - node.query(f"SELECT x FROM {CATALOG_NAME}.`{root_namespace}.{table_b}`") == "b\n" - ) - - schema_b = catalog.load_table(f"{root_namespace}.{table_b}").schema() - assert [field.name for field in schema_b.fields] == ["x"] + table_b_columns -def test_partitioning_by_time(started_cluster): - node = started_cluster.instances["node1"] - - test_ref = f"test_partitioning_by_time_{uuid.uuid4()}" - table_name = f"{test_ref}_table" - root_namespace = f"{test_ref}_namespace" - - namespace = f"{root_namespace}.A" - catalog = load_catalog_impl(started_cluster) - catalog.create_namespace(namespace) - - schema = Schema( - NestedField( - field_id=1, - name="key", - field_type=TimeType(), - required=False - ), - NestedField( - field_id=2, - name="value", - field_type=StringType(), - required=False, - ), - ) - - partition_spec = PartitionSpec( - PartitionField( - source_id=1, field_id=1000, transform=IdentityTransform(), name="partition_key" - ) - ) - - table = create_table(catalog, namespace, table_name, schema=schema, partition_spec=partition_spec) - data = [{"key": dtime(12,0,0), "value": "test1"}, - {"key": dtime(13,0,0), "value": "test2"}, - {"key": dtime(14,0,0), "value": "test3"}, - ] - df = pa.Table.from_pylist(data) - table.append(df) - - create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) - - assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{namespace}.{table_name}` ORDER BY key") == "12:00:00.000000\ttest1\n13:00:00.000000\ttest2\n14:00:00.000000\ttest3\n" - assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{namespace}.{table_name}` WHERE key = '13:00:00.000000' ORDER BY key") == "13:00:00.000000\ttest2\n" - assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{namespace}.{table_name}` WHERE key >= '13:00:00.000000' ORDER BY key") == "13:00:00.000000\ttest2\n14:00:00.000000\ttest3\n" - assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{namespace}.{table_name}` WHERE key <= '13:00:00.000000' ORDER BY key") == "12:00:00.000000\ttest1\n13:00:00.000000\ttest2\n" - - -def test_partitioning_by_string(started_cluster): - node = started_cluster.instances["node1"] - - test_ref = f"test_partitioning_by_string_{uuid.uuid4()}" - table_name = f"{test_ref}_table" - root_namespace = f"{test_ref}_namespace" - - namespace = f"{root_namespace}.A" - catalog = load_catalog_impl(started_cluster) - catalog.create_namespace(namespace) - - schema = Schema( - NestedField( - field_id=1, - name="key", - field_type=StringType(), - required=False - ), - NestedField( - field_id=2, - name="value", - field_type=StringType(), - required=False, - ), - NestedField( - field_id=3, - name="time_value", - field_type=TimeType(), - required=False, - ), - ) - - partition_spec = PartitionSpec( - PartitionField( - source_id=1, field_id=1000, transform=IdentityTransform(), name="partition_key" - ) - ) - - table = create_table(catalog, namespace, table_name, schema=schema, partition_spec=partition_spec) - data = [{"key": "a:b,c[d=e/f%g?h", "value": "test", "time_value": dtime(12,0,0)}] - df = pa.Table.from_pylist(data) - table.append(df) - - create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) - - assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{namespace}.{table_name}`") == "a:b,c[d=e/f%g?h\ttest\t12:00:00.000000\n" diff --git a/tests/integration/test_storage_iceberg_no_spark/test_writes_add_column.py b/tests/integration/test_storage_iceberg_no_spark/test_writes_add_column.py index 630cafdf3a06..b1c8a08407cc 100644 --- a/tests/integration/test_storage_iceberg_no_spark/test_writes_add_column.py +++ b/tests/integration/test_storage_iceberg_no_spark/test_writes_add_column.py @@ -70,3 +70,37 @@ def test_add_column_errors(started_cluster_iceberg_no_spark, format_version, sto assert instance.query( f"SELECT name FROM system.columns WHERE database = currentDatabase() AND table = '{TABLE_NAME}' ORDER BY name" ) == "id\nvalue\n" + + +@pytest.mark.parametrize("format_version", [1, 2]) +@pytest.mark.parametrize("storage_type", ["local", "s3"]) +def test_add_column_bool_and_decimal(started_cluster_iceberg_no_spark, format_version, storage_type): + """ADD COLUMN with Bool (Iceberg boolean) and Decimal (Iceberg decimal) types.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + TABLE_NAME = "test_add_column_bool_dec_" + storage_type + "_" + get_uuid_str() + + create_iceberg_table( + storage_type, + instance, + TABLE_NAME, + started_cluster_iceberg_no_spark, + "(id Int32, value Nullable(String))", + format_version, + ) + + instance.query(f"INSERT INTO {TABLE_NAME} VALUES (1, 'a'), (2, 'b');", settings=INSERT_SETTINGS) + + instance.query(f"ALTER TABLE {TABLE_NAME} ADD COLUMN flag Nullable(Bool);", settings=INSERT_SETTINGS) + instance.query(f"ALTER TABLE {TABLE_NAME} ADD COLUMN price Nullable(Decimal(10, 2));", settings=INSERT_SETTINGS) + + assert instance.query(f"SELECT id, value, flag, price FROM {TABLE_NAME} ORDER BY id") == ( + "1\ta\t\\N\t\\N\n2\tb\t\\N\t\\N\n" + ) + + instance.query( + f"INSERT INTO {TABLE_NAME} VALUES (3, 'c', true, 99.95), (4, 'd', false, 123.40);", + settings=INSERT_SETTINGS, + ) + assert instance.query(f"SELECT id, flag, price FROM {TABLE_NAME} ORDER BY id") == ( + "1\t\\N\t\\N\n2\t\\N\t\\N\n3\ttrue\t99.95\n4\tfalse\t123.40\n" + ) From bdb0d15f0d41dc231742110516cf9c402a7dab7c Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Thu, 6 Aug 2026 16:46:29 +0200 Subject: [PATCH 04/18] Add missing include to gtest_iceberg_type_mapping --- .../DataLakes/Iceberg/tests/gtest_iceberg_type_mapping.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_type_mapping.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_type_mapping.cpp index 3f323df48811..6f50d87e8bbe 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_type_mapping.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_type_mapping.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include From 04c009e36bf6a13296aa90e12bf30fc82b0c4b68 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Fri, 7 Aug 2026 03:13:19 +0200 Subject: [PATCH 05/18] Restored removed tests --- .../integration/test_database_iceberg/test.py | 102 +++++++++++++++++- 1 file changed, 100 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_database_iceberg/test.py b/tests/integration/test_database_iceberg/test.py index 31ec8882a357..c451a42d8d55 100644 --- a/tests/integration/test_database_iceberg/test.py +++ b/tests/integration/test_database_iceberg/test.py @@ -4,7 +4,7 @@ import time import uuid from concurrent.futures import ThreadPoolExecutor -from datetime import datetime +from datetime import datetime, time as dtime import pyarrow as pa import pytest @@ -21,7 +21,8 @@ StringType, StructType, TimestampType, - TimestamptzType + TimestamptzType, + TimeType, ) from helpers.cluster import ClickHouseCluster @@ -1238,3 +1239,100 @@ def test_iceberg_file_progress_callback(started_cluster): f"`IcebergIterator::next` did not invoke the file-progress callback " f"(regression of PR #105413 wiring)." ) + + +def test_partitioning_by_time(started_cluster): + node = started_cluster.instances["node1"] + + test_ref = f"test_partitioning_by_time_{uuid.uuid4()}" + table_name = f"{test_ref}_table" + root_namespace = f"{test_ref}_namespace" + + namespace = f"{root_namespace}.A" + catalog = load_catalog_impl(started_cluster) + catalog.create_namespace(namespace) + + schema = Schema( + NestedField( + field_id=1, + name="key", + field_type=TimeType(), + required=False + ), + NestedField( + field_id=2, + name="value", + field_type=StringType(), + required=False, + ), + ) + + partition_spec = PartitionSpec( + PartitionField( + source_id=1, field_id=1000, transform=IdentityTransform(), name="partition_key" + ) + ) + + table = create_table(catalog, namespace, table_name, schema=schema, partition_spec=partition_spec) + data = [{"key": dtime(12,0,0), "value": "test1"}, + {"key": dtime(13,0,0), "value": "test2"}, + {"key": dtime(14,0,0), "value": "test3"}, + ] + df = pa.Table.from_pylist(data) + table.append(df) + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + + assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{namespace}.{table_name}` ORDER BY key") == "12:00:00.000000\ttest1\n13:00:00.000000\ttest2\n14:00:00.000000\ttest3\n" + assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{namespace}.{table_name}` WHERE key = '13:00:00.000000' ORDER BY key") == "13:00:00.000000\ttest2\n" + assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{namespace}.{table_name}` WHERE key >= '13:00:00.000000' ORDER BY key") == "13:00:00.000000\ttest2\n14:00:00.000000\ttest3\n" + assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{namespace}.{table_name}` WHERE key <= '13:00:00.000000' ORDER BY key") == "12:00:00.000000\ttest1\n13:00:00.000000\ttest2\n" + + +def test_partitioning_by_string(started_cluster): + node = started_cluster.instances["node1"] + + test_ref = f"test_partitioning_by_string_{uuid.uuid4()}" + table_name = f"{test_ref}_table" + root_namespace = f"{test_ref}_namespace" + + namespace = f"{root_namespace}.A" + catalog = load_catalog_impl(started_cluster) + catalog.create_namespace(namespace) + + schema = Schema( + NestedField( + field_id=1, + name="key", + field_type=StringType(), + required=False + ), + NestedField( + field_id=2, + name="value", + field_type=StringType(), + required=False, + ), + NestedField( + field_id=3, + name="time_value", + field_type=TimeType(), + required=False, + ), + ) + + partition_spec = PartitionSpec( + PartitionField( + source_id=1, field_id=1000, transform=IdentityTransform(), name="partition_key" + ) + ) + + table = create_table(catalog, namespace, table_name, schema=schema, partition_spec=partition_spec) + data = [{"key": "a:b,c[d=e/f%g?h", "value": "test", "time_value": dtime(12,0,0)}] + df = pa.Table.from_pylist(data) + table.append(df) + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + + assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{namespace}.{table_name}`") == "a:b,c[d=e/f%g?h\ttest\t12:00:00.000000\n" + From 6e6f1f726e08da59769193b4ce9db0117e953239 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Fri, 7 Aug 2026 22:34:55 +0200 Subject: [PATCH 06/18] Fix to support decimal datatypes in iceberg --- .../DataLakes/Iceberg/MetadataGenerator.cpp | 26 +++++++++++++++ .../ObjectStorage/DataLakes/Iceberg/Utils.cpp | 8 +---- .../tests/gtest_iceberg_type_mapping.cpp | 33 +++++-------------- 3 files changed, 35 insertions(+), 32 deletions(-) diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp index f85b14764067..9a14cec78e9a 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp @@ -1,3 +1,4 @@ +#include #include #include @@ -54,6 +55,31 @@ bool checkValidSchemaEvolution(Poco::Dynamic::Var old_type, Poco::Dynamic::Var n return true; } + if (old_type.isString() && new_type.isString()) + { + auto old_str = old_type.extract(); + auto new_str = new_type.extract(); + if (old_str.starts_with("decimal(") && old_str.ends_with(')') + && new_str.starts_with("decimal(") && new_str.ends_with(')')) + { + auto parse = [](const String & s) -> std::pair + { + DB::ReadBufferFromString buf(std::string_view(s.begin() + 8, s.end() - 1)); + size_t p = 0, sc = 0; + readIntText(p, buf); + skipWhitespaceIfAny(buf); + assertChar(',', buf); + skipWhitespaceIfAny(buf); + tryReadIntText(sc, buf); + return {p, sc}; + }; + auto [old_precision, old_scale] = parse(old_str); + auto [new_precision, new_scale] = parse(new_str); + if (old_precision <= new_precision && old_scale <= new_scale) + return true; + } + } + if (!old_type.isString() && !new_type.isString()) { auto old_complex_type = old_type.extract(); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp index 0dd1491b1e0e..fcd7a1c59680 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp @@ -549,13 +549,7 @@ std::pair getIcebergType(DataTypePtr type, Int32 & ite case TypeIndex::Decimal64: case TypeIndex::Decimal128: case TypeIndex::Decimal256: - { - Poco::JSON::Object::Ptr result = new Poco::JSON::Object; - result->set("type", "decimal"); - result->set("precision", static_cast(getDecimalPrecision(*type))); - result->set("scale", static_cast(getDecimalScale(*type))); - return {result, true}; - } + return {"decimal(" + std::to_string(getDecimalPrecision(*type)) + ", " + std::to_string(getDecimalScale(*type)) + ")", true}; case TypeIndex::Tuple: { auto type_tuple = std::static_pointer_cast(type); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_type_mapping.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_type_mapping.cpp index 6f50d87e8bbe..0379efd819fd 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_type_mapping.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_type_mapping.cpp @@ -9,7 +9,6 @@ #include #include #include -#include #include using namespace DB; @@ -77,12 +76,8 @@ TEST(IcebergTypeMapping, Decimal32MapsToDecimal) auto type = std::make_shared>(9, 2); Int32 iter = 0; auto [iceberg_type, required] = getIcebergType(type, iter); - ASSERT_FALSE(iceberg_type.isString()); - auto obj = iceberg_type.extract(); - ASSERT_TRUE(obj); - EXPECT_EQ(obj->getValue("type"), "decimal"); - EXPECT_EQ(obj->getValue("precision"), 9); - EXPECT_EQ(obj->getValue("scale"), 2); + ASSERT_TRUE(iceberg_type.isString()); + EXPECT_EQ(iceberg_type.extract(), "decimal(9, 2)"); EXPECT_TRUE(required); } @@ -91,12 +86,8 @@ TEST(IcebergTypeMapping, Decimal64MapsToDecimal) auto type = std::make_shared>(18, 5); Int32 iter = 0; auto [iceberg_type, required] = getIcebergType(type, iter); - ASSERT_FALSE(iceberg_type.isString()); - auto obj = iceberg_type.extract(); - ASSERT_TRUE(obj); - EXPECT_EQ(obj->getValue("type"), "decimal"); - EXPECT_EQ(obj->getValue("precision"), 18); - EXPECT_EQ(obj->getValue("scale"), 5); + ASSERT_TRUE(iceberg_type.isString()); + EXPECT_EQ(iceberg_type.extract(), "decimal(18, 5)"); } TEST(IcebergTypeMapping, Decimal128MapsToDecimal) @@ -104,12 +95,8 @@ TEST(IcebergTypeMapping, Decimal128MapsToDecimal) auto type = std::make_shared>(38, 10); Int32 iter = 0; auto [iceberg_type, required] = getIcebergType(type, iter); - ASSERT_FALSE(iceberg_type.isString()); - auto obj = iceberg_type.extract(); - ASSERT_TRUE(obj); - EXPECT_EQ(obj->getValue("type"), "decimal"); - EXPECT_EQ(obj->getValue("precision"), 38); - EXPECT_EQ(obj->getValue("scale"), 10); + ASSERT_TRUE(iceberg_type.isString()); + EXPECT_EQ(iceberg_type.extract(), "decimal(38, 10)"); } TEST(IcebergTypeMapping, NullableDecimalMapsToDecimalNotRequired) @@ -117,12 +104,8 @@ TEST(IcebergTypeMapping, NullableDecimalMapsToDecimalNotRequired) auto type = makeNullable(std::make_shared>(7, 3)); Int32 iter = 0; auto [iceberg_type, required] = getIcebergType(type, iter); - ASSERT_FALSE(iceberg_type.isString()); - auto obj = iceberg_type.extract(); - ASSERT_TRUE(obj); - EXPECT_EQ(obj->getValue("type"), "decimal"); - EXPECT_EQ(obj->getValue("precision"), 7); - EXPECT_EQ(obj->getValue("scale"), 3); + ASSERT_TRUE(iceberg_type.isString()); + EXPECT_EQ(iceberg_type.extract(), "decimal(7, 3)"); EXPECT_FALSE(required); } From 8e9c6f77223463c501be8f7b397e705dfc9645fe Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Tue, 11 Aug 2026 18:29:23 +0200 Subject: [PATCH 07/18] Fix nullptr exception when there are no snapshots --- src/Core/SettingsChangesHistory.cpp | 1 + src/Databases/DataLake/GlueCatalog.cpp | 2 +- src/Databases/DataLake/GlueCatalog.h | 2 +- src/Databases/DataLake/ICatalog.cpp | 2 +- src/Databases/DataLake/RestCatalog.cpp | 77 +++++++++---------- .../DataLakes/Iceberg/Mutations.cpp | 53 ++++++++++++- .../integration/test_database_iceberg/test.py | 30 ++++++++ 7 files changed, 122 insertions(+), 45 deletions(-) diff --git a/src/Core/SettingsChangesHistory.cpp b/src/Core/SettingsChangesHistory.cpp index c2b62c209863..ca41902b67f5 100644 --- a/src/Core/SettingsChangesHistory.cpp +++ b/src/Core/SettingsChangesHistory.cpp @@ -43,6 +43,7 @@ const VersionToSettingsChangesMap & getSettingsChangesHistory() { {"analyzer_compatibility_allow_non_aggregate_in_having", false, false, "New compatibility setting. When enabled, the new analyzer mimics the legacy `HAVING`-to-`WHERE` rewrite for non-aggregate AND-conjuncts instead of raising `NOT_AN_AGGREGATE`."}, {"reserve_memory", 0, 0, "New setting to reserve memory for specific workload before starting a query."}, + {"iceberg_manifest_min_count_to_compact", 30, 30, "New setting to control manifest compaction for Iceberg tables."}, {"output_format_image_width", 1024, 1024, "New setting controlling the width of the output image for image output formats such as PNG."}, {"output_format_image_height", 1024, 1024, "New setting controlling the height of the output image for image output formats such as PNG."}, {"output_format_image_terminal_mode", "", "", "New setting controlling whether image output formats such as PNG are rendered directly to the terminal using an inline image protocol."}, diff --git a/src/Databases/DataLake/GlueCatalog.cpp b/src/Databases/DataLake/GlueCatalog.cpp index 966fd334a521..936d84af89a1 100644 --- a/src/Databases/DataLake/GlueCatalog.cpp +++ b/src/Databases/DataLake/GlueCatalog.cpp @@ -680,7 +680,7 @@ bool GlueCatalog::updateSchema( const String & new_metadata_path, Poco::JSON::Object::Ptr /*new_schema*/, Int32 /*previous_schema_id*/, - Poco::JSON::Object::Ptr /*full_metadata*/) const + Int32 /*new_last_column_id*/) const { return updateMetadata(namespace_name, table_name, new_metadata_path, nullptr); } diff --git a/src/Databases/DataLake/GlueCatalog.h b/src/Databases/DataLake/GlueCatalog.h index d5b566050469..e9304adaf252 100644 --- a/src/Databases/DataLake/GlueCatalog.h +++ b/src/Databases/DataLake/GlueCatalog.h @@ -74,7 +74,7 @@ class GlueCatalog final : public ICatalog, private DB::WithContext const String & new_metadata_path, Poco::JSON::Object::Ptr new_schema, Int32 previous_schema_id, - Poco::JSON::Object::Ptr full_metadata = nullptr) const override; + Int32 new_last_column_id) const override; void dropTable(const String & namespace_name, const String & table_name) const override; diff --git a/src/Databases/DataLake/ICatalog.cpp b/src/Databases/DataLake/ICatalog.cpp index 70eccb5fc113..e1225d7a9a6a 100644 --- a/src/Databases/DataLake/ICatalog.cpp +++ b/src/Databases/DataLake/ICatalog.cpp @@ -326,7 +326,7 @@ bool ICatalog::updateSchema( const String & /*new_metadata_path*/, Poco::JSON::Object::Ptr /*new_schema*/, Int32 /*previous_schema_id*/, - Poco::JSON::Object::Ptr /*full_metadata*/) const + Int32 /*new_last_column_id*/) const { throw DB::Exception(DB::ErrorCodes::NOT_IMPLEMENTED, "updateSchema is not implemented"); } diff --git a/src/Databases/DataLake/RestCatalog.cpp b/src/Databases/DataLake/RestCatalog.cpp index a19148b7e124..407c9a454388 100644 --- a/src/Databases/DataLake/RestCatalog.cpp +++ b/src/Databases/DataLake/RestCatalog.cpp @@ -59,6 +59,7 @@ namespace DB::ErrorCodes extern const int LOGICAL_ERROR; extern const int BAD_ARGUMENTS; extern const int FAULT_INJECTED; + extern const int NOT_IMPLEMENTED; } namespace DB::Setting @@ -1543,6 +1544,12 @@ void RestCatalog::createTable(const String & namespace_name, const String & tabl bool RestCatalog::updateMetadata(const String & namespace_name, const String & table_name, const String & /*new_metadata_path*/, Poco::JSON::Object::Ptr new_snapshot) const { + if (!new_snapshot) + throw Exception( + ErrorCodes::NOT_IMPLEMENTED, + "REST catalog does not support metadata-only updates without a snapshot " + "(required for EXPIRE SNAPSHOTS)"); + fiu_do_on(DB::FailPoints::iceberg_alter_catalog_update_metadata_fail, { return false; }); const std::string endpoint = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables" / table_name).generic_string(); @@ -1570,58 +1577,46 @@ bool RestCatalog::updateSchema( const String & /*new_metadata_path*/, Poco::JSON::Object::Ptr new_schema, Int32 previous_schema_id, - Poco::JSON::Object::Ptr full_metadata) const + Int32 new_last_column_id) const { const std::string endpoint = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables" / table_name).generic_string(); - Poco::JSON::Object::Ptr request_body; - - /// When full metadata is available, use the richer builder which handles - /// equivalent-schema dedup and sort-order reset. - if (full_metadata) + Poco::JSON::Object::Ptr request_body = new Poco::JSON::Object; { - request_body = buildUpdateMetadataRequestBody(namespace_name, table_name, full_metadata); - if (!request_body) - return true; + Poco::JSON::Object::Ptr identifier = new Poco::JSON::Object; + identifier->set("name", table_name); + Poco::JSON::Array::Ptr namespaces = new Poco::JSON::Array; + namespaces->add(namespace_name); + identifier->set("namespace", namespaces); + request_body->set("identifier", identifier); } - else + { - request_body = new Poco::JSON::Object; - { - Poco::JSON::Object::Ptr identifier = new Poco::JSON::Object; - identifier->set("name", table_name); - Poco::JSON::Array::Ptr namespaces = new Poco::JSON::Array; - namespaces->add(namespace_name); - identifier->set("namespace", namespaces); - request_body->set("identifier", identifier); - } + Poco::JSON::Object::Ptr requirement = new Poco::JSON::Object; + requirement->set("type", "assert-current-schema-id"); + requirement->set("current-schema-id", previous_schema_id); - { - Poco::JSON::Object::Ptr requirement = new Poco::JSON::Object; - requirement->set("type", "assert-current-schema-id"); - requirement->set("current-schema-id", previous_schema_id); + Poco::JSON::Array::Ptr requirements = new Poco::JSON::Array; + requirements->add(requirement); + request_body->set("requirements", requirements); + } - Poco::JSON::Array::Ptr requirements = new Poco::JSON::Array; - requirements->add(requirement); - request_body->set("requirements", requirements); + { + Poco::JSON::Array::Ptr updates = new Poco::JSON::Array; + { + Poco::JSON::Object::Ptr add_schema = new Poco::JSON::Object; + add_schema->set("action", "add-schema"); + add_schema->set("schema", new_schema); + add_schema->set("last-column-id", new_last_column_id); + updates->add(add_schema); } - { - Poco::JSON::Array::Ptr updates = new Poco::JSON::Array; - { - Poco::JSON::Object::Ptr add_schema = new Poco::JSON::Object; - add_schema->set("action", "add-schema"); - add_schema->set("schema", new_schema); - updates->add(add_schema); - } - { - Poco::JSON::Object::Ptr set_current_schema = new Poco::JSON::Object; - set_current_schema->set("action", "set-current-schema"); - set_current_schema->set("schema-id", -1); - updates->add(set_current_schema); - } - request_body->set("updates", updates); + Poco::JSON::Object::Ptr set_current_schema = new Poco::JSON::Object; + set_current_schema->set("action", "set-current-schema"); + set_current_schema->set("schema-id", -1); + updates->add(set_current_schema); } + request_body->set("updates", updates); } try diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp index 8a04b5c83b3e..2f2d5fc9aa4b 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp @@ -63,6 +63,54 @@ static constexpr const char * block_datafile_path = "_iceberg_metadata_file_path static constexpr const char * block_row_number = "_row_number"; static constexpr auto MAX_TRANSACTION_RETRIES = 100; +/// Walk an Iceberg type descriptor and return the highest field id found. +static Int32 getHighestFieldIdFromType(const Poco::Dynamic::Var & type_var) +{ + if (type_var.type() != typeid(Poco::JSON::Object::Ptr)) + return 0; + auto obj = type_var.extract(); + Int32 result = 0; + + auto type_str = obj->optValue(Iceberg::f_type, ""); + if (type_str == "struct") + { + auto fields = obj->getArray(Iceberg::f_fields); + for (UInt32 i = 0; i < fields->size(); ++i) + { + auto field = fields->getObject(i); + result = std::max(result, field->getValue(Iceberg::f_id)); + result = std::max(result, getHighestFieldIdFromType(field->get(Iceberg::f_type))); + } + } + else if (type_str == "list") + { + result = std::max(result, obj->getValue(Iceberg::f_element_id)); + result = std::max(result, getHighestFieldIdFromType(obj->get(Iceberg::f_element))); + } + else if (type_str == "map") + { + result = std::max(result, obj->getValue(Iceberg::f_key_id)); + result = std::max(result, obj->getValue(Iceberg::f_value_id)); + result = std::max(result, getHighestFieldIdFromType(obj->get(Iceberg::f_key))); + result = std::max(result, getHighestFieldIdFromType(obj->get(Iceberg::f_value))); + } + return result; +} + +/// Return the highest field id across all fields in an Iceberg schema object. +static Int32 getHighestFieldId(Poco::JSON::Object::Ptr schema) +{ + Int32 result = 0; + auto fields = schema->getArray(Iceberg::f_fields); + for (UInt32 i = 0; i < fields->size(); ++i) + { + auto field = fields->getObject(i); + result = std::max(result, field->getValue(Iceberg::f_id)); + result = std::max(result, getHighestFieldIdFromType(field->get(Iceberg::f_type))); + } + return result; +} + struct DeleteFileWriteResult { /// Metadata path (e.g. "wasb://container@account/table/data/uuid-deletes.parquet") @@ -851,7 +899,10 @@ void alter( { auto catalog_filename = persistent_table_components.path_resolver.resolveForCatalog(metadata_info.path); const auto & [namespace_name, table_name] = DataLake::parseTableName(storage_id.getTableName()); - if (!catalog->updateSchema(namespace_name, table_name, catalog_filename, new_schema, previous_schema_id)) + const auto new_last_column_id = std::max( + metadata->getValue(Iceberg::f_last_column_id), + getHighestFieldId(new_schema)); + if (!catalog->updateSchema(namespace_name, table_name, catalog_filename, new_schema, previous_schema_id, new_last_column_id)) { ++i; continue; diff --git a/tests/integration/test_database_iceberg/test.py b/tests/integration/test_database_iceberg/test.py index c451a42d8d55..5e139e26e954 100644 --- a/tests/integration/test_database_iceberg/test.py +++ b/tests/integration/test_database_iceberg/test.py @@ -1087,6 +1087,36 @@ def test_writes_schema_evolution(started_cluster): ) +def test_writes_schema_evolution_drop_last_column(started_cluster): + """DROP COLUMN of the highest-id column must not be rejected by the catalog. + + Reproducer for the bug where the REST add-schema update omitted + last-column-id, causing the catalog to derive it from the schema's + highestFieldId which decreases after dropping the last-added column. + """ + node = started_cluster.instances["node1"] + + test_ref = f"test_writes_schema_evolution_drop_last_{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_iceberg_database(started_cluster, node, CATALOG_NAME) + create_clickhouse_iceberg_table(started_cluster, node, root_namespace, table_name, "(x String, y Int32)") + + node.query(f"INSERT INTO {table_ref} VALUES ('abc', 1);", settings=write_settings) + + node.query(f"ALTER TABLE {table_ref} ADD COLUMN z Nullable(String);", settings=write_settings) + assert "z" in node.query(f"DESCRIBE TABLE {table_ref}", settings=write_settings) + + node.query(f"ALTER TABLE {table_ref} DROP COLUMN z;", settings=write_settings) + desc = node.query(f"DESCRIBE TABLE {table_ref}", settings=write_settings) + assert "z" not in desc + + assert node.query(f"SELECT x, y FROM {table_ref} ORDER BY ALL", settings=write_settings) == "abc\t1\n" + + def test_writes_schema_evolution_concurrent_add_columns(started_cluster): node = started_cluster.instances["node1"] From bedcad999d3fce01de6ccf95eec31b35415833ff Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Tue, 11 Aug 2026 18:48:37 +0200 Subject: [PATCH 08/18] Fix compiler error --- src/Databases/DataLake/ICatalog.h | 2 +- src/Databases/DataLake/RestCatalog.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Databases/DataLake/ICatalog.h b/src/Databases/DataLake/ICatalog.h index 8fd5ba85666c..5375f9f842bf 100644 --- a/src/Databases/DataLake/ICatalog.h +++ b/src/Databases/DataLake/ICatalog.h @@ -197,7 +197,7 @@ class ICatalog const String & new_metadata_path, Poco::JSON::Object::Ptr new_schema, Int32 previous_schema_id, - Poco::JSON::Object::Ptr full_metadata = nullptr) const; + Int32 new_last_column_id) const; /// Drop table from catalog. virtual void dropTable(const String & namespace_name, const String & table_name) const; diff --git a/src/Databases/DataLake/RestCatalog.h b/src/Databases/DataLake/RestCatalog.h index 00799d781142..7d65a1b95c0c 100644 --- a/src/Databases/DataLake/RestCatalog.h +++ b/src/Databases/DataLake/RestCatalog.h @@ -80,7 +80,7 @@ class RestCatalog : public ICatalog, public DB::WithContext const String & new_metadata_path, Poco::JSON::Object::Ptr new_schema, Int32 previous_schema_id, - Poco::JSON::Object::Ptr full_metadata = nullptr) const override; + Int32 new_last_column_id) const override; bool isTransactional() const override { return true; } From 12df29167390e994a1a1ff2a90d87917185f3a6f Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Tue, 11 Aug 2026 19:11:48 +0200 Subject: [PATCH 09/18] Fix compiler error --- src/Databases/DataLake/RestCatalog.cpp | 4 ++-- .../DataLakes/Iceberg/MetadataGenerator.cpp | 5 +++-- .../ObjectStorage/DataLakes/Iceberg/Mutations.cpp | 10 +++++++++- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/Databases/DataLake/RestCatalog.cpp b/src/Databases/DataLake/RestCatalog.cpp index 407c9a454388..008a638c70eb 100644 --- a/src/Databases/DataLake/RestCatalog.cpp +++ b/src/Databases/DataLake/RestCatalog.cpp @@ -1545,8 +1545,8 @@ void RestCatalog::createTable(const String & namespace_name, const String & tabl bool RestCatalog::updateMetadata(const String & namespace_name, const String & table_name, const String & /*new_metadata_path*/, Poco::JSON::Object::Ptr new_snapshot) const { if (!new_snapshot) - throw Exception( - ErrorCodes::NOT_IMPLEMENTED, + throw DB::Exception( + DB::ErrorCodes::NOT_IMPLEMENTED, "REST catalog does not support metadata-only updates without a snapshot " "(required for EXPIRE SNAPSHOTS)"); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp index 9a14cec78e9a..d08c70e49e92 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp @@ -75,7 +75,7 @@ bool checkValidSchemaEvolution(Poco::Dynamic::Var old_type, Poco::Dynamic::Var n }; auto [old_precision, old_scale] = parse(old_str); auto [new_precision, new_scale] = parse(new_str); - if (old_precision <= new_precision && old_scale <= new_scale) + if (old_precision <= new_precision && old_scale == new_scale) return true; } } @@ -87,7 +87,7 @@ bool checkValidSchemaEvolution(Poco::Dynamic::Var old_type, Poco::Dynamic::Var n if (old_complex_type && new_complex_type && old_complex_type->has("precision") && new_complex_type->has("precision") && (old_complex_type->getValue("precision") <= new_complex_type->getValue("precision") && - old_complex_type->getValue("scale") <= new_complex_type->getValue("scale"))) + old_complex_type->getValue("scale") == new_complex_type->getValue("scale"))) { return true; } @@ -408,6 +408,7 @@ bool MetadataGenerator::generateModifyColumnMetadata(const String & column_name, metadata_object->set(Iceberg::f_current_schema_id, current_schema_id + 1); current_schema->set(Iceberg::f_schema_id, current_schema_id + 1); metadata_object->getArray(Iceberg::f_schemas)->add(current_schema); + metadata_object->set(Iceberg::f_last_column_id, last_column_id); return true; } } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp index 2f2d5fc9aa4b..f1236cc8d5cf 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp @@ -851,8 +851,13 @@ void alter( metadata_json_generator.generateDropColumnMetadata(params[0].column_name); break; case AlterCommand::Type::MODIFY_COLUMN: - metadata_json_generator.generateModifyColumnMetadata(params[0].column_name, params[0].data_type); + { + if (!metadata_json_generator.generateModifyColumnMetadata(params[0].column_name, params[0].data_type)) + { + succeeded = true; + } break; + } case AlterCommand::Type::RENAME_COLUMN: metadata_json_generator.generateRenameColumnMetadata(params[0].column_name, params[0].rename_to); break; @@ -860,6 +865,9 @@ void alter( throw Exception(ErrorCodes::LOGICAL_ERROR, "Unknown type of alter {}", params[0].type); } + if (succeeded) + break; + const auto new_schema_id = metadata->getValue(Iceberg::f_current_schema_id); Poco::JSON::Object::Ptr new_schema; auto schemas = metadata->getArray(Iceberg::f_schemas); From d4c5df2416736089d08080242318089fce10ce9d Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Tue, 11 Aug 2026 20:29:00 +0200 Subject: [PATCH 10/18] Revert back change to settings regarding iceberg manifest compaction, fix medium defects --- src/Core/SettingsChangesHistory.cpp | 6 - src/Databases/DataLake/GlueCatalog.cpp | 3 +- src/Databases/DataLake/GlueCatalog.h | 3 +- src/Databases/DataLake/ICatalog.cpp | 3 +- src/Databases/DataLake/ICatalog.h | 3 +- src/Databases/DataLake/RestCatalog.cpp | 181 ++++++++++--- src/Databases/DataLake/RestCatalog.h | 13 +- .../gtest_rest_catalog_update_metadata.cpp | 252 ++++++++++++++++++ .../DataLakes/Iceberg/MetadataGenerator.cpp | 3 +- .../DataLakes/Iceberg/Mutations.cpp | 2 +- 10 files changed, 418 insertions(+), 51 deletions(-) diff --git a/src/Core/SettingsChangesHistory.cpp b/src/Core/SettingsChangesHistory.cpp index 8871fbc282a2..bc7f336c8896 100644 --- a/src/Core/SettingsChangesHistory.cpp +++ b/src/Core/SettingsChangesHistory.cpp @@ -41,12 +41,6 @@ const VersionToSettingsChangesMap & getSettingsChangesHistory() /// Note: please check if the key already exists to prevent duplicate entries. addSettingsChanges(settings_changes_history, "26.6", { - {"analyzer_compatibility_allow_non_aggregate_in_having", false, false, "New compatibility setting. When enabled, the new analyzer mimics the legacy `HAVING`-to-`WHERE` rewrite for non-aggregate AND-conjuncts instead of raising `NOT_AN_AGGREGATE`."}, - {"reserve_memory", 0, 0, "New setting to reserve memory for specific workload before starting a query."}, - {"iceberg_manifest_min_count_to_compact", 30, 30, "New setting to control manifest compaction for Iceberg tables."}, - {"output_format_image_width", 1024, 1024, "New setting controlling the width of the output image for image output formats such as PNG."}, - {"output_format_image_height", 1024, 1024, "New setting controlling the height of the output image for image output formats such as PNG."}, - {"output_format_image_terminal_mode", "", "", "New setting controlling whether image output formats such as PNG are rendered directly to the terminal using an inline image protocol."}, {"join_runtime_filter_from_fixed_hash_table", false, true, "New setting."}, {"use_lightweight_primary_key_index_analysis", false, true, "New setting to optimize primary key index analysis for tables with long primary keys"}, {"ai_function_embedding_max_batch_size", 100, 100, "New setting"}, diff --git a/src/Databases/DataLake/GlueCatalog.cpp b/src/Databases/DataLake/GlueCatalog.cpp index 936d84af89a1..4547e7d84c74 100644 --- a/src/Databases/DataLake/GlueCatalog.cpp +++ b/src/Databases/DataLake/GlueCatalog.cpp @@ -680,7 +680,8 @@ bool GlueCatalog::updateSchema( const String & new_metadata_path, Poco::JSON::Object::Ptr /*new_schema*/, Int32 /*previous_schema_id*/, - Int32 /*new_last_column_id*/) const + Int32 /*new_last_column_id*/, + Poco::JSON::Object::Ptr /*metadata*/) const { return updateMetadata(namespace_name, table_name, new_metadata_path, nullptr); } diff --git a/src/Databases/DataLake/GlueCatalog.h b/src/Databases/DataLake/GlueCatalog.h index e9304adaf252..6817765b4098 100644 --- a/src/Databases/DataLake/GlueCatalog.h +++ b/src/Databases/DataLake/GlueCatalog.h @@ -74,7 +74,8 @@ class GlueCatalog final : public ICatalog, private DB::WithContext const String & new_metadata_path, Poco::JSON::Object::Ptr new_schema, Int32 previous_schema_id, - Int32 new_last_column_id) const override; + Int32 new_last_column_id, + Poco::JSON::Object::Ptr metadata = nullptr) const override; void dropTable(const String & namespace_name, const String & table_name) const override; diff --git a/src/Databases/DataLake/ICatalog.cpp b/src/Databases/DataLake/ICatalog.cpp index e1225d7a9a6a..8906d5ffe7d8 100644 --- a/src/Databases/DataLake/ICatalog.cpp +++ b/src/Databases/DataLake/ICatalog.cpp @@ -326,7 +326,8 @@ bool ICatalog::updateSchema( const String & /*new_metadata_path*/, Poco::JSON::Object::Ptr /*new_schema*/, Int32 /*previous_schema_id*/, - Int32 /*new_last_column_id*/) const + Int32 /*new_last_column_id*/, + Poco::JSON::Object::Ptr /*metadata*/) const { throw DB::Exception(DB::ErrorCodes::NOT_IMPLEMENTED, "updateSchema is not implemented"); } diff --git a/src/Databases/DataLake/ICatalog.h b/src/Databases/DataLake/ICatalog.h index 5375f9f842bf..0acb80328576 100644 --- a/src/Databases/DataLake/ICatalog.h +++ b/src/Databases/DataLake/ICatalog.h @@ -197,7 +197,8 @@ class ICatalog const String & new_metadata_path, Poco::JSON::Object::Ptr new_schema, Int32 previous_schema_id, - Int32 new_last_column_id) const; + Int32 new_last_column_id, + Poco::JSON::Object::Ptr metadata = nullptr) const; /// Drop table from catalog. virtual void dropTable(const String & namespace_name, const String & table_name) const; diff --git a/src/Databases/DataLake/RestCatalog.cpp b/src/Databases/DataLake/RestCatalog.cpp index 008a638c70eb..d3ac6e250f26 100644 --- a/src/Databases/DataLake/RestCatalog.cpp +++ b/src/Databases/DataLake/RestCatalog.cpp @@ -227,6 +227,43 @@ bool schemasEquivalentIgnoringId(const Poco::JSON::Object::Ptr & lhs, const Poco return icebergJsonObjectEquals(lhs_copy, rhs_copy); } +void collectFieldIdsFromType(const Poco::Dynamic::Var & type_var, std::unordered_set & ids) +{ + if (type_var.type() != typeid(Poco::JSON::Object::Ptr)) + return; + auto obj = type_var.extract(); + const auto type_str = obj->optValue(DB::Iceberg::f_type, ""); + if (type_str == "struct") + { + auto fields = obj->getArray(DB::Iceberg::f_fields); + for (UInt32 i = 0; i < fields->size(); ++i) + { + auto field = fields->getObject(i); + if (field->has(DB::Iceberg::f_id)) + ids.insert(field->getValue(DB::Iceberg::f_id)); + collectFieldIdsFromType(field->get(DB::Iceberg::f_type), ids); + } + } + else if (type_str == "list") + { + if (obj->has(DB::Iceberg::f_element_id)) + ids.insert(obj->getValue(DB::Iceberg::f_element_id)); + if (obj->has(DB::Iceberg::f_element)) + collectFieldIdsFromType(obj->get(DB::Iceberg::f_element), ids); + } + else if (type_str == "map") + { + if (obj->has(DB::Iceberg::f_key_id)) + ids.insert(obj->getValue(DB::Iceberg::f_key_id)); + if (obj->has(DB::Iceberg::f_value_id)) + ids.insert(obj->getValue(DB::Iceberg::f_value_id)); + if (obj->has(DB::Iceberg::f_key)) + collectFieldIdsFromType(obj->get(DB::Iceberg::f_key), ids); + if (obj->has(DB::Iceberg::f_value)) + collectFieldIdsFromType(obj->get(DB::Iceberg::f_value), ids); + } +} + void collectSchemaFieldIdsFromFields(const Poco::JSON::Array::Ptr & fields, std::unordered_set & ids) { for (UInt32 i = 0; i < fields->size(); ++i) @@ -234,6 +271,8 @@ void collectSchemaFieldIdsFromFields(const Poco::JSON::Array::Ptr & fields, std: auto field = fields->getObject(i); if (field->has(DB::Iceberg::f_id)) ids.insert(field->getValue(DB::Iceberg::f_id)); + if (field->has(DB::Iceberg::f_type)) + collectFieldIdsFromType(field->get(DB::Iceberg::f_type), ids); } } @@ -289,6 +328,106 @@ bool sortOrderIncompatibleWithSchema( } +Poco::JSON::Object::Ptr buildUpdateSchemaRequestBody( + const String & namespace_name, + const String & table_name, + Poco::JSON::Object::Ptr metadata, + Poco::JSON::Object::Ptr new_schema, + Int32 previous_schema_id, + Int32 new_last_column_id) +{ + Poco::JSON::Object::Ptr request_body = new Poco::JSON::Object; + { + Poco::JSON::Object::Ptr identifier = new Poco::JSON::Object; + identifier->set("name", table_name); + Poco::JSON::Array::Ptr namespaces = new Poco::JSON::Array; + namespaces->add(namespace_name); + identifier->set("namespace", namespaces); + request_body->set("identifier", identifier); + } + + if (previous_schema_id >= 0) + { + Poco::JSON::Object::Ptr requirement = new Poco::JSON::Object; + requirement->set("type", "assert-current-schema-id"); + requirement->set("current-schema-id", previous_schema_id); + + Poco::JSON::Array::Ptr requirements = new Poco::JSON::Array; + requirements->add(requirement); + request_body->set("requirements", requirements); + } + + Poco::JSON::Object::Ptr schema_for_rest = cloneJsonObject(new_schema); + if (!schema_for_rest->has("identifier-field-ids")) + { + Poco::JSON::Array::Ptr empty_identifier_field_ids = new Poco::JSON::Array; + schema_for_rest->set("identifier-field-ids", empty_identifier_field_ids); + } + + std::optional existing_equivalent_schema_id; + if (metadata && metadata->has(DB::Iceberg::f_schemas)) + { + auto schemas = metadata->getArray(DB::Iceberg::f_schemas); + auto new_schema_id = new_schema->getValue(DB::Iceberg::f_schema_id); + for (UInt32 i = 0; i < schemas->size(); ++i) + { + auto existing_schema = schemas->getObject(i); + if (existing_schema->getValue(DB::Iceberg::f_schema_id) == new_schema_id) + continue; + if (schemasEquivalentIgnoringId(existing_schema, new_schema)) + { + existing_equivalent_schema_id = existing_schema->getValue(DB::Iceberg::f_schema_id); + break; + } + } + } + + Poco::JSON::Array::Ptr updates = new Poco::JSON::Array; + if (existing_equivalent_schema_id.has_value()) + { + Poco::JSON::Object::Ptr set_current_schema = new Poco::JSON::Object; + set_current_schema->set("action", "set-current-schema"); + set_current_schema->set("schema-id", *existing_equivalent_schema_id); + updates->add(set_current_schema); + } + else + { + { + Poco::JSON::Object::Ptr add_schema = new Poco::JSON::Object; + add_schema->set("action", "add-schema"); + add_schema->set("schema", schema_for_rest); + add_schema->set("last-column-id", new_last_column_id); + updates->add(add_schema); + } + { + Poco::JSON::Object::Ptr set_current_schema = new Poco::JSON::Object; + set_current_schema->set("action", "set-current-schema"); + set_current_schema->set("schema-id", -1); + updates->add(set_current_schema); + } + } + + if (metadata && sortOrderIncompatibleWithSchema(metadata, new_schema)) + { + Poco::JSON::Object::Ptr unsorted_sort_order = new Poco::JSON::Object; + unsorted_sort_order->set(DB::Iceberg::f_order_id, 0); + unsorted_sort_order->set(DB::Iceberg::f_fields, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); + + Poco::JSON::Object::Ptr add_sort_order = new Poco::JSON::Object; + add_sort_order->set("action", "add-sort-order"); + add_sort_order->set("sort-order", unsorted_sort_order); + updates->add(add_sort_order); + + Poco::JSON::Object::Ptr set_default_sort_order = new Poco::JSON::Object; + set_default_sort_order->set("action", "set-default-sort-order"); + set_default_sort_order->set("sort-order-id", -1); + updates->add(set_default_sort_order); + } + + request_body->set("updates", updates); + return request_body; +} + Poco::JSON::Object::Ptr buildUpdateMetadataRequestBody( const String & namespace_name, const String & table_name, Poco::JSON::Object::Ptr new_snapshot) { @@ -1577,47 +1716,13 @@ bool RestCatalog::updateSchema( const String & /*new_metadata_path*/, Poco::JSON::Object::Ptr new_schema, Int32 previous_schema_id, - Int32 new_last_column_id) const + Int32 new_last_column_id, + Poco::JSON::Object::Ptr metadata) const { const std::string endpoint = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables" / table_name).generic_string(); - Poco::JSON::Object::Ptr request_body = new Poco::JSON::Object; - { - Poco::JSON::Object::Ptr identifier = new Poco::JSON::Object; - identifier->set("name", table_name); - Poco::JSON::Array::Ptr namespaces = new Poco::JSON::Array; - namespaces->add(namespace_name); - identifier->set("namespace", namespaces); - request_body->set("identifier", identifier); - } - - { - Poco::JSON::Object::Ptr requirement = new Poco::JSON::Object; - requirement->set("type", "assert-current-schema-id"); - requirement->set("current-schema-id", previous_schema_id); - - Poco::JSON::Array::Ptr requirements = new Poco::JSON::Array; - requirements->add(requirement); - request_body->set("requirements", requirements); - } - - { - Poco::JSON::Array::Ptr updates = new Poco::JSON::Array; - { - Poco::JSON::Object::Ptr add_schema = new Poco::JSON::Object; - add_schema->set("action", "add-schema"); - add_schema->set("schema", new_schema); - add_schema->set("last-column-id", new_last_column_id); - updates->add(add_schema); - } - { - Poco::JSON::Object::Ptr set_current_schema = new Poco::JSON::Object; - set_current_schema->set("action", "set-current-schema"); - set_current_schema->set("schema-id", -1); - updates->add(set_current_schema); - } - request_body->set("updates", updates); - } + auto request_body = buildUpdateSchemaRequestBody( + namespace_name, table_name, metadata, new_schema, previous_schema_id, new_last_column_id); try { diff --git a/src/Databases/DataLake/RestCatalog.h b/src/Databases/DataLake/RestCatalog.h index 7d65a1b95c0c..4f7d64e5c54b 100644 --- a/src/Databases/DataLake/RestCatalog.h +++ b/src/Databases/DataLake/RestCatalog.h @@ -80,7 +80,8 @@ class RestCatalog : public ICatalog, public DB::WithContext const String & new_metadata_path, Poco::JSON::Object::Ptr new_schema, Int32 previous_schema_id, - Int32 new_last_column_id) const override; + Int32 new_last_column_id, + Poco::JSON::Object::Ptr metadata = nullptr) const override; bool isTransactional() const override { return true; } @@ -249,6 +250,16 @@ class BigLakeCatalog : public RestCatalog /// Returns `nullptr` when `new_snapshot` is null (nothing to commit). Throws /// `DB::Exception(DATALAKE_DATABASE_ERROR)` with a specific message when the metadata /// blob is malformed (e.g. missing `current-schema-id`, no schema object matching it). +/// Builds the JSON body for a schema-update commit via the Iceberg REST catalog. +/// Includes schema deduplication, sort-order incompatibility reset, and last-column-id. +Poco::JSON::Object::Ptr buildUpdateSchemaRequestBody( + const String & namespace_name, + const String & table_name, + Poco::JSON::Object::Ptr metadata, + Poco::JSON::Object::Ptr new_schema, + Int32 previous_schema_id, + Int32 new_last_column_id); + Poco::JSON::Object::Ptr buildUpdateMetadataRequestBody( const String & namespace_name, const String & table_name, diff --git a/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp b/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp index 11fcc085990c..f1bb12c858f3 100644 --- a/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp +++ b/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp @@ -183,4 +183,256 @@ TEST(RestCatalogUpdateMetadataBody, SnapshotUpdateParentMinusOneNoRequirement) EXPECT_FALSE(body->has("requirements")); } +TEST(RestCatalogUpdateSchemaBody, NestedSortKeySurvivesSchemaChange) +{ + Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; + + Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; + Poco::JSON::Object::Ptr schema = new Poco::JSON::Object; + schema->set(Iceberg::f_schema_id, 0); + schema->set(Iceberg::f_type, "struct"); + + Poco::JSON::Array::Ptr fields = new Poco::JSON::Array; + + Poco::JSON::Object::Ptr struct_field = new Poco::JSON::Object; + struct_field->set(Iceberg::f_id, 1); + struct_field->set(Iceberg::f_name, "s"); + struct_field->set(Iceberg::f_required, false); + + Poco::JSON::Object::Ptr struct_type = new Poco::JSON::Object; + struct_type->set(Iceberg::f_type, "struct"); + Poco::JSON::Array::Ptr nested_fields = new Poco::JSON::Array; + Poco::JSON::Object::Ptr nested_field = new Poco::JSON::Object; + nested_field->set(Iceberg::f_id, 2); + nested_field->set(Iceberg::f_name, "x"); + nested_field->set(Iceberg::f_required, false); + nested_field->set(Iceberg::f_type, "int"); + nested_fields->add(nested_field); + struct_type->set(Iceberg::f_fields, nested_fields); + struct_field->set(Iceberg::f_type, struct_type); + fields->add(struct_field); + + Poco::JSON::Object::Ptr other_field = new Poco::JSON::Object; + other_field->set(Iceberg::f_id, 3); + other_field->set(Iceberg::f_name, "a"); + other_field->set(Iceberg::f_required, false); + other_field->set(Iceberg::f_type, "int"); + fields->add(other_field); + + schema->set(Iceberg::f_fields, fields); + schemas->add(schema); + + Poco::JSON::Object::Ptr new_schema = new Poco::JSON::Object; + new_schema->set(Iceberg::f_schema_id, 1); + new_schema->set(Iceberg::f_type, "struct"); + Poco::JSON::Array::Ptr new_fields = new Poco::JSON::Array; + new_fields->add(struct_field); + new_schema->set(Iceberg::f_fields, new_fields); + schemas->add(new_schema); + + snapshot->set(Iceberg::f_schemas, schemas); + snapshot->set(Iceberg::f_current_schema_id, 1); + snapshot->set(Iceberg::f_last_column_id, 3); + + Poco::JSON::Array::Ptr sort_orders = new Poco::JSON::Array; + Poco::JSON::Object::Ptr sort_order = new Poco::JSON::Object; + sort_order->set(Iceberg::f_order_id, 1); + Poco::JSON::Array::Ptr sort_fields = new Poco::JSON::Array; + Poco::JSON::Object::Ptr sort_field = new Poco::JSON::Object; + sort_field->set(Iceberg::f_source_id, 2); + sort_field->set("transform", "identity"); + sort_field->set("direction", "asc"); + sort_field->set("null-order", "nulls-first"); + sort_fields->add(sort_field); + sort_order->set(Iceberg::f_fields, sort_fields); + sort_orders->add(sort_order); + snapshot->set(Iceberg::f_sort_orders, sort_orders); + snapshot->set(Iceberg::f_default_sort_order_id, static_cast(1)); + + auto body = DataLake::buildUpdateMetadataRequestBody("ns", "t", snapshot); + ASSERT_TRUE(body); + + auto updates = body->getArray("updates"); + EXPECT_FALSE(findUpdateByAction(updates, "add-sort-order")); +} + +TEST(RestCatalogUpdateSchemaBody, DroppedSortKeyResetsSortOrder) +{ + Poco::JSON::Object::Ptr metadata = new Poco::JSON::Object; + + Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; + Poco::JSON::Object::Ptr schema = new Poco::JSON::Object; + schema->set(Iceberg::f_schema_id, 0); + schema->set(Iceberg::f_type, "struct"); + Poco::JSON::Array::Ptr fields = new Poco::JSON::Array; + Poco::JSON::Object::Ptr field1 = new Poco::JSON::Object; + field1->set(Iceberg::f_id, 1); + field1->set(Iceberg::f_name, "a"); + field1->set(Iceberg::f_required, false); + field1->set(Iceberg::f_type, "int"); + fields->add(field1); + Poco::JSON::Object::Ptr field2 = new Poco::JSON::Object; + field2->set(Iceberg::f_id, 2); + field2->set(Iceberg::f_name, "b"); + field2->set(Iceberg::f_required, false); + field2->set(Iceberg::f_type, "int"); + fields->add(field2); + schema->set(Iceberg::f_fields, fields); + schemas->add(schema); + metadata->set(Iceberg::f_schemas, schemas); + metadata->set(Iceberg::f_current_schema_id, 0); + + Poco::JSON::Array::Ptr sort_orders = new Poco::JSON::Array; + Poco::JSON::Object::Ptr sort_order = new Poco::JSON::Object; + sort_order->set(Iceberg::f_order_id, 1); + Poco::JSON::Array::Ptr sort_fields = new Poco::JSON::Array; + Poco::JSON::Object::Ptr sort_field = new Poco::JSON::Object; + sort_field->set(Iceberg::f_source_id, 2); + sort_field->set("transform", "identity"); + sort_field->set("direction", "asc"); + sort_field->set("null-order", "nulls-first"); + sort_fields->add(sort_field); + sort_order->set(Iceberg::f_fields, sort_fields); + sort_orders->add(sort_order); + metadata->set(Iceberg::f_sort_orders, sort_orders); + metadata->set(Iceberg::f_default_sort_order_id, static_cast(1)); + + Poco::JSON::Object::Ptr new_schema = new Poco::JSON::Object; + new_schema->set(Iceberg::f_schema_id, 1); + new_schema->set(Iceberg::f_type, "struct"); + Poco::JSON::Array::Ptr new_fields = new Poco::JSON::Array; + new_fields->add(field1); + new_schema->set(Iceberg::f_fields, new_fields); + + auto body = DataLake::buildUpdateSchemaRequestBody("ns", "t", metadata, new_schema, 0, 2); + ASSERT_TRUE(body); + + auto updates = body->getArray("updates"); + auto add_sort = findUpdateByAction(updates, "add-sort-order"); + ASSERT_TRUE(add_sort); + auto set_sort = findUpdateByAction(updates, "set-default-sort-order"); + ASSERT_TRUE(set_sort); + EXPECT_EQ(set_sort->getValue("sort-order-id"), -1); +} + +TEST(RestCatalogUpdateSchemaBody, EquivalentSchemaDeduplicates) +{ + Poco::JSON::Object::Ptr metadata = new Poco::JSON::Object; + + Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; + Poco::JSON::Object::Ptr schema0 = new Poco::JSON::Object; + schema0->set(Iceberg::f_schema_id, 0); + schema0->set(Iceberg::f_type, "struct"); + Poco::JSON::Array::Ptr fields = new Poco::JSON::Array; + Poco::JSON::Object::Ptr field1 = new Poco::JSON::Object; + field1->set(Iceberg::f_id, 1); + field1->set(Iceberg::f_name, "a"); + field1->set(Iceberg::f_required, false); + field1->set(Iceberg::f_type, "int"); + fields->add(field1); + schema0->set(Iceberg::f_fields, fields); + schemas->add(schema0); + + Poco::JSON::Object::Ptr schema1 = new Poco::JSON::Object; + schema1->set(Iceberg::f_schema_id, 1); + schema1->set(Iceberg::f_type, "struct"); + Poco::JSON::Array::Ptr fields1 = new Poco::JSON::Array; + Poco::JSON::Object::Ptr field1b = new Poco::JSON::Object; + field1b->set(Iceberg::f_id, 1); + field1b->set(Iceberg::f_name, "a"); + field1b->set(Iceberg::f_required, false); + field1b->set(Iceberg::f_type, "int"); + Poco::JSON::Object::Ptr field2b = new Poco::JSON::Object; + field2b->set(Iceberg::f_id, 2); + field2b->set(Iceberg::f_name, "b"); + field2b->set(Iceberg::f_required, false); + field2b->set(Iceberg::f_type, "int"); + fields1->add(field1b); + fields1->add(field2b); + schema1->set(Iceberg::f_fields, fields1); + schemas->add(schema1); + metadata->set(Iceberg::f_schemas, schemas); + + Poco::JSON::Object::Ptr new_schema = new Poco::JSON::Object; + new_schema->set(Iceberg::f_schema_id, 2); + new_schema->set(Iceberg::f_type, "struct"); + Poco::JSON::Array::Ptr new_fields = new Poco::JSON::Array; + Poco::JSON::Object::Ptr nf1 = new Poco::JSON::Object; + nf1->set(Iceberg::f_id, 1); + nf1->set(Iceberg::f_name, "a"); + nf1->set(Iceberg::f_required, false); + nf1->set(Iceberg::f_type, "int"); + new_fields->add(nf1); + new_schema->set(Iceberg::f_fields, new_fields); + + auto body = DataLake::buildUpdateSchemaRequestBody("ns", "t", metadata, new_schema, 1, 2); + ASSERT_TRUE(body); + + auto updates = body->getArray("updates"); + EXPECT_FALSE(findUpdateByAction(updates, "add-schema")); + + auto set_schema = findUpdateByAction(updates, "set-current-schema"); + ASSERT_TRUE(set_schema); + EXPECT_EQ(set_schema->getValue("schema-id"), 0); +} + +TEST(RestCatalogUpdateSchemaBody, NormalPathEmitsAddSchema) +{ + Poco::JSON::Object::Ptr metadata = new Poco::JSON::Object; + + Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; + Poco::JSON::Object::Ptr schema0 = new Poco::JSON::Object; + schema0->set(Iceberg::f_schema_id, 0); + schema0->set(Iceberg::f_type, "struct"); + Poco::JSON::Array::Ptr fields = new Poco::JSON::Array; + Poco::JSON::Object::Ptr field1 = new Poco::JSON::Object; + field1->set(Iceberg::f_id, 1); + field1->set(Iceberg::f_name, "a"); + field1->set(Iceberg::f_required, false); + field1->set(Iceberg::f_type, "int"); + fields->add(field1); + schema0->set(Iceberg::f_fields, fields); + schemas->add(schema0); + metadata->set(Iceberg::f_schemas, schemas); + + Poco::JSON::Object::Ptr new_schema = new Poco::JSON::Object; + new_schema->set(Iceberg::f_schema_id, 1); + new_schema->set(Iceberg::f_type, "struct"); + Poco::JSON::Array::Ptr new_fields = new Poco::JSON::Array; + Poco::JSON::Object::Ptr nf1 = new Poco::JSON::Object; + nf1->set(Iceberg::f_id, 1); + nf1->set(Iceberg::f_name, "a"); + nf1->set(Iceberg::f_required, false); + nf1->set(Iceberg::f_type, "int"); + Poco::JSON::Object::Ptr nf2 = new Poco::JSON::Object; + nf2->set(Iceberg::f_id, 2); + nf2->set(Iceberg::f_name, "b"); + nf2->set(Iceberg::f_required, false); + nf2->set(Iceberg::f_type, "string"); + new_fields->add(nf1); + new_fields->add(nf2); + new_schema->set(Iceberg::f_fields, new_fields); + + auto body = DataLake::buildUpdateSchemaRequestBody("ns", "t", metadata, new_schema, 0, 5); + ASSERT_TRUE(body); + + auto updates = body->getArray("updates"); + + auto add_schema = findUpdateByAction(updates, "add-schema"); + ASSERT_TRUE(add_schema); + EXPECT_EQ(add_schema->getValue("last-column-id"), 5); + EXPECT_TRUE(add_schema->has("schema")); + auto schema_obj = add_schema->getObject("schema"); + EXPECT_TRUE(schema_obj->has("identifier-field-ids")); + + auto set_schema = findUpdateByAction(updates, "set-current-schema"); + ASSERT_TRUE(set_schema); + EXPECT_EQ(set_schema->getValue("schema-id"), -1); + + ASSERT_TRUE(body->has("requirements")); + auto req = body->getArray("requirements")->getObject(0); + EXPECT_EQ(req->getValue("type"), "assert-current-schema-id"); + EXPECT_EQ(req->getValue("current-schema-id"), 0); +} + #endif diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp index d08c70e49e92..09415cfbf9de 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp @@ -347,7 +347,6 @@ void MetadataGenerator::generateAddColumnMetadata(const String & column_name, Da } auto last_column_id = metadata_object->getValue(Iceberg::f_last_column_id); - metadata_object->set(Iceberg::f_last_column_id, last_column_id + 1); auto new_type = Iceberg::getIcebergType(type, last_column_id); Poco::JSON::Object::Ptr new_field = new Poco::JSON::Object; @@ -356,6 +355,8 @@ void MetadataGenerator::generateAddColumnMetadata(const String & column_name, Da new_field->set(Iceberg::f_required, new_type.second); new_field->set(Iceberg::f_type, new_type.first); + metadata_object->set(Iceberg::f_last_column_id, last_column_id + 1); + current_schema->getArray(Iceberg::f_fields)->add(new_field); current_schema->set(Iceberg::f_schema_id, current_schema_id + 1); metadata_object->getArray(Iceberg::f_schemas)->add(current_schema); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp index f1236cc8d5cf..2f072b5ca3f2 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp @@ -910,7 +910,7 @@ void alter( const auto new_last_column_id = std::max( metadata->getValue(Iceberg::f_last_column_id), getHighestFieldId(new_schema)); - if (!catalog->updateSchema(namespace_name, table_name, catalog_filename, new_schema, previous_schema_id, new_last_column_id)) + if (!catalog->updateSchema(namespace_name, table_name, catalog_filename, new_schema, previous_schema_id, new_last_column_id, metadata)) { ++i; continue; From 422f409f184a99ac70a8b65747129317dc1a7506 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Tue, 11 Aug 2026 20:32:31 +0200 Subject: [PATCH 11/18] Reverted back settings changes(removal) --- src/Core/SettingsChangesHistory.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/Core/SettingsChangesHistory.cpp b/src/Core/SettingsChangesHistory.cpp index bc7f336c8896..8871fbc282a2 100644 --- a/src/Core/SettingsChangesHistory.cpp +++ b/src/Core/SettingsChangesHistory.cpp @@ -41,6 +41,12 @@ const VersionToSettingsChangesMap & getSettingsChangesHistory() /// Note: please check if the key already exists to prevent duplicate entries. addSettingsChanges(settings_changes_history, "26.6", { + {"analyzer_compatibility_allow_non_aggregate_in_having", false, false, "New compatibility setting. When enabled, the new analyzer mimics the legacy `HAVING`-to-`WHERE` rewrite for non-aggregate AND-conjuncts instead of raising `NOT_AN_AGGREGATE`."}, + {"reserve_memory", 0, 0, "New setting to reserve memory for specific workload before starting a query."}, + {"iceberg_manifest_min_count_to_compact", 30, 30, "New setting to control manifest compaction for Iceberg tables."}, + {"output_format_image_width", 1024, 1024, "New setting controlling the width of the output image for image output formats such as PNG."}, + {"output_format_image_height", 1024, 1024, "New setting controlling the height of the output image for image output formats such as PNG."}, + {"output_format_image_terminal_mode", "", "", "New setting controlling whether image output formats such as PNG are rendered directly to the terminal using an inline image protocol."}, {"join_runtime_filter_from_fixed_hash_table", false, true, "New setting."}, {"use_lightweight_primary_key_index_analysis", false, true, "New setting to optimize primary key index analysis for tables with long primary keys"}, {"ai_function_embedding_max_batch_size", 100, 100, "New setting"}, From 8bbea38393346a4a1b160f35ad0bf04a4c98609c Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Tue, 11 Aug 2026 21:50:18 +0200 Subject: [PATCH 12/18] Reverted back settings changes(removal) --- src/Core/SettingsChangesHistory.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Core/SettingsChangesHistory.cpp b/src/Core/SettingsChangesHistory.cpp index 8871fbc282a2..60698d507d27 100644 --- a/src/Core/SettingsChangesHistory.cpp +++ b/src/Core/SettingsChangesHistory.cpp @@ -43,7 +43,6 @@ const VersionToSettingsChangesMap & getSettingsChangesHistory() { {"analyzer_compatibility_allow_non_aggregate_in_having", false, false, "New compatibility setting. When enabled, the new analyzer mimics the legacy `HAVING`-to-`WHERE` rewrite for non-aggregate AND-conjuncts instead of raising `NOT_AN_AGGREGATE`."}, {"reserve_memory", 0, 0, "New setting to reserve memory for specific workload before starting a query."}, - {"iceberg_manifest_min_count_to_compact", 30, 30, "New setting to control manifest compaction for Iceberg tables."}, {"output_format_image_width", 1024, 1024, "New setting controlling the width of the output image for image output formats such as PNG."}, {"output_format_image_height", 1024, 1024, "New setting controlling the height of the output image for image output formats such as PNG."}, {"output_format_image_terminal_mode", "", "", "New setting controlling whether image output formats such as PNG are rendered directly to the terminal using an inline image protocol."}, From c72983e935f9c3476a9273fbf9219ef42a769459 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Wed, 12 Aug 2026 01:07:32 +0200 Subject: [PATCH 13/18] Address medium defects --- src/Databases/DataLake/RestCatalog.cpp | 293 +++--------------- .../gtest_rest_catalog_update_metadata.cpp | 232 -------------- .../DataLakes/Iceberg/MetadataGenerator.cpp | 120 ++++++- .../DataLakes/Iceberg/Mutations.cpp | 16 +- .../gtest_iceberg_metadata_generator.cpp | 191 ++++++++++++ .../integration/test_database_iceberg/test.py | 114 ++++++- .../test_writes_modify_column.py | 50 +++ 7 files changed, 516 insertions(+), 500 deletions(-) create mode 100644 src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp diff --git a/src/Databases/DataLake/RestCatalog.cpp b/src/Databases/DataLake/RestCatalog.cpp index d3ac6e250f26..60cd1ecff44b 100644 --- a/src/Databases/DataLake/RestCatalog.cpp +++ b/src/Databases/DataLake/RestCatalog.cpp @@ -227,105 +227,6 @@ bool schemasEquivalentIgnoringId(const Poco::JSON::Object::Ptr & lhs, const Poco return icebergJsonObjectEquals(lhs_copy, rhs_copy); } -void collectFieldIdsFromType(const Poco::Dynamic::Var & type_var, std::unordered_set & ids) -{ - if (type_var.type() != typeid(Poco::JSON::Object::Ptr)) - return; - auto obj = type_var.extract(); - const auto type_str = obj->optValue(DB::Iceberg::f_type, ""); - if (type_str == "struct") - { - auto fields = obj->getArray(DB::Iceberg::f_fields); - for (UInt32 i = 0; i < fields->size(); ++i) - { - auto field = fields->getObject(i); - if (field->has(DB::Iceberg::f_id)) - ids.insert(field->getValue(DB::Iceberg::f_id)); - collectFieldIdsFromType(field->get(DB::Iceberg::f_type), ids); - } - } - else if (type_str == "list") - { - if (obj->has(DB::Iceberg::f_element_id)) - ids.insert(obj->getValue(DB::Iceberg::f_element_id)); - if (obj->has(DB::Iceberg::f_element)) - collectFieldIdsFromType(obj->get(DB::Iceberg::f_element), ids); - } - else if (type_str == "map") - { - if (obj->has(DB::Iceberg::f_key_id)) - ids.insert(obj->getValue(DB::Iceberg::f_key_id)); - if (obj->has(DB::Iceberg::f_value_id)) - ids.insert(obj->getValue(DB::Iceberg::f_value_id)); - if (obj->has(DB::Iceberg::f_key)) - collectFieldIdsFromType(obj->get(DB::Iceberg::f_key), ids); - if (obj->has(DB::Iceberg::f_value)) - collectFieldIdsFromType(obj->get(DB::Iceberg::f_value), ids); - } -} - -void collectSchemaFieldIdsFromFields(const Poco::JSON::Array::Ptr & fields, std::unordered_set & ids) -{ - for (UInt32 i = 0; i < fields->size(); ++i) - { - auto field = fields->getObject(i); - if (field->has(DB::Iceberg::f_id)) - ids.insert(field->getValue(DB::Iceberg::f_id)); - if (field->has(DB::Iceberg::f_type)) - collectFieldIdsFromType(field->get(DB::Iceberg::f_type), ids); - } -} - -/// Returns true when the default sort order references field ids that are absent -/// from the new schema (i.e. the sort order became incompatible after a column drop). -bool sortOrderIncompatibleWithSchema( - const Poco::JSON::Object::Ptr & metadata_obj, - const Poco::JSON::Object::Ptr & new_schema_obj) -{ - if (!metadata_obj->has(DB::Iceberg::f_sort_orders) || !metadata_obj->has(DB::Iceberg::f_default_sort_order_id)) - return false; - - const Int64 default_sort_order_id = metadata_obj->getValue(DB::Iceberg::f_default_sort_order_id); - if (default_sort_order_id == 0) - return false; - - auto sort_orders = metadata_obj->getArray(DB::Iceberg::f_sort_orders); - Poco::JSON::Object::Ptr default_sort_order; - for (UInt32 i = 0; i < sort_orders->size(); ++i) - { - auto sort_order = sort_orders->getObject(i); - if (sort_order->getValue(DB::Iceberg::f_order_id) == default_sort_order_id) - { - default_sort_order = sort_order; - break; - } - } - - if (!default_sort_order || !default_sort_order->has(DB::Iceberg::f_fields)) - return false; - - auto sort_fields = default_sort_order->getArray(DB::Iceberg::f_fields); - if (sort_fields->size() == 0) - return false; - - std::unordered_set new_schema_field_ids; - if (new_schema_obj->has(DB::Iceberg::f_fields)) - collectSchemaFieldIdsFromFields(new_schema_obj->getArray(DB::Iceberg::f_fields), new_schema_field_ids); - - for (UInt32 i = 0; i < sort_fields->size(); ++i) - { - auto field = sort_fields->getObject(i); - if (!field->has(DB::Iceberg::f_source_id)) - continue; - - const Int32 source_id = field->getValue(DB::Iceberg::f_source_id); - if (!new_schema_field_ids.contains(source_id)) - return true; - } - - return false; -} - } Poco::JSON::Object::Ptr buildUpdateSchemaRequestBody( @@ -407,23 +308,6 @@ Poco::JSON::Object::Ptr buildUpdateSchemaRequestBody( } } - if (metadata && sortOrderIncompatibleWithSchema(metadata, new_schema)) - { - Poco::JSON::Object::Ptr unsorted_sort_order = new Poco::JSON::Object; - unsorted_sort_order->set(DB::Iceberg::f_order_id, 0); - unsorted_sort_order->set(DB::Iceberg::f_fields, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); - - Poco::JSON::Object::Ptr add_sort_order = new Poco::JSON::Object; - add_sort_order->set("action", "add-sort-order"); - add_sort_order->set("sort-order", unsorted_sort_order); - updates->add(add_sort_order); - - Poco::JSON::Object::Ptr set_default_sort_order = new Poco::JSON::Object; - set_default_sort_order->set("action", "set-default-sort-order"); - set_default_sort_order->set("sort-order-id", -1); - updates->add(set_default_sort_order); - } - request_body->set("updates", updates); return request_body; } @@ -445,149 +329,38 @@ Poco::JSON::Object::Ptr buildUpdateMetadataRequestBody( request_body->set("identifier", identifier); } - if (new_snapshot->has(DB::Iceberg::f_schemas)) + if (new_snapshot->has("parent-snapshot-id")) { - if (!new_snapshot->has(DB::Iceberg::f_current_schema_id)) - throw DB::Exception( - DB::ErrorCodes::DATALAKE_DATABASE_ERROR, - "Iceberg update-metadata for {}.{} is missing '{}' field", - namespace_name, table_name, DB::Iceberg::f_current_schema_id); - - const Int32 new_schema_id = new_snapshot->getValue(DB::Iceberg::f_current_schema_id); - const Int32 old_schema_id = new_schema_id - 1; - - Poco::JSON::Object::Ptr new_schema_obj; - auto schemas = new_snapshot->getArray(DB::Iceberg::f_schemas); - for (UInt32 i = 0; i < schemas->size(); ++i) - { - auto s = schemas->getObject(i); - if (s->getValue(DB::Iceberg::f_schema_id) == new_schema_id) - { - new_schema_obj = s; - break; - } - } - if (!new_schema_obj) - throw DB::Exception( - DB::ErrorCodes::DATALAKE_DATABASE_ERROR, - "Iceberg update-metadata for {}.{}: no schema object matching current-schema-id={}", - namespace_name, table_name, new_schema_id); - - Poco::JSON::Object::Ptr schema_for_rest = cloneJsonObject(new_schema_obj); - if (!schema_for_rest->has("identifier-field-ids")) - { - Poco::JSON::Array::Ptr empty_identifier_field_ids = new Poco::JSON::Array; - schema_for_rest->set("identifier-field-ids", empty_identifier_field_ids); - } - - if (old_schema_id >= 0) + auto parent_snapshot_id = new_snapshot->getValue("parent-snapshot-id"); + if (parent_snapshot_id != -1) { Poco::JSON::Object::Ptr requirement = new Poco::JSON::Object; - requirement->set("type", "assert-current-schema-id"); - requirement->set("current-schema-id", old_schema_id); + requirement->set("type", "assert-ref-snapshot-id"); + requirement->set("ref", "main"); + requirement->set("snapshot-id", parent_snapshot_id); Poco::JSON::Array::Ptr requirements = new Poco::JSON::Array; requirements->add(requirement); request_body->set("requirements", requirements); } + } - /// The target schema may be identical to a schema already present in the table's - /// schema history. The Iceberg catalog deduplicates identical schemas, so an - /// `add-schema` update becomes a no-op and a subsequent `set-current-schema: -1` - /// is rejected. In that case we point `set-current-schema` at the existing id. - std::optional existing_equivalent_schema_id; - for (UInt32 i = 0; i < schemas->size(); ++i) - { - auto existing_schema = schemas->getObject(i); - if (existing_schema->getValue(DB::Iceberg::f_schema_id) == new_schema_id) - continue; - if (schemasEquivalentIgnoringId(existing_schema, new_schema_obj)) - { - existing_equivalent_schema_id = existing_schema->getValue(DB::Iceberg::f_schema_id); - break; - } - } - - Poco::JSON::Array::Ptr updates = new Poco::JSON::Array; - if (existing_equivalent_schema_id.has_value()) - { - Poco::JSON::Object::Ptr set_current_schema = new Poco::JSON::Object; - set_current_schema->set("action", "set-current-schema"); - set_current_schema->set("schema-id", *existing_equivalent_schema_id); - updates->add(set_current_schema); - } - else - { - { - Poco::JSON::Object::Ptr add_schema = new Poco::JSON::Object; - add_schema->set("action", "add-schema"); - add_schema->set("schema", schema_for_rest); - if (new_snapshot->has(DB::Iceberg::f_last_column_id)) - add_schema->set("last-column-id", new_snapshot->getValue(DB::Iceberg::f_last_column_id)); - updates->add(add_schema); - } - { - Poco::JSON::Object::Ptr set_current_schema = new Poco::JSON::Object; - set_current_schema->set("action", "set-current-schema"); - set_current_schema->set("schema-id", -1); - updates->add(set_current_schema); - } - } - - if (sortOrderIncompatibleWithSchema(new_snapshot, new_schema_obj)) - { - Poco::JSON::Object::Ptr unsorted_sort_order = new Poco::JSON::Object; - unsorted_sort_order->set(DB::Iceberg::f_order_id, 0); - unsorted_sort_order->set(DB::Iceberg::f_fields, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); - - Poco::JSON::Object::Ptr add_sort_order = new Poco::JSON::Object; - add_sort_order->set("action", "add-sort-order"); - add_sort_order->set("sort-order", unsorted_sort_order); - updates->add(add_sort_order); - - Poco::JSON::Object::Ptr set_default_sort_order = new Poco::JSON::Object; - set_default_sort_order->set("action", "set-default-sort-order"); - set_default_sort_order->set("sort-order-id", -1); - updates->add(set_default_sort_order); - } - - request_body->set("updates", updates); + Poco::JSON::Array::Ptr updates = new Poco::JSON::Array; + { + Poco::JSON::Object::Ptr add_snapshot = new Poco::JSON::Object; + add_snapshot->set("action", "add-snapshot"); + add_snapshot->set("snapshot", new_snapshot); + updates->add(add_snapshot); } - else { - if (new_snapshot->has("parent-snapshot-id")) - { - auto parent_snapshot_id = new_snapshot->getValue("parent-snapshot-id"); - if (parent_snapshot_id != -1) - { - Poco::JSON::Object::Ptr requirement = new Poco::JSON::Object; - requirement->set("type", "assert-ref-snapshot-id"); - requirement->set("ref", "main"); - requirement->set("snapshot-id", parent_snapshot_id); - - Poco::JSON::Array::Ptr requirements = new Poco::JSON::Array; - requirements->add(requirement); - request_body->set("requirements", requirements); - } - } - - Poco::JSON::Array::Ptr updates = new Poco::JSON::Array; - { - Poco::JSON::Object::Ptr add_snapshot = new Poco::JSON::Object; - add_snapshot->set("action", "add-snapshot"); - add_snapshot->set("snapshot", new_snapshot); - updates->add(add_snapshot); - } - { - Poco::JSON::Object::Ptr set_snapshot = new Poco::JSON::Object; - set_snapshot->set("action", "set-snapshot-ref"); - set_snapshot->set("ref-name", "main"); - set_snapshot->set("type", "branch"); - set_snapshot->set("snapshot-id", new_snapshot->getValue("snapshot-id")); - updates->add(set_snapshot); - } - request_body->set("updates", updates); + Poco::JSON::Object::Ptr set_snapshot = new Poco::JSON::Object; + set_snapshot->set("action", "set-snapshot-ref"); + set_snapshot->set("ref-name", "main"); + set_snapshot->set("type", "branch"); + set_snapshot->set("snapshot-id", new_snapshot->getValue("snapshot-id")); + updates->add(set_snapshot); } + request_body->set("updates", updates); return request_body; } @@ -1689,8 +1462,6 @@ bool RestCatalog::updateMetadata(const String & namespace_name, const String & t "REST catalog does not support metadata-only updates without a snapshot " "(required for EXPIRE SNAPSHOTS)"); - fiu_do_on(DB::FailPoints::iceberg_alter_catalog_update_metadata_fail, { return false; }); - const std::string endpoint = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables" / table_name).generic_string(); auto request_body = buildUpdateMetadataRequestBody(namespace_name, table_name, new_snapshot); @@ -1703,9 +1474,14 @@ bool RestCatalog::updateMetadata(const String & namespace_name, const String & t } catch (const DB::HTTPException & ex) { - LOG_WARNING(log, "Iceberg REST updateMetadata for {}.{} failed: {}", - namespace_name, table_name, ex.displayText()); - return false; + const auto status = static_cast(ex.getHTTPStatus()); + if (status == 409 || status == 429 || status >= 500) + { + LOG_WARNING(log, "Iceberg REST updateMetadata for {}.{} got retryable HTTP {}: {}", + namespace_name, table_name, status, ex.displayText()); + return false; + } + throw; } return true; } @@ -1719,6 +1495,8 @@ bool RestCatalog::updateSchema( Int32 new_last_column_id, Poco::JSON::Object::Ptr metadata) const { + fiu_do_on(DB::FailPoints::iceberg_alter_catalog_update_metadata_fail, { return false; }); + const std::string endpoint = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables" / table_name).generic_string(); auto request_body = buildUpdateSchemaRequestBody( @@ -1730,9 +1508,14 @@ bool RestCatalog::updateSchema( } catch (const DB::HTTPException & ex) { - LOG_WARNING(log, "Iceberg REST updateSchema for {}.{} failed: {}", - namespace_name, table_name, ex.displayText()); - return false; + const auto status = static_cast(ex.getHTTPStatus()); + if (status == 409 || status == 429 || status >= 500) + { + LOG_WARNING(log, "Iceberg REST updateSchema for {}.{} got retryable HTTP {}: {}", + namespace_name, table_name, status, ex.displayText()); + return false; + } + throw; } return true; } diff --git a/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp b/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp index f1bb12c858f3..06dbc4591da1 100644 --- a/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp +++ b/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp @@ -9,7 +9,6 @@ #include #include #include -#include using namespace DB; @@ -33,105 +32,6 @@ TEST(RestCatalogUpdateMetadataBody, NullSnapshotReturnsNull) EXPECT_FALSE(body); } -TEST(RestCatalogUpdateMetadataBody, SchemaUpdateValid) -{ - Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; - Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; - Poco::JSON::Object::Ptr schema = new Poco::JSON::Object; - schema->set(Iceberg::f_schema_id, 1); - schema->set(Iceberg::f_type, "struct"); - schema->set(Iceberg::f_fields, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); - schemas->add(schema); - snapshot->set(Iceberg::f_schemas, schemas); - snapshot->set(Iceberg::f_current_schema_id, 1); - snapshot->set(Iceberg::f_last_column_id, 3); - - auto body = DataLake::buildUpdateMetadataRequestBody("my.ns", "tbl", snapshot); - ASSERT_TRUE(body); - - auto id = body->getObject("identifier"); - EXPECT_EQ(id->getValue("name"), "tbl"); - auto ns = id->getArray("namespace"); - ASSERT_EQ(ns->size(), 1u); - EXPECT_EQ(ns->getElement(0), "my.ns"); - - ASSERT_TRUE(body->has("requirements")); - auto req = body->getArray("requirements")->getObject(0); - EXPECT_EQ(req->getValue("type"), "assert-current-schema-id"); - EXPECT_EQ(req->getValue("current-schema-id"), 0); - - auto updates = body->getArray("updates"); - auto add_schema = findUpdateByAction(updates, "add-schema"); - ASSERT_TRUE(add_schema); - EXPECT_TRUE(add_schema->has("schema")); - EXPECT_EQ(add_schema->getValue("last-column-id"), 3); - - auto set_schema = findUpdateByAction(updates, "set-current-schema"); - ASSERT_TRUE(set_schema); - EXPECT_EQ(set_schema->getValue("schema-id"), -1); -} - -TEST(RestCatalogUpdateMetadataBody, SchemaUpdateCurrentIdZeroNoRequirement) -{ - Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; - Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; - Poco::JSON::Object::Ptr schema = new Poco::JSON::Object; - schema->set(Iceberg::f_schema_id, 0); - schema->set(Iceberg::f_type, "struct"); - schema->set(Iceberg::f_fields, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); - schemas->add(schema); - snapshot->set(Iceberg::f_schemas, schemas); - snapshot->set(Iceberg::f_current_schema_id, 0); - - auto body = DataLake::buildUpdateMetadataRequestBody("ns", "t", snapshot); - ASSERT_TRUE(body); - EXPECT_FALSE(body->has("requirements")); -} - -TEST(RestCatalogUpdateMetadataBody, SchemaUpdateBodyIsStringifiable) -{ - Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; - Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; - Poco::JSON::Object::Ptr schema = new Poco::JSON::Object; - schema->set(Iceberg::f_schema_id, 1); - schema->set(Iceberg::f_type, "struct"); - schema->set(Iceberg::f_fields, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); - schemas->add(schema); - snapshot->set(Iceberg::f_schemas, schemas); - snapshot->set(Iceberg::f_current_schema_id, 1); - - auto body = DataLake::buildUpdateMetadataRequestBody("ns", "t", snapshot); - ASSERT_TRUE(body); - - std::ostringstream oss; // STYLE_CHECK_ALLOW_STD_STRING_STREAM - ASSERT_NO_THROW(body->stringify(oss)); - EXPECT_NE(oss.str().find("\"identifier-field-ids\""), std::string::npos); - EXPECT_NE(oss.str().find("\"add-schema\""), std::string::npos); -} - -TEST(RestCatalogUpdateMetadataBody, SchemaUpdateMissingCurrentSchemaIdThrows) -{ - Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; - snapshot->set(Iceberg::f_schemas, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); - - EXPECT_THROW(DataLake::buildUpdateMetadataRequestBody("ns", "t", snapshot), DB::Exception); -} - -TEST(RestCatalogUpdateMetadataBody, SchemaUpdateNoMatchingSchemaIdThrows) -{ - Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; - Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; - Poco::JSON::Object::Ptr schema = new Poco::JSON::Object; - schema->set(Iceberg::f_schema_id, 1); - schema->set(Iceberg::f_type, "struct"); - schema->set(Iceberg::f_fields, Poco::JSON::Array::Ptr(new Poco::JSON::Array)); - schemas->add(schema); - snapshot->set(Iceberg::f_schemas, schemas); - snapshot->set(Iceberg::f_current_schema_id, 99); - - EXPECT_THROW(DataLake::buildUpdateMetadataRequestBody("ns", "t", snapshot), DB::Exception); -} - TEST(RestCatalogUpdateMetadataBody, SnapshotUpdateWithParent) { Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; @@ -183,138 +83,6 @@ TEST(RestCatalogUpdateMetadataBody, SnapshotUpdateParentMinusOneNoRequirement) EXPECT_FALSE(body->has("requirements")); } -TEST(RestCatalogUpdateSchemaBody, NestedSortKeySurvivesSchemaChange) -{ - Poco::JSON::Object::Ptr snapshot = new Poco::JSON::Object; - - Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; - Poco::JSON::Object::Ptr schema = new Poco::JSON::Object; - schema->set(Iceberg::f_schema_id, 0); - schema->set(Iceberg::f_type, "struct"); - - Poco::JSON::Array::Ptr fields = new Poco::JSON::Array; - - Poco::JSON::Object::Ptr struct_field = new Poco::JSON::Object; - struct_field->set(Iceberg::f_id, 1); - struct_field->set(Iceberg::f_name, "s"); - struct_field->set(Iceberg::f_required, false); - - Poco::JSON::Object::Ptr struct_type = new Poco::JSON::Object; - struct_type->set(Iceberg::f_type, "struct"); - Poco::JSON::Array::Ptr nested_fields = new Poco::JSON::Array; - Poco::JSON::Object::Ptr nested_field = new Poco::JSON::Object; - nested_field->set(Iceberg::f_id, 2); - nested_field->set(Iceberg::f_name, "x"); - nested_field->set(Iceberg::f_required, false); - nested_field->set(Iceberg::f_type, "int"); - nested_fields->add(nested_field); - struct_type->set(Iceberg::f_fields, nested_fields); - struct_field->set(Iceberg::f_type, struct_type); - fields->add(struct_field); - - Poco::JSON::Object::Ptr other_field = new Poco::JSON::Object; - other_field->set(Iceberg::f_id, 3); - other_field->set(Iceberg::f_name, "a"); - other_field->set(Iceberg::f_required, false); - other_field->set(Iceberg::f_type, "int"); - fields->add(other_field); - - schema->set(Iceberg::f_fields, fields); - schemas->add(schema); - - Poco::JSON::Object::Ptr new_schema = new Poco::JSON::Object; - new_schema->set(Iceberg::f_schema_id, 1); - new_schema->set(Iceberg::f_type, "struct"); - Poco::JSON::Array::Ptr new_fields = new Poco::JSON::Array; - new_fields->add(struct_field); - new_schema->set(Iceberg::f_fields, new_fields); - schemas->add(new_schema); - - snapshot->set(Iceberg::f_schemas, schemas); - snapshot->set(Iceberg::f_current_schema_id, 1); - snapshot->set(Iceberg::f_last_column_id, 3); - - Poco::JSON::Array::Ptr sort_orders = new Poco::JSON::Array; - Poco::JSON::Object::Ptr sort_order = new Poco::JSON::Object; - sort_order->set(Iceberg::f_order_id, 1); - Poco::JSON::Array::Ptr sort_fields = new Poco::JSON::Array; - Poco::JSON::Object::Ptr sort_field = new Poco::JSON::Object; - sort_field->set(Iceberg::f_source_id, 2); - sort_field->set("transform", "identity"); - sort_field->set("direction", "asc"); - sort_field->set("null-order", "nulls-first"); - sort_fields->add(sort_field); - sort_order->set(Iceberg::f_fields, sort_fields); - sort_orders->add(sort_order); - snapshot->set(Iceberg::f_sort_orders, sort_orders); - snapshot->set(Iceberg::f_default_sort_order_id, static_cast(1)); - - auto body = DataLake::buildUpdateMetadataRequestBody("ns", "t", snapshot); - ASSERT_TRUE(body); - - auto updates = body->getArray("updates"); - EXPECT_FALSE(findUpdateByAction(updates, "add-sort-order")); -} - -TEST(RestCatalogUpdateSchemaBody, DroppedSortKeyResetsSortOrder) -{ - Poco::JSON::Object::Ptr metadata = new Poco::JSON::Object; - - Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; - Poco::JSON::Object::Ptr schema = new Poco::JSON::Object; - schema->set(Iceberg::f_schema_id, 0); - schema->set(Iceberg::f_type, "struct"); - Poco::JSON::Array::Ptr fields = new Poco::JSON::Array; - Poco::JSON::Object::Ptr field1 = new Poco::JSON::Object; - field1->set(Iceberg::f_id, 1); - field1->set(Iceberg::f_name, "a"); - field1->set(Iceberg::f_required, false); - field1->set(Iceberg::f_type, "int"); - fields->add(field1); - Poco::JSON::Object::Ptr field2 = new Poco::JSON::Object; - field2->set(Iceberg::f_id, 2); - field2->set(Iceberg::f_name, "b"); - field2->set(Iceberg::f_required, false); - field2->set(Iceberg::f_type, "int"); - fields->add(field2); - schema->set(Iceberg::f_fields, fields); - schemas->add(schema); - metadata->set(Iceberg::f_schemas, schemas); - metadata->set(Iceberg::f_current_schema_id, 0); - - Poco::JSON::Array::Ptr sort_orders = new Poco::JSON::Array; - Poco::JSON::Object::Ptr sort_order = new Poco::JSON::Object; - sort_order->set(Iceberg::f_order_id, 1); - Poco::JSON::Array::Ptr sort_fields = new Poco::JSON::Array; - Poco::JSON::Object::Ptr sort_field = new Poco::JSON::Object; - sort_field->set(Iceberg::f_source_id, 2); - sort_field->set("transform", "identity"); - sort_field->set("direction", "asc"); - sort_field->set("null-order", "nulls-first"); - sort_fields->add(sort_field); - sort_order->set(Iceberg::f_fields, sort_fields); - sort_orders->add(sort_order); - metadata->set(Iceberg::f_sort_orders, sort_orders); - metadata->set(Iceberg::f_default_sort_order_id, static_cast(1)); - - Poco::JSON::Object::Ptr new_schema = new Poco::JSON::Object; - new_schema->set(Iceberg::f_schema_id, 1); - new_schema->set(Iceberg::f_type, "struct"); - Poco::JSON::Array::Ptr new_fields = new Poco::JSON::Array; - new_fields->add(field1); - new_schema->set(Iceberg::f_fields, new_fields); - - auto body = DataLake::buildUpdateSchemaRequestBody("ns", "t", metadata, new_schema, 0, 2); - ASSERT_TRUE(body); - - auto updates = body->getArray("updates"); - auto add_sort = findUpdateByAction(updates, "add-sort-order"); - ASSERT_TRUE(add_sort); - auto set_sort = findUpdateByAction(updates, "set-default-sort-order"); - ASSERT_TRUE(set_sort); - EXPECT_EQ(set_sort->getValue("sort-order-id"), -1); -} - TEST(RestCatalogUpdateSchemaBody, EquivalentSchemaDeduplicates) { Poco::JSON::Object::Ptr metadata = new Poco::JSON::Object; diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp index 09415cfbf9de..d077a7d24795 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -11,6 +12,7 @@ #include #include +#include #include #include @@ -113,6 +115,17 @@ bool icebergTypesEqual(Poco::Dynamic::Var old_type, Poco::Dynamic::Var new_type) return false; } +/// Allocate the next schema id as max(existing schema ids) + 1 to avoid +/// collisions when current-schema-id is not the highest in the list. +Int32 getNextSchemaId(Poco::JSON::Object::Ptr metadata_object) +{ + Int32 max_id = 0; + auto schemas = metadata_object->getArray(Iceberg::f_schemas); + for (UInt32 i = 0; i < schemas->size(); ++i) + max_id = std::max(max_id, schemas->getObject(i)->getValue(Iceberg::f_schema_id)); + return max_id + 1; +} + } MetadataGenerator::MetadataGenerator(Poco::JSON::Object::Ptr metadata_object_) @@ -283,7 +296,7 @@ MetadataGenerator::NextMetadataResult MetadataGenerator::generateNextMetadata( void MetadataGenerator::generateDropColumnMetadata(const String & column_name) { auto current_schema_id = metadata_object->getValue(Iceberg::f_current_schema_id); - metadata_object->set(Iceberg::f_current_schema_id, current_schema_id + 1); + const auto next_schema_id = getNextSchemaId(metadata_object); Poco::JSON::Object::Ptr current_schema; auto schemas = metadata_object->getArray(Iceberg::f_schemas); @@ -302,18 +315,77 @@ void MetadataGenerator::generateDropColumnMetadata(const String & column_name) auto fields = current_schema->getArray(Iceberg::f_fields); UInt32 index_to_drop = static_cast(fields->size()); + Int32 dropped_field_id = -1; for (UInt32 i = 0; i < fields->size(); ++i) { if (fields->getObject(i)->getValue(Iceberg::f_name) == column_name) { index_to_drop = i; + dropped_field_id = fields->getObject(i)->getValue(Iceberg::f_id); break; } } if (index_to_drop == fields->size()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Not found column {}", column_name); + + /// Reject the drop if the column is referenced by the active sort order. + if (metadata_object->has(Iceberg::f_sort_orders) && metadata_object->has(Iceberg::f_default_sort_order_id)) + { + auto default_sort_order_id = metadata_object->getValue(Iceberg::f_default_sort_order_id); + if (default_sort_order_id != 0) + { + auto sort_orders = metadata_object->getArray(Iceberg::f_sort_orders); + for (UInt32 i = 0; i < sort_orders->size(); ++i) + { + auto sort_order = sort_orders->getObject(i); + if (sort_order->getValue(Iceberg::f_order_id) != default_sort_order_id) + continue; + if (!sort_order->has(Iceberg::f_fields)) + break; + auto sort_fields = sort_order->getArray(Iceberg::f_fields); + for (UInt32 j = 0; j < sort_fields->size(); ++j) + { + auto sf = sort_fields->getObject(j); + if (sf->has(Iceberg::f_source_id) && sf->getValue(Iceberg::f_source_id) == dropped_field_id) + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "Cannot drop column '{}' (field id {}): it is referenced by the active sort order", + column_name, dropped_field_id); + } + break; + } + } + } + + /// Reject the drop if the column is referenced by the active partition spec. + if (metadata_object->has(Iceberg::f_partition_specs) && metadata_object->has(Iceberg::f_default_spec_id)) + { + auto default_spec_id = metadata_object->getValue(Iceberg::f_default_spec_id); + auto partition_specs = metadata_object->getArray(Iceberg::f_partition_specs); + for (UInt32 i = 0; i < partition_specs->size(); ++i) + { + auto spec = partition_specs->getObject(i); + if (spec->getValue(Iceberg::f_spec_id) != default_spec_id) + continue; + if (!spec->has(Iceberg::f_fields)) + break; + auto spec_fields = spec->getArray(Iceberg::f_fields); + for (UInt32 j = 0; j < spec_fields->size(); ++j) + { + auto pf = spec_fields->getObject(j); + if (pf->has(Iceberg::f_source_id) && pf->getValue(Iceberg::f_source_id) == dropped_field_id) + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "Cannot drop column '{}' (field id {}): it is referenced by the active partition spec", + column_name, dropped_field_id); + } + break; + } + } + current_schema->getArray(Iceberg::f_fields)->remove(index_to_drop); - current_schema->set(Iceberg::f_schema_id, current_schema_id + 1); + current_schema->set(Iceberg::f_schema_id, next_schema_id); + metadata_object->set(Iceberg::f_current_schema_id, next_schema_id); metadata_object->getArray(Iceberg::f_schemas)->add(current_schema); } @@ -322,7 +394,7 @@ void MetadataGenerator::generateAddColumnMetadata(const String & column_name, Da if (!type->isNullable()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Iceberg spec doesn't allow to add non-nullable columns"); auto current_schema_id = metadata_object->getValue(Iceberg::f_current_schema_id); - metadata_object->set(Iceberg::f_current_schema_id, current_schema_id + 1); + const auto next_schema_id = getNextSchemaId(metadata_object); Poco::JSON::Object::Ptr current_schema; auto schemas = metadata_object->getArray(Iceberg::f_schemas); @@ -358,7 +430,8 @@ void MetadataGenerator::generateAddColumnMetadata(const String & column_name, Da metadata_object->set(Iceberg::f_last_column_id, last_column_id + 1); current_schema->getArray(Iceberg::f_fields)->add(new_field); - current_schema->set(Iceberg::f_schema_id, current_schema_id + 1); + current_schema->set(Iceberg::f_schema_id, next_schema_id); + metadata_object->set(Iceberg::f_current_schema_id, next_schema_id); metadata_object->getArray(Iceberg::f_schemas)->add(current_schema); } @@ -391,7 +464,35 @@ bool MetadataGenerator::generateModifyColumnMetadata(const String & column_name, { if (current_field->getValue(Iceberg::f_required) == new_type.second && icebergTypesEqual(current_field->get(Iceberg::f_type), new_type.first)) + { + /// Iceberg types are identical. Reconstruct the ClickHouse type the + /// existing field maps back to and check whether it equals the + /// requested type. For simple string-typed fields we can use + /// IcebergSchemaProcessor::getSimpleType; for complex types (JSON + /// objects) reconstruction is lossy so we allow the no-op silently. + auto existing_iceberg_type = current_field->get(Iceberg::f_type); + if (existing_iceberg_type.isString()) + { + auto reconstructed_ch_type = Iceberg::IcebergSchemaProcessor::getSimpleType( + existing_iceberg_type.extract(), /* allow_geo_parser */ false); + if (!current_field->getValue(Iceberg::f_required) && reconstructed_ch_type->canBeInsideNullable()) + reconstructed_ch_type = makeNullable(reconstructed_ch_type); + + auto requested_type_normalized = type; + if (reconstructed_ch_type->equals(*requested_type_normalized)) + return false; + + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "Cannot MODIFY COLUMN '{}' from {} to {}: both map to the same Iceberg type '{}' " + "so the change cannot be recorded in the Iceberg schema", + column_name, + reconstructed_ch_type->getName(), + requested_type_normalized->getName(), + existing_iceberg_type.extract()); + } return false; + } if (!checkValidSchemaEvolution(current_field->get(Iceberg::f_type), new_type.first)) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Iceberg spec doesn't allow schema evolution to type {}", type->getPrettyName()); @@ -399,6 +500,8 @@ bool MetadataGenerator::generateModifyColumnMetadata(const String & column_name, if (!current_field->getValue(Iceberg::f_required) && !type->isNullable()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Iceberg spec doesn't allow change type from nullable to non-nullable {}", type->getPrettyName()); + const auto next_schema_id = getNextSchemaId(metadata_object); + current_schema = deepCopy(current_schema); schema_fields = current_schema->getArray(Iceberg::f_fields); current_field = schema_fields->getObject(i); @@ -406,8 +509,8 @@ bool MetadataGenerator::generateModifyColumnMetadata(const String & column_name, current_field->set(Iceberg::f_type, new_type.first); current_field->set(Iceberg::f_required, new_type.second); - metadata_object->set(Iceberg::f_current_schema_id, current_schema_id + 1); - current_schema->set(Iceberg::f_schema_id, current_schema_id + 1); + metadata_object->set(Iceberg::f_current_schema_id, next_schema_id); + current_schema->set(Iceberg::f_schema_id, next_schema_id); metadata_object->getArray(Iceberg::f_schemas)->add(current_schema); metadata_object->set(Iceberg::f_last_column_id, last_column_id); return true; @@ -459,8 +562,9 @@ void MetadataGenerator::generateRenameColumnMetadata(const String & column_name, if (!found) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Not found column {}", column_name); - metadata_object->set(Iceberg::f_current_schema_id, current_schema_id + 1); - current_schema->set(Iceberg::f_schema_id, current_schema_id + 1); + const auto next_schema_id = getNextSchemaId(metadata_object); + metadata_object->set(Iceberg::f_current_schema_id, next_schema_id); + current_schema->set(Iceberg::f_schema_id, next_schema_id); metadata_object->getArray(Iceberg::f_schemas)->add(current_schema); } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp index 2f072b5ca3f2..90b9582f9165 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -41,6 +42,7 @@ namespace DB::ErrorCodes extern const int BAD_ARGUMENTS; extern const int LOGICAL_ERROR; extern const int LIMIT_EXCEEDED; +extern const int QUERY_WAS_CANCELLED; } namespace DB::DataLakeStorageSetting @@ -774,6 +776,9 @@ void alter( bool succeeded = false; while (i < MAX_TRANSACTION_RETRIES) { + if (auto elem = context->getProcessListElement(); elem && elem->isKilled()) + throw Exception(ErrorCodes::QUERY_WAS_CANCELLED, "ALTER TABLE cancelled during retry loop"); + auto log = getLogger("IcebergMutations"); int last_version = 0; @@ -871,11 +876,12 @@ void alter( const auto new_schema_id = metadata->getValue(Iceberg::f_current_schema_id); Poco::JSON::Object::Ptr new_schema; auto schemas = metadata->getArray(Iceberg::f_schemas); - for (UInt32 schema_index = 0; schema_index < schemas->size(); ++schema_index) + for (auto schema_index = schemas->size(); schema_index > 0; --schema_index) { - if (schemas->getObject(schema_index)->getValue(Iceberg::f_schema_id) == new_schema_id) + auto candidate = schemas->getObject(static_cast(schema_index - 1)); + if (candidate->getValue(Iceberg::f_schema_id) == new_schema_id) { - new_schema = schemas->getObject(schema_index); + new_schema = candidate; break; } } @@ -922,7 +928,9 @@ void alter( } if (!succeeded) - throw Exception(ErrorCodes::LIMIT_EXCEEDED, "Too many unsuccessed retries to alter iceberg table"); + throw Exception(ErrorCodes::LIMIT_EXCEEDED, + "ALTER TABLE commit kept losing to concurrent modifications after {} retries", + MAX_TRANSACTION_RETRIES); /// Invalidate the metadata files cache so that subsequent operations on this table see the /// schema we just wrote. See `PersistentTableComponents::invalidateMetadataCache` for the diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp new file mode 100644 index 000000000000..a8727f8ce248 --- /dev/null +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp @@ -0,0 +1,191 @@ +#include "config.h" + +#if USE_AVRO + +#include + +#include +#include +#include +#include +#include +#include +#include + +using namespace DB; +using namespace DB::Iceberg; + +namespace +{ + +Poco::JSON::Object::Ptr makeMinimalMetadata(Int32 current_schema_id, Int32 last_column_id) +{ + auto metadata = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + metadata->set(f_format_version, 2); + metadata->set(f_current_schema_id, current_schema_id); + metadata->set(f_last_column_id, last_column_id); + + auto schemas = Poco::JSON::Array::Ptr(new Poco::JSON::Array); + auto schema = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + schema->set(f_schema_id, current_schema_id); + schema->set(f_type, "struct"); + + auto fields = Poco::JSON::Array::Ptr(new Poco::JSON::Array); + auto field = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + field->set(f_id, 1); + field->set(f_name, "x"); + field->set(f_required, true); + field->set(f_type, "int"); + fields->add(field); + schema->set(f_fields, fields); + schemas->add(schema); + metadata->set(f_schemas, schemas); + + return metadata; +} + +Poco::JSON::Object::Ptr makeMetadataWithGap() +{ + auto metadata = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + metadata->set(f_format_version, 2); + metadata->set(f_current_schema_id, 0); + metadata->set(f_last_column_id, 2); + + auto schemas = Poco::JSON::Array::Ptr(new Poco::JSON::Array); + + auto schema0 = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + schema0->set(f_schema_id, 0); + schema0->set(f_type, "struct"); + auto fields0 = Poco::JSON::Array::Ptr(new Poco::JSON::Array); + auto field_x = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + field_x->set(f_id, 1); + field_x->set(f_name, "x"); + field_x->set(f_required, true); + field_x->set(f_type, "int"); + fields0->add(field_x); + auto field_y = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + field_y->set(f_id, 2); + field_y->set(f_name, "y"); + field_y->set(f_required, false); + field_y->set(f_type, "string"); + fields0->add(field_y); + schema0->set(f_fields, fields0); + schemas->add(schema0); + + // Simulate a historical schema with id=5 (higher than current-schema-id=0) + auto schema5 = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + schema5->set(f_schema_id, 5); + schema5->set(f_type, "struct"); + auto fields5 = Poco::JSON::Array::Ptr(new Poco::JSON::Array); + fields5->add(field_x); + schema5->set(f_fields, fields5); + schemas->add(schema5); + + metadata->set(f_schemas, schemas); + return metadata; +} + +} + + +TEST(IcebergMetadataGenerator, AddColumnAllocatesSchemaIdAboveMax) +{ + auto metadata = makeMetadataWithGap(); + MetadataGenerator gen(metadata); + + gen.generateAddColumnMetadata("z", makeNullable(std::make_shared())); + + auto new_schema_id = metadata->getValue(f_current_schema_id); + EXPECT_EQ(new_schema_id, 6); + + auto schemas = metadata->getArray(f_schemas); + bool found = false; + for (UInt32 i = 0; i < schemas->size(); ++i) + { + if (schemas->getObject(i)->getValue(f_schema_id) == 6) + { + found = true; + break; + } + } + EXPECT_TRUE(found); +} + + +TEST(IcebergMetadataGenerator, DropColumnAllocatesSchemaIdAboveMax) +{ + auto metadata = makeMetadataWithGap(); + MetadataGenerator gen(metadata); + + gen.generateDropColumnMetadata("y"); + + EXPECT_EQ(metadata->getValue(f_current_schema_id), 6); +} + + +TEST(IcebergMetadataGenerator, DropColumnRejectsIfInSortOrder) +{ + auto metadata = makeMinimalMetadata(0, 1); + + auto sort_orders = Poco::JSON::Array::Ptr(new Poco::JSON::Array); + auto sort_order = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + sort_order->set(f_order_id, static_cast(1)); + auto sort_fields = Poco::JSON::Array::Ptr(new Poco::JSON::Array); + auto sf = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + sf->set(f_source_id, 1); + sf->set("transform", "identity"); + sf->set("direction", "asc"); + sf->set("null-order", "nulls-first"); + sort_fields->add(sf); + sort_order->set(f_fields, sort_fields); + sort_orders->add(sort_order); + metadata->set(f_sort_orders, sort_orders); + metadata->set(f_default_sort_order_id, static_cast(1)); + + MetadataGenerator gen(metadata); + EXPECT_THROW(gen.generateDropColumnMetadata("x"), DB::Exception); +} + + +TEST(IcebergMetadataGenerator, DropColumnRejectsIfInPartitionSpec) +{ + auto metadata = makeMinimalMetadata(0, 1); + + auto partition_specs = Poco::JSON::Array::Ptr(new Poco::JSON::Array); + auto spec = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + spec->set(f_spec_id, static_cast(1)); + auto spec_fields = Poco::JSON::Array::Ptr(new Poco::JSON::Array); + auto pf = Poco::JSON::Object::Ptr(new Poco::JSON::Object); + pf->set(f_source_id, 1); + pf->set("transform", "identity"); + pf->set("name", "x_part"); + spec_fields->add(pf); + spec->set(f_fields, spec_fields); + partition_specs->add(spec); + metadata->set(f_partition_specs, partition_specs); + metadata->set(f_default_spec_id, static_cast(1)); + + MetadataGenerator gen(metadata); + EXPECT_THROW(gen.generateDropColumnMetadata("x"), DB::Exception); +} + + +TEST(IcebergMetadataGenerator, ModifyColumnNoopSameType) +{ + auto metadata = makeMinimalMetadata(0, 1); + MetadataGenerator gen(metadata); + + bool changed = gen.generateModifyColumnMetadata("x", std::make_shared()); + EXPECT_FALSE(changed); +} + + +TEST(IcebergMetadataGenerator, ModifyColumnRejectsIndistinguishableType) +{ + auto metadata = makeMinimalMetadata(0, 1); + MetadataGenerator gen(metadata); + + EXPECT_THROW(gen.generateModifyColumnMetadata("x", std::make_shared()), DB::Exception); +} + +#endif diff --git a/tests/integration/test_database_iceberg/test.py b/tests/integration/test_database_iceberg/test.py index 5e139e26e954..02f3bf3b60ef 100644 --- a/tests/integration/test_database_iceberg/test.py +++ b/tests/integration/test_database_iceberg/test.py @@ -745,6 +745,109 @@ def test_insert(started_cluster): assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}` ORDER BY ALL") == "\\N\tAAPL\t193.24\t193.31\t('bot')\n\\N\tPavel Ivanov (pudge1000-7) pereezhai v amsterdam\t193.24\t193.31\t('bot')\n" +def test_optimize_manifest_with_catalog(started_cluster): + # OPTIMIZE TABLE ... MANIFEST on a catalog-managed table must consolidate the per-insert manifests + # and commit the new snapshot back through the catalog, without changing the data. + node = started_cluster.instances["node1"] + + test_ref = f"test_optimize_manifest_{uuid.uuid4()}" + table_name = f"{test_ref}_table" + root_namespace = f"{test_ref}_namespace" + + catalog = load_catalog_impl(started_cluster) + catalog.create_namespace(root_namespace) + # Unpartitioned table, so every per-insert data manifest can consolidate into a single one. + create_table(catalog, root_namespace, table_name, DEFAULT_SCHEMA, PartitionSpec(), DEFAULT_SORT_ORDER) + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + + table_ref = f"{CATALOG_NAME}.`{root_namespace}.{table_name}`" + write_settings = {"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1} + + # Several separate inserts -> several snapshots, each adding its own data manifest. + num_inserts = 5 + for i in range(num_inserts): + node.query( + f"INSERT INTO {table_ref} VALUES (NULL, 'sym{i}', {100 + i}, {200 + i}, tuple('bot'));", + settings=write_settings, + ) + + def current_snapshot_id(): + # Read the current snapshot from the catalog's metadata.json (avoids parsing the manifest-list + # Avro, which pyiceberg rejects because ClickHouse omits field-ids there). + table = catalog.load_table(f"{root_namespace}.{table_name}") + assert table.current_snapshot() is not None, "expected a current snapshot after inserts" + return table.metadata.current_snapshot_id + + snapshot_id_before = current_snapshot_id() + rows_before = node.query(f"SELECT symbol, bid, ask FROM {table_ref} ORDER BY ALL") + + node.query( + f"OPTIMIZE TABLE {table_ref} MANIFEST", + settings={ + "allow_experimental_iceberg_compaction": 1, + "iceberg_manifest_min_count_to_compact": 2, + "allow_insert_into_iceberg": 1, + "write_full_path_in_iceberg_metadata": 1, + }, + ) + + # The compaction must commit a new (replace) snapshot back through the catalog. + assert current_snapshot_id() != snapshot_id_before, ( + "OPTIMIZE TABLE ... MANIFEST did not commit a new snapshot through the catalog" + ) + + # The metadata-only rewrite must not change the data. + rows_after = node.query(f"SELECT symbol, bid, ask FROM {table_ref} ORDER BY ALL") + assert rows_after == rows_before + + +@pytest.mark.parametrize( + "fields_to_remove", + [ + ["snapshots"], + ["metadata-log"], + ["snapshot-log"], + ["snapshots", "metadata-log", "snapshot-log"], + ], +) +def test_insert_into_table_without_optional_metadata_arrays(started_cluster, fields_to_remove): + # The Iceberg spec marks snapshots / metadata-log / snapshot-log as optional, so external + # engines may create empty-table metadata that omits any of them. Inserting into such a table + # must still succeed instead of aborting in the metadata write path. + node = started_cluster.instances["node1"] + + test_ref = f"test_insert_no_optional_arrays_{uuid.uuid4()}" + table_name = f"{test_ref}_table" + root_namespace = f"{test_ref}_namespace" + + catalog = load_catalog_impl(started_cluster) + catalog.create_namespace(root_namespace) + create_table(catalog, root_namespace, table_name, DEFAULT_SCHEMA, PartitionSpec(), DEFAULT_SORT_ORDER) + + iceberg_table = catalog.load_table(f"{root_namespace}.{table_name}") + assert iceberg_table.metadata_location.startswith("s3://") + metadata_bucket, metadata_key = iceberg_table.metadata_location[len("s3://"):].split("/", 1) + metadata = json.loads(get_file_contents(started_cluster.minio_client, metadata_bucket, metadata_key)) + for field in fields_to_remove: + metadata.pop(field, None) + metadata_bytes = json.dumps(metadata).encode() + started_cluster.minio_client.put_object( + metadata_bucket, + metadata_key, + io.BytesIO(metadata_bytes), + len(metadata_bytes), + content_type="application/json", + ) + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + node.query( + f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_name}` VALUES (NULL, 'AAPL', 193.24, 193.31, tuple('bot'));", + settings={"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1}, + ) + assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") == "\\N\tAAPL\t193.24\t193.31\t('bot')\n" + + def test_create(started_cluster): node = started_cluster.instances["node1"] @@ -1116,6 +1219,16 @@ def test_writes_schema_evolution_drop_last_column(started_cluster): assert node.query(f"SELECT x, y FROM {table_ref} ORDER BY ALL", settings=write_settings) == "abc\t1\n" + # Add another column after the drop to exercise schema-id allocation when + # current-schema-id is not the highest in the schemas list (Fix 1 reproducer). + node.query(f"ALTER TABLE {table_ref} ADD COLUMN w Nullable(Int64);", settings=write_settings) + desc = node.query(f"DESCRIBE TABLE {table_ref}", settings=write_settings) + assert "w" in desc + assert "z" not in desc + + node.query(f"INSERT INTO {table_ref} (x, y, w) VALUES ('def', 2, 42);", settings=write_settings) + assert node.query(f"SELECT x, y, w FROM {table_ref} ORDER BY x", settings=write_settings) == "abc\t1\t\\N\ndef\t2\t42\n" + def test_writes_schema_evolution_concurrent_add_columns(started_cluster): node = started_cluster.instances["node1"] @@ -1365,4 +1478,3 @@ def test_partitioning_by_string(started_cluster): create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{namespace}.{table_name}`") == "a:b,c[d=e/f%g?h\ttest\t12:00:00.000000\n" - diff --git a/tests/integration/test_storage_iceberg_no_spark/test_writes_modify_column.py b/tests/integration/test_storage_iceberg_no_spark/test_writes_modify_column.py index 314927cd586b..2c392a648817 100644 --- a/tests/integration/test_storage_iceberg_no_spark/test_writes_modify_column.py +++ b/tests/integration/test_storage_iceberg_no_spark/test_writes_modify_column.py @@ -68,3 +68,53 @@ def test_modify_column_errors(started_cluster_iceberg_no_spark, format_version, assert instance.query( f"SELECT name FROM system.columns WHERE database = currentDatabase() AND table = '{TABLE_NAME}' ORDER BY name" ) == "id\nvalue\n" + + +@pytest.mark.parametrize("format_version", [1, 2]) +@pytest.mark.parametrize("storage_type", ["local", "s3"]) +def test_modify_column_noop_same_type(started_cluster_iceberg_no_spark, format_version, storage_type): + """MODIFY COLUMN to the same type (Int32→Int32) is a no-op and must succeed silently.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + TABLE_NAME = "test_modify_noop_" + storage_type + "_" + get_uuid_str() + + create_iceberg_table( + storage_type, + instance, + TABLE_NAME, + started_cluster_iceberg_no_spark, + "(id Int32, value Nullable(String))", + format_version, + ) + + instance.query(f"INSERT INTO {TABLE_NAME} VALUES (1, 'a');", settings=INSERT_SETTINGS) + + # MODIFY to the exact same type should be a silent no-op (no schema change). + instance.query(f"ALTER TABLE {TABLE_NAME} MODIFY COLUMN id Int32;", settings=INSERT_SETTINGS) + + assert instance.query(f"SELECT id, value FROM {TABLE_NAME} ORDER BY id") == "1\ta\n" + + +@pytest.mark.parametrize("format_version", [1, 2]) +@pytest.mark.parametrize("storage_type", ["local", "s3"]) +def test_modify_column_rejects_indistinguishable_type(started_cluster_iceberg_no_spark, format_version, storage_type): + """MODIFY COLUMN id UInt32 on an Iceberg 'int' column (Int32) must fail because + Iceberg represents both as 'int' and the change cannot be recorded.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + TABLE_NAME = "test_modify_reject_" + storage_type + "_" + get_uuid_str() + + create_iceberg_table( + storage_type, + instance, + TABLE_NAME, + started_cluster_iceberg_no_spark, + "(id Int32, value Nullable(String))", + format_version, + ) + + instance.query(f"INSERT INTO {TABLE_NAME} VALUES (1, 'x');", settings=INSERT_SETTINGS) + + error = instance.query_and_get_error( + f"ALTER TABLE {TABLE_NAME} MODIFY COLUMN id UInt32;", + settings=INSERT_SETTINGS, + ) + assert "same iceberg type" in error.lower() or "cannot modify" in error.lower() or "bad_arguments" in error.lower() From a73db85c35ca2f1e88f6403abf59c7bb4042c64f Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Wed, 12 Aug 2026 18:38:37 +0200 Subject: [PATCH 14/18] Fix high defect with nullptr in failsafe --- src/Common/FailPoint.cpp | 1 + src/Databases/DataLake/RestCatalog.cpp | 6 + .../DataLakes/DataLakeConfiguration.h | 6 + .../DataLakes/Iceberg/MetadataGenerator.cpp | 143 ++++++++++-------- .../DataLakes/Iceberg/MetadataGenerator.h | 13 ++ .../DataLakes/Iceberg/Mutations.cpp | 61 ++++++-- .../gtest_iceberg_metadata_generator.cpp | 53 +++++++ .../integration/test_database_iceberg/test.py | 72 +++++++++ 8 files changed, 283 insertions(+), 72 deletions(-) diff --git a/src/Common/FailPoint.cpp b/src/Common/FailPoint.cpp index 736969ba372f..c0023b47f43d 100644 --- a/src/Common/FailPoint.cpp +++ b/src/Common/FailPoint.cpp @@ -164,6 +164,7 @@ static struct InitFiu REGULAR(slowdown_parallel_replicas_local_plan_read) \ ONCE(iceberg_writes_cleanup) \ ONCE(iceberg_alter_catalog_update_metadata_fail) \ + ONCE(iceberg_alter_catalog_commit_reported_as_failed) \ REGULAR(iceberg_alter_orphan_metadata_cleanup_fail) \ REGULAR(datalake_iceberg_metadata_create_fail) \ REGULAR(storage_cluster_read_sleep) \ diff --git a/src/Databases/DataLake/RestCatalog.cpp b/src/Databases/DataLake/RestCatalog.cpp index 60cd1ecff44b..d75993e74b0d 100644 --- a/src/Databases/DataLake/RestCatalog.cpp +++ b/src/Databases/DataLake/RestCatalog.cpp @@ -71,6 +71,7 @@ namespace DB::FailPoints { extern const char check_database_datalake_negative[]; extern const char iceberg_alter_catalog_update_metadata_fail[]; + extern const char iceberg_alter_catalog_commit_reported_as_failed[]; } namespace DataLake @@ -1517,6 +1518,11 @@ bool RestCatalog::updateSchema( } throw; } + + /// Simulates the Iceberg "commit state unknown" case: the catalog applied the update but the + /// client observes a failure, e.g. because a proxy turned the response into a 5xx. + fiu_do_on(DB::FailPoints::iceberg_alter_catalog_commit_reported_as_failed, { return false; }); + return true; } diff --git a/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h b/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h index bdc5e173addc..55a576ddd239 100644 --- a/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h +++ b/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h @@ -179,12 +179,14 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl void checkMutationIsPossible(ObjectStoragePtr object_storage, ContextPtr context, const MutationCommands & commands) override { lazyInitializeIfNeeded(object_storage, context); + assertInitialized(); current_metadata->checkMutationIsPossible(commands); } void checkAlterIsPossible(ObjectStoragePtr object_storage, ContextPtr context, const AlterCommands & commands) override { lazyInitializeIfNeeded(object_storage, context); + assertInitialized(); current_metadata->checkAlterIsPossible(commands); } @@ -196,6 +198,7 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl std::shared_ptr catalog) override { lazyInitializeIfNeeded(object_storage, context); + assertInitialized(); current_metadata->alter(params, context, storage_id, catalog); } @@ -350,6 +353,7 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl std::shared_ptr catalog) override { lazyInitializeIfNeeded(object_storage, context); + assertInitialized(); return current_metadata->write( sample_block, table_id, @@ -385,11 +389,13 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl bool optimize(ObjectStoragePtr object_storage, const StorageMetadataPtr & metadata_snapshot, ContextPtr context, const std::optional & format_settings) override { lazyInitializeIfNeeded(object_storage, context); + assertInitialized(); return current_metadata->optimize(metadata_snapshot, context, format_settings); } void addDeleteTransformers(ObjectInfoPtr object_info, QueryPipelineBuilder & builder, const std::optional & format_settings, FormatParserSharedResourcesPtr parser_shared_resources, ContextPtr local_context) const override { + assertInitialized(); current_metadata->addDeleteTransformers(object_info, builder, format_settings, parser_shared_resources, local_context); } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp index d077a7d24795..65e6d34a7586 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp @@ -154,6 +154,84 @@ Int64 MetadataGenerator::getMaxSequenceNumber() return max_seq_number; } +Poco::JSON::Object::Ptr MetadataGenerator::findCurrentSchema() const +{ + auto current_schema_id = metadata_object->getValue(Iceberg::f_current_schema_id); + auto schemas = metadata_object->getArray(Iceberg::f_schemas); + for (UInt32 i = 0; i < schemas->size(); ++i) + { + if (schemas->getObject(i)->getValue(Iceberg::f_schema_id) == current_schema_id) + return schemas->getObject(i); + } + return nullptr; +} + +Poco::JSON::Object::Ptr MetadataGenerator::getCurrentSchema() const +{ + auto current_schema = findCurrentSchema(); + if (!current_schema) + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "Not found schema with id {}", + metadata_object->getValue(Iceberg::f_current_schema_id)); + return current_schema; +} + +bool MetadataGenerator::isAddColumnApplied(const String & column_name, DataTypePtr type) const +{ + auto current_schema = findCurrentSchema(); + if (!current_schema) + return false; + + Int32 unused_field_id = metadata_object->getValue(Iceberg::f_last_column_id); + auto expected_type = Iceberg::getIcebergType(type, unused_field_id); + + auto fields = current_schema->getArray(Iceberg::f_fields); + for (UInt32 i = 0; i < fields->size(); ++i) + { + auto field = fields->getObject(i); + if (field->getValue(Iceberg::f_name) != column_name) + continue; + return field->getValue(Iceberg::f_required) == expected_type.second + && icebergTypesEqual(field->get(Iceberg::f_type), expected_type.first); + } + return false; +} + +bool MetadataGenerator::isDropColumnApplied(const String & column_name) const +{ + auto current_schema = findCurrentSchema(); + if (!current_schema) + return false; + + auto fields = current_schema->getArray(Iceberg::f_fields); + for (UInt32 i = 0; i < fields->size(); ++i) + { + if (fields->getObject(i)->getValue(Iceberg::f_name) == column_name) + return false; + } + return true; +} + +bool MetadataGenerator::isRenameColumnApplied(const String & column_name, const String & new_column_name) const +{ + auto current_schema = findCurrentSchema(); + if (!current_schema) + return false; + + bool found_new_name = false; + auto fields = current_schema->getArray(Iceberg::f_fields); + for (UInt32 i = 0; i < fields->size(); ++i) + { + auto name = fields->getObject(i)->getValue(Iceberg::f_name); + if (name == column_name) + return false; + if (name == new_column_name) + found_new_name = true; + } + return found_new_name; +} + Poco::JSON::Object::Ptr MetadataGenerator::getParentSnapshot(Int64 parent_snapshot_id) { auto snapshots = metadata_object->get(Iceberg::f_snapshots).extract(); @@ -295,23 +373,9 @@ MetadataGenerator::NextMetadataResult MetadataGenerator::generateNextMetadata( void MetadataGenerator::generateDropColumnMetadata(const String & column_name) { - auto current_schema_id = metadata_object->getValue(Iceberg::f_current_schema_id); const auto next_schema_id = getNextSchemaId(metadata_object); - Poco::JSON::Object::Ptr current_schema; - auto schemas = metadata_object->getArray(Iceberg::f_schemas); - for (UInt32 i = 0; i < schemas->size(); ++i) - { - if (schemas->getObject(i)->getValue(Iceberg::f_schema_id) == current_schema_id) - { - current_schema = schemas->getObject(i); - break; - } - } - - if (!current_schema) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Not found schema with id {}", current_schema_id); - current_schema = deepCopy(current_schema); + auto current_schema = deepCopy(getCurrentSchema()); auto fields = current_schema->getArray(Iceberg::f_fields); UInt32 index_to_drop = static_cast(fields->size()); @@ -393,23 +457,9 @@ void MetadataGenerator::generateAddColumnMetadata(const String & column_name, Da { if (!type->isNullable()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Iceberg spec doesn't allow to add non-nullable columns"); - auto current_schema_id = metadata_object->getValue(Iceberg::f_current_schema_id); const auto next_schema_id = getNextSchemaId(metadata_object); - Poco::JSON::Object::Ptr current_schema; - auto schemas = metadata_object->getArray(Iceberg::f_schemas); - for (UInt32 i = 0; i < schemas->size(); ++i) - { - if (schemas->getObject(i)->getValue(Iceberg::f_schema_id) == current_schema_id) - { - current_schema = schemas->getObject(i); - break; - } - } - - if (!current_schema) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Not found schema with id {}", current_schema_id); - current_schema = deepCopy(current_schema); + auto current_schema = deepCopy(getCurrentSchema()); auto existing_fields = current_schema->getArray(Iceberg::f_fields); for (UInt32 i = 0; i < existing_fields->size(); ++i) @@ -437,21 +487,7 @@ void MetadataGenerator::generateAddColumnMetadata(const String & column_name, Da bool MetadataGenerator::generateModifyColumnMetadata(const String & column_name, DataTypePtr type) { - auto current_schema_id = metadata_object->getValue(Iceberg::f_current_schema_id); - - Poco::JSON::Object::Ptr current_schema; - auto schemas = metadata_object->getArray(Iceberg::f_schemas); - for (UInt32 i = 0; i < schemas->size(); ++i) - { - if (schemas->getObject(i)->getValue(Iceberg::f_schema_id) == current_schema_id) - { - current_schema = schemas->getObject(i); - break; - } - } - - if (!current_schema) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Not found schema with id {}", current_schema_id); + auto current_schema = getCurrentSchema(); auto last_column_id = metadata_object->getValue(Iceberg::f_last_column_id); auto new_type = Iceberg::getIcebergType(type, last_column_id); @@ -522,22 +558,7 @@ bool MetadataGenerator::generateModifyColumnMetadata(const String & column_name, void MetadataGenerator::generateRenameColumnMetadata(const String & column_name, const String & new_column_name) { - auto current_schema_id = metadata_object->getValue(Iceberg::f_current_schema_id); - - Poco::JSON::Object::Ptr current_schema; - auto schemas = metadata_object->getArray(Iceberg::f_schemas); - for (UInt32 i = 0; i < schemas->size(); ++i) - { - if (schemas->getObject(i)->getValue(Iceberg::f_schema_id) == current_schema_id) - { - current_schema = schemas->getObject(i); - break; - } - } - - if (!current_schema) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Not found schema with id {}", current_schema_id); - current_schema = deepCopy(current_schema); + auto current_schema = deepCopy(getCurrentSchema()); auto schema_fields = current_schema->getArray(Iceberg::f_fields); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h index 676185c4ae63..8c4cdfaac5a5 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h @@ -47,6 +47,14 @@ class MetadataGenerator bool generateModifyColumnMetadata(const String & column_name, DataTypePtr type); void generateRenameColumnMetadata(const String & column_name, const String & new_column_name); + /// A commit attempt can land in the catalog even when the client observes a failure + /// (the Iceberg "commit state unknown" case, e.g. a proxy returning 5xx after the catalog + /// applied the update). These predicates let a retry detect that the requested change is + /// already present instead of applying it a second time and failing. + bool isAddColumnApplied(const String & column_name, DataTypePtr type) const; + bool isDropColumnApplied(const String & column_name) const; + bool isRenameColumnApplied(const String & column_name, const String & new_column_name) const; + private: Poco::JSON::Object::Ptr metadata_object; @@ -55,6 +63,11 @@ class MetadataGenerator Int64 getMaxSequenceNumber(); Poco::JSON::Object::Ptr getParentSnapshot(Int64 parent_snapshot_id); + + /// Returns the schema referenced by `current-schema-id`, or nullptr when it is absent. + Poco::JSON::Object::Ptr findCurrentSchema() const; + /// Returns the schema referenced by `current-schema-id`, throwing when it is absent. + Poco::JSON::Object::Ptr getCurrentSchema() const; }; #endif diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp index 90b9582f9165..6dc4b9d5e56f 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp @@ -99,6 +99,28 @@ static Int32 getHighestFieldIdFromType(const Poco::Dynamic::Var & type_var) return result; } +/// Whether the schema already reflects `command`. Used after a commit attempt whose outcome is +/// unknown: the catalog may have applied the update and still reported a failure, in which case +/// re-applying the same command on the refreshed metadata would fail with "Column already exists" +/// or "Not found column" for an ALTER that actually succeeded. +static bool alterAlreadyApplied(const MetadataGenerator & generator, const AlterCommand & command) +{ + switch (command.type) + { + case AlterCommand::Type::ADD_COLUMN: + return generator.isAddColumnApplied(command.column_name, command.data_type); + case AlterCommand::Type::DROP_COLUMN: + return generator.isDropColumnApplied(command.column_name); + case AlterCommand::Type::RENAME_COLUMN: + return generator.isRenameColumnApplied(command.column_name, command.rename_to); + case AlterCommand::Type::MODIFY_COLUMN: + /// `generateModifyColumnMetadata` already reports an unchanged schema as a no-op. + return false; + default: + return false; + } +} + /// Return the highest field id across all fields in an Iceberg schema object. static Int32 getHighestFieldId(Poco::JSON::Object::Ptr schema) { @@ -774,6 +796,8 @@ void alter( size_t i = 0; bool succeeded = false; + /// Set once we hand a commit to storage or to the catalog, i.e. once its outcome can be unknown. + bool commit_attempted = false; while (i < MAX_TRANSACTION_RETRIES) { if (auto elem = context->getProcessListElement(); elem && elem->isKilled()) @@ -845,6 +869,17 @@ void alter( auto metadata_json_generator = MetadataGenerator(metadata); + if (commit_attempted && alterAlreadyApplied(metadata_json_generator, params[0])) + { + LOG_WARNING( + log, + "A previous ALTER TABLE commit attempt for {} was reported as failed but is present in the " + "table metadata, treating the operation as succeeded", + storage_id.getNameForLogs()); + succeeded = true; + break; + } + switch (params[0].type) { case AlterCommand::Type::ADD_COLUMN: @@ -895,18 +930,21 @@ void alter( auto hint_path = filename_generator.generateVersionHint(); const bool catalog_writes_metadata_file = catalog && catalog->isTransactional(); - if (!catalog_writes_metadata_file - && !writeMetadataFileAndVersionHint( - persistent_table_components.path_resolver, - metadata_info, - json_representation, - hint_path, - object_storage, - context, - data_lake_settings[DataLakeStorageSetting::iceberg_use_version_hint])) + if (!catalog_writes_metadata_file) { - ++i; - continue; + commit_attempted = true; + if (!writeMetadataFileAndVersionHint( + persistent_table_components.path_resolver, + metadata_info, + json_representation, + hint_path, + object_storage, + context, + data_lake_settings[DataLakeStorageSetting::iceberg_use_version_hint])) + { + ++i; + continue; + } } if (catalog) @@ -916,6 +954,7 @@ void alter( const auto new_last_column_id = std::max( metadata->getValue(Iceberg::f_last_column_id), getHighestFieldId(new_schema)); + commit_attempted = true; if (!catalog->updateSchema(namespace_name, table_name, catalog_filename, new_schema, previous_schema_id, new_last_column_id, metadata)) { ++i; diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp index a8727f8ce248..44dc9a86b234 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp @@ -188,4 +188,57 @@ TEST(IcebergMetadataGenerator, ModifyColumnRejectsIndistinguishableType) EXPECT_THROW(gen.generateModifyColumnMetadata("x", std::make_shared()), DB::Exception); } + +TEST(IcebergMetadataGenerator, AddColumnAppliedDetectsCommittedColumn) +{ + auto metadata = makeMetadataWithGap(); + MetadataGenerator gen(metadata); + + auto type = makeNullable(std::make_shared()); + EXPECT_FALSE(gen.isAddColumnApplied("z", type)); + + /// Emulate the commit that the catalog applied while reporting a failure. + gen.generateAddColumnMetadata("z", type); + EXPECT_TRUE(gen.isAddColumnApplied("z", type)); +} + + +TEST(IcebergMetadataGenerator, AddColumnAppliedRejectsTypeMismatch) +{ + auto metadata = makeMetadataWithGap(); + MetadataGenerator gen(metadata); + + /// `y` exists as an optional Iceberg `string`, so the same name with another type is not the + /// column this ALTER asked for and must still be applied. + EXPECT_TRUE(gen.isAddColumnApplied("y", makeNullable(std::make_shared()))); + EXPECT_FALSE(gen.isAddColumnApplied("y", makeNullable(std::make_shared()))); + EXPECT_FALSE(gen.isAddColumnApplied("y", std::make_shared())); +} + + +TEST(IcebergMetadataGenerator, DropColumnAppliedDetectsCommittedDrop) +{ + auto metadata = makeMetadataWithGap(); + MetadataGenerator gen(metadata); + + EXPECT_FALSE(gen.isDropColumnApplied("y")); + + gen.generateDropColumnMetadata("y"); + EXPECT_TRUE(gen.isDropColumnApplied("y")); +} + + +TEST(IcebergMetadataGenerator, RenameColumnAppliedDetectsCommittedRename) +{ + auto metadata = makeMetadataWithGap(); + MetadataGenerator gen(metadata); + + EXPECT_FALSE(gen.isRenameColumnApplied("y", "w")); + + gen.generateRenameColumnMetadata("y", "w"); + EXPECT_TRUE(gen.isRenameColumnApplied("y", "w")); + /// A rename to a different target name is not what this ALTER asked for. + EXPECT_FALSE(gen.isRenameColumnApplied("y", "v")); +} + #endif diff --git a/tests/integration/test_database_iceberg/test.py b/tests/integration/test_database_iceberg/test.py index 02f3bf3b60ef..97558573a628 100644 --- a/tests/integration/test_database_iceberg/test.py +++ b/tests/integration/test_database_iceberg/test.py @@ -1230,6 +1230,78 @@ def test_writes_schema_evolution_drop_last_column(started_cluster): assert node.query(f"SELECT x, y, w FROM {table_ref} ORDER BY x", settings=write_settings) == "abc\t1\t\\N\ndef\t2\t42\n" +def test_writes_alter_when_commit_is_reported_as_failed(started_cluster): + """An Iceberg commit can land in the catalog while the client observes a failure + (commit state unknown, e.g. a proxy rewriting the response to 5xx). The ALTER retry + must notice that the change is already present instead of applying it a second time + and failing with `Column already exists`. + """ + node = started_cluster.instances["node1"] + + test_ref = f"test_writes_alter_commit_unknown_{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_iceberg_database(started_cluster, node, CATALOG_NAME) + create_clickhouse_iceberg_table(started_cluster, node, root_namespace, table_name, "(x String, y Int32)") + + node.query(f"INSERT INTO {table_ref} VALUES ('abc', 1);", settings=write_settings) + + failpoint = "iceberg_alter_catalog_commit_reported_as_failed" + node.query(f"SYSTEM ENABLE FAILPOINT {failpoint}") + try: + node.query(f"ALTER TABLE {table_ref} ADD COLUMN z Nullable(String);", settings=write_settings) + finally: + node.query(f"SYSTEM DISABLE FAILPOINT {failpoint}") + + description = node.query(f"DESCRIBE TABLE {table_ref}", settings=write_settings) + columns = [line.split("\t")[0] for line in description.strip().split("\n")] + assert columns.count("z") == 1, f"expected exactly one `z` column in:\n{description}" + assert sorted(columns) == sorted(["x", "y", "z"]) + + node.query(f"INSERT INTO {table_ref} VALUES ('def', 2, 'zz');", settings=write_settings) + assert ( + node.query(f"SELECT x, y, z FROM {table_ref} ORDER BY x", settings=write_settings) + == "abc\t1\t\\N\ndef\t2\tzz\n" + ) + + +def test_writes_when_metadata_is_not_initialized(started_cluster): + """When the Iceberg metadata cannot be created, the write entrypoints must report + `NOT_INITIALIZED` instead of dereferencing the null metadata pointer. + """ + node = started_cluster.instances["node1"] + + test_ref = f"test_writes_uninitialized_metadata_{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_iceberg_database(started_cluster, node, CATALOG_NAME) + create_clickhouse_iceberg_table(started_cluster, node, root_namespace, table_name, "(x String, y Int32)") + + node.query(f"INSERT INTO {table_ref} VALUES ('abc', 1);", settings=write_settings) + + failpoint = "datalake_iceberg_metadata_create_fail" + node.query(f"SYSTEM ENABLE FAILPOINT {failpoint}") + try: + for query in [ + f"ALTER TABLE {table_ref} ADD COLUMN z Nullable(String)", + f"ALTER TABLE {table_ref} DELETE WHERE y = 1", + f"INSERT INTO {table_ref} VALUES ('def', 2)", + ]: + error = node.query_and_get_error(query, settings=write_settings) + assert "NOT_INITIALIZED" in error, f"unexpected error for `{query}`:\n{error}" + finally: + node.query(f"SYSTEM DISABLE FAILPOINT {failpoint}") + + # The server must still be alive and the table unchanged. + assert node.query(f"SELECT x, y FROM {table_ref} ORDER BY ALL", settings=write_settings) == "abc\t1\n" + + def test_writes_schema_evolution_concurrent_add_columns(started_cluster): node = started_cluster.instances["node1"] From 846e827462273ce7bf7d6eb77099a59591fd4a51 Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Thu, 13 Aug 2026 18:58:24 +0200 Subject: [PATCH 15/18] Fix wrong doc comment --- src/Databases/DataLake/RestCatalog.h | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/Databases/DataLake/RestCatalog.h b/src/Databases/DataLake/RestCatalog.h index 4f7d64e5c54b..4d9562f05af4 100644 --- a/src/Databases/DataLake/RestCatalog.h +++ b/src/Databases/DataLake/RestCatalog.h @@ -245,13 +245,10 @@ class BigLakeCatalog : public RestCatalog AccessToken retrieveGoogleCloudAccessTokenFromRefreshToken() const; }; -/// Builds the JSON body for `POST .../namespaces/{ns}/tables/{table}` (Iceberg REST update). -/// -/// Returns `nullptr` when `new_snapshot` is null (nothing to commit). Throws -/// `DB::Exception(DATALAKE_DATABASE_ERROR)` with a specific message when the metadata -/// blob is malformed (e.g. missing `current-schema-id`, no schema object matching it). /// Builds the JSON body for a schema-update commit via the Iceberg REST catalog. -/// Includes schema deduplication, sort-order incompatibility reset, and last-column-id. +/// Includes an assert-current-schema-id requirement (when previous_schema_id >= 0), +/// schema deduplication against existing schemas in metadata, and last-column-id +/// propagation when adding a new schema. Poco::JSON::Object::Ptr buildUpdateSchemaRequestBody( const String & namespace_name, const String & table_name, From 59681636438b4326aafb6cf96b90ba838741b7df Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Thu, 13 Aug 2026 20:48:33 +0200 Subject: [PATCH 16/18] Remove dead code, fix/remove failpoint --- src/Common/FailPoint.cpp | 4 +-- src/Databases/DataLake/RestCatalog.cpp | 6 ++-- .../DataLakes/DataLakeConfiguration.h | 10 +----- .../DataLakes/Iceberg/MetadataGenerator.cpp | 5 +-- .../DataLakes/Iceberg/MetadataGenerator.h | 3 +- .../DataLakes/Iceberg/Mutations.cpp | 7 +++- .../integration/test_database_iceberg/test.py | 33 ------------------- .../test_writes_modify_column.py | 10 ++---- 8 files changed, 17 insertions(+), 61 deletions(-) diff --git a/src/Common/FailPoint.cpp b/src/Common/FailPoint.cpp index c0023b47f43d..92e0ae1bffd9 100644 --- a/src/Common/FailPoint.cpp +++ b/src/Common/FailPoint.cpp @@ -163,10 +163,8 @@ static struct InitFiu ONCE(write_file_operation_fail_on_read) \ REGULAR(slowdown_parallel_replicas_local_plan_read) \ ONCE(iceberg_writes_cleanup) \ - ONCE(iceberg_alter_catalog_update_metadata_fail) \ + ONCE(iceberg_alter_catalog_update_schema_fail) \ ONCE(iceberg_alter_catalog_commit_reported_as_failed) \ - REGULAR(iceberg_alter_orphan_metadata_cleanup_fail) \ - REGULAR(datalake_iceberg_metadata_create_fail) \ REGULAR(storage_cluster_read_sleep) \ ONCE(backup_add_empty_memory_table) \ PAUSEABLE_ONCE(backup_pause_on_start) \ diff --git a/src/Databases/DataLake/RestCatalog.cpp b/src/Databases/DataLake/RestCatalog.cpp index d75993e74b0d..3be765f3ee64 100644 --- a/src/Databases/DataLake/RestCatalog.cpp +++ b/src/Databases/DataLake/RestCatalog.cpp @@ -70,7 +70,7 @@ namespace DB::Setting namespace DB::FailPoints { extern const char check_database_datalake_negative[]; - extern const char iceberg_alter_catalog_update_metadata_fail[]; + extern const char iceberg_alter_catalog_update_schema_fail[]; extern const char iceberg_alter_catalog_commit_reported_as_failed[]; } @@ -1466,8 +1466,6 @@ bool RestCatalog::updateMetadata(const String & namespace_name, const String & t const std::string endpoint = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables" / table_name).generic_string(); auto request_body = buildUpdateMetadataRequestBody(namespace_name, table_name, new_snapshot); - if (!request_body) - return true; try { @@ -1496,7 +1494,7 @@ bool RestCatalog::updateSchema( Int32 new_last_column_id, Poco::JSON::Object::Ptr metadata) const { - fiu_do_on(DB::FailPoints::iceberg_alter_catalog_update_metadata_fail, { return false; }); + fiu_do_on(DB::FailPoints::iceberg_alter_catalog_update_schema_fail, { return false; }); const std::string endpoint = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables" / table_name).generic_string(); diff --git a/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h b/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h index 55a576ddd239..073194351c9e 100644 --- a/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h +++ b/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h @@ -27,7 +27,6 @@ #include #include -#include #include #include #include @@ -49,15 +48,9 @@ namespace ErrorCodes { extern const int BAD_ARGUMENTS; extern const int LOGICAL_ERROR; - extern const int NOT_INITIALIZED; extern const int PATH_ACCESS_DENIED; } -namespace FailPoints -{ - extern const char datalake_iceberg_metadata_create_fail[]; -} - namespace DataLakeStorageSetting { extern DataLakeStorageSettingsDatabaseDataLakeCatalogType storage_catalog_type; @@ -129,7 +122,6 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl { if (current_metadata != nullptr) return; - fiu_do_on(FailPoints::datalake_iceberg_metadata_create_fail, { return; }); BaseStorageConfiguration::update(object_storage, local_context); assertLocalPathCorrect(object_storage, local_context); current_metadata = DataLakeMetadata::create(object_storage, weak_from_this(), local_context); @@ -438,7 +430,7 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl void assertInitialized() const { if (!current_metadata) - throw Exception(ErrorCodes::NOT_INITIALIZED, "Metadata is not initialized"); + throw Exception(ErrorCodes::LOGICAL_ERROR, "Metadata is not initialized"); } ReadFromFormatInfo prepareReadingFromFormat( diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp index 65e6d34a7586..f8e5fff7dbf8 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp @@ -128,8 +128,9 @@ Int32 getNextSchemaId(Poco::JSON::Object::Ptr metadata_object) } -MetadataGenerator::MetadataGenerator(Poco::JSON::Object::Ptr metadata_object_) +MetadataGenerator::MetadataGenerator(Poco::JSON::Object::Ptr metadata_object_, bool allow_geo_parser_) : metadata_object(metadata_object_) + , allow_geo_parser(allow_geo_parser_) , gen(randomSeed()) , dis(1, std::numeric_limits::max()) { @@ -510,7 +511,7 @@ bool MetadataGenerator::generateModifyColumnMetadata(const String & column_name, if (existing_iceberg_type.isString()) { auto reconstructed_ch_type = Iceberg::IcebergSchemaProcessor::getSimpleType( - existing_iceberg_type.extract(), /* allow_geo_parser */ false); + existing_iceberg_type.extract(), allow_geo_parser); if (!current_field->getValue(Iceberg::f_required) && reconstructed_ch_type->canBeInsideNullable()) reconstructed_ch_type = makeNullable(reconstructed_ch_type); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h index 8c4cdfaac5a5..153bc4bee9a7 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h @@ -17,7 +17,7 @@ namespace DB class MetadataGenerator { public: - explicit MetadataGenerator(Poco::JSON::Object::Ptr metadata_object_); + explicit MetadataGenerator(Poco::JSON::Object::Ptr metadata_object_, bool allow_geo_parser_ = false); struct NextMetadataResult { @@ -57,6 +57,7 @@ class MetadataGenerator private: Poco::JSON::Object::Ptr metadata_object; + bool allow_geo_parser; pcg64_fast gen; std::uniform_int_distribution dis; diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp index 6dc4b9d5e56f..cd3df642c74b 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp @@ -51,6 +51,11 @@ extern const DataLakeStorageSettingsBool iceberg_use_version_hint; extern const DataLakeStorageSettingsString iceberg_metadata_file_path; } +namespace DB::Setting +{ +extern const SettingsBool allow_experimental_geo_types_in_iceberg; +} + namespace DB::FailPoints { extern const char iceberg_writes_cleanup[]; @@ -867,7 +872,7 @@ void alter( const auto previous_schema_id = metadata->getValue(Iceberg::f_current_schema_id); - auto metadata_json_generator = MetadataGenerator(metadata); + auto metadata_json_generator = MetadataGenerator(metadata, context->getSettingsRef()[Setting::allow_experimental_geo_types_in_iceberg]); if (commit_attempted && alterAlreadyApplied(metadata_json_generator, params[0])) { diff --git a/tests/integration/test_database_iceberg/test.py b/tests/integration/test_database_iceberg/test.py index 97558573a628..c2a8fff1496f 100644 --- a/tests/integration/test_database_iceberg/test.py +++ b/tests/integration/test_database_iceberg/test.py @@ -1268,39 +1268,6 @@ def test_writes_alter_when_commit_is_reported_as_failed(started_cluster): ) -def test_writes_when_metadata_is_not_initialized(started_cluster): - """When the Iceberg metadata cannot be created, the write entrypoints must report - `NOT_INITIALIZED` instead of dereferencing the null metadata pointer. - """ - node = started_cluster.instances["node1"] - - test_ref = f"test_writes_uninitialized_metadata_{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_iceberg_database(started_cluster, node, CATALOG_NAME) - create_clickhouse_iceberg_table(started_cluster, node, root_namespace, table_name, "(x String, y Int32)") - - node.query(f"INSERT INTO {table_ref} VALUES ('abc', 1);", settings=write_settings) - - failpoint = "datalake_iceberg_metadata_create_fail" - node.query(f"SYSTEM ENABLE FAILPOINT {failpoint}") - try: - for query in [ - f"ALTER TABLE {table_ref} ADD COLUMN z Nullable(String)", - f"ALTER TABLE {table_ref} DELETE WHERE y = 1", - f"INSERT INTO {table_ref} VALUES ('def', 2)", - ]: - error = node.query_and_get_error(query, settings=write_settings) - assert "NOT_INITIALIZED" in error, f"unexpected error for `{query}`:\n{error}" - finally: - node.query(f"SYSTEM DISABLE FAILPOINT {failpoint}") - - # The server must still be alive and the table unchanged. - assert node.query(f"SELECT x, y FROM {table_ref} ORDER BY ALL", settings=write_settings) == "abc\t1\n" - def test_writes_schema_evolution_concurrent_add_columns(started_cluster): node = started_cluster.instances["node1"] diff --git a/tests/integration/test_storage_iceberg_no_spark/test_writes_modify_column.py b/tests/integration/test_storage_iceberg_no_spark/test_writes_modify_column.py index 2c392a648817..ac03af1bd624 100644 --- a/tests/integration/test_storage_iceberg_no_spark/test_writes_modify_column.py +++ b/tests/integration/test_storage_iceberg_no_spark/test_writes_modify_column.py @@ -56,14 +56,8 @@ def test_modify_column_errors(started_cluster_iceberg_no_spark, format_version, settings=INSERT_SETTINGS, ) el = error.lower() - # String→integer: mismatched Poco::Var kinds in checkValidSchemaEvolution → BadCastException - assert ( - "bad cast" in el - or "can not convert" in el - or "cannot convert" in el - or "schema evolution" in el - or "doesn't allow" in el - ) + # String→Int64 is not a valid Iceberg schema evolution; must get BAD_ARGUMENTS. + assert "doesn't allow schema evolution" in el assert instance.query( f"SELECT name FROM system.columns WHERE database = currentDatabase() AND table = '{TABLE_NAME}' ORDER BY name" From 7dc6cfb37f77fabae7a2a430ced00776e4deff2f Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Thu, 13 Aug 2026 21:30:57 +0200 Subject: [PATCH 17/18] Address medium defects --- src/Databases/DataLake/RestCatalog.cpp | 20 +++-- .../gtest_rest_catalog_update_metadata.cpp | 78 +++++++++++++++++++ .../DataLakes/Iceberg/MetadataGenerator.cpp | 63 ++++++++++++++- .../gtest_iceberg_metadata_generator.cpp | 41 ++++++++++ 4 files changed, 196 insertions(+), 6 deletions(-) diff --git a/src/Databases/DataLake/RestCatalog.cpp b/src/Databases/DataLake/RestCatalog.cpp index 3be765f3ee64..231b60d1d3b3 100644 --- a/src/Databases/DataLake/RestCatalog.cpp +++ b/src/Databases/DataLake/RestCatalog.cpp @@ -158,6 +158,8 @@ std::unordered_set getAllowedBigLakeMetadataServiceHosts( namespace { +constexpr auto IDENTIFIER_FIELD_IDS = "identifier-field-ids"; + Poco::JSON::Object::Ptr cloneJsonObject(const Poco::JSON::Object::Ptr & obj) { std::ostringstream oss; // STYLE_CHECK_ALLOW_STD_STRING_STREAM @@ -219,12 +221,20 @@ bool icebergJsonValueEquals(const Poco::Dynamic::Var & lhs, const Poco::Dynamic: } /// Two Iceberg schemas are equivalent when they differ only by their `schema-id`. +/// `identifier-field-ids` is ignored as well: a schema committed through this catalog always +/// carries it, while a freshly generated one does not, and an absent list means the same as an +/// empty one. bool schemasEquivalentIgnoringId(const Poco::JSON::Object::Ptr & lhs, const Poco::JSON::Object::Ptr & rhs) { Poco::JSON::Object::Ptr lhs_copy = cloneJsonObject(lhs); Poco::JSON::Object::Ptr rhs_copy = cloneJsonObject(rhs); - lhs_copy->remove(DB::Iceberg::f_schema_id); - rhs_copy->remove(DB::Iceberg::f_schema_id); + for (auto * copy : {&lhs_copy, &rhs_copy}) + { + (*copy)->remove(DB::Iceberg::f_schema_id); + if (auto identifier_field_ids = (*copy)->getArray(IDENTIFIER_FIELD_IDS); + identifier_field_ids.isNull() || identifier_field_ids->size() == 0) + (*copy)->remove(IDENTIFIER_FIELD_IDS); + } return icebergJsonObjectEquals(lhs_copy, rhs_copy); } @@ -260,10 +270,10 @@ Poco::JSON::Object::Ptr buildUpdateSchemaRequestBody( } Poco::JSON::Object::Ptr schema_for_rest = cloneJsonObject(new_schema); - if (!schema_for_rest->has("identifier-field-ids")) + if (!schema_for_rest->has(IDENTIFIER_FIELD_IDS)) { Poco::JSON::Array::Ptr empty_identifier_field_ids = new Poco::JSON::Array; - schema_for_rest->set("identifier-field-ids", empty_identifier_field_ids); + schema_for_rest->set(IDENTIFIER_FIELD_IDS, empty_identifier_field_ids); } std::optional existing_equivalent_schema_id; @@ -1424,7 +1434,7 @@ void RestCatalog::createTable(const String & namespace_name, const String & tabl { Poco::JSON::Object::Ptr initial_schema = metadata_content->getArray("schemas")->getObject(0); Poco::JSON::Array::Ptr identifier_fields = new Poco::JSON::Array; - initial_schema->set("identifier-field-ids", identifier_fields); + initial_schema->set(IDENTIFIER_FIELD_IDS, identifier_fields); request_body->set("schema", initial_schema); } request_body->set("partition-spec", metadata_content->getArray("partition-specs")->get(0)); diff --git a/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp b/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp index 06dbc4591da1..2f6eebf8fdab 100644 --- a/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp +++ b/src/Databases/DataLake/tests/gtest_rest_catalog_update_metadata.cpp @@ -10,6 +10,9 @@ #include #include +#include +#include + using namespace DB; namespace @@ -24,6 +27,45 @@ Poco::JSON::Object::Ptr findUpdateByAction(const Poco::JSON::Array::Ptr & update } return nullptr; } + +Poco::JSON::Object::Ptr makeIntField(Int32 id, const std::string & name) +{ + Poco::JSON::Object::Ptr field = new Poco::JSON::Object; + field->set(Iceberg::f_id, id); + field->set(Iceberg::f_name, name); + field->set(Iceberg::f_required, false); + field->set(Iceberg::f_type, "int"); + return field; +} + +/// Builds a single-column (`a`) struct schema, optionally carrying `identifier-field-ids`. +Poco::JSON::Object::Ptr makeSingleColumnSchema(Int32 schema_id, std::optional> identifier_field_ids) +{ + Poco::JSON::Object::Ptr schema = new Poco::JSON::Object; + schema->set(Iceberg::f_schema_id, schema_id); + schema->set(Iceberg::f_type, "struct"); + Poco::JSON::Array::Ptr fields = new Poco::JSON::Array; + fields->add(makeIntField(1, "a")); + schema->set(Iceberg::f_fields, fields); + if (identifier_field_ids.has_value()) + { + Poco::JSON::Array::Ptr ids = new Poco::JSON::Array; + for (auto id : *identifier_field_ids) + ids->add(id); + schema->set("identifier-field-ids", ids); + } + return schema; +} + +Poco::JSON::Object::Ptr makeMetadataWithSchemas(const std::vector & schema_list) +{ + Poco::JSON::Object::Ptr metadata = new Poco::JSON::Object; + Poco::JSON::Array::Ptr schemas = new Poco::JSON::Array; + for (const auto & schema : schema_list) + schemas->add(schema); + metadata->set(Iceberg::f_schemas, schemas); + return metadata; +} } TEST(RestCatalogUpdateMetadataBody, NullSnapshotReturnsNull) @@ -144,6 +186,42 @@ TEST(RestCatalogUpdateSchemaBody, EquivalentSchemaDeduplicates) EXPECT_EQ(set_schema->getValue("schema-id"), 0); } +TEST(RestCatalogUpdateSchemaBody, EquivalentSchemaDeduplicatesAcrossIdentifierFieldIds) +{ + /// A schema already committed through this catalog carries an empty `identifier-field-ids`, + /// while a freshly generated one does not. That difference must not prevent deduplication. + auto metadata = makeMetadataWithSchemas({makeSingleColumnSchema(0, std::vector{})}); + auto new_schema = makeSingleColumnSchema(1, std::nullopt); + + auto body = DataLake::buildUpdateSchemaRequestBody("ns", "t", metadata, new_schema, 0, 1); + ASSERT_TRUE(body); + + auto updates = body->getArray("updates"); + EXPECT_FALSE(findUpdateByAction(updates, "add-schema")); + + auto set_schema = findUpdateByAction(updates, "set-current-schema"); + ASSERT_TRUE(set_schema); + EXPECT_EQ(set_schema->getValue("schema-id"), 0); +} + +TEST(RestCatalogUpdateSchemaBody, NonEmptyIdentifierFieldIdsPreventDeduplication) +{ + /// Identifier fields define row identity, so a schema that declares them is not equivalent + /// to one that does not. + auto metadata = makeMetadataWithSchemas({makeSingleColumnSchema(0, std::vector{1})}); + auto new_schema = makeSingleColumnSchema(1, std::nullopt); + + auto body = DataLake::buildUpdateSchemaRequestBody("ns", "t", metadata, new_schema, 0, 1); + ASSERT_TRUE(body); + + auto updates = body->getArray("updates"); + ASSERT_TRUE(findUpdateByAction(updates, "add-schema")); + + auto set_schema = findUpdateByAction(updates, "set-current-schema"); + ASSERT_TRUE(set_schema); + EXPECT_EQ(set_schema->getValue("schema-id"), -1); +} + TEST(RestCatalogUpdateSchemaBody, NormalPathEmitsAddSchema) { Poco::JSON::Object::Ptr metadata = new Poco::JSON::Object; diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp index f8e5fff7dbf8..1d1244b2fa12 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp @@ -115,6 +115,65 @@ bool icebergTypesEqual(Poco::Dynamic::Var old_type, Poco::Dynamic::Var new_type) return false; } +/// Recursively drop the field ids Iceberg assigns to nested elements of a complex type. +/// `getIcebergType` allocates them from a running counter, so regenerating the same +/// ClickHouse type with a different counter start yields a different - but structurally +/// identical - descriptor. Removing the ids makes such descriptors comparable. +void stripNestedFieldIds(Poco::JSON::Object::Ptr type_object) +{ + for (const auto & id_field : {Iceberg::f_id, Iceberg::f_element_id, Iceberg::f_key_id, Iceberg::f_value_id}) + type_object->remove(id_field); + + for (const auto & nested_field : {Iceberg::f_element, Iceberg::f_key, Iceberg::f_value, Iceberg::f_type}) + { + if (!type_object->has(nested_field)) + continue; + auto nested = type_object->get(nested_field); + if (nested.isString()) + continue; + if (auto nested_object = nested.extract()) + stripNestedFieldIds(nested_object); + } + + if (type_object->has(Iceberg::f_fields)) + { + auto fields = type_object->getArray(Iceberg::f_fields); + for (UInt32 i = 0; i < fields->size(); ++i) + { + if (auto field = fields->getObject(i)) + stripNestedFieldIds(field); + } + } +} + +/// Like `icebergTypesEqual`, but ignores the field ids embedded in complex types. +/// Used to recognize a type that a previous attempt already wrote, where the ids +/// were allocated from a lower `last-column-id` than the one we would use now. +bool icebergTypesEqualIgnoringIds(Poco::Dynamic::Var old_type, Poco::Dynamic::Var new_type) +{ + if (old_type.isString() && new_type.isString()) + return old_type.extract() == new_type.extract(); + + if (old_type.isString() || new_type.isString()) + return false; + + auto old_object = old_type.extract(); + auto new_object = new_type.extract(); + if (!old_object || !new_object) + return false; + + auto old_stripped = deepCopy(old_object); + auto new_stripped = deepCopy(new_object); + stripNestedFieldIds(old_stripped); + stripNestedFieldIds(new_stripped); + + std::ostringstream oss_old; // STYLE_CHECK_ALLOW_STD_STRING_STREAM + std::ostringstream oss_new; // STYLE_CHECK_ALLOW_STD_STRING_STREAM + old_stripped->stringify(oss_old); + new_stripped->stringify(oss_new); + return oss_old.str() == oss_new.str(); +} + /// Allocate the next schema id as max(existing schema ids) + 1 to avoid /// collisions when current-schema-id is not the highest in the list. Int32 getNextSchemaId(Poco::JSON::Object::Ptr metadata_object) @@ -193,8 +252,10 @@ bool MetadataGenerator::isAddColumnApplied(const String & column_name, DataTypeP auto field = fields->getObject(i); if (field->getValue(Iceberg::f_name) != column_name) continue; + /// The stored descriptor was produced from a lower `last-column-id` than the one we + /// just used, so the ids of nested elements differ even for the very same type. return field->getValue(Iceberg::f_required) == expected_type.second - && icebergTypesEqual(field->get(Iceberg::f_type), expected_type.first); + && icebergTypesEqualIgnoringIds(field->get(Iceberg::f_type), expected_type.first); } return false; } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp index 44dc9a86b234..4ad6dd7b1b8e 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -203,6 +204,46 @@ TEST(IcebergMetadataGenerator, AddColumnAppliedDetectsCommittedColumn) } +TEST(IcebergMetadataGenerator, AddColumnAppliedDetectsCommittedComplexColumn) +{ + auto metadata = makeMetadataWithGap(); + MetadataGenerator gen(metadata); + + /// `getIcebergType` numbers the nested fields of a complex type from `last-column-id`, which + /// the applied commit has already advanced. The detection must look past those ids. + auto type = makeNullable(std::make_shared( + DataTypes{std::make_shared(), std::make_shared()}, + Names{"a", "b"})); + EXPECT_FALSE(gen.isAddColumnApplied("t", type)); + + gen.generateAddColumnMetadata("t", type); + EXPECT_TRUE(gen.isAddColumnApplied("t", type)); +} + + +TEST(IcebergMetadataGenerator, AddColumnAppliedRejectsComplexTypeMismatch) +{ + auto metadata = makeMetadataWithGap(); + MetadataGenerator gen(metadata); + + auto type = makeNullable(std::make_shared( + DataTypes{std::make_shared(), std::make_shared()}, + Names{"a", "b"})); + gen.generateAddColumnMetadata("t", type); + + /// Ignoring the nested ids must not make structurally different types compare equal. + auto renamed_element = makeNullable(std::make_shared( + DataTypes{std::make_shared(), std::make_shared()}, + Names{"a", "c"})); + EXPECT_FALSE(gen.isAddColumnApplied("t", renamed_element)); + + auto retyped_element = makeNullable(std::make_shared( + DataTypes{std::make_shared(), std::make_shared()}, + Names{"a", "b"})); + EXPECT_FALSE(gen.isAddColumnApplied("t", retyped_element)); +} + + TEST(IcebergMetadataGenerator, AddColumnAppliedRejectsTypeMismatch) { auto metadata = makeMetadataWithGap(); From 62ba6a0ff4b95c258b55e32233b303e1a86b334c Mon Sep 17 00:00:00 2001 From: Kanthi Subramanian Date: Fri, 14 Aug 2026 18:47:41 +0200 Subject: [PATCH 18/18] Replace AssertInitialized with AssertInitializedDL --- .../DataLakes/DataLakeConfiguration.h | 12 ++++++------ .../DataLakes/Iceberg/MetadataGenerator.cpp | 16 ++++++++++++---- .../DataLakes/Iceberg/MetadataGenerator.h | 8 +++++--- .../DataLakes/Iceberg/Mutations.cpp | 9 ++------- .../tests/gtest_iceberg_metadata_generator.cpp | 5 +++-- 5 files changed, 28 insertions(+), 22 deletions(-) diff --git a/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h b/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h index 253412aefdde..92c8bf4cf60a 100644 --- a/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h +++ b/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h @@ -184,14 +184,14 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl void checkMutationIsPossible(ObjectStoragePtr object_storage, ContextPtr context, const MutationCommands & commands) override { lazyInitializeIfNeeded(object_storage, context); - assertInitialized(); + assertInitializedDL(); current_metadata->checkMutationIsPossible(commands); } void checkAlterIsPossible(ObjectStoragePtr object_storage, ContextPtr context, const AlterCommands & commands) override { lazyInitializeIfNeeded(object_storage, context); - assertInitialized(); + assertInitializedDL(); current_metadata->checkAlterIsPossible(commands); } @@ -203,7 +203,7 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl std::shared_ptr catalog) override { lazyInitializeIfNeeded(object_storage, context); - assertInitialized(); + assertInitializedDL(); current_metadata->alter(params, context, storage_id, catalog); } @@ -358,7 +358,7 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl std::shared_ptr catalog) override { lazyInitializeIfNeeded(object_storage, context); - assertInitialized(); + assertInitializedDL(); return current_metadata->write( sample_block, table_id, @@ -394,13 +394,13 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl bool optimize(ObjectStoragePtr object_storage, const StorageMetadataPtr & metadata_snapshot, ContextPtr context, const std::optional & format_settings) override { lazyInitializeIfNeeded(object_storage, context); - assertInitialized(); + assertInitializedDL(); return current_metadata->optimize(metadata_snapshot, context, format_settings); } void addDeleteTransformers(ObjectInfoPtr object_info, QueryPipelineBuilder & builder, const std::optional & format_settings, FormatParserSharedResourcesPtr parser_shared_resources, ContextPtr local_context) const override { - assertInitialized(); + assertInitializedDL(); current_metadata->addDeleteTransformers(object_info, builder, format_settings, parser_shared_resources, local_context); } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp index aa3ca0686e22..e802d15f4b51 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp @@ -1,6 +1,8 @@ +#include #include #include #include +#include #include #include @@ -23,6 +25,11 @@ namespace DB::ErrorCodes extern const int BAD_ARGUMENTS; } +namespace DB::Setting +{ +extern const SettingsBool allow_experimental_geo_types_in_iceberg; +} + namespace DB { @@ -187,9 +194,8 @@ Int32 getNextSchemaId(Poco::JSON::Object::Ptr metadata_object) } -MetadataGenerator::MetadataGenerator(Poco::JSON::Object::Ptr metadata_object_, bool allow_geo_parser_) +MetadataGenerator::MetadataGenerator(Poco::JSON::Object::Ptr metadata_object_) : metadata_object(metadata_object_) - , allow_geo_parser(allow_geo_parser_) , gen(randomSeed()) , dis(1, std::numeric_limits::max()) { @@ -562,7 +568,7 @@ void MetadataGenerator::generateAddColumnMetadata(const String & column_name, Da metadata_object->getArray(Iceberg::f_schemas)->add(current_schema); } -bool MetadataGenerator::generateModifyColumnMetadata(const String & column_name, DataTypePtr type) +bool MetadataGenerator::generateModifyColumnMetadata(const String & column_name, DataTypePtr type, ContextPtr context) { auto current_schema = getCurrentSchema(); @@ -587,7 +593,9 @@ bool MetadataGenerator::generateModifyColumnMetadata(const String & column_name, if (existing_iceberg_type.isString()) { auto reconstructed_ch_type = Iceberg::IcebergSchemaProcessor::getSimpleType( - existing_iceberg_type.extract(), allow_geo_parser); + existing_iceberg_type.extract(), + context, + context->getSettingsRef()[Setting::allow_experimental_geo_types_in_iceberg]); if (!current_field->getValue(Iceberg::f_required) && reconstructed_ch_type->canBeInsideNullable()) reconstructed_ch_type = makeNullable(reconstructed_ch_type); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h index 3559badd170b..97b095e8735f 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h @@ -4,6 +4,7 @@ #include "config.h" #include +#include #include #include #include @@ -17,7 +18,7 @@ namespace DB class MetadataGenerator { public: - explicit MetadataGenerator(Poco::JSON::Object::Ptr metadata_object_, bool allow_geo_parser_ = false); + explicit MetadataGenerator(Poco::JSON::Object::Ptr metadata_object_); struct NextMetadataResult { @@ -45,7 +46,9 @@ class MetadataGenerator void generateAddColumnMetadata(const String & column_name, DataTypePtr type); void generateDropColumnMetadata(const String & column_name); /// Returns false when the column already has the requested type (no metadata change). - bool generateModifyColumnMetadata(const String & column_name, DataTypePtr type); + /// `context` supplies the settings used to map the stored Iceberg type back to a ClickHouse + /// type (the timestamptz timezone and whether geo types are allowed). + bool generateModifyColumnMetadata(const String & column_name, DataTypePtr type, ContextPtr context); void generateRenameColumnMetadata(const String & column_name, const String & new_column_name); /// A commit attempt can land in the catalog even when the client observes a failure @@ -58,7 +61,6 @@ class MetadataGenerator private: Poco::JSON::Object::Ptr metadata_object; - bool allow_geo_parser; pcg64_fast gen; std::uniform_int_distribution dis; diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp index 33bb729a1f6c..482684e1315e 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp @@ -51,11 +51,6 @@ extern const DataLakeStorageSettingsBool iceberg_use_version_hint; extern const DataLakeStorageSettingsString iceberg_metadata_file_path; } -namespace DB::Setting -{ -extern const SettingsBool allow_experimental_geo_types_in_iceberg; -} - namespace DB::FailPoints { extern const char iceberg_writes_cleanup[]; @@ -872,7 +867,7 @@ void alter( const auto previous_schema_id = metadata->getValue(Iceberg::f_current_schema_id); - auto metadata_json_generator = MetadataGenerator(metadata, context->getSettingsRef()[Setting::allow_experimental_geo_types_in_iceberg]); + auto metadata_json_generator = MetadataGenerator(metadata); if (commit_attempted && alterAlreadyApplied(metadata_json_generator, params[0])) { @@ -897,7 +892,7 @@ void alter( break; case AlterCommand::Type::MODIFY_COLUMN: { - if (!metadata_json_generator.generateModifyColumnMetadata(params[0].column_name, params[0].data_type)) + if (!metadata_json_generator.generateModifyColumnMetadata(params[0].column_name, params[0].data_type, context)) { succeeded = true; } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp index 4ad6dd7b1b8e..137e5e9e382d 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/tests/gtest_iceberg_metadata_generator.cpp @@ -4,6 +4,7 @@ #include +#include #include #include #include @@ -176,7 +177,7 @@ TEST(IcebergMetadataGenerator, ModifyColumnNoopSameType) auto metadata = makeMinimalMetadata(0, 1); MetadataGenerator gen(metadata); - bool changed = gen.generateModifyColumnMetadata("x", std::make_shared()); + bool changed = gen.generateModifyColumnMetadata("x", std::make_shared(), getContext().context); EXPECT_FALSE(changed); } @@ -186,7 +187,7 @@ TEST(IcebergMetadataGenerator, ModifyColumnRejectsIndistinguishableType) auto metadata = makeMinimalMetadata(0, 1); MetadataGenerator gen(metadata); - EXPECT_THROW(gen.generateModifyColumnMetadata("x", std::make_shared()), DB::Exception); + EXPECT_THROW(gen.generateModifyColumnMetadata("x", std::make_shared(), getContext().context), DB::Exception); }