From 35a7f03cea8b10f88a81900af854ae5c88359fa8 Mon Sep 17 00:00:00 2001 From: Andrey Zvonov <32552679+zvonand@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:45:53 +0000 Subject: [PATCH 1/4] Merge 38bc6be1f656c91dfc5f14d0a18277d7f007fd69 into 4e1add46ec15df58b82416d8b4951d9a8c310350 Port of https://github.com/ClickHouse/ClickHouse/pull/98670: native `CREATE TABLE` and `DROP TABLE` for `DataLakeCatalog`, with an optional catalog-side data purge requested through the new `data_lake_delete_data_on_drop` setting. Dropped: the explicit `catalog->createNamespaceIfNotExists` calls the PR adds to `DatabaseDataLake::createTable` and `IcebergMetadata::createInitial` - `ICatalog` has no such virtual on antalya-26.6, where `RestCatalog::createTable` and `GlueCatalog::createTable` create the namespace themselves Adapted: `toIcebergMetadataCompressionExtension` does not exist on antalya-26.6, so `GlueCatalog::createTable` derives the initial metadata file name with `toContentEncodingName`, reproducing the name `IcebergMetadata::createInitial` produces on this branch; `test_native_create_gzip_metadata` asserts that name Adapted: `RestCatalog` keeps `config` as a direct member instead of a `MultiVersion` state snapshot, so `getDefaultBaseLocation` and the REST endpoints read `config` directly Adapted: the Iceberg REST namespace-identifier fix (`namespaceToJSONArray`) was applied inside `buildUpdateMetadataRequestBody` and `buildUpdateSchemaRequestBody`, where antalya-26.6 builds those request bodies Adapted: `IcebergMetadata::drop` takes the resolved `delete_data` flag; the context its file enumeration needs now comes from `Context::getGlobalContextInstance` Adapted: `DatabaseDataLake` stores its settings in a plain member, not a `MultiVersion`, so `createTable` reads `settings` directly Adapted: the dynamic `DataLakeConfiguration` wrapper's `drop` override was re-signatured to `drop(bool)` Adapted: `ICEBERG_DELTA_SHARING` and `ICEBERG_HORIZON` were removed from the new catalog-type switches - antalya-26.6 does not have those enum values Adapted: `S3TablesCatalog::dropTable` only gains the new parameters; antalya-26.6 always purges and has no `delete_data` guard to extend Adapted: `src/Databases/DatabaseRemote.{h,cpp}` are not on antalya-26.6, so the `dropTable` signature change there does not apply --- .../BuzzHouse/Generator/SessionSettings.cpp | 2 +- src/Core/Settings.cpp | 6 +- src/Core/SettingsChangesHistory.cpp | 1 + src/Databases/DataLake/Common.cpp | 91 +++ src/Databases/DataLake/Common.h | 8 + src/Databases/DataLake/DatabaseDataLake.cpp | 345 +++++++- src/Databases/DataLake/DatabaseDataLake.h | 12 +- .../DataLake/DatabaseDataLakeSettings.cpp | 1 + src/Databases/DataLake/GlueCatalog.cpp | 137 +++- src/Databases/DataLake/GlueCatalog.h | 10 +- src/Databases/DataLake/ICatalog.cpp | 37 +- src/Databases/DataLake/ICatalog.h | 29 +- src/Databases/DataLake/RestCatalog.cpp | 88 +- src/Databases/DataLake/RestCatalog.h | 12 +- src/Databases/DataLake/S3TablesCatalog.cpp | 3 +- src/Databases/DataLake/S3TablesCatalog.h | 2 +- .../tests/gtest_construct_table_location.cpp | 136 ++++ src/Databases/DatabaseAtomic.cpp | 2 +- src/Databases/DatabaseAtomic.h | 2 +- src/Databases/DatabaseBackup.cpp | 1 + src/Databases/DatabaseBackup.h | 3 +- src/Databases/DatabaseMemory.cpp | 3 +- src/Databases/DatabaseMemory.h | 3 +- src/Databases/DatabaseOnDisk.cpp | 2 +- src/Databases/DatabaseOnDisk.h | 3 +- src/Databases/DatabaseOverlay.cpp | 4 +- src/Databases/DatabaseOverlay.h | 2 +- src/Databases/DatabaseReplicated.cpp | 2 +- src/Databases/DatabaseReplicated.h | 2 +- src/Databases/IDatabase.cpp | 3 +- src/Databases/IDatabase.h | 9 +- src/Databases/MySQL/DatabaseMySQL.cpp | 2 +- src/Databases/MySQL/DatabaseMySQL.h | 2 +- .../DatabaseMaterializedPostgreSQL.cpp | 8 +- .../DatabaseMaterializedPostgreSQL.h | 2 +- .../PostgreSQL/DatabasePostgreSQL.cpp | 2 +- src/Databases/PostgreSQL/DatabasePostgreSQL.h | 2 +- src/Interpreters/DDLWorker.cpp | 4 + src/Interpreters/InterpreterAlterQuery.cpp | 3 + src/Interpreters/InterpreterCreateQuery.cpp | 175 +++- src/Interpreters/InterpreterCreateQuery.h | 2 +- src/Interpreters/InterpreterDropQuery.cpp | 9 +- src/Interpreters/executeDDLQueryOnCluster.cpp | 42 + src/Interpreters/executeDDLQueryOnCluster.h | 8 + src/Storages/IStorage.h | 7 + .../DataLakes/DataLakeConfiguration.h | 6 +- .../DataLakes/IDataLakeMetadata.h | 4 +- .../DataLakes/Iceberg/Compaction.cpp | 2 +- .../DataLakes/Iceberg/IcebergMetadata.cpp | 83 +- .../DataLakes/Iceberg/IcebergMetadata.h | 2 +- .../DataLakes/Iceberg/IcebergPath.h | 2 + .../DataLakes/Iceberg/IcebergWrites.cpp | 6 +- .../DataLakes/Iceberg/IcebergWrites.h | 1 + .../DataLakes/Iceberg/MetadataGenerator.cpp | 8 +- .../DataLakes/Iceberg/MetadataGenerator.h | 2 +- .../DataLakes/Iceberg/Mutations.cpp | 18 +- .../ObjectStorage/DataLakes/Iceberg/Utils.cpp | 22 +- .../ObjectStorage/StorageObjectStorage.cpp | 22 +- .../ObjectStorage/StorageObjectStorage.h | 11 + .../StorageObjectStorageConfiguration.h | 4 +- src/Storages/StorageProxy.h | 4 + src/Storages/StorageTableFunction.h | 7 + src/Storages/StorageTableProxy.h | 10 + tests/integration/test_database_glue/test.py | 108 +++ .../integration/test_database_iceberg/test.py | 753 ++++++++++++++++++ .../test_drop_delete_data_on_drop.py | 49 ++ 66 files changed, 2216 insertions(+), 137 deletions(-) create mode 100644 src/Databases/DataLake/tests/gtest_construct_table_location.cpp create mode 100644 tests/integration/test_storage_iceberg_no_spark/test_drop_delete_data_on_drop.py diff --git a/src/Client/BuzzHouse/Generator/SessionSettings.cpp b/src/Client/BuzzHouse/Generator/SessionSettings.cpp index cdc597a90780..19dc85643cf9 100644 --- a/src/Client/BuzzHouse/Generator/SessionSettings.cpp +++ b/src/Client/BuzzHouse/Generator/SessionSettings.cpp @@ -873,7 +873,7 @@ std::unordered_map serverSettings = { [](RandomGenerator & rg, FuzzConfig &) { return std::to_string(rg.thresholdGenerator(0.3, 0.2, 0, 10800)); }, {}, false)}, - {"iceberg_delete_data_on_drop", trueOrFalseSettingNoOracle}, + {"data_lake_delete_data_on_drop", trueOrFalseSettingNoOracle}, {"iceberg_expire_default_min_snapshots_to_keep", CHSetting( [](RandomGenerator & rg, FuzzConfig &) { return std::to_string(rg.thresholdGenerator(0.2, 0.2, 0, 10)); }, {}, false)}, diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index 5d0f3cc34cc2..4b7c085db584 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -5551,9 +5551,9 @@ Possible values: - manifest_file_entry - Everything above + traversed avro manifest files entries. )", 0) \ \ - DECLARE(Bool, iceberg_delete_data_on_drop, false, R"( -Whether to delete all iceberg files on drop or not. -)", 0) \ + DECLARE_WITH_ALIAS(Bool, data_lake_delete_data_on_drop, false, R"( +Whether to delete the underlying data files when dropping a data lake table. For catalog databases the catalog is asked to purge the data (`purgeRequested=true`); for self-managed tables ClickHouse removes the files directly. +)", 0, iceberg_delete_data_on_drop) \ DECLARE(Int64, iceberg_expire_default_min_snapshots_to_keep, 1, R"( Default value for Iceberg table property `history.expire.min-snapshots-to-keep` used by `expire_snapshots` when that property is absent. )", 0) \ diff --git a/src/Core/SettingsChangesHistory.cpp b/src/Core/SettingsChangesHistory.cpp index 008ba54f43d2..9a61ec6784ee 100644 --- a/src/Core/SettingsChangesHistory.cpp +++ b/src/Core/SettingsChangesHistory.cpp @@ -42,6 +42,7 @@ const VersionToSettingsChangesMap & getSettingsChangesHistory() addSettingsChanges(settings_changes_history, "26.6.2.20001.altinityantalya", { {"use_puffin_files_cache", false, true, "Enables cache of parsed Puffin file content such as deletion vectors."}, + {"data_lake_delete_data_on_drop", false, false, "New setting that unifies dropping of data lake data; the released `iceberg_delete_data_on_drop` is kept as an alias for it."}, }); addSettingsChanges(settings_changes_history, "26.6", diff --git a/src/Databases/DataLake/Common.cpp b/src/Databases/DataLake/Common.cpp index 8946d3412d70..db917b750790 100644 --- a/src/Databases/DataLake/Common.cpp +++ b/src/Databases/DataLake/Common.cpp @@ -16,6 +16,9 @@ #include +#include +#include + namespace DB::ErrorCodes { extern const int BAD_ARGUMENTS; @@ -121,4 +124,92 @@ std::pair parseTableName(const std::string & name) return {namespace_name, table_name}; } +String constructTableLocation( + const String & location_scheme, + const String & storage_endpoint, + const String & namespace_name, + const String & table_name, + DB::S3UriStyle uri_style) +{ + Poco::URI uri(storage_endpoint); + auto path = uri.getPath(); + while (path.starts_with('/')) + path.erase(0, 1); + while (path.ends_with('/')) + path.pop_back(); + + if (location_scheme == "abfss") + { + /// Azure: `abfss://@/`. `storage_endpoint` is + /// `https:////` or `abfss://@/` + String container = uri.getUserInfo(); + String account_host = uri.getHost(); + String extra_path = path; + + if (container.empty()) + { + auto first_slash = extra_path.find('/'); + if (first_slash == String::npos) + { + container = std::move(extra_path); + extra_path.clear(); + } + else + { + container = extra_path.substr(0, first_slash); + extra_path = extra_path.substr(first_slash + 1); + } + } + + if (account_host.empty() || container.empty()) + throw DB::Exception( + DB::ErrorCodes::BAD_ARGUMENTS, + "`storage_endpoint` ({}) for Azure must include both account host and container " + "(expected https://.dfs.core.windows.net/[/] or " + "abfss://@.dfs.core.windows.net[/])", + storage_endpoint); + + if (extra_path.empty()) + return fmt::format("abfss://{}@{}/{}/{}", container, account_host, namespace_name, table_name); + return fmt::format("abfss://{}@{}/{}/{}/{}", container, account_host, extra_path, namespace_name, table_name); + } + + if (location_scheme == "s3") + { + if (uri_style == DB::S3UriStyle::VIRTUAL_HOSTED) + /// A virtual-hosted endpoint cannot be split into bucket and service unambiguously + /// (`s3.us-east-1.amazonaws.com`, an IP host, and a dotted bucket name all look alike), so deriving + /// the bucket from the host would silently misplace data. Require an explicit `default_base_location`. + throw DB::Exception( + DB::ErrorCodes::BAD_ARGUMENTS, + "CREATE TABLE with `storage_uri_style = 'virtual_hosted'` cannot derive the bucket from " + "`storage_endpoint` ({}); set `default_base_location` (a full s3:///) instead.", + storage_endpoint); + + if (path.empty()) + throw DB::Exception( + DB::ErrorCodes::BAD_ARGUMENTS, + "`storage_endpoint` ({}) does not contain a bucket; " + "CREATE TABLE in DataLakeCatalog requires `storage_endpoint` to include a non-empty bucket path.", + storage_endpoint); + return fmt::format("s3://{}/{}/{}", path, namespace_name, table_name); + } + + /// HDFS / file / other schemes that may have `authority`. + String authority = uri.getAuthority(); + if (authority.empty()) + { + if (path.empty()) + throw DB::Exception( + DB::ErrorCodes::BAD_ARGUMENTS, + "`storage_endpoint` ({}) does not contain a path", + storage_endpoint); + return fmt::format("{}:///{}/{}/{}", location_scheme, path, namespace_name, table_name); + } + + if (path.empty()) + return fmt::format("{}://{}/{}/{}", location_scheme, authority, namespace_name, table_name); + return fmt::format("{}://{}/{}/{}/{}", location_scheme, authority, path, namespace_name, table_name); +} + } diff --git a/src/Databases/DataLake/Common.h b/src/Databases/DataLake/Common.h index 9b0dd7c626a6..912758f7fae8 100644 --- a/src/Databases/DataLake/Common.h +++ b/src/Databases/DataLake/Common.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include @@ -19,4 +20,11 @@ DB::DataTypePtr getType(const String & type_name, bool nullable, DB::ContextPtr /// `E` is a table name. std::pair parseTableName(const std::string & name); +String constructTableLocation( + const String & location_scheme, + const String & storage_endpoint, + const String & namespace_name, + const String & table_name, + DB::S3UriStyle uri_style = DB::S3UriStyle::AUTO); + } diff --git a/src/Databases/DataLake/DatabaseDataLake.cpp b/src/Databases/DataLake/DatabaseDataLake.cpp index ba2fc89c7799..955600d0a11a 100644 --- a/src/Databases/DataLake/DatabaseDataLake.cpp +++ b/src/Databases/DataLake/DatabaseDataLake.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #if USE_AVRO && USE_PARQUET @@ -44,14 +45,19 @@ #include #include +#include #include #include #include #include #include +#include +#include +#include #include #include +#include namespace DB { @@ -63,6 +69,7 @@ namespace DatabaseDataLakeSetting extern const DatabaseDataLakeSettingsString auth_header; extern const DatabaseDataLakeSettingsString auth_scope; extern const DatabaseDataLakeSettingsString storage_endpoint; + extern const DatabaseDataLakeSettingsString default_base_location; extern const DatabaseDataLakeSettingsS3UriStyle storage_uri_style; extern const DatabaseDataLakeSettingsString oauth_server_uri; extern const DatabaseDataLakeSettingsBool oauth_server_use_request_body; @@ -107,7 +114,8 @@ namespace Setting extern const SettingsBool parallel_replicas_for_cluster_engines; extern const SettingsString cluster_for_parallel_replicas; extern const SettingsBool database_datalake_require_metadata_access; - + extern const SettingsBool data_lake_delete_data_on_drop; + extern const SettingsString iceberg_metadata_compression_method; } namespace DataLakeStorageSetting @@ -123,6 +131,7 @@ namespace ErrorCodes extern const int DATALAKE_DATABASE_ERROR; extern const int CANNOT_GET_CREATE_TABLE_QUERY; extern const int LOGICAL_ERROR; + extern const int TABLE_ALREADY_EXISTS; } namespace FailPoints @@ -131,6 +140,63 @@ namespace FailPoints extern const char datalake_try_get_table_return_nullptr[]; } +namespace +{ + +String getLocationSchemeForTableCreation(const std::shared_ptr & catalog) +{ + if (auto storage_type = catalog->getStorageType(); storage_type.has_value()) + return DataLake::storageTypeToScheme(*storage_type); + + /// Fall back only for catalogs whose backing storage is fixed. + /// REST/Hive/Glue/Paimon/Unity can be backed by anything, so we refuse to guess. + switch (catalog->getCatalogType()) + { + case DatabaseDataLakeCatalogType::ICEBERG_ONELAKE: + return "abfss"; /// Azure-only + case DatabaseDataLakeCatalogType::ICEBERG_BIGLAKE: + return "s3"; /// GCS via S3 API + case DatabaseDataLakeCatalogType::ICEBERG_REST: + case DatabaseDataLakeCatalogType::S3_TABLES: + case DatabaseDataLakeCatalogType::ICEBERG_HIVE: + case DatabaseDataLakeCatalogType::GLUE: + case DatabaseDataLakeCatalogType::PAIMON_REST: + case DatabaseDataLakeCatalogType::UNITY: + case DatabaseDataLakeCatalogType::NONE: + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "Cannot determine storage scheme for CREATE TABLE for catalog type '{}': the catalog does not " + "report a backing storage type. Set `default_base_location` on the database or configure " + "the catalog to expose `default-base-location`.", + catalog->getCatalogType()); + } + + throw Exception(ErrorCodes::LOGICAL_ERROR, "Unexpected catalog type in CREATE TABLE location scheme resolution"); +} + +/// The storage backend a catalog is pinned to when creating a table: what the catalog reports, or, +/// for the catalogs whose backing storage is fixed by the service itself, the backend that +/// `getLocationSchemeForTableCreation` falls back to. `CREATE TABLE` must validate the engine and +/// `default_base_location` against this, not only against what the catalog reports: a `OneLake` +/// catalog that does not expose `default-base-location` is still Azure-only. +std::optional getFixedStorageTypeForTableCreation(const std::shared_ptr & catalog) +{ + if (auto storage_type = catalog->getStorageType(); storage_type.has_value()) + return storage_type; + + switch (catalog->getCatalogType()) + { + case DatabaseDataLakeCatalogType::ICEBERG_ONELAKE: + return DatabaseDataLakeStorageType::Azure; + case DatabaseDataLakeCatalogType::ICEBERG_BIGLAKE: + return DatabaseDataLakeStorageType::S3; /// GCS via the S3 API + default: + return {}; + } +} + +} + DatabaseDataLake::DatabaseDataLake( const std::string & database_name_, const std::string & url_, @@ -678,9 +744,8 @@ StoragePtr DatabaseDataLake::tryGetTableImpl(const String & name, ContextPtr con if (!metadata_location.empty()) { metadata_location = table_metadata.getMetadataLocation(metadata_location); + (*storage_settings)[DB::DataLakeStorageSetting::iceberg_metadata_file_path] = metadata_location; } - - (*storage_settings)[DB::DataLakeStorageSetting::iceberg_metadata_file_path] = metadata_location; } const auto configuration = getConfiguration(storage_type, storage_settings); @@ -779,16 +844,190 @@ StoragePtr DatabaseDataLake::tryGetTableImpl(const String & name, ContextPtr con return storage_cluster; } -void DatabaseDataLake::dropTable( /// NOLINT +void DatabaseDataLake::validateCreateTableEngine(const String & engine_name) const +{ + /// `Iceberg` selects its backend from the optional `disk` setting. The setting is resolved by the + /// storage factory, after this database-level validation, so it cannot be safely checked here. + /// A fixed-backend catalog must not accept it: it could create a table that the catalog reopens + /// with a different backend. + if (engine_name == "Iceberg" && getFixedStorageTypeForTableCreation(getCatalog()).has_value()) + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "The generic 'Iceberg' engine is not supported for a DataLakeCatalog with a fixed storage backend. " + "Use the matching backend-specific Iceberg engine instead"); + + /// Unrecognized names pin no backend and are accepted here; they are rejected by the storage factory. + std::optional engine_backend; + if (engine_name == "IcebergS3") + engine_backend = DatabaseDataLakeStorageType::S3; + else if (engine_name == "IcebergAzure") + engine_backend = DatabaseDataLakeStorageType::Azure; + else if (engine_name == "IcebergHDFS") + engine_backend = DatabaseDataLakeStorageType::HDFS; + else if (engine_name == "IcebergLocal") + engine_backend = DatabaseDataLakeStorageType::Local; + + if (!engine_backend.has_value()) + return; + + /// A catalog without a fixed backend reopens the table using its own location, so any backend fits. + auto catalog_storage_type = getFixedStorageTypeForTableCreation(getCatalog()); + if (!catalog_storage_type.has_value() || *catalog_storage_type == *engine_backend) + return; + + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "Table engine '{}' uses the {} storage backend, but this DataLakeCatalog stores tables on {}. " + "The table would be reopened with the catalog's storage backend and become unreadable " + "immediately after creation. Use a matching Iceberg engine or the generic 'Iceberg' engine", + engine_name, *engine_backend, *catalog_storage_type); +} + +void DatabaseDataLake::createTable( ContextPtr context_, const String & name, - bool /*sync*/) + const StoragePtr & table, + const ASTPtr & query) { - auto table = tryGetTable(name, context_); + /// Engine-clause path: the storage's own initialization (IcebergMetadata::createInitial) + /// already wrote metadata and registered the table in the catalog; a path there that + /// registers nothing throws `TABLE_ALREADY_EXISTS` instead of returning. if (table) - table->drop(); + return; + + auto catalog = getCatalog(); + const auto & create = query->as(); + const auto [namespace_name, table_name] = DataLake::parseTableName(name); + + ColumnsDescription columns; + if (create.columns_list && create.columns_list->columns) + { + for (const auto & child : create.columns_list->columns->children) + { + const auto * col_decl = child->as(); + if (!col_decl || !col_decl->getType()) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Invalid column declaration in CREATE TABLE"); + + if (col_decl->default_specifier != ColumnDefaultSpecifier::Empty) + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "Column '{}': {} is not yet supported by DataLakeCatalog table creation", + col_decl->name, + toString(col_decl->default_specifier)); + + if (col_decl->getComment() || col_decl->getCodec() || col_decl->getTTL() + || col_decl->getStatisticsDesc() || col_decl->getSettings() + || col_decl->primary_key_specifier) + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "Column '{}': COMMENT, CODEC, TTL, STATISTICS, SETTINGS, and PRIMARY KEY are not supported by DataLakeCatalog table creation", + col_decl->name); + + columns.add(ColumnDescription(col_decl->name, DataTypeFactory::instance().get(col_decl->getType()))); + } + } + + if (columns.empty()) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Cannot create table without columns"); + + if (create.columns_list + && ((create.columns_list->indices && !create.columns_list->indices->children.empty()) + || (create.columns_list->constraints && !create.columns_list->constraints->children.empty()) + || (create.columns_list->projections && !create.columns_list->projections->children.empty()) + || create.columns_list->primary_key + || create.columns_list->primary_key_from_columns)) + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "DataLakeCatalog CREATE TABLE does not support PRIMARY KEY, indices, constraints, or projections"); + + ASTPtr partition_by; + ASTPtr order_by; + if (create.storage) + { + if (create.storage->primary_key || create.storage->sample_by + || create.storage->ttl_table || create.storage->unique_key + || create.storage->settings) + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "DataLakeCatalog CREATE TABLE supports only PARTITION BY and ORDER BY; " + "PRIMARY KEY, SAMPLE BY, TTL, UNIQUE KEY, and engine SETTINGS are not supported"); + + if (create.storage->partition_by) + partition_by = create.storage->partition_by->clone(); + if (create.storage->order_by) + order_by = create.storage->order_by->clone(); + } + + String base_location = catalog->getDefaultBaseLocation(); + if (base_location.empty()) + base_location = settings[DatabaseDataLakeSetting::default_base_location].value; + + String location; + if (!base_location.empty()) + { + if (auto catalog_storage_type = getFixedStorageTypeForTableCreation(catalog); catalog_storage_type.has_value() + && DataLake::parseStorageTypeFromLocation(base_location) != *catalog_storage_type) + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "`default_base_location` uses the {} storage backend, but this DataLakeCatalog stores tables on {}. " + "The table would be reopened with the catalog's storage backend and become unreadable " + "immediately after creation", + DataLake::parseStorageTypeFromLocation(base_location), *catalog_storage_type); + + while (base_location.ends_with('/')) + base_location.pop_back(); + location = fmt::format("{}/{}/{}", base_location, namespace_name, table_name); + } else - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Cannot drop table {} because it does not exist", name); + { + const auto storage_endpoint = settings[DatabaseDataLakeSetting::storage_endpoint].value; + if (storage_endpoint.empty()) + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "CREATE TABLE in DataLakeCatalog requires `default_base_location` or `storage_endpoint`"); + location = DataLake::constructTableLocation( + getLocationSchemeForTableCreation(catalog), storage_endpoint, namespace_name, table_name, + settings[DatabaseDataLakeSetting::storage_uri_style]); + } + + auto [metadata_content, metadata_str] = Iceberg::createEmptyMetadataFile( + location, + columns, + partition_by, + order_by, + context_); + + /// Catalogs that write the initial metadata file themselves (they get an empty `metadata_path`) must + /// honour `iceberg_metadata_compression_method` too, otherwise the native CREATE TABLE path would + /// diverge from the explicit Iceberg engine path, which applies it in `IcebergMetadata::createInitial`. + const auto compression_method_str = context_->getSettingsRef()[Setting::iceberg_metadata_compression_method].value; + const auto compression_method = chooseCompressionMethod(compression_method_str, compression_method_str); + + const bool created = catalog->createTable( + namespace_name, table_name, /* metadata_path */ "", metadata_content, compression_method, create.if_not_exists); + if (!created) + { + /// `IF NOT EXISTS`, and the catalog answered that the table is already there: it is shared, so + /// another client can create the same name between the existence check in `doCreateTable` and + /// this call. Nothing was created here, and the caller must be able to tell - report it exactly + /// like the local existence check does, so that `CREATE TABLE IF NOT EXISTS ... AS SELECT` does + /// not insert the selected rows into the table the other client created. + throw Exception(ErrorCodes::TABLE_ALREADY_EXISTS, + "Table {}.{} already exists in the catalog", namespace_name, table_name); + } + + LOG_INFO(log, "Created table {}.{}", namespace_name, table_name); +} + +void DatabaseDataLake::dropTable( /// NOLINT + ContextPtr context_, + const String & name, + bool /*sync*/, + bool if_exists) +{ + auto catalog = getCatalog(); + const auto [namespace_name, table_name] = DataLake::parseTableName(name); + + bool purge = context_->getSettingsRef()[Setting::data_lake_delete_data_on_drop]; + catalog->dropTable(namespace_name, table_name, purge, if_exists); + + /// A catalog-side drop removes remote metadata and, when purge is set, can request deletion of the + /// underlying data. Log it at an operational level so accidental drops/purges leave an audit trail. + LOG_INFO(log, "Dropped table {}.{} from DataLakeCatalog (purge={})", namespace_name, table_name, purge); } DatabaseTablesIteratorPtr DatabaseDataLake::getTablesIterator( @@ -1268,6 +1507,7 @@ The following settings are supported: | `auth_header` | Custom HTTP header for authentication with the catalog service | | `auth_scope` | OAuth2 scope for authentication (if using OAuth) | | `storage_endpoint` | Endpoint URL for the underlying storage | +| `default_base_location` | Base URI for new tables when the catalog does not report `default-base-location`. New tables are placed under `//` (e.g. `s3://warehouse/data`) | | `oauth_server_uri` | URI of the OAuth2 authorization server for authentication | | `vended_credentials` | Boolean indicating whether to use vended credentials from the catalog (supports AWS S3 and Azure ADLS Gen2) | | `vended_credentials_cache_ttl` | Maximum cache entry lifetime (in seconds) for vended credentials (REST catalogs only). Default `300`; `0` disables caching. | @@ -1277,6 +1517,95 @@ The following settings are supported: | `dlf_access_key_id` | Access key ID for DLF access | | `dlf_access_key_secret` | Access key Secret for DLF access | +## Creating tables {#creating-tables} + +An Iceberg table in a `DataLakeCatalog` database can be created directly from ClickHouse. + +:::note +`CREATE TABLE` and `DROP TABLE` require a catalog that can perform catalog mutations. They are supported +for Iceberg REST catalogs (including OneLake, BigLake, and Delta Sharing) and for the AWS Glue catalog. +Other catalog types (Unity, Hive Metastore, Paimon REST) are read-only and reject these statements. +::: + +The location of a newly created table comes from `default_base_location` (a full `s3://bucket/prefix`) when +set, otherwise the bucket is derived from `storage_endpoint`. With `storage_uri_style = 'virtual_hosted'` the +bucket cannot be derived from the endpoint unambiguously, so `default_base_location` is required for +`CREATE TABLE`. + +The table name must be quoted with backticks and include the namespace separated by a dot: + +```sql +CREATE TABLE catalog_db.`namespace.table_name` +( + id Int64, + name String, + value Float64 +) +PARTITION BY id +ORDER BY name +SETTINGS allow_database_iceberg = 1; +``` + +Iceberg accepts only a fixed set of partition transforms, so `PARTITION BY` +must use one of the following expressions: + +| Expression | Iceberg transform | +|-------------------------------|-------------------| +| `` | `identity` | +| `toYearNumSinceEpoch()` | `year` | +| `toMonthNumSinceEpoch()` | `month` | +| `toRelativeDayNum()` | `day` | +| `toRelativeHourNum()` | `hour` | +| `icebergTruncate(N, )` | `truncate[N]` | +| `icebergBucket(N, )` | `bucket[N]` | + +Composite partitioning is supported via `PARTITION BY (expr1, expr2, ...)`. +Other expressions (e.g. `toYYYYMM`, `intDiv`) are rejected at `CREATE TABLE`. + +Only the column names and types, `PARTITION BY`, and `ORDER BY` are persisted into the Iceberg +table metadata. Anything else — the storage clauses `PRIMARY KEY`, `SAMPLE BY`, `TTL`, and +`UNIQUE KEY`; indices, constraints, and projections; and the column modifiers `DEFAULT`, +`MATERIALIZED`, `ALIAS`, `EPHEMERAL`, `COMMENT`, `CODEC`, `TTL`, `STATISTICS`, and `SETTINGS` — +is rejected rather than silently dropped. This applies both with and without an explicit +`ENGINE` clause. Engine `SETTINGS` are accepted only together with an explicit Iceberg engine, +where they are the engine's storage settings (e.g. `iceberg_format_version`). + +You can also create an Iceberg table that inherits the schema of an existing table: + +```sql +CREATE TABLE catalog_db.`namespace.table_name` +AS other_db.source_table +SETTINGS allow_database_iceberg = 1; +``` + +If the source table's `PARTITION BY` and `ORDER BY` use only the expressions +listed above, they are copied into the new Iceberg table. + +## Dropping tables {#dropping-tables} + +Tables can be dropped from a `DataLakeCatalog` database. +`DROP TABLE` sends a delete request to the remote catalog, which removes +the table entry from the catalog. + +```sql +DROP TABLE catalog_db.`namespace.table_name` +``` + +By default, ClickHouse does not request the catalog to delete the underlying data. In order to do it, use the `data_lake_delete_data_on_drop` setting: + +```sql +DROP TABLE catalog_db.`namespace.table_name` +SETTINGS data_lake_delete_data_on_drop = 1 +``` + +:::note +Whether data files are actually deleted depends on the catalog itself. +The `purgeRequested` flag is sent to the catalog, but the catalog may choose to ignore it. +For the Glue catalog, `DROP TABLE` only removes the catalog entry and does not delete the underlying data +files, so `DROP TABLE` with `data_lake_delete_data_on_drop = 1` is rejected instead of silently leaving the +data behind. +::: + ## Examples {#examples} See below sections for examples of using the `DataLakeCatalog` engine: diff --git a/src/Databases/DataLake/DatabaseDataLake.h b/src/Databases/DataLake/DatabaseDataLake.h index bd5fc6ac44f0..ad4160f76c0f 100644 --- a/src/Databases/DataLake/DatabaseDataLake.h +++ b/src/Databases/DataLake/DatabaseDataLake.h @@ -29,6 +29,7 @@ class DatabaseDataLake final : public IDatabase, WithContext UUID getUUID() const override { return db_uuid; } bool shouldBeEmptyOnDetach() const override { return false; } + bool isRemoteDatabase() const override { return true; } bool isDatalakeCatalog() const override { return true; } bool empty() const override; @@ -56,16 +57,19 @@ class DatabaseDataLake final : public IDatabase, WithContext std::vector> getTablesForBackup(const FilterByNameFunction &, const ContextPtr &) const override { return {}; } + void validateCreateTableEngine(const String & engine_name) const override; + void createTable( - ContextPtr /*context*/, - const String & /*name*/, + ContextPtr context, + const String & name, const StoragePtr & /*table*/, - const ASTPtr & /*query*/) override {} + const ASTPtr & query) override; void dropTable( /// NOLINT ContextPtr context_, const String & name, - bool /*sync*/) override; + bool /*sync*/, + bool if_exists) override; std::shared_ptr getCatalog() const; protected: diff --git a/src/Databases/DataLake/DatabaseDataLakeSettings.cpp b/src/Databases/DataLake/DatabaseDataLakeSettings.cpp index 216d32290c24..3514738daf50 100644 --- a/src/Databases/DataLake/DatabaseDataLakeSettings.cpp +++ b/src/Databases/DataLake/DatabaseDataLakeSettings.cpp @@ -33,6 +33,7 @@ namespace ErrorCodes DECLARE(String, aws_role_session_name, "", "Role session name for AWS connection for Glue catalog", 0) \ DECLARE(String, aws_external_id, "", "External id for the AWS STS AssumeRole trust policy for Glue catalog", 0) \ DECLARE(String, storage_endpoint, "", "Object storage endpoint", 0) \ + DECLARE(String, default_base_location, "", "Base URI under which CREATE TABLE places new tables. Used only when the catalog does not report `default-base-location`", 0) \ DECLARE(S3UriStyle, storage_uri_style, S3UriStyle::AUTO, "URL style used when constructing object storage URLs from catalog-provided table locations. Use 'virtual_hosted' when the object storage server requires the bucket in the hostname (e.g. https://bucket.endpoint.com/path/)", 0) \ DECLARE(String, onelake_tenant_id, "", "Tenant id from azure", 0) \ DECLARE(String, onelake_client_id, "", "Client id from azure", 0) \ diff --git a/src/Databases/DataLake/GlueCatalog.cpp b/src/Databases/DataLake/GlueCatalog.cpp index a8dda240e7ce..2aa3bfe1fef3 100644 --- a/src/Databases/DataLake/GlueCatalog.cpp +++ b/src/Databases/DataLake/GlueCatalog.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -49,9 +50,12 @@ #include #include #include +#include #include #include +#include #include +#include #include #include @@ -61,6 +65,8 @@ namespace DB::ErrorCodes extern const int DATALAKE_DATABASE_ERROR; extern const int FAULT_INJECTED; extern const int CATALOG_NAMESPACE_DISABLED; + extern const int NOT_IMPLEMENTED; + extern const int S3_ERROR; } namespace DB::FailPoints @@ -380,7 +386,7 @@ bool GlueCatalog::tryGetTableMetadata( auto setup_specific_properties = [&] { const auto & table_params = table_outcome.GetParameters(); - if (table_params.contains("metadata_location")) + if (table_params.contains("metadata_location") && !table_params.at("metadata_location").empty()) { result.setDataLakeSpecificProperties(DataLakeSpecificProperties{.iceberg_metadata_file_location = table_params.at("metadata_location")}); } @@ -642,6 +648,23 @@ String GlueCatalog::resolveMetadataPathFromTableLocation(const String & table_lo void GlueCatalog::createNamespaceIfNotExists(const String & namespace_name) const { + /// Check existence first: `CreateDatabase` may be denied to a principal that is still allowed to + /// create tables in a pre-provisioned namespace, so it must not be called when there is nothing to create. + Aws::Glue::Model::GetDatabaseRequest get_request; + get_request.SetName(namespace_name); + + auto get_outcome = glue_client->GetDatabase(get_request); + if (get_outcome.IsSuccess()) + return; + + if (get_outcome.GetError().GetErrorType() != Aws::Glue::GlueErrors::ENTITY_NOT_FOUND) + { + throw DB::Exception( + DB::ErrorCodes::DATALAKE_DATABASE_ERROR, + "Exception calling GetDatabase for namespace {}: {}", + namespace_name, get_outcome.GetError().GetMessage()); + } + Aws::Glue::Model::CreateDatabaseRequest create_request; Aws::Glue::Model::DatabaseInput db_input; db_input.SetName(namespace_name); @@ -652,7 +675,13 @@ void GlueCatalog::createNamespaceIfNotExists(const String & namespace_name) cons glue_client->CreateDatabase(create_request); } -void GlueCatalog::createTable(const String & namespace_name, const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr /*metadata_content*/) const +bool GlueCatalog::createTable( + const String & namespace_name, + const String & table_name, + const String & new_metadata_path, + Poco::JSON::Object::Ptr metadata_content, + DB::CompressionMethod metadata_compression_method, + bool if_not_exists) const { if (!isNamespaceAllowed(namespace_name)) throw DB::Exception(DB::ErrorCodes::CATALOG_NAMESPACE_DISABLED, @@ -661,6 +690,63 @@ void GlueCatalog::createTable(const String & namespace_name, const String & tabl createNamespaceIfNotExists(namespace_name); + String effective_metadata_path = new_metadata_path; + + DB::ObjectStoragePtr written_metadata_storage; + String written_metadata_file; + + /// The initial metadata file staged below must not outlive a registration that did not happen: the + /// write is guarded by `If-None-Match: *`, so a leftover permanently blocks every retry, and when the + /// winner of an `IF NOT EXISTS` name race registered the table at another location (an explicit + /// Iceberg engine path, or a different `default_base_location`), the file is left behind in an orphan + /// directory the catalog does not point at. Only this call can have created that object - a writer + /// that lost the `If-None-Match` race returns before staging anything - so removing it is safe. + bool registered = false; + SCOPE_EXIT_SAFE({ + if (!registered && written_metadata_storage) + written_metadata_storage->removeObjectIfExists(DB::StoredObject(written_metadata_file)); + }); + + if (effective_metadata_path.empty() && metadata_content && metadata_content->has("location")) + { + String table_location = metadata_content->getValue("location"); + while (table_location.ends_with('/')) + table_location = table_location.substr(0, table_location.size() - 1); + + TableMetadata dummy_metadata; + auto [object_storage, bucket_name, table_path] = createObjectStorageForEarlyTableAccess(table_location, dummy_metadata); + + /// Name the file exactly like `IcebergMetadata::createInitial` does, so other engines can + /// locate the metadata file. + String compression_suffix = DB::toContentEncodingName(metadata_compression_method); + if (!compression_suffix.empty()) + compression_suffix = "." + compression_suffix; + + String metadata_filename = fmt::format("{}/metadata/v1{}.metadata.json", table_path, compression_suffix); + + std::ostringstream oss; // STYLE_CHECK_ALLOW_STD_STRING_STREAM + Poco::JSON::Stringifier::stringify(metadata_content, oss, 4); + String metadata_str = DB::removeEscapedSlashes(oss.str()); + + try + { + DB::Iceberg::writeMessageToFile(metadata_str, metadata_filename, object_storage, getContext(), "*", "", metadata_compression_method); + } + catch (const DB::Exception & e) + { + /// The write is guarded by `If-None-Match: *`, so S3 answers `PreconditionFailed` once the + /// initial metadata file is there - someone else created this table first. + if (if_not_exists && e.code() == DB::ErrorCodes::S3_ERROR && e.message().contains("PreconditionFailed")) + return false; + throw; + } + + written_metadata_storage = object_storage; + written_metadata_file = metadata_filename; + + effective_metadata_path = "s3://" + bucket_name + "/" + metadata_filename; + } + Aws::Glue::Model::CreateTableRequest request; request.SetDatabaseName(namespace_name); @@ -668,18 +754,21 @@ void GlueCatalog::createTable(const String & namespace_name, const String & tabl table_input.SetName(table_name); Aws::Glue::Model::StorageDescriptor sd; - fs::path original_path = new_metadata_path; + if (!effective_metadata_path.empty()) + { + fs::path original_path = effective_metadata_path; - fs::path parent = original_path.parent_path(); - fs::path grandparent = parent.parent_path(); + fs::path parent = original_path.parent_path(); + fs::path grandparent = parent.parent_path(); - sd.SetLocation(grandparent.c_str()); + sd.SetLocation(grandparent.c_str()); + } table_input.SetStorageDescriptor(sd); table_input.SetTableType("ICEBERG"); Aws::Map parameters; - parameters["metadata_location"] = new_metadata_path; + parameters["metadata_location"] = effective_metadata_path; parameters["table_type"] = "ICEBERG"; table_input.SetParameters(parameters); @@ -695,7 +784,17 @@ void GlueCatalog::createTable(const String & namespace_name, const String & tabl } if (!response.IsSuccess()) - throw DB::Exception(DB::ErrorCodes::DATALAKE_DATABASE_ERROR, "Can not create metadata in glue catalog: {}", response.GetError().GetMessage()); + { + /// `AlreadyExistsException` means someone else registered the table first. The staged metadata + /// file is removed by the scope guard, so `IF NOT EXISTS` leaves nothing behind. + if (if_not_exists && response.GetError().GetErrorType() == Aws::Glue::GlueErrors::ALREADY_EXISTS) + return false; + throw DB::Exception( + DB::ErrorCodes::DATALAKE_DATABASE_ERROR, "Can not create metadata in glue catalog: {}", response.GetError().GetMessage()); + } + + registered = true; + return true; } bool GlueCatalog::updateMetadata(const String & namespace_name, const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr /*new_snapshot*/) const @@ -753,13 +852,29 @@ bool GlueCatalog::updateSchema( return updateMetadata(namespace_name, table_name, new_metadata_path, nullptr); } -void GlueCatalog::dropTable(const String & namespace_name, const String & table_name) const +void GlueCatalog::dropTable(const String & namespace_name, const String & table_name, bool purge, bool if_exists) const { if (!isNamespaceAllowed(namespace_name)) throw DB::Exception(DB::ErrorCodes::CATALOG_NAMESPACE_DISABLED, "Failed to drop table {}, namespace {} is filtered by `namespaces` database parameter", table_name, namespace_name); + /// We drop only via Glue's `DeleteTable`, which removes the catalog entry but leaves the data files in + /// object storage; deleting them (the client-side purge that e.g. Iceberg's own GlueCatalog performs) is + /// not implemented here. Reject a requested purge rather than silently ignore it and orphan the data. + /// TODO: implement the client-side purge (delete the table's data/manifest/metadata files, then + /// `DeleteTable`) in a follow-up PR so `data_lake_delete_data_on_drop` can be honored for Glue. + if (purge) + { + if (if_exists && !existsTable(namespace_name, table_name)) + return; + + throw DB::Exception( + DB::ErrorCodes::NOT_IMPLEMENTED, + "data_lake_delete_data_on_drop is not supported for the Glue catalog: dropping only removes the Glue " + "catalog entry and does not delete the underlying data files"); + } + Aws::Glue::Model::DeleteTableRequest request; request.SetDatabaseName(namespace_name); request.SetName(table_name); @@ -772,7 +887,9 @@ void GlueCatalog::dropTable(const String & namespace_name, const String & table_ response = glue_client->DeleteTable(request); } - if (!response.IsSuccess()) + /// `EntityNotFoundException` means the table is already gone - someone else dropped it first. + if (!response.IsSuccess() + && !(if_exists && response.GetError().GetErrorType() == Aws::Glue::GlueErrors::ENTITY_NOT_FOUND)) throw DB::Exception( DB::ErrorCodes::DATALAKE_DATABASE_ERROR, "Can not delete table from glue catalog: {}", diff --git a/src/Databases/DataLake/GlueCatalog.h b/src/Databases/DataLake/GlueCatalog.h index 8d10ba0c8667..27c1785b345c 100644 --- a/src/Databases/DataLake/GlueCatalog.h +++ b/src/Databases/DataLake/GlueCatalog.h @@ -66,7 +66,13 @@ class GlueCatalog final : public ICatalog, private DB::WithContext return DB::DatabaseDataLakeCatalogType::GLUE; } - void createTable(const String & namespace_name, const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr metadata_content) const override; + bool createTable( + const String & namespace_name, + const String & table_name, + const String & new_metadata_path, + Poco::JSON::Object::Ptr metadata_content, + DB::CompressionMethod metadata_compression_method, + bool if_not_exists) const override; bool updateMetadata(const String & namespace_name, const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr new_snapshot) const override; @@ -79,7 +85,7 @@ class GlueCatalog final : public ICatalog, private DB::WithContext 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; + void dropTable(const String & namespace_name, const String & table_name, bool purge, bool if_exists) const override; /// Returns a callback that re-vends fresh AWS credentials from the configured /// credentials provider chain. Invoked by `ReadBufferFromS3` when an S3 call diff --git a/src/Databases/DataLake/ICatalog.cpp b/src/Databases/DataLake/ICatalog.cpp index 62cf44930225..1740f1ec8a56 100644 --- a/src/Databases/DataLake/ICatalog.cpp +++ b/src/Databases/DataLake/ICatalog.cpp @@ -81,6 +81,25 @@ StorageType parseStorageTypeFromString(const std::string & type) return *storage_type; } +std::string storageTypeToScheme(StorageType type) +{ + switch (type) + { + case StorageType::S3: + return "s3"; + case StorageType::Azure: + return "abfss"; + case StorageType::Local: + return "file"; + case StorageType::HDFS: + return "hdfs"; + case StorageType::Other: + throw DB::Exception( + DB::ErrorCodes::BAD_ARGUMENTS, + "Cannot determine URI scheme for storage type 'Other'"); + } +} + void TableMetadata::setLocation(const std::string & location_) { if (!with_location) @@ -335,9 +354,17 @@ DB::SettingsChanges CatalogSettings::allChanged() const return changes; } -void ICatalog::createTable(const String & /*namespace_name*/, const String & /*table_name*/, const String & /*new_metadata_path*/, Poco::JSON::Object::Ptr /*metadata_content*/) const +bool ICatalog::createTable( + const String & /*namespace_name*/, + const String & /*table_name*/, + const String & /*new_metadata_path*/, + Poco::JSON::Object::Ptr /*metadata_content*/, + DB::CompressionMethod /*metadata_compression_method*/, + bool /*if_not_exists*/) const { - throw DB::Exception(DB::ErrorCodes::NOT_IMPLEMENTED, "createTable is not implemented"); + throw DB::Exception(DB::ErrorCodes::NOT_IMPLEMENTED, + "CREATE TABLE is not supported for this DataLakeCatalog catalog type; " + "it is available only for Iceberg REST (including OneLake, BigLake, Delta Sharing) and Glue catalogs"); } bool ICatalog::updateMetadata(const String & /*namespace_name*/, const String & /*table_name*/, const String & /*new_metadata_path*/, Poco::JSON::Object::Ptr /*new_snapshot*/) const @@ -357,9 +384,11 @@ bool ICatalog::updateSchema( throw DB::Exception(DB::ErrorCodes::NOT_IMPLEMENTED, "updateSchema is not implemented"); } -void ICatalog::dropTable(const String & /*namespace_name*/, const String & /*table_name*/) const +void ICatalog::dropTable(const String & /*namespace_name*/, const String & /*table_name*/, bool /*purge*/, bool /*if_exists*/) const { - throw DB::Exception(DB::ErrorCodes::NOT_IMPLEMENTED, "dropTable is not implemented"); + throw DB::Exception(DB::ErrorCodes::NOT_IMPLEMENTED, + "DROP TABLE is not supported for this DataLakeCatalog catalog type; " + "it is available only for Iceberg REST (including OneLake, BigLake, Delta Sharing) and Glue catalogs"); } } diff --git a/src/Databases/DataLake/ICatalog.h b/src/Databases/DataLake/ICatalog.h index f77bfcff0405..ef7d046d0544 100644 --- a/src/Databases/DataLake/ICatalog.h +++ b/src/Databases/DataLake/ICatalog.h @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -25,6 +26,7 @@ namespace DataLake using StorageType = DB::DatabaseDataLakeStorageType; StorageType parseStorageTypeFromLocation(const std::string & location); StorageType parseStorageTypeFromString(const std::string &type); +std::string storageTypeToScheme(StorageType type); struct DataLakeSpecificProperties { @@ -189,8 +191,28 @@ class ICatalog /// E.g. one of S3, Azure, Local, HDFS. virtual std::optional getStorageType() const = 0; - /// Creates new table in catalog. - virtual void createTable(const String & namespace_name, const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr metadata_content) const; + /// Catalog-wide base location for new tables, e.g. `s3://warehouse/data`. Empty if unknown. + virtual String getDefaultBaseLocation() const { return ""; } + + /// Creates new table in catalog. Callers must ensure the namespace exists before + /// writing any table files to storage: a catalog that shares its storage view with + /// the data refuses to create a namespace over a plain directory those files create. + /// `metadata_compression_method` is the codec requested by `iceberg_metadata_compression_method`. It is + /// only meaningful for catalogs that write the initial metadata file themselves (i.e. when + /// `new_metadata_path` is empty): they must name the file `v1..metadata.json` and compress its + /// contents accordingly, exactly like `DB::Iceberg::IcebergMetadata::createInitial` does. + /// Returns `true` when this call created the table. Returns `false` only when `if_not_exists` is set + /// and the catalog answered that the table is already there - the catalog is shared, so another client + /// can win the name race after the caller checked that the table does not exist. Callers must not + /// report that as a creation: `CREATE TABLE IF NOT EXISTS ... AS SELECT` must not fill a table the + /// winner of the race created. + virtual bool createTable( + const String & namespace_name, + const String & table_name, + const String & new_metadata_path, + Poco::JSON::Object::Ptr metadata_content, + DB::CompressionMethod metadata_compression_method, + bool if_not_exists) const; /// Updates metadata in catalog. virtual bool updateMetadata(const String & namespace_name, const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr new_snapshot) const; @@ -212,7 +234,8 @@ class ICatalog Poco::JSON::Object::Ptr metadata = nullptr) const; /// Drop table from catalog. - virtual void dropTable(const String & namespace_name, const String & table_name) const; + /// If purge, the catalog is requested to also delete underlying data files. + virtual void dropTable(const String & namespace_name, const String & table_name, bool purge, bool if_exists) const; /// Does the catalog support transactions or anything like that? /// For example, the Iceberg REST catalog supports atomic operations "compare if snapshot X is equal to" and "add new snapshot Y". diff --git a/src/Databases/DataLake/RestCatalog.cpp b/src/Databases/DataLake/RestCatalog.cpp index 58bf459992f0..528762847e5c 100644 --- a/src/Databases/DataLake/RestCatalog.cpp +++ b/src/Databases/DataLake/RestCatalog.cpp @@ -176,6 +176,22 @@ String encodeNamespaceForURI(const String & namespace_name) return encoded; } +/// Per Iceberg REST spec, `namespace` is a JSON array of segments. Split `ns.a.b` on dots. +Poco::JSON::Array::Ptr namespaceToJSONArray(const String & namespace_name) +{ + Poco::JSON::Array::Ptr segments = new Poco::JSON::Array; + size_t start = 0; + while (start <= namespace_name.size()) + { + size_t dot = namespace_name.find('.', start); + if (dot == String::npos) + dot = namespace_name.size(); + segments->add(namespace_name.substr(start, dot - start)); + start = dot + 1; + } + return segments; +} + std::unordered_set getAllowedBigLakeMetadataServiceHosts( const Poco::Util::AbstractConfiguration & config) { @@ -191,7 +207,6 @@ std::unordered_set getAllowedBigLakeMetadataServiceHosts( return allowed; } - } namespace @@ -291,9 +306,7 @@ Poco::JSON::Object::Ptr buildUpdateSchemaRequestBody( { 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); + identifier->set("namespace", namespaceToJSONArray(namespace_name)); request_body->set("identifier", identifier); } @@ -372,9 +385,7 @@ Poco::JSON::Object::Ptr buildUpdateMetadataRequestBody( { 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); + identifier->set("namespace", namespaceToJSONArray(namespace_name)); request_body->set("identifier", identifier); } @@ -897,6 +908,11 @@ std::optional RestCatalog::getStorageType() const return parseStorageTypeFromLocation(config.default_base_location); } +String RestCatalog::getDefaultBaseLocation() const +{ + return config.default_base_location; +} + DB::ReadWriteBufferFromHTTPPtr RestCatalog::createReadBuffer( const std::string & endpoint, const Poco::URI::QueryParameters & params, @@ -1642,11 +1658,9 @@ void RestCatalog::createNamespaceIfNotExists(const String & namespace_name, cons const std::string endpoint = (base_url / config.prefix / NAMESPACES_ENDPOINT).generic_string(); Poco::JSON::Object::Ptr request_body = new Poco::JSON::Object; - { - Poco::JSON::Array::Ptr namespaces = new Poco::JSON::Array; - namespaces->add(namespace_name); - request_body->set("namespace", namespaces); - } + request_body->set("namespace", namespaceToJSONArray(namespace_name)); + + if (!location.empty()) // the caller has no namespace base to offer { Poco::JSON::Object::Ptr properties = new Poco::JSON::Object; properties->set("location", location); @@ -1665,19 +1679,29 @@ void RestCatalog::createNamespaceIfNotExists(const String & namespace_name, cons } } -void RestCatalog::createTable(const String & namespace_name, const String & table_name, const String & /*new_metadata_path*/, Poco::JSON::Object::Ptr metadata_content) const +/// `metadata_compression_method` is unused here: the REST server writes and names the initial metadata +/// file itself, we only send it the metadata content. +bool RestCatalog::createTable( + const String & namespace_name, + const String & table_name, + const String & /*new_metadata_path*/, + Poco::JSON::Object::Ptr metadata_content, + DB::CompressionMethod /*metadata_compression_method*/, + bool if_not_exists) const { if (!allowed_namespaces.isNamespaceAllowed(namespace_name, /*nested*/ false)) throw DB::Exception(DB::ErrorCodes::CATALOG_NAMESPACE_DISABLED, "Failed to create table {}, namespace {} is filtered by `namespaces` database parameter", table_name, namespace_name); - createNamespaceIfNotExists(namespace_name, metadata_content->getValue("location")); + const String location = metadata_content->getValue("location"); + + createNamespaceIfNotExists(namespace_name, location); const std::string endpoint = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables").generic_string(); Poco::JSON::Object::Ptr request_body = new Poco::JSON::Object; request_body->set("name", table_name); - request_body->set("location", metadata_content->getValue("location")); + request_body->set("location", location); { Poco::JSON::Object::Ptr initial_schema = metadata_content->getArray("schemas")->getObject(0); Poco::JSON::Array::Ptr identifier_fields = new Poco::JSON::Array; @@ -1686,13 +1710,21 @@ void RestCatalog::createTable(const String & namespace_name, const String & tabl } request_body->set("partition-spec", metadata_content->getArray("partition-specs")->get(0)); + if (metadata_content->has("sort-orders")) { - Poco::JSON::Object::Ptr write_order = new Poco::JSON::Object; - write_order->set("order-id", 0); - Poco::JSON::Array::Ptr fields = new Poco::JSON::Array; - write_order->set("fields", fields); - request_body->set("write-order", write_order); + if (auto sort_orders = metadata_content->getArray("sort-orders"); sort_orders->size() > 0) + { + auto sort_order = sort_orders->getObject(0); + auto fields = sort_order->getArray("fields"); + if (fields && fields->size() > 0) + { + if (sort_order->getValue("order-id") == 0) + sort_order->set("order-id", 1); + request_body->set("write-order", sort_order); + } + } } + request_body->set("stage-create", false); Poco::JSON::Object::Ptr properties = new Poco::JSON::Object; @@ -1709,8 +1741,13 @@ void RestCatalog::createTable(const String & namespace_name, const String & tabl } catch (const DB::HTTPException & ex) { + /// The catalog answers `409` when someone else created the table first. + if (if_not_exists && ex.getHTTPStatus() == Poco::Net::HTTPResponse::HTTPStatus::HTTP_CONFLICT) + return false; throw DB::Exception(DB::ErrorCodes::DATALAKE_DATABASE_ERROR, "Failed to create table {}", ex.displayText()); } + + return true; } @@ -1785,16 +1822,16 @@ bool RestCatalog::updateSchema( return true; } -void RestCatalog::dropTable(const String & namespace_name, const String & table_name) const +void RestCatalog::dropTable(const String & namespace_name, const String & table_name, bool purge, bool if_exists) const { if (!allowed_namespaces.isNamespaceAllowed(namespace_name, /*nested*/ false)) throw DB::Exception(DB::ErrorCodes::CATALOG_NAMESPACE_DISABLED, "Failed to drop table {}, namespace {} is filtered by `namespaces` database parameter", table_name, namespace_name); - const std::string endpoint - = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables" / table_name).generic_string() - + "?purgeRequested=False"; + const std::string base_endpoint + = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables" / table_name).generic_string(); + const std::string endpoint = fmt::format("{}?purgeRequested={}", base_endpoint, purge ? "true" : "false"); Poco::JSON::Object::Ptr request_body = nullptr; try @@ -1805,6 +1842,9 @@ void RestCatalog::dropTable(const String & namespace_name, const String & table_ } catch (const DB::HTTPException & ex) { + /// The catalog answers `404` when the table is already gone - someone else dropped it first. + if (if_exists && ex.getHTTPStatus() == Poco::Net::HTTPResponse::HTTPStatus::HTTP_NOT_FOUND) + return; throw DB::Exception(DB::ErrorCodes::DATALAKE_DATABASE_ERROR, "Failed to drop table {}", ex.displayText()); } } diff --git a/src/Databases/DataLake/RestCatalog.h b/src/Databases/DataLake/RestCatalog.h index 4eb33d1045ab..0ad6ce356fc0 100644 --- a/src/Databases/DataLake/RestCatalog.h +++ b/src/Databases/DataLake/RestCatalog.h @@ -82,12 +82,20 @@ class RestCatalog : public ICatalog, public DB::WithContext std::optional getStorageType() const override; + String getDefaultBaseLocation() const override; + DB::DatabaseDataLakeCatalogType getCatalogType() const override { return DB::DatabaseDataLakeCatalogType::ICEBERG_REST; } - void createTable(const String & namespace_name, const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr metadata_content) const override; + bool createTable( + const String & namespace_name, + const String & table_name, + const String & new_metadata_path, + Poco::JSON::Object::Ptr metadata_content, + DB::CompressionMethod metadata_compression_method, + bool if_not_exists) const override; bool updateMetadata(const String & namespace_name, const String & table_name, const String & new_metadata_path, Poco::JSON::Object::Ptr new_snapshot) const override; @@ -102,7 +110,7 @@ class RestCatalog : public ICatalog, public DB::WithContext bool isTransactional() const override { return true; } - void dropTable(const String & namespace_name, const String & table_name) const override; + void dropTable(const String & namespace_name, const String & table_name, bool purge, bool if_exists) const override; ICatalog::CredentialsRefreshCallback getCredentialsConfigurationCallback(const DB::StorageID & storage_id) override; diff --git a/src/Databases/DataLake/S3TablesCatalog.cpp b/src/Databases/DataLake/S3TablesCatalog.cpp index 07cd7e723da3..c1517977ede6 100644 --- a/src/Databases/DataLake/S3TablesCatalog.cpp +++ b/src/Databases/DataLake/S3TablesCatalog.cpp @@ -214,7 +214,8 @@ ICatalog::CredentialsRefreshCallback S3TablesCatalog::getCredentialsConfiguratio }; } -void S3TablesCatalog::dropTable(const String & namespace_name, const String & table_name) const +void S3TablesCatalog::dropTable( + const String & namespace_name, const String & table_name, bool /*delete_data*/, bool /*if_exists*/) const { const std::string endpoint = (base_url / config.prefix / "namespaces" / namespace_name / "tables" / table_name).string() diff --git a/src/Databases/DataLake/S3TablesCatalog.h b/src/Databases/DataLake/S3TablesCatalog.h index aff432c1b679..251253f72bc7 100644 --- a/src/Databases/DataLake/S3TablesCatalog.h +++ b/src/Databases/DataLake/S3TablesCatalog.h @@ -41,7 +41,7 @@ class S3TablesCatalog final : public RestCatalog DB::ContextPtr context_, TableMetadata & result) const override; - void dropTable(const String & namespace_name, const String & table_name) const override; + void dropTable(const String & namespace_name, const String & table_name, bool delete_data, bool if_exists) const override; ICatalog::CredentialsRefreshCallback getCredentialsConfigurationCallback(const DB::StorageID & storage_id) override; diff --git a/src/Databases/DataLake/tests/gtest_construct_table_location.cpp b/src/Databases/DataLake/tests/gtest_construct_table_location.cpp new file mode 100644 index 000000000000..c32b32ebfc36 --- /dev/null +++ b/src/Databases/DataLake/tests/gtest_construct_table_location.cpp @@ -0,0 +1,136 @@ +#include +#include + +#include + +#include +#include + +namespace DataLake::Test +{ + +class ConstructTableLocationTest : public ::testing::Test +{ +}; + +/// S3: bucket is the first path segment of the HTTPS-style storage_endpoint; +/// a sub-prefix after the bucket must be preserved. +TEST_F(ConstructTableLocationTest, S3HttpsEndpoint) +{ + EXPECT_EQ( + constructTableLocation("s3", "http://minio:9000/warehouse-rest", "ns", "tbl"), + "s3://warehouse-rest/ns/tbl"); + EXPECT_EQ( + constructTableLocation("s3", "http://minio:9000/warehouse/data", "ns", "tbl"), + "s3://warehouse/data/ns/tbl"); +} + +TEST_F(ConstructTableLocationTest, S3RejectsEndpointWithoutBucket) +{ + EXPECT_THROW( + constructTableLocation("s3", "http://minio:9000/", "ns", "tbl"), + DB::Exception); +} + +/// Virtual-hosted style cannot derive a bucket from the endpoint unambiguously (a service host, an IP host, +/// and a dotted bucket name are indistinguishable), so CREATE TABLE rejects it regardless of endpoint shape; +/// users must set `default_base_location`. +TEST_F(ConstructTableLocationTest, S3VirtualHostedIsRejected) +{ + for (const auto * endpoint : { + "https://warehouse-rest.minio.example.com", + "https://warehouse-rest.minio.example.com/prefix", + "https://s3.us-east-1.amazonaws.com", + "https://10.0.0.5:9000", + }) + EXPECT_THROW( + constructTableLocation("s3", endpoint, "ns", "tbl", DB::S3UriStyle::VIRTUAL_HOSTED), + DB::Exception); +} + +/// Azure: HTTPS-form storage_endpoint must round-trip through `setLocation`, +/// which means the constructed URI must include the `@` authority. +/// A sub-path after the container must be preserved, a trailing slash ignored. +TEST_F(ConstructTableLocationTest, AzureHttpsEndpoint) +{ + const String location = constructTableLocation( + "abfss", + "https://account.dfs.core.windows.net/mycontainer", + "ns", + "tbl"); + EXPECT_EQ(location, "abfss://mycontainer@account.dfs.core.windows.net/ns/tbl"); + + /// Verify the produced URI parses back into the expected components. + TableMetadata metadata; + metadata.withLocation(); + metadata.setLocation(location); + EXPECT_EQ(metadata.getLocation(), location); + EXPECT_EQ(metadata.getStorageType(), StorageType::Azure); + + EXPECT_EQ( + constructTableLocation( + "abfss", + "https://account.dfs.core.windows.net/mycontainer/warehouse/data", + "ns", + "tbl"), + "abfss://mycontainer@account.dfs.core.windows.net/warehouse/data/ns/tbl"); + EXPECT_EQ( + constructTableLocation( + "abfss", + "https://account.dfs.core.windows.net/mycontainer/", + "ns", + "tbl"), + "abfss://mycontainer@account.dfs.core.windows.net/ns/tbl"); +} + +/// Azure: ABFSS-form storage_endpoint (container in the authority's user-info) +/// is also accepted, with and without a sub-path. +TEST_F(ConstructTableLocationTest, AzureAbfssEndpoint) +{ + EXPECT_EQ( + constructTableLocation( + "abfss", + "abfss://mycontainer@account.dfs.core.windows.net/", + "ns", + "tbl"), + "abfss://mycontainer@account.dfs.core.windows.net/ns/tbl"); + EXPECT_EQ( + constructTableLocation( + "abfss", + "abfss://mycontainer@account.dfs.core.windows.net/warehouse/data", + "ns", + "tbl"), + "abfss://mycontainer@account.dfs.core.windows.net/warehouse/data/ns/tbl"); +} + +TEST_F(ConstructTableLocationTest, AzureRejectsEndpointWithoutContainer) +{ + EXPECT_THROW( + constructTableLocation("abfss", "https://account.dfs.core.windows.net/", "ns", "tbl"), + DB::Exception); + EXPECT_THROW( + constructTableLocation("abfss", "abfss://account.dfs.core.windows.net/", "ns", "tbl"), + DB::Exception); +} + +/// HDFS: the authority (host:port) must be preserved in the location URI, +/// with or without a warehouse path after it. +TEST_F(ConstructTableLocationTest, HdfsPreservesAuthority) +{ + EXPECT_EQ( + constructTableLocation("hdfs", "hdfs://namenode:9000/warehouse", "ns", "tbl"), + "hdfs://namenode:9000/warehouse/ns/tbl"); + EXPECT_EQ( + constructTableLocation("hdfs", "hdfs://namenode:9000", "ns", "tbl"), + "hdfs://namenode:9000/ns/tbl"); +} + +/// `file://` URIs have an empty authority and just a local filesystem path. +TEST_F(ConstructTableLocationTest, FileWithoutAuthority) +{ + EXPECT_EQ( + constructTableLocation("file", "file:///var/iceberg/warehouse", "ns", "tbl"), + "file:///var/iceberg/warehouse/ns/tbl"); +} + +} diff --git a/src/Databases/DatabaseAtomic.cpp b/src/Databases/DatabaseAtomic.cpp index 1e21692c2b03..9bf815f58bfa 100644 --- a/src/Databases/DatabaseAtomic.cpp +++ b/src/Databases/DatabaseAtomic.cpp @@ -189,7 +189,7 @@ StoragePtr DatabaseAtomic::detachTable(ContextPtr /* context */, const String & return detached_table; } -void DatabaseAtomic::dropTable(ContextPtr local_context, const String & table_name, bool sync) +void DatabaseAtomic::dropTable(ContextPtr local_context, const String & table_name, bool sync, bool /*if_exists*/) { auto component_guard = Coordination::setCurrentComponent("DatabaseAtomic::dropTable"); waitDatabaseStarted(); diff --git a/src/Databases/DatabaseAtomic.h b/src/Databases/DatabaseAtomic.h index f1626a66087f..a6d4374249d7 100644 --- a/src/Databases/DatabaseAtomic.h +++ b/src/Databases/DatabaseAtomic.h @@ -48,7 +48,7 @@ class DatabaseAtomic : public DatabaseOrdinary bool exchange, bool dictionary) override; - void dropTable(ContextPtr context, const String & table_name, bool sync) override; + void dropTable(ContextPtr context, const String & table_name, bool sync, bool if_exists) override; void dropTableImpl(ContextPtr context, const String & table_name, bool sync); void attachTable(ContextPtr context, const String & name, const StoragePtr & table, const String & relative_table_path) override; diff --git a/src/Databases/DatabaseBackup.cpp b/src/Databases/DatabaseBackup.cpp index a60fb82f166a..77585b6586d7 100644 --- a/src/Databases/DatabaseBackup.cpp +++ b/src/Databases/DatabaseBackup.cpp @@ -187,6 +187,7 @@ void DatabaseBackup::detachTablePermanently(ContextPtr, const String &) void DatabaseBackup::dropTable( ContextPtr, const String &, + bool, bool) { throw Exception(ErrorCodes::UNSUPPORTED_METHOD, "DROP TABLE is not supported for Backup database"); diff --git a/src/Databases/DatabaseBackup.h b/src/Databases/DatabaseBackup.h index 180de68f4037..5abf0aa306e7 100644 --- a/src/Databases/DatabaseBackup.h +++ b/src/Databases/DatabaseBackup.h @@ -45,7 +45,8 @@ class DatabaseBackup final : public DatabaseOrdinary void dropTable( ContextPtr context, const String & table_name, - bool sync) override; + bool sync, + bool if_exists) override; void renameTable( ContextPtr context, diff --git a/src/Databases/DatabaseMemory.cpp b/src/Databases/DatabaseMemory.cpp index 8dd9741c9aeb..bd10a02604a8 100644 --- a/src/Databases/DatabaseMemory.cpp +++ b/src/Databases/DatabaseMemory.cpp @@ -59,7 +59,8 @@ void DatabaseMemory::createTable( void DatabaseMemory::dropTable( ContextPtr /*context*/, const String & table_name, - bool /*sync*/) + bool /*sync*/, + bool /*if_exists*/) { StoragePtr table; { diff --git a/src/Databases/DatabaseMemory.h b/src/Databases/DatabaseMemory.h index 2e7d313c864b..219cf1bd09f3 100644 --- a/src/Databases/DatabaseMemory.h +++ b/src/Databases/DatabaseMemory.h @@ -32,7 +32,8 @@ class DatabaseMemory final : public DatabaseWithOwnTablesBase void dropTable( ContextPtr context, const String & table_name, - bool sync) override; + bool sync, + bool if_exists) override; ASTPtr getCreateTableQueryImpl(const String & name, ContextPtr context, bool throw_on_error) const override; diff --git a/src/Databases/DatabaseOnDisk.cpp b/src/Databases/DatabaseOnDisk.cpp index 89480f6bbece..731a4971a380 100644 --- a/src/Databases/DatabaseOnDisk.cpp +++ b/src/Databases/DatabaseOnDisk.cpp @@ -353,7 +353,7 @@ void DatabaseOnDisk::detachTablePermanently(ContextPtr query_context, const Stri } } -void DatabaseOnDisk::dropTable(ContextPtr local_context, const String & table_name, bool /*sync*/) +void DatabaseOnDisk::dropTable(ContextPtr local_context, const String & table_name, bool /*sync*/, bool /*if_exists*/) { auto component_guard = Coordination::setCurrentComponent("DatabaseOnDisk::dropTable"); waitDatabaseStarted(); diff --git a/src/Databases/DatabaseOnDisk.h b/src/Databases/DatabaseOnDisk.h index 4260c0d82730..bd4070a816a7 100644 --- a/src/Databases/DatabaseOnDisk.h +++ b/src/Databases/DatabaseOnDisk.h @@ -48,7 +48,8 @@ class DatabaseOnDisk : public DatabaseWithOwnTablesBase void dropTable( ContextPtr context, const String & table_name, - bool sync) override; + bool sync, + bool if_exists) override; void renameTable( ContextPtr context, diff --git a/src/Databases/DatabaseOverlay.cpp b/src/Databases/DatabaseOverlay.cpp index b8d861a079c1..cb515b5481cd 100644 --- a/src/Databases/DatabaseOverlay.cpp +++ b/src/Databases/DatabaseOverlay.cpp @@ -72,13 +72,13 @@ void DatabaseOverlay::createTable(ContextPtr context_, const String & table_name getEngineName()); } -void DatabaseOverlay::dropTable(ContextPtr context_, const String & table_name, bool sync) +void DatabaseOverlay::dropTable(ContextPtr context_, const String & table_name, bool sync, bool if_exists) { for (auto & db : databases) { if (db->isTableExist(table_name, context_)) { - db->dropTable(context_, table_name, sync); + db->dropTable(context_, table_name, sync, if_exists); return; } } diff --git a/src/Databases/DatabaseOverlay.h b/src/Databases/DatabaseOverlay.h index 72cb8d71b322..74a710ab1d14 100644 --- a/src/Databases/DatabaseOverlay.h +++ b/src/Databases/DatabaseOverlay.h @@ -29,7 +29,7 @@ class DatabaseOverlay : public IDatabase, protected WithContext void createTable(ContextPtr context, const String & table_name, const StoragePtr & table, const ASTPtr & query) override; - void dropTable(ContextPtr context, const String & table_name, bool sync) override; + void dropTable(ContextPtr context, const String & table_name, bool sync, bool if_exists) override; void attachTable(ContextPtr context, const String & table_name, const StoragePtr & table, const String & relative_table_path) override; diff --git a/src/Databases/DatabaseReplicated.cpp b/src/Databases/DatabaseReplicated.cpp index 27af8b4126be..94d4f393e9fa 100644 --- a/src/Databases/DatabaseReplicated.cpp +++ b/src/Databases/DatabaseReplicated.cpp @@ -2198,7 +2198,7 @@ void DatabaseReplicated::shutdown() DatabaseAtomic::shutdown(); } -void DatabaseReplicated::dropTable(ContextPtr local_context, const String & table_name, bool sync) +void DatabaseReplicated::dropTable(ContextPtr local_context, const String & table_name, bool sync, bool /*if_exists*/) { auto component_guard = Coordination::setCurrentComponent("DatabaseReplicated::dropTable"); waitDatabaseStarted(); diff --git a/src/Databases/DatabaseReplicated.h b/src/Databases/DatabaseReplicated.h index a2934f9a6586..7a5b30b4ed9a 100644 --- a/src/Databases/DatabaseReplicated.h +++ b/src/Databases/DatabaseReplicated.h @@ -79,7 +79,7 @@ class DatabaseReplicated : public DatabaseAtomic String getEngineName() const override { return "Replicated"; } /// If current query is initial, then the following methods add metadata updating ZooKeeper operations to current ZooKeeperMetadataTransaction. - void dropTable(ContextPtr, const String & table_name, bool sync) override; + void dropTable(ContextPtr, const String & table_name, bool sync, bool if_exists) override; void renameTable(ContextPtr context, const String & table_name, IDatabase & to_database, const String & to_table_name, bool exchange, bool dictionary) override; void detachTablePermanently(ContextPtr context, const String & table_name) override; diff --git a/src/Databases/IDatabase.cpp b/src/Databases/IDatabase.cpp index 2e62449b6176..7ae3437df500 100644 --- a/src/Databases/IDatabase.cpp +++ b/src/Databases/IDatabase.cpp @@ -144,7 +144,8 @@ void IDatabase::createTable( void IDatabase::dropTable( /// NOLINT ContextPtr /*context*/, const String & /*name*/, - [[maybe_unused]] bool sync) + [[maybe_unused]] bool sync, + [[maybe_unused]] bool if_exists) { throw Exception(ErrorCodes::NOT_IMPLEMENTED, "There is no DROP TABLE query for Database{}", getEngineName()); } diff --git a/src/Databases/IDatabase.h b/src/Databases/IDatabase.h index b92e15ac35d3..dc35d5f6f7e7 100644 --- a/src/Databases/IDatabase.h +++ b/src/Databases/IDatabase.h @@ -189,6 +189,11 @@ class IDatabase : public std::enable_shared_from_this virtual bool isDatalakeCatalog() const { return false; } + /// Reject an explicitly-specified table engine that is incompatible with this database, before the + /// table is created. A datalake catalog with a fixed storage backend rejects an Iceberg engine that + /// pins a different backend. Default: no-op. + virtual void validateCreateTableEngine(const String & /*engine_name*/) const {} + /// True for databases such as `MySQL`/`PostgreSQL` whose table list lives on a remote service. /// This is distinct from `isExternal`, which classifies whether the engine supports ClickHouse internal table types. virtual bool isRemoteDatabase() const { return false; } @@ -320,10 +325,12 @@ class IDatabase : public std::enable_shared_from_this const ASTPtr & /*query*/); /// Delete the table from the database, drop table and delete the metadata. + /// `if_exists` is the flag of the originating query. virtual void dropTable( /// NOLINT ContextPtr /*context*/, const String & /*name*/, - [[maybe_unused]] bool sync = false); + [[maybe_unused]] bool sync = false, + [[maybe_unused]] bool if_exists = false); /// Add a table to the database, but do not add it to the metadata. The database may not support this method. /// diff --git a/src/Databases/MySQL/DatabaseMySQL.cpp b/src/Databases/MySQL/DatabaseMySQL.cpp index a9494fa2abd3..67ff8eb04f15 100644 --- a/src/Databases/MySQL/DatabaseMySQL.cpp +++ b/src/Databases/MySQL/DatabaseMySQL.cpp @@ -533,7 +533,7 @@ void DatabaseMySQL::detachTablePermanently(ContextPtr, const String & table_name table_iter->second.second->is_detached = true; } -void DatabaseMySQL::dropTable(ContextPtr local_context, const String & table_name, bool /*sync*/) +void DatabaseMySQL::dropTable(ContextPtr local_context, const String & table_name, bool /*sync*/, bool /*if_exists*/) { if (!persistent) throw Exception(ErrorCodes::NOT_IMPLEMENTED, "DROP TABLE is not supported for non-persistent MySQL database"); diff --git a/src/Databases/MySQL/DatabaseMySQL.h b/src/Databases/MySQL/DatabaseMySQL.h index 5ac2e10474d4..7fe80687b58a 100644 --- a/src/Databases/MySQL/DatabaseMySQL.h +++ b/src/Databases/MySQL/DatabaseMySQL.h @@ -79,7 +79,7 @@ class DatabaseMySQL final : public DatabaseWithAltersOnDiskBase, WithContext void detachTablePermanently(ContextPtr context, const String & table_name) override; - void dropTable(ContextPtr context, const String & table_name, bool sync) override; + void dropTable(ContextPtr context, const String & table_name, bool sync, bool if_exists) override; void attachTable(ContextPtr context, const String & table_name, const StoragePtr & storage, const String & relative_table_path) override; diff --git a/src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp b/src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp index b17234d69570..89061ead199a 100644 --- a/src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp +++ b/src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.cpp @@ -391,7 +391,7 @@ void DatabaseMaterializedPostgreSQL::attachTable(ContextPtr context_, const Stri catch (...) { /// This is a failed attach table. Remove already created nested table. - DatabaseAtomic::dropTable(current_context, table_name, true); + DatabaseAtomic::dropTable(current_context, table_name, true, /* if_exists */ false); throw; } } @@ -439,7 +439,7 @@ void DatabaseMaterializedPostgreSQL::detachTablePermanently(ContextPtr, const St { auto current_context = Context::createCopy(getContext()->getGlobalContext()); current_context->makeQueryContext(); - DatabaseAtomic::dropTable(current_context, table_name, true); + DatabaseAtomic::dropTable(current_context, table_name, true, /* if_exists */ false); } catch (Exception & e) { @@ -480,10 +480,10 @@ void DatabaseMaterializedPostgreSQL::stopReplication() } -void DatabaseMaterializedPostgreSQL::dropTable(ContextPtr local_context, const String & table_name, bool sync) +void DatabaseMaterializedPostgreSQL::dropTable(ContextPtr local_context, const String & table_name, bool sync, bool if_exists) { /// Modify context into nested_context and pass query to Atomic database. - DatabaseAtomic::dropTable(StorageMaterializedPostgreSQL::makeNestedTableContext(local_context), table_name, sync); + DatabaseAtomic::dropTable(StorageMaterializedPostgreSQL::makeNestedTableContext(local_context), table_name, sync, if_exists); } diff --git a/src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.h b/src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.h index d1963b2ec8a6..239914dc584c 100644 --- a/src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.h +++ b/src/Databases/PostgreSQL/DatabaseMaterializedPostgreSQL.h @@ -59,7 +59,7 @@ class DatabaseMaterializedPostgreSQL : public DatabaseAtomic StoragePtr detachTable(ContextPtr context, const String & table_name) override; - void dropTable(ContextPtr local_context, const String & name, bool sync) override; + void dropTable(ContextPtr local_context, const String & name, bool sync, bool if_exists) override; void drop(ContextPtr local_context) override; diff --git a/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp b/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp index e30586e34f5e..6d85ee6f7475 100644 --- a/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp +++ b/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp @@ -296,7 +296,7 @@ void DatabasePostgreSQL::createTable(ContextPtr local_context, const String & ta } -void DatabasePostgreSQL::dropTable(ContextPtr, const String & table_name, bool /* sync */) +void DatabasePostgreSQL::dropTable(ContextPtr, const String & table_name, bool /* sync */, bool /* if_exists */) { if (!persistent) throw Exception(ErrorCodes::NOT_IMPLEMENTED, "DROP TABLE is not supported for non-persistent MySQL database"); diff --git a/src/Databases/PostgreSQL/DatabasePostgreSQL.h b/src/Databases/PostgreSQL/DatabasePostgreSQL.h index 59d49e37940a..fb218017278f 100644 --- a/src/Databases/PostgreSQL/DatabasePostgreSQL.h +++ b/src/Databases/PostgreSQL/DatabasePostgreSQL.h @@ -54,7 +54,7 @@ class DatabasePostgreSQL final : public DatabaseWithAltersOnDiskBase, WithContex StoragePtr tryGetTable(const String & name, ContextPtr context) const override; void createTable(ContextPtr, const String & table_name, const StoragePtr & storage, const ASTPtr & create_query) override; - void dropTable(ContextPtr, const String & table_name, bool sync) override; + void dropTable(ContextPtr, const String & table_name, bool sync, bool if_exists) override; void attachTable(ContextPtr context, const String & table_name, const StoragePtr & storage, const String & relative_table_path) override; StoragePtr detachTable(ContextPtr context, const String & table_name) override; diff --git a/src/Interpreters/DDLWorker.cpp b/src/Interpreters/DDLWorker.cpp index 81355067db4a..064f0774ccac 100644 --- a/src/Interpreters/DDLWorker.cpp +++ b/src/Interpreters/DDLWorker.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -572,6 +573,9 @@ bool DDLWorker::tryExecuteQuery(DDLTaskBase & task, const ZooKeeperPtr & zookeep try { + /// Worker-side guard + checkQueryDatabasesSupportOnClusterDDL(task.query, context); + auto query_context = task.makeQueryContext(context, zookeeper); chassert(!query_context->getCurrentTransaction()); diff --git a/src/Interpreters/InterpreterAlterQuery.cpp b/src/Interpreters/InterpreterAlterQuery.cpp index 0c3eb06684fe..01df31077d01 100644 --- a/src/Interpreters/InterpreterAlterQuery.cpp +++ b/src/Interpreters/InterpreterAlterQuery.cpp @@ -421,6 +421,9 @@ BlockIO InterpreterAlterQuery::executeToTable(const ASTAlterQuery & alter) if (table && table->as()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Mutations with ON CLUSTER are not allowed for KeeperMap tables"); + if (table_id) + checkDatabaseSupportsOnClusterDDL(DatabaseCatalog::instance().tryGetDatabase(table_id.database_name)); + DDLQueryOnClusterParams params; params.access_to_check = getRequiredAccess(); return executeDDLQueryOnCluster(query_ptr, getContext(), params); diff --git a/src/Interpreters/InterpreterCreateQuery.cpp b/src/Interpreters/InterpreterCreateQuery.cpp index dcd3f3ea2674..e8be9016c2d4 100644 --- a/src/Interpreters/InterpreterCreateQuery.cpp +++ b/src/Interpreters/InterpreterCreateQuery.cpp @@ -36,6 +36,7 @@ #include #include #include +#include #include #include #include @@ -1766,6 +1767,31 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create) if (!UserDefinedSQLFunctionFactory::instance().empty()) UserDefinedSQLFunctionVisitor::visit(query_ptr, getContext()); + /// Snapshot whether the user wrote an explicit ENGINE before `setEngine` fills in a default below. + /// Used to detect an engine-less `CREATE TABLE` on a `DataLakeCatalog` database. + const bool engine_user_specified = create.storage && create.storage->engine; + + /// Capture explicit unsupported storage clauses before `setEngine` merges the source's clauses for + /// `CREATE TABLE ... AS` and makes explicit and inherited indistinguishable; rejected in the check below. + /// The clauses are unsupported on both DataLakeCatalog create paths (engine-less and explicit + /// `ENGINE = Iceberg*`). Engine SETTINGS are meaningful only with an explicit engine (they are real + /// data-lake storage settings, e.g. `iceberg_format_version`), so they are rejected only on the + /// engine-less path where they would be silently dropped. + const char * datalake_unsupported_storage_clause = nullptr; + if (create.storage) + { + if (create.storage->primary_key) + datalake_unsupported_storage_clause = "PRIMARY KEY"; + else if (create.storage->sample_by) + datalake_unsupported_storage_clause = "SAMPLE BY"; + else if (create.storage->ttl_table) + datalake_unsupported_storage_clause = "TTL"; + else if (create.storage->unique_key) + datalake_unsupported_storage_clause = "UNIQUE KEY"; + else if (create.storage->settings && !engine_user_specified) + datalake_unsupported_storage_clause = "engine SETTINGS"; + } + /// Set and retrieve list of columns, indices and constraints. Set table engine if needed. Rewrite query in canonical way. TableProperties properties = getTablePropertiesAndNormalizeCreateQuery(create, mode); @@ -1838,6 +1864,7 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create) if (!create.cluster.empty()) { + checkDatabaseSupportsOnClusterDDL(database); chassert(!ddl_guard); return executeQueryOnCluster(create); } @@ -1845,6 +1872,78 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create) if (need_add_to_database && !database) throw Exception(ErrorCodes::UNKNOWN_DATABASE, "Database {} does not exist", backQuoteIfNeed(database_name)); + if (database && database->isDatalakeCatalog()) + { + if (create.is_ordinary_view || create.is_materialized_view + || create.is_dictionary || create.attach || create.is_clone_as + || create.replace_table || create.replace_view) + throw Exception(ErrorCodes::NOT_IMPLEMENTED, + "DataLakeCatalog supports only plain CREATE TABLE; " + "views, dictionaries, ATTACH, CLONE AS, and REPLACE TABLE are not allowed"); + + if (engine_user_specified) + { + if (!create.storage->engine->name.starts_with("Iceberg")) + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "DataLakeCatalog only supports Iceberg-family table engines; got '{}'", + create.storage->engine->name); + + database->validateCreateTableEngine(create.storage->engine->name); + } + + /// For `CREATE TABLE ... AS` the storage is later rebuilt to keep only PARTITION BY / ORDER BY, so + /// these explicit clauses (captured before `setEngine`) can only be rejected here. + if (datalake_unsupported_storage_clause) + { + if (engine_user_specified) + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "DataLakeCatalog CREATE TABLE with an explicit table engine supports only " + "PARTITION BY, ORDER BY, and engine SETTINGS; " + "PRIMARY KEY, SAMPLE BY, TTL, and UNIQUE KEY are not supported " + "(got {})", datalake_unsupported_storage_clause); + + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "DataLakeCatalog CREATE TABLE supports only PARTITION BY and ORDER BY; " + "PRIMARY KEY, SAMPLE BY, TTL, UNIQUE KEY, and engine SETTINGS are not supported " + "(got {})", datalake_unsupported_storage_clause); + } + + /// Both create paths persist only column names/types (plus PARTITION BY / ORDER BY) into the + /// initial Iceberg metadata, and the table is re-instantiated from the catalog on every access, + /// so any other column property or secondary structure would be silently lost. Reject them here, + /// before any catalog or storage work; `properties` also covers columns inherited via + /// `CREATE TABLE ... AS`. `DatabaseDataLake::createTable` re-validates the engine-less path + /// at the AST level. + for (const auto & column : properties.columns) + { + if (column.default_desc.expression || column.default_desc.kind != ColumnDefaultKind::Default) + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "Column '{}': {} is not yet supported by DataLakeCatalog table creation", + column.name, toString(column.default_desc.kind)); + + /// Implicit auto-statistics on a `CREATE TABLE ... AS` source do not count as an explicit + /// column property (same distinction as `formatColumns`). + if (!column.comment.empty() || column.codec || column.ttl + || !column.settings.empty() || column.statistics.hasExplicitStatistics()) + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "Column '{}': COMMENT, CODEC, TTL, STATISTICS, SETTINGS, and PRIMARY KEY " + "are not supported by DataLakeCatalog table creation", + column.name); + } + + if (!properties.indices.empty() || !properties.constraints.empty() || !properties.projections.empty() + || (create.columns_list && (create.columns_list->primary_key || create.columns_list->primary_key_from_columns))) + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "DataLakeCatalog CREATE TABLE does not support PRIMARY KEY, indices, constraints, or projections"); + + /// The comment is not persisted in Iceberg metadata or catalog properties, + /// so reject it instead of silently dropping it. + if (create.comment) + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "Table COMMENT is not supported by DataLakeCatalog table creation " + "(note: CREATE TABLE ... AS inherits the comment from the source table)"); + } + if (create.isTemporary() && create.replace_table) { chassert(!ddl_guard); @@ -1859,7 +1958,23 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create) } /// Actually creates table - bool created = doCreateTable(create, properties, ddl_guard, mode); + bool created = false; + try + { + created = doCreateTable(create, properties, ddl_guard, mode, engine_user_specified); + } + catch (const Exception & e) + { + /// A `DataLakeCatalog` is shared with other clients and nodes, so the existence check inside + /// `doCreateTable` can be stale by the time the create reaches the catalog. Both create paths + /// (engine-less and explicit `ENGINE = Iceberg*`) report that lost race as `TABLE_ALREADY_EXISTS` + /// and leave nothing behind, so `IF NOT EXISTS` means here what it means for the local check: the + /// query created nothing, and `CREATE TABLE IF NOT EXISTS ... AS SELECT` must not fill the table + /// the winner of the race created. + if (!create.if_not_exists || e.code() != ErrorCodes::TABLE_ALREADY_EXISTS || !database || !database->isDatalakeCatalog()) + throw; + created = false; + } ddl_guard.reset(); if (!created) /// Table already exists @@ -1937,7 +2052,7 @@ catch (...) bool InterpreterCreateQuery::doCreateTable(ASTCreateQuery & create, const InterpreterCreateQuery::TableProperties & properties, - DDLGuardPtr & ddl_guard, LoadingStrictnessLevel mode) + DDLGuardPtr & ddl_guard, LoadingStrictnessLevel mode, bool engine_user_specified) { if (create.isTemporary()) { @@ -2034,6 +2149,59 @@ bool InterpreterCreateQuery::doCreateTable(ASTCreateQuery & create, database->checkTableNameLength(create.getTable()); } + auto & create_query = query_ptr->as(); + if (database->isDatalakeCatalog() && !engine_user_specified) + { + /// Extract partition/order from the source table if CREATE TABLE ... AS was used. + if (!as_table_saved.empty()) + { + String as_database_name = getContext()->resolveDatabase(as_database_saved); + StoragePtr as_storage = DatabaseCatalog::instance().getTable({as_database_name, as_table_saved}, getContext()); + auto as_storage_metadata = as_storage->getInMemoryMetadataPtr(getContext(), false); + + /// `setEngine` merged the source's engine, SETTINGS and clauses into `create_query.storage`; none + /// apply to a `DataLakeCatalog` table, so rebuild it keeping only the partition and sorting keys + /// (explicit ones take precedence, the source's are the fallback). Unsupported explicit clauses + /// were already rejected in `createTable`. + ASTPtr partition_by; + ASTPtr order_by; + if (create_query.storage) + { + if (create_query.storage->partition_by) + partition_by = create_query.storage->partition_by->clone(); + if (create_query.storage->order_by) + order_by = create_query.storage->order_by->clone(); + } + + if (!partition_by && as_storage_metadata->isPartitionKeyDefined() && as_storage_metadata->hasPartitionKey()) + partition_by = as_storage_metadata->getPartitionKeyAST()->clone(); + if (!order_by && as_storage_metadata->isSortingKeyDefined() && as_storage_metadata->hasSortingKey()) + order_by = as_storage_metadata->getSortingKeyAST()->clone(); + + auto storage_ast = make_intrusive(); + create_query.set(create_query.storage, storage_ast); + if (partition_by) + create_query.storage->set(create_query.storage->partition_by, partition_by); + if (order_by) + create_query.storage->set(create_query.storage->order_by, order_by); + } + + /// Ensure columns are in the query AST (mainly for `CREATE TABLE ... AS source`). `formatColumns` keeps + /// every column modifier (COMMENT, CODEC, TTL, SETTINGS, STATISTICS, defaults) so `DatabaseDataLake` + /// rejects the unsupported ones rather than silently dropping them. + if (!create_query.columns_list + || !create_query.columns_list->columns + || create_query.columns_list->columns->children.empty()) + { + auto columns_declare_list = make_intrusive(); + columns_declare_list->set(columns_declare_list->columns, formatColumns(properties.columns)); + create_query.set(create_query.columns_list, columns_declare_list); + } + + database->createTable(getContext(), create.getTable(), nullptr, query_ptr); + return true; + } + data_path = database->getTableDataPath(create); // When creating a table, when checking if the data path exists, it should use the local disk to check, not the database disk. Because the database disk stores metadata files only. auto full_data_path = fs::path{getContext()->getPath()} / data_path; @@ -2146,7 +2314,6 @@ bool InterpreterCreateQuery::doCreateTable(ASTCreateQuery & create, is_restore_from_backup); /// If schema was inferred while storage creation, add columns description to create query. - auto & create_query = query_ptr->as(); addColumnsDescriptionToCreateQueryIfNecessary(create_query, res); /// Add any inferred engine args if needed. For example, data format for engines File/S3/URL/etc if (auto * engine_args = getEngineArgsFromCreateQuery(create_query)) @@ -2327,7 +2494,7 @@ BlockIO InterpreterCreateQuery::doCreateOrReplaceTable(ASTCreateQuery & create, { /// Create temporary table (random name will be generated) DDLGuardPtr ddl_guard; - [[maybe_unused]] bool done = InterpreterCreateQuery(query_ptr, create_context).doCreateTable(create, properties, ddl_guard, mode); + [[maybe_unused]] bool done = InterpreterCreateQuery(query_ptr, create_context).doCreateTable(create, properties, ddl_guard, mode, /*engine_user_specified=*/false); ddl_guard.reset(); chassert(done); created = true; diff --git a/src/Interpreters/InterpreterCreateQuery.h b/src/Interpreters/InterpreterCreateQuery.h index cb50a1cd9aeb..bed1e44de5b1 100644 --- a/src/Interpreters/InterpreterCreateQuery.h +++ b/src/Interpreters/InterpreterCreateQuery.h @@ -108,7 +108,7 @@ class InterpreterCreateQuery : public IInterpreter, WithMutableContext AccessRightsElements getRequiredAccess() const; /// Create IStorage and add it to database. If table already exists and IF NOT EXISTS specified, do nothing and return false. - bool doCreateTable(ASTCreateQuery & create, const TableProperties & properties, DDLGuardPtr & ddl_guard, LoadingStrictnessLevel mode); + bool doCreateTable(ASTCreateQuery & create, const TableProperties & properties, DDLGuardPtr & ddl_guard, LoadingStrictnessLevel mode, bool engine_user_specified); BlockIO doCreateOrReplaceTable(ASTCreateQuery & create, const InterpreterCreateQuery::TableProperties & properties, LoadingStrictnessLevel mode); BlockIO doCreateOrReplaceTemporaryTable(ASTCreateQuery & create, const InterpreterCreateQuery::TableProperties & properties, LoadingStrictnessLevel mode); #if CLICKHOUSE_CLOUD diff --git a/src/Interpreters/InterpreterDropQuery.cpp b/src/Interpreters/InterpreterDropQuery.cpp index 81c275ae7269..ab397bf4b326 100644 --- a/src/Interpreters/InterpreterDropQuery.cpp +++ b/src/Interpreters/InterpreterDropQuery.cpp @@ -98,6 +98,9 @@ BlockIO InterpreterDropQuery::execute() BlockIO InterpreterDropQuery::executeSingleDropQuery(const ASTPtr & drop_query_ptr) { auto & drop = drop_query_ptr->as(); + if (!drop.cluster.empty() && drop.table) + checkDatabaseSupportsOnClusterDDL( + DatabaseCatalog::instance().tryGetDatabase(getContext()->resolveDatabase(drop.getDatabase()))); if (!drop.cluster.empty() && drop.table && !drop.if_empty && !maybeRemoveOnCluster(current_query_ptr, getContext())) { DDLQueryOnClusterParams params; @@ -347,6 +350,10 @@ BlockIO InterpreterDropQuery::executeToTableImpl(const ContextPtr & context_, AS bool check_loading_deps = !check_ref_deps && getContext()->getSettingsRef()[Setting::check_table_dependencies]; DatabaseCatalog::instance().checkTableCanBeRemovedOrRenamed(table_id, check_ref_deps, check_loading_deps, is_drop_or_detach_database); + /// The actual data removal can happen in a background thread long after this query has + /// finished, so let the storage capture whatever it needs from the query context now. + table->prepareForDrop(context_); + table->flushAndShutdown(true); TableExclusiveLockHolder table_lock; @@ -355,7 +362,7 @@ BlockIO InterpreterDropQuery::executeToTableImpl(const ContextPtr & context_, AS DatabaseCatalog::instance().removeDependencies(table_id, check_ref_deps, check_loading_deps, is_drop_or_detach_database); NamedCollectionFactory::instance().removeDependencies(table_id); - database->dropTable(context_, table_id.table_name, query.sync); + database->dropTable(context_, table_id.table_name, query.sync, query.if_exists); /// We have to clear mmapio cache when dropping table from Ordinary database /// to avoid reading old data if new table with the same name is created diff --git a/src/Interpreters/executeDDLQueryOnCluster.cpp b/src/Interpreters/executeDDLQueryOnCluster.cpp index a1f76f89cfdc..d4ad5bc64225 100644 --- a/src/Interpreters/executeDDLQueryOnCluster.cpp +++ b/src/Interpreters/executeDDLQueryOnCluster.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -16,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -49,6 +51,35 @@ extern const int LOGICAL_ERROR; } +void checkQueryDatabasesSupportOnClusterDDL(const ASTPtr & query_ptr, ContextPtr context) +{ + /// RENAME / EXCHANGE TABLE is `ASTRenameQuery` and does not derive from `ASTQueryWithTableAndOutput`; + /// queries with no target table (`CREATE DATABASE`, `SYSTEM`, ...) contribute none. + std::vector target_databases; + const auto add_target_database = [&](String name) + { + target_databases.push_back(name.empty() ? context->getCurrentDatabase() : std::move(name)); + }; + + if (const auto * with_table = dynamic_cast(query_ptr.get()); + with_table && with_table->table) + { + add_target_database(with_table->getDatabase()); + } + else if (const auto * rename = dynamic_cast(query_ptr.get()); rename && !rename->database) + { + for (const auto & elem : rename->getElements()) + { + add_target_database(elem.from.getDatabase()); + add_target_database(elem.to.getDatabase()); + } + } + + for (const auto & database_name : target_databases) + checkDatabaseSupportsOnClusterDDL(DatabaseCatalog::instance().tryGetDatabase(database_name)); +} + + bool isSupportedAlterTypeForOnClusterDDLQuery(int type) { chassert(type != ASTAlterCommand::NO_TYPE); @@ -85,6 +116,9 @@ BlockIO executeDDLQueryOnCluster(const ASTPtr & query_ptr_, ContextPtr context, throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Distributed execution is not supported for such DDL queries"); } + /// Initiator-side guard; workers re-check in `DDLWorker::tryExecuteQuery`. + checkQueryDatabasesSupportOnClusterDDL(query_ptr, context); + if (!context->getSettingsRef()[Setting::allow_distributed_ddl]) throw Exception(ErrorCodes::QUERY_IS_PROHIBITED, "Distributed DDL queries are prohibited for the user"); @@ -224,6 +258,14 @@ BlockIO getDDLOnClusterStatus(const String & node_path, const String & replicas_ return io; } +void checkDatabaseSupportsOnClusterDDL(const DatabasePtr & database) +{ + if (database && database->isDatalakeCatalog()) + throw Exception(ErrorCodes::NOT_IMPLEMENTED, + "ON CLUSTER is not supported for DataLakeCatalog databases: " + "the catalog is shared, run the query without ON CLUSTER"); +} + bool maybeRemoveOnCluster(const ASTPtr & query_ptr, ContextPtr context) { const auto * query = dynamic_cast(query_ptr.get()); diff --git a/src/Interpreters/executeDDLQueryOnCluster.h b/src/Interpreters/executeDDLQueryOnCluster.h index 69e0c38834e6..f2b4bc2e5a00 100644 --- a/src/Interpreters/executeDDLQueryOnCluster.h +++ b/src/Interpreters/executeDDLQueryOnCluster.h @@ -19,10 +19,18 @@ namespace DB struct DDLLogEntry; class Cluster; using ClusterPtr = std::shared_ptr; +class IDatabase; +using DatabasePtr = std::shared_ptr; /// Returns true if provided ALTER type can be executed ON CLUSTER bool isSupportedAlterTypeForOnClusterDDLQuery(int type); +/// Throws if DDL on the database's tables cannot be mutated with ON CLUSTER +void checkDatabaseSupportsOnClusterDDL(const DatabasePtr & database); + +/// Throws if a target database of the query is a shared `DataLakeCatalog`, which does not support `ON CLUSTER` DDL. +void checkQueryDatabasesSupportOnClusterDDL(const ASTPtr & query_ptr, ContextPtr context); + struct DDLQueryOnClusterParams { /// A cluster to execute a distributed query. diff --git a/src/Storages/IStorage.h b/src/Storages/IStorage.h index a77f425b9f75..dca7097d04ed 100644 --- a/src/Storages/IStorage.h +++ b/src/Storages/IStorage.h @@ -520,6 +520,13 @@ It is currently only implemented in StorageObjectStorage. */ virtual void drop() {} + /** Called by `DROP TABLE` with the query context, while the query is still running and before the + * table is detached. `drop` itself can be executed much later in a background thread, where only + * the global context is available, so a storage that needs query- or session-level settings while + * dropping (for example `data_lake_delete_data_on_drop`) has to capture them here. + */ + virtual void prepareForDrop(ContextPtr /* query_context */) {} + virtual void dropInnerTableIfAny(bool /* sync */, ContextPtr /* context */) {} /// Return true if the storage supports TRUNCATE operation. diff --git a/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h b/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h index 92c8bf4cf60a..6095b4dc4ba5 100644 --- a/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h +++ b/src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h @@ -343,10 +343,10 @@ class DataLakeConfiguration : public BaseStorageConfiguration, public std::enabl return current_metadata->getColumnMapperForCurrentSchema(storage_metadata_snapshot, context); } - void drop(ContextPtr local_context) override + void drop(bool delete_data) override { if (current_metadata) - current_metadata->drop(local_context); + current_metadata->drop(delete_data); } SinkToStoragePtr write( @@ -853,7 +853,7 @@ class StorageIcebergConfiguration : public StorageObjectStorageConfiguration, pu bool supportsPrewhere() const override { return getImpl().supportsPrewhere(); } - void drop(ContextPtr context) override { getImpl().drop(context); } + void drop(bool delete_data) override { getImpl().drop(delete_data); } protected: void createDynamicConfiguration(ASTs & args, ContextPtr context) diff --git a/src/Storages/ObjectStorage/DataLakes/IDataLakeMetadata.h b/src/Storages/ObjectStorage/DataLakes/IDataLakeMetadata.h index 8784705e85b2..79864179cdcc 100644 --- a/src/Storages/ObjectStorage/DataLakes/IDataLakeMetadata.h +++ b/src/Storages/ObjectStorage/DataLakes/IDataLakeMetadata.h @@ -287,7 +287,9 @@ class IDataLakeMetadata : boost::noncopyable throwNotImplemented("truncate"); } - virtual void drop(ContextPtr) { } + /// `delete_data` is the effective `data_lake_delete_data_on_drop`, resolved by + /// `StorageObjectStorage::drop`. There is no context here: the drop runs in the background. + virtual void drop(bool /* delete_data */) { } virtual ObjectStorageType getObjectStorageType() const { return ObjectStorageType::None; } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp index 477c22316d9e..6cd49cfba9c6 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp @@ -437,7 +437,7 @@ static void writeMetadataFiles( auto new_snapshot = metadata_generator.generateNextMetadata( plan.generator, - generated_metadata_info.path, + Iceberg::IcebergPathFromMetadata{}, history_record.parent_id, append->added_files, total_records_count, diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp index 24930b88462a..3599fc2beb3a 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp @@ -121,6 +121,7 @@ extern const int NOT_IMPLEMENTED; extern const int ICEBERG_SPECIFICATION_VIOLATION; extern const int S3_ERROR; extern const int TABLE_ALREADY_EXISTS; +extern const int FILE_ALREADY_EXISTS; extern const int SUPPORT_IS_DISABLED; extern const int METADATA_MISMATCH; extern const int UNFINISHED; @@ -145,7 +146,6 @@ extern const SettingsBool allow_experimental_iceberg_compaction; extern const SettingsBool allow_experimental_geo_types_in_iceberg; extern const SettingsBool allow_iceberg_remove_orphan_files; extern const SettingsBool allow_experimental_expire_snapshots; -extern const SettingsBool iceberg_delete_data_on_drop; } static constexpr size_t MAX_TRANSACTION_RETRIES = 100; @@ -890,11 +890,13 @@ void IcebergMetadata::createInitial( } if (!metadata_files.empty()) { - if (if_not_exists) + /// Without a catalog `IF NOT EXISTS` attaches to the metadata that is already there. With a + /// catalog nothing gets registered here, so success would report an invisible table; + /// `InterpreterCreateQuery` turns `TABLE_ALREADY_EXISTS` into an `IF NOT EXISTS` no-op. + if (if_not_exists && !catalog) return; - else - throw Exception( - ErrorCodes::TABLE_ALREADY_EXISTS, "Iceberg table with path {} already exists", configuration_ptr->getPathForRead().path); + throw Exception( + ErrorCodes::TABLE_ALREADY_EXISTS, "Iceberg table with path {} already exists", configuration_ptr->getPathForRead().path); } String location_path = configuration_ptr->getRawPath().path; @@ -912,7 +914,11 @@ void IcebergMetadata::createInitial( if (!compression_suffix.empty()) compression_suffix = "." + compression_suffix; - auto filename = fmt::format("{}metadata/v1{}.metadata.json", configuration_ptr->getRawPath().path, compression_suffix); + auto table_uuid = metadata_content_object->getValue(Iceberg::f_table_uuid); + auto metadata_file_name = (catalog && catalog->isTransactional()) + ? fmt::format("v1-{}{}.metadata.json", table_uuid, compression_suffix) + : fmt::format("v1{}.metadata.json", compression_suffix); + auto filename = fmt::format("{}metadata/{}", configuration_ptr->getRawPath().path, metadata_file_name); try { @@ -921,26 +927,57 @@ void IcebergMetadata::createInitial( catch (const Exception & e) { /// The write uses `If-None-Match: *`, so S3 returns PreconditionFailed when the metadata file - /// already exists (e.g. leftover data after `DROP TABLE` with `iceberg_delete_data_on_drop` off, + /// already exists (e.g. leftover data after `DROP TABLE` with `data_lake_delete_data_on_drop` off, /// or a concurrent creation). When `IF NOT EXISTS` was specified, this is expected. - if (if_not_exists && e.code() == ErrorCodes::S3_ERROR - && e.message().find("PreconditionFailed") != String::npos) - return; + const bool precondition_failed + = (e.code() == ErrorCodes::S3_ERROR && e.message().contains("PreconditionFailed")) + || e.code() == ErrorCodes::FILE_ALREADY_EXISTS; + if (if_not_exists && precondition_failed) + { + /// As in the `metadata_files` probe above: with a catalog nothing was registered. + if (!catalog) + return; + throw Exception( + ErrorCodes::TABLE_ALREADY_EXISTS, "Iceberg table with path {} already exists", configuration_ptr->getPathForRead().path); + } throw; } - if (configuration_ptr->getDataLakeSettings()[DataLakeStorageSetting::iceberg_use_version_hint].value) + String filename_version_hint; + try { - auto filename_version_hint = configuration_ptr->getRawPath().path + "metadata/version-hint.text"; - writeMessageToFile("1", filename_version_hint, object_storage, local_context, "*", ""); - } + if (configuration_ptr->getDataLakeSettings()[DataLakeStorageSetting::iceberg_use_version_hint].value) + { + auto version_hint_path = configuration_ptr->getRawPath().path + "metadata/version-hint.text"; + writeMessageToFile("1", version_hint_path, object_storage, local_context, "*", ""); + filename_version_hint = version_hint_path; + } - if (catalog) + if (catalog) + { + auto catalog_filename = configuration_ptr->getTypeName() + "://" + configuration_ptr->getNamespace() + "/" + + configuration_ptr->getRawPath().path + "metadata/" + metadata_file_name; + const auto & [namespace_name, table_name] = DataLake::parseTableName(table_id_.getTableName()); + if (!catalog->createTable(namespace_name, table_name, catalog_filename, metadata_content_object, compression_method, if_not_exists)) + { + /// `IF NOT EXISTS`, and another client registered this table first. `TABLE_ALREADY_EXISTS` + /// lets `InterpreterCreateQuery` tell that nothing was created, so `CREATE TABLE IF NOT + /// EXISTS ... AS SELECT` does not fill the other client's table. + throw Exception(ErrorCodes::TABLE_ALREADY_EXISTS, + "Table {}.{} already exists in the catalog", namespace_name, table_name); + } + } + } + catch (...) { - auto catalog_filename = configuration_ptr->getTypeName() + "://" + configuration_ptr->getNamespace() + "/" - + configuration_ptr->getRawPath().path + "metadata/v1.metadata.json"; - const auto & [namespace_name, table_name] = DataLake::parseTableName(table_id_.getTableName()); - catalog->createTable(namespace_name, table_name, catalog_filename, metadata_content_object); + /// A leftover `.metadata.json` makes the next `CREATE` report an existing table although nothing + /// was registered in the catalog, so the rollback fails closed: a failed removal propagates in + /// place of the original exception, which is logged here. + tryLogCurrentException(__PRETTY_FUNCTION__, "Removing the files of the Iceberg table that failed to be created"); + object_storage->removeObjectIfExists(StoredObject(filename)); + if (!filename_version_hint.empty()) + object_storage->removeObjectIfExists(StoredObject(filename_version_hint)); + throw; } } @@ -1537,11 +1574,15 @@ SinkToStoragePtr IcebergMetadata::write( } } -void IcebergMetadata::drop(ContextPtr context) +void IcebergMetadata::drop(bool delete_data) { - if (!context->getSettingsRef()[Setting::iceberg_delete_data_on_drop].value) + if (!delete_data) return; + /// The drop runs in the background, when the query context is already gone; `delete_data` was + /// resolved from it by `StorageObjectStorage::drop`. + auto context = Context::getGlobalContextInstance(); + /// Files outside `table_path` (secondary storage, or base storage elsewhere in the bucket) are only /// discoverable through the metadata graph the base wipe below removes, so enumerate them first. Let /// a failure propagate rather than wiping the metadata re-enumeration on retry depends on (fail closed). diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.h index b28d476438da..b7f378a52997 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.h @@ -218,7 +218,7 @@ class IcebergMetadata : public IDataLakeMetadata StorageMetadataPtr storage_metadata, ContextPtr local_context) const override; - void drop(ContextPtr context) override; + void drop(bool delete_data) override; Poco::JSON::Object::Ptr getMetadataJSON(ContextPtr local_context) const; diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergPath.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergPath.h index 26cb3a564d0f..9d8878f394fc 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergPath.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergPath.h @@ -89,6 +89,8 @@ class IcebergPathResolver IcebergPathFromMetadata reverseResolve(const String & storage_path) const { + if (!table_location.empty() && storage_path.starts_with(table_location)) + return IcebergPathFromMetadata::deserialize(storage_path); if (storage_path.size() > table_root.size() && storage_path.starts_with(table_root)) return IcebergPathFromMetadata::deserialize(table_location + storage_path.substr(table_root.size())); diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp index 5ddc5ff3744b..576c0cd2641a 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp @@ -918,7 +918,7 @@ void generateManifestList( writer.write(entry_datum); } - if (use_previous_snapshots) + if (use_previous_snapshots && new_snapshot->has(Iceberg::f_parent_snapshot_id)) { auto parent_snapshot_id = new_snapshot->getValue(Iceberg::f_parent_snapshot_id); auto snapshots = metadata->getArray(Iceberg::f_snapshots); @@ -1039,6 +1039,7 @@ IcebergStorageSink::IcebergStorageSink( compression_method, persistent_table_components.table_uuid); metadata_compression_method = compression_method; + previous_metadata_file_path = metadata_path; filename_generator = FileNamesGenerator( persistent_table_components.path_resolver.getTableLocation(), (catalog != nullptr && catalog->isTransactional()), metadata_compression_method, write_format); @@ -1264,7 +1265,7 @@ bool IcebergStorageSink::initializeMetadata() total_data_files += static_cast(writer.getDataFiles().size()); auto [new_snapshot, manifest_list_path] = MetadataGenerator(metadata).generateNextMetadata( filename_generator, - metadata_info.path, + previous_metadata_file_path.empty() ? Iceberg::IcebergPathFromMetadata{} : resolver.reverseResolve(previous_metadata_file_path), parent_snapshot, total_data_files, total_rows, @@ -1313,6 +1314,7 @@ bool IcebergStorageSink::initializeMetadata() LOG_DEBUG(log, "Rereading metadata file {} with version {}", metadata_path, last_version); metadata_compression_method = compression_method; + previous_metadata_file_path = metadata_path; filename_generator.setVersion(last_version + 1); metadata = getMetadataJSONObject( diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.h index 988aece81874..84ab625257bb 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.h @@ -158,6 +158,7 @@ class IcebergStorageSink final : public SinkToStorage bool initializeMetadata(); FileNamesGenerator filename_generator; + String previous_metadata_file_path; std::optional partitioner; Poco::JSON::Object::Ptr partititon_spec; Int64 partition_spec_id; diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp index 55ee1c99baf3..23f7ef7f2363 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp @@ -343,7 +343,7 @@ Poco::JSON::Object::Ptr MetadataGenerator::getParentSnapshot(Int64 parent_snapsh MetadataGenerator::NextMetadataResult MetadataGenerator::generateNextMetadata( FileNamesGenerator & generator, - const Iceberg::IcebergPathFromMetadata & metadata_file_path, + const Iceberg::IcebergPathFromMetadata & previous_metadata_file_path, Int64 parent_snapshot_id, Int64 added_files, Int64 added_records, @@ -367,7 +367,8 @@ MetadataGenerator::NextMetadataResult MetadataGenerator::generateNextMetadata( auto manifest_list_path = generator.generateManifestListName(snapshot_id, format_version); new_snapshot->set(Iceberg::f_metadata_snapshot_id, snapshot_id); - new_snapshot->set(Iceberg::f_parent_snapshot_id, parent_snapshot_id); + if (parent_snapshot_id != -1) + new_snapshot->set(Iceberg::f_parent_snapshot_id, parent_snapshot_id); auto now = std::chrono::system_clock::now(); auto ms = duration_cast(now.time_since_epoch()); @@ -453,9 +454,10 @@ MetadataGenerator::NextMetadataResult MetadataGenerator::generateNextMetadata( else metadata_object->getObject(Iceberg::f_refs)->getObject(Iceberg::f_main)->set(Iceberg::f_metadata_snapshot_id, snapshot_id); + if (!previous_metadata_file_path.empty()) { Poco::JSON::Object::Ptr new_metadata_item = new Poco::JSON::Object; - new_metadata_item->set(Iceberg::f_metadata_file, metadata_file_path.serialize()); + new_metadata_item->set(Iceberg::f_metadata_file, previous_metadata_file_path.serialize()); new_metadata_item->set(Iceberg::f_timestamp_ms, timestamp); getOrCreateArray(metadata_object, Iceberg::f_metadata_log)->add(new_metadata_item); } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h index a20c1cfdc827..bf21fbcefe58 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.h @@ -31,7 +31,7 @@ class MetadataGenerator NextMetadataResult generateNextMetadata( FileNamesGenerator & generator, - const Iceberg::IcebergPathFromMetadata & metadata_file_path, + const Iceberg::IcebergPathFromMetadata & previous_metadata_file_path, Int64 parent_snapshot_id, Int64 added_files, Int64 added_records, diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp index 2933f50f9296..5e80c8df477a 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp @@ -437,12 +437,13 @@ static bool writeMetadataFiles( std::optional & chunk_partitioner, Iceberg::FileContentType content_type, SharedHeader sample_block, - bool write_metadata_json_file) + bool write_metadata_json_file, + const Iceberg::IcebergPathFromMetadata & previous_metadata_file_path) { auto metadata_info = filename_generator.generateMetadataPathWithInfo(); auto storage_metadata_name = path_resolver.resolve(metadata_info.path); Int64 parent_snapshot = -1; - if (metadata->has(Iceberg::f_current_snapshot_id)) + if (metadata->has(Iceberg::f_current_snapshot_id) && !metadata->isNull(Iceberg::f_current_snapshot_id)) parent_snapshot = metadata->getValue(Iceberg::f_current_snapshot_id); Int64 total_rows = 0; @@ -461,7 +462,7 @@ static bool writeMetadataFiles( { auto result = MetadataGenerator(metadata).generateNextMetadata( filename_generator, - metadata_info.path, + previous_metadata_file_path, parent_snapshot, /* added_files */ 0, /* added_records */ 0, @@ -476,7 +477,7 @@ static bool writeMetadataFiles( { auto result = MetadataGenerator(metadata).generateNextMetadata( filename_generator, - metadata_info.path, + previous_metadata_file_path, parent_snapshot, /* added_files */ total_files, /* added_records */ total_rows, @@ -672,6 +673,7 @@ void mutate( filename_generator.setCompressionMethod(compression_method); auto metadata = getMetadataJSONObject(metadata_path, object_storage, persistent_table_components.metadata_cache, context, log, compression_method, persistent_table_components.table_uuid); + auto previous_metadata_path = persistent_table_components.path_resolver.reverseResolve(metadata_path); /// Iceberg v3 writers must not add new position-delete files; row-level deletes require /// deletion vectors. Fail closed before any object writes until ClickHouse can write DVs. @@ -715,7 +717,7 @@ void mutate( current_iceberg_snapshot.metadata_file_path = metadata_path; current_iceberg_snapshot.metadata_version = last_version; current_iceberg_snapshot.schema_id = static_cast(current_schema_id); - if (metadata->has(Iceberg::f_current_snapshot_id)) + if (metadata->has(Iceberg::f_current_snapshot_id) && !metadata->isNull(Iceberg::f_current_snapshot_id)) { Int64 snapshot_id_val = metadata->getValue(Iceberg::f_current_snapshot_id); if (snapshot_id_val >= 0) @@ -761,7 +763,8 @@ void mutate( chunk_partitioner, Iceberg::FileContentType::POSITION_DELETE, std::make_shared(getPositionDeleteFileSampleBlock()), - !mutation_files->data_file); + !mutation_files->data_file, + previous_metadata_path); if (!result_delete_files_metadata) continue; @@ -784,7 +787,8 @@ void mutate( chunk_partitioner, Iceberg::FileContentType::DATA, sample_block, - true); + true, + previous_metadata_path); if (!result_data_files_metadata) { continue; diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp index 48f088cbe96e..8cb26df67f69 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp @@ -376,7 +376,17 @@ bool writeMetadataFileAndVersionHint( } else { - break; + /// Remove the metadata file written above, otherwise the resolver could later + /// pick this uncommitted file as the latest version. + /// + /// We deliberately do NOT swallow exceptions here. Returning `false` tells the caller + /// the write did not commit and is safe to retry; but if the removal throws, the + /// just-written `vN-.metadata.json` may still be present, and a subsequent + /// version-hint resolution could pick it as the latest version - exactly the race this + /// code guards against. Fail closed by letting the exception propagate so the operation + /// is reported as failed instead of silently leaving uncommitted metadata behind. + object_storage->removeObjectIfExists(StoredObject(storage_metadata_path)); + return false; } ++i; } @@ -793,6 +803,11 @@ static Poco::JSON::Object::Ptr getPartitionField( { if (!param.has_value()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "TRUNCATE function for iceberg partitioning requires one integer parameter"); + /// The Iceberg spec requires the truncate width to be a positive integer; otherwise we would + /// serialize an invalid `truncate[N]` transform that some catalogs (e.g. Glue) happily register, + /// leaving an unreadable table. + if (*param <= 0) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "TRUNCATE function for iceberg partitioning requires a positive width, got {}", *param); result->set(Iceberg::f_transform, fmt::format("truncate[{}]", *param)); return result; } @@ -800,6 +815,11 @@ static Poco::JSON::Object::Ptr getPartitionField( { if (!param.has_value()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "BUCKET function for iceberg partitioning requires one integer parameter"); + /// The Iceberg spec requires the number of buckets to be a positive integer; otherwise we would + /// serialize an invalid `bucket[N]` transform that some catalogs (e.g. Glue) happily register, + /// leaving an unreadable table. + if (*param <= 0) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "BUCKET function for iceberg partitioning requires a positive number of buckets, got {}", *param); result->set(Iceberg::f_transform, fmt::format("bucket[{}]", *param)); return result; } diff --git a/src/Storages/ObjectStorage/StorageObjectStorage.cpp b/src/Storages/ObjectStorage/StorageObjectStorage.cpp index abd0cb896222..093980c4e50f 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorage.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorage.cpp @@ -55,6 +55,7 @@ namespace Setting extern const SettingsInt64 delta_lake_snapshot_start_version; extern const SettingsInt64 delta_lake_snapshot_end_version; extern const SettingsUInt64 max_streams_for_files_processing_in_cluster_functions; + extern const SettingsBool data_lake_delete_data_on_drop; } namespace ErrorCodes @@ -849,15 +850,30 @@ void StorageObjectStorage::truncate( object_storage->removeObjectsIfExist(objects); } +void StorageObjectStorage::prepareForDrop(ContextPtr query_context) +{ + /// `drop` is executed in the background, when the query context is already gone, so the setting + /// has to be read while the `DROP TABLE` query is still running. A per-query or per-session + /// `data_lake_delete_data_on_drop` would be lost otherwise. + delete_data_on_drop = query_context->getSettingsRef()[Setting::data_lake_delete_data_on_drop]; +} + void StorageObjectStorage::drop() { + /// We cannot use query context here, because drop is executed in the background. `prepareForDrop` + /// captured it for us if this drop came from a `DROP TABLE` query; otherwise the server-level + /// value applies. + const bool delete_data = delete_data_on_drop.load().value_or( + Context::getGlobalContextInstance()->getSettingsRef()[Setting::data_lake_delete_data_on_drop]); + if (catalog) { const auto [namespace_name, table_name] = DataLake::parseTableName(storage_id.getTableName()); - catalog->dropTable(namespace_name, table_name); + /// This runs in the background, after the query has finished, so a missing table cannot be + /// reported as a no-op to the user anyway: keep reporting it as an error in the log. + catalog->dropTable(namespace_name, table_name, delete_data, /* if_exists */ false); } - /// We cannot use query context here, because drop is executed in the background. - configuration->drop(Context::getGlobalContextInstance()); + configuration->drop(delete_data); } std::unique_ptr StorageObjectStorage::createReadBufferIterator( diff --git a/src/Storages/ObjectStorage/StorageObjectStorage.h b/src/Storages/ObjectStorage/StorageObjectStorage.h index 5f1db8f2a527..a40fd5652a6b 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorage.h +++ b/src/Storages/ObjectStorage/StorageObjectStorage.h @@ -18,7 +18,9 @@ #include #include +#include #include +#include #include namespace DB @@ -110,6 +112,8 @@ class StorageObjectStorage : public IStorage, public IBackgroundOperation void drop() override; + void prepareForDrop(ContextPtr query_context) override; + bool supportsPartitionBy() const override { return true; } bool supportsSubcolumns() const override { return true; } @@ -258,6 +262,13 @@ class StorageObjectStorage : public IStorage, public IBackgroundOperation std::shared_ptr catalog; StorageID storage_id; BackgroundJobsAssignee background_operations_assignee; + + /// `data_lake_delete_data_on_drop` as it was set for the `DROP TABLE` query, captured by + /// `prepareForDrop` because `drop` runs without a query context. Stays empty when the drop does + /// not come from a `DROP TABLE` query (the rollback of a failed `CREATE`, or a lazily loaded + /// table that is only materialized inside `drop`); `drop` then falls back to the server-level + /// value, which is what this code read before the query-level capture existed. + std::atomic> delete_data_on_drop; }; } diff --git a/src/Storages/ObjectStorage/StorageObjectStorageConfiguration.h b/src/Storages/ObjectStorage/StorageObjectStorageConfiguration.h index c1af64950396..7ffc653d75db 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageConfiguration.h +++ b/src/Storages/ObjectStorage/StorageObjectStorageConfiguration.h @@ -338,7 +338,9 @@ class StorageObjectStorageConfiguration return true; } - virtual void drop(ContextPtr) {} + /// `delete_data` is the effective `data_lake_delete_data_on_drop`, resolved by + /// `StorageObjectStorage::drop`. There is no context here: the drop runs in the background. + virtual void drop(bool /* delete_data */) {} virtual bool isBackgroundExecutable() const { diff --git a/src/Storages/StorageProxy.h b/src/Storages/StorageProxy.h index e3ad0474a062..96d0f5dddd06 100644 --- a/src/Storages/StorageProxy.h +++ b/src/Storages/StorageProxy.h @@ -84,6 +84,10 @@ class StorageProxy : public IStorage return getNested()->write(query, metadata_snapshot, context, async_insert); } + /// `prepareForDrop` is deliberately NOT forwarded alongside this: it runs on the query thread, so + /// resolving the nested storage there would turn a resolution failure into a failed `DROP TABLE`, + /// while `drop` resolves it in the background where the failure is only logged. The proxies that + /// can forward it without resolving (`StorageTableProxy`, `StorageTableFunctionProxy`) do so. void drop() override { getNested()->drop(); } void truncate( diff --git a/src/Storages/StorageTableFunction.h b/src/Storages/StorageTableFunction.h index 884908cf0ccf..ef7044ff9af9 100644 --- a/src/Storages/StorageTableFunction.h +++ b/src/Storages/StorageTableFunction.h @@ -85,6 +85,13 @@ class StorageTableFunctionProxy final : public StorageProxy nested->drop(); } + void prepareForDrop(ContextPtr query_context) override + { + std::lock_guard lock{nested_mutex}; + if (nested) + nested->prepareForDrop(query_context); + } + void read( QueryPlan & query_plan, const Names & column_names, diff --git a/src/Storages/StorageTableProxy.h b/src/Storages/StorageTableProxy.h index e935de70e221..a6fbdb2e121c 100644 --- a/src/Storages/StorageTableProxy.h +++ b/src/Storages/StorageTableProxy.h @@ -79,6 +79,16 @@ class StorageTableProxy final : public StorageProxy nested->flushAndPrepareForShutdown(); } + void prepareForDrop(ContextPtr query_context) override + { + std::lock_guard lock{nested_mutex}; + /// A table that has not been loaded yet is materialized by `drop` below, and forcing the load + /// here would move a load failure from the background drop onto the user's query. Such a table + /// is dropped with the server-level settings, as it was before this hook existed. + if (nested) + nested->prepareForDrop(query_context); + } + void drop() override { std::lock_guard lock{nested_mutex}; diff --git a/tests/integration/test_database_glue/test.py b/tests/integration/test_database_glue/test.py index 665aee5cd205..d03c94e3691c 100644 --- a/tests/integration/test_database_glue/test.py +++ b/tests/integration/test_database_glue/test.py @@ -724,6 +724,106 @@ def test_create(started_cluster): assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") == "AAPL\n" +def test_native_create_gzip_metadata(started_cluster): + # The native `DataLakeCatalog` CREATE TABLE path (no engine clause) does not go through + # `IcebergMetadata::createInitial`: `DatabaseDataLake::createTable` hands an empty metadata path to + # the catalog, and `GlueCatalog::createTable` writes and registers the first metadata file itself. + # That branch must honour `iceberg_metadata_compression_method` just like the explicit Iceberg + # engine path does, otherwise the same CREATE TABLE query would produce differently compressed + # metadata depending on which create path served it. + node = started_cluster.instances["node1"] + + test_ref = f"test_native_create_gzip_{uuid.uuid4()}" + table_name = f"{test_ref}_table" + root_namespace = f"{test_ref}_namespace" + + create_clickhouse_glue_database(started_cluster, node, CATALOG_NAME) + + node.query( + f"CREATE TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` (x String)", + settings={ + "allow_experimental_database_glue_catalog": 1, + "allow_database_glue_catalog": 1, + "write_full_path_in_iceberg_metadata": 1, + "iceberg_metadata_compression_method": "gzip", + }, + ) + + glue_client = boto3.client( + "glue", region_name="us-east-1", endpoint_url=get_glue_local_url(started_cluster) + ) + table_info = glue_client.get_table(DatabaseName=root_namespace, Name=table_name)["Table"] + metadata_location = table_info["Parameters"]["metadata_location"] + # The file is named with the compression token, exactly as `IcebergMetadata::createInitial` names it. + assert metadata_location.endswith(".gzip.metadata.json"), metadata_location + + # The registered file must exist and its contents must really be gzip, not just carry the name. + assert metadata_location.startswith("s3://"), metadata_location + bucket, _, key = metadata_location[len("s3://") :].partition("/") + metadata_bytes = started_cluster.minio_client.get_object(bucket, key).read() + assert metadata_bytes[:2] == b"\x1f\x8b", metadata_bytes[:16] + + # Reopen through the catalog (fresh database) and confirm the table is readable and writable. + create_clickhouse_glue_database(started_cluster, node, CATALOG_NAME) + assert node.query(f"SELECT count() FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") == "0\n" + node.query( + f"INSERT INTO {CATALOG_NAME}.`{root_namespace}.{table_name}` VALUES ('AAPL');", + settings={ + "allow_insert_into_iceberg": 1, + "write_full_path_in_iceberg_metadata": 1, + "iceberg_metadata_compression_method": "gzip", + }, + ) + assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") == "AAPL\n" + + +def test_create_table_engine_backend_mismatch_rejected(started_cluster): + # Glue has a fixed S3 backend and reopens every table with it, so an explicit Iceberg engine + # pinning a different backend must be rejected up front instead of yielding an unreadable table. + node = started_cluster.instances["node1"] + + test_ref = f"test_engine_backend_mismatch_{uuid.uuid4()}" + root_namespace = f"{test_ref}_namespace" + + create_clickhouse_glue_database(started_cluster, node, CATALOG_NAME) + + for engine in [ + "IcebergAzure('http://acc.blob.core.windows.net/cont/tbl/', 'acc', 'key')", + "IcebergLocal('/var/lib/clickhouse/user_files/tbl/')", + "IcebergHDFS('hdfs://namenode:9000/tbl/')", + ]: + error = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`{root_namespace}.mismatch` (x String) ENGINE = {engine}", + settings={ + "allow_experimental_database_glue_catalog": 1, + "allow_database_glue_catalog": 1, + }, + ) + assert ( + "would be reopened with the catalog's storage backend and become unreadable" in error + ), error + assert "stores tables on S3" in error, error + + # Positive control: the matching S3 engine is accepted. + create_clickhouse_glue_table( + started_cluster, node, root_namespace, f"{test_ref}_s3", "(x String)" + ) + assert ( + node.query(f"SELECT count() FROM {CATALOG_NAME}.`{root_namespace}.{test_ref}_s3`") == "0\n" + ) + + error = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`{root_namespace}.{test_ref}_generic` (x String) " + f"ENGINE = Iceberg('http://minio1:9001/warehouse-glue/{test_ref}_generic/', " + f"'{minio_access_key}', '{minio_secret_key}')", + settings={ + "allow_experimental_database_glue_catalog": 1, + "write_full_path_in_iceberg_metadata": 1, + }, + ) + assert "generic 'Iceberg' engine is not supported" in error, error + + def test_schema_evolution(started_cluster): node = started_cluster.instances["node1"] @@ -807,6 +907,14 @@ def test_drop_table(started_cluster): assert len(catalog.list_tables(root_namespace)) == 1 assert node.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") == "" + # ClickHouse's Glue drop only removes the catalog entry and does not delete the data files, so a purge + # drop is rejected and the table is left intact. + error = node.query_and_get_error( + f"DROP TABLE {CATALOG_NAME}.`{root_namespace}.{table_name}` SETTINGS data_lake_delete_data_on_drop = 1" + ) + assert "not supported for the Glue catalog" in error + assert len(catalog.list_tables(root_namespace)) == 1 + drop_clickhouse_glue_table(node, root_namespace, table_name) assert len(catalog.list_tables(root_namespace)) == 0 diff --git a/tests/integration/test_database_iceberg/test.py b/tests/integration/test_database_iceberg/test.py index 5ec77a1a8fbd..a30c0b8b469c 100644 --- a/tests/integration/test_database_iceberg/test.py +++ b/tests/integration/test_database_iceberg/test.py @@ -11,6 +11,7 @@ import pytest import requests import pytz +from minio import Minio from pyiceberg.catalog import load_catalog from pyiceberg.partitioning import PartitionField, PartitionSpec, UNPARTITIONED_PARTITION_SPEC from pyiceberg.schema import Schema @@ -195,6 +196,7 @@ def started_cluster(): user_configs=[], stay_alive=True, with_iceberg_catalog=True, + with_zookeeper=True, ) cluster.add_instance( @@ -1006,6 +1008,67 @@ def test_cluster_select(started_cluster): assert node2.query(f"SELECT * FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`", settings={"parallel_replicas_for_cluster_engines": 1, "enable_parallel_replicas": 2, "cluster_for_parallel_replicas": "cluster_simple"}) == 'pablo\n' +def test_on_cluster_ddl_rejected_for_datalake_catalog(started_cluster): + # ON CLUSTER DDL against a shared DataLakeCatalog must be rejected on the worker as well as the initiator, + # because an initiator that lacks the catalog database locally cannot detect it and enqueues the query anyway. + node1 = started_cluster.instances["node1"] + node2 = started_cluster.instances["node2"] + + test_ref = f"test_on_cluster_ban_{uuid.uuid4().hex}" + table_name = f"{test_ref}_table" + namespace = f"{test_ref}_namespace" + qualified = f"{CATALOG_NAME}.`{namespace}.{table_name}`" + engine = ( + f"ENGINE = IcebergS3('http://minio1:9001/warehouse-rest/{table_name}/', " + f"'{minio_access_key}', '{minio_secret_key}')" + ) + ddl_settings = { + "allow_experimental_database_iceberg": 1, + "allow_database_iceberg": 1, + "write_full_path_in_iceberg_metadata": 1, + "distributed_ddl_output_mode": "throw", + } + + # Initiator has the catalog locally: rejected centrally before the query is ever enqueued. + create_clickhouse_iceberg_database(started_cluster, node1, CATALOG_NAME) + create_clickhouse_iceberg_database(started_cluster, node2, CATALOG_NAME) + err = node1.query_and_get_error( + f"CREATE TABLE {qualified} ON CLUSTER cluster_simple (x String) {engine}", + settings=ddl_settings, + ) + assert "ON CLUSTER is not supported for DataLakeCatalog" in err, err + + # Initiator lacks the catalog, but a worker (node1) has it, so only the worker guard can fire. The plain + # ON CLUSTER control below confirms the query reaches the workers, ruling out an unrelated failure. + node2.query(f"DROP DATABASE IF EXISTS {CATALOG_NAME}") + try: + control_table = f"{test_ref}_control" + node2.query( + f"CREATE TABLE default.{control_table} ON CLUSTER cluster_simple (x Int32) ENGINE = MergeTree ORDER BY x", + settings={"distributed_ddl_output_mode": "throw"}, + ) + assert node1.query(f"EXISTS TABLE default.{control_table}") == "1\n" + node2.query( + f"DROP TABLE default.{control_table} ON CLUSTER cluster_simple SYNC", + settings={"distributed_ddl_output_mode": "throw"}, + ) + + node2.query_and_get_error( + f"CREATE TABLE {qualified} ON CLUSTER cluster_simple (x String) {engine}", + settings=ddl_settings, + ) + + # The shared catalog must be untouched: without the worker guard node1 would have created the table here. + catalog = load_catalog_impl(started_cluster) + existing_namespaces = {".".join(ns) for ns in catalog.list_namespaces()} + if namespace in existing_namespaces: + tables = {ident[-1] for ident in catalog.list_tables(namespace)} + assert table_name not in tables, f"table must not have been created in the shared catalog: {tables}" + finally: + # Restore the catalog database on node2 for subsequent tests, even if an assertion above failed. + create_clickhouse_iceberg_database(started_cluster, node2, CATALOG_NAME) + + def test_used_storages_in_query_log(started_cluster): node1 = started_cluster.instances["node1"] node2 = started_cluster.instances["node2"] @@ -1169,6 +1232,696 @@ def test_system_tables_with_nullptr_table(started_cluster): node.query(f"DROP DATABASE IF EXISTS {CATALOG_NAME}") + +def test_create_table_as(started_cluster): + node = started_cluster.instances["node1"] + + namespace = "test_ctas_ns" + src_table = "src_ctas" + catalog = load_catalog_impl(started_cluster) + + create_clickhouse_iceberg_database( + started_cluster, + node, + CATALOG_NAME, + additional_settings={"default_base_location": "s3://warehouse-rest/data"}, + ) + + node.query(f"DROP TABLE IF EXISTS default.{src_table}") + for table in ["from_as", "override"]: + node.query( + f"DROP TABLE IF EXISTS {CATALOG_NAME}.`{namespace}.{table}` SETTINGS allow_database_iceberg=1" + ) + + # Intentional: CTAS must work from a source with a functional partition key. + node.query( + f""" + CREATE TABLE default.{src_table} + ( + id Int64, + name String, + dt Date + ) + ENGINE = MergeTree + PARTITION BY toYearNumSinceEpoch(dt) + ORDER BY (id, name) + """ + ) + + node.query( + f""" + CREATE TABLE {CATALOG_NAME}.`{namespace}.from_as` + AS default.{src_table} SETTINGS allow_database_iceberg=1; + """ + ) + + # CTAS with explicit PARTITION BY / ORDER BY overriding the source table. + node.query( + f""" + CREATE TABLE {CATALOG_NAME}.`{namespace}.override` + AS default.{src_table} + PARTITION BY id + ORDER BY name + SETTINGS allow_database_iceberg=1; + """ + ) + + tables = catalog.list_tables(namespace) + table_names = [t[1] for t in tables] + assert "from_as" in table_names + assert "override" in table_names + + tbl = catalog.load_table(f"{namespace}.from_as") + col_names = [f.name for f in tbl.schema().fields] + assert col_names == ["id", "name", "dt"] + + tbl = catalog.load_table(f"{namespace}.override") + assert len(tbl.spec().fields) == 1 + assert tbl.spec().fields[0].name == "id" + assert str(tbl.spec().fields[0].transform) == "identity" + + col_names = [f.name for f in tbl.schema().fields] + assert col_names == ["id", "name", "dt"] + + # Unsupported storage clauses written explicitly on CREATE ... AS must be rejected, not silently dropped + # when the storage is rebuilt to keep only PARTITION BY / ORDER BY. + for clause in [ + "PRIMARY KEY id", + "SAMPLE BY id", + "TTL toDate('2099-01-01')", + "UNIQUE KEY id", + "SETTINGS index_granularity = 8192", + ]: + err = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`{namespace}.ctas_unsupp` " + f"AS default.{src_table} {clause}", + settings={"allow_database_iceberg": 1}, + ) + assert "supports only PARTITION BY and ORDER BY" in err + + for table in ["from_as", "override"]: + node.query( + f"DROP TABLE {CATALOG_NAME}.`{namespace}.{table}` SETTINGS allow_database_iceberg=1" + ) + node.query(f"DROP TABLE default.{src_table}") + + +def test_create_table_as_rejects_column_modifiers(started_cluster): + node = started_cluster.instances["node1"] + + namespace = "test_ctas_colmod_ns" + + create_clickhouse_iceberg_database( + started_cluster, + node, + CATALOG_NAME, + additional_settings={"default_base_location": "s3://warehouse-rest/data"}, + ) + + # Each source table carries a column modifier that Iceberg tables do not support. CREATE TABLE ... AS + # must reject it instead of silently creating the Iceberg table with weaker semantics than the source. + cases = [ + ("comment", "id Int64, name String COMMENT 'the name'"), + ("codec", "id Int64, name String CODEC(ZSTD)"), + ("ttl", "id Int64, dt Date, val Int64 TTL dt + INTERVAL 1 DAY"), + ] + for src_suffix, cols in cases: + src_table = f"src_colmod_{src_suffix}" + node.query(f"DROP TABLE IF EXISTS default.{src_table}") + node.query( + f"CREATE TABLE default.{src_table} ({cols}) ENGINE = MergeTree ORDER BY id" + ) + err = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`{namespace}.dst_{src_suffix}` " + f"AS default.{src_table} SETTINGS allow_database_iceberg = 1" + ) + assert "COMMENT, CODEC, TTL, STATISTICS, SETTINGS, and PRIMARY KEY are not supported" in err + node.query(f"DROP TABLE default.{src_table}") + + +def test_create_table_explicit_columns(started_cluster): + node = started_cluster.instances["node1"] + + namespace = "test_ctex_ns" + catalog = load_catalog_impl(started_cluster) + + create_clickhouse_iceberg_database( + started_cluster, + node, + CATALOG_NAME, + additional_settings={"default_base_location": "s3://warehouse-rest/data"}, + ) + + node.query( + f"DROP TABLE IF EXISTS {CATALOG_NAME}.`{namespace}.explicit` SETTINGS allow_database_iceberg=1" + ) + + node.query( + f""" + CREATE TABLE {CATALOG_NAME}.`{namespace}.explicit` + ( + id Int64, + name String, + value Float64 + ) + PARTITION BY id + ORDER BY name + SETTINGS allow_database_iceberg=1; + """ + ) + + tables = catalog.list_tables(namespace) + table_names = [t[1] for t in tables] + assert "explicit" in table_names + + tbl = catalog.load_table(f"{namespace}.explicit") + col_names = [f.name for f in tbl.schema().fields] + assert col_names == ["id", "name", "value"] + + iceberg_types = {f.name: str(f.field_type) for f in tbl.schema().fields} + assert iceberg_types["id"] == "long" + assert iceberg_types["name"] == "string" + assert iceberg_types["value"] == "double" + + node.query( + f"INSERT INTO {CATALOG_NAME}.`{namespace}.explicit` VALUES (1, 'a', 1.5);", + settings={ + "allow_insert_into_iceberg": 1, + "write_full_path_in_iceberg_metadata": 1, + }, + ) + assert ( + node.query( + f"SELECT id, name, value FROM {CATALOG_NAME}.`{namespace}.explicit`" + ) + == "1\ta\t1.5\n" + ) + + node.query( + f"DROP TABLE {CATALOG_NAME}.`{namespace}.explicit` SETTINGS allow_database_iceberg=1" + ) + + +def test_create_table_nested_namespace(started_cluster): + node = started_cluster.instances["node1"] + + namespace = "test_nested_ns.a.b" + catalog = load_catalog_impl(started_cluster) + + create_clickhouse_iceberg_database( + started_cluster, + node, + CATALOG_NAME, + additional_settings={"default_base_location": "s3://warehouse-rest/data"}, + ) + + node.query( + f"DROP TABLE IF EXISTS {CATALOG_NAME}.`{namespace}.nested` SETTINGS allow_database_iceberg=1" + ) + node.query( + f""" + CREATE TABLE {CATALOG_NAME}.`{namespace}.nested` + ( + id Int64 + ) + SETTINGS allow_database_iceberg=1; + """ + ) + + tables = catalog.list_tables(namespace) + table_names = [t[-1] for t in tables] + assert "nested" in table_names + + write_settings = {"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1} + + # INSERT exercises RestCatalog::updateMetadata for the nested namespace. + node.query( + f"INSERT INTO {CATALOG_NAME}.`{namespace}.nested` VALUES (1);", + settings=write_settings, + ) + assert node.query( + f"SELECT id FROM {CATALOG_NAME}.`{namespace}.nested`", + settings={"allow_database_iceberg": 1}, + ).strip() == "1" + + # ALTER exercises RestCatalog::updateSchema for the nested namespace. + node.query( + f"ALTER TABLE {CATALOG_NAME}.`{namespace}.nested` ADD COLUMN z Nullable(String);", + settings=write_settings, + ) + assert "z" in node.query( + f"DESCRIBE TABLE {CATALOG_NAME}.`{namespace}.nested`", + settings=write_settings, + ) + + node.query( + f"DROP TABLE {CATALOG_NAME}.`{namespace}.nested` SETTINGS allow_database_iceberg=1" + ) + + +def test_create_non_table_rejected(started_cluster): + node = started_cluster.instances["node1"] + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + + # Each DDL flips a distinct flag in the interpreter's "plain CREATE TABLE only" guard. + for ddl in [ + f"CREATE VIEW {CATALOG_NAME}.`ns.v` AS SELECT 1", + f"CREATE MATERIALIZED VIEW {CATALOG_NAME}.`ns.mv` ENGINE = Memory AS SELECT 1", + f"ATTACH TABLE {CATALOG_NAME}.`ns.attached` (x Int32) ENGINE = Memory", + f"CREATE OR REPLACE TABLE {CATALOG_NAME}.`ns.replaced` (x Int32)", + ]: + err = node.query_and_get_error(ddl, settings={"allow_database_iceberg": 1}) + assert "supports only plain CREATE TABLE" in err + + node.query("DROP TABLE IF EXISTS default.src_clone") + node.query( + "CREATE TABLE default.src_clone (x Int32) ENGINE = MergeTree ORDER BY x" + ) + err = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`ns.cloned` CLONE AS default.src_clone", + settings={"allow_database_iceberg": 1}, + ) + assert "supports only plain CREATE TABLE" in err + node.query("DROP TABLE default.src_clone") + + err = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`ns.mem` (x Int32) ENGINE = Memory", + settings={"allow_database_iceberg": 1}, + ) + assert "only supports Iceberg-family table engines" in err + + +def test_create_table_unsupported_clauses(started_cluster): + node = started_cluster.instances["node1"] + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + + base_ddl = f"CREATE TABLE {CATALOG_NAME}.`ns.unsupp` (id Int64, name String)" + for clause in [ + "PRIMARY KEY id ORDER BY id", + "ORDER BY id SAMPLE BY id", + "ORDER BY id TTL toDate('2099-01-01')", + "ORDER BY id SETTINGS index_granularity = 8192", + ]: + err = node.query_and_get_error( + f"{base_ddl} {clause}", + settings={"allow_database_iceberg": 1}, + ) + assert "supports only PARTITION BY and ORDER BY" in err + + # The table COMMENT is not persisted anywhere, so it is rejected by a separate check. + err = node.query_and_get_error( + f"{base_ddl} ORDER BY id COMMENT 'tbl comment'", + settings={"allow_database_iceberg": 1}, + ) + assert "Table COMMENT is not supported" in err + + for table_element in [ + "INDEX idx_name name TYPE bloom_filter GRANULARITY 1", + "PROJECTION p (SELECT id ORDER BY name)", + "CONSTRAINT c CHECK id > 0", + ]: + err = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`ns.unsupp_elem` (id Int64, name String, {table_element}) ORDER BY id", + settings={"allow_database_iceberg": 1}, + ) + assert "does not support PRIMARY KEY, indices" in err + + # Column-level PRIMARY KEY is normalized into the storage-level clause, covered above. + for col_clause in [ + "(id Int64 COMMENT 'pk', name String)", + "(id Int64, name String CODEC(ZSTD))", + "(id Int64, dt Date TTL dt + INTERVAL 1 DAY)", + "(id Int64, name String SETTINGS (max_compress_block_size = 1))", + ]: + err = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`ns.unsupp_col` {col_clause} ORDER BY id", + settings={"allow_database_iceberg": 1}, + ) + assert "COMMENT, CODEC, TTL, STATISTICS, SETTINGS, and PRIMARY KEY are not supported" in err + + for col_clause in [ + "(id Int64, d Int64 DEFAULT 1)", + "(id Int64, d Int64 MATERIALIZED id + 1)", + ]: + err = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`ns.unsupp_def` {col_clause} ORDER BY id", + settings={"allow_database_iceberg": 1}, + ) + assert "is not yet supported" in err + + +def test_create_table_with_engine_unsupported_clauses(started_cluster): + node = started_cluster.instances["node1"] + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + + engine = ( + f"ENGINE = IcebergS3('http://minio1:9001/warehouse-rest/engine_unsupp/', " + f"'{minio_access_key}', '{minio_secret_key}')" + ) + + # The explicit-ENGINE path persists only column names/types, PARTITION BY, and ORDER BY into the + # initial Iceberg metadata, so unsupported storage clauses must be rejected there too, not silently + # dropped. Unlike the engine-less path, engine SETTINGS stay allowed: they are real data-lake + # storage settings (e.g. `iceberg_format_version`) used during creation. + for clause in [ + "PRIMARY KEY id", + "ORDER BY id SAMPLE BY id", + "TTL toDate('2099-01-01')", + ]: + err = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`ns.engine_unsupp` (id Int64, name String) {engine} {clause}", + settings={"allow_database_iceberg": 1}, + ) + assert "PRIMARY KEY, SAMPLE BY, TTL, and UNIQUE KEY are not supported" in err + + for table_element in [ + "INDEX idx_name name TYPE bloom_filter GRANULARITY 1", + "PROJECTION p (SELECT id ORDER BY name)", + "CONSTRAINT c CHECK id > 0", + ]: + err = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`ns.engine_unsupp` (id Int64, name String, {table_element}) {engine}", + settings={"allow_database_iceberg": 1}, + ) + assert "does not support PRIMARY KEY, indices" in err + + for col_clause in [ + "(id Int64 COMMENT 'pk', name String)", + "(id Int64, name String CODEC(ZSTD))", + "(id Int64, dt Date TTL dt + INTERVAL 1 DAY)", + "(id Int64, name String SETTINGS (max_compress_block_size = 1))", + ]: + err = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`ns.engine_unsupp` {col_clause} {engine}", + settings={"allow_database_iceberg": 1}, + ) + assert "COMMENT, CODEC, TTL, STATISTICS, SETTINGS, and PRIMARY KEY are not supported" in err + + for col_clause in [ + "(id Int64, d Int64 DEFAULT 1)", + "(id Int64, d Int64 MATERIALIZED id + 1)", + ]: + err = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`ns.engine_unsupp` {col_clause} {engine}", + settings={"allow_database_iceberg": 1}, + ) + assert "is not yet supported" in err + + # Modifiers inherited via CREATE TABLE ... AS are rejected on the explicit-ENGINE path too. + node.query("DROP TABLE IF EXISTS default.src_engine_colmod") + node.query( + "CREATE TABLE default.src_engine_colmod (id Int64, name String COMMENT 'the name') " + "ENGINE = MergeTree ORDER BY id" + ) + err = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`ns.engine_unsupp` AS default.src_engine_colmod {engine}", + settings={"allow_database_iceberg": 1}, + ) + assert "COMMENT, CODEC, TTL, STATISTICS, SETTINGS, and PRIMARY KEY are not supported" in err + node.query("DROP TABLE default.src_engine_colmod") + + # Positive control: PARTITION BY, ORDER BY, and engine SETTINGS remain supported with an explicit engine. + namespace = f"test_engine_supp_{uuid.uuid4().hex}" + node.query( + f"CREATE TABLE {CATALOG_NAME}.`{namespace}.engine_supp` (id Int64, name String) " + f"ENGINE = IcebergS3('http://minio1:9001/warehouse-rest/engine_supp/', " + f"'{minio_access_key}', '{minio_secret_key}') " + f"PARTITION BY id ORDER BY name SETTINGS iceberg_format_version = 2", + settings={ + "allow_database_iceberg": 1, + "write_full_path_in_iceberg_metadata": 1, + }, + ) + node.query( + f"DROP TABLE {CATALOG_NAME}.`{namespace}.engine_supp`", + settings={"allow_database_iceberg": 1}, + ) + + +def test_create_table_invalid_partition_transforms(started_cluster): + node = started_cluster.instances["node1"] + + namespace = "test_invalid_part_ns" + catalog = load_catalog_impl(started_cluster) + + create_clickhouse_iceberg_database( + started_cluster, + node, + CATALOG_NAME, + additional_settings={"default_base_location": "s3://warehouse-rest/data"}, + ) + + # A valid positive transform is accepted (and creates the namespace). + node.query( + f"DROP TABLE IF EXISTS {CATALOG_NAME}.`{namespace}.good` SETTINGS allow_database_iceberg=1" + ) + node.query( + f""" + CREATE TABLE {CATALOG_NAME}.`{namespace}.good` + ( + id Int64 + ) + PARTITION BY icebergBucket(8, id) + SETTINGS allow_database_iceberg=1; + """ + ) + assert "good" in [t[1] for t in catalog.list_tables(namespace)] + + # Invalid transform parameters must be rejected before any catalog metadata is written. + # Otherwise CREATE TABLE could register an unreadable Iceberg table whose partition spec + # serializes as bucket[0], bucket[-1], or truncate[0]. + for i, transform in enumerate( + ["icebergBucket(0, id)", "icebergBucket(-1, id)", "icebergTruncate(0, id)"] + ): + tbl = f"bad_{i}" + err = node.query_and_get_error( + f"CREATE TABLE {CATALOG_NAME}.`{namespace}.{tbl}` (id Int64) " + f"PARTITION BY {transform} SETTINGS allow_database_iceberg=1" + ) + assert "requires a positive" in err, err + assert tbl not in [t[1] for t in catalog.list_tables(namespace)] + + node.query( + f"DROP TABLE {CATALOG_NAME}.`{namespace}.good` SETTINGS allow_database_iceberg=1" + ) + + +def test_create_table_namespace_location(started_cluster): + node = started_cluster.instances["node1"] + + namespace = f"test_ns_location_{uuid.uuid4().hex[:8]}" + catalog = load_catalog_impl(started_cluster) + + create_clickhouse_iceberg_database( + started_cluster, + node, + CATALOG_NAME, + additional_settings={"default_base_location": "s3://warehouse-rest/data"}, + ) + + node.query( + f"DROP TABLE IF EXISTS {CATALOG_NAME}.`{namespace}.first` SETTINGS allow_database_iceberg=1" + ) + node.query( + f""" + CREATE TABLE {CATALOG_NAME}.`{namespace}.first` + ( + id Int64 + ) + SETTINGS allow_database_iceberg=1; + """ + ) + + table_location = catalog.load_table(f"{namespace}.first").location().rstrip("/") + ns_location = catalog.load_namespace_properties(namespace).get("location") + + # The namespace default location must point at the namespace base, not at the first table's + # directory. Otherwise another client creating a table in the same namespace without an explicit + # location could be placed under the first table's path. + assert ns_location is not None, "namespace is missing its location property" + ns_location = ns_location.rstrip("/") + assert ns_location != table_location, ( + f"namespace location {ns_location} must not equal the first table location {table_location}" + ) + assert table_location.startswith(ns_location + "/"), (ns_location, table_location) + assert table_location[len(ns_location):].strip("/") == "first", (ns_location, table_location) + + node.query( + f"DROP TABLE {CATALOG_NAME}.`{namespace}.first` SETTINGS allow_database_iceberg=1" + ) + + +def test_create_table_with_engine_namespace_location(started_cluster): + node = started_cluster.instances["node1"] + + namespace = f"test_ns_engine_location_{uuid.uuid4().hex[:8]}" + table_dir = f"engine_ns_location_{uuid.uuid4().hex[:8]}" + catalog = load_catalog_impl(started_cluster) + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + + node.query( + f"CREATE TABLE {CATALOG_NAME}.`{namespace}.first` (id Int64) " + f"ENGINE = IcebergS3('http://minio1:9001/warehouse-rest/{table_dir}/', " + f"'{minio_access_key}', '{minio_secret_key}')", + settings={ + "allow_database_iceberg": 1, + "write_full_path_in_iceberg_metadata": 1, + }, + ) + + table_location = catalog.load_table(f"{namespace}.first").location().rstrip("/") + assert table_location.endswith(table_dir), table_location + + # The engine path here is `//`, which does not follow `//
`, + # so there is no namespace base to derive. The table's own directory must not be registered as the + # namespace default location: another client creating a table in the same namespace without an + # explicit location would then be placed under this table's directory. + ns_location = catalog.load_namespace_properties(namespace).get("location") + if ns_location is not None: + ns_location = ns_location.rstrip("/") + assert ns_location != table_location, (ns_location, table_location) + assert not ns_location.startswith(table_location + "/"), (ns_location, table_location) + + node.query( + f"DROP TABLE {CATALOG_NAME}.`{namespace}.first`", + settings={"allow_database_iceberg": 1}, + ) + + # When the engine path does follow `//
`, the namespace base is unambiguous + # and is registered as the namespace default location, so tables created later by other clients + # land next to this one instead of inside it. + base_dir = f"engine_ns_base_{uuid.uuid4().hex[:8]}" + nested_namespace = f"test_ns_engine_derived_{uuid.uuid4().hex[:8]}" + node.query( + f"CREATE TABLE {CATALOG_NAME}.`{nested_namespace}.second` (id Int64) " + f"ENGINE = IcebergS3('http://minio1:9001/warehouse-rest/{base_dir}/{nested_namespace}/second/', " + f"'{minio_access_key}', '{minio_secret_key}')", + settings={ + "allow_database_iceberg": 1, + "write_full_path_in_iceberg_metadata": 1, + }, + ) + + ns_location = catalog.load_namespace_properties(nested_namespace).get("location") + assert ns_location is not None, "namespace is missing its location property" + assert ns_location.rstrip("/") == f"s3://warehouse-rest/{base_dir}/{nested_namespace}", ns_location + + node.query( + f"DROP TABLE {CATALOG_NAME}.`{nested_namespace}.second`", + settings={"allow_database_iceberg": 1}, + ) + + +def test_drop_table_purge(started_cluster): + node = started_cluster.instances["node1"] + + namespace = "test_drop_purge_ns" + catalog = load_catalog_impl(started_cluster) + minio_client = Minio( + f"{started_cluster.minio_ip}:{started_cluster.minio_port}", + access_key=minio_access_key, + secret_key=minio_secret_key, + secure=False, + ) + + create_clickhouse_iceberg_database( + started_cluster, + node, + CATALOG_NAME, + additional_settings={"default_base_location": "s3://warehouse-rest/data"}, + ) + + for table in ["to_keep", "to_purge"]: + node.query( + f"DROP TABLE IF EXISTS {CATALOG_NAME}.`{namespace}.{table}` SETTINGS allow_database_iceberg=1" + ) + node.query( + f""" + CREATE TABLE {CATALOG_NAME}.`{namespace}.{table}` + ( + id Int64 + ) + SETTINGS allow_database_iceberg=1; + """ + ) + + table_names = [t[1] for t in catalog.list_tables(namespace)] + assert "to_keep" in table_names + assert "to_purge" in table_names + + def table_prefix(table): + location = catalog.load_table(f"{namespace}.{table}").location() + assert location.startswith("s3://warehouse-rest/") + prefix = location[len("s3://warehouse-rest/"):].rstrip("/") + "/" + assert list( + minio_client.list_objects("warehouse-rest", prefix=prefix, recursive=True) + ), f"Expected metadata under {prefix} before drop" + return prefix + + keep_prefix = table_prefix("to_keep") + purge_prefix = table_prefix("to_purge") + + node.query( + f"DROP TABLE {CATALOG_NAME}.`{namespace}.to_keep` SETTINGS allow_database_iceberg=1" + ) + node.query( + f"DROP TABLE {CATALOG_NAME}.`{namespace}.to_purge` SETTINGS allow_database_iceberg=1, data_lake_delete_data_on_drop=1" + ) + + table_names = [t[1] for t in catalog.list_tables(namespace)] + assert "to_keep" not in table_names + assert "to_purge" not in table_names + + # A drop without purge unregisters the table from the catalog but must keep its data. + assert list( + minio_client.list_objects("warehouse-rest", prefix=keep_prefix, recursive=True) + ), f"Expected objects under {keep_prefix} to survive a drop without purge" + + remaining = [ + o.object_name + for o in minio_client.list_objects("warehouse-rest", prefix=purge_prefix, recursive=True) + ] + assert not remaining, f"Expected purge to remove objects under {purge_prefix}, found: {remaining}" + + + +def test_create_if_not_exists_with_engine_over_leftover_metadata(started_cluster): + """ + A drop without `data_lake_delete_data_on_drop` keeps the metadata, so a later + `CREATE TABLE IF NOT EXISTS ... ENGINE = IcebergS3(...)` over the same path registers nothing and + must report that nothing was created - otherwise the insert of `... AS SELECT` gets no table. + """ + node = started_cluster.instances["node1"] + namespace = f"test_ns_leftover_{uuid.uuid4().hex[:8]}" + engine = ( + f"IcebergS3('http://minio1:9001/warehouse-rest/{namespace}/t/', " + f"'{minio_access_key}', '{minio_secret_key}')" + ) + settings = { + "allow_database_iceberg": 1, + "allow_insert_into_iceberg": 1, + "write_full_path_in_iceberg_metadata": 1, + } + + create_clickhouse_iceberg_database(started_cluster, node, CATALOG_NAME) + node.query( + f"CREATE TABLE {CATALOG_NAME}.`{namespace}.t` (id Int64) ENGINE = {engine}", + settings=settings, + ) + node.query(f"DROP TABLE {CATALOG_NAME}.`{namespace}.t`", settings=settings) + + node.query( + f"CREATE TABLE IF NOT EXISTS {CATALOG_NAME}.`{namespace}.t` (id Int64) " + f"ENGINE = {engine} AS SELECT 1 AS id", + settings=settings, + ) + assert node.query(f"EXISTS TABLE {CATALOG_NAME}.`{namespace}.t`", settings=settings) == "0\n" + + def test_delete_on_lazy_initialized_table(started_cluster): """ Regression test for https://github.com/ClickHouse/ClickHouse/issues/96806. diff --git a/tests/integration/test_storage_iceberg_no_spark/test_drop_delete_data_on_drop.py b/tests/integration/test_storage_iceberg_no_spark/test_drop_delete_data_on_drop.py new file mode 100644 index 000000000000..9ad8c45838e1 --- /dev/null +++ b/tests/integration/test_storage_iceberg_no_spark/test_drop_delete_data_on_drop.py @@ -0,0 +1,49 @@ +import pytest + +from helpers.iceberg_utils import create_iceberg_table, get_uuid_str + + +def _table_dir(table_name): + return f"/var/lib/clickhouse/user_files/iceberg_data/default/{table_name}" + + +def _files_left(instance, table_name): + return int( + instance.exec_in_container( + ["bash", "-c", f"find {_table_dir(table_name)} -type f 2>/dev/null | wc -l"] + ).strip() + ) + + +@pytest.mark.parametrize("delete_data_on_drop", [0, 1]) +@pytest.mark.parametrize("storage_type", ["local"]) +def test_drop_honours_query_level_delete_data_on_drop( + started_cluster_iceberg_no_spark, storage_type, delete_data_on_drop +): + # `StorageObjectStorage::drop` runs in a background thread, where the query context is already + # gone. A query-level `data_lake_delete_data_on_drop` still has to reach it, because it is + # captured by `IStorage::prepareForDrop` while the `DROP TABLE` query is running. Without that + # capture the setting would only ever work when set server-wide in the default profile. + instance = started_cluster_iceberg_no_spark.instances["node1"] + table_name = f"test_delete_data_on_drop_{delete_data_on_drop}_{get_uuid_str()}" + + create_iceberg_table( + storage_type, + instance, + table_name, + started_cluster_iceberg_no_spark, + "(x String, y Int64)", + ) + instance.query(f"INSERT INTO {table_name} VALUES ('123', 1)") + assert instance.query(f"SELECT * FROM {table_name} ORDER BY ALL") == "123\t1\n" + assert _files_left(instance, table_name) > 0 + + instance.query( + f"DROP TABLE {table_name} SYNC", + settings={"data_lake_delete_data_on_drop": delete_data_on_drop}, + ) + + if delete_data_on_drop: + assert _files_left(instance, table_name) == 0 + else: + assert _files_left(instance, table_name) > 0 From 73509df036b820eb49547574e3e7e5e546a98e9d Mon Sep 17 00:00:00 2001 From: Andrey Zvonov <32552679+zvonand@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:29:47 +0200 Subject: [PATCH 2/4] Address review of native `CREATE TABLE` / `DROP TABLE` for `DataLakeCatalog` * Never remove the initial metadata file when `catalog->createTable` fails: the registration may have succeeded with only its response lost, which leaves the catalog pointing at a deleted object. Roll back only on a definitive answer. * Add `DataLake::TableAlreadyExistsInCatalogException` so `IF NOT EXISTS` swallows only that case instead of any `TABLE_ALREADY_EXISTS` raised below `doCreateTable`, log when it does, and report leftover metadata as what it is. * Keep the data in `StorageObjectStorage::drop` when `prepareForDrop` did not run, instead of falling back to the server-level value. * Log every metadata file removal. * Drop the `isRemoteDatabase` override, which re-coupled `DataLakeCatalog` visibility in `system.tables` to `show_remote_databases_in_system_tables`. * Trim comments; drop those describing behaviour that does not exist. --- src/Databases/DataLake/Common.cpp | 5 +- src/Databases/DataLake/Common.h | 26 ++++++ src/Databases/DataLake/DatabaseDataLake.cpp | 36 +++----- src/Databases/DataLake/DatabaseDataLake.h | 1 - src/Databases/DataLake/GlueCatalog.cpp | 33 ++++--- src/Databases/DataLake/ICatalog.h | 16 ++-- src/Databases/DataLake/RestCatalog.cpp | 11 ++- .../tests/gtest_construct_table_location.cpp | 18 +--- src/Databases/IDatabase.h | 4 +- src/Interpreters/DDLWorker.cpp | 1 - src/Interpreters/InterpreterCreateQuery.cpp | 56 +++++------ src/Interpreters/InterpreterDropQuery.cpp | 4 +- src/Interpreters/executeDDLQueryOnCluster.cpp | 2 +- src/Interpreters/executeDDLQueryOnCluster.h | 4 +- src/Storages/IStorage.h | 7 +- .../DataLakes/IDataLakeMetadata.h | 4 +- .../DataLakes/Iceberg/IcebergMetadata.cpp | 92 +++++++++++++------ .../ObjectStorage/DataLakes/Iceberg/Utils.cpp | 28 +++--- .../ObjectStorage/StorageObjectStorage.cpp | 26 ++++-- .../ObjectStorage/StorageObjectStorage.h | 6 +- .../StorageObjectStorageConfiguration.h | 4 +- src/Storages/StorageProxy.h | 6 +- src/Storages/StorageTableProxy.h | 5 +- tests/integration/test_database_glue/test.py | 10 +- .../integration/test_database_iceberg/test.py | 37 ++++---- .../test_drop_delete_data_on_drop.py | 7 +- 26 files changed, 236 insertions(+), 213 deletions(-) diff --git a/src/Databases/DataLake/Common.cpp b/src/Databases/DataLake/Common.cpp index db917b750790..220df15010d4 100644 --- a/src/Databases/DataLake/Common.cpp +++ b/src/Databases/DataLake/Common.cpp @@ -177,9 +177,8 @@ String constructTableLocation( if (location_scheme == "s3") { if (uri_style == DB::S3UriStyle::VIRTUAL_HOSTED) - /// A virtual-hosted endpoint cannot be split into bucket and service unambiguously - /// (`s3.us-east-1.amazonaws.com`, an IP host, and a dotted bucket name all look alike), so deriving - /// the bucket from the host would silently misplace data. Require an explicit `default_base_location`. + /// A virtual-hosted host cannot be split into bucket and service unambiguously + /// (`s3.us-east-1.amazonaws.com`, an IP host and a dotted bucket name all look alike). throw DB::Exception( DB::ErrorCodes::BAD_ARGUMENTS, "CREATE TABLE with `storage_uri_style = 'virtual_hosted'` cannot derive the bucket from " diff --git a/src/Databases/DataLake/Common.h b/src/Databases/DataLake/Common.h index 912758f7fae8..7f6d2bc781d6 100644 --- a/src/Databases/DataLake/Common.h +++ b/src/Databases/DataLake/Common.h @@ -4,10 +4,36 @@ #include #include #include +#include + +namespace DB::ErrorCodes +{ +extern const int TABLE_ALREADY_EXISTS; +} namespace DataLake { +/// Thrown by a `CREATE TABLE` that registered nothing in the data lake catalog: the table name, or the +/// location the table would use, is already taken. `InterpreterCreateQuery` matches on this type rather +/// than on its `TABLE_ALREADY_EXISTS` code, which would also swallow unrelated exceptions from below. +class TableAlreadyExistsInCatalogException : public DB::Exception +{ +public: + template + explicit TableAlreadyExistsInCatalogException(FormatStringHelper fmt, Args &&... args) + : DB::Exception(DB::ErrorCodes::TABLE_ALREADY_EXISTS, std::move(fmt), std::forward(args)...) + { + } + + TableAlreadyExistsInCatalogException * clone() const override { return new TableAlreadyExistsInCatalogException(*this); } + void rethrow() const override { throw *this; } /// NOLINT(bugprone-exception-copy-constructor-throws,cert-err60-cpp) + +private: + const char * name() const noexcept override { return "DataLake::TableAlreadyExistsInCatalogException"; } + const char * className() const noexcept override { return "DataLake::TableAlreadyExistsInCatalogException"; } +}; + String trim(const String & str); std::vector splitTypeArguments(const String & type_str); diff --git a/src/Databases/DataLake/DatabaseDataLake.cpp b/src/Databases/DataLake/DatabaseDataLake.cpp index 955600d0a11a..6ecd239b0c8e 100644 --- a/src/Databases/DataLake/DatabaseDataLake.cpp +++ b/src/Databases/DataLake/DatabaseDataLake.cpp @@ -131,7 +131,6 @@ namespace ErrorCodes extern const int DATALAKE_DATABASE_ERROR; extern const int CANNOT_GET_CREATE_TABLE_QUERY; extern const int LOGICAL_ERROR; - extern const int TABLE_ALREADY_EXISTS; } namespace FailPoints @@ -174,11 +173,9 @@ String getLocationSchemeForTableCreation(const std::shared_ptr getFixedStorageTypeForTableCreation(const std::shared_ptr & catalog) { if (auto storage_type = catalog->getStorageType(); storage_type.has_value()) @@ -846,10 +843,8 @@ StoragePtr DatabaseDataLake::tryGetTableImpl(const String & name, ContextPtr con void DatabaseDataLake::validateCreateTableEngine(const String & engine_name) const { - /// `Iceberg` selects its backend from the optional `disk` setting. The setting is resolved by the - /// storage factory, after this database-level validation, so it cannot be safely checked here. - /// A fixed-backend catalog must not accept it: it could create a table that the catalog reopens - /// with a different backend. + /// `Iceberg` picks its backend from the optional `disk` setting, which the storage factory resolves + /// only after this database-level validation, so a fixed-backend catalog cannot accept it here. if (engine_name == "Iceberg" && getFixedStorageTypeForTableCreation(getCatalog()).has_value()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "The generic 'Iceberg' engine is not supported for a DataLakeCatalog with a fixed storage backend. " @@ -887,9 +882,8 @@ void DatabaseDataLake::createTable( const StoragePtr & table, const ASTPtr & query) { - /// Engine-clause path: the storage's own initialization (IcebergMetadata::createInitial) - /// already wrote metadata and registered the table in the catalog; a path there that - /// registers nothing throws `TABLE_ALREADY_EXISTS` instead of returning. + /// Engine-clause path: `IcebergMetadata::createInitial` has already written the metadata and + /// registered the table; a path there that registers nothing throws instead of returning. if (table) return; @@ -991,9 +985,8 @@ void DatabaseDataLake::createTable( order_by, context_); - /// Catalogs that write the initial metadata file themselves (they get an empty `metadata_path`) must - /// honour `iceberg_metadata_compression_method` too, otherwise the native CREATE TABLE path would - /// diverge from the explicit Iceberg engine path, which applies it in `IcebergMetadata::createInitial`. + /// Catalogs that write the initial metadata file themselves (empty `metadata_path`) must honour + /// `iceberg_metadata_compression_method`, as `IcebergMetadata::createInitial` does. const auto compression_method_str = context_->getSettingsRef()[Setting::iceberg_metadata_compression_method].value; const auto compression_method = chooseCompressionMethod(compression_method_str, compression_method_str); @@ -1001,12 +994,9 @@ void DatabaseDataLake::createTable( namespace_name, table_name, /* metadata_path */ "", metadata_content, compression_method, create.if_not_exists); if (!created) { - /// `IF NOT EXISTS`, and the catalog answered that the table is already there: it is shared, so - /// another client can create the same name between the existence check in `doCreateTable` and - /// this call. Nothing was created here, and the caller must be able to tell - report it exactly - /// like the local existence check does, so that `CREATE TABLE IF NOT EXISTS ... AS SELECT` does - /// not insert the selected rows into the table the other client created. - throw Exception(ErrorCodes::TABLE_ALREADY_EXISTS, + /// `IF NOT EXISTS`, and another client registered this name between the existence check in + /// `doCreateTable` and this call. Nothing was created here, and the caller must be able to tell. + throw DataLake::TableAlreadyExistsInCatalogException( "Table {}.{} already exists in the catalog", namespace_name, table_name); } @@ -1025,8 +1015,6 @@ void DatabaseDataLake::dropTable( /// NOLINT bool purge = context_->getSettingsRef()[Setting::data_lake_delete_data_on_drop]; catalog->dropTable(namespace_name, table_name, purge, if_exists); - /// A catalog-side drop removes remote metadata and, when purge is set, can request deletion of the - /// underlying data. Log it at an operational level so accidental drops/purges leave an audit trail. LOG_INFO(log, "Dropped table {}.{} from DataLakeCatalog (purge={})", namespace_name, table_name, purge); } diff --git a/src/Databases/DataLake/DatabaseDataLake.h b/src/Databases/DataLake/DatabaseDataLake.h index ad4160f76c0f..f9b5ee1ccb29 100644 --- a/src/Databases/DataLake/DatabaseDataLake.h +++ b/src/Databases/DataLake/DatabaseDataLake.h @@ -29,7 +29,6 @@ class DatabaseDataLake final : public IDatabase, WithContext UUID getUUID() const override { return db_uuid; } bool shouldBeEmptyOnDetach() const override { return false; } - bool isRemoteDatabase() const override { return true; } bool isDatalakeCatalog() const override { return true; } bool empty() const override; diff --git a/src/Databases/DataLake/GlueCatalog.cpp b/src/Databases/DataLake/GlueCatalog.cpp index 2aa3bfe1fef3..daa153622b7f 100644 --- a/src/Databases/DataLake/GlueCatalog.cpp +++ b/src/Databases/DataLake/GlueCatalog.cpp @@ -648,8 +648,8 @@ String GlueCatalog::resolveMetadataPathFromTableLocation(const String & table_lo void GlueCatalog::createNamespaceIfNotExists(const String & namespace_name) const { - /// Check existence first: `CreateDatabase` may be denied to a principal that is still allowed to - /// create tables in a pre-provisioned namespace, so it must not be called when there is nothing to create. + /// `CreateDatabase` may be denied to a principal that is still allowed to create tables in a + /// pre-provisioned namespace, so it must not be called when there is nothing to create. Aws::Glue::Model::GetDatabaseRequest get_request; get_request.SetName(namespace_name); @@ -695,16 +695,22 @@ bool GlueCatalog::createTable( DB::ObjectStoragePtr written_metadata_storage; String written_metadata_file; - /// The initial metadata file staged below must not outlive a registration that did not happen: the - /// write is guarded by `If-None-Match: *`, so a leftover permanently blocks every retry, and when the - /// winner of an `IF NOT EXISTS` name race registered the table at another location (an explicit - /// Iceberg engine path, or a different `default_base_location`), the file is left behind in an orphan - /// directory the catalog does not point at. Only this call can have created that object - a writer - /// that lost the `If-None-Match` race returns before staging anything - so removing it is safe. + /// The initial metadata file staged below must not outlive a registration that did not happen: it is + /// written with `If-None-Match: *`, so a leftover permanently blocks every retry. Removing it is safe + /// because only this call can have created it - a writer that lost the `If-None-Match` race returns + /// before staging anything. bool registered = false; SCOPE_EXIT_SAFE({ if (!registered && written_metadata_storage) + { + LOG_INFO( + log, + "Table {}.{} was not registered in the Glue catalog, removing the staged initial metadata file {}", + namespace_name, + table_name, + written_metadata_file); written_metadata_storage->removeObjectIfExists(DB::StoredObject(written_metadata_file)); + } }); if (effective_metadata_path.empty() && metadata_content && metadata_content->has("location")) @@ -785,8 +791,7 @@ bool GlueCatalog::createTable( if (!response.IsSuccess()) { - /// `AlreadyExistsException` means someone else registered the table first. The staged metadata - /// file is removed by the scope guard, so `IF NOT EXISTS` leaves nothing behind. + /// The staged metadata file is removed by the scope guard, so `IF NOT EXISTS` leaves nothing behind. if (if_not_exists && response.GetError().GetErrorType() == Aws::Glue::GlueErrors::ALREADY_EXISTS) return false; throw DB::Exception( @@ -859,11 +864,9 @@ void GlueCatalog::dropTable(const String & namespace_name, const String & table_ "Failed to drop table {}, namespace {} is filtered by `namespaces` database parameter", table_name, namespace_name); - /// We drop only via Glue's `DeleteTable`, which removes the catalog entry but leaves the data files in - /// object storage; deleting them (the client-side purge that e.g. Iceberg's own GlueCatalog performs) is - /// not implemented here. Reject a requested purge rather than silently ignore it and orphan the data. - /// TODO: implement the client-side purge (delete the table's data/manifest/metadata files, then - /// `DeleteTable`) in a follow-up PR so `data_lake_delete_data_on_drop` can be honored for Glue. + /// Glue's `DeleteTable` removes only the catalog entry; the client-side purge of the data files is not + /// implemented, so reject a requested purge rather than silently ignore it and orphan the data. + /// TODO: implement the client-side purge so `data_lake_delete_data_on_drop` can be honored for Glue. if (purge) { if (if_exists && !existsTable(namespace_name, table_name)) diff --git a/src/Databases/DataLake/ICatalog.h b/src/Databases/DataLake/ICatalog.h index ef7d046d0544..074d03a8d1f0 100644 --- a/src/Databases/DataLake/ICatalog.h +++ b/src/Databases/DataLake/ICatalog.h @@ -197,15 +197,11 @@ class ICatalog /// Creates new table in catalog. Callers must ensure the namespace exists before /// writing any table files to storage: a catalog that shares its storage view with /// the data refuses to create a namespace over a plain directory those files create. - /// `metadata_compression_method` is the codec requested by `iceberg_metadata_compression_method`. It is - /// only meaningful for catalogs that write the initial metadata file themselves (i.e. when - /// `new_metadata_path` is empty): they must name the file `v1..metadata.json` and compress its - /// contents accordingly, exactly like `DB::Iceberg::IcebergMetadata::createInitial` does. - /// Returns `true` when this call created the table. Returns `false` only when `if_not_exists` is set - /// and the catalog answered that the table is already there - the catalog is shared, so another client - /// can win the name race after the caller checked that the table does not exist. Callers must not - /// report that as a creation: `CREATE TABLE IF NOT EXISTS ... AS SELECT` must not fill a table the - /// winner of the race created. + /// `metadata_compression_method` applies only to catalogs that write the initial metadata file + /// themselves (`new_metadata_path` is empty): they must name it `v1..metadata.json` and compress + /// it accordingly, like `DB::IcebergMetadata::createInitial` does. + /// Returns `true` if this call created the table, `false` if `if_not_exists` is set and the shared + /// catalog reported that another client had already created it. virtual bool createTable( const String & namespace_name, const String & table_name, @@ -234,7 +230,7 @@ class ICatalog Poco::JSON::Object::Ptr metadata = nullptr) const; /// Drop table from catalog. - /// If purge, the catalog is requested to also delete underlying data files. + /// If `purge`, the catalog is also asked to delete the underlying data files. virtual void dropTable(const String & namespace_name, const String & table_name, bool purge, bool if_exists) const; /// Does the catalog support transactions or anything like that? diff --git a/src/Databases/DataLake/RestCatalog.cpp b/src/Databases/DataLake/RestCatalog.cpp index 528762847e5c..16f1cfad454e 100644 --- a/src/Databases/DataLake/RestCatalog.cpp +++ b/src/Databases/DataLake/RestCatalog.cpp @@ -1660,7 +1660,8 @@ void RestCatalog::createNamespaceIfNotExists(const String & namespace_name, cons Poco::JSON::Object::Ptr request_body = new Poco::JSON::Object; request_body->set("namespace", namespaceToJSONArray(namespace_name)); - if (!location.empty()) // the caller has no namespace base to offer + /// The caller leaves `location` empty when it has no namespace base to offer. + if (!location.empty()) { Poco::JSON::Object::Ptr properties = new Poco::JSON::Object; properties->set("location", location); @@ -1679,8 +1680,7 @@ void RestCatalog::createNamespaceIfNotExists(const String & namespace_name, cons } } -/// `metadata_compression_method` is unused here: the REST server writes and names the initial metadata -/// file itself, we only send it the metadata content. +/// `metadata_compression_method` is unused: the REST server writes and names the initial metadata file itself. bool RestCatalog::createTable( const String & namespace_name, const String & table_name, @@ -1741,7 +1741,7 @@ bool RestCatalog::createTable( } catch (const DB::HTTPException & ex) { - /// The catalog answers `409` when someone else created the table first. + /// `409` means the catalog already has a table with this name. if (if_not_exists && ex.getHTTPStatus() == Poco::Net::HTTPResponse::HTTPStatus::HTTP_CONFLICT) return false; throw DB::Exception(DB::ErrorCodes::DATALAKE_DATABASE_ERROR, "Failed to create table {}", ex.displayText()); @@ -1829,6 +1829,7 @@ void RestCatalog::dropTable(const String & namespace_name, const String & table_ "Failed to drop table {}, namespace {} is filtered by `namespaces` database parameter", table_name, namespace_name); + /// Same URL shape as `createTable`, `updateMetadata` and `getTableMetadataImpl`. const std::string base_endpoint = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables" / table_name).generic_string(); const std::string endpoint = fmt::format("{}?purgeRequested={}", base_endpoint, purge ? "true" : "false"); @@ -1842,7 +1843,7 @@ void RestCatalog::dropTable(const String & namespace_name, const String & table_ } catch (const DB::HTTPException & ex) { - /// The catalog answers `404` when the table is already gone - someone else dropped it first. + /// `404` means the table is already gone. if (if_exists && ex.getHTTPStatus() == Poco::Net::HTTPResponse::HTTPStatus::HTTP_NOT_FOUND) return; throw DB::Exception(DB::ErrorCodes::DATALAKE_DATABASE_ERROR, "Failed to drop table {}", ex.displayText()); diff --git a/src/Databases/DataLake/tests/gtest_construct_table_location.cpp b/src/Databases/DataLake/tests/gtest_construct_table_location.cpp index c32b32ebfc36..5547bde1bc4e 100644 --- a/src/Databases/DataLake/tests/gtest_construct_table_location.cpp +++ b/src/Databases/DataLake/tests/gtest_construct_table_location.cpp @@ -13,8 +13,6 @@ class ConstructTableLocationTest : public ::testing::Test { }; -/// S3: bucket is the first path segment of the HTTPS-style storage_endpoint; -/// a sub-prefix after the bucket must be preserved. TEST_F(ConstructTableLocationTest, S3HttpsEndpoint) { EXPECT_EQ( @@ -32,9 +30,8 @@ TEST_F(ConstructTableLocationTest, S3RejectsEndpointWithoutBucket) DB::Exception); } -/// Virtual-hosted style cannot derive a bucket from the endpoint unambiguously (a service host, an IP host, -/// and a dotted bucket name are indistinguishable), so CREATE TABLE rejects it regardless of endpoint shape; -/// users must set `default_base_location`. +/// A virtual-hosted host cannot be split into bucket and service unambiguously, so `CREATE TABLE` rejects it +/// whatever the endpoint looks like; `default_base_location` has to be set instead. TEST_F(ConstructTableLocationTest, S3VirtualHostedIsRejected) { for (const auto * endpoint : { @@ -48,9 +45,8 @@ TEST_F(ConstructTableLocationTest, S3VirtualHostedIsRejected) DB::Exception); } -/// Azure: HTTPS-form storage_endpoint must round-trip through `setLocation`, -/// which means the constructed URI must include the `@` authority. -/// A sub-path after the container must be preserved, a trailing slash ignored. +/// The constructed Azure URI must round-trip through `setLocation`, which means it has to carry the +/// `@` authority. TEST_F(ConstructTableLocationTest, AzureHttpsEndpoint) { const String location = constructTableLocation( @@ -60,7 +56,6 @@ TEST_F(ConstructTableLocationTest, AzureHttpsEndpoint) "tbl"); EXPECT_EQ(location, "abfss://mycontainer@account.dfs.core.windows.net/ns/tbl"); - /// Verify the produced URI parses back into the expected components. TableMetadata metadata; metadata.withLocation(); metadata.setLocation(location); @@ -83,8 +78,6 @@ TEST_F(ConstructTableLocationTest, AzureHttpsEndpoint) "abfss://mycontainer@account.dfs.core.windows.net/ns/tbl"); } -/// Azure: ABFSS-form storage_endpoint (container in the authority's user-info) -/// is also accepted, with and without a sub-path. TEST_F(ConstructTableLocationTest, AzureAbfssEndpoint) { EXPECT_EQ( @@ -113,8 +106,6 @@ TEST_F(ConstructTableLocationTest, AzureRejectsEndpointWithoutContainer) DB::Exception); } -/// HDFS: the authority (host:port) must be preserved in the location URI, -/// with or without a warehouse path after it. TEST_F(ConstructTableLocationTest, HdfsPreservesAuthority) { EXPECT_EQ( @@ -125,7 +116,6 @@ TEST_F(ConstructTableLocationTest, HdfsPreservesAuthority) "hdfs://namenode:9000/ns/tbl"); } -/// `file://` URIs have an empty authority and just a local filesystem path. TEST_F(ConstructTableLocationTest, FileWithoutAuthority) { EXPECT_EQ( diff --git a/src/Databases/IDatabase.h b/src/Databases/IDatabase.h index dc35d5f6f7e7..4d8ca640dc00 100644 --- a/src/Databases/IDatabase.h +++ b/src/Databases/IDatabase.h @@ -190,8 +190,7 @@ class IDatabase : public std::enable_shared_from_this virtual bool isDatalakeCatalog() const { return false; } /// Reject an explicitly-specified table engine that is incompatible with this database, before the - /// table is created. A datalake catalog with a fixed storage backend rejects an Iceberg engine that - /// pins a different backend. Default: no-op. + /// table is created. virtual void validateCreateTableEngine(const String & /*engine_name*/) const {} /// True for databases such as `MySQL`/`PostgreSQL` whose table list lives on a remote service. @@ -325,7 +324,6 @@ class IDatabase : public std::enable_shared_from_this const ASTPtr & /*query*/); /// Delete the table from the database, drop table and delete the metadata. - /// `if_exists` is the flag of the originating query. virtual void dropTable( /// NOLINT ContextPtr /*context*/, const String & /*name*/, diff --git a/src/Interpreters/DDLWorker.cpp b/src/Interpreters/DDLWorker.cpp index 064f0774ccac..f5ca0071d204 100644 --- a/src/Interpreters/DDLWorker.cpp +++ b/src/Interpreters/DDLWorker.cpp @@ -573,7 +573,6 @@ bool DDLWorker::tryExecuteQuery(DDLTaskBase & task, const ZooKeeperPtr & zookeep try { - /// Worker-side guard checkQueryDatabasesSupportOnClusterDDL(task.query, context); auto query_context = task.makeQueryContext(context, zookeeper); diff --git a/src/Interpreters/InterpreterCreateQuery.cpp b/src/Interpreters/InterpreterCreateQuery.cpp index e8be9016c2d4..73a80be5a535 100644 --- a/src/Interpreters/InterpreterCreateQuery.cpp +++ b/src/Interpreters/InterpreterCreateQuery.cpp @@ -83,6 +83,7 @@ #include #include +#include #include #include #include @@ -1767,16 +1768,13 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create) if (!UserDefinedSQLFunctionFactory::instance().empty()) UserDefinedSQLFunctionVisitor::visit(query_ptr, getContext()); - /// Snapshot whether the user wrote an explicit ENGINE before `setEngine` fills in a default below. - /// Used to detect an engine-less `CREATE TABLE` on a `DataLakeCatalog` database. + /// Snapshot whether the user wrote an explicit `ENGINE` before `setEngine` fills in a default below. const bool engine_user_specified = create.storage && create.storage->engine; /// Capture explicit unsupported storage clauses before `setEngine` merges the source's clauses for - /// `CREATE TABLE ... AS` and makes explicit and inherited indistinguishable; rejected in the check below. - /// The clauses are unsupported on both DataLakeCatalog create paths (engine-less and explicit - /// `ENGINE = Iceberg*`). Engine SETTINGS are meaningful only with an explicit engine (they are real - /// data-lake storage settings, e.g. `iceberg_format_version`), so they are rejected only on the - /// engine-less path where they would be silently dropped. + /// `CREATE TABLE ... AS` and makes explicit and inherited indistinguishable; rejected below. + /// Engine `SETTINGS` are real data-lake storage settings (e.g. `iceberg_format_version`), so they are + /// unsupported only on the engine-less path, where they would be silently dropped. const char * datalake_unsupported_storage_clause = nullptr; if (create.storage) { @@ -1908,12 +1906,10 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create) "(got {})", datalake_unsupported_storage_clause); } - /// Both create paths persist only column names/types (plus PARTITION BY / ORDER BY) into the - /// initial Iceberg metadata, and the table is re-instantiated from the catalog on every access, - /// so any other column property or secondary structure would be silently lost. Reject them here, - /// before any catalog or storage work; `properties` also covers columns inherited via - /// `CREATE TABLE ... AS`. `DatabaseDataLake::createTable` re-validates the engine-less path - /// at the AST level. + /// Only column names and types (plus `PARTITION BY` / `ORDER BY`) reach the initial Iceberg + /// metadata, and the table is re-instantiated from the catalog on every access, so any other + /// column property would be silently lost. `properties` also covers columns inherited via + /// `CREATE TABLE ... AS`; `DatabaseDataLake::createTable` re-validates the engine-less path. for (const auto & column : properties.columns) { if (column.default_desc.expression || column.default_desc.kind != ColumnDefaultKind::Default) @@ -1965,14 +1961,21 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create) } catch (const Exception & e) { - /// A `DataLakeCatalog` is shared with other clients and nodes, so the existence check inside - /// `doCreateTable` can be stale by the time the create reaches the catalog. Both create paths - /// (engine-less and explicit `ENGINE = Iceberg*`) report that lost race as `TABLE_ALREADY_EXISTS` - /// and leave nothing behind, so `IF NOT EXISTS` means here what it means for the local check: the - /// query created nothing, and `CREATE TABLE IF NOT EXISTS ... AS SELECT` must not fill the table - /// the winner of the race created. - if (!create.if_not_exists || e.code() != ErrorCodes::TABLE_ALREADY_EXISTS || !database || !database->isDatalakeCatalog()) + /// A `DataLakeCatalog` is shared, so the existence check inside `doCreateTable` can be stale by + /// the time the create reaches the catalog. Both create paths report that lost race with + /// `TableAlreadyExistsInCatalogException` and leave nothing behind, so `IF NOT EXISTS` means what + /// it means for the local check: the query created nothing, and `AS SELECT` must not fill the + /// table the winner of the race created. + if (!create.if_not_exists || !dynamic_cast(&e) + || !database || !database->isDatalakeCatalog()) throw; + /// The query succeeds and no table appears, so record in the log why. + LOG_INFO( + getLogger("InterpreterCreateQuery"), + "CREATE TABLE IF NOT EXISTS {}.{} created nothing: {}", + backQuoteIfNeed(create.getDatabase()), + backQuoteIfNeed(create.getTable()), + e.message()); created = false; } ddl_guard.reset(); @@ -2152,17 +2155,15 @@ bool InterpreterCreateQuery::doCreateTable(ASTCreateQuery & create, auto & create_query = query_ptr->as(); if (database->isDatalakeCatalog() && !engine_user_specified) { - /// Extract partition/order from the source table if CREATE TABLE ... AS was used. if (!as_table_saved.empty()) { String as_database_name = getContext()->resolveDatabase(as_database_saved); StoragePtr as_storage = DatabaseCatalog::instance().getTable({as_database_name, as_table_saved}, getContext()); auto as_storage_metadata = as_storage->getInMemoryMetadataPtr(getContext(), false); - /// `setEngine` merged the source's engine, SETTINGS and clauses into `create_query.storage`; none - /// apply to a `DataLakeCatalog` table, so rebuild it keeping only the partition and sorting keys - /// (explicit ones take precedence, the source's are the fallback). Unsupported explicit clauses - /// were already rejected in `createTable`. + /// `setEngine` merged the source's engine, `SETTINGS` and clauses into `create_query.storage`; + /// none apply to a `DataLakeCatalog` table, so rebuild it keeping only the partition and + /// sorting keys - explicit ones take precedence, the source's are the fallback. ASTPtr partition_by; ASTPtr order_by; if (create_query.storage) @@ -2186,9 +2187,8 @@ bool InterpreterCreateQuery::doCreateTable(ASTCreateQuery & create, create_query.storage->set(create_query.storage->order_by, order_by); } - /// Ensure columns are in the query AST (mainly for `CREATE TABLE ... AS source`). `formatColumns` keeps - /// every column modifier (COMMENT, CODEC, TTL, SETTINGS, STATISTICS, defaults) so `DatabaseDataLake` - /// rejects the unsupported ones rather than silently dropping them. + /// Ensure the columns are in the query AST (mainly for `CREATE TABLE ... AS source`). `formatColumns` + /// keeps every column modifier, so `DatabaseDataLake` can reject the unsupported ones. if (!create_query.columns_list || !create_query.columns_list->columns || create_query.columns_list->columns->children.empty()) diff --git a/src/Interpreters/InterpreterDropQuery.cpp b/src/Interpreters/InterpreterDropQuery.cpp index ab397bf4b326..ffcf50fef94c 100644 --- a/src/Interpreters/InterpreterDropQuery.cpp +++ b/src/Interpreters/InterpreterDropQuery.cpp @@ -350,8 +350,8 @@ BlockIO InterpreterDropQuery::executeToTableImpl(const ContextPtr & context_, AS bool check_loading_deps = !check_ref_deps && getContext()->getSettingsRef()[Setting::check_table_dependencies]; DatabaseCatalog::instance().checkTableCanBeRemovedOrRenamed(table_id, check_ref_deps, check_loading_deps, is_drop_or_detach_database); - /// The actual data removal can happen in a background thread long after this query has - /// finished, so let the storage capture whatever it needs from the query context now. + /// `drop` can run in a background thread long after this query, so let the storage capture + /// what it needs from the query context now. table->prepareForDrop(context_); table->flushAndShutdown(true); diff --git a/src/Interpreters/executeDDLQueryOnCluster.cpp b/src/Interpreters/executeDDLQueryOnCluster.cpp index d4ad5bc64225..1817371d4a1f 100644 --- a/src/Interpreters/executeDDLQueryOnCluster.cpp +++ b/src/Interpreters/executeDDLQueryOnCluster.cpp @@ -53,7 +53,7 @@ extern const int LOGICAL_ERROR; void checkQueryDatabasesSupportOnClusterDDL(const ASTPtr & query_ptr, ContextPtr context) { - /// RENAME / EXCHANGE TABLE is `ASTRenameQuery` and does not derive from `ASTQueryWithTableAndOutput`; + /// `RENAME` / `EXCHANGE TABLE` is `ASTRenameQuery`, which does not derive from `ASTQueryWithTableAndOutput`; /// queries with no target table (`CREATE DATABASE`, `SYSTEM`, ...) contribute none. std::vector target_databases; const auto add_target_database = [&](String name) diff --git a/src/Interpreters/executeDDLQueryOnCluster.h b/src/Interpreters/executeDDLQueryOnCluster.h index f2b4bc2e5a00..822cdd84c29d 100644 --- a/src/Interpreters/executeDDLQueryOnCluster.h +++ b/src/Interpreters/executeDDLQueryOnCluster.h @@ -25,10 +25,10 @@ using DatabasePtr = std::shared_ptr; /// Returns true if provided ALTER type can be executed ON CLUSTER bool isSupportedAlterTypeForOnClusterDDLQuery(int type); -/// Throws if DDL on the database's tables cannot be mutated with ON CLUSTER +/// Throws if DDL against this database's tables does not support `ON CLUSTER`. void checkDatabaseSupportsOnClusterDDL(const DatabasePtr & database); -/// Throws if a target database of the query is a shared `DataLakeCatalog`, which does not support `ON CLUSTER` DDL. +/// Same, for every database the query targets. void checkQueryDatabasesSupportOnClusterDDL(const ASTPtr & query_ptr, ContextPtr context); struct DDLQueryOnClusterParams diff --git a/src/Storages/IStorage.h b/src/Storages/IStorage.h index dca7097d04ed..bd4527f39065 100644 --- a/src/Storages/IStorage.h +++ b/src/Storages/IStorage.h @@ -520,10 +520,9 @@ It is currently only implemented in StorageObjectStorage. */ virtual void drop() {} - /** Called by `DROP TABLE` with the query context, while the query is still running and before the - * table is detached. `drop` itself can be executed much later in a background thread, where only - * the global context is available, so a storage that needs query- or session-level settings while - * dropping (for example `data_lake_delete_data_on_drop`) has to capture them here. + /** Called by `DROP TABLE` while the query is still running. `drop` itself can run much later in a + * background thread, where only the global context is available, so a storage that needs + * query-level settings while dropping (for example `data_lake_delete_data_on_drop`) captures them here. */ virtual void prepareForDrop(ContextPtr /* query_context */) {} diff --git a/src/Storages/ObjectStorage/DataLakes/IDataLakeMetadata.h b/src/Storages/ObjectStorage/DataLakes/IDataLakeMetadata.h index 79864179cdcc..d64e8529f0dd 100644 --- a/src/Storages/ObjectStorage/DataLakes/IDataLakeMetadata.h +++ b/src/Storages/ObjectStorage/DataLakes/IDataLakeMetadata.h @@ -287,8 +287,8 @@ class IDataLakeMetadata : boost::noncopyable throwNotImplemented("truncate"); } - /// `delete_data` is the effective `data_lake_delete_data_on_drop`, resolved by - /// `StorageObjectStorage::drop`. There is no context here: the drop runs in the background. + /// `delete_data` is what `StorageObjectStorage::drop` resolved from `data_lake_delete_data_on_drop`. + /// There is no context here: the drop runs in the background. virtual void drop(bool /* delete_data */) { } virtual ObjectStorageType getObjectStorageType() const { return ObjectStorageType::None; } diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp index 3599fc2beb3a..1f137572715d 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp @@ -879,6 +879,11 @@ void IcebergMetadata::createInitial( if (!configuration_ptr) throw Exception(ErrorCodes::LOGICAL_ERROR, "Trying to create Iceberg table, but storage configuration is expired"); + String namespace_name; + String table_name; + if (catalog) + std::tie(namespace_name, table_name) = DataLake::parseTableName(table_id_.getTableName()); + std::vector metadata_files; try { @@ -890,13 +895,22 @@ void IcebergMetadata::createInitial( } if (!metadata_files.empty()) { - /// Without a catalog `IF NOT EXISTS` attaches to the metadata that is already there. With a - /// catalog nothing gets registered here, so success would report an invisible table; - /// `InterpreterCreateQuery` turns `TABLE_ALREADY_EXISTS` into an `IF NOT EXISTS` no-op. - if (if_not_exists && !catalog) - return; - throw Exception( - ErrorCodes::TABLE_ALREADY_EXISTS, "Iceberg table with path {} already exists", configuration_ptr->getPathForRead().path); + /// Without a catalog `IF NOT EXISTS` attaches to the metadata already there. With a catalog + /// nothing gets registered, so success would report an invisible table; the thrown + /// `TableAlreadyExistsInCatalogException` becomes the `IF NOT EXISTS` no-op instead. + if (!catalog) + { + if (if_not_exists) + return; + throw Exception( + ErrorCodes::TABLE_ALREADY_EXISTS, "Iceberg table with path {} already exists", configuration_ptr->getPathForRead().path); + } + throw DataLake::TableAlreadyExistsInCatalogException( + "The catalog has no table {}.{} registered, but Iceberg metadata files are already present at {}, " + "so creating the table there would clash with them. This is usually left behind by a previous " + "`DROP TABLE` without `data_lake_delete_data_on_drop`, which keeps the data and metadata in " + "place: remove the leftover files, or create the table at a different location", + namespace_name, table_name, configuration_ptr->getPathForRead().path); } String location_path = configuration_ptr->getRawPath().path; @@ -926,9 +940,8 @@ void IcebergMetadata::createInitial( } catch (const Exception & e) { - /// The write uses `If-None-Match: *`, so S3 returns PreconditionFailed when the metadata file - /// already exists (e.g. leftover data after `DROP TABLE` with `data_lake_delete_data_on_drop` off, - /// or a concurrent creation). When `IF NOT EXISTS` was specified, this is expected. + /// The write uses `If-None-Match: *`, so S3 answers `PreconditionFailed` when the metadata + /// file is already there: leftovers from an earlier drop, or a concurrent creation. const bool precondition_failed = (e.code() == ErrorCodes::S3_ERROR && e.message().contains("PreconditionFailed")) || e.code() == ErrorCodes::FILE_ALREADY_EXISTS; @@ -937,8 +950,12 @@ void IcebergMetadata::createInitial( /// As in the `metadata_files` probe above: with a catalog nothing was registered. if (!catalog) return; - throw Exception( - ErrorCodes::TABLE_ALREADY_EXISTS, "Iceberg table with path {} already exists", configuration_ptr->getPathForRead().path); + throw DataLake::TableAlreadyExistsInCatalogException( + "The catalog has no table {}.{} registered, but Iceberg metadata files are already present at {}, " + "so creating the table there would clash with them. This is usually left behind by a previous " + "`DROP TABLE` without `data_lake_delete_data_on_drop`, which keeps the data and metadata in " + "place: remove the leftover files, or create the table at a different location", + namespace_name, table_name, configuration_ptr->getPathForRead().path); } throw; } @@ -952,33 +969,48 @@ void IcebergMetadata::createInitial( writeMessageToFile("1", version_hint_path, object_storage, local_context, "*", ""); filename_version_hint = version_hint_path; } - - if (catalog) - { - auto catalog_filename = configuration_ptr->getTypeName() + "://" + configuration_ptr->getNamespace() + "/" - + configuration_ptr->getRawPath().path + "metadata/" + metadata_file_name; - const auto & [namespace_name, table_name] = DataLake::parseTableName(table_id_.getTableName()); - if (!catalog->createTable(namespace_name, table_name, catalog_filename, metadata_content_object, compression_method, if_not_exists)) - { - /// `IF NOT EXISTS`, and another client registered this table first. `TABLE_ALREADY_EXISTS` - /// lets `InterpreterCreateQuery` tell that nothing was created, so `CREATE TABLE IF NOT - /// EXISTS ... AS SELECT` does not fill the other client's table. - throw Exception(ErrorCodes::TABLE_ALREADY_EXISTS, - "Table {}.{} already exists in the catalog", namespace_name, table_name); - } - } } catch (...) { - /// A leftover `.metadata.json` makes the next `CREATE` report an existing table although nothing - /// was registered in the catalog, so the rollback fails closed: a failed removal propagates in - /// place of the original exception, which is logged here. + /// Nothing is registered in the catalog yet, so removing the files we just wrote loses nothing, + /// and a leftover `.metadata.json` would make the next `CREATE` report an existing table. The + /// rollback fails closed: a failed removal propagates in place of the original exception, which + /// is logged here. tryLogCurrentException(__PRETTY_FUNCTION__, "Removing the files of the Iceberg table that failed to be created"); object_storage->removeObjectIfExists(StoredObject(filename)); if (!filename_version_hint.empty()) object_storage->removeObjectIfExists(StoredObject(filename_version_hint)); throw; } + + if (catalog) + { + auto catalog_filename = configuration_ptr->getTypeName() + "://" + configuration_ptr->getNamespace() + "/" + + configuration_ptr->getRawPath().path + "metadata/" + metadata_file_name; + + /// The registration sits outside any rollback: a failed `createTable` is ambiguous, because the + /// HTTP layer retries connection failures and the request may have succeeded with only its response + /// lost. Removing the metadata file would then leave the catalog pointing at an object that is gone, + /// with no earlier version to fall back to; an orphaned file is recoverable, so it stays. + if (!catalog->createTable(namespace_name, table_name, catalog_filename, metadata_content_object, compression_method, if_not_exists)) + { + /// Unlike an exception, this answer is definitive: the catalog did not create the table, so the + /// files we wrote are ours to remove - a transactional catalog's file name carries our own table + /// UUID, and otherwise the `If-None-Match: *` write already proved that we created them. + LOG_INFO( + getLogger("IcebergMetadata"), + "Table {}.{} was registered in the catalog by another client, removing the initial metadata file {} " + "written by this `CREATE`", + namespace_name, + table_name, + filename); + object_storage->removeObjectIfExists(StoredObject(filename)); + if (!filename_version_hint.empty()) + object_storage->removeObjectIfExists(StoredObject(filename_version_hint)); + throw DataLake::TableAlreadyExistsInCatalogException( + "Table {}.{} already exists in the catalog", namespace_name, table_name); + } + } } Iceberg::IcebergDataSnapshotPtr IcebergMetadata::getRelevantDataSnapshotFromTableStateSnapshot( diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp index 8cb26df67f69..3112fb0af552 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp @@ -376,15 +376,17 @@ bool writeMetadataFileAndVersionHint( } else { - /// Remove the metadata file written above, otherwise the resolver could later - /// pick this uncommitted file as the latest version. - /// - /// We deliberately do NOT swallow exceptions here. Returning `false` tells the caller - /// the write did not commit and is safe to retry; but if the removal throws, the - /// just-written `vN-.metadata.json` may still be present, and a subsequent - /// version-hint resolution could pick it as the latest version - exactly the race this - /// code guards against. Fail closed by letting the exception propagate so the operation - /// is reported as failed instead of silently leaving uncommitted metadata behind. + /// Remove the metadata file written above, otherwise version-hint resolution could later + /// pick this uncommitted file as the latest version. Deliberately not wrapped in a + /// try/catch: a failed removal must fail the whole operation instead of returning a + /// retriable `false` while the uncommitted `vN-.metadata.json` is still there. + LOG_INFO( + getLogger("IcebergMetadataFileWriter"), + "Removing the uncommitted Iceberg metadata file {}: the version hint is already at version {}, " + "at or past version {} this commit tried to write, so the write did not commit", + storage_metadata_path, + old_version, + metadata_file_info.version); object_storage->removeObjectIfExists(StoredObject(storage_metadata_path)); return false; } @@ -803,9 +805,7 @@ static Poco::JSON::Object::Ptr getPartitionField( { if (!param.has_value()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "TRUNCATE function for iceberg partitioning requires one integer parameter"); - /// The Iceberg spec requires the truncate width to be a positive integer; otherwise we would - /// serialize an invalid `truncate[N]` transform that some catalogs (e.g. Glue) happily register, - /// leaving an unreadable table. + /// The Iceberg spec requires the truncate width to be a positive integer. if (*param <= 0) throw Exception(ErrorCodes::BAD_ARGUMENTS, "TRUNCATE function for iceberg partitioning requires a positive width, got {}", *param); result->set(Iceberg::f_transform, fmt::format("truncate[{}]", *param)); @@ -815,9 +815,7 @@ static Poco::JSON::Object::Ptr getPartitionField( { if (!param.has_value()) throw Exception(ErrorCodes::BAD_ARGUMENTS, "BUCKET function for iceberg partitioning requires one integer parameter"); - /// The Iceberg spec requires the number of buckets to be a positive integer; otherwise we would - /// serialize an invalid `bucket[N]` transform that some catalogs (e.g. Glue) happily register, - /// leaving an unreadable table. + /// The Iceberg spec requires the number of buckets to be a positive integer. if (*param <= 0) throw Exception(ErrorCodes::BAD_ARGUMENTS, "BUCKET function for iceberg partitioning requires a positive number of buckets, got {}", *param); result->set(Iceberg::f_transform, fmt::format("bucket[{}]", *param)); diff --git a/src/Storages/ObjectStorage/StorageObjectStorage.cpp b/src/Storages/ObjectStorage/StorageObjectStorage.cpp index 093980c4e50f..c2d383909299 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorage.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorage.cpp @@ -852,19 +852,29 @@ void StorageObjectStorage::truncate( void StorageObjectStorage::prepareForDrop(ContextPtr query_context) { - /// `drop` is executed in the background, when the query context is already gone, so the setting - /// has to be read while the `DROP TABLE` query is still running. A per-query or per-session - /// `data_lake_delete_data_on_drop` would be lost otherwise. + /// `drop` runs in the background, when the query context is gone, so a per-query or per-session + /// `data_lake_delete_data_on_drop` has to be read while the `DROP TABLE` query is still running. delete_data_on_drop = query_context->getSettingsRef()[Setting::data_lake_delete_data_on_drop]; } void StorageObjectStorage::drop() { - /// We cannot use query context here, because drop is executed in the background. `prepareForDrop` - /// captured it for us if this drop came from a `DROP TABLE` query; otherwise the server-level - /// value applies. - const bool delete_data = delete_data_on_drop.load().value_or( - Context::getGlobalContextInstance()->getSettingsRef()[Setting::data_lake_delete_data_on_drop]); + /// No query context here, because `drop` runs in the background. `prepareForDrop` captured the value + /// if this drop came from a `DROP TABLE` query; without a capture we keep the data, because deleting + /// it is irreversible while an orphaned data directory can still be removed later. + const std::optional captured_delete_data = delete_data_on_drop.load(); + const bool delete_data = captured_delete_data.value_or(false); + + if (!captured_delete_data + && Context::getGlobalContextInstance()->getSettingsRef()[Setting::data_lake_delete_data_on_drop]) + { + LOG_WARNING( + log, + "Keeping the data of table {} although `data_lake_delete_data_on_drop` is enabled server-wide: the value for this drop " + "could not be captured, which happens when the table was never loaded, and data is never deleted on a fallback path. " + "Access the table before dropping it, so that the settings of the `DROP TABLE` query reach the table.", + storage_id.getNameForLogs()); + } if (catalog) { diff --git a/src/Storages/ObjectStorage/StorageObjectStorage.h b/src/Storages/ObjectStorage/StorageObjectStorage.h index a40fd5652a6b..cefaecd4d839 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorage.h +++ b/src/Storages/ObjectStorage/StorageObjectStorage.h @@ -264,10 +264,8 @@ class StorageObjectStorage : public IStorage, public IBackgroundOperation BackgroundJobsAssignee background_operations_assignee; /// `data_lake_delete_data_on_drop` as it was set for the `DROP TABLE` query, captured by - /// `prepareForDrop` because `drop` runs without a query context. Stays empty when the drop does - /// not come from a `DROP TABLE` query (the rollback of a failed `CREATE`, or a lazily loaded - /// table that is only materialized inside `drop`); `drop` then falls back to the server-level - /// value, which is what this code read before the query-level capture existed. + /// `prepareForDrop` because `drop` runs without a query context. Stays empty when the drop does not + /// come from a `DROP TABLE` query, and `drop` then keeps the data rather than guessing. std::atomic> delete_data_on_drop; }; diff --git a/src/Storages/ObjectStorage/StorageObjectStorageConfiguration.h b/src/Storages/ObjectStorage/StorageObjectStorageConfiguration.h index 7ffc653d75db..543683246cd8 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageConfiguration.h +++ b/src/Storages/ObjectStorage/StorageObjectStorageConfiguration.h @@ -338,8 +338,8 @@ class StorageObjectStorageConfiguration return true; } - /// `delete_data` is the effective `data_lake_delete_data_on_drop`, resolved by - /// `StorageObjectStorage::drop`. There is no context here: the drop runs in the background. + /// `delete_data` is what `StorageObjectStorage::drop` resolved from `data_lake_delete_data_on_drop`. + /// There is no context here: the drop runs in the background. virtual void drop(bool /* delete_data */) {} virtual bool isBackgroundExecutable() const diff --git a/src/Storages/StorageProxy.h b/src/Storages/StorageProxy.h index 96d0f5dddd06..c78ae3f67f68 100644 --- a/src/Storages/StorageProxy.h +++ b/src/Storages/StorageProxy.h @@ -84,10 +84,8 @@ class StorageProxy : public IStorage return getNested()->write(query, metadata_snapshot, context, async_insert); } - /// `prepareForDrop` is deliberately NOT forwarded alongside this: it runs on the query thread, so - /// resolving the nested storage there would turn a resolution failure into a failed `DROP TABLE`, - /// while `drop` resolves it in the background where the failure is only logged. The proxies that - /// can forward it without resolving (`StorageTableProxy`, `StorageTableFunctionProxy`) do so. + /// `prepareForDrop` is not forwarded here; only the proxies that hold a nested storage + /// (`StorageTableProxy`, `StorageTableFunctionProxy`) forward it, and only once it is resolved. void drop() override { getNested()->drop(); } void truncate( diff --git a/src/Storages/StorageTableProxy.h b/src/Storages/StorageTableProxy.h index a6fbdb2e121c..8c481b7c621c 100644 --- a/src/Storages/StorageTableProxy.h +++ b/src/Storages/StorageTableProxy.h @@ -82,9 +82,8 @@ class StorageTableProxy final : public StorageProxy void prepareForDrop(ContextPtr query_context) override { std::lock_guard lock{nested_mutex}; - /// A table that has not been loaded yet is materialized by `drop` below, and forcing the load - /// here would move a load failure from the background drop onto the user's query. Such a table - /// is dropped with the server-level settings, as it was before this hook existed. + /// Forwarded only to an already-resolved nested storage. A storage that receives no capture must + /// not infer a destructive action from a server-level default (see `StorageObjectStorage::drop`). if (nested) nested->prepareForDrop(query_context); } diff --git a/tests/integration/test_database_glue/test.py b/tests/integration/test_database_glue/test.py index d03c94e3691c..c2b8bb2342ea 100644 --- a/tests/integration/test_database_glue/test.py +++ b/tests/integration/test_database_glue/test.py @@ -725,12 +725,9 @@ def test_create(started_cluster): def test_native_create_gzip_metadata(started_cluster): - # The native `DataLakeCatalog` CREATE TABLE path (no engine clause) does not go through - # `IcebergMetadata::createInitial`: `DatabaseDataLake::createTable` hands an empty metadata path to - # the catalog, and `GlueCatalog::createTable` writes and registers the first metadata file itself. - # That branch must honour `iceberg_metadata_compression_method` just like the explicit Iceberg - # engine path does, otherwise the same CREATE TABLE query would produce differently compressed - # metadata depending on which create path served it. + # The engine-less `CREATE TABLE` path does not go through `IcebergMetadata::createInitial`: + # `GlueCatalog::createTable` writes and registers the first metadata file itself, and must honour + # `iceberg_metadata_compression_method` just like the explicit engine path (`test_create_gzip_metadata`). node = started_cluster.instances["node1"] test_ref = f"test_native_create_gzip_{uuid.uuid4()}" @@ -763,7 +760,6 @@ def test_native_create_gzip_metadata(started_cluster): metadata_bytes = started_cluster.minio_client.get_object(bucket, key).read() assert metadata_bytes[:2] == b"\x1f\x8b", metadata_bytes[:16] - # Reopen through the catalog (fresh database) and confirm the table is readable and writable. create_clickhouse_glue_database(started_cluster, node, CATALOG_NAME) assert node.query(f"SELECT count() FROM {CATALOG_NAME}.`{root_namespace}.{table_name}`") == "0\n" node.query( diff --git a/tests/integration/test_database_iceberg/test.py b/tests/integration/test_database_iceberg/test.py index a30c0b8b469c..9579c2bba6ff 100644 --- a/tests/integration/test_database_iceberg/test.py +++ b/tests/integration/test_database_iceberg/test.py @@ -1065,7 +1065,7 @@ def test_on_cluster_ddl_rejected_for_datalake_catalog(started_cluster): tables = {ident[-1] for ident in catalog.list_tables(namespace)} assert table_name not in tables, f"table must not have been created in the shared catalog: {tables}" finally: - # Restore the catalog database on node2 for subsequent tests, even if an assertion above failed. + # Restore the catalog database on node2 for the tests that follow. create_clickhouse_iceberg_database(started_cluster, node2, CATALOG_NAME) @@ -1253,7 +1253,7 @@ def test_create_table_as(started_cluster): f"DROP TABLE IF EXISTS {CATALOG_NAME}.`{namespace}.{table}` SETTINGS allow_database_iceberg=1" ) - # Intentional: CTAS must work from a source with a functional partition key. + # CTAS must work from a source with a functional partition key. node.query( f""" CREATE TABLE default.{src_table} @@ -1275,7 +1275,6 @@ def test_create_table_as(started_cluster): """ ) - # CTAS with explicit PARTITION BY / ORDER BY overriding the source table. node.query( f""" CREATE TABLE {CATALOG_NAME}.`{namespace}.override` @@ -1454,7 +1453,7 @@ def test_create_table_nested_namespace(started_cluster): write_settings = {"allow_insert_into_iceberg": 1, "write_full_path_in_iceberg_metadata": 1} - # INSERT exercises RestCatalog::updateMetadata for the nested namespace. + # INSERT exercises `RestCatalog::updateMetadata` for the nested namespace. node.query( f"INSERT INTO {CATALOG_NAME}.`{namespace}.nested` VALUES (1);", settings=write_settings, @@ -1464,7 +1463,7 @@ def test_create_table_nested_namespace(started_cluster): settings={"allow_database_iceberg": 1}, ).strip() == "1" - # ALTER exercises RestCatalog::updateSchema for the nested namespace. + # ALTER exercises `RestCatalog::updateSchema` for the nested namespace. node.query( f"ALTER TABLE {CATALOG_NAME}.`{namespace}.nested` ADD COLUMN z Nullable(String);", settings=write_settings, @@ -1582,10 +1581,9 @@ def test_create_table_with_engine_unsupported_clauses(started_cluster): f"'{minio_access_key}', '{minio_secret_key}')" ) - # The explicit-ENGINE path persists only column names/types, PARTITION BY, and ORDER BY into the - # initial Iceberg metadata, so unsupported storage clauses must be rejected there too, not silently - # dropped. Unlike the engine-less path, engine SETTINGS stay allowed: they are real data-lake - # storage settings (e.g. `iceberg_format_version`) used during creation. + # The explicit-ENGINE path persists only column names/types, `PARTITION BY` and `ORDER BY` into the + # initial metadata, so unsupported storage clauses must be rejected there too. Engine `SETTINGS` stay + # allowed, unlike on the engine-less path: they are real data-lake settings used during creation. for clause in [ "PRIMARY KEY id", "ORDER BY id SAMPLE BY id", @@ -1690,9 +1688,9 @@ def test_create_table_invalid_partition_transforms(started_cluster): ) assert "good" in [t[1] for t in catalog.list_tables(namespace)] - # Invalid transform parameters must be rejected before any catalog metadata is written. - # Otherwise CREATE TABLE could register an unreadable Iceberg table whose partition spec - # serializes as bucket[0], bucket[-1], or truncate[0]. + # Invalid transform parameters must be rejected before any catalog metadata is written, or `CREATE TABLE` + # would register an unreadable table whose partition spec serializes as `bucket[0]`, `bucket[-1]` + # or `truncate[0]`. for i, transform in enumerate( ["icebergBucket(0, id)", "icebergBucket(-1, id)", "icebergTruncate(0, id)"] ): @@ -1738,9 +1736,8 @@ def test_create_table_namespace_location(started_cluster): table_location = catalog.load_table(f"{namespace}.first").location().rstrip("/") ns_location = catalog.load_namespace_properties(namespace).get("location") - # The namespace default location must point at the namespace base, not at the first table's - # directory. Otherwise another client creating a table in the same namespace without an explicit - # location could be placed under the first table's path. + # The namespace default location must point at the namespace base, not at the first table's directory, + # or later tables created without an explicit location would land under that first table. assert ns_location is not None, "namespace is missing its location property" ns_location = ns_location.rstrip("/") assert ns_location != table_location, ( @@ -1777,9 +1774,8 @@ def test_create_table_with_engine_namespace_location(started_cluster): assert table_location.endswith(table_dir), table_location # The engine path here is `//`, which does not follow `//
`, - # so there is no namespace base to derive. The table's own directory must not be registered as the - # namespace default location: another client creating a table in the same namespace without an - # explicit location would then be placed under this table's directory. + # so there is no namespace base to derive. The table's own directory must not become the namespace + # default location, or later tables in this namespace would land inside it. ns_location = catalog.load_namespace_properties(namespace).get("location") if ns_location is not None: ns_location = ns_location.rstrip("/") @@ -1791,9 +1787,8 @@ def test_create_table_with_engine_namespace_location(started_cluster): settings={"allow_database_iceberg": 1}, ) - # When the engine path does follow `//
`, the namespace base is unambiguous - # and is registered as the namespace default location, so tables created later by other clients - # land next to this one instead of inside it. + # When the engine path does follow `//
`, the namespace base is unambiguous and + # is registered as the namespace default location, so later tables land next to this one, not inside it. base_dir = f"engine_ns_base_{uuid.uuid4().hex[:8]}" nested_namespace = f"test_ns_engine_derived_{uuid.uuid4().hex[:8]}" node.query( diff --git a/tests/integration/test_storage_iceberg_no_spark/test_drop_delete_data_on_drop.py b/tests/integration/test_storage_iceberg_no_spark/test_drop_delete_data_on_drop.py index 9ad8c45838e1..449e80dcecf3 100644 --- a/tests/integration/test_storage_iceberg_no_spark/test_drop_delete_data_on_drop.py +++ b/tests/integration/test_storage_iceberg_no_spark/test_drop_delete_data_on_drop.py @@ -20,10 +20,9 @@ def _files_left(instance, table_name): def test_drop_honours_query_level_delete_data_on_drop( started_cluster_iceberg_no_spark, storage_type, delete_data_on_drop ): - # `StorageObjectStorage::drop` runs in a background thread, where the query context is already - # gone. A query-level `data_lake_delete_data_on_drop` still has to reach it, because it is - # captured by `IStorage::prepareForDrop` while the `DROP TABLE` query is running. Without that - # capture the setting would only ever work when set server-wide in the default profile. + # `StorageObjectStorage::drop` runs in a background thread, where the query context is already gone. + # A query-level `data_lake_delete_data_on_drop` reaches it only because `IStorage::prepareForDrop` + # captures it while the `DROP TABLE` query is still running. instance = started_cluster_iceberg_no_spark.instances["node1"] table_name = f"test_delete_data_on_drop_{delete_data_on_drop}_{get_uuid_str()}" From ef588a60963c7b8d13055f95aa7df0562ef7cc97 Mon Sep 17 00:00:00 2001 From: Andrey Zvonov <32552679+zvonand@users.noreply.github.com> Date: Thu, 3 Sep 2026 11:58:14 +0200 Subject: [PATCH 3/4] Delete the data of a dropped Iceberg table on bucket-relative paths `IcebergMetadata::drop` passed `table_path` to `listFiles` as both the path and the prefix, and `listFiles` joins the two, so it listed `table_path/table_path`. The listing came back empty and `data_lake_delete_data_on_drop` deleted nothing whenever the table path is relative to the bucket root, as it is for `IcebergS3`. It only worked for `IcebergLocal`, whose path is absolute and therefore replaces the joined prefix, which is also the only case the test covered. Run `test_drop_honours_query_level_delete_data_on_drop` over `s3` too. (cherry picked from commit 9a4e9cfb748af5e981df0752b8512f435857d61f) --- .../test_drop_delete_data_on_drop.py | 42 +++++++++++++------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/tests/integration/test_storage_iceberg_no_spark/test_drop_delete_data_on_drop.py b/tests/integration/test_storage_iceberg_no_spark/test_drop_delete_data_on_drop.py index 449e80dcecf3..5487851202fb 100644 --- a/tests/integration/test_storage_iceberg_no_spark/test_drop_delete_data_on_drop.py +++ b/tests/integration/test_storage_iceberg_no_spark/test_drop_delete_data_on_drop.py @@ -3,20 +3,36 @@ from helpers.iceberg_utils import create_iceberg_table, get_uuid_str -def _table_dir(table_name): - return f"/var/lib/clickhouse/user_files/iceberg_data/default/{table_name}" +def _table_path(table_name): + return f"var/lib/clickhouse/user_files/iceberg_data/default/{table_name}" -def _files_left(instance, table_name): - return int( - instance.exec_in_container( - ["bash", "-c", f"find {_table_dir(table_name)} -type f 2>/dev/null | wc -l"] - ).strip() +def _files_left(cluster, storage_type, table_name): + if storage_type == "local": + return int( + cluster.instances["node1"] + .exec_in_container( + [ + "bash", + "-c", + f"find /{_table_path(table_name)} -type f 2>/dev/null | wc -l", + ] + ) + .strip() + ) + + # The S3 table path is relative to the bucket root, unlike the absolute local one. + return len( + list( + cluster.minio_client.list_objects( + cluster.minio_bucket, f"{_table_path(table_name)}/", recursive=True + ) + ) ) @pytest.mark.parametrize("delete_data_on_drop", [0, 1]) -@pytest.mark.parametrize("storage_type", ["local"]) +@pytest.mark.parametrize("storage_type", ["local", "s3"]) def test_drop_honours_query_level_delete_data_on_drop( started_cluster_iceberg_no_spark, storage_type, delete_data_on_drop ): @@ -24,7 +40,7 @@ def test_drop_honours_query_level_delete_data_on_drop( # A query-level `data_lake_delete_data_on_drop` reaches it only because `IStorage::prepareForDrop` # captures it while the `DROP TABLE` query is still running. instance = started_cluster_iceberg_no_spark.instances["node1"] - table_name = f"test_delete_data_on_drop_{delete_data_on_drop}_{get_uuid_str()}" + table_name = f"test_delete_data_on_drop_{storage_type}_{delete_data_on_drop}_{get_uuid_str()}" create_iceberg_table( storage_type, @@ -35,7 +51,7 @@ def test_drop_honours_query_level_delete_data_on_drop( ) instance.query(f"INSERT INTO {table_name} VALUES ('123', 1)") assert instance.query(f"SELECT * FROM {table_name} ORDER BY ALL") == "123\t1\n" - assert _files_left(instance, table_name) > 0 + assert _files_left(started_cluster_iceberg_no_spark, storage_type, table_name) > 0 instance.query( f"DROP TABLE {table_name} SYNC", @@ -43,6 +59,8 @@ def test_drop_honours_query_level_delete_data_on_drop( ) if delete_data_on_drop: - assert _files_left(instance, table_name) == 0 + assert ( + _files_left(started_cluster_iceberg_no_spark, storage_type, table_name) == 0 + ) else: - assert _files_left(instance, table_name) > 0 + assert _files_left(started_cluster_iceberg_no_spark, storage_type, table_name) > 0 From 7809d87ff19fbda330d3238b289f338c146bb533 Mon Sep 17 00:00:00 2001 From: Andrey Zvonov <32552679+zvonand@users.noreply.github.com> Date: Thu, 3 Sep 2026 18:23:02 +0200 Subject: [PATCH 4/4] Fix `DROP TABLE` data deletion and the namespace default location --- src/Databases/DataLake/DatabaseDataLake.cpp | 10 ++++++++ src/Databases/DataLake/GlueCatalog.cpp | 4 +--- src/Databases/DataLake/GlueCatalog.h | 4 +++- src/Databases/DataLake/ICatalog.cpp | 5 ++++ src/Databases/DataLake/ICatalog.h | 6 +++++ src/Databases/DataLake/RestCatalog.cpp | 2 -- src/Databases/DataLake/RestCatalog.h | 4 ++-- src/Databases/DataLake/S3TablesCatalog.cpp | 20 ++++++++++++++-- .../DataLakes/Iceberg/IcebergMetadata.cpp | 24 +++++++++++++++++++ .../StorageObjectStorageCluster.cpp | 9 +++++++ .../StorageObjectStorageCluster.h | 2 ++ 11 files changed, 80 insertions(+), 10 deletions(-) diff --git a/src/Databases/DataLake/DatabaseDataLake.cpp b/src/Databases/DataLake/DatabaseDataLake.cpp index 6ecd239b0c8e..6a44e3d1a103 100644 --- a/src/Databases/DataLake/DatabaseDataLake.cpp +++ b/src/Databases/DataLake/DatabaseDataLake.cpp @@ -990,6 +990,16 @@ void DatabaseDataLake::createTable( const auto compression_method_str = context_->getSettingsRef()[Setting::iceberg_metadata_compression_method].value; const auto compression_method = chooseCompressionMethod(compression_method_str, compression_method_str); + /// Register the namespace before `createTable`, which requires it to exist and, for catalogs that + /// write the initial metadata file themselves, must not be preceded by any file written to storage + /// (see `ICatalog::createTable`). Do it after all local validation, so a rejected `CREATE` leaves no + /// trace in the catalog. The namespace's default location must be the namespace base, not this first + /// table's directory, or later tables created without an explicit location would land under it. + String namespace_location = location; + if (const String table_suffix = "/" + table_name; namespace_location.ends_with(table_suffix)) + namespace_location.resize(namespace_location.size() - table_suffix.size()); + catalog->createNamespaceIfNotExists(namespace_name, namespace_location); + const bool created = catalog->createTable( namespace_name, table_name, /* metadata_path */ "", metadata_content, compression_method, create.if_not_exists); if (!created) diff --git a/src/Databases/DataLake/GlueCatalog.cpp b/src/Databases/DataLake/GlueCatalog.cpp index daa153622b7f..25d992448864 100644 --- a/src/Databases/DataLake/GlueCatalog.cpp +++ b/src/Databases/DataLake/GlueCatalog.cpp @@ -646,7 +646,7 @@ String GlueCatalog::resolveMetadataPathFromTableLocation(const String & table_lo } } -void GlueCatalog::createNamespaceIfNotExists(const String & namespace_name) const +void GlueCatalog::createNamespaceIfNotExists(const String & namespace_name, const String & /*location*/) const { /// `CreateDatabase` may be denied to a principal that is still allowed to create tables in a /// pre-provisioned namespace, so it must not be called when there is nothing to create. @@ -688,8 +688,6 @@ bool GlueCatalog::createTable( "Failed to create table {}, namespace {} is filtered by `namespaces` database parameter", table_name, namespace_name); - createNamespaceIfNotExists(namespace_name); - String effective_metadata_path = new_metadata_path; DB::ObjectStoragePtr written_metadata_storage; diff --git a/src/Databases/DataLake/GlueCatalog.h b/src/Databases/DataLake/GlueCatalog.h index 27c1785b345c..913f943b98aa 100644 --- a/src/Databases/DataLake/GlueCatalog.h +++ b/src/Databases/DataLake/GlueCatalog.h @@ -101,8 +101,10 @@ class GlueCatalog final : public ICatalog, private DB::WithContext const String & column_name, const String & glue_column_type); + /// Glue namespaces carry no default location, so `location` is ignored. + void createNamespaceIfNotExists(const String & namespace_name, const String & location) const override; + private: - void createNamespaceIfNotExists(const String & namespace_name) const; std::unique_ptr glue_client; const LoggerPtr log; diff --git a/src/Databases/DataLake/ICatalog.cpp b/src/Databases/DataLake/ICatalog.cpp index 1740f1ec8a56..2eab9740d7c7 100644 --- a/src/Databases/DataLake/ICatalog.cpp +++ b/src/Databases/DataLake/ICatalog.cpp @@ -384,6 +384,11 @@ bool ICatalog::updateSchema( throw DB::Exception(DB::ErrorCodes::NOT_IMPLEMENTED, "updateSchema is not implemented"); } +void ICatalog::createNamespaceIfNotExists(const String & /*namespace_name*/, const String & /*location*/) const +{ + throw DB::Exception(DB::ErrorCodes::NOT_IMPLEMENTED, "createNamespaceIfNotExists is not implemented"); +} + void ICatalog::dropTable(const String & /*namespace_name*/, const String & /*table_name*/, bool /*purge*/, bool /*if_exists*/) const { throw DB::Exception(DB::ErrorCodes::NOT_IMPLEMENTED, diff --git a/src/Databases/DataLake/ICatalog.h b/src/Databases/DataLake/ICatalog.h index 074d03a8d1f0..6b9062122c5b 100644 --- a/src/Databases/DataLake/ICatalog.h +++ b/src/Databases/DataLake/ICatalog.h @@ -229,6 +229,12 @@ class ICatalog Int32 new_last_column_id, Poco::JSON::Object::Ptr metadata = nullptr) const; + /// Register `namespace_name` if the catalog does not have it yet. `location` is the namespace's + /// default location (its base directory), and is empty when the caller has no base to offer - a + /// table's own directory must never be used, or later tables created in the namespace without an + /// explicit location would land inside that first table. + virtual void createNamespaceIfNotExists(const String & namespace_name, const String & location) const; + /// Drop table from catalog. /// If `purge`, the catalog is also asked to delete the underlying data files. virtual void dropTable(const String & namespace_name, const String & table_name, bool purge, bool if_exists) const; diff --git a/src/Databases/DataLake/RestCatalog.cpp b/src/Databases/DataLake/RestCatalog.cpp index 16f1cfad454e..2730c0b78f19 100644 --- a/src/Databases/DataLake/RestCatalog.cpp +++ b/src/Databases/DataLake/RestCatalog.cpp @@ -1695,8 +1695,6 @@ bool RestCatalog::createTable( const String location = metadata_content->getValue("location"); - createNamespaceIfNotExists(namespace_name, location); - const std::string endpoint = (base_url / config.prefix / NAMESPACES_ENDPOINT / encodeNamespaceForURI(namespace_name) / "tables").generic_string(); Poco::JSON::Object::Ptr request_body = new Poco::JSON::Object; diff --git a/src/Databases/DataLake/RestCatalog.h b/src/Databases/DataLake/RestCatalog.h index 0ad6ce356fc0..c2c8ac2f37c9 100644 --- a/src/Databases/DataLake/RestCatalog.h +++ b/src/Databases/DataLake/RestCatalog.h @@ -112,6 +112,8 @@ class RestCatalog : public ICatalog, public DB::WithContext void dropTable(const String & namespace_name, const String & table_name, bool purge, bool if_exists) const override; + void createNamespaceIfNotExists(const String & namespace_name, const String & location) const override; + ICatalog::CredentialsRefreshCallback getCredentialsConfigurationCallback(const DB::StorageID & storage_id) override; String getClientId() const { return client_id; } @@ -129,8 +131,6 @@ class RestCatalog : public ICatalog, public DB::WithContext const std::string & namespaces_, DB::ContextPtr context_); - void createNamespaceIfNotExists(const String & namespace_name, const String & location) const; - struct Config { /// Prefix is a path of the catalog endpoint, diff --git a/src/Databases/DataLake/S3TablesCatalog.cpp b/src/Databases/DataLake/S3TablesCatalog.cpp index c1517977ede6..2aaca65d3b42 100644 --- a/src/Databases/DataLake/S3TablesCatalog.cpp +++ b/src/Databases/DataLake/S3TablesCatalog.cpp @@ -33,6 +33,7 @@ namespace DB::ErrorCodes { extern const int BAD_ARGUMENTS; extern const int DATALAKE_DATABASE_ERROR; + extern const int SUPPORT_IS_DISABLED; } namespace DB::Setting @@ -214,9 +215,24 @@ ICatalog::CredentialsRefreshCallback S3TablesCatalog::getCredentialsConfiguratio }; } -void S3TablesCatalog::dropTable( - const String & namespace_name, const String & table_name, bool /*delete_data*/, bool /*if_exists*/) const +void S3TablesCatalog::dropTable(const String & namespace_name, const String & table_name, bool delete_data, bool if_exists) const { + /// The API only offers a purging delete, so keeping the data is not expressible here; refuse rather + /// than delete data the `DROP TABLE` asked to keep. + /// https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-tables-delete.html + if (!delete_data) + { + /// A table that is not there is nothing to refuse, so `IF EXISTS` still means a no-op. + if (if_exists && !existsTable(namespace_name, table_name)) + return; + + throw DB::Exception( + DB::ErrorCodes::SUPPORT_IS_DISABLED, + "S3 Tables cannot drop table {}.{} without deleting its data, and `data_lake_delete_data_on_drop` is disabled. " + "Enable `data_lake_delete_data_on_drop` to drop the table together with its data", + namespace_name, table_name); + } + const std::string endpoint = (base_url / config.prefix / "namespaces" / namespace_name / "tables" / table_name).string() + "?purgeRequested=True"; diff --git a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp index 1f137572715d..d9e339ebc2ee 100644 --- a/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp +++ b/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp @@ -934,6 +934,30 @@ void IcebergMetadata::createInitial( : fmt::format("v1{}.metadata.json", compression_suffix); auto filename = fmt::format("{}metadata/{}", configuration_ptr->getRawPath().path, metadata_file_name); + if (catalog) + { + /// The namespace default location must be the namespace base, not this table's directory, or + /// later tables created in the same namespace without an explicit location would land under it. + /// The engine clause names an arbitrary path, so the base is only derivable when that path ends + /// with `/
`; otherwise there is no base to offer and the location stays empty. + String namespace_location = location_path; + while (namespace_location.ends_with('/')) + namespace_location.pop_back(); + + String namespace_path = namespace_name; + std::replace(namespace_path.begin(), namespace_path.end(), '.', '/'); + if (namespace_location.ends_with("/" + namespace_path + "/" + table_name)) + namespace_location.resize(namespace_location.size() - table_name.size() - 1); + else + namespace_location.clear(); + + /// Register the namespace before any file is written (but after all local validation, so a + /// rejected `CREATE` leaves no trace in the catalog): a catalog that shares its storage view + /// with the data (e.g. SeaweedFS) refuses to create a namespace over the plain directory + /// those files would leave behind. + catalog->createNamespaceIfNotExists(namespace_name, namespace_location); + } + try { writeMessageToFile(metadata_content, filename, object_storage, local_context, "*", "", compression_method); diff --git a/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp b/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp index 9eefd709aba1..7eee9e8796f3 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageCluster.cpp @@ -777,6 +777,15 @@ void StorageObjectStorageCluster::drop() IStorageCluster::drop(); } +/// `drop` forwards to `pure_storage`, so the capture `drop` reads has to land on the same object. +/// Without this the query-level `data_lake_delete_data_on_drop` never reaches the storage and the +/// data of a dropped table is kept. +void StorageObjectStorageCluster::prepareForDrop(ContextPtr query_context) +{ + if (pure_storage) + pure_storage->prepareForDrop(query_context); +} + void StorageObjectStorageCluster::dropInnerTableIfAny(bool sync, ContextPtr context) { if (getClusterName(context).empty()) diff --git a/src/Storages/ObjectStorage/StorageObjectStorageCluster.h b/src/Storages/ObjectStorage/StorageObjectStorageCluster.h index 6894bb76d2e1..fd9e92671a71 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageCluster.h +++ b/src/Storages/ObjectStorage/StorageObjectStorageCluster.h @@ -74,6 +74,8 @@ class StorageObjectStorageCluster : public IStorageCluster void drop() override; + void prepareForDrop(ContextPtr query_context) override; + void dropInnerTableIfAny(bool sync, ContextPtr context) override; void truncate(