Audit Review — Defects — Altinity/ClickHouse PR #2305

PR: Altinity/ClickHouse#2305 — Native CREATE TABLE / DROP TABLE for DataLakeCatalog
Audited head: f9b636f20d9 (merge base b73e7588a4f7). None of the findings duplicate the issues already reported in the PR comments.

Defect 1 [Medium] — CREATE TABLE ... AS from an unkeyed S3 Tables table fails with LOGICAL_ERROR: Cannot set engine, it's a bug

Description

SHOW CREATE TABLE for an S3 Tables (location-managed) catalog table omits the ENGINE clause — that's intentional. But when the table has no PARTITION BY and no ORDER BY (the default for tables created via pyiceberg or the AWS console), the create query is produced without any ASTStorage at all. InterpreterCreateQuery::setEngine then falls through every AS-branch condition and hits the generic "this is a bug" else-branch instead of the DataLakeCatalog-aware branch right above it.

Result: a legitimate, documented feature path (CTAS into a DataLakeCatalog, introduced by this PR) throws a bug-class exception for the most common kind of source table.

How to reproduce

-- 1. Have an S3 Tables catalog database (ENGINE = S3TablesCatalog ...) and inside it
--    a table WITHOUT partition/sort keys, e.g. created by pyiceberg or AWS console:
SHOW CREATE TABLE s3tables_db.`ns.src`;
-- returns:  CREATE TABLE s3tables_db.`ns.src` (`id` Int32, ...)   -- no ENGINE, no PARTITION BY, no ORDER BY

-- 2. Try to create a table from it:
CREATE TABLE s3tables_db.`ns.dst` AS s3tables_db.`ns.src`;

Expected: table created (keys copied — there are none), or a clean user-facing error.
Actual: Code: 49. DB::Exception: Cannot set engine, it's a bug.

Note: if the user gives an explicit ENGINE in the CTAS, setEngine returns early (the create.storage branch) and the query succeeds — the failure is specific to the engine-less AS form, i.e. exactly the form the PR's CTAS feature promotes.

Evidence

// src/Databases/DataLake/DatabaseDataLake.cpp:1395 (DatabaseDataLake::getCreateTableQueryImpl, PR head f9b636f20d9)
if (catalog->managesTableLocation())
{
    if (partition_by || order_by)          // unkeyed table: storage clause never set
    {
        auto storage = make_intrusive<ASTStorage>();
        ...
        create_table_query->set(create_table_query->storage, storage);
    }
    return create_table_query;             // <-- create query with NO storage clause at all
}

// src/Interpreters/InterpreterCreateQuery.cpp:1428-1441 (InterpreterCreateQuery::setEngine, PR head)
else if (as_create.storage)                 // nullptr for an unkeyed source -> this branch is SKIPPED
{
    /// A `DataLakeCatalog` that assigns table locations itself shows its tables without `ENGINE`.
    /// Only a `DataLakeCatalog` target can be created from such a table ...
    ...
}
else
{
    throw Exception(ErrorCodes::LOGICAL_ERROR, "Cannot set engine, it's a bug.");   // <-- reached
}

Defect 2 [Medium] — Per-table SETTINGS on the engine clause are accepted at CREATE but silently disappear after restart

Description

The new validation (findUnsupportedDatalakeStorageClause) accepts engine-clause SETTINGS when the engine is explicitly specified (allow_settings=true), and those settings are applied to the live storage created at CREATE time. But nothing persists them:

• the catalog entry stores no engine settings;
• SHOW CREATE TABLE wipes them (storage->settings = {});
• on reopen, tryGetTableImpl rebuilds settings only from the database engine settings.

After a server restart (or any cache eviction + reload) the table silently reverts to database defaults. Concrete impact with Glue: iceberg_use_version_hint = 1 writes metadata/version-hint.text at CREATE; after restart the flag is gone, so every later INSERT / OPTIMIZE / mutation stops updating version-hint.text (all commit paths read data_lake_settings[iceberg_use_version_hint]), and Spark's file-based Iceberg reader — which prefers version-hint.text — keeps reading stale metadata.

How to reproduce

-- 1. Create a table with a per-table engine setting (explicit-engine path, Glue or REST catalog):
CREATE TABLE glue_db.`ns.t` (id Int32)
ENGINE = IcebergS3('s3://bucket/path/', ...)
SETTINGS iceberg_use_version_hint = 1;
-- -> accepted, no error. version-hint.text is maintained on commits.

-- 2. Restart the server (or otherwise force the table to be re-fetched from the catalog).

-- 3. INSERT INTO glue_db.`ns.t` VALUES (1);
-- -> commit no longer updates version-hint.text (setting reverted to default 'false').

-- 4. Confirm: check the file — version-hint.text stays at its old version
--    while new *.metadata.json files appear.

Expected: the setting survives restart (or CREATE rejects it up front).
Actual: accepted at CREATE, silently reverted to DB default on reopen; Spark reads stale data.

Evidence

// src/Interpreters/InterpreterCreateQuery.cpp:1570-1582 (PR head)
const char * findUnsupportedDatalakeStorageClause(const ASTStorage & storage, bool allow_settings)
{
    ...
    if (!allow_settings && storage.settings)   // engine-less path REJECTS SETTINGS,
        return "engine SETTINGS";              // explicit-ENGINE path (allow_settings=true) ACCEPTS them
    return nullptr;
}

// src/Databases/DataLake/DatabaseDataLake.cpp:724-725 (DatabaseDataLake::tryGetTableImpl — the reopen path)
auto storage_settings = std::make_shared<DataLakeStorageSettings>();
storage_settings->loadFromSettingsChanges(settings.allChanged());   // database-level settings ONLY —
                                                                    // nothing per-table is restored

// src/Databases/DataLake/DatabaseDataLake.cpp:1416 (DatabaseDataLake::getCreateTableQueryImpl)
storage->settings = {};   // per-table engine settings are wiped from SHOW CREATE — nothing persists them

Commit paths that consult the (now reverted) setting: IcebergMetadata.cpp:1040, IcebergWrites.cpp:1755, Mutations.cpp:609/964, Compaction.cpp:909.

Defect 3 [Medium] — OPTIMIZE TABLE stops maintaining the Iceberg metadata-log (and truncate/export write the wrong entry)

Description

The PR renamed the second parameter of MetadataGenerator::generateNextMetadata from "the path to append to metadata-log" to "the previous metadata file path" and gated the append on it being non-empty. This is the correct Iceberg semantics (Java Iceberg logs the superseded file). But not all call sites were updated:

Before this PR, the compaction path appended an entry on every commit (merge base b73e7588 appended unconditionally). After it, OPTIMIZE produces a commit whose metadata-log never grows. External cleanup tools (Spark expireSnapshots metadata cleanup, pyiceberg expire_metadata) derive the list of deletable old metadata files from metadata-log — the pre-OPTIMIZE *.metadata.json files become permanently invisible to them and accumulate on every OPTIMIZE. No test asserts the new behavior, and the PR's own call sites are mutually inconsistent — this looks like an oversight, not a design decision.

How to reproduce

-- 1. Create an Iceberg table in a REST/Glue DataLakeCatalog and insert some rows:
CREATE TABLE dl.`ns.t` (id Int32, dt Date) ENGINE = IcebergS3('s3://...') PARTITION BY dt;
INSERT INTO dl.`ns.t` VALUES (1, '2026-01-01'), ...;

-- 2. Fetch the current metadata JSON, note its file name and the length of "metadata-log".

-- 3. OPTIMIZE TABLE dl.`ns.t`;   -- needs allow_experimental_iceberg_compaction

-- 4. Fetch the new current metadata JSON again.

Expected: metadata-log gained one entry naming the pre-OPTIMIZE metadata file (what Mutations/INSERT commits do).
Actual: metadata-log is unchanged; the pre-OPTIMIZE metadata file is no longer tracked anywhere. (Same check after TRUNCATE shows an entry naming the file that just became current instead.)

Evidence

--- a/src/Storages/ObjectStorage/DataLakes/Iceberg/Compaction.cpp   (PR diff hunk)
         auto new_snapshot = metadata_generator.generateNextMetadata(
             plan.generator,
-            generated_metadata_info.path,
+            Iceberg::IcebergPathFromMetadata{},     // <-- empty: the append is skipped entirely

--- a/src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp   (PR diff hunk)
-    const Iceberg::IcebergPathFromMetadata & metadata_file_path,
+    const Iceberg::IcebergPathFromMetadata & previous_metadata_file_path,
...
+    if (!previous_metadata_file_path.empty())       // <-- gate added: empty path = no entry
     {
         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());

// src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp:775-777 (truncate path, NOT updated by the PR)
auto [new_snapshot, manifest_list_path] = MetadataGenerator(metadata_object).generateNextMetadata(
    filename_generator, metadata_info.path,   // <-- under the new semantics this is the NEW file,
    parent_snapshot_id, ...                   //     so the current file is logged as "previous"

For contrast, the same PR fixes the mutation path correctly (Mutations.cpp:676):
previous_metadata_path = persistent_table_components.path_resolver.reverseResolve(metadata_path);

Defect 4 [Low] — RENAME DATABASE ... ON CLUSTER skips the new DataLakeCatalog ON CLUSTER guard and fails confusingly on every worker

Description

The PR added an initiator-side check (checkQueryDatabasesSupportOnClusterDDL) so that ON CLUSTER DDL targeting a DataLakeCatalog fails immediately with a clear "Database engine DataLakeCatalog doesn't support ON CLUSTER" error. The ASTRenameQuery branch of that check has a typo-like condition: !rename->database. But ASTRenameQuery::database is a bool flag meaning "this is a RENAME DATABASE query" — so for RENAME DATABASE the condition is false, no target databases are collected, and the guard is bypassed entirely.

The query is then broadcast to every worker in the cluster; each one fails with an unrelated NOT_IMPLEMENTED from IDatabase::renameDatabase (DataLakeCatalog doesn't implement rename). Diagnostics/UX gap only — no data risk, the rename still fails everywhere — but the error the user sees points at the wrong layer and the guard the PR added is dead for exactly the query shape it was written to handle.

How to reproduce

-- Given a DataLakeCatalog database and a cluster with ≥2 nodes:
RENAME DATABASE my_datalake_catalog TO other ON CLUSTER '{cluster}';

Expected (as for CREATE/DROP): immediate initiator-side error — "Database engine DataLakeCatalog doesn't support ON CLUSTER".
Actual: the DDL is enqueued to all cluster nodes; each node reports NOT_IMPLEMENTED: Database engine DataLakeCatalog doesn't support rename (from IDatabase::renameDatabase), the initiator shows distributed-DDL task failures.

Evidence

// src/Interpreters/executeDDLQueryOnCluster.cpp:69 (PR head)
else if (const auto * rename = dynamic_cast<const ASTRenameQuery *>(query_ptr.get()); rename && !rename->database)
//                                                                            ^^^^^^^^^^^^^^^^^ false for RENAME DATABASE:
// ASTRenameQuery::database is a bool flag ("For RENAME DATABASE"), so this branch never runs
// and the query's databases are never checked by checkDatabaseSupportsOnClusterDDL

// src/Parsers/ASTRenameQuery.h (PR head)
bool database{false};   /// For RENAME DATABASE