diff --git a/docs/en/sql-reference/table-functions/iceberg.md b/docs/en/sql-reference/table-functions/iceberg.md index 9fd78c5e17d7..3a159cfb7b81 100644 --- a/docs/en/sql-reference/table-functions/iceberg.md +++ b/docs/en/sql-reference/table-functions/iceberg.md @@ -420,7 +420,7 @@ y: 993 ### Schema evolution {#iceberg-writes-schema-evolution} -ClickHouse allows you to add, drop, or modify columns with simple types (non-tuple, non-array, non-map). +ClickHouse allows you to add, drop, modify, or rename columns with simple types (non-tuple, non-array, non-map). ### Example {#example-iceberg-writes-evolution} @@ -479,6 +479,27 @@ Row 1: ────── x: Ivanov y: 993 + +ALTER TABLE iceberg_writes_example RENAME COLUMN y TO value; +SHOW CREATE TABLE iceberg_writes_example; + + ┌─statement─────────────────────────────────────────────────┐ +1. │ CREATE TABLE default.iceberg_writes_example ↴│ + │↳( ↴│ + │↳ `x` Nullable(String), ↴│ + │↳ `value` Nullable(Int64) ↴│ + │↳) ↴│ + │↳ENGINE = IcebergLocal('/home/scanhex12/iceberg_example/') │ + └───────────────────────────────────────────────────────────┘ + +SELECT * +FROM iceberg_writes_example +FORMAT VERTICAL; + +Row 1: +────── +x: Ivanov +value: 993 ``` ### Compaction {#iceberg-writes-compaction} diff --git a/src/Databases/DataLake/RestCatalog.cpp b/src/Databases/DataLake/RestCatalog.cpp index fb7031e3d052..5d9bc5f4955a 100644 --- a/src/Databases/DataLake/RestCatalog.cpp +++ b/src/Databases/DataLake/RestCatalog.cpp @@ -29,6 +29,7 @@ #include #include +#include #include #include #include @@ -42,6 +43,8 @@ #include #include +#include + namespace DB::ErrorCodes { @@ -114,6 +117,15 @@ String encodeNamespaceForURI(const String & namespace_name) return encoded; } +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(); +} + } std::string RestCatalog::Config::toString() const @@ -1305,43 +1317,105 @@ bool RestCatalog::updateMetadata(const String & namespace_name, const String & t request_body->set("identifier", identifier); } - if (new_snapshot->has("parent-snapshot-id")) + if (!new_snapshot) + return true; + + if (new_snapshot->has(DB::Iceberg::f_schemas)) { - auto parent_snapshot_id = new_snapshot->getValue("parent-snapshot-id"); - if (parent_snapshot_id != -1) + if (!new_snapshot->has(DB::Iceberg::f_current_schema_id)) + return false; + + 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) + return false; + + Poco::JSON::Object::Ptr schema_for_rest = cloneJsonObject(new_schema_obj); + { + Poco::JSON::Array::Ptr identifier_fields = new Poco::JSON::Array; + schema_for_rest->set("identifier-field-ids", identifier_fields); + } + + if (old_schema_id >= 0) { 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); + 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); } - } - { 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 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", new_schema_id); + updates->add(set_current_schema); + } + 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::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")); + Poco::JSON::Array::Ptr updates = new Poco::JSON::Array; - updates->add(set_snapshot); + { + 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); } - request_body->set("updates", updates); } try diff --git a/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h b/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h index 93ffa305b89f..a7c66b5b7f53 100644 --- a/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h +++ b/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h @@ -168,15 +168,15 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl void checkAlterIsPossible(const AlterCommands & commands) override { - assertInitializedDL(); - current_metadata->checkAlterIsPossible(commands); + if(current_metadata) + current_metadata->checkAlterIsPossible(commands); } - void alter(const AlterCommands & params, ContextPtr context) override + void alter(const AlterCommands & params, ContextPtr context, + const StorageID & storage_id, std::shared_ptr catalog) override { assertInitializedDL(); - current_metadata->alter(params, context); - + current_metadata->alter(params, shared_from_this(), context, storage_id, catalog); } ObjectStoragePtr createObjectStorage(ContextPtr context, bool is_readonly) override @@ -451,7 +451,6 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl void assertInitializedDL() const { - BaseStorageConfiguration::assertInitialized(); if (!current_metadata) throw Exception(ErrorCodes::LOGICAL_ERROR, "Metadata is not initialized"); } @@ -675,7 +674,9 @@ class StorageIcebergConfiguration : public StorageObjectStorageConfiguration, pu void checkAlterIsPossible(const AlterCommands & commands) override { getImpl().checkAlterIsPossible(commands); } - void alter(const AlterCommands & params, ContextPtr context) override { getImpl().alter(params, context); } + void alter(const AlterCommands & params, ContextPtr context, + const StorageID & storage_id, std::shared_ptr catalog) override + { getImpl().alter(params, context, storage_id, catalog); } const DataLakeStorageSettings & getDataLakeSettings() const override { return getImpl().getDataLakeSettings(); } diff --git a/src/Storages/ObjectStorage/DataLakes/IDataLakeMetadata.h b/src/Storages/ObjectStorage/DataLakes/IDataLakeMetadata.h index f58bf8d04dcd..1a18cc7c2163 100644 --- a/src/Storages/ObjectStorage/DataLakes/IDataLakeMetadata.h +++ b/src/Storages/ObjectStorage/DataLakes/IDataLakeMetadata.h @@ -201,7 +201,12 @@ class IDataLakeMetadata : boost::noncopyable virtual void addDeleteTransformers(ObjectInfoPtr, QueryPipelineBuilder &, const std::optional &, FormatParserSharedResourcesPtr, ContextPtr) const { } virtual void checkAlterIsPossible(const AlterCommands & /*commands*/) { throwNotImplemented("alter"); } - virtual void alter(const AlterCommands & /*params*/, ContextPtr /*context*/) { throwNotImplemented("alter"); } + virtual void alter( + const AlterCommands & /*params*/, + StorageObjectStorageConfigurationPtr /*configuration*/, + ContextPtr /*context*/, + const StorageID & /*storage_id*/, + std::shared_ptr /*catalog*/) { throwNotImplemented("alter"); } virtual void drop(ContextPtr) { } virtual std::optional partitionKey(ContextPtr) const { return {}; } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp index 36ac299adc4b..76aa32ee5a06 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp @@ -717,12 +717,17 @@ void IcebergMetadata::checkAlterIsPossible(const AlterCommands & commands) for (const auto & command : commands) { if (command.type != AlterCommand::Type::ADD_COLUMN && command.type != AlterCommand::Type::DROP_COLUMN - && command.type != AlterCommand::Type::MODIFY_COLUMN) + && command.type != AlterCommand::Type::MODIFY_COLUMN && command.type != AlterCommand::Type::RENAME_COLUMN) throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Alter of type '{}' is not supported by Iceberg storage", command.type); } } -void IcebergMetadata::alter(const AlterCommands & params, ContextPtr context) +void IcebergMetadata::alter( + const AlterCommands & params, + StorageObjectStorageConfigurationPtr configuration, + ContextPtr context, + const StorageID & storage_id, + std::shared_ptr catalog) { if (!context->getSettingsRef()[Setting::allow_experimental_insert_into_iceberg].value) { @@ -732,7 +737,11 @@ void IcebergMetadata::alter(const AlterCommands & params, ContextPtr context) "To allow its usage, enable setting allow_experimental_insert_into_iceberg"); } - Iceberg::alter(params, context, object_storage, data_lake_settings, persistent_components, write_format); + Iceberg::alter( + params, context, object_storage, data_lake_settings, persistent_components, write_format, + storage_id, catalog, + configuration ? configuration->getTypeName() : "", + configuration ? configuration->getNamespace() : ""); } void IcebergMetadata::createInitial( diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.h index c672f36de014..6fadce65baf5 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.h @@ -129,7 +129,12 @@ class IcebergMetadata : public IDataLakeMetadata void modifyFormatSettings(FormatSettings & format_settings, const Context & local_context) const override; void addDeleteTransformers(ObjectInfoPtr object_info, QueryPipelineBuilder & builder, const std::optional & format_settings, FormatParserSharedResourcesPtr parser_shared_resources, ContextPtr local_context) const override; void checkAlterIsPossible(const AlterCommands & commands) override; - void alter(const AlterCommands & params, ContextPtr context) override; + void alter( + const AlterCommands & params, + StorageObjectStorageConfigurationPtr configuration, + ContextPtr context, + const StorageID & storage_id, + std::shared_ptr catalog) override; ObjectIterator iterate( const ActionsDAG * filter_dag, diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp index 184c6a7f9359..6073c0a4bbb1 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp @@ -351,6 +351,53 @@ void MetadataGenerator::generateModifyColumnMetadata(const String & column_name, metadata_object->getArray(Iceberg::f_schemas)->add(current_schema); } +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 schema_fields = current_schema->getArray(Iceberg::f_fields); + + for (UInt32 i = 0; i < schema_fields->size(); ++i) + { + if (schema_fields->getObject(i)->getValue(Iceberg::f_name) == new_column_name) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Column {} already exists", new_column_name); + } + + 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) + { + current_field->set(Iceberg::f_name, new_column_name); + found = true; + break; + } + } + + 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); + metadata_object->getArray(Iceberg::f_schemas)->add(current_schema); +} + } #endif diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h index 035747dafa14..652d6009a37f 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h @@ -36,6 +36,7 @@ class MetadataGenerator void generateAddColumnMetadata(const String & column_name, DataTypePtr type); void generateDropColumnMetadata(const String & column_name); void generateModifyColumnMetadata(const String & column_name, DataTypePtr type); + void generateRenameColumnMetadata(const String & column_name, const String & new_column_name); private: Poco::JSON::Object::Ptr metadata_object; diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp index deeb05a49102..b58574b3d218 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp @@ -677,13 +677,16 @@ void alter( ObjectStoragePtr object_storage, const DataLakeStorageSettings & data_lake_settings, PersistentTableComponents & persistent_table_components, - const String & write_format) + const String & write_format, + StorageID storage_id, + std::shared_ptr catalog, + const String & blob_storage_type_name, + const String & blob_storage_namespace_name) { 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; - while (i++ < MAX_TRANSACTION_RETRIES) + for (size_t i = 0; i < MAX_TRANSACTION_RETRIES; ++i) { FileNamesGenerator filename_generator( persistent_table_components.table_path, persistent_table_components.table_path, false, CompressionMethod::None, write_format); @@ -717,6 +720,9 @@ void alter( case AlterCommand::Type::MODIFY_COLUMN: 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); + break; default: throw Exception(ErrorCodes::LOGICAL_ERROR, "Unknown type of alter {}", params[0].type); } @@ -727,6 +733,8 @@ void alter( auto [metadata_name, storage_metadata_name] = filename_generator.generateMetadataName(); + LOG_INFO(log, "Iceberg alter: writing metadata to '{}', latest version was {}", storage_metadata_name, last_version); + auto hint = filename_generator.generateVersionHint(); if (writeMetadataFileAndVersionHint( storage_metadata_name, @@ -737,11 +745,43 @@ void alter( context, compression_method, data_lake_settings[DataLakeStorageSetting::iceberg_use_version_hint])) - break; + { + if (catalog) + { + String catalog_filename = metadata_name; + if (!catalog_filename.starts_with(blob_storage_type_name)) + catalog_filename = blob_storage_type_name + "://" + blob_storage_namespace_name + "/" + metadata_name; + + const auto & [namespace_name, table_name] = DataLake::parseTableName(storage_id.getTableName()); + if (!catalog->updateMetadata(namespace_name, table_name, catalog_filename, metadata)) + { + LOG_WARNING(log, "Iceberg alter: catalog update failed for '{}'", catalog_filename); + continue; + } + } + return; + } + + bool file_exists = object_storage->exists(StoredObject(storage_metadata_name)); + LOG_WARNING(log, "Iceberg alter: failed to write metadata to '{}' (attempt {}, file exists: {})", + storage_metadata_name, i + 1, file_exists); + + if (file_exists && catalog) + { + String catalog_filename = metadata_name; + if (!catalog_filename.starts_with(blob_storage_type_name)) + catalog_filename = blob_storage_type_name + "://" + blob_storage_namespace_name + "/" + metadata_name; + + const auto & [namespace_name, table_name] = DataLake::parseTableName(storage_id.getTableName()); + if (catalog->updateMetadata(namespace_name, table_name, catalog_filename, metadata)) + { + LOG_INFO(log, "Iceberg alter: adopted existing metadata file '{}' and updated catalog", storage_metadata_name); + return; + } + } } - if (i == MAX_TRANSACTION_RETRIES) - throw Exception(ErrorCodes::LIMIT_EXCEEDED, "Too many unsuccessed retries to alter iceberg table"); + throw Exception(ErrorCodes::LIMIT_EXCEEDED, "Too many unsuccessful retries to alter iceberg table"); } #endif diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.h index c19282318319..8a7a0057c7ae 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.h @@ -38,7 +38,11 @@ void alter( ObjectStoragePtr object_storage, const DataLakeStorageSettings & data_lake_settings, PersistentTableComponents & persistent_table_components, - const String & write_format); + const String & write_format, + StorageID storage_id, + std::shared_ptr catalog, + const String & blob_storage_type_name, + const String & blob_storage_namespace_name); } #endif diff --git a/src/Storages/ObjectStorage/StorageObjectStorage.cpp b/src/Storages/ObjectStorage/StorageObjectStorage.cpp index b7a3260bc791..02dcb59ad3df 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorage.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorage.cpp @@ -792,14 +792,17 @@ void StorageObjectStorage::checkMutationIsPossible(const MutationCommands & comm void StorageObjectStorage::alter(const AlterCommands & params, ContextPtr context, AlterLockHolder & /*alter_lock_holder*/) { + configuration->update(object_storage, context, /* if_not_updated_before */ true); + StorageInMemoryMetadata new_metadata = getInMemoryMetadata(); params.apply(new_metadata, context); - configuration->alter(params, context); + configuration->alter(params, context, getStorageID(), catalog); + + auto database = DatabaseCatalog::instance().getDatabase(storage_id.database_name); + if (!database->isDatalakeCatalog()) + database->alterTable(context, storage_id, new_metadata, /*validate_new_create_query=*/true); - DatabaseCatalog::instance() - .getDatabase(storage_id.database_name) - ->alterTable(context, storage_id, new_metadata, /*validate_new_create_query=*/true); setInMemoryMetadata(new_metadata); } diff --git a/src/Storages/ObjectStorage/StorageObjectStorageConfiguration.h b/src/Storages/ObjectStorage/StorageObjectStorageConfiguration.h index 3f3129a9d3c4..175867021751 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageConfiguration.h +++ b/src/Storages/ObjectStorage/StorageObjectStorageConfiguration.h @@ -244,7 +244,8 @@ class StorageObjectStorageConfiguration } } - virtual void alter(const AlterCommands & /*params*/, ContextPtr /*context*/) {} + virtual void alter(const AlterCommands & /*params*/, ContextPtr /*context*/, + const StorageID & /*storage_id*/, std::shared_ptr /*catalog*/) {} virtual const DataLakeStorageSettings & getDataLakeSettings() const { diff --git a/tests/integration/test_storage_iceberg_no_spark/test_writes_rename_column.py b/tests/integration/test_storage_iceberg_no_spark/test_writes_rename_column.py new file mode 100644 index 000000000000..9d9f4220660a --- /dev/null +++ b/tests/integration/test_storage_iceberg_no_spark/test_writes_rename_column.py @@ -0,0 +1,74 @@ +import pytest + +from helpers.iceberg_utils import ( + create_iceberg_table, + get_uuid_str, +) + +INSERT_SETTINGS = {"allow_experimental_insert_into_iceberg": 1} + + +@pytest.mark.parametrize("format_version", [1, 2]) +@pytest.mark.parametrize("storage_type", ["local", "s3"]) +def test_rename_column_basic(started_cluster_iceberg_no_spark, format_version, storage_type): + """Rename a column: existing rows are readable under the new name, and new inserts work.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + TABLE_NAME = "test_rename_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} RENAME COLUMN value TO label;", settings=INSERT_SETTINGS) + + # existing rows readable under the new name + assert instance.query(f"SELECT id, label FROM {TABLE_NAME} ORDER BY id") == "1\thello\n2\tworld\n" + + # new inserts work under the new name + instance.query(f"INSERT INTO {TABLE_NAME} VALUES (3, 'foo');", settings=INSERT_SETTINGS) + assert instance.query(f"SELECT id, label FROM {TABLE_NAME} ORDER BY id") == "1\thello\n2\tworld\n3\tfoo\n" + + +@pytest.mark.parametrize("format_version", [1, 2]) +@pytest.mark.parametrize("storage_type", ["local", "s3"]) +def test_rename_column_errors(started_cluster_iceberg_no_spark, format_version, storage_type): + """Renaming a non-existent column or renaming to an already-used name must raise BAD_ARGUMENTS.""" + instance = started_cluster_iceberg_no_spark.instances["node1"] + TABLE_NAME = "test_rename_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, + ) + + # rename a column that does not exist — rejected by AlterCommands::validate (NOT_FOUND_COLUMN_IN_BLOCK) + error = instance.query_and_get_error( + f"ALTER TABLE {TABLE_NAME} RENAME COLUMN nonexistent TO other;", + settings=INSERT_SETTINGS, + ) + assert "nonexistent" in error + + # rename to a name already used by another column — rejected by AlterCommands::validate (DUPLICATE_COLUMN) + error = instance.query_and_get_error( + f"ALTER TABLE {TABLE_NAME} RENAME COLUMN value TO id;", + settings=INSERT_SETTINGS, + ) + assert "DUPLICATE_COLUMN" in error + assert "id" in error + + # table structure must be unchanged after both failed renames + assert instance.query( + f"SELECT name FROM system.columns WHERE database = currentDatabase() AND table = '{TABLE_NAME}' ORDER BY name" + ) == "id\nvalue\n"