Repository navigation
Native CREATE TABLE / DROP TABLE for DataLakeCatalog; optionally prune on DROP - #2305
Conversation
RelEasy — resolution kept with warningsThe AI resolution was pushed, but a post-resolution check is still failing on it. Nothing was rolled back — please review the point(s) below before merging and either fix them or dismiss them as false positives.
|
zvonand
added
antalya
antalya-26.6
port-antalya
zvonand
changed the title
CREATE TABLE / DROP TABLE for DataLakeCatalog; optionally prune on DROPCREATE TABLE / DROP TABLE for DataLakeCatalog; optionally prune on DROP
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
`-Wdocumentation-html` treats `<table>` in a `///` comment as an unclosed
HTML start tag (backticks are Markdown and the Doxygen-style parser does
not honor them), which is fatal under `-Weverything -Werror`:
src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp:942:32:
error: HTML tag 'table' requires an end tag [-Werror,-Wdocumentation-html]
Build report:
https://altinity-build-artifacts.s3.amazonaws.com/json.html?PR=2305&sha=7809d87ff19fbda330d3238b289f338c146bb533&name_0=PR&name_1=Build%20%28amd_binary%29
#2305
This comment was marked as outdated.
This comment was marked as outdated.
|
For the initial check, here is what audit review was able to hit. PR #2305 Audit Findings1)
|
PR #2305 fix verificationRun from BUILD="https://altinity-build-artifacts.s3.amazonaws.com/PRs/2305/<sha>/build_amd_release/clickhouse-common-static_26.6.2.20001.altinityantalya_amd64.deb"Backport 114864 (transform names
|
6ec36d2 to
d592818
Compare
d592818 to
49215ff
Compare
arthurpassos
left a comment
arthurpassos
left a comment
There was a problem hiding this comment.
It is a big PR, I'll do it partially. Below are my human generated comments
|
Below you can find AI comments: |
…prune on `DROP` Adds native `CREATE TABLE`, `CREATE TABLE ... AS source` and `DROP TABLE` support for `DataLakeCatalog` databases, so tables can be created and removed in the catalog itself instead of only being read through it. `CREATE TABLE` registers the table in the catalog (REST, Glue, Unity, S3 Tables), honouring `IF NOT EXISTS` and rolling the staged metadata back when the creation fails or loses a race. `iceberg_metadata_compression_method` is respected by `GlueCatalog::createTable`. `DROP TABLE` unregisters the table and, when the new `data_lake_delete_data_on_drop` setting is enabled, asks the catalog to purge the table data as well. `IF EXISTS` and `IF NOT EXISTS` are passed down to the catalog so the backends can honour them before rejecting an unsupported purge. The engine named in `CREATE TABLE` is validated against the catalog backend, including its arguments: `tryGetTableImpl` reopens a table from its catalog location plus the database engine arguments, so any other argument given in `CREATE` - storage credentials above all - would apply only to the creation and then be replaced on the first read. Engine `SETTINGS` are accepted with an explicit engine. Clauses unsupported for a catalog table are rejected when inherited from a `CREATE TABLE ... AS` source, also when the destination gives its own engine; `datalake_create_table_as_ignore_unsupported_source_properties` drops them instead. The accepted engine family is the catalog's own, so `CREATE TABLE ... ENGINE = DeltaLakeLocal(...)` is accepted in a Unity-backed database. Only Iceberg metadata is generated for a `CREATE TABLE` without a table engine, so that path throws `NOT_IMPLEMENTED` for a catalog of another family before resolving the table location. `OneLakeCatalog`, `BigLakeCatalog` and `S3TablesCatalog` report their fixed backend from `getStorageType`, so every backend check reads `ICatalog::getStorageType`. A catalog that assigns table locations itself rejects an explicit `ENGINE` and `storage_endpoint` as the placement for a new table and asks for `default_base_location`; `SHOW CREATE TABLE` omits the `ENGINE` clause for such a catalog. `SHOW CREATE TABLE` now includes the Iceberg `PARTITION BY` / `ORDER BY` keys when they can be represented. A lost `IF NOT EXISTS` race throws `TABLE_ALREADY_EXISTS`, which `InterpreterCreateQuery` matches on to turn into a no-op, and the Glue rollback removes the staged metadata file only after `GetTable` confirms that nothing still points at it. A transactional (REST) catalog writes the initial metadata file itself, so ClickHouse no longer leaves an orphaned one next to it. `ON CLUSTER` DDL, including `DROP DATABASE ... ON CLUSTER`, is rejected for `DataLakeCatalog`. ClickHouse#98670 (cherry picked from the state of ClickHouse#98670 at b3b0b22, squashed) Adaptations for antalya-26.6: * Dropped: `UnityV2Catalog`, `DatabaseRemote` and `DeltaLakeCatalogRegistration.cpp` do not exist here, nor do the Delta Lake `createTable` path of `DeltaLakeMetadataDeltaKernel` and `UnityCatalog`, so their changes are not applied. Upstream-only tests that the PR merely touched (`test_create_gzip_metadata`, `test_cluster_insert`, `test_database_unity_v2`) are not added. * Added: `ICatalog::managesTableLocation` and the `S3TablesCatalog` override, which upstream already had in its base. * Adapted: `ICatalog::getTableEngineName` does not exist here, so the engine family comes from `table_engine_definition`, and `getCreateTableQueryImpl` keeps this branch's engine name for unreadable tables. * Adapted: `toIcebergMetadataCompressionExtension` and `Iceberg::makeIcebergLocationURI` do not exist here, so `GlueCatalog` and `IcebergMetadata::createInitial` keep this branch's file naming and location construction. * Adapted: `parseTransformAndArgument` takes a time zone here; the new `getPartitionAndSortingKeyASTsFromMetadata` passes an empty one. * Adapted: the Iceberg REST namespace-identifier fix (`namespaceToJSONArray`) was applied inside `buildUpdateMetadataRequestBody` and `buildUpdateSchemaRequestBody`, where this branch builds those request bodies. * Adapted: `IcebergMetadata::drop` keeps this branch's reachable-file enumeration and takes the resolved `delete_data` flag. * Adapted: `StorageObjectStorageCluster` delegates to `pure_storage` here, so `prepareForDrop` forwards there instead of capturing the setting and calling `StorageObjectStorage::dropImpl` itself. * Adapted: `writeMetadataFiles` in `Iceberg/Mutations.cpp` gained `previous_metadata_file_path` beside this branch's `content_type` and `write_metadata_json_file` parameters. * Adapted: the namespace filter (`allowed_namespaces` / `isNamespaceAllowed`) of `RestCatalog` and `GlueCatalog`, `GlueCatalog::getOrFetchMetadataObject`, and the `NoSuchBucket` wrapping in `createInitial` are kept. `TableMetadata::hasDataLakeSpecificProperties` does not exist here, so `GlueCatalog` checks `getDataLakeSpecificProperties().has_value()`. * Adapted: the new settings are registered in `SettingsChangesHistory.cpp`, since this branch does not declare the history inline.
49215ff to
9c43382
Compare
|
@zvonand have you had time to look into #2305 (comment)? |
| "(got {})", datalake_unsupported_storage_clause); | ||
| } | ||
|
|
||
| if (!ignore_unsupported_source_properties || columns_user_specified) |
There was a problem hiding this comment.
Nit: the amount of times ignore_unsupported_source_properties is checked surprised me. Asked AI for a simplification - not sure it looks better, I'll leave it up to you to decide:
auto reject_unsupported_columns = [&]
{
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));
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");
};
if (ignore_unsupported_source_properties)
{
if (columns_user_specified)
reject_unsupported_columns();
else
{
ColumnsDescription plain_columns;
for (const auto & column : properties.columns)
plain_columns.add(ColumnDescription(column.name, column.type));
properties.columns = std::move(plain_columns);
properties.indices = {};
properties.constraints = {};
properties.projections = {};
auto columns_list = make_intrusive<ASTColumns>();
columns_list->set(columns_list->columns, formatColumns(properties.columns));
create.set(create.columns_list, columns_list);
}
if (comment_user_specified)
{
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)");
}
else
create.reset(create.comment);
}
else
{
reject_unsupported_columns();
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 (!as_table_saved.empty())
{
// existing source-storage lookup and findUnsupportedDatalakeStorageClause call
}
}
only partially. now I'll make AI look into AI findings first :) |
arthurpassos
left a comment
arthurpassos
left a comment
There was a problem hiding this comment.
I read the drop code, and parts of the create as. Left a couple of comments. I also went through the tests, left a couple of comments regarding test duplication. Regardless, it seems like it works and that's how far I can go with a human review in a reasonable time.
I'll leave it to the author to fix the AI comments.
(cherry picked from commit 7ed36c5)
|
@blau-ai is regression fail caused by this PR? how to fix it? https://altinity-build-artifacts.s3.amazonaws.com/REFs/2305/merge/2d2e7bc503673c76748b0d9d39dfecea9ef34ae0/regression/x86_64/with_analyzer/zookeeper/without_thread_fuzzer/iceberg2/report.html |
Regression triage —
|
| Scenario | Expected | Got |
|---|---|---|
…/ice catalog/replicated merge tree/sanity/export multiple partitions in one ALTER |
4 rows | 1 |
…/ice catalog/replicated merge tree/concurrent writes/multi_statement_alter_commits_each_partition |
9 rows | 6 |
In both cases some commits are silently dropped (lost writes). Single-partition exports and every other Iceberg scenario pass — so the bug is specifically in accumulating multiple sequential/concurrent commits into one catalog table.
Why it's this PR (not pre-existing, not flaky)
- Base branch passes the identical tests. The
antalya-26.6nightly (run 37083467466, commitd8345a0) ran the same iceberg suite and had 0 hard fails iniceberg2. The regression commit it used (5f7decba) is a 7-commit descendant of the one your PR run used (25030e18), and none of those 7 commits touchiceberg/orexport_partition/— so the test code is identical between the two runs. The only variable is the ClickHouse binary. - Neither test is
xfail'd in the suite (checkediceberg/regression.pyxfails — the onlyexport partitionxfails are formanifest integrity,truncate,datatypes,KILL, etc.; these two are not listed). - Deterministic, not flaky: your PR run (36753531651) auto-retried twice (
job.retry=2) and produced the identical assertion each time. - The PR rewrites exactly the code these tests exercise. The EXPORT PARTITION driver itself (
StorageMergeTree/PartitionCommands) is not touched — only the Iceberg write/commit target is:IcebergMetadata::createInitial,writeMetadataFileAndVersionHint,MetadataGenerator::generateNextMetadata,IcebergWrites.cpp(generateManifestList/initializeMetadata), and the catalogupdateMetadatapath.
Likely root cause (lost updates on multi-commit append)
Each exported partition is a separate append/commit to the destination catalog table, so N partitions = N snapshots that must chain off each other. The symptom (only a subset of partitions survive, always the "latest wins") is a classic lost-update: a commit that should carry forward the previously-committed snapshot instead overwrites it. The changed hunks that most plausibly cause this:
-
MetadataGenerator.cpp:parent-snapshot-idis now set only whenparent_snapshot_id != -1, and -
IcebergWrites.cppgenerateManifestList: previous-snapshot carry-forward is now gated onnew_snapshot->has(f_parent_snapshot_id).→ If a second/retried commit computes
parent_snapshot == -1(e.g. because it reads a stale metadata/catalog pointer rather than the just-committed snapshot), the new snapshot gets no parent and skips manifest carry-forward, so it contains only its own partition's data and silently replaces the prior commit. -
writeMetadataFileAndVersionHint(Utils.cpp) changed frombreak(keep file, succeed) to removing the metadata file and returningfalseonold_version >= metadata_file_info.version. That is the right call only if the caller then re-reads the newly-committed state and retries to completion; combined with the catalog CAS inIcebergWrites::write/catalog->updateMetadataand the newinvalidateMetadataCache(), a conflict that doesn't re-read the winning snapshot as the new parent will drop the losing writer's rows.
So the fix should ensure that on a commit conflict/retry, the append re-reads the latest committed snapshot and carries it forward (parent-snapshot-id + manifest list) before writing — rather than overwriting — for the catalog-backed path.
Next step
I have not pushed anything. I can:
- Pull
clickhouse-server.logfrom the failing run and pinpoint the exact commit that drops the data (snapshot IDs / parent chain), then - Prepare the fix — by default as a new
blau/*PR, or committed directly toreleasy/port/pr-98670-2f540fif you'd rather (just confirm, since direct commits need your explicit go-ahead).
Want me to go ahead and dig into the server log + draft the patch?
`MetadataGenerator::generateNextMetadata` no longer writes `parent-snapshot-id` for the first snapshot of a table, but `buildUpdateMetadataRequestBody` only added the `assert-ref-snapshot-id` requirement when a parent was present. The first commit to an empty table therefore had no conflict check: two concurrent commits (e.g. several partitions of one `EXPORT PARTITION` statement) were both accepted and the later one replaced `main`, losing the other's data. Always send the requirement for `main`, with `snapshot-id` only when there is a parent; without it the catalog asserts that `main` does not exist yet. This matches upstream ClickHouse#115652. #2305 (comment) https://github.com/Altinity/ClickHouse/actions/runs/36753531651
|
Last round of audit review, this time done with GLM. If none of this are something we want to fix in this PR - I can mark it as "Verified" from my side. Audit Review — Defects — Altinity/ClickHouse PR #2305PR: Altinity/ClickHouse#2305 — Native Defect 1 [Medium] —
|
| Call site (PR head) | Passes | Result |
|---|---|---|
Mutations.cpp:463/478 |
reverseResolve(metadata_path) — the previous file |
correct (fixed by this PR) |
IcebergWrites.cpp:1585 |
previous file or {} when none |
correct |
Compaction.cpp:1024 |
Iceberg::IcebergPathFromMetadata{} — empty |
no metadata-log entry at all |
IcebergMetadata.cpp:775 (truncate) |
metadata_info.path — the new file |
wrong file logged; true previous file orphaned |
IcebergMetadata.cpp:1944 (export-partition) |
metadata_info.path — the new file |
same |
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
|
@Selfeer I think you can put a |
CI triage — no failures caused by this PRVerdict: 4 distinct red checks, 0 PR-caused. All are pre-existing/infra/flaky and reproduce (or are known-flaky) independent of this change. Head SHA analyzed: The PR's own additions are green: all 8 Integration-test shards passed (
1. Grype Scan — keeper (4 high/crit), server-alpine (1 high/crit)Container-image vulnerability scans over the built Docker images. These flag CVEs in the base image / bundled OS packages, not in anything this C++ PR touches. The same scans also fail on unrelated PRs (e.g. #2477, an error-codes change) — it's branch-wide, handled at the release/base-image level, not in a feature PR. 2. SQLLogic test — job timeout (exit 137)Not an assertion failure. From the job log, the job ran out of wall-clock time and was SIGKILLed:
3. Regression release / Swarms — 1 flaky scenario
The test runs a long 4.
|
Selfeer
added
verified
|
I'm okay with the merge. Marked both as "Verified" and "Verified with issues" so we have correct track. The regression fails are unrelated to changes here. |
5 participants
Cherry-picked from ClickHouse#98670.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Supports
CREATE TABLE,CREATE TABLE … AS source, andDROP TABLEforDataLakeCatalog;DROP TABLEcan request catalog-side data purge via the newdata_lake_delete_data_on_dropsettingCI/CD Options
Exclude tests:
Regression jobs to run: