Native CREATE TABLE / DROP TABLE for DataLakeCatalog; optionally prune on DROP

[zvonand]

Changelog category (leave one):

• Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Supports CREATE TABLE, CREATE TABLE … AS source, and DROP TABLE for DataLakeCatalog; optional catalog-side data purge can be triggered ( new database_iceberg_purge_on_drop setting).

Documentation entry for user-facing changes

• Documentation is written (mandatory for new features)

CI/CD Options

Exclude tests:

• Fast test
• Integration Tests
• Stateless tests
• Stateful tests
• Performance tests
• Aarch64 tests
• All with ASAN
• All with TSAN
• All with MSAN
• All with UBSAN
• All with Coverage
• All Regression
• Disable CI Cache

Regression jobs to run:

• Fast suites (mostly <1h)
• Aggregate Functions (2h)
• Alter (1.5h)
• Benchmark (30m)
• ClickHouse Keeper (1h)
• Iceberg (2h)
• LDAP (1h)
• OAuth (5m)
• Parquet (1.5h)
• RBAC (1.5h)
• SSL Server (1h)
• S3 (2h)
• S3 Export (2h)
• Swarms (30m)
• Tiered Storage (2h)

[github-actions]

Workflow [PR], commit [498d41e]

[alsugiliazova]

#1897

[alsugiliazova]

#1898

[alsugiliazova]

AI audit note: This review comment was generated by AI (claude-4.6-opus).

Audit update for PR #1896 (Native CREATE TABLE / DROP TABLE for DataLakeCatalog)

Confirmed defects

Medium: GlueCatalog orphans metadata file on CreateTable API failure

• Impact: Stale v1.metadata.json left on S3 if Glue CreateTable call fails after the file was written.
• Anchor: src/Databases/DataLake/GlueCatalog.cpp / createTable (the new write-then-register block)
• Trigger: GlueCatalog::createTable writes metadata to S3 successfully, but the subsequent glue_client->CreateTable(request) returns an error (e.g. AlreadyExistsException, permissions, network timeout).
• Why defect: The S3 write is not rolled back on Glue API failure; repeated retries or re-creates at the same location may find the orphaned file and misinterpret it.
• Fix direction: Wrap the Glue API call in try/catch and removeObjectIfExists the written metadata file on failure (mirroring the cleanup pattern added in Utils.cpp).
• Regression test direction: Mock Glue CreateTable to fail, verify no metadata file remains on S3.

Medium: reverseResolve produces malformed path when storage_path does not start with table_root

