Feature: Support TRUNCATE TABLE for Iceberg engine#1529
Feature: Support TRUNCATE TABLE for Iceberg engine#1529il9ue wants to merge 19 commits intoantalya-26.1from
Conversation
This commit introduces native support for the TRUNCATE TABLE command for the Iceberg database engine. Execution no longer throws a NOT_IMPLEMENTED exception for DataLake engines. To align with Iceberg's architectural standards, this is a metadata-only operation. It creates a new snapshot with an explicitly generated, strictly typed empty Avro manifest list, increments the metadata version, and performs an atomic catalog update. File changes: - StorageObjectStorage.cpp: Remove hardcoded exception, delegate to data_lake_metadata->truncate(). - IDataLakeMetadata.h: Introduce supportsTruncate() and truncate() virtual methods. - IcebergMetadata.h/cpp: Implement the Iceberg-specific metadata truncation, empty manifest list generation via MetadataGenerator, and atomic catalog swap. - tests/integration/: Add PyIceberg integration tests. - tests/queries/0_stateless/: Add SQL stateless tests.
arthurpassos
left a comment
arthurpassos
left a comment
There was a problem hiding this comment.
I haven't implemented anything for Iceberg yet, tho it is in my todo list. I left two small comments for now.
I also looked at the transactional model and it looks ok (assuming I understood it correctly).
My understanding of the Iceberg + catalog transactional model is that updating the catalog is the commit marker, and if it fails, the transaction isn't complete even if the new metadata files were already uploaded. Those become orphan and must be ignored. This also implies an Iceberg table should always be read through a catalog if one exists, otherwise it becomes hard to determine the latest metadata snapshot.
I'll read the code more carefully later.
| { | ||
| throw Exception(ErrorCodes::NOT_IMPLEMENTED, | ||
| "Truncate is not supported for data lake engine"); | ||
| if (isDataLake()) |
There was a problem hiding this comment.
Isn't isDataLake() the same as the above configuration->isDataLakeconfiguration()? see
| virtual bool supportsParallelInsert() const { return false; } | ||
|
|
||
| virtual void modifyFormatSettings(FormatSettings &, const Context &) const {} | ||
| virtual void modifyFormatSettings(FormatSettings & /*format_settings*/, const Context & /*local_context*/) const {} |
There was a problem hiding this comment.
I would not change this method simply to avoid merge conflicts with upstream later on
il9ue
force-pushed
the
feature/iceberg-truncate
branch
from
41eff19 to
c1d252e
Compare
- Addressed Arthur's review comments (removed redundant isDataLake check, reverted IDataLakeMetadata.h signature). - Removed the stateless SQL test entirely. Iceberg table bootstrapping requires external catalog initialization, which is fully covered by the PyIceberg integration tests.
il9ue
force-pushed
the
feature/iceberg-truncate
branch
from
c1d252e to
601250f
Compare
|
@arthurpassos Looking at the latest CI pipeline, there are a couple of red checks remaining, but they appear to be unrelated infrastructure flakes/upstream regressions:
Since the core logical truncation architecture is in place and the specific Iceberg integration tests are passing, I believe this is ready for another look whenever you have the time. Let me know if you need any further adjustments! |
|
@codex review |
| persistent_components.metadata_compression_method, | ||
| persistent_components.table_uuid); | ||
|
|
||
| Int64 parent_snapshot_id = actual_table_state_snapshot.snapshot_id.value_or(0); |
There was a problem hiding this comment.
Why 0? I have seen "-1" in a few places https://github.com/ClickHouse/ClickHouse/blob/e3064cae1e27cafb79d37215a8d1d13a24c7801f/src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp#L879 and https://github.com/ClickHouse/ClickHouse/blob/e3064cae1e27cafb79d37215a8d1d13a24c7801f/src/Databases/DataLake/RestCatalog.cpp#L1055.
Perhaps it is part of a specification?
| const auto & [namespace_name, table_name] = DataLake::parseTableName(storage_id.getTableName()); | ||
| bool success = catalog->updateMetadata(namespace_name, table_name, storage_metadata_name, metadata_object); | ||
| if (!success) | ||
| throw Exception(ErrorCodes::LOGICAL_ERROR, "Failed to commit Iceberg truncate update to catalog."); |
There was a problem hiding this comment.
I don't think it should be a LOGICAL_ERROR. This will crash ClickHouse
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a7e014507
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (catalog) | ||
| { | ||
| const auto & [namespace_name, table_name] = DataLake::parseTableName(storage_id.getTableName()); | ||
| bool success = catalog->updateMetadata(namespace_name, table_name, storage_metadata_name, metadata_object); |
There was a problem hiding this comment.
truncate() builds new_snapshot but calls catalog->updateMetadata(..., metadata_object). For REST catalogs, RestCatalog::updateMetadata treats this argument as a snapshot and reads snapshot-id; table metadata objects do not have that field, so truncate fails before the request is sent. This makes TRUNCATE TABLE fail for Iceberg tables backed by REST catalogs.
Useful? React with 👍 / 👎.
| if (catalog) | ||
| { | ||
| const auto & [namespace_name, table_name] = DataLake::parseTableName(storage_id.getTableName()); | ||
| bool success = catalog->updateMetadata(namespace_name, table_name, storage_metadata_name, metadata_object); |
There was a problem hiding this comment.
The truncate commit passes storage_metadata_name to updateMetadata, but catalog updates need the catalog-facing metadata URI (as done in normal Iceberg writes). With Glue, this value is written to metadata_location; using an internal storage path instead of an s3://... URI leaves the catalog pointing at a non-resolvable location after truncate.
Useful? React with 👍 / 👎.
| persistent_components.metadata_compression_method, | ||
| persistent_components.table_uuid); | ||
|
|
||
| Int64 parent_snapshot_id = actual_table_state_snapshot.snapshot_id.value_or(0); |
There was a problem hiding this comment.
Using value_or(0) fabricates parent snapshot id 0 for tables that currently have no snapshot. Catalog commits then assert main is at snapshot 0 instead of using the existing no-parent sentinel (-1 used in other Iceberg write paths), so truncating a freshly created empty table can fail with optimistic-lock checks.
Useful? React with 👍 / 👎.
|
|
||
| # Assert PyIceberg reads the empty snapshot successfully | ||
| assert len(table.scan().to_arrow()) == 0 | ||
|
|
There was a problem hiding this comment.
Perhaps it is a good idea to insert data again and check it can be read just to make sure we haven't broken anything?
There was a problem hiding this comment.
Good call. Added in the latest commit — after asserting count() == 0, now append a new row via PyIceberg and verify ClickHouse reads it back as count() == 1. This confirms the table remains fully writable after truncation and that the metadata state is consistent.
…log support)
## Overview
Implements metadata-only TRUNCATE TABLE for the Iceberg database engine,
targeting REST catalog (transactional) backends. Leaves physical file
garbage collection to standard Iceberg maintenance operations.
## Root Cause Analysis & Fixes (7 bugs)
### Bug 1: Premature isTransactional() guard blocked REST catalog
RCA: A guard was added that threw NOT_IMPLEMENTED for any transactional
catalog (i.e. REST), which is the primary target of this feature.
Fix: Removed the guard entirely. REST catalogs are fully supported.
### Bug 2: Catalog commit block was deleted
RCA: The catalog->updateMetadata() call was removed alongside the guard,
leaving object storage updated but the REST catalog pointer never atomically
swapped. The table appeared truncated locally but the catalog still pointed
to stale metadata.
Fix: Restored the catalog commit block, building the catalog-visible URI
(blob_type://namespace/metadata_name for transactional catalogs) consistent
with the pattern in IcebergWrites.cpp and Mutations.cpp.
### Bug 3: FileNamesGenerator hardcoded is_transactional=false
RCA: The refactored truncate path hardcoded false for the isTransactional
flag, causing FileNamesGenerator to produce bare /path/ style filenames
while the REST catalog expected full s3://... URIs. This triggered a
FileNamesGenerator::convertMetadataPathToStoragePath() consistency check
(BAD_ARGUMENTS: 'Paths in Iceberg must use a consistent format').
Fix: Force full URI base (from f_location) when catalog->isTransactional()
is true, regardless of write_full_path_in_iceberg_metadata setting.
### Bug 4: value_or(0) used wrong no-parent sentinel
RCA: Using 0 as the no-parent snapshot ID fabricates a fake parent snapshot
ID for tables with no existing snapshot. REST catalog optimistic-lock checks
assert the current snapshot matches; using 0 instead of -1 (the Iceberg
spec sentinel) causes lock check failures on empty tables.
Fix: Changed value_or(0) to value_or(-1).
### Bug 5: updateMetadata passed metadata_object instead of new_snapshot
RCA: RestCatalog::updateMetadata() reads snapshot-id (a long) from its 4th
argument to build the commit request. The truncate path passed metadata_object
(full table metadata JSON) which has no top-level snapshot-id field. Poco
threw InvalidAccessException: 'Can not convert empty value' (POCO_EXCEPTION).
Fix: Pass new_snapshot (the generated snapshot object) to updateMetadata,
consistent with how IcebergWrites.cpp and Mutations.cpp call it.
### Bug 6: Empty manifest list wrote field-id-less Avro schema header
RCA: avro::DataFileWriter calls writeHeader() eagerly in its constructor,
committing the binary encoder state. Attempting writer.setMetadata() after
construction to inject the full Iceberg schema JSON (with field-id on each
field) corrupted the encoder's internal StreamWriter::next_ pointer to NULL,
causing a segfault in avro::StreamWriter::flush() on close(). Without the
override, the Avro C++ library strips unknown field properties (like field-id)
when reconstructing schema JSON from its internal node representation, causing
PyIceberg to reject the manifest list with:
ValueError: Cannot convert field, missing field-id: {'name': 'manifest_path'}
Fix: For empty manifest lists (TRUNCATE path), bypass DataFileWriter entirely
and write a minimal valid Avro Object Container File manually. This embeds
the original schema_representation string (with all field-ids intact) directly
into the avro.schema metadata entry. The Avro container format is:
[magic(4)] [meta_map] [sync_marker(16)]
with no data blocks for an empty manifest list. This avoids all contrib
library issues without modifying vendored code.
### Bug 7: use_previous_snapshots=is_v2 copied old data into truncate snapshot
RCA: The generateManifestList() call in truncate passed is_v2 as the
use_previous_snapshots argument (true for v2 tables). This caused the
function to read and copy all manifest entries from the parent snapshot
into the new manifest list, defeating the truncation. PyIceberg then
returned 3 rows instead of 0.
Fix: Pass false explicitly for use_previous_snapshots in the truncate path.
Truncation must always produce an empty manifest list.
## Changes
- src/Databases/DataLake/RestCatalog.cpp
Improve error propagation: re-throw HTTP exceptions with detail text
instead of silently returning false from updateMetadata().
- src/Storages/ObjectStorage/DataLakes/Iceberg/Constant.h
Add field aliases: deleted_records (deleted-records) and
deleted_data_files (deleted-data-files) for truncate summary fields.
- src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp
Core truncate implementation: correct FileNamesGenerator construction,
correct parent snapshot sentinel (-1), restored catalog commit, manual
Avro container write for empty manifest list, correct use_previous_snapshots.
- src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergWrites.cpp
Fix updateMetadata() call to pass new_snapshot instead of metadata_object.
Add empty manifest list fast path with manual Avro container serialization.
- src/Storages/ObjectStorage/DataLakes/Iceberg/MetadataGenerator.cpp/.h
Add is_truncate parameter to generateNextMetadata(). When true, sets
operation=overwrite, zeroes out cumulative totals, and populates
deleted_records/deleted_data_files from parent snapshot summary for
spec-compliant truncate snapshot summary.
- src/Storages/ObjectStorage/DataLakes/Mutations.cpp
Fix updateMetadata() call to pass new_snapshot instead of metadata_object.
- tests/integration/test_storage_iceberg_no_spark/test_iceberg_truncate.py
New integration test: PyIceberg creates REST catalog table with 3 rows,
ClickHouse truncates via REST catalog, validates count=0 from both
ClickHouse and PyIceberg (cross-engine validation), then verifies table
remains writable by appending a new row.
## Test Results
19/19 passed across all backends (s3, azure, local) and all test cases
including the new test_iceberg_truncate.
RCA: IcebergStorageSink::initializeMetadata() was passing the full table
metadata object to catalog->updateMetadata(), but RestCatalog::updateMetadata()
expects a snapshot object with a top-level snapshot-id field. This caused
a Poco::InvalidAccessException ('Can not convert empty value') on every
INSERT into a REST catalog table.
Fix: Pass new_snapshot instead of metadata as the 4th argument, consistent
with the truncate path and Mutations.cpp.
|
On the CI regression failures: The failures across other test suites seems pre-existing and unrelated to this PR's changes:
None of these touch |
Requesting a peer review on the Iceberg TRUNCATE TABLE implementation for the Antalya release.What this does Architecture
One non-obvious design decision worth reviewing Also fixed as part of this work
Test results On the CI failures Would love eyes on the |
Feature Scope Elaboration[Feature] Support TRUNCATE TABLE for Iceberg EngineOverview To support standard analytics workflows and testing pipelines without requiring users to Proposed Architecture Core Workflow:
Implementation Details
|
Throwing an exception broke the retry contract — callers in IcebergStorageSink::initializeMetadata and writeMetadataFiles treat false as a retryable metadata conflict signal and re-read the latest metadata before retrying. Replaced throw with LOG_WARNING to preserve retry semantics while still surfacing HTTP error details in logs.
|
Flagging the CI failures for QA review. All observed failures are pre-existing and unrelated to this PR's changes. Summary below:
Could QA confirm these are known flaky/pre-existing failures so we can proceed to merge? |
il9ue
changed the title
Ran an audit for this PR. Could you have a look please?AI audit note: This review comment was generated by AI (claude-4.6-sonnet-medium-thinking). Audit update for PR #1529 (Feature: Support TRUNCATE TABLE for Iceberg engine): Confirmed DefectsHigh: Mutations.cpp regression — full
Medium: No cleanup of orphaned files on catalog commit failure during TRUNCATE
Low: Manual Avro container writer for empty manifest list — unnecessary complexity
Coverage Summary
|
|
The audit correctly identified this as a regression — passing metadata (full table metadata JSON) instead of new_snapshot (snapshot object) to catalog->updateMetadata() causes Poco::InvalidAccessException on snapshot-id read, breaking all ALTER TABLE DELETE/UPDATE on REST catalog Iceberg tables.
|
@arthurpassos @ianton-ru @zvonand |
| { | ||
| auto write_avro_long = [](WriteBuffer & out, int64_t val) | ||
| { | ||
| uint64_t n = (static_cast<uint64_t>(val) << 1) ^ static_cast<uint64_t>(val >> 63); |
There was a problem hiding this comment.
Please add a comment with explain of this magic. What this code does, and why writing long is so complex.
There was a problem hiding this comment.
Valid point!
Will Add a comment explaining both the zigzag encoding step ((val << 1) ^ (val >> 63) maps signed integers to unsigned so small magnitudes stay small regardless of sign) and the variable-length base-128 serialization (high bit of each byte signals continuation).
There was a problem hiding this comment.
I think link on avro specs is enough.
Just to avoid WTFs from someone who is not very close with avro. :)
There was a problem hiding this comment.
Honestly, I would say it is better to move this code out to a helper function
There was a problem hiding this comment.
Isn't there a Avro writer in ClickHouse that could abstract this away?
…erateManifestList Explain the two-step integer encoding used in the manual Avro OCF writer: 1. Zigzag encoding: maps signed int64 to unsigned so small magnitudes stay compact regardless of sign (positive n → 2n, negative n → 2(-n)-1) 2. Variable-length base-128 serialization: high bit of each byte signals whether more bytes follow (little-endian order) Ref: https://avro.apache.org/docs/1.11.1/specification/#binary-encoding
arthurpassos
left a comment
arthurpassos
left a comment
There was a problem hiding this comment.
Can you check if it's possible to write a test in which you truncate a table, re-start clickhouse, and the table is functional? If you manage to implement that, maybe one in which you truncate, insert data, and re-start would also be good.
I know this might sound odd, but while working on Iceberg related writes, there was a time in which the table was not readable after I re-started clickhouse. Good to check that.
| "Iceberg truncate is experimental. " | ||
| "To allow its usage, enable setting allow_experimental_insert_into_iceberg"); | ||
|
|
||
| // Bug 1 fix: REMOVE the isTransactional() guard entirely. |
There was a problem hiding this comment.
Nitpick: this type of documentation seems a bit weird to me. If there is a thing you want to explain, I would say just write down some text explaining it instead of loose references like "bug 1 fix".
There was a problem hiding this comment.
Sure — will remove those debug-style references and replaced with plain explanatory prose.
|
|
||
| auto [new_snapshot, manifest_list_name, storage_manifest_list_name] = MetadataGenerator(metadata_object).generateNextMetadata( | ||
| filename_generator, metadata_name, parent_snapshot_id, | ||
| 0, 0, 0, 0, 0, 0, std::nullopt, std::nullopt, /*is_truncate=*/true); |
There was a problem hiding this comment.
Nit: Add the comments for the bunch of zeroes, just like you did for is_truncate or create variables with proper names and pass them
| { | ||
| // Build the catalog-visible path (blob URI for transactional, bare path otherwise) | ||
| String catalog_filename = metadata_name; | ||
| if (is_transactional) |
There was a problem hiding this comment.
I see in IcebergWrites the check is different, but leads to the same result. To the reader like me, it is not very clear why transactional needs a different URI.
I suggest one of the below:
- Use the same condition as IcebergWrites - that one is easier to understand because it is a path condition
- Add a comment explaining why transactional paths are different
| { | ||
| auto write_avro_long = [](WriteBuffer & out, int64_t val) | ||
| { | ||
| uint64_t n = (static_cast<uint64_t>(val) << 1) ^ static_cast<uint64_t>(val >> 63); |
There was a problem hiding this comment.
Honestly, I would say it is better to move this code out to a helper function
| { | ||
| auto write_avro_long = [](WriteBuffer & out, int64_t val) | ||
| { | ||
| uint64_t n = (static_cast<uint64_t>(val) << 1) ^ static_cast<uint64_t>(val >> 63); |
There was a problem hiding this comment.
Isn't there a Avro writer in ClickHouse that could abstract this away?
| if (num_deleted_rows == 0) | ||
| if (is_truncate) | ||
| { | ||
| summary->set(Iceberg::f_operation, Iceberg::f_overwrite); |
There was a problem hiding this comment.
Can't sum_with_parent_snapshot be used here?
There was a problem hiding this comment.
On Abstraction
I DID try avro::DataFileWriter first and it's the organic one. But, unfortunately in the vendored Avro C++ version, writeHeader() is called eagerly in the constructor committing the binary encoder state. Calling setMetadata("avro.schema", ...) post-construction corrupts StreamWriter::next_ causing a NULL dereference on close(). I have a confirmed server segfault stack trace from this exact path. The manual OCF serialization exists because the abstraction is broken for this specific use case in this library version.
On sum_with_parent_snapshot
The is_truncate guard inside sum_with_parent_snapshot already correctly zeroes cumulative totals (total_records, total_data_files, etc.) — you're correct that we could lean on it there. However deleted_records and deleted_data_files are different: they need to be populated from the parent snapshot's values (not zero), since they represent what was removed. That's why they're set separately. Glad to add a comment making this distinction explicit.
…ation - Remove debug-style 'Bug N fix' comments; replace with plain explanatory prose - Add named zero arguments to generateNextMetadata() call for readability - Add comment explaining why transactional catalogs require full blob URIs - Extract Avro zigzag encoding into static writeAvroLong/writeAvroBytes helpers with full comment explaining the encoding and link to Avro spec - Add test_iceberg_truncate_restart: verifies table is readable after ClickHouse restart post-truncation, and remains writable for new inserts Signed-off-by: Daniel Q. Kim <daniel.kim@altinity.com>
|
@arthurpassos |
PR #1529 CI TriagePR Summary
PR DescriptionAdds Files Changed
CI Failures AnalysisSummary Table
Quick Links
Upstream CI Failures (Integration/Stateless/Fuzzer)BuzzHouse (amd_debug) - 1 failure
Integration tests (amd_asan, db disk, old analyzer, 4/6) - 23 failures
All failures are in Click to expand full list of 23 failing tests
Integration tests (arm_binary, distributed plan, 3/4) - 1 failure
Stateless tests (amd_binary, ParallelReplicas, s3 storage, parallel) - 1 failure
Regression Test Failures (clickhouse-regression)aggregate_functions_3 - Multiple failures
Failing tests: Click to expand full list of failing data types
swarms - Multiple failures
Failing tests:
settings - 2 failures
Failing tests:
Iceberg Test Results ✅The PR includes dedicated Iceberg regression tests that were explicitly requested via the CI configuration:
Iceberg Test Status
Both Iceberg test suites passed, confirming the TRUNCATE TABLE implementation works correctly. Root Cause AnalysisNo PR-Caused Regressions IdentifiedAll failures fall into these categories:
Evidence Supporting No PR Regression
Recommendations
References
ConclusionCI Status: PASS (no PR-caused regressions) All identified failures are either:
The PR's core functionality (Iceberg TRUNCATE TABLE) is verified by the passing iceberg_1 and iceberg_2 regression test suites. |
Do we already have an issue opened for this? If not I can do that so we don't lose track of the things that need to be fixed later-on. @il9ue |
On opening an issueThanks for offering to open an issue — that would be great. Please go ahead and create one for the orphaned file cleanup on catalog commit failure. Happy to link it from the PR description and add a TODO comment in the code pointing to it. On the Mutations.cpp revertyes, that's already been fixed. The commit
|
6 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add
TRUNCATE TABLEsupport for the Iceberg database engine with REST catalog backends.CI/CD Options
Exclude tests:
Regression jobs to run: