Skip to content

Commit 601250f

Browse files
author
Daniel Q. Kim
committed
Fix: Address code review feedback and drop stateless test
- 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.
1 parent de68832 commit 601250f

File tree

6 files changed

+13
-34
lines changed

6 files changed

+13
-34
lines changed

src/Storages/ObjectStorage/DataLakes/IDataLakeMetadata.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ class IDataLakeMetadata : boost::noncopyable
142142
virtual bool supportsWrites() const { return false; }
143143
virtual bool supportsParallelInsert() const { return false; }
144144

145-
virtual void modifyFormatSettings(FormatSettings & /*format_settings*/, const Context & /*local_context*/) const {}
145+
virtual void modifyFormatSettings(FormatSettings &, const Context &) const {}
146146

147147
virtual bool supportsTruncate() const { return false; }
148148
virtual void truncate(ContextPtr /*context*/, std::shared_ptr<DataLake::ICatalog> /*catalog*/, const StorageID & /*storage_id*/)

src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -538,13 +538,12 @@ void IcebergMetadata::truncate(ContextPtr context, std::shared_ptr<DataLake::ICa
538538
persistent_components.table_uuid);
539539

540540
Int64 parent_snapshot_id = actual_table_state_snapshot.snapshot_id.value_or(0);
541-
542541
auto config_path = persistent_components.table_path;
543542
if (config_path.empty() || config_path.back() != '/')
544543
config_path += "/";
545544
if (!config_path.starts_with('/'))
546545
config_path = '/' + config_path;
547-
546+
548547
FileNamesGenerator filename_generator;
549548
if (!context->getSettingsRef()[Setting::write_full_path_in_iceberg_metadata])
550549
{
@@ -559,15 +558,14 @@ void IcebergMetadata::truncate(ContextPtr context, std::shared_ptr<DataLake::ICa
559558
filename_generator = FileNamesGenerator(
560559
bucket, config_path, (catalog != nullptr && catalog->isTransactional()), persistent_components.metadata_compression_method, write_format);
561560
}
562-
561+
563562
Int32 new_metadata_version = actual_table_state_snapshot.metadata_version + 1;
564563
filename_generator.setVersion(new_metadata_version);
565-
566564
auto [metadata_name, storage_metadata_name] = filename_generator.generateMetadataName();
567565

568566
auto [new_snapshot, manifest_list_name, storage_manifest_list_name] = MetadataGenerator(metadata_object).generateNextMetadata(
569-
filename_generator, metadata_name, parent_snapshot_id,
570-
/* added_files */ 0, /* added_records */ 0, /* added_files_size */ 0,
567+
filename_generator, metadata_name, parent_snapshot_id,
568+
/* added_files */ 0, /* added_records */ 0, /* added_files_size */ 0,
571569
/* num_partitions */ 0, /* added_delete_files */ 0, /* num_deleted_rows */ 0);
572570

573571
// generate manifest list with 0 manifest files
@@ -581,10 +579,9 @@ void IcebergMetadata::truncate(ContextPtr context, std::shared_ptr<DataLake::ICa
581579
);
582580
generateManifestList(filename_generator, metadata_object, object_storage, context, {}, new_snapshot, 0, *buf, Iceberg::FileContentType::DATA, false);
583581
buf->finalize();
584-
582+
585583
String metadata_content = dumpMetadataObjectToString(metadata_object);
586584
writeMessageToFile(metadata_content, storage_metadata_name, object_storage, context, "*", "", persistent_components.metadata_compression_method);
587-
588585
if (catalog)
589586
{
590587
const auto & [namespace_name, table_name] = DataLake::parseTableName(storage_id.getTableName());

src/Storages/ObjectStorage/StorageObjectStorage.cpp

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -645,15 +645,12 @@ void StorageObjectStorage::truncate(
645645

646646
if (configuration->isDataLakeConfiguration())
647647
{
648-
if (isDataLake())
649-
{
650-
auto * data_lake_metadata = getExternalMetadata(context);
651-
if (!data_lake_metadata || !data_lake_metadata->supportsTruncate())
652-
throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Truncate is not supported for this data lake engine");
653-
654-
data_lake_metadata->truncate(context, catalog, getStorageID());
655-
return;
656-
}
648+
auto * data_lake_metadata = getExternalMetadata(context);
649+
if (!data_lake_metadata || !data_lake_metadata->supportsTruncate())
650+
throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Truncate is not supported for this data lake engine");
651+
652+
data_lake_metadata->truncate(context, catalog, getStorageID());
653+
return;
657654
}
658655

659656
if (path.hasGlobs())

tests/integration/test_storage_iceberg_no_spark/test_iceberg_truncate.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ def test_iceberg_truncate(started_cluster_iceberg_no_spark):
3636
)
3737

3838
table_name = "test_truncate"
39-
4039
table = catalog.create_table(
4140
identifier=f"{namespace}.{table_name}",
4241
schema=schema,
@@ -63,7 +62,7 @@ def test_iceberg_truncate(started_cluster_iceberg_no_spark):
6362
SETTINGS catalog_type='rest', warehouse='demo', storage_endpoint='http://minio:9000/warehouse-rest';
6463
"""
6564
)
66-
65+
6766
# Assert data from ClickHouse
6867
assert int(instance.query(f"SELECT count() FROM {namespace}.{table_name}").strip()) == 3
6968

tests/queries/0_stateless/03595_iceberg_truncate_table.reference

Lines changed: 0 additions & 2 deletions
This file was deleted.

tests/queries/0_stateless/03595_iceberg_truncate_table.sql

Lines changed: 0 additions & 12 deletions
This file was deleted.

0 commit comments

Comments
 (0)