• Impact: Corrupted metadata-log entry in Iceberg metadata JSON, potentially causing metadata reads to fail on subsequent operations.
• Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergPath.h / reverseResolve
• Trigger: storage_path is an absolute/full URI (e.g. starts with s3://bucket/...) while table_root is a relative path — the starts_with check fails and the fallback concatenates table_location + storage_path producing a doubled URI.
• Why defect: The else-branch blindly prepends table_location without checking if storage_path is already absolute, which can produce s3://bucket/table/s3://bucket/table/metadata/v1.metadata.json.
• Fix direction: Add a check: if storage_path already starts with table_location, return it as-is without prepending.
• Regression test direction: Unit test reverseResolve with an absolute URI storage_path that doesn't match table_root.

Low: S3Tables catalog silently accepts the CREATE TABLE location-resolution path but throws a confusing error later

• Impact: User gets "createTable is not implemented" from the catalog layer rather than a clear rejection that S3Tables doesn't support CREATE TABLE from ClickHouse.
• Anchor: src/Databases/DataLake/DatabaseDataLake.cpp / getLocationSchemeForTableCreation (S3_TABLES case returns "s3") + ICatalog::createTable base throwing NOT_IMPLEMENTED
• Trigger: User runs CREATE TABLE on a database backed by S3Tables catalog type.
• Why defect: getLocationSchemeForTableCreation succeeds for S3_TABLES, location is constructed, metadata generated, then catalog->createTable throws a generic NOT_IMPLEMENTED error rather than an early, specific rejection.
• Fix direction: Either add S3_TABLES to the "throw BAD_ARGUMENTS" cases in getLocationSchemeForTableCreation, or override createTable in S3TablesCatalog with a clear message.
• Regression test direction: Integration test: attempt CREATE TABLE on an S3Tables database and assert a user-friendly error.

Coverage summary

Category	Status
Scope reviewed	InterpreterCreateQuery engine-less path, InterpreterDropQuery ON CLUSTER check, InterpreterAlterQuery ON CLUSTER check, DatabaseDataLake::createTable, DatabaseDataLake::dropTable, GlueCatalog::createTable/dropTable, RestCatalog::createTable/dropTable/namespaceToJSONArray/encodeNamespaceForURI, S3TablesCatalog::dropTable, ICatalog::storageTypeToScheme, constructTableLocation, IcebergMetadata::createInitial metadata filename, MetadataGenerator::generateNextMetadata parent_snapshot/metadata-log changes, IcebergWrites previous_metadata_file_path tracking, Mutations.cpp reverseResolve + writeMetadataFiles, Utils.cpp cleanup-on-failure, IcebergPath::reverseResolve, StorageObjectStorage::alter/checkAlterIsPossible/drop
Categories failed	Partial-update rollback (GlueCatalog metadata write), path resolution edge case (reverseResolve)
Categories passed	Concurrency/thread-safety, memory lifetime, exception safety (DDL guard RAII), integer overflow (N/A), ON CLUSTER rejection, namespace encoding, setting rename/alias, DROP purge semantics, metadata-log correctness, metadata filename UUID, test coverage
Assumptions/limits	Static analysis only; GlueCatalog AWS interactions not runtime-tested; Mutations.cpp retry logic assumed correct based on pre-existing patterns

[DimensionWieldr]

There's an export partition regression fail that looks like it's caused by this PR.

Under ice catalog, exporting multiple partitions in one ALTER does some concurrent writes. Exports are marked as COMPLETED but DataLakeCatalog read-back is short on rows. The same scenarios pass on baseline Antalya and under no_catalog (direct S3). The regression lines up with the PR’s Iceberg commit changes in IcebergWrites.cpp / MetadataGenerator.cpp: when one ALTER fires several parallel exports to the same REST-catalog table, later commits can land without chaining onto earlier snapshots’ manifests, so only the latest partition’s data is visible even though every export succeeded.

[zvonand]

@DimensionWieldr how it could've happened? this PR is not yet merged, and the latest run has not yet finished build jobs (there are no regression results yet)

[DimensionWieldr]

@DimensionWieldr how it could've happened? this PR is not yet merged, and the latest run has not yet finished build jobs (there are no regression results yet)

Sorry I should've specified. This is a fail from CI on this PR, from the previous commit. I was asked to take a look at export partition fails earlier today. Maybe the new CI run for the most recent commit will look better.

[alsugiliazova]

AI audit note: This review comment was generated by AI (claude-4.6-opus).

Audit update for PR #1896 — commit 7a5ddbf (Native CREATE TABLE / DROP TABLE for DataLakeCatalog)

Confirmed defects

Medium: Missing null-safety for current-snapshot-id in IcebergStorageSink::initializeMetadata

• Impact: Exception/crash when the first INSERT targets a table whose initial metadata has "current-snapshot-id": null (Iceberg v2 convention for tables created by external tools like Spark/PyIceberg).
• Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp line 1207
• Trigger: Insert into a table created externally with Iceberg v2 metadata where current-snapshot-id is JSON null rather than -1.
• Why defect: Mutations.cpp:379 was correctly updated to include !metadata->isNull(Iceberg::f_current_snapshot_id), but the same check in IcebergWrites.cpp was not updated — getValue<Int64> on a null Poco JSON value throws.
• Fix direction: Add && !metadata->isNull(Iceberg::f_current_snapshot_id) to the guard at line 1207 of IcebergWrites.cpp.
• Regression test direction: Create a table externally with "current-snapshot-id": null, then INSERT from ClickHouse.
• Note: This is pre-existing on the base branch, but the PR fixed it in Mutations.cpp and should fix it here too for consistency.

Low: S3Tables catalog silently accepts the CREATE TABLE location-resolution path but throws a confusing error later

• Impact: User gets "createTable is not implemented" from the catalog layer rather than a clear rejection that S3Tables doesn't support CREATE TABLE from ClickHouse.
• Anchor: src/Databases/DataLake/DatabaseDataLake.cpp / getLocationSchemeForTableCreation (S3_TABLES case returns "s3") + ICatalog::createTable base throwing NOT_IMPLEMENTED
• Trigger: User runs CREATE TABLE on a database backed by S3Tables catalog type.
• Why defect: getLocationSchemeForTableCreation succeeds for S3_TABLES, location is constructed, metadata generated, then catalog->createTable throws a generic NOT_IMPLEMENTED error rather than an early, specific rejection.
• Fix direction: Either add S3_TABLES to the "throw BAD_ARGUMENTS" cases in getLocationSchemeForTableCreation, or override createTable in S3TablesCatalog with a clear message.
• Regression test direction: Attempt CREATE TABLE on an S3Tables database and assert a user-friendly error.

Resolved from previous audit (fixed in commit 7a5ddbf)

1. GlueCatalog orphans metadata file on CreateTable API failure — Fixed with SCOPE_EXIT + registered flag. Metadata file is now cleaned up if the Glue API call fails.
2. reverseResolve produces malformed path — Fixed by adding a table_location prefix check before the fallback path.
3. IcebergMetadata::createInitial partial-update — Additionally fixed: SCOPE_EXIT now cleans up both the metadata file and version-hint if catalog registration fails.

Regarding the reported export partition regression (DimensionWieldr's comment)

Traced the parallel-export flow through IcebergStorageSink::initializeMetadata → generateNextMetadata → generateManifestList:

• use_previous_snapshots defaults to true in the write path.
• The new guard new_snapshot->has(Iceberg::f_parent_snapshot_id) only blocks manifest chaining for initial snapshots (parent_snapshot == -1). For exports on tables with existing data, parent IS set and previous manifests ARE correctly included.
• On CAS/version-hint conflict, the retry re-reads metadata (previous_metadata_file_path updated, metadata object re-read), then parent_snapshot correctly points to the winning snapshot whose manifest list already includes all prior data.
• The Utils.cpp cleanup (removing stale metadata files on conflict) prevents the resolver from picking uncommitted metadata as latest.

Conclusion: The code changes in MetadataGenerator.cpp and IcebergWrites.cpp do not introduce a mechanism that would lose manifest entries from concurrent exports. The regression may be pre-existing or related to a different code path not modified by this PR.

Coverage summary

Category	Status
Scope reviewed	All 36 changed files; focused re-analysis on IcebergWrites.cpp retry/concurrency path, MetadataGenerator.cpp parent-snapshot logic, generateManifestList chaining, GlueCatalog cleanup, IcebergMetadata::createInitial cleanup, reverseResolve fix
Categories failed	Null-safety (IcebergWrites.cpp current-snapshot-id), UX (S3Tables CREATE TABLE error message)
Categories passed	Partial-update rollback (fixed), path resolution (fixed), concurrency/manifest-chaining (correct), memory lifetime, exception safety, ON CLUSTER rejection, namespace encoding, setting rename/alias, DROP purge semantics, metadata-log correctness, test coverage
Assumptions/limits	Static analysis only; export partition regression analyzed by code tracing only, not runtime-reproduced; Poco JSON isNull vs has semantics assumed standard

[DimensionWieldr]

Conclusion: The code changes in MetadataGenerator.cpp and IcebergWrites.cpp do not introduce a mechanism that would lose manifest entries from concurrent exports. The regression may be pre-existing or related to a different code path not modified by this PR.

Hmm, are we sure? Regression test /iceberg/export partition/ice catalog/sanity/export multiple partitions in one ALTER/* passes on base 26.3.10.20001.altinityantalya, but fails on this PR. I tried it locally as well and was able to reproduce this. At the very least, there is something going on with the manifests. I asked Composer and it came up with a few instructions. I'm not sure if it is helpful, but I'll put it here just in case:

Fix directions

1. Complete the 1896 port in IcebergMetadata.cpp export path (same as IcebergWrites / Mutations): track previous_metadata_file_path + reverseResolve, pass that into generateNextMetadata, not metadata_info.path
2. In RestCatalog::updateMetadata, always attach assert-ref-snapshot-id from the snapshot read at commit time (handle empty-table case per REST spec)
3. Optionally serialize Iceberg commits per destination table in ExportPartitionUtils::commit

[alsugiliazova]

Two issues left:

1. Duplicate metadata.json on insert
2. Prune is not working with glue catalog

[alsugiliazova]

SHOW TABLES is not working #1988 (using build from the latest commit)

[alsugiliazova]

why metadata json does not use uuid in the filename?

[alsugiliazova]

AI audit note: This review comment was generated by AI (claude-4.6-sonnet).

Audit update for PR #1896 — commit 498d41e

Scope: Native CREATE TABLE / DROP TABLE for DataLakeCatalog; optionally prune on DROP

---

Confirmed defects

Medium: Missing isNull guard for current-snapshot-id in IcebergStorageSink::initializeMetadata

• Impact: Exception on INSERT into an Iceberg v2 table whose latest metadata has "current-snapshot-id": null (convention for newly created tables before any write, e.g. tables created by Spark/PyIceberg).
• Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp line 1215
• Trigger: INSERT into a catalog-backed Iceberg v2 table that has "current-snapshot-id": null in its metadata JSON.
• Why defect: if (metadata->has(Iceberg::f_current_snapshot_id)) is present but without !metadata->isNull(...). getValue<Int64> on a JSON null throws. Mutations.cpp:453 was correctly fixed in this PR with the !metadata->isNull(...) guard, but the identical check in IcebergWrites.cpp:1215 was not updated.
• Fix direction: Add && !metadata->isNull(Iceberg::f_current_snapshot_id) to the guard at IcebergWrites.cpp:1215.
• Regression test direction: Create a table externally with "current-snapshot-id": null, then INSERT from ClickHouse.

---

Medium: constructTableLocation loses S3 bucket when storage_endpoint uses s3://bucket/path URI format

• Impact: CREATE TABLE on a Glue-backed DataLakeCatalog generates a table location such as s3://warehouse/ns/table (bucket omitted); all subsequent S3 operations for the table target the wrong path.
• Anchor: src/Databases/DataLake/Common.cpp / constructTableLocation, S3 branch: return fmt::format("s3://{}/{}/{}", path, namespace_name, table_name) where path = uri.getPath().
• Trigger: User configures a DataLakeCatalog database with storage_endpoint = 's3://my-bucket/warehouse' (virtual-hosted URI style) and default_base_location is not set.
• Why defect: For s3://bucket/path URIs, Poco::URI::getPath() returns only /path; the bucket sits in getHost() / getAuthority() and is never included in the formatted location. For https://endpoint/bucket/path (path-style), getPath() correctly includes the bucket, so that variant works.
• Fix direction: For the S3 branch, check uri.getAuthority() and prepend it if non-empty: return fmt::format("s3://{}/{}/{}/{}", authority, path, namespace_name, table_name).
• Regression test direction: Add a unit test in gtest_construct_table_location.cpp for s3://bucket/warehouse input and assert the bucket appears in the output.

---

Medium: getMetadataLocationFromCatalog path fixup can produce a doubled path

• Impact: All INSERT and mutation operations on transactional catalog tables throw "No such file" because the metadata path passed to getMetadataFileAndVersion is malformed (e.g. warehouse/ns/table//s3://bucket/warehouse/ns/table/metadata/v3.metadata.json).
• Anchor: src/Databases/DataLake/Common.cpp / getMetadataLocationFromCatalog: if (!metadata_path.starts_with(table_path)) metadata_path = std::filesystem::path(table_path) / metadata_path.
• Trigger: The REST catalog returns an absolute metadata URI (e.g. s3://bucket/warehouse/ns/table/metadata/v3.metadata.json) but table_path in persistent_components is a relative storage path (e.g. warehouse/ns/table/). starts_with is false → the relative prefix is prepended to the absolute URI.
• Why defect: The fixup mirrors the reverseResolve pattern but does not handle the absolute/relative mismatch. The existing IcebergPath.h::reverseResolve handles this correctly via a table_location prefix check first.
• Fix direction: Delegate to path_resolver.reverseResolve(metadata_path).serialize() instead of repeating the fixup inline, or strip the scheme/authority before comparing against table_path.
• Regression test direction: Verify INSERT into a REST-catalog table created externally where table_path is relative but the catalog metadata location is absolute.
• Note: REST catalog is confirmed working in practice, suggesting table_path may be absolute for catalog-backed tables in the current deployment — this defect may only surface in specific URI configurations.

---

Low: S3Tables CREATE TABLE still hits NOT_IMPLEMENTED with confusing error

• Impact: User gets "createTable is not implemented" instead of a clear rejection that S3Tables does not support CREATE TABLE from ClickHouse.
• Anchor: src/Databases/DataLake/DatabaseDataLake.cpp / getLocationSchemeForTableCreation returns "s3" for S3_TABLES → location is built → catalog->createTable hits ICatalog::createTable base which throws NOT_IMPLEMENTED.
• Trigger: CREATE TABLE on any database backed by the S3Tables catalog type.
• Fix direction: Add S3_TABLES to the throw BAD_ARGUMENTS branch in getLocationSchemeForTableCreation with a message that S3Tables manages its own metadata.

---

Low: checkAlterIsPossible triggers a metadata update as a side effect

• Impact: Every ALTER command re-reads and updates Iceberg metadata from the catalog inside a function that is supposed to be a read-only feasibility check.
• Anchor: src/Storages/ObjectStorage/StorageObjectStorage.cpp / checkAlterIsPossible: configuration->update(object_storage, context).
• Trigger: Any ALTER command on a catalog-backed Iceberg table.
• Fix direction: Move the update call to alter() only (where updateExternalDynamicMetadataIfExists is already called), or add a staleness guard before calling update in checkAlterIsPossible.

---

Resolved from previous audit rounds (confirmed fixed in current HEAD)

Finding	Fix
GlueCatalog orphans metadata file on createTable failure	SCOPE_EXIT + registered flag cleans up the written S3 metadata file if the Glue API call fails
reverseResolve produces malformed path	table_location prefix check added before the concatenation fallback in IcebergPath.h
IcebergMetadata::createInitial partial-update rollback	SCOPE_EXIT removes both metadata file and version-hint on catalog registration failure
Concurrent export-partition data loss (REST catalog)	assert-ref-snapshot-id always included in updateMetadata, covering initial (ref-absent) and subsequent (parent-snapshot-id) commits; parent-snapshot-id omitted from snapshot when -1 so generateManifestList skips manifest-chaining for first snapshot
Transactional catalog writes duplicate metadata files	Metadata JSON write skipped for transactional catalogs in IcebergWrites, Mutations, truncate, and expireSnapshots; metadata submitted exclusively via catalog->updateMetadata
IcebergMetadata::drop doubled path	purgeTableDataFiles uses listFiles(object_storage, table_path, "", "") (empty prefix); IcebergMetadata::drop now delegates to it
iceberg_delete_data_on_drop setting rename	Renamed to data_lake_delete_data_on_drop with iceberg_delete_data_on_drop kept as alias via DECLARE_WITH_ALIAS
Glue metadata_location empty-string populating iceberg_metadata_file_location	!table_params.at("metadata_location").empty() guard added

---

Coverage summary

• Scope reviewed: All 44 changed files at 498d41e; deep analysis on IcebergWrites.cpp (transactional metadata fetch, null-safety, metadata-log path), Mutations.cpp (null check, previous_metadata_file_path), MetadataGenerator.cpp (parent-snapshot-id omission, metadata-log guard), IcebergMetadata.cpp (transactional createInitial early-return, drop → purge), GlueCatalog.cpp (S3-write + SCOPE_EXIT, purge), RestCatalog.cpp (CAS requirement, sort-order, namespace encoding), DatabaseDataLake.cpp (engine-less CREATE, context-aware DROP), Common.cpp (constructTableLocation, getMetadataLocationFromCatalog), InterpreterCreateQuery.cpp (engine-less gate, isDatalakeCatalog checks), executeDDLQueryOnCluster.cpp (checkDatabaseSupportsOnClusterDDL), StorageObjectStorage.cpp (drop passes purge=false, checkAlterIsPossible update call)
• Categories failed: Null-safety (IcebergWrites.cpp current-snapshot-id), URI parsing (constructTableLocation S3 bucket), path format mismatch (getMetadataLocationFromCatalog), UX (S3Tables CREATE TABLE)
• Categories passed: Partial-update rollback, concurrent commit safety, manifest chaining, duplicate metadata-file write prevention, ON CLUSTER rejection, namespace encoding, setting rename/alias, DROP purge semantics (Glue + REST + S3Tables), metadata-log correctness, purge path correctness, IF NOT EXISTS, memory lifetime/RAII, exception-safety
• Assumptions/limits: Static analysis only; getMetadataLocationFromCatalog path-format mismatch not runtime-reproduced (REST catalog confirmed working, suggesting table_path may be absolute for catalog tables in practice — defect may be configuration-dependent); constructTableLocation S3 URI bug not covered by the new unit tests in gtest_construct_table_location.cpp.

[mkmkme]

Can't say I understood everything, but from my perspective looks good.

UPD: but also please have a look at the latest audit report

[mkmkme]

Why not just pop_back()?

[mkmkme]

I would probably add some comment here explaining the change. I had to dig through the spec with Claude to understand why this change was made :)

[mkmkme]

Just a style nitpick: the checks for transactional catalog are a bit inconsistent. You added const bool at some parts, but not here. I'd personally say that maybe introducing that bool is not really necessary, but I'll leave this for you to decide.