From 6f503f9af75978a34b13dfab121a9cbdc9268b6a Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Sun, 21 Jun 2026 23:49:29 +0800 Subject: [PATCH 01/16] feat: add DeleteFiles update API (#709) ## Summary - Add a DeleteFiles snapshot update API and wire it through table and transaction update flows. - Add DeleteFiles implementation/build integration. - Add coverage for path matching, case-insensitive row filters, empty delete commits, and strict-projection partial-match rejection. ## Validation - `cmake --build build --target table_update_test` - `./build/src/iceberg/test/table_update_test '--gtest_filter=DeleteFilesTest.*'` Co-authored-by: Codex --- src/iceberg/CMakeLists.txt | 1 + src/iceberg/meson.build | 1 + src/iceberg/table.cc | 7 + src/iceberg/table.h | 3 + src/iceberg/test/CMakeLists.txt | 1 + src/iceberg/test/delete_files_test.cc | 203 ++++++++++++++++++++++++++ src/iceberg/transaction.cc | 8 + src/iceberg/transaction.h | 3 + src/iceberg/type_fwd.h | 1 + src/iceberg/update/delete_files.cc | 79 ++++++++++ src/iceberg/update/delete_files.h | 72 +++++++++ src/iceberg/update/meson.build | 1 + 12 files changed, 380 insertions(+) create mode 100644 src/iceberg/test/delete_files_test.cc create mode 100644 src/iceberg/update/delete_files.cc create mode 100644 src/iceberg/update/delete_files.h diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index d4224cd65..b3cc8a7a9 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -99,6 +99,7 @@ set(ICEBERG_SOURCES transform.cc transform_function.cc type.cc + update/delete_files.cc update/expire_snapshots.cc update/fast_append.cc update/merge_append.cc diff --git a/src/iceberg/meson.build b/src/iceberg/meson.build index 508d5e0bf..0c467605b 100644 --- a/src/iceberg/meson.build +++ b/src/iceberg/meson.build @@ -124,6 +124,7 @@ iceberg_sources = files( 'transform.cc', 'transform_function.cc', 'type.cc', + 'update/delete_files.cc', 'update/expire_snapshots.cc', 'update/fast_append.cc', 'update/merge_append.cc', diff --git a/src/iceberg/table.cc b/src/iceberg/table.cc index 9e8a35bd4..817e5917c 100644 --- a/src/iceberg/table.cc +++ b/src/iceberg/table.cc @@ -31,6 +31,7 @@ #include "iceberg/table_properties.h" #include "iceberg/table_scan.h" #include "iceberg/transaction.h" +#include "iceberg/update/delete_files.h" #include "iceberg/update/expire_snapshots.h" #include "iceberg/update/fast_append.h" #include "iceberg/update/merge_append.h" @@ -224,6 +225,12 @@ Result> Table::NewMergeAppend() { return MergeAppend::Make(name().name, std::move(ctx)); } +Result> Table::NewDeleteFiles() { + ICEBERG_ASSIGN_OR_RAISE( + auto ctx, TransactionContext::Make(shared_from_this(), TransactionKind::kUpdate)); + return DeleteFiles::Make(name().name, std::move(ctx)); +} + Result> Table::NewUpdateStatistics() { ICEBERG_ASSIGN_OR_RAISE( auto ctx, TransactionContext::Make(shared_from_this(), TransactionKind::kUpdate)); diff --git a/src/iceberg/table.h b/src/iceberg/table.h index 45f4bd961..b71a1ddbc 100644 --- a/src/iceberg/table.h +++ b/src/iceberg/table.h @@ -179,6 +179,9 @@ class ICEBERG_EXPORT Table : public std::enable_shared_from_this { /// \brief Create a new MergeAppend to append data files and merge manifests. virtual Result> NewMergeAppend(); + /// \brief Create a new DeleteFiles to delete data files and commit the changes. + virtual Result> NewDeleteFiles(); + /// \brief Create a new SnapshotManager to manage snapshots and snapshot references. virtual Result> NewSnapshotManager(); diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index 0e738924f..1d8ea472b 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -222,6 +222,7 @@ if(ICEBERG_BUILD_BUNDLE) add_iceberg_test(table_update_test USE_BUNDLE SOURCES + delete_files_test.cc expire_snapshots_test.cc fast_append_test.cc manifest_filter_manager_test.cc diff --git a/src/iceberg/test/delete_files_test.cc b/src/iceberg/test/delete_files_test.cc new file mode 100644 index 000000000..7c547ac49 --- /dev/null +++ b/src/iceberg/test/delete_files_test.cc @@ -0,0 +1,203 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/update/delete_files.h" + +#include +#include +#include + +#include +#include + +#include "iceberg/avro/avro_register.h" +#include "iceberg/expression/expressions.h" +#include "iceberg/expression/literal.h" +#include "iceberg/manifest/manifest_entry.h" +#include "iceberg/partition_spec.h" +#include "iceberg/row/partition_values.h" +#include "iceberg/schema.h" +#include "iceberg/snapshot.h" +#include "iceberg/table.h" +#include "iceberg/test/matchers.h" +#include "iceberg/test/update_test_base.h" +#include "iceberg/update/fast_append.h" + +namespace iceberg { + +class DeleteFilesTest : public MinimalUpdateTestBase { + protected: + static void SetUpTestSuite() { avro::RegisterAll(); } + + void SetUp() override { + MinimalUpdateTestBase::SetUp(); + + ICEBERG_UNWRAP_OR_FAIL(spec_, table_->spec()); + ICEBERG_UNWRAP_OR_FAIL(schema_, table_->schema()); + file_a_ = MakeDataFile("/data/file_a.parquet", /*partition_x=*/1L); + file_b_ = MakeDataFile("/data/file_b.parquet", /*partition_x=*/2L); + } + + std::shared_ptr MakeDataFile(const std::string& path, int64_t partition_x) { + auto file = std::make_shared(); + file->content = DataFile::Content::kData; + file->file_path = table_location_ + path; + file->file_format = FileFormatType::kParquet; + file->partition = PartitionValues(std::vector{Literal::Long(partition_x)}); + file->file_size_in_bytes = 1024; + file->record_count = 100; + file->partition_spec_id = spec_->spec_id(); + return file; + } + + void SetLongBounds(const std::shared_ptr& file, int32_t field_id, + int64_t lower, int64_t upper) { + ASSERT_NE(file, nullptr); + ICEBERG_UNWRAP_OR_FAIL(auto lower_bound, Literal::Long(lower).Serialize()); + ICEBERG_UNWRAP_OR_FAIL(auto upper_bound, Literal::Long(upper).Serialize()); + file->value_counts[field_id] = file->record_count; + file->null_value_counts[field_id] = 0; + file->lower_bounds[field_id] = lower_bound; + file->upper_bounds[field_id] = upper_bound; + } + + void CommitFiles(const std::vector>& files) { + ICEBERG_UNWRAP_OR_FAIL(auto append, table_->NewFastAppend()); + for (const auto& file : files) { + append->AppendFile(file); + } + ASSERT_THAT(append->Commit(), IsOk()); + ASSERT_THAT(table_->Refresh(), IsOk()); + } + + void CommitInitialFiles() { CommitFiles({file_a_, file_b_}); } + + void ExpectOneFileDeleted() { + ASSERT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kOperation), + DataOperation::kDelete); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kDeletedDataFiles), "1"); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kDeletedRecords), "100"); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kRemovedFileSize), "1024"); + } + + std::shared_ptr spec_; + std::shared_ptr schema_; + std::shared_ptr file_a_; + std::shared_ptr file_b_; + + static constexpr int32_t kYFieldId = 2; +}; + +TEST_F(DeleteFilesTest, DeleteFileByPath) { + CommitInitialFiles(); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); + delete_files->DeleteFile(file_a_->file_path); + + EXPECT_THAT(delete_files->Commit(), IsOk()); + ExpectOneFileDeleted(); +} + +TEST_F(DeleteFilesTest, DeleteFileByDataFile) { + CommitInitialFiles(); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); + delete_files->DeleteFile(file_a_); + + EXPECT_THAT(delete_files->Commit(), IsOk()); + ExpectOneFileDeleted(); +} + +TEST_F(DeleteFilesTest, DeleteFromRowFilterCaseInsensitive) { + CommitInitialFiles(); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); + delete_files->CaseSensitive(false).DeleteFromRowFilter( + Expressions::Equal("X", Literal::Long(1L))); + + EXPECT_THAT(delete_files->Commit(), IsOk()); + ExpectOneFileDeleted(); +} + +TEST_F(DeleteFilesTest, EmptyDeleteCommit) { + CommitInitialFiles(); + ICEBERG_UNWRAP_OR_FAIL(auto previous_snapshot, table_->current_snapshot()); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); + + EXPECT_THAT(delete_files->Commit(), IsOk()); + + ASSERT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + ASSERT_TRUE(snapshot->parent_snapshot_id.has_value()); + EXPECT_EQ(snapshot->parent_snapshot_id.value(), previous_snapshot->snapshot_id); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kOperation), + DataOperation::kDelete); + EXPECT_EQ(snapshot->summary.count(SnapshotSummaryFields::kDeletedDataFiles), 0U); + EXPECT_EQ(snapshot->summary.count(SnapshotSummaryFields::kDeletedRecords), 0U); + EXPECT_EQ(snapshot->summary.count(SnapshotSummaryFields::kRemovedFileSize), 0U); +} + +TEST_F(DeleteFilesTest, DeleteFromRowFilter) { + CommitInitialFiles(); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); + delete_files->DeleteFromRowFilter(Expressions::Equal("x", Literal::Long(1L))); + + EXPECT_THAT(delete_files->Commit(), IsOk()); + ExpectOneFileDeleted(); +} + +TEST_F(DeleteFilesTest, DeleteFromRowFilterRejectsPartialMatchFile) { + auto partial_match_file = MakeDataFile("/data/partial_match.parquet", + /*partition_x=*/1L); + SetLongBounds(partial_match_file, kYFieldId, /*lower=*/0L, /*upper=*/10L); + CommitFiles({partial_match_file}); + ICEBERG_UNWRAP_OR_FAIL(auto previous_snapshot, table_->current_snapshot()); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); + delete_files->DeleteFromRowFilter(Expressions::Equal("y", Literal::Long(5L))); + + auto status = delete_files->Commit(); + EXPECT_THAT(status, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(status, + HasErrorMessage("Cannot delete file where some, but not all, rows match " + "filter")); + EXPECT_THAT(status, HasErrorMessage(partial_match_file->file_path)); + + ASSERT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_EQ(snapshot->snapshot_id, previous_snapshot->snapshot_id); +} + +TEST_F(DeleteFilesTest, ValidateFilesExistRejectsMissingPath) { + CommitInitialFiles(); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); + delete_files->DeleteFile(table_location_ + "/data/missing.parquet") + .ValidateFilesExist(); + + auto status = delete_files->Commit(); + EXPECT_THAT(status, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(status, HasErrorMessage("Missing required files to delete")); +} + +} // namespace iceberg diff --git a/src/iceberg/transaction.cc b/src/iceberg/transaction.cc index 673f43769..ac1f08241 100644 --- a/src/iceberg/transaction.cc +++ b/src/iceberg/transaction.cc @@ -32,6 +32,7 @@ #include "iceberg/table_requirement.h" #include "iceberg/table_requirements.h" #include "iceberg/table_update.h" +#include "iceberg/update/delete_files.h" #include "iceberg/update/expire_snapshots.h" #include "iceberg/update/fast_append.h" #include "iceberg/update/merge_append.h" @@ -497,6 +498,13 @@ Result> Transaction::NewMergeAppend() { return merge_append; } +Result> Transaction::NewDeleteFiles() { + ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr delete_files, + DeleteFiles::Make(ctx_->table->name().name, ctx_)); + ICEBERG_RETURN_UNEXPECTED(AddUpdate(delete_files)); + return delete_files; +} + Result> Transaction::NewUpdateStatistics() { ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr update_statistics, UpdateStatistics::Make(ctx_)); diff --git a/src/iceberg/transaction.h b/src/iceberg/transaction.h index 0cc4cb9ac..52a0605c6 100644 --- a/src/iceberg/transaction.h +++ b/src/iceberg/transaction.h @@ -109,6 +109,9 @@ class ICEBERG_EXPORT Transaction : public std::enable_shared_from_this> NewMergeAppend(); + /// \brief Create a new DeleteFiles to delete data files and commit the changes. + Result> NewDeleteFiles(); + /// \brief Create a new SnapshotManager to manage snapshots. Result> NewSnapshotManager(); diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index c82da5132..6c34d3a8d 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -223,6 +223,7 @@ class Transaction; class TransactionContext; /// \brief Update family. +class DeleteFiles; class ExpireSnapshots; class FastAppend; class MergeAppend; diff --git a/src/iceberg/update/delete_files.cc b/src/iceberg/update/delete_files.cc new file mode 100644 index 000000000..9759e3eb9 --- /dev/null +++ b/src/iceberg/update/delete_files.cc @@ -0,0 +1,79 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/update/delete_files.h" + +#include +#include +#include + +#include "iceberg/snapshot.h" +#include "iceberg/transaction.h" +#include "iceberg/util/error_collector.h" +#include "iceberg/util/macros.h" + +namespace iceberg { + +Result> DeleteFiles::Make( + std::string table_name, std::shared_ptr ctx) { + ICEBERG_PRECHECK(!table_name.empty(), "Table name cannot be empty"); + ICEBERG_PRECHECK(ctx != nullptr, "Cannot create DeleteFiles without a context"); + return std::unique_ptr( + new DeleteFiles(std::move(table_name), std::move(ctx))); +} + +DeleteFiles::DeleteFiles(std::string table_name, std::shared_ptr ctx) + : MergingSnapshotUpdate(std::move(table_name), std::move(ctx)) {} + +DeleteFiles& DeleteFiles::DeleteFile(std::string_view path) { + ICEBERG_BUILDER_CHECK(!path.empty(), "Cannot delete an empty file path"); + ICEBERG_BUILDER_RETURN_IF_ERROR(DeleteByPath(path)); + return *this; +} + +DeleteFiles& DeleteFiles::DeleteFile(const std::shared_ptr& file) { + ICEBERG_BUILDER_RETURN_IF_ERROR(DeleteDataFile(file)); + return *this; +} + +DeleteFiles& DeleteFiles::DeleteFromRowFilter(std::shared_ptr expr) { + ICEBERG_BUILDER_RETURN_IF_ERROR(DeleteByRowFilter(std::move(expr))); + return *this; +} + +DeleteFiles& DeleteFiles::CaseSensitive(bool case_sensitive) { + MergingSnapshotUpdate::CaseSensitive(case_sensitive); + return *this; +} + +DeleteFiles& DeleteFiles::ValidateFilesExist() { + validate_files_to_delete_exist_ = true; + return *this; +} + +std::string DeleteFiles::operation() { return DataOperation::kDelete; } + +Status DeleteFiles::Validate(const TableMetadata&, const std::shared_ptr&) { + if (validate_files_to_delete_exist_) { + FailMissingDeletePaths(); + } + return {}; +} + +} // namespace iceberg diff --git a/src/iceberg/update/delete_files.h b/src/iceberg/update/delete_files.h new file mode 100644 index 000000000..1be08e35b --- /dev/null +++ b/src/iceberg/update/delete_files.h @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +/// \file iceberg/update/delete_files.h + +#include +#include +#include + +#include "iceberg/iceberg_export.h" +#include "iceberg/result.h" +#include "iceberg/type_fwd.h" +#include "iceberg/update/merging_snapshot_update.h" + +namespace iceberg { + +/// \brief API for deleting data files from a table. +/// +/// This accumulates data-file deletions, produces a new snapshot, and commits that +/// snapshot as current. File paths are matched exactly against table metadata values; +/// equivalent but differently-normalized URIs are not considered matches. +class ICEBERG_EXPORT DeleteFiles : public MergingSnapshotUpdate { + public: + static Result> Make( + std::string table_name, std::shared_ptr ctx); + + /// \brief Delete a data-file path from the table. + DeleteFiles& DeleteFile(std::string_view path); + + /// \brief Delete a data file tracked by object identity and path. + DeleteFiles& DeleteFile(const std::shared_ptr& file); + + /// \brief Delete files whose rows all match the given expression. + DeleteFiles& DeleteFromRowFilter(std::shared_ptr expr); + + /// \brief Set case sensitivity for expression binding. + DeleteFiles& CaseSensitive(bool case_sensitive); + + /// \brief Validate that explicitly requested deleted files still exist. + DeleteFiles& ValidateFilesExist(); + + std::string operation() override; + + protected: + Status Validate(const TableMetadata& current_metadata, + const std::shared_ptr& snapshot) override; + + private: + DeleteFiles(std::string table_name, std::shared_ptr ctx); + + bool validate_files_to_delete_exist_ = false; +}; + +} // namespace iceberg diff --git a/src/iceberg/update/meson.build b/src/iceberg/update/meson.build index 031c79452..9f950e8d0 100644 --- a/src/iceberg/update/meson.build +++ b/src/iceberg/update/meson.build @@ -17,6 +17,7 @@ install_headers( [ + 'delete_files.h', 'expire_snapshots.h', 'fast_append.h', 'merge_append.h', From 82b0adb1ab8989f8e5df47cb58419a28727ca2a7 Mon Sep 17 00:00:00 2001 From: Zehua Zou Date: Mon, 22 Jun 2026 16:08:12 +0800 Subject: [PATCH 02/16] chore: make pre-commit and ci clang-format version consistent (#769) --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 522923a47..cd468a464 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -33,7 +33,7 @@ repos: exclude: ^src/iceberg/catalog/hive/gen-cpp/ - repo: https://github.com/pre-commit/mirrors-clang-format - rev: v20.1.8 + rev: v22.1.5 hooks: - id: clang-format exclude_types: [json] From 28163ce3ff26963b0310dff6dbcf2f6c14f94748 Mon Sep 17 00:00:00 2001 From: Zehua Zou Date: Mon, 22 Jun 2026 16:42:41 +0800 Subject: [PATCH 03/16] feat: parallelize reading manifests (#697) --- src/iceberg/manifest/manifest_group.cc | 119 +++++++----- src/iceberg/manifest/manifest_group.h | 11 ++ src/iceberg/test/CMakeLists.txt | 1 + src/iceberg/test/executor_util_test.cc | 189 ++++++++++++++++++ src/iceberg/test/manifest_group_test.cc | 5 + src/iceberg/test/meson.build | 1 + src/iceberg/util/executor_util_internal.h | 221 ++++++++++++++++++++++ 7 files changed, 499 insertions(+), 48 deletions(-) create mode 100644 src/iceberg/test/executor_util_test.cc create mode 100644 src/iceberg/util/executor_util_internal.h diff --git a/src/iceberg/manifest/manifest_group.cc b/src/iceberg/manifest/manifest_group.cc index 61bb57da2..932f3bf3d 100644 --- a/src/iceberg/manifest/manifest_group.cc +++ b/src/iceberg/manifest/manifest_group.cc @@ -21,6 +21,8 @@ #include #include +#include +#include #include #include #include @@ -41,6 +43,7 @@ #include "iceberg/type.h" #include "iceberg/util/checked_cast.h" #include "iceberg/util/content_file_util.h" +#include "iceberg/util/executor_util_internal.h" #include "iceberg/util/macros.h" namespace iceberg { @@ -189,6 +192,11 @@ ManifestGroup& ManifestGroup::ColumnsToKeepStats(std::unordered_set col return *this; } +ManifestGroup& ManifestGroup::PlanWith(OptionalExecutor executor) { + executor_ = executor; + return *this; +} + Result>> ManifestGroup::PlanFiles() { auto create_file_scan_tasks = [this](std::vector&& entries, @@ -343,10 +351,23 @@ Result> ManifestGroup::MakeReader( Result>> ManifestGroup::ReadEntries() { + // TODO(zehua): Replace with a thread-safe LRU cache. + std::shared_mutex eval_cache_mutex; std::unordered_map> eval_cache; + auto get_manifest_evaluator = [&](int32_t spec_id) -> Result { - if (eval_cache.contains(spec_id)) { - return eval_cache[spec_id].get(); + { + std::shared_lock lock(eval_cache_mutex); + auto iter = eval_cache.find(spec_id); + if (iter != eval_cache.end()) { + return iter->second.get(); + } + } + + std::lock_guard lock(eval_cache_mutex); + auto iter = eval_cache.find(spec_id); + if (iter != eval_cache.end()) { + return iter->second.get(); } auto spec_iter = specs_by_id_.find(spec_id); @@ -376,61 +397,63 @@ ManifestGroup::ReadEntries() { Evaluator::Make(*DataFileFilterSchema(), file_filter_, case_sensitive_)); } - std::unordered_map> result; + return ParallelCollect( + executor_, data_manifests_, + [&](const ManifestFile& manifest) + -> Result>> { + const int32_t spec_id = manifest.partition_spec_id; - // TODO(gangwu): Parallelize reading manifests - for (const auto& manifest : data_manifests_) { - const int32_t spec_id = manifest.partition_spec_id; - - ICEBERG_ASSIGN_OR_RAISE(auto manifest_evaluator, get_manifest_evaluator(spec_id)); - ICEBERG_ASSIGN_OR_RAISE(bool should_match, manifest_evaluator->Evaluate(manifest)); - if (!should_match) { - // Skip this manifest because it doesn't match partition filter - continue; - } + ICEBERG_ASSIGN_OR_RAISE(auto manifest_evaluator, get_manifest_evaluator(spec_id)); + ICEBERG_ASSIGN_OR_RAISE(bool should_match, + manifest_evaluator->Evaluate(manifest)); + if (!should_match) { + // Skip this manifest because it doesn't match partition filter + return {}; + } - if (ignore_deleted_) { - // only scan manifests that have entries other than deletes - if (!manifest.has_added_files() && !manifest.has_existing_files()) { - continue; - } - } + if (ignore_deleted_) { + // only scan manifests that have entries other than deletes + if (!manifest.has_added_files() && !manifest.has_existing_files()) { + return {}; + } + } - if (ignore_existing_) { - // only scan manifests that have entries other than existing - if (!manifest.has_added_files() && !manifest.has_deleted_files()) { - continue; - } - } + if (ignore_existing_) { + // only scan manifests that have entries other than existing + if (!manifest.has_added_files() && !manifest.has_deleted_files()) { + return {}; + } + } - // Read manifest entries - ICEBERG_ASSIGN_OR_RAISE(auto reader, MakeReader(manifest)); - ICEBERG_ASSIGN_OR_RAISE(auto entries, - ignore_deleted_ ? reader->LiveEntries() : reader->Entries()); + // Read manifest entries + ICEBERG_ASSIGN_OR_RAISE(auto reader, MakeReader(manifest)); + ICEBERG_ASSIGN_OR_RAISE( + auto entries, ignore_deleted_ ? reader->LiveEntries() : reader->Entries()); - for (auto& entry : entries) { - if (ignore_existing_ && entry.status == ManifestStatus::kExisting) { - continue; - } + std::unordered_map> manifest_result; - if (data_file_evaluator != nullptr) { - DataFileStructLike data_file(*entry.data_file); - ICEBERG_ASSIGN_OR_RAISE(bool should_match, - data_file_evaluator->Evaluate(data_file)); - if (!should_match) { - continue; - } - } + for (auto& entry : entries) { + if (ignore_existing_ && entry.status == ManifestStatus::kExisting) { + continue; + } - if (!manifest_entry_predicate_(entry)) { - continue; - } + if (data_file_evaluator != nullptr) { + DataFileStructLike data_file(*entry.data_file); + ICEBERG_ASSIGN_OR_RAISE(bool should_match, + data_file_evaluator->Evaluate(data_file)); + if (!should_match) { + continue; + } + } - result[spec_id].push_back(std::move(entry)); - } - } + if (!manifest_entry_predicate_(entry)) { + continue; + } - return result; + manifest_result[spec_id].push_back(std::move(entry)); + } + return manifest_result; + }); } } // namespace iceberg diff --git a/src/iceberg/manifest/manifest_group.h b/src/iceberg/manifest/manifest_group.h index 10b552786..09ae4a503 100644 --- a/src/iceberg/manifest/manifest_group.h +++ b/src/iceberg/manifest/manifest_group.h @@ -36,6 +36,7 @@ #include "iceberg/result.h" #include "iceberg/type_fwd.h" #include "iceberg/util/error_collector.h" +#include "iceberg/util/executor.h" namespace iceberg { @@ -94,6 +95,9 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { /// \brief Set a custom manifest entry filter predicate. /// + /// When an executor is configured with PlanWith(), this predicate may be called + /// concurrently. Callers must synchronize any captured mutable state. + /// /// \param predicate A function that returns true if the entry should be included. ManifestGroup& FilterManifestEntries( std::function predicate); @@ -120,6 +124,12 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { /// \param column_ids Field IDs of columns whose statistics should be preserved. ManifestGroup& ColumnsToKeepStats(std::unordered_set column_ids); + /// \brief Configure an optional executor for manifest planning. + /// + /// \param executor Executor to use, or std::nullopt to plan manifests serially. + /// \return Reference to this for method chaining. + ManifestGroup& PlanWith(OptionalExecutor executor); + /// \brief Plan scan tasks for all matching data files. Result>> PlanFiles(); @@ -158,6 +168,7 @@ class ICEBERG_EXPORT ManifestGroup : public ErrorCollector { std::function manifest_entry_predicate_; std::vector columns_; std::unordered_set columns_to_keep_stats_; + OptionalExecutor executor_; bool case_sensitive_ = true; bool ignore_deleted_ = false; bool ignore_existing_ = false; diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index 1d8ea472b..c8c815797 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -139,6 +139,7 @@ add_iceberg_test(util_test lazy_test.cc location_util_test.cc math_util_internal_test.cc + executor_util_test.cc roaring_position_bitmap_test.cc position_delete_index_test.cc position_delete_range_consumer_test.cc diff --git a/src/iceberg/test/executor_util_test.cc b/src/iceberg/test/executor_util_test.cc new file mode 100644 index 000000000..3bb9ad9fc --- /dev/null +++ b/src/iceberg/test/executor_util_test.cc @@ -0,0 +1,189 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +#include "iceberg/result.h" +#include "iceberg/test/executor.h" +#include "iceberg/test/matchers.h" +#include "iceberg/util/executor_util_internal.h" + +namespace iceberg { + +using ::testing::ElementsAre; +using ::testing::Pair; +using ::testing::UnorderedElementsAre; + +namespace { + +struct IntTask { + Result> operator()(int) { + return Result>{std::vector{}}; + } +}; + +struct BoolTask { + Result> operator()(bool) { + return Result>{std::vector{}}; + } +}; + +static_assert(internal::ParallelCollectible&, IntTask>); +static_assert( + std::same_as&>(), + IntTask{})), + Result>>); +static_assert(!internal::ParallelCollectible); +static_assert(!internal::ParallelCollectible&, BoolTask>); + +} // namespace + +TEST(ParallelReduceTest, MergesSets) { + std::vector> values = {{1, 2}, {2, 3}, {}}; + + auto result = ParallelReduce>::Reduce(values); + + EXPECT_THAT(result, UnorderedElementsAre(1, 2, 3)); +} + +TEST(ParallelReduceTest, JoinsVectors) { + std::vector> values = {{1, 2}, {}, {3}}; + + auto result = ParallelReduce>::Reduce(values); + + EXPECT_THAT(result, ElementsAre(1, 2, 3)); +} + +TEST(ParallelReduceTest, MergesMapsAndAppendsDuplicateVectors) { + std::vector>> values = { + {{1, {"a"}}, {2, {"b"}}}, {{1, {"c"}}, {3, {"d"}}}}; + + auto result = + ParallelReduce>>::Reduce(values); + + EXPECT_THAT(result, + UnorderedElementsAre(Pair(1, ElementsAre("a", "c")), + Pair(2, ElementsAre("b")), Pair(3, ElementsAre("d")))); +} + +TEST(ParallelReduceTest, ReducesPairElements) { + using Value = std::pair, std::vector>; + + std::vector values = {{{1}, {"a"}}, {{2}, {"b"}}}; + + auto result = ParallelReduce::Reduce(values); + + EXPECT_THAT(result.first, UnorderedElementsAre(1, 2)); + EXPECT_THAT(result.second, ElementsAre("a", "b")); +} + +TEST(ParallelReduceTest, ReducesTupleElements) { + using Value = std::tuple, std::vector>; + + std::vector values = {{{1}, {"a"}}, {{2}, {"b"}}}; + + auto result = ParallelReduce::Reduce(values); + + EXPECT_THAT(std::get<0>(result), UnorderedElementsAre(1, 2)); + EXPECT_THAT(std::get<1>(result), ElementsAre("a", "b")); +} + +TEST(ParallelReduceTest, ReducesViewElements) { + using Value = std::tuple, std::vector>; + + std::vector values = {{{0}, {"skip"}}, {{1}, {"a"}}, {{2}, {"b"}}}; + + auto result = ParallelReduce::Reduce(values | std::views::drop(1)); + + EXPECT_THAT(std::get<0>(result), UnorderedElementsAre(1, 2)); + EXPECT_THAT(std::get<1>(result), ElementsAre("a", "b")); +} + +TEST(ParallelCollectTest, CollectsSingleRange) { + std::vector input = {1, 2, 3}; + + auto result = ParallelCollect(std::nullopt, input, [](int value) { + return Result>{{value * 2}}; + }); + + EXPECT_THAT(result, IsOk()); + EXPECT_THAT(*result, UnorderedElementsAre(2, 4, 6)); +} + +TEST(ParallelCollectTest, KeepsTupleResultFromSingleRange) { + std::vector input = {1, 2}; + + auto result = ParallelCollect( + std::nullopt, input, + [](int value) + -> Result, std::vector>> { + return {{{value}, {std::to_string(value)}}}; + }); + + EXPECT_THAT(result, IsOk()); + EXPECT_THAT(std::get<0>(*result), UnorderedElementsAre(1, 2)); + EXPECT_THAT(std::get<1>(*result), ElementsAre("1", "2")); +} + +TEST(ParallelCollectTest, CollectsMultipleRanges) { + test::ThreadExecutor executor; + std::vector left = {1, 2}; + std::vector right = {"a", "b"}; + + auto result = ParallelCollect( + std::ref(executor), left, + [](int value) { return Result>{{value}}; }, right, + [](const std::string& value) { return Result>{{value}}; }); + + EXPECT_THAT(result, IsOk()); + EXPECT_THAT(std::get<0>(*result), UnorderedElementsAre(1, 2)); + EXPECT_THAT(std::get<1>(*result), ElementsAre("a", "b")); + EXPECT_EQ(executor.submit_count(), 4); +} + +TEST(ParallelCollectTest, PropagatesTaskErrors) { + std::vector input = {1, 2, 3}; + std::atomic calls = 0; + + auto result = ParallelCollect(std::nullopt, input, [&calls](int value) { + calls.fetch_add(1, std::memory_order_relaxed); + if (value == 2) { + return Result>{ValidationFailed("bad value")}; + } + return Result>{{value}}; + }); + + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_EQ(calls.load(std::memory_order_relaxed), 3); +} + +} // namespace iceberg diff --git a/src/iceberg/test/manifest_group_test.cc b/src/iceberg/test/manifest_group_test.cc index 70e2cea99..aa2d6810d 100644 --- a/src/iceberg/test/manifest_group_test.cc +++ b/src/iceberg/test/manifest_group_test.cc @@ -39,6 +39,7 @@ #include "iceberg/partition_spec.h" #include "iceberg/schema.h" #include "iceberg/table_scan.h" +#include "iceberg/test/executor.h" #include "iceberg/test/matchers.h" #include "iceberg/transform.h" #include "iceberg/type.h" @@ -625,11 +626,15 @@ TEST_P(ManifestGroupTest, MultipleDataManifests) { auto group, ManifestGroup::Make(file_io_, schema_, GetSpecsById(), std::move(manifests))); + test::ThreadExecutor executor; + group->PlanWith(std::ref(executor)); + // Plan files - should return files from both manifests ICEBERG_UNWRAP_OR_FAIL(auto tasks, group->PlanFiles()); ASSERT_EQ(tasks.size(), 2); EXPECT_THAT(GetPaths(tasks), testing::UnorderedElementsAre("/path/to/data1.parquet", "/path/to/data2.parquet")); + EXPECT_EQ(executor.submit_count(), 2); } TEST_P(ManifestGroupTest, PartitionFilter) { diff --git a/src/iceberg/test/meson.build b/src/iceberg/test/meson.build index a76a15553..a17d9841a 100644 --- a/src/iceberg/test/meson.build +++ b/src/iceberg/test/meson.build @@ -93,6 +93,7 @@ iceberg_tests = { 'data_file_set_test.cc', 'decimal_test.cc', 'endian_test.cc', + 'executor_util_test.cc', 'file_io_test.cc', 'formatter_test.cc', 'lazy_test.cc', diff --git a/src/iceberg/util/executor_util_internal.h b/src/iceberg/util/executor_util_internal.h new file mode 100644 index 000000000..5eed3a8bd --- /dev/null +++ b/src/iceberg/util/executor_util_internal.h @@ -0,0 +1,221 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "iceberg/result.h" +#include "iceberg/util/executor.h" +#include "iceberg/util/macros.h" +#include "iceberg/util/task_group.h" + +namespace iceberg { + +template +struct ParallelReduce; + +namespace internal { + +template +concept ParallelReducible = requires(std::vector& values) { + typename ParallelReduce::result_type; + { + ParallelReduce::Reduce(values) + } -> std::same_as::result_type>; +}; + +template +using ParallelCollectValueT = + ResultValueT&, + std::add_lvalue_reference_t>>>>; + +template +struct ParallelCollectTraits { + using args_tuple_type = std::tuple; + using input_type = std::tuple_element_t; + using task_type = std::tuple_element_t; + using value_type = ParallelCollectValueT; +}; + +template +concept ParallelCollectible = + std::ranges::forward_range && std::ranges::sized_range && + std::is_lvalue_reference_v> && + requires(std::remove_reference_t& task, + std::ranges::range_reference_t item) { + { std::invoke(task, item) } -> AsResult; + requires(!std::same_as>); + requires std::default_initializable>; + requires ParallelReducible, Options...>; + }; + +} // namespace internal + +template +struct ParallelReduce> { + using result_type = std::unordered_set; + + template + static result_type Reduce(Values&& values) { + result_type result; + for (auto&& value : values) { + result.merge(value); + } + return result; + } +}; + +template +struct ParallelReduce> { + using result_type = std::vector; + + template + static result_type Reduce(Values&& values) { + return std::forward(values) | std::views::join | std::views::as_rvalue | + std::ranges::to(); + } +}; + +template +struct ParallelReduce, MapArgs...>> { + using result_type = std::unordered_map, MapArgs...>; + + template + static result_type Reduce(Values&& values) { + result_type result; + for (auto&& value : values) { + result.merge(value); + for (auto& [key, entries] : value) { + auto& out = result[key]; + out.insert(out.end(), std::make_move_iterator(entries.begin()), + std::make_move_iterator(entries.end())); + } + } + return result; + } +}; + +template +struct ParallelReduce> { + using result_type = std::pair::result_type, + typename ParallelReduce::result_type>; + + template + static result_type Reduce(Values&& values) { + return {ParallelReduce::Reduce(values | std::views::elements<0>), + ParallelReduce::Reduce(values | std::views::elements<1>)}; + } +}; + +template +struct ParallelReduce> { + using result_type = std::tuple::result_type...>; + + template + static result_type Reduce(Values&& values) { + return Reduce(values, std::index_sequence_for{}); + } + + private: + template + static result_type Reduce(Values&& values, std::index_sequence) { + return result_type{ParallelReduce>>::Reduce( + values | std::views::elements)...}; + } +}; + +template + requires(sizeof...(Args) >= 2 && sizeof...(Args) % 2 == 0 && + [](std::index_sequence) consteval { + return (internal::ParallelCollectible< + typename internal::ParallelCollectTraits::input_type, + typename internal::ParallelCollectTraits::task_type, + Options...> && + ...); + }(std::make_index_sequence{})) +auto ParallelCollect(OptionalExecutor executor, Args&&... args) { + constexpr std::size_t pair_count = sizeof...(Args) / 2; + using indices = std::make_index_sequence; + + auto args_tuple = std::forward_as_tuple(std::forward(args)...); + + auto values_tuple = [&](std::index_sequence) { + return std::tuple{[&] { + using traits = internal::ParallelCollectTraits; + + return std::vector( + std::ranges::size(std::get(args_tuple))); + }()...}; + }(indices{}); + + auto reduce_all = [&](std::index_sequence) { + auto reduce_one = [&] { + using traits = internal::ParallelCollectTraits; + using value_type = typename traits::value_type; + return ParallelReduce::Reduce( + std::get(values_tuple)); + }; + + if constexpr (pair_count == 1) { + return reduce_one.template operator()<0>(); + } else { + return std::tuple{reduce_one.template operator()()...}; + } + }; + + using result_type = decltype(reduce_all(indices{})); + + TaskGroup group; + group.SetExecutor(executor); + + [&](std::index_sequence) { + ( + [&] { + for (auto&& [item, value] : + std::views::zip(std::get(args_tuple), std::get(values_tuple))) { + group.Submit([&]() -> Status { + ICEBERG_ASSIGN_OR_RAISE(value, + std::invoke(std::get(args_tuple), item)); + return {}; + }); + } + }(), + ...); + }(indices{}); + + auto status = std::move(group).Run(); + if (!status.has_value()) { + return Result(std::unexpected(status.error())); + } + + return Result(reduce_all(indices{})); +} + +} // namespace iceberg From a38e0cdef88a2cd3673a78e659cbadb575fa6c2e Mon Sep 17 00:00:00 2001 From: kamcheungting-db <91572897+kamcheungting-db@users.noreply.github.com> Date: Mon, 22 Jun 2026 02:14:36 -0700 Subject: [PATCH 04/16] feat(logging): add Logger interface and default logger (#723) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part 2 of the logging stack (builds on #722). Adds the logging API and a swappable default logger — the foundation the backends and macros plug into. **What's here** - `Logger`: the pluggable sink interface (`ShouldLog` / `Log` / `SetLevel` / `Flush` / `Initialize`). `ShouldLog()` is the single source of truth for runtime filtering. - `LogMessage` owns its formatted text so a sink can safely keep a record; reserves an attributes field for future structured logging. - Process-global default logger: `GetDefaultLogger` / `SetDefaultLogger` / `SetDefaultLevel`, with a lock-free thread-local fast path so logging stays cheap. - `Initialize` applies the `level` property, so config-driven levels actually work. `CurrentLogger()` is safe to call even from a `thread_local` destructor during thread shutdown. - `logger.h` stays backend-agnostic (never includes the build config header), so consumers see one stable API regardless of backend. **Examples** — using the API directly (the `LOG_*` macros that wrap it arrive in #725): ```cpp // A custom sink, installed as the process default. class MySink : public Logger { public: bool ShouldLog(LogLevel level) const override { return level >= level_; } void Log(LogMessage&& m) noexcept override { write_line(m.message); } void SetLevel(LogLevel level) override { level_ = level; } LogLevel level() const override { return level_; } private: std::atomic level_{LogLevel::kInfo}; }; SetDefaultLogger(std::make_shared()); // install process-wide SetDefaultLevel(LogLevel::kDebug); // adjust the threshold auto logger = GetDefaultLogger(); // borrow the current default if (logger->ShouldLog(LogLevel::kInfo)) { logger->Log(LogMessage{.level = LogLevel::kInfo, .message = "scan ready"}); } // Or configure from catalog-style properties (applies the "level" key): auto sink = std::make_shared(); auto status = sink->Initialize({{std::string(kLevelProperty), "warn"}}); // -> kWarn ``` The same example is documented inline in `logger.h`. **Tests** — `logger_test`: default-logger API, level-from-property, invalid level rejected, concurrent swap/read, and logging during thread teardown. Built and run with clang/libc++ (spdlog ON and OFF). This pull request and its description were written by Isaac. --- src/iceberg/CMakeLists.txt | 1 + src/iceberg/logging/logger.cc | 217 ++++++++++++ src/iceberg/logging/logger.h | 373 ++++++++++++++++++++ src/iceberg/logging/meson.build | 2 +- src/iceberg/meson.build | 1 + src/iceberg/test/CMakeLists.txt | 2 +- src/iceberg/test/logger_test.cc | 450 ++++++++++++++++++++++++ src/iceberg/test/logging_test_helpers.h | 84 +++++ src/iceberg/test/meson.build | 2 +- 9 files changed, 1129 insertions(+), 3 deletions(-) create mode 100644 src/iceberg/logging/logger.cc create mode 100644 src/iceberg/logging/logger.h create mode 100644 src/iceberg/test/logger_test.cc create mode 100644 src/iceberg/test/logging_test_helpers.h diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index b3cc8a7a9..ea76c641a 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -49,6 +49,7 @@ set(ICEBERG_SOURCES inheritable_metadata.cc json_serde.cc location_provider.cc + logging/logger.cc manifest/manifest_adapter.cc manifest/manifest_entry.cc manifest/manifest_filter_manager.cc diff --git a/src/iceberg/logging/logger.cc b/src/iceberg/logging/logger.cc new file mode 100644 index 000000000..3adbf75dd --- /dev/null +++ b/src/iceberg/logging/logger.cc @@ -0,0 +1,217 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/logging/logger.h" + +#include +#include +#include +#include +#include +#include + +namespace iceberg { + +namespace { + +/// \brief Logger that drops every record. +class NoopLogger final : public Logger { + public: + bool ShouldLog(LogLevel /*level*/) const noexcept override { return false; } + void Log(LogMessage&& /*message*/) noexcept override {} + void SetLevel(LogLevel /*level*/) noexcept override {} + LogLevel level() const noexcept override { return LogLevel::kOff; } + bool IsNoop() const override { return true; } +}; + +/// \brief Construct the process default logger for this build configuration. +/// +/// This block ships only the interface and the no-op logger; the concrete +/// std::cerr and spdlog sinks (and the build-config selection between them) +/// arrive in later blocks, which update this factory. +std::shared_ptr MakeDefaultLogger() { return std::make_shared(); } + +/// \brief The process-global default-logger slot. +struct DefaultSlot { + std::mutex mtx; + std::shared_ptr logger; + // Seeded to 1 so a fresh thread (tls_gen == 0) always refreshes on first use. + std::atomic gen{1}; + + DefaultSlot() : logger(MakeDefaultLogger()) {} +}; + +/// \brief Immortal (leaked, hence reachable -> LSan-clean) accessor for the slot. +DefaultSlot& Slot() { + static auto* slot = new DefaultSlot(); + return *slot; +} + +/// \brief A thread's cached view of the default logger and the generation it was +/// cached at. Heap-allocated per thread and freed at thread exit (see +/// AccessThreadCache). `override_` is the active ScopedLogger binding for this +/// thread (empty when none); when set it supersedes the cached default. +struct ThreadCache { + std::shared_ptr logger; + uint64_t gen = 0; // 0 != Slot().gen (seeded to 1) -> first use always refreshes + std::shared_ptr override_; // active ScopedLogger binding, empty if none +}; + +} // namespace + +std::shared_ptr Logger::Noop() { + // Intentionally leaked: reachable via the function-local static (LSan-clean) + // and never destroyed, so logging during static teardown stays safe. + static auto* instance = new std::shared_ptr(std::make_shared()); + return *instance; +} + +std::shared_ptr GetDefaultLogger() { + DefaultSlot& slot = Slot(); + std::lock_guard lock(slot.mtx); + return slot.logger; +} + +void SetDefaultLogger(std::shared_ptr logger) { + if (!logger) { + logger = Logger::Noop(); + } + DefaultSlot& slot = Slot(); + std::lock_guard lock(slot.mtx); + slot.logger = std::move(logger); + // Publish the swap; the mutex provides the happens-before, gen is a detector. + slot.gen.fetch_add(1, std::memory_order_relaxed); +} + +void SetDefaultLevel(LogLevel level) { + DefaultSlot& slot = Slot(); + std::lock_guard lock(slot.mtx); + slot.logger->SetLevel(level); +} + +namespace internal { + +namespace { + +/// \brief The one place the per-thread cache's lifetime is managed; shared by +/// CurrentLogger, GetCurrentLogger, and the ScopedLogger helpers. +/// +/// Safe to call from any thread_local destructor during teardown: `dead`/`cache` +/// are trivially destructible so their storage lives the whole thread, and `guard` +/// (the only one with a destructor) sets `dead` before freeing the cache -- so a +/// late caller gets nullptr instead of touching freed memory, in any order. +/// +/// \param create allocate the cache if absent (writers pass true; read-only +/// callers pass false so a query never allocates). Returns nullptr while tearing +/// down, or when !create and no cache exists -- caller falls back to global/noop. +ThreadCache* AccessThreadCache(bool create) noexcept { + static thread_local bool dead = false; + static thread_local ThreadCache* cache = nullptr; + static thread_local struct Guard { + ~Guard() { + dead = true; // mark BEFORE freeing, so a re-entrant log hits the fallback + delete cache; + cache = nullptr; + } + } guard; + std::ignore = guard; // mark the thread_local as intentionally used (its dtor is + // registered by reaching the declaration above) + + if (dead) return nullptr; + if (cache == nullptr && create) cache = new ThreadCache(); + return cache; +} + +} // namespace + +const std::shared_ptr& CurrentLogger() noexcept { + ThreadCache* cache = AccessThreadCache(/*create=*/true); + if (cache == nullptr) { + // Thread teardown after the cache was freed: serve an immortal no-op so a log + // from a later thread_local destructor is safe. Such teardown logs are dropped. + static auto* fallback = new std::shared_ptr(Logger::Noop()); + return *fallback; + } + + // A scoped override wins -- no lock, no gen load. After the cache check (deref is + // teardown-safe), before the refresh (keeps the override path cheapest). + if (cache->override_) return cache->override_; + + DefaultSlot& slot = Slot(); + uint64_t current = slot.gen.load(std::memory_order_relaxed); + if (current != cache->gen) { + std::lock_guard lock(slot.mtx); + cache->logger = slot.logger; + cache->gen = current; + } + return cache->logger; +} + +std::shared_ptr ExchangeThreadOverride(std::shared_ptr next) noexcept { + ThreadCache* cache = AccessThreadCache(/*create=*/true); + if (cache == nullptr) return {}; // tearing down -> binding a scope is a no-op + std::shared_ptr prev = std::move(cache->override_); + cache->override_ = std::move(next); + return prev; // empty == no override was active +} + +void RestoreThreadOverride(std::shared_ptr prev) noexcept { + ThreadCache* cache = AccessThreadCache(/*create=*/false); // never allocate to restore + if (cache == nullptr) return; // dead or never created -> nothing to restore + cache->override_ = std::move(prev); +} + +void Emit(Logger& logger, LogLevel level, const std::source_location& location, + std::string&& message) { + logger.Log(LogMessage{.level = level, + .message = std::move(message), + .location = location, + .attributes = {}}); +} + +void EmitFormatError(Logger& logger, LogLevel level, + const std::source_location& location) noexcept { + // Fixed short literal (<= 15 bytes, fits SSO on libstdc++/libc++/MSVC -> no heap + // allocation), no std::format, no retry. Cannot throw or recurse. + logger.Log(LogMessage{.level = level, + .message = std::string(""), + .location = location, + .attributes = {}}); +} + +} // namespace internal + +ScopedLogger::ScopedLogger(std::shared_ptr logger) noexcept + : previous_(internal::ExchangeThreadOverride(std::move(logger))) {} + +ScopedLogger::~ScopedLogger() { internal::RestoreThreadOverride(std::move(previous_)); } + +std::shared_ptr GetCurrentLogger() { + ThreadCache* cache = internal::AccessThreadCache(/*create=*/false); + if (cache == nullptr) { + // Teardown, or no per-thread cache ever created -> no override possible. + // GetDefaultLogger() touches only the immortal slot (no thread_local), so it + // stays valid during teardown, and is never null. + return GetDefaultLogger(); + } + if (cache->override_) return cache->override_; + return GetDefaultLogger(); +} + +} // namespace iceberg diff --git a/src/iceberg/logging/logger.h b/src/iceberg/logging/logger.h new file mode 100644 index 000000000..8a642f833 --- /dev/null +++ b/src/iceberg/logging/logger.h @@ -0,0 +1,373 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +/// \file iceberg/logging/logger.h +/// \brief Pluggable logging interface and the process-global default logger. +/// +/// This header is backend-agnostic: it never includes the build-generated +/// backend configuration header and never references the spdlog feature macro, +/// so consumers see one stable API regardless of how the backend was configured. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "iceberg/iceberg_export.h" +#include "iceberg/logging/log_level.h" +#include "iceberg/result.h" + +namespace iceberg { + +/// \brief A structured key/value attribute attached to a log record. +/// +/// Both key and value are owned so a sink may retain the record safely. Engine +/// loggers can surface these as discrete fields (query id, task id, table name, +/// snapshot id, file path, ...); see LogMessage::Builder to populate them. +struct ICEBERG_EXPORT LogAttribute { + std::string key; + std::string value; +}; + +/// \brief A single log record handed to a Logger. +/// +/// The formatted message is owned (moved in by the logging macros), so a sink +/// may safely retain the record beyond the Log() call. The member set must not +/// depend on the build's logging backend (the spdlog backend never appears here). +/// Use LogMessage::Builder for a readable way to assemble one, especially with +/// structured attributes. +struct ICEBERG_EXPORT LogMessage { + LogLevel level = LogLevel::kOff; + std::string message; + std::source_location location = std::source_location::current(); + std::vector attributes; + + class Builder; +}; + +/// \brief Fluent builder for LogMessage, the easy path to attach structured +/// attributes. +/// +/// Example: +/// auto record = LogMessage::Builder(LogLevel::kInfo) +/// .Message("scan finished") +/// .Attribute("table", table_name) +/// .Attribute("snapshot_id", std::to_string(id)) +/// .Build(); +/// logger->Log(std::move(record)); +/// +/// The location defaults to the caller's construction site (captured via the +/// constructor's default argument); override it with Location() (e.g. to forward +/// a caller's std::source_location). +class ICEBERG_EXPORT LogMessage::Builder { + public: + explicit Builder(LogLevel level, + std::source_location location = std::source_location::current()) + : level_(level), location_(location) {} + + /// \brief Set the already-formatted message text. + Builder& Message(std::string message) { + message_ = std::move(message); + return *this; + } + + /// \brief Append a structured key/value attribute. + Builder& Attribute(std::string key, std::string value) { + attributes_.push_back(LogAttribute{.key = std::move(key), .value = std::move(value)}); + return *this; + } + + /// \brief Override the record's source location (defaults to the build site). + Builder& Location(std::source_location location) { + location_ = location; + return *this; + } + + /// \brief Materialize the LogMessage, moving the accumulated state out. + LogMessage Build() { + return LogMessage{.level = level_, + .message = std::move(message_), + .location = location_, + .attributes = std::move(attributes_)}; + } + + private: + LogLevel level_; + std::string message_; + // `location_` is a trivially copyable members no need to move. + std::source_location location_; + std::vector attributes_; +}; + +/// \brief Well-known Logger::Initialize() property keys. +/// +/// `level` is honored by the base Logger::Initialize (parsed via +/// LogLevelFromString). `pattern` is honored by the formatting sinks +/// (CerrLogger, SpdLogger). +inline constexpr std::string_view kLevelProperty = "level"; +inline constexpr std::string_view kPatternProperty = "pattern"; + +/// \brief Pluggable logging sink. +/// +/// ShouldLog() is the single authority for runtime filtering -- the macros call +/// it on every (compile-time-enabled) statement, so level changes by any path +/// take effect immediately. Implementations must be thread-safe and must not +/// throw. They must also obey: +/// - No reentrancy: Log()/Flush() must not call the logging macros or +/// GetDefaultLogger() (UB -- deadlock with mutex-based sinks). +/// - level() is an accessor consistent with ShouldLog (used by SetDefaultLevel +/// and introspection); ShouldLog may implement finer logic than a level compare. +class ICEBERG_EXPORT Logger { + public: + virtual ~Logger() = default; + + /// \brief Property-based setup, called by Loggers::Load() before first use. + /// + /// The base implementation applies the "level" property (parsed via + /// LogLevelFromString); an unrecognized value is an InvalidArgument error. + /// Formatting sinks override this to also apply "pattern" and then delegate + /// to this base for "level". + virtual Status Initialize( + const std::unordered_map& properties) { + if (auto it = properties.find(std::string(kLevelProperty)); it != properties.end()) { + auto parsed = LogLevelFromString(it->second); + if (!parsed) return std::unexpected(parsed.error()); + SetLevel(*parsed); + } + return {}; + } + + /// \brief Cheap check whether a record at \p level would be emitted. + virtual bool ShouldLog(LogLevel level) const noexcept = 0; + + /// \brief Emit one (already-formatted) record, taking ownership. Must not throw. + virtual void Log(LogMessage&& message) noexcept = 0; + + /// \brief Set the minimum level this logger emits. + virtual void SetLevel(LogLevel level) noexcept = 0; + + /// \brief Return the minimum level this logger emits. + virtual LogLevel level() const noexcept = 0; + + /// \brief Flush any buffered output. Must not throw; best-effort on the fatal path. + virtual void Flush() noexcept {} + + /// \brief Return true if this logger is a no-op. + virtual bool IsNoop() const { return false; } + + /// \brief Return a shared, immortal no-op logger singleton. + static std::shared_ptr Noop(); +}; + +/// \brief Return the process-global default logger (never null). +/// +/// Off the hot path -- acquires the slot lock and returns an owning copy. The +/// logging macros use the cheaper internal hot-path accessor instead. +ICEBERG_EXPORT std::shared_ptr GetDefaultLogger(); + +/// \brief Return the effective logger for this thread (never null): the active +/// ScopedLogger binding if any, else the global default. +/// +/// Off the hot path -- returns an owning copy, e.g. to capture the current logger +/// and re-bind it on a worker thread (see ScopedLogger). During teardown, prefer +/// the Log(...) overloads over emitting through this handle. +ICEBERG_EXPORT std::shared_ptr GetCurrentLogger(); + +/// \brief Install a new process-global default logger. +/// +/// A null argument installs the no-op logger. Thread-safe; intended for +/// occasional (configuration-time) use rather than the hot path. +ICEBERG_EXPORT void SetDefaultLogger(std::shared_ptr logger); + +/// \brief Set the minimum level of the current default logger. +/// +/// Convenience for `GetDefaultLogger()->SetLevel(level)`. Filtering is always +/// decided by the logger's own ShouldLog(), so changing a logger's level by any +/// means (this, SetLevel on a held handle, or Initialize) takes effect immediately. +ICEBERG_EXPORT void SetDefaultLevel(LogLevel level); + +/// \brief Bind a logger for the current thread until this object leaves scope. +/// +/// The default logging path on this thread -- CurrentLogger(), Log(level, ...), +/// and the LOG_* macros -- routes to \p logger instead of the global default; +/// explicit Log(logger, ...) is unaffected. Bindings nest and restore on exit, and +/// nullptr masks any enclosing binding back to the global default. Lets an engine +/// route Iceberg's own logs into a per catalog/session/query/task context with no +/// call-site changes. +/// +/// \code +/// auto query_log = std::make_shared(); +/// iceberg::ScopedLogger bind(query_log); // this thread, this scope +/// iceberg::Log(LogLevel::kInfo, "scan {}", id); // -> query_log +/// \endcode +/// +/// Stack-only and same-thread (non-copyable, non-movable). For thread pools, +/// capture on the submitting thread and re-bind on the worker: +/// \code +/// auto captured = iceberg::GetCurrentLogger(); +/// pool.submit([captured, work] { iceberg::ScopedLogger bind(captured); work(); }); +/// \endcode +class ICEBERG_EXPORT ScopedLogger { + public: + explicit ScopedLogger(std::shared_ptr logger) noexcept; + ~ScopedLogger(); + + ScopedLogger(const ScopedLogger&) = delete; + ScopedLogger& operator=(const ScopedLogger&) = delete; + ScopedLogger(ScopedLogger&&) = delete; + ScopedLogger& operator=(ScopedLogger&&) = delete; + + private: + std::shared_ptr previous_; +}; + +// --------------------------------------------------------------------------- +// Using the API directly (the LOG_* macros that wrap this are added later in +// the stack). Example: a custom sink, installed as the process default. +// +// class MySink : public Logger { +// public: +// bool ShouldLog(LogLevel level) const noexcept override { return level >= level_; } +// void Log(LogMessage&& m) noexcept override { write_line(m.message); } +// void SetLevel(LogLevel level) noexcept override { level_ = level; } +// LogLevel level() const noexcept override { return level_; } +// private: +// std::atomic level_{LogLevel::kInfo}; +// }; +// +// SetDefaultLogger(std::make_shared()); // install process-wide +// SetDefaultLevel(LogLevel::kDebug); // adjust the threshold +// +// auto logger = GetDefaultLogger(); // borrow the current default +// if (logger->ShouldLog(LogLevel::kInfo)) { +// logger->Log(LogMessage{.level = LogLevel::kInfo, .message = "scan ready"}); +// } +// +// // Or configure from catalog-style properties (applies the "level" key): +// auto sink = std::make_shared(); +// auto status = sink->Initialize({{std::string(kLevelProperty), "warn"}}); // -> kWarn +// --------------------------------------------------------------------------- + +namespace internal { + +/// \brief Hot-path accessor for the default logger. +/// +/// Returns a reference to a thread-local cached shared_ptr that is refreshed +/// only when the default logger has changed (no lock / no refcount churn in +/// steady state). The reference is valid for the duration of the calling +/// statement. +ICEBERG_EXPORT const std::shared_ptr& CurrentLogger() noexcept; + +/// \brief Build a LogMessage from the already-formatted text and dispatch it. +/// +/// Declared ICEBERG_EXPORT because the logging macros expand into this call in +/// consumer translation units. +ICEBERG_EXPORT void Emit(Logger& logger, LogLevel level, + const std::source_location& location, std::string&& message); + +/// \brief Emit a fixed fallback record when formatting threw. +/// +/// noexcept, allocation-light (small/SSO literal), performs no std::format, and +/// does not recurse -- so the macro's "logging never throws" guarantee holds +/// even when a format argument throws. +ICEBERG_EXPORT void EmitFormatError(Logger& logger, LogLevel level, + const std::source_location& location) noexcept; + +/// \brief Runtime (non-literal) format-string helper. +/// +/// std::format requires a compile-time format string; this routes a runtime +/// string through std::vformat. Args are bound as named lvalues and the +/// arg-store is held in a named variable so it outlives the vformat call +/// (C++23 make_format_args rejects rvalues -- P2905 / LWG3631). +template +std::string VFormat(std::string_view fmt, Args&&... args) { + auto store = std::make_format_args(args...); + return std::vformat(fmt, store); +} + +/// \brief A checked format string bundled with the caller's source_location. +/// +/// The consteval constructor preserves std::format's compile-time format-string +/// checking while capturing the call site (the std::print/println technique), +/// so the function-style Log() can record an accurate file:line without a macro. +/// Used as a non-deduced parameter so the trailing args drive deduction. +template +struct FmtWithLoc { + std::format_string fmt; + std::source_location loc; + + template + requires std::convertible_to> + consteval FmtWithLoc( // NOLINT(google-explicit-constructor): mirrors + // std::format_string + const T& s, std::source_location loc = std::source_location::current()) + : fmt(s), loc(loc) {} +}; + +/// \brief Shared gate -> format -> emit body for the function-style Log() API. +/// +/// Formats only when the logger is enabled for \p level, and never throws (a +/// formatting failure routes to EmitFormatError, matching the macros). +template +void FormatAndEmit(Logger& logger, LogLevel level, const std::source_location& loc, + std::format_string fmt, Args&&... args) noexcept { + if (!logger.ShouldLog(level)) return; + try { + Emit(logger, level, loc, std::format(fmt, std::forward(args)...)); + } catch (...) { + EmitFormatError(logger, level, loc); + } +} + +} // namespace internal + +/// \brief Log to the process-default logger, std::format style. Formats only if +/// the level is enabled; never throws. +/// +/// Example: `iceberg::Log(LogLevel::kInfo, "loaded {} files", n);` +template +void Log(LogLevel level, internal::FmtWithLoc...> fmt, + Args&&... args) noexcept { + const std::shared_ptr& logger = internal::CurrentLogger(); + if (logger) { + internal::FormatAndEmit(*logger, level, fmt.loc, fmt.fmt, + std::forward(args)...); + } +} + +/// \brief Log to an explicit logger, std::format style. Formats only if enabled. +/// +/// Example: `iceberg::Log(logger, LogLevel::kWarn, "retry {}", attempt);` +template +void Log(Logger& logger, LogLevel level, + internal::FmtWithLoc...> fmt, + Args&&... args) noexcept { + internal::FormatAndEmit(logger, level, fmt.loc, fmt.fmt, std::forward(args)...); +} + +} // namespace iceberg diff --git a/src/iceberg/logging/meson.build b/src/iceberg/logging/meson.build index 3c286a196..3f7af4fec 100644 --- a/src/iceberg/logging/meson.build +++ b/src/iceberg/logging/meson.build @@ -15,4 +15,4 @@ # specific language governing permissions and limitations # under the License. -install_headers(['log_level.h'], subdir: 'iceberg/logging') +install_headers(['log_level.h', 'logger.h'], subdir: 'iceberg/logging') diff --git a/src/iceberg/meson.build b/src/iceberg/meson.build index 0c467605b..7bd2e052c 100644 --- a/src/iceberg/meson.build +++ b/src/iceberg/meson.build @@ -74,6 +74,7 @@ iceberg_sources = files( 'inspect/snapshots_table.cc', 'json_serde.cc', 'location_provider.cc', + 'logging/logger.cc', 'manifest/manifest_adapter.cc', 'manifest/manifest_entry.cc', 'manifest/manifest_filter_manager.cc', diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index c8c815797..bf00c91ac 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -102,7 +102,7 @@ add_iceberg_test(table_test table_test.cc table_update_test.cc) -add_iceberg_test(logging_test SOURCES log_level_test.cc) +add_iceberg_test(logging_test SOURCES log_level_test.cc logger_test.cc) add_iceberg_test(expression_test SOURCES diff --git a/src/iceberg/test/logger_test.cc b/src/iceberg/test/logger_test.cc new file mode 100644 index 000000000..81d8e7f9b --- /dev/null +++ b/src/iceberg/test/logger_test.cc @@ -0,0 +1,450 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/logging/logger.h" + +#include +#include +#include +#include +#include + +#include + +#include "iceberg/logging/log_level.h" +#include "iceberg/test/logging_test_helpers.h" +#include "iceberg/test/matchers.h" + +namespace iceberg { + +TEST(LoggerTest, NoopIsSharedImmortalAndSilent) { + auto noop = Logger::Noop(); + ASSERT_NE(noop, nullptr); + EXPECT_TRUE(noop->IsNoop()); + EXPECT_FALSE(noop->ShouldLog(LogLevel::kFatal)); + EXPECT_EQ(noop->level(), LogLevel::kOff); + // Same singleton instance every call. + EXPECT_EQ(noop.get(), Logger::Noop().get()); +} + +TEST(LoggerTest, DefaultLoggerIsNeverNull) { EXPECT_NE(GetDefaultLogger(), nullptr); } + +TEST(LoggerTest, SetAndGetDefaultLogger) { + auto capturing = std::make_shared(); + ScopedDefaultLogger guard(capturing); + EXPECT_EQ(GetDefaultLogger().get(), capturing.get()); + EXPECT_EQ(internal::CurrentLogger().get(), capturing.get()); +} + +TEST(LoggerTest, SetNullFallsBackToNoop) { + ScopedDefaultLogger guard(std::make_shared()); + SetDefaultLogger(nullptr); + EXPECT_TRUE(GetDefaultLogger()->IsNoop()); +} + +TEST(LoggerTest, CurrentLoggerTracksSwaps) { + auto first = std::make_shared(); + auto second = std::make_shared(); + ScopedDefaultLogger guard(first); + EXPECT_EQ(internal::CurrentLogger().get(), first.get()); + SetDefaultLogger(second); + // Generation bump must invalidate the thread-local cache. + EXPECT_EQ(internal::CurrentLogger().get(), second.get()); +} + +TEST(LoggerTest, SetDefaultLevelUpdatesLogger) { + auto capturing = std::make_shared(); + ScopedDefaultLogger guard(capturing); + SetDefaultLevel(LogLevel::kError); + EXPECT_EQ(capturing->level(), LogLevel::kError); +} + +// Filtering is decided by the logger's own ShouldLog (no separate cached gate), +// so lowering a logger's level out-of-band (not via SetDefaultLevel) takes effect +// immediately -- this is the regression guard for the dropped g_effective_level gate. +TEST(LoggerTest, OutOfBandLevelLoweringTakesEffect) { + auto capturing = std::make_shared(); + capturing->SetLevel(LogLevel::kError); + ScopedDefaultLogger guard(capturing); + EXPECT_FALSE(internal::CurrentLogger()->ShouldLog(LogLevel::kInfo)); + capturing->SetLevel(LogLevel::kTrace); // lowered directly on the handle + EXPECT_TRUE(internal::CurrentLogger()->ShouldLog(LogLevel::kInfo)); +} + +TEST(LoggerTest, ConcurrentSwapAndReadIsSafe) { + // Stress CurrentLogger()/GetDefaultLogger() against SetDefaultLogger() swaps. + // Run under TSan in CI; here it asserts no crash and a valid logger throughout. + auto a = std::make_shared(); + auto b = std::make_shared(); + ScopedDefaultLogger guard(a); + std::atomic stop{false}; + std::atomic saw_null{false}; + std::vector readers; + for (int i = 0; i < 6; ++i) { + readers.emplace_back([&stop, &saw_null] { + // ASSERT_* doesn't propagate from non-main threads; record via a flag. + while (!stop.load(std::memory_order_relaxed)) { + const auto& l = internal::CurrentLogger(); + if (!l) saw_null.store(true, std::memory_order_relaxed); + std::ignore = l->ShouldLog(LogLevel::kError); + std::ignore = GetDefaultLogger(); + } + }); + } + for (int i = 0; i < 2000; ++i) SetDefaultLogger((i & 1) ? a : b); + stop.store(true, std::memory_order_relaxed); + for (auto& t : readers) t.join(); + EXPECT_FALSE(saw_null.load()); // CurrentLogger() is never null across swaps +} + +TEST(LoggerTest, InitializeAppliesLevelProperty) { + CapturingLogger logger; + auto status = logger.Initialize({{std::string(kLevelProperty), std::string("error")}}); + ASSERT_TRUE(status.has_value()); + EXPECT_EQ(logger.level(), LogLevel::kError); +} + +TEST(LoggerTest, InitializeRejectsInvalidLevel) { + CapturingLogger logger; + auto status = + logger.Initialize({{std::string(kLevelProperty), std::string("not-a-level")}}); + ASSERT_FALSE(status.has_value()); + EXPECT_THAT(status, IsError(ErrorKind::kInvalidArgument)); +} + +// Logging during thread teardown (from a thread_local destructor) must not crash. +// The per-thread cache is freed at thread exit, but CurrentLogger()'s teardown +// guard (a trivially-destructible "dead" flag whose storage outlives every +// thread_local) makes it safe even when the logging statement runs from a +// thread_local destroyed AFTER the cache -- the hard case. Probe is constructed +// before CurrentLogger() is first touched, so it is destroyed last. Run under +// ASan/TSan in CI for full signal. +TEST(LoggerTest, LoggingFromThreadLocalDestructorIsSafe) { + std::thread([] { + struct Probe { + ~Probe() { + const auto& logger = internal::CurrentLogger(); + if (logger) { + internal::Emit(*logger, LogLevel::kInfo, std::source_location::current(), + "from thread_local dtor"); + } + } + }; + static thread_local Probe probe; + std::ignore = probe; // construct Probe first ... + std::ignore = + internal::CurrentLogger(); // ... then the logger cache (destroyed first) + }).join(); + SUCCEED(); +} + +// Teardown interleaved with concurrent default-logger swaps: many short-lived +// threads each log from a thread_local destructor while another thread swaps the +// default logger. Exercises the per-thread cache being freed at thread exit at +// the same time the global slot is mutated. Run under ASan/TSan in CI. +TEST(LoggerTest, ConcurrentTeardownAndSwapIsSafe) { + auto a = std::make_shared(); + ScopedDefaultLogger guard(a); + std::atomic stop{false}; + std::thread swapper([&] { + auto b = std::make_shared(); + while (!stop.load(std::memory_order_relaxed)) { + SetDefaultLogger(b); + SetDefaultLogger(a); + } + }); + for (int i = 0; i < 100; ++i) { + std::thread worker([] { + struct Probe { + ~Probe() { + const auto& l = internal::CurrentLogger(); + if (l) l->ShouldLog(LogLevel::kError); + } + }; + static thread_local Probe probe; + std::ignore = probe; // constructed before the cache + std::ignore = internal::CurrentLogger(); // touch the cache in normal code + }); + worker.join(); // teardown runs concurrently with the swapper + } + stop.store(true, std::memory_order_relaxed); + swapper.join(); + SUCCEED(); +} + +// --- Per-context routing: ScopedLogger + GetCurrentLogger --- + +TEST(LoggerTest, ScopedLoggerOverridesDefaultPath) { + auto global = std::make_shared(); + auto scoped = std::make_shared(); + ScopedDefaultLogger guard(global); + { + ScopedLogger bind(scoped); + EXPECT_EQ(internal::CurrentLogger().get(), scoped.get()); + Log(LogLevel::kInfo, "hi {}", 1); + } + EXPECT_EQ(scoped->count(), 1u); + EXPECT_EQ(global->count(), 0u); +} + +TEST(LoggerTest, ScopedLoggerRestoresOnScopeExit) { + auto global = std::make_shared(); + auto scoped = std::make_shared(); + ScopedDefaultLogger guard(global); + { + ScopedLogger bind(scoped); + } + EXPECT_EQ(internal::CurrentLogger().get(), global.get()); + Log(LogLevel::kInfo, "back"); + EXPECT_EQ(global->count(), 1u); + EXPECT_EQ(scoped->count(), 0u); +} + +TEST(LoggerTest, ExplicitLoggerBypassesOverride) { + auto scoped = std::make_shared(); + auto explicit_sink = std::make_shared(); + ScopedDefaultLogger guard(std::make_shared()); + ScopedLogger bind(scoped); + Log(*explicit_sink, LogLevel::kInfo, "e {}", 1); + EXPECT_EQ(explicit_sink->count(), 1u); + EXPECT_EQ(scoped->count(), 0u); +} + +TEST(LoggerTest, NestedScopedLoggersRestoreInLifo) { + auto global = std::make_shared(); + auto x = std::make_shared(); + auto y = std::make_shared(); + ScopedDefaultLogger guard(global); + { + ScopedLogger a(x); + EXPECT_EQ(internal::CurrentLogger().get(), x.get()); + { + ScopedLogger b(y); + EXPECT_EQ(internal::CurrentLogger().get(), y.get()); + } + EXPECT_EQ(internal::CurrentLogger().get(), x.get()); + } + EXPECT_EQ(internal::CurrentLogger().get(), global.get()); +} + +TEST(LoggerTest, ScopedLoggerNullMasksToGlobalDefault) { + auto global = std::make_shared(); + auto x = std::make_shared(); + ScopedDefaultLogger guard(global); + ScopedLogger a(x); + { + ScopedLogger mask(nullptr); + EXPECT_EQ(internal::CurrentLogger().get(), global.get()); // not x, not Noop + EXPECT_FALSE(internal::CurrentLogger()->IsNoop()); + } + EXPECT_EQ(internal::CurrentLogger().get(), x.get()); // enclosing binding restored +} + +TEST(LoggerTest, GetCurrentLoggerReturnsOverrideThenDefault) { + auto global = std::make_shared(); + auto scoped = std::make_shared(); + ScopedDefaultLogger guard(global); + EXPECT_EQ(GetCurrentLogger().get(), global.get()); + { + ScopedLogger bind(scoped); + EXPECT_EQ(GetCurrentLogger().get(), scoped.get()); + } + EXPECT_EQ(GetCurrentLogger().get(), global.get()); +} + +TEST(LoggerTest, GetCurrentLoggerOnFreshThreadReturnsDefault) { + auto global = std::make_shared(); + ScopedDefaultLogger guard(global); + Logger* seen = nullptr; + std::thread([&] { seen = GetCurrentLogger().get(); }).join(); // never used a scope + EXPECT_EQ(seen, global.get()); +} + +TEST(LoggerTest, ThreadPoolPropagationPattern) { + auto global = std::make_shared(); + auto scoped = std::make_shared(); + ScopedDefaultLogger guard(global); + ScopedLogger bind(scoped); + auto captured = GetCurrentLogger(); // capture the effective logger at "submit" + EXPECT_EQ(captured.get(), scoped.get()); + std::thread([captured] { + ScopedLogger rebind(captured); // re-bind on the worker thread + Log(LogLevel::kInfo, "task {}", 7); + }).join(); + EXPECT_EQ(scoped->count(), 1u); + EXPECT_EQ(global->count(), 0u); +} + +TEST(LoggerTest, WorkerOverrideDoesNotLeakAcrossTasksOnReusedThread) { + auto global = std::make_shared(); + auto o1 = std::make_shared(); + ScopedDefaultLogger guard(global); + Logger* during = nullptr; + Logger* between = nullptr; + std::thread([&] { + { + ScopedLogger b1(o1); + during = GetCurrentLogger().get(); + } + between = GetCurrentLogger().get(); // no scope active -> global default + }).join(); + EXPECT_EQ(during, o1.get()); + EXPECT_EQ(between, global.get()); +} + +// Binding/unbinding a ScopedLogger from a thread_local destructor that runs after +// the per-thread cache was freed (Probe constructed before CurrentLogger is first +// touched) must no-op against the dead cache, never touch freed memory. Run under +// ASan/TSan in CI for full signal. +TEST(LoggerTest, ScopedLoggerBindUnbindDuringTeardownIsSafe) { + std::thread([] { + struct Probe { + ~Probe() { + ScopedLogger late(std::make_shared()); // ctor: no-op on dead + const auto& l = internal::CurrentLogger(); // -> Noop fallback + if (l) l->ShouldLog(LogLevel::kError); + } // ~ScopedLogger: restore is a no-op on dead + }; + static thread_local Probe probe; + std::ignore = probe; // constructed first + std::ignore = internal::CurrentLogger(); // cache created after -> freed first + }).join(); + SUCCEED(); +} + +// The override path deliberately skips the generation refresh, but a swap that +// happened while an override was active must still be observed on the first call +// after the override is popped (gen is monotonic). +TEST(LoggerTest, OverrideActiveSkipsGenRefreshButSwapStillSeenAfterPop) { + auto a = std::make_shared(); + auto b = std::make_shared(); + auto c = std::make_shared(); + ScopedDefaultLogger guard(a); + EXPECT_EQ(internal::CurrentLogger().get(), a.get()); + { + ScopedLogger bind(b); + SetDefaultLogger(c); // bump gen while the override is active + EXPECT_EQ(internal::CurrentLogger().get(), b.get()); // override wins + } + EXPECT_EQ(internal::CurrentLogger().get(), c.get()); // swap seen after pop +} + +// Many short-lived threads each bind a ScopedLogger and tear down while another +// thread swaps the global default. Run under ASan/TSan in CI. +TEST(LoggerTest, ConcurrentOverrideTeardownAndSwapIsSafe) { + auto a = std::make_shared(); + ScopedDefaultLogger guard(a); + std::atomic stop{false}; + std::thread swapper([&] { + auto b = std::make_shared(); + while (!stop.load(std::memory_order_relaxed)) { + SetDefaultLogger(b); + SetDefaultLogger(a); + } + }); + for (int i = 0; i < 100; ++i) { + std::thread worker([] { + auto local = std::make_shared(); + ScopedLogger bind(local); + const auto& l = internal::CurrentLogger(); + if (l) l->ShouldLog(LogLevel::kError); + }); + worker.join(); + } + stop.store(true, std::memory_order_relaxed); + swapper.join(); + SUCCEED(); +} + +// --- Function-style API (non-macro): overloaded Log() --- + +TEST(LoggerTest, LogToExplicitLoggerFormats) { + auto sink = std::make_shared(); + Log(*sink, LogLevel::kInfo, "x={} y={}", 1, 2); + auto records = sink->records(); + ASSERT_EQ(records.size(), 1u); + EXPECT_EQ(records[0].level, LogLevel::kInfo); + EXPECT_EQ(records[0].message, "x=1 y=2"); + EXPECT_NE(records[0].location.line(), 0u); // call-site location captured +} + +TEST(LoggerTest, LogRespectsLevelAndDoesNotFormatWhenDisabled) { + auto sink = std::make_shared(); + sink->SetLevel(LogLevel::kError); + Log(*sink, LogLevel::kInfo, "dropped {}", 1); + EXPECT_EQ(sink->count(), 0u); +} + +TEST(LoggerTest, LogToDefaultLoggerFormatStyle) { + auto sink = std::make_shared(); + ScopedDefaultLogger guard(sink); + Log(LogLevel::kWarn, "v={}", 7); + auto records = sink->records(); + ASSERT_EQ(records.size(), 1u); + EXPECT_EQ(records[0].level, LogLevel::kWarn); + EXPECT_EQ(records[0].message, "v=7"); +} + +// --- LogMessage::Builder (structured attributes) --- + +TEST(LoggerTest, BuilderAssemblesMessageAndAttributes) { + auto record = LogMessage::Builder(LogLevel::kInfo) + .Message("scan finished") + .Attribute("table", "db.t") + .Attribute("snapshot_id", "42") + .Build(); + EXPECT_EQ(record.level, LogLevel::kInfo); + EXPECT_EQ(record.message, "scan finished"); + ASSERT_EQ(record.attributes.size(), 2u); + EXPECT_EQ(record.attributes[0].key, "table"); + EXPECT_EQ(record.attributes[0].value, "db.t"); + EXPECT_EQ(record.attributes[1].key, "snapshot_id"); + EXPECT_EQ(record.attributes[1].value, "42"); +} + +TEST(LoggerTest, BuilderDefaultsAndEmitToSink) { + auto sink = std::make_shared(); + sink->Log(LogMessage::Builder(LogLevel::kError).Message("boom").Build()); + auto records = sink->records(); + ASSERT_EQ(records.size(), 1u); + EXPECT_EQ(records[0].level, LogLevel::kError); + EXPECT_EQ(records[0].message, "boom"); + EXPECT_TRUE(records[0].attributes.empty()); + EXPECT_NE(records[0].location.line(), 0u); // location defaulted at build site +} + +// The constructor captures source_location as a default argument, so without +// Location() the default is the caller's construction site (this file), not +// logger.h. The Builder is constructed exactly one line below `here`. +TEST(LoggerTest, BuilderDefaultLocationIsCallerSite) { + auto here = std::source_location::current(); + auto record = LogMessage::Builder(LogLevel::kInfo).Message("m").Build(); + EXPECT_STREQ(record.location.file_name(), here.file_name()); + EXPECT_EQ(record.location.line(), here.line() + 1); +} + +// Location() replaces the constructor default with the caller's site (file + line). +TEST(LoggerTest, BuilderLocationOverrideUsesCallerSite) { + auto caller = std::source_location::current(); + auto record = LogMessage::Builder(LogLevel::kDebug).Location(caller).Build(); + EXPECT_EQ(record.location.line(), caller.line()); + EXPECT_STREQ(record.location.file_name(), caller.file_name()); +} + +} // namespace iceberg diff --git a/src/iceberg/test/logging_test_helpers.h b/src/iceberg/test/logging_test_helpers.h new file mode 100644 index 000000000..f3999195e --- /dev/null +++ b/src/iceberg/test/logging_test_helpers.h @@ -0,0 +1,84 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include +#include +#include +#include +#include + +#include "iceberg/logging/logger.h" + +namespace iceberg { + +/// \brief Test sink that records every emitted LogMessage under a mutex. +class CapturingLogger : public Logger { + public: + bool ShouldLog(LogLevel level) const noexcept override { + return level >= level_.load(std::memory_order_relaxed); + } + + void Log(LogMessage&& message) noexcept override { + std::lock_guard lock(mutex_); + records_.push_back(std::move(message)); + } + + void SetLevel(LogLevel level) noexcept override { + level_.store(level, std::memory_order_relaxed); + } + LogLevel level() const noexcept override { + return level_.load(std::memory_order_relaxed); + } + + std::vector records() const { + std::lock_guard lock(mutex_); + return records_; + } + + std::size_t count() const { + std::lock_guard lock(mutex_); + return records_.size(); + } + + private: + mutable std::mutex mutex_; + std::atomic level_ = LogLevel::kTrace; + std::vector records_; +}; + +/// \brief RAII guard that restores the process default logger on scope exit, so +/// tests that swap the global default don't leak state into other tests. +class ScopedDefaultLogger { + public: + explicit ScopedDefaultLogger(std::shared_ptr logger) + : previous_(GetDefaultLogger()) { + SetDefaultLogger(std::move(logger)); + } + ~ScopedDefaultLogger() { SetDefaultLogger(previous_); } + + ScopedDefaultLogger(const ScopedDefaultLogger&) = delete; + ScopedDefaultLogger& operator=(const ScopedDefaultLogger&) = delete; + + private: + std::shared_ptr previous_; +}; + +} // namespace iceberg diff --git a/src/iceberg/test/meson.build b/src/iceberg/test/meson.build index a17d9841a..6f9c4c31b 100644 --- a/src/iceberg/test/meson.build +++ b/src/iceberg/test/meson.build @@ -61,7 +61,7 @@ iceberg_tests = { 'table_update_test.cc', ), }, - 'logging_test': {'sources': files('log_level_test.cc')}, + 'logging_test': {'sources': files('log_level_test.cc', 'logger_test.cc')}, 'expression_test': { 'sources': files( 'aggregate_test.cc', From 0aff5e99c7f696d476e92face67d1dba34369e84 Mon Sep 17 00:00:00 2001 From: Abanoub Doss Date: Mon, 22 Jun 2026 04:28:29 -0500 Subject: [PATCH 05/16] ci: cache vcpkg packages on Windows builds (#766) ## What Cache the installed vcpkg packages on the Windows builds in `test` and `sql_catalog_test`, and skip the install step when the cache is present. ## Why Every Windows run reinstalls the same packages (zlib, nlohmann-json, nanoarrow, roaring, plus cpr / sqlite3) from scratch, costing a couple of minutes each time. Caching them removes that on every run after the first. This matches what `aws_test` already does. ## Validation A warm run reused the cached packages and skipped the install. The cache only rebuilds when the package list changes. Co-authored-by: Abanoub Doss --- .github/workflows/sql_catalog_test.yml | 9 ++++++++- .github/workflows/test.yml | 7 +++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/.github/workflows/sql_catalog_test.yml b/.github/workflows/sql_catalog_test.yml index cc95169af..79c6328b5 100644 --- a/.github/workflows/sql_catalog_test.yml +++ b/.github/workflows/sql_catalog_test.yml @@ -80,8 +80,15 @@ jobs: run: | echo "CC=${{ matrix.CC }}" >> $GITHUB_ENV echo "CXX=${{ matrix.CXX }}" >> $GITHUB_ENV - - name: Install dependencies on Windows + - name: Cache vcpkg packages if: ${{ startsWith(matrix.runs-on, 'windows') }} + uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + id: vcpkg-cache + with: + path: C:/vcpkg/installed + key: vcpkg-x64-windows-sql-catalog-${{ hashFiles('.github/workflows/sql_catalog_test.yml') }} + - name: Install dependencies on Windows + if: ${{ startsWith(matrix.runs-on, 'windows') && steps.vcpkg-cache.outputs.cache-hit != 'true' }} shell: pwsh run: | vcpkg install zlib:x64-windows nlohmann-json:x64-windows nanoarrow:x64-windows roaring:x64-windows sqlite3:x64-windows diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 652999f61..6f420bde5 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -99,7 +99,14 @@ jobs: uses: ilammy/msvc-dev-cmd@0b201ec74fa43914dc39ae48a89fd1d8cb592756 # v1.13.0 with: arch: x64 + - name: Cache vcpkg packages + uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + id: vcpkg-cache + with: + path: C:/vcpkg/installed + key: vcpkg-x64-windows-test-${{ hashFiles('.github/workflows/test.yml') }} - name: Install dependencies + if: ${{ steps.vcpkg-cache.outputs.cache-hit != 'true' }} shell: pwsh run: | vcpkg install zlib:x64-windows nlohmann-json:x64-windows nanoarrow:x64-windows roaring:x64-windows cpr:x64-windows From 42b498f60c02314fbca02d3c97ae4f5c787b3fc5 Mon Sep 17 00:00:00 2001 From: Abanoub Doss Date: Mon, 22 Jun 2026 04:38:21 -0500 Subject: [PATCH 06/16] ci: cancel superseded runs in remaining workflows (#767) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What Add a concurrency block to the five workflows that don't have one — `cpp-linter`, `pre-commit`, `license_check`, `zizmor`, and `codeql`: ```yaml concurrency: group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} cancel-in-progress: true ``` ## Why The heavier workflows already cancel outdated runs, but these five don't - so pushing again to a PR leaves the old runs going and tying up runners. Grouping on `head_ref || sha` cancels superseded PR runs while leaving `main` and scheduled runs untouched. Co-authored-by: Abanoub Doss --- .github/workflows/codeql.yml | 4 ++++ .github/workflows/cpp-linter.yml | 4 ++++ .github/workflows/license_check.yml | 4 ++++ .github/workflows/pre-commit.yml | 4 ++++ .github/workflows/zizmor.yml | 4 ++++ 5 files changed, 20 insertions(+) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 98090e34d..ae771aea4 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -28,6 +28,10 @@ on: schedule: - cron: '16 4 * * 1' +concurrency: + group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} + cancel-in-progress: true + permissions: contents: read diff --git a/.github/workflows/cpp-linter.yml b/.github/workflows/cpp-linter.yml index 834ade883..377e806ad 100644 --- a/.github/workflows/cpp-linter.yml +++ b/.github/workflows/cpp-linter.yml @@ -27,6 +27,10 @@ on: branches: - main +concurrency: + group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} + cancel-in-progress: true + jobs: cpp-linter: if: ${{ github.event_name != 'pull_request' || github.event.pull_request.draft == false }} diff --git a/.github/workflows/license_check.yml b/.github/workflows/license_check.yml index 361314812..ea037f1a3 100644 --- a/.github/workflows/license_check.yml +++ b/.github/workflows/license_check.yml @@ -21,6 +21,10 @@ on: pull_request: types: [opened, synchronize, reopened, ready_for_review] +concurrency: + group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} + cancel-in-progress: true + permissions: contents: read diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml index d8a06a108..413eee866 100644 --- a/.github/workflows/pre-commit.yml +++ b/.github/workflows/pre-commit.yml @@ -25,6 +25,10 @@ on: - '**' - '!dependabot/**' +concurrency: + group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} + cancel-in-progress: true + permissions: contents: write diff --git a/.github/workflows/zizmor.yml b/.github/workflows/zizmor.yml index 5265d6f04..f6665ffae 100644 --- a/.github/workflows/zizmor.yml +++ b/.github/workflows/zizmor.yml @@ -26,6 +26,10 @@ on: types: [opened, synchronize, reopened, ready_for_review] branches: ["**"] +concurrency: + group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} + cancel-in-progress: true + permissions: {} jobs: From 7fe2e93ed1d34a583098ce29e03b4a7851f213cc Mon Sep 17 00:00:00 2001 From: Yuya Ebihara Date: Tue, 23 Jun 2026 07:08:16 +0900 Subject: [PATCH 07/16] CI: Check ASF action allowlist on every PR (#772) Relates to: * https://github.com/apache/iceberg/issues/16934 * https://github.com/apache/iceberg/pull/16926 --- .github/workflows/asf-allowlist-check.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.github/workflows/asf-allowlist-check.yml b/.github/workflows/asf-allowlist-check.yml index 85ee0f463..202852115 100644 --- a/.github/workflows/asf-allowlist-check.yml +++ b/.github/workflows/asf-allowlist-check.yml @@ -26,13 +26,9 @@ name: "ASF Allowlist Check" on: pull_request: types: [opened, synchronize, reopened, ready_for_review] - paths: - - ".github/**" push: branches: - main - paths: - - ".github/**" permissions: contents: read From 5c8c4e5d024ab72c1486a8d5ad32d67431dff894 Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Tue, 23 Jun 2026 11:59:08 +0800 Subject: [PATCH 08/16] fix(rest): align error handlers with Java implementation (#763) While reviewing #614, I noticed that `PlanErrorHandler::Accept` does not match the behavior of the Java implementation. A detailed comparison with Java's `ErrorHandlers` revealed several gaps between iceberg-cpp and Iceberg Java. Some of these are oversights in the original implementation, while others correspond to improvements that were made later in Iceberg Java, including: * https://github.com/apache/iceberg/pull/13143 * https://github.com/apache/iceberg/pull/14927 * https://github.com/apache/iceberg/pull/15051 * https://github.com/apache/iceberg/pull/16059 This PR closes those gaps and brings the error handling behavior in iceberg-cpp closer to the Java implementation. --- src/iceberg/catalog/rest/auth/oauth2_util.cc | 2 +- src/iceberg/catalog/rest/error_handlers.cc | 157 ++++++++-- src/iceberg/catalog/rest/error_handlers.h | 67 ++++- src/iceberg/catalog/rest/http_client.cc | 16 +- src/iceberg/catalog/rest/rest_catalog.cc | 14 +- src/iceberg/result.h | 2 + src/iceberg/test/CMakeLists.txt | 1 + src/iceberg/test/error_handlers_test.cc | 285 +++++++++++++++++++ src/iceberg/test/meson.build | 1 + 9 files changed, 488 insertions(+), 57 deletions(-) create mode 100644 src/iceberg/test/error_handlers_test.cc diff --git a/src/iceberg/catalog/rest/auth/oauth2_util.cc b/src/iceberg/catalog/rest/auth/oauth2_util.cc index 692ef47f3..d5e94821c 100644 --- a/src/iceberg/catalog/rest/auth/oauth2_util.cc +++ b/src/iceberg/catalog/rest/auth/oauth2_util.cc @@ -66,7 +66,7 @@ Result FetchToken(HttpClient& client, AuthSession& session, ICEBERG_ASSIGN_OR_RAISE( auto response, client.PostForm(properties.oauth2_server_uri(), form_data, - /*headers=*/{}, *DefaultErrorHandler::Instance(), session)); + /*headers=*/{}, *OAuthErrorHandler::Instance(), session)); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); ICEBERG_ASSIGN_OR_RAISE(auto token_response, FromJson(json)); diff --git a/src/iceberg/catalog/rest/error_handlers.cc b/src/iceberg/catalog/rest/error_handlers.cc index 67146e745..78696a49b 100644 --- a/src/iceberg/catalog/rest/error_handlers.cc +++ b/src/iceberg/catalog/rest/error_handlers.cc @@ -21,7 +21,11 @@ #include +#include "iceberg/catalog/rest/json_serde_internal.h" #include "iceberg/catalog/rest/types.h" +#include "iceberg/json_serde_internal.h" +#include "iceberg/util/json_util_internal.h" +#include "iceberg/util/macros.h" namespace iceberg::rest { @@ -31,8 +35,29 @@ constexpr std::string_view kIllegalArgumentException = "IllegalArgumentException constexpr std::string_view kNoSuchNamespaceException = "NoSuchNamespaceException"; constexpr std::string_view kNamespaceNotEmptyException = "NamespaceNotEmptyException"; constexpr std::string_view kNoSuchTableException = "NoSuchTableException"; -constexpr std::string_view kNoSuchPlanIdException = "NoSuchPlanIdException"; -constexpr std::string_view kNoSuchPlanTaskException = "NoSuchPlanTaskException"; +constexpr std::string_view kNotFoundException = "NotFoundException"; +constexpr std::string_view kRestException = "RESTException"; +constexpr std::string_view kInvalidClient = "invalid_client"; +constexpr std::string_view kInvalidRequest = "invalid_request"; +constexpr std::string_view kInvalidGrant = "invalid_grant"; +constexpr std::string_view kUnauthorizedClient = "unauthorized_client"; +constexpr std::string_view kUnsupportedGrantType = "unsupported_grant_type"; +constexpr std::string_view kInvalidScope = "invalid_scope"; +constexpr std::string_view kNull = "null"; +constexpr std::string_view kOAuthError = "error"; +constexpr std::string_view kOAuthErrorDescription = "error_description"; + +std::string_view NullIfEmpty(const std::string& value) { + if (value.empty()) { + return kNull; + } + return value; +} + +Status CreateRestError(const ErrorResponse& error) { + return RestError("Unable to process (code: {}, type: {}): {}", error.code, + NullIfEmpty(error.type), NullIfEmpty(error.message)); +} } // namespace @@ -63,7 +88,17 @@ Status DefaultErrorHandler::Accept(const ErrorResponse& error) const { return ServiceUnavailable("Service unavailable: {}", error.message); } - return RestError("Code: {}, message: {}", error.code, error.message); + return CreateRestError(error); +} + +Result DefaultErrorHandler::ParseResponse(uint32_t /*code*/, + const std::string& text) const { + if (text.empty()) { + return InvalidArgument("Empty response body"); + } + ICEBERG_ASSIGN_OR_RAISE(auto json_result, FromJsonString(text)); + ICEBERG_ASSIGN_OR_RAISE(auto error_result, ErrorResponseFromJson(json_result)); + return error_result; } const std::shared_ptr& NamespaceErrorHandler::Instance() { @@ -84,7 +119,7 @@ Status NamespaceErrorHandler::Accept(const ErrorResponse& error) const { case 409: return AlreadyExists(error.message); case 422: - return RestError("Unable to process: {}", error.message); + return CreateRestError(error); } return DefaultErrorHandler::Accept(error); @@ -104,37 +139,34 @@ Status DropNamespaceErrorHandler::Accept(const ErrorResponse& error) const { return NamespaceErrorHandler::Accept(error); } -const std::shared_ptr& TableErrorHandler::Instance() { - static const std::shared_ptr instance{new TableErrorHandler()}; +const std::shared_ptr& ConfigErrorHandler::Instance() { + static const std::shared_ptr instance{new ConfigErrorHandler()}; return instance; } -Status TableErrorHandler::Accept(const ErrorResponse& error) const { - switch (error.code) { - case 404: - if (error.type == kNoSuchNamespaceException) { - return NoSuchNamespace(error.message); - } - return NoSuchTable(error.message); - case 409: - return AlreadyExists(error.message); +Status ConfigErrorHandler::Accept(const ErrorResponse& error) const { + if (error.code == 404 && !error.type.empty() && error.type != kRestException) { + return NoSuchWarehouse(error.message); } return DefaultErrorHandler::Accept(error); } -const std::shared_ptr& ViewErrorHandler::Instance() { - static const std::shared_ptr instance{new ViewErrorHandler()}; +const std::shared_ptr& TableErrorHandler::Instance() { + static const std::shared_ptr instance{new TableErrorHandler()}; return instance; } -Status ViewErrorHandler::Accept(const ErrorResponse& error) const { +Status TableErrorHandler::Accept(const ErrorResponse& error) const { switch (error.code) { case 404: if (error.type == kNoSuchNamespaceException) { return NoSuchNamespace(error.message); } - return NoSuchView(error.message); + if (error.type == kNotFoundException) { + return NotFound(error.message); + } + return NoSuchTable(error.message); case 409: return AlreadyExists(error.message); } @@ -164,6 +196,23 @@ Status TableCommitErrorHandler::Accept(const ErrorResponse& error) const { return DefaultErrorHandler::Accept(error); } +const std::shared_ptr& CreateTableErrorHandler::Instance() { + static const std::shared_ptr instance{ + new CreateTableErrorHandler()}; + return instance; +} + +Status CreateTableErrorHandler::Accept(const ErrorResponse& error) const { + switch (error.code) { + case 404: + return NoSuchNamespace(error.message); + case 409: + return AlreadyExists(error.message); + } + + return TableCommitErrorHandler::Accept(error); +} + const std::shared_ptr& ViewCommitErrorHandler::Instance() { static const std::shared_ptr instance{ new ViewCommitErrorHandler()}; @@ -186,6 +235,25 @@ Status ViewCommitErrorHandler::Accept(const ErrorResponse& error) const { return DefaultErrorHandler::Accept(error); } +const std::shared_ptr& ViewErrorHandler::Instance() { + static const std::shared_ptr instance{new ViewErrorHandler()}; + return instance; +} + +Status ViewErrorHandler::Accept(const ErrorResponse& error) const { + switch (error.code) { + case 404: + if (error.type == kNoSuchNamespaceException) { + return NoSuchNamespace(error.message); + } + return NoSuchView(error.message); + case 409: + return AlreadyExists(error.message); + } + + return DefaultErrorHandler::Accept(error); +} + const std::shared_ptr& PlanErrorHandler::Instance() { static const std::shared_ptr instance{new PlanErrorHandler()}; return instance; @@ -200,12 +268,7 @@ Status PlanErrorHandler::Accept(const ErrorResponse& error) const { if (error.type == kNoSuchTableException) { return NoSuchTable(error.message); } - if (error.type == kNoSuchPlanIdException) { - return NoSuchPlanId(error.message); - } - return NotFound(error.message); - case 406: - return NotSupported(error.message); + return NoSuchPlanId(error.message); } return DefaultErrorHandler::Accept(error); @@ -225,13 +288,49 @@ Status PlanTaskErrorHandler::Accept(const ErrorResponse& error) const { if (error.type == kNoSuchTableException) { return NoSuchTable(error.message); } - if (error.type == kNoSuchPlanTaskException) { - return NoSuchPlanTask(error.message); - } - return NotFound(error.message); + return NoSuchPlanTask(error.message); } return DefaultErrorHandler::Accept(error); } +const std::shared_ptr& OAuthErrorHandler::Instance() { + static const std::shared_ptr instance{new OAuthErrorHandler()}; + return instance; +} + +Status OAuthErrorHandler::Accept(const ErrorResponse& error) const { + if (!error.type.empty()) { + if (error.type == kInvalidClient) { + return NotAuthorized("Not authorized: {}: {}", error.type, + NullIfEmpty(error.message)); + } + if (error.type == kInvalidRequest || error.type == kInvalidGrant || + error.type == kUnauthorizedClient || error.type == kUnsupportedGrantType || + error.type == kInvalidScope) { + return BadRequest("Malformed request: {}: {}", error.type, + NullIfEmpty(error.message)); + } + } + + return CreateRestError(error); +} + +Result OAuthErrorHandler::ParseResponse(uint32_t code, + const std::string& text) const { + if (text.empty()) { + return InvalidArgument("Empty response body"); + } + + ICEBERG_ASSIGN_OR_RAISE(auto json_result, FromJsonString(text)); + + ErrorResponse error; + error.code = code; + ICEBERG_ASSIGN_OR_RAISE(error.type, + GetJsonValue(json_result, kOAuthError)); + ICEBERG_ASSIGN_OR_RAISE(error.message, GetJsonValueOrDefault( + json_result, kOAuthErrorDescription)); + return error; +} + } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/error_handlers.h b/src/iceberg/catalog/rest/error_handlers.h index ee338fb3e..e77a0c71d 100644 --- a/src/iceberg/catalog/rest/error_handlers.h +++ b/src/iceberg/catalog/rest/error_handlers.h @@ -19,10 +19,12 @@ #pragma once +#include #include +#include #include "iceberg/catalog/rest/iceberg_rest_export.h" -#include "iceberg/catalog/rest/type_fwd.h" +#include "iceberg/catalog/rest/types.h" #include "iceberg/result.h" /// \file iceberg/catalog/rest/error_handlers.h @@ -41,6 +43,14 @@ class ICEBERG_REST_EXPORT ErrorHandler { /// \param error The error response parsed from the HTTP response body /// \return An Error object with appropriate ErrorKind and message virtual Status Accept(const ErrorResponse& error) const = 0; + + /// \brief Parse an HTTP error response body. + /// + /// \param code The HTTP status code from the failed response + /// \param text The HTTP response body + /// \return The parsed error response + virtual Result ParseResponse(uint32_t code, + const std::string& text) const = 0; }; /// \brief Default error handler for REST API responses. @@ -50,6 +60,8 @@ class ICEBERG_REST_EXPORT DefaultErrorHandler : public ErrorHandler { static const std::shared_ptr& Instance(); Status Accept(const ErrorResponse& error) const override; + Result ParseResponse(uint32_t code, + const std::string& text) const override; protected: constexpr DefaultErrorHandler() = default; @@ -79,6 +91,18 @@ class ICEBERG_REST_EXPORT DropNamespaceErrorHandler final : public NamespaceErro constexpr DropNamespaceErrorHandler() = default; }; +/// \brief Error handler for the catalog config endpoint. +class ICEBERG_REST_EXPORT ConfigErrorHandler final : public DefaultErrorHandler { + public: + /// \brief Returns the singleton instance + static const std::shared_ptr& Instance(); + + Status Accept(const ErrorResponse& error) const override; + + private: + constexpr ConfigErrorHandler() = default; +}; + /// \brief Table-level error handler. class ICEBERG_REST_EXPORT TableErrorHandler final : public DefaultErrorHandler { public: @@ -91,28 +115,40 @@ class ICEBERG_REST_EXPORT TableErrorHandler final : public DefaultErrorHandler { constexpr TableErrorHandler() = default; }; -/// \brief View-level error handler. -class ICEBERG_REST_EXPORT ViewErrorHandler final : public DefaultErrorHandler { +/// \brief Table commit operation error handler. +class ICEBERG_REST_EXPORT TableCommitErrorHandler : public DefaultErrorHandler { public: /// \brief Returns the singleton instance - static const std::shared_ptr& Instance(); + static const std::shared_ptr& Instance(); + + Status Accept(const ErrorResponse& error) const override; + + protected: + constexpr TableCommitErrorHandler() = default; +}; + +/// \brief Table create commit operation error handler. +class ICEBERG_REST_EXPORT CreateTableErrorHandler final : public TableCommitErrorHandler { + public: + /// \brief Returns the singleton instance + static const std::shared_ptr& Instance(); Status Accept(const ErrorResponse& error) const override; private: - constexpr ViewErrorHandler() = default; + constexpr CreateTableErrorHandler() = default; }; -/// \brief Table commit operation error handler. -class ICEBERG_REST_EXPORT TableCommitErrorHandler final : public DefaultErrorHandler { +/// \brief View-level error handler. +class ICEBERG_REST_EXPORT ViewErrorHandler final : public DefaultErrorHandler { public: /// \brief Returns the singleton instance - static const std::shared_ptr& Instance(); + static const std::shared_ptr& Instance(); Status Accept(const ErrorResponse& error) const override; private: - constexpr TableCommitErrorHandler() = default; + constexpr ViewErrorHandler() = default; }; /// \brief View commit operation error handler. @@ -149,4 +185,17 @@ class ICEBERG_REST_EXPORT PlanTaskErrorHandler final : public DefaultErrorHandle constexpr PlanTaskErrorHandler() = default; }; +/// \brief OAuth token endpoint error handler. +class ICEBERG_REST_EXPORT OAuthErrorHandler final : public ErrorHandler { + public: + static const std::shared_ptr& Instance(); + + Status Accept(const ErrorResponse& error) const override; + Result ParseResponse(uint32_t code, + const std::string& text) const override; + + private: + constexpr OAuthErrorHandler() = default; +}; + } // namespace iceberg::rest diff --git a/src/iceberg/catalog/rest/http_client.cc b/src/iceberg/catalog/rest/http_client.cc index 609116eb8..6661c5098 100644 --- a/src/iceberg/catalog/rest/http_client.cc +++ b/src/iceberg/catalog/rest/http_client.cc @@ -22,14 +22,11 @@ #include #include -#include #include "iceberg/catalog/rest/auth/auth_session.h" #include "iceberg/catalog/rest/constant.h" #include "iceberg/catalog/rest/error_handlers.h" -#include "iceberg/catalog/rest/json_serde_internal.h" #include "iceberg/catalog/rest/rest_util.h" -#include "iceberg/json_serde_internal.h" #include "iceberg/result.h" #include "iceberg/util/macros.h" @@ -141,23 +138,14 @@ ErrorResponse BuildDefaultErrorResponse(const cpr::Response& response) { }; } -/// \brief Tries to parse the response body as an ErrorResponse. -Result TryParseErrorResponse(const std::string& text) { - if (text.empty()) { - return InvalidArgument("Empty response body"); - } - ICEBERG_ASSIGN_OR_RAISE(auto json_result, FromJsonString(text)); - ICEBERG_ASSIGN_OR_RAISE(auto error_result, ErrorResponseFromJson(json_result)); - return error_result; -} - /// \brief Handles failure responses by invoking the provided error handler. Status HandleFailureResponse(const cpr::Response& response, const ErrorHandler& error_handler) { if (IsSuccessful(response.status_code)) { return {}; } - auto parse_result = TryParseErrorResponse(response.text); + auto parse_result = error_handler.ParseResponse( + static_cast(response.status_code), response.text); const ErrorResponse final_error = parse_result.value_or(BuildDefaultErrorResponse(response)); return error_handler.Accept(final_error); diff --git a/src/iceberg/catalog/rest/rest_catalog.cc b/src/iceberg/catalog/rest/rest_catalog.cc index 7c79b94d1..4cb4fd349 100644 --- a/src/iceberg/catalog/rest/rest_catalog.cc +++ b/src/iceberg/catalog/rest/rest_catalog.cc @@ -48,6 +48,7 @@ #include "iceberg/sort_order.h" #include "iceberg/table.h" #include "iceberg/table_requirement.h" +#include "iceberg/table_requirements.h" #include "iceberg/table_update.h" #include "iceberg/transaction.h" #include "iceberg/util/macros.h" @@ -89,7 +90,7 @@ Result FetchServerConfig(const ResourcePaths& paths, ICEBERG_ASSIGN_OR_RAISE(const auto response, client.Get(config_path, params, /*headers=*/{}, - *DefaultErrorHandler::Instance(), session)); + *ConfigErrorHandler::Instance(), session)); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); return CatalogConfigFromJson(json); } @@ -710,9 +711,14 @@ Result RestCatalog::UpdateTableInternal( } ICEBERG_ASSIGN_OR_RAISE(auto json_request, ToJsonString(ToJson(request))); - ICEBERG_ASSIGN_OR_RAISE(const auto response, - client_->Post(path, json_request, /*headers=*/{}, - *TableErrorHandler::Instance(), session)); + ICEBERG_ASSIGN_OR_RAISE(auto is_create, TableRequirements::IsCreate(requirements)); + const ErrorHandler* error_handler = TableCommitErrorHandler::Instance().get(); + if (is_create) { + error_handler = CreateTableErrorHandler::Instance().get(); + } + ICEBERG_ASSIGN_OR_RAISE( + const auto response, + client_->Post(path, json_request, /*headers=*/{}, *error_handler, session)); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(response.body())); ICEBERG_ASSIGN_OR_RAISE(auto commit_response, CommitTableResponseFromJson(json)); diff --git a/src/iceberg/result.h b/src/iceberg/result.h index 70155db6a..8765f852a 100644 --- a/src/iceberg/result.h +++ b/src/iceberg/result.h @@ -53,6 +53,7 @@ enum class ErrorKind { kNoSuchPlanId, kNoSuchPlanTask, kNoSuchTable, + kNoSuchWarehouse, kNoSuchView, kNotAllowed, kNotAuthorized, @@ -118,6 +119,7 @@ DEFINE_ERROR_FUNCTION(NoSuchNamespace) DEFINE_ERROR_FUNCTION(NoSuchPlanId) DEFINE_ERROR_FUNCTION(NoSuchPlanTask) DEFINE_ERROR_FUNCTION(NoSuchTable) +DEFINE_ERROR_FUNCTION(NoSuchWarehouse) DEFINE_ERROR_FUNCTION(NoSuchView) DEFINE_ERROR_FUNCTION(NotAllowed) DEFINE_ERROR_FUNCTION(NotAuthorized) diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index bf00c91ac..c681de8c9 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -291,6 +291,7 @@ if(ICEBERG_BUILD_REST) add_rest_iceberg_test(rest_catalog_test SOURCES auth_manager_test.cc + error_handlers_test.cc endpoint_test.cc rest_file_io_test.cc rest_json_serde_test.cc diff --git a/src/iceberg/test/error_handlers_test.cc b/src/iceberg/test/error_handlers_test.cc new file mode 100644 index 000000000..52b2f0da6 --- /dev/null +++ b/src/iceberg/test/error_handlers_test.cc @@ -0,0 +1,285 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/catalog/rest/error_handlers.h" + +#include + +#include + +#include "iceberg/catalog/rest/types.h" +#include "iceberg/test/matchers.h" + +namespace iceberg::rest { + +namespace { + +void ExpectErrorWithMessage(const Status& status, ErrorKind kind, + std::string_view message) { + ASSERT_FALSE(status.has_value()); + EXPECT_EQ(status.error().kind, kind); + EXPECT_EQ(status.error().message, message); +} + +} // namespace + +TEST(ErrorHandlersTest, DefaultErrorHandlerIncludesCodeAndType) { + ErrorResponse error{ + .code = 422, + .type = "ValidationException", + .message = "Invalid input", + }; + + ExpectErrorWithMessage( + DefaultErrorHandler::Instance()->Accept(error), ErrorKind::kRestError, + "Unable to process (code: 422, type: ValidationException): Invalid input"); +} + +TEST(ErrorHandlersTest, DefaultErrorHandlerWithCodeOnly) { + ErrorResponse error{ + .code = 422, + .type = "", + .message = "", + }; + + ExpectErrorWithMessage(DefaultErrorHandler::Instance()->Accept(error), + ErrorKind::kRestError, + "Unable to process (code: 422, type: null): null"); +} + +TEST(ErrorHandlersTest, DefaultErrorHandlerWithCodeAndMessageOnly) { + ErrorResponse error{ + .code = 422, + .type = "", + .message = "Invalid input", + }; + + ExpectErrorWithMessage(DefaultErrorHandler::Instance()->Accept(error), + ErrorKind::kRestError, + "Unable to process (code: 422, type: null): Invalid input"); +} + +TEST(ErrorHandlersTest, DefaultErrorHandlerWithCodeAndTypeOnly) { + ErrorResponse error{ + .code = 422, + .type = "ValidationException", + .message = "", + }; + + ExpectErrorWithMessage( + DefaultErrorHandler::Instance()->Accept(error), ErrorKind::kRestError, + "Unable to process (code: 422, type: ValidationException): null"); +} + +TEST(ErrorHandlersTest, NamespaceErrorHandlerFormats422AsRestError) { + ErrorResponse error{ + .code = 422, + .type = "ValidationException", + .message = "Invalid namespace", + }; + + ExpectErrorWithMessage( + NamespaceErrorHandler::Instance()->Accept(error), ErrorKind::kRestError, + "Unable to process (code: 422, type: ValidationException): Invalid namespace"); +} + +TEST(ErrorHandlersTest, TableErrorHandlerMaps404NotFoundToNotFound) { + ErrorResponse error{ + .code = 404, + .type = "NotFoundException", + .message = "Failed to open input stream for file: metadata.json", + }; + + EXPECT_THAT(TableErrorHandler::Instance()->Accept(error), + IsError(ErrorKind::kNotFound)); + EXPECT_THAT(TableErrorHandler::Instance()->Accept(error), + HasErrorMessage("metadata.json")); +} + +TEST(ErrorHandlersTest, TableErrorHandlerMaps404ToNoSuchTableByDefault) { + ErrorResponse error{ + .code = 404, + .type = "NoSuchTableException", + .message = "Table does not exist", + }; + + EXPECT_THAT(TableErrorHandler::Instance()->Accept(error), + IsError(ErrorKind::kNoSuchTable)); +} + +TEST(ErrorHandlersTest, CreateTableErrorHandlerMaps404ToNoSuchNamespace) { + ErrorResponse error{ + .code = 404, + .type = "NoSuchNamespaceException", + .message = "Namespace does not exist", + }; + + EXPECT_THAT(CreateTableErrorHandler::Instance()->Accept(error), + IsError(ErrorKind::kNoSuchNamespace)); +} + +TEST(ErrorHandlersTest, CreateTableErrorHandlerMaps409ToAlreadyExists) { + ErrorResponse error{ + .code = 409, + .type = "AlreadyExistsException", + .message = "Table already exists", + }; + + EXPECT_THAT(CreateTableErrorHandler::Instance()->Accept(error), + IsError(ErrorKind::kAlreadyExists)); +} + +TEST(ErrorHandlersTest, CreateTableErrorHandlerMapsServiceFailureToCommitStateUnknown) { + ErrorResponse error{ + .code = 503, + .type = "ServiceFailureException", + .message = "Service unavailable", + }; + + EXPECT_THAT(CreateTableErrorHandler::Instance()->Accept(error), + IsError(ErrorKind::kCommitStateUnknown)); + EXPECT_THAT(CreateTableErrorHandler::Instance()->Accept(error), + HasErrorMessage("Service failed: 503: Service unavailable")); +} + +TEST(ErrorHandlersTest, PlanErrorHandlerMapsUnknown404ToNoSuchPlanId) { + ErrorResponse error{ + .code = 404, + .type = "UnknownException", + .message = "Plan does not exist", + }; + + EXPECT_THAT(PlanErrorHandler::Instance()->Accept(error), + IsError(ErrorKind::kNoSuchPlanId)); +} + +TEST(ErrorHandlersTest, PlanErrorHandlerDelegates406ToDefaultHandler) { + ErrorResponse error{ + .code = 406, + .type = "NotAcceptableException", + .message = "Not acceptable", + }; + + ExpectErrorWithMessage( + PlanErrorHandler::Instance()->Accept(error), ErrorKind::kRestError, + "Unable to process (code: 406, type: NotAcceptableException): Not acceptable"); +} + +TEST(ErrorHandlersTest, PlanTaskErrorHandlerMapsUnknown404ToNoSuchPlanTask) { + ErrorResponse error{ + .code = 404, + .type = "UnknownException", + .message = "Plan task does not exist", + }; + + EXPECT_THAT(PlanTaskErrorHandler::Instance()->Accept(error), + IsError(ErrorKind::kNoSuchPlanTask)); +} + +TEST(ErrorHandlersTest, OAuthErrorHandlerMapsInvalidClientToNotAuthorized) { + ErrorResponse error{ + .code = 400, + .type = "invalid_client", + .message = "Credentials given were invalid", + }; + + EXPECT_THAT(OAuthErrorHandler::Instance()->Accept(error), + IsError(ErrorKind::kNotAuthorized)); + EXPECT_THAT( + OAuthErrorHandler::Instance()->Accept(error), + HasErrorMessage("Not authorized: invalid_client: Credentials given were invalid")); +} + +TEST(ErrorHandlersTest, OAuthErrorHandlerParsesOAuthErrorResponse) { + auto parse_result = OAuthErrorHandler::Instance()->ParseResponse( + 400, + R"({"error":"invalid_client","error_description":"Credentials given were invalid"})"); + ASSERT_TRUE(parse_result.has_value()); + + EXPECT_EQ(parse_result->code, 400); + EXPECT_EQ(parse_result->type, "invalid_client"); + EXPECT_EQ(parse_result->message, "Credentials given were invalid"); + EXPECT_THAT(OAuthErrorHandler::Instance()->Accept(*parse_result), + IsError(ErrorKind::kNotAuthorized)); +} + +TEST(ErrorHandlersTest, OAuthErrorHandlerMapsClientErrorsToBadRequest) { + ErrorResponse error{ + .code = 400, + .type = "invalid_grant", + .message = "Grant is invalid", + }; + + EXPECT_THAT(OAuthErrorHandler::Instance()->Accept(error), + IsError(ErrorKind::kBadRequest)); + EXPECT_THAT(OAuthErrorHandler::Instance()->Accept(error), + HasErrorMessage("Malformed request: invalid_grant: Grant is invalid")); +} + +TEST(ErrorHandlersTest, ConfigErrorHandlerMapsTyped404ToNoSuchWarehouse) { + ErrorResponse error{ + .code = 404, + .type = "NotFoundException", + .message = "Warehouse not found", + }; + + EXPECT_THAT(ConfigErrorHandler::Instance()->Accept(error), + IsError(ErrorKind::kNoSuchWarehouse)); +} + +TEST(ErrorHandlersTest, ConfigErrorHandlerDelegatesUntyped404ToDefaultHandler) { + ErrorResponse error{ + .code = 404, + .type = "", + .message = "Not Found", + }; + + EXPECT_THAT(ConfigErrorHandler::Instance()->Accept(error), + IsError(ErrorKind::kRestError)); + EXPECT_THAT(ConfigErrorHandler::Instance()->Accept(error), + HasErrorMessage("Not Found")); +} + +TEST(ErrorHandlersTest, ConfigErrorHandlerDelegatesFallback404ToDefaultHandler) { + ErrorResponse error{ + .code = 404, + .type = "RESTException", + .message = "Not Found", + }; + + EXPECT_THAT(ConfigErrorHandler::Instance()->Accept(error), + IsError(ErrorKind::kRestError)); + EXPECT_THAT(ConfigErrorHandler::Instance()->Accept(error), + HasErrorMessage("Not Found")); +} + +TEST(ErrorHandlersTest, ConfigErrorHandlerDelegatesNon404ToDefaultHandler) { + ErrorResponse error{ + .code = 500, + .type = "", + .message = "Internal server error", + }; + + EXPECT_THAT(ConfigErrorHandler::Instance()->Accept(error), + IsError(ErrorKind::kInternalServerError)); + EXPECT_THAT(ConfigErrorHandler::Instance()->Accept(error), + HasErrorMessage("Internal server error")); +} + +} // namespace iceberg::rest diff --git a/src/iceberg/test/meson.build b/src/iceberg/test/meson.build index 6f9c4c31b..b01a61904 100644 --- a/src/iceberg/test/meson.build +++ b/src/iceberg/test/meson.build @@ -131,6 +131,7 @@ if get_option('rest').enabled() 'sources': files( 'auth_manager_test.cc', 'endpoint_test.cc', + 'error_handlers_test.cc', 'rest_file_io_test.cc', 'rest_json_serde_test.cc', 'rest_util_test.cc', From f38090e0c7b192cdb95bf9c153f135e870c19fa1 Mon Sep 17 00:00:00 2001 From: Abanoub Doss Date: Mon, 22 Jun 2026 23:06:08 -0500 Subject: [PATCH 09/16] ci: cache the sccache directory across C++ test builds (#765) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What Turn on compiler caching (sccache) for the Linux and macOS builds in `test`, `aws_test`, `sanitizer_test`, and `sql_catalog_test`, and switch the Windows `test` build to the same setup. `main` builds once and saves the cache; pull requests reuse it without writing back. ## Why Right now only the Windows builds reuse compiled output — every Linux and macOS build recompiles the whole bundled Arrow/Parquet/Avro/Boost stack from scratch, even though it never changes between PRs. Building it once and reusing it removes most of that repeated work. Saving the cache as a single file (instead of one upload per compiled file) also avoids the upload rate limit that causes "cache write error" spam. ## Validation On a warm pull-request run, every build reused the cache: 99.6–99.9% of files came from cache, zero write errors. The heavy builds drop from ~10–27 min to ~1.5–5 min. --------- Co-authored-by: Abanoub Doss --- .github/workflows/aws_test.yml | 47 ++++++++++++- .github/workflows/sanitizer_test.yml | 24 ++++++- .github/workflows/sql_catalog_test.yml | 23 +++++++ .github/workflows/test.yml | 93 ++++++++++++++++++++++---- 4 files changed, 171 insertions(+), 16 deletions(-) diff --git a/.github/workflows/aws_test.yml b/.github/workflows/aws_test.yml index 4d58edddb..388d7508e 100644 --- a/.github/workflows/aws_test.yml +++ b/.github/workflows/aws_test.yml @@ -74,6 +74,8 @@ jobs: AWS_DEFAULT_REGION: us-east-1 AWS_ENDPOINT_URL: http://127.0.0.1:9000 AWS_EC2_METADATA_DISABLED: "TRUE" + SCCACHE_DIR: ${{ github.workspace }}/.sccache + SCCACHE_CACHE_SIZE: "2G" steps: - name: Checkout iceberg-cpp uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 @@ -113,11 +115,29 @@ jobs: if: ${{ matrix.s3 == 'ON' }} shell: bash run: bash ci/scripts/start_minio.sh + - name: Restore sccache cache + uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-aws-${{ matrix.runs-on }}-bundle${{ matrix.bundle_awssdk }}-s3${{ matrix.s3 }}-sigv4${{ matrix.sigv4 }}-${{ github.run_id }} + restore-keys: | + sccache-aws-${{ matrix.runs-on }}-bundle${{ matrix.bundle_awssdk }}-s3${{ matrix.s3 }}-sigv4${{ matrix.sigv4 }}- + - name: Setup sccache + uses: mozilla-actions/sccache-action@9e7fa8a12102821edf02ca5dbea1acd0f89a2696 # v0.0.10 - name: Build and test Iceberg shell: bash env: CMAKE_TOOLCHAIN_FILE: ${{ startsWith(matrix.runs-on, 'ubuntu') && matrix.bundle_awssdk == 'OFF' && '/usr/local/share/vcpkg/scripts/buildsystems/vcpkg.cmake' || '' }} - run: ci/scripts/build_iceberg.sh "$(pwd)" OFF OFF ${{ matrix.s3 }} ${{ matrix.sigv4 }} ${{ matrix.bundle_awssdk }} + run: ci/scripts/build_iceberg.sh "$(pwd)" OFF ON ${{ matrix.s3 }} ${{ matrix.sigv4 }} ${{ matrix.bundle_awssdk }} + - name: Show sccache stats + shell: bash + run: sccache --show-stats + - name: Save sccache cache + if: github.ref == 'refs/heads/main' + uses: actions/cache/save@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-aws-${{ matrix.runs-on }}-bundle${{ matrix.bundle_awssdk }}-s3${{ matrix.s3 }}-sigv4${{ matrix.sigv4 }}-${{ github.run_id }} # Exercise the Meson build with SigV4 enabled (resolves aws-cpp-sdk-core via # its CMake config, not pkg-config whose Cflags force -std=c++11). @@ -126,6 +146,9 @@ jobs: name: Meson SigV4 (AMD64 Ubuntu 24.04) runs-on: ubuntu-24.04 timeout-minutes: 45 + env: + SCCACHE_DIR: ${{ github.workspace }}/.sccache + SCCACHE_CACHE_SIZE: "2G" steps: - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 with: @@ -161,9 +184,20 @@ jobs: echo "::error::vcpkg install failed after 3 attempts" exit 1 - name: Set Ubuntu Compilers + # Wrap the compiler with sccache: Meson uses an explicit CC/CXX verbatim, + # so the launcher must be prepended here to route compiles through sccache. run: | - echo "CC=gcc-14" >> $GITHUB_ENV - echo "CXX=g++-14" >> $GITHUB_ENV + echo "CC=sccache gcc-14" >> $GITHUB_ENV + echo "CXX=sccache g++-14" >> $GITHUB_ENV + - name: Restore sccache cache + uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-meson-sigv4-${{ github.run_id }} + restore-keys: | + sccache-meson-sigv4- + - name: Setup sccache + uses: mozilla-actions/sccache-action@9e7fa8a12102821edf02ca5dbea1acd0f89a2696 # v0.0.10 - name: Build and test Iceberg shell: bash env: @@ -171,4 +205,11 @@ jobs: run: | meson setup builddir -Dsigv4=enabled meson compile -C builddir + sccache --show-stats meson test -C builddir --timeout-multiplier 0 --print-errorlogs + - name: Save sccache cache + if: github.ref == 'refs/heads/main' + uses: actions/cache/save@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-meson-sigv4-${{ github.run_id }} diff --git a/.github/workflows/sanitizer_test.yml b/.github/workflows/sanitizer_test.yml index f4edecfec..e461bd1ed 100644 --- a/.github/workflows/sanitizer_test.yml +++ b/.github/workflows/sanitizer_test.yml @@ -39,6 +39,9 @@ jobs: if: ${{ github.event_name != 'pull_request' || github.event.pull_request.draft == false }} name: "ASAN and UBSAN Tests" runs-on: ubuntu-24.04 + env: + SCCACHE_DIR: ${{ github.workspace }}/.sccache + SCCACHE_CACHE_SIZE: "2G" steps: - name: Checkout iceberg-cpp uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 @@ -47,14 +50,33 @@ jobs: - name: Install dependencies shell: bash run: sudo apt-get update && sudo apt-get install -y libcurl4-openssl-dev + - name: Restore sccache cache + uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-sanitizer-ubuntu-${{ github.run_id }} + restore-keys: | + sccache-sanitizer-ubuntu- + - name: Setup sccache + uses: mozilla-actions/sccache-action@9e7fa8a12102821edf02ca5dbea1acd0f89a2696 # v0.0.10 - name: Configure and Build with ASAN & UBSAN env: CC: gcc-14 CXX: g++-14 run: | mkdir build && cd build - cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Debug -DICEBERG_ENABLE_ASAN=ON -DICEBERG_ENABLE_UBSAN=ON + cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Debug -DICEBERG_ENABLE_ASAN=ON -DICEBERG_ENABLE_UBSAN=ON \ + -DCMAKE_C_COMPILER_LAUNCHER=sccache -DCMAKE_CXX_COMPILER_LAUNCHER=sccache cmake --build . --verbose + - name: Show sccache stats + shell: bash + run: sccache --show-stats + - name: Save sccache cache + if: github.ref == 'refs/heads/main' + uses: actions/cache/save@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-sanitizer-ubuntu-${{ github.run_id }} - name: Run Tests working-directory: build env: diff --git a/.github/workflows/sql_catalog_test.yml b/.github/workflows/sql_catalog_test.yml index 79c6328b5..d089ae4c6 100644 --- a/.github/workflows/sql_catalog_test.yml +++ b/.github/workflows/sql_catalog_test.yml @@ -61,6 +61,9 @@ jobs: runs-on: windows-2025 cmake_build_type: Release cmake_extra_args: -DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake + env: + SCCACHE_DIR: ${{ github.workspace }}/.sccache + SCCACHE_CACHE_SIZE: "2G" steps: - name: Checkout iceberg-cpp uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 @@ -92,6 +95,15 @@ jobs: shell: pwsh run: | vcpkg install zlib:x64-windows nlohmann-json:x64-windows nanoarrow:x64-windows roaring:x64-windows sqlite3:x64-windows + - name: Restore sccache cache + uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-sqlcatalog-${{ matrix.runs-on }}-${{ github.run_id }} + restore-keys: | + sccache-sqlcatalog-${{ matrix.runs-on }}- + - name: Setup sccache + uses: mozilla-actions/sccache-action@9e7fa8a12102821edf02ca5dbea1acd0f89a2696 # v0.0.10 - name: Configure Iceberg shell: bash run: | @@ -103,10 +115,21 @@ jobs: -DICEBERG_BUILD_REST=OFF \ -DICEBERG_BUILD_SQL_CATALOG=ON \ -DICEBERG_SQL_SQLITE=ON \ + -DCMAKE_C_COMPILER_LAUNCHER=sccache \ + -DCMAKE_CXX_COMPILER_LAUNCHER=sccache \ ${{ matrix.cmake_extra_args }} - name: Build SQL catalog tests shell: bash run: cmake --build build --target sql_catalog_test + - name: Show sccache stats + shell: bash + run: sccache --show-stats + - name: Save sccache cache + if: github.ref == 'refs/heads/main' + uses: actions/cache/save@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-sqlcatalog-${{ matrix.runs-on }}-${{ github.run_id }} - name: Run SQL catalog tests shell: bash run: ctest --test-dir build -R '^sql_catalog_test$' --output-on-failure -C ${{ matrix.cmake_build_type }} diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 6f420bde5..a51a29866 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -45,6 +45,9 @@ jobs: timeout-minutes: 30 strategy: fail-fast: false + env: + SCCACHE_DIR: ${{ github.workspace }}/.sccache + SCCACHE_CACHE_SIZE: "2G" steps: - name: Checkout iceberg-cpp uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 @@ -53,12 +56,30 @@ jobs: - name: Install dependencies shell: bash run: sudo apt-get update && sudo apt-get install -y libcurl4-openssl-dev + - name: Restore sccache cache + uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-test-ubuntu-${{ github.run_id }} + restore-keys: | + sccache-test-ubuntu- + - name: Setup sccache + uses: mozilla-actions/sccache-action@9e7fa8a12102821edf02ca5dbea1acd0f89a2696 # v0.0.10 - name: Build Iceberg shell: bash env: CC: gcc-14 CXX: g++-14 - run: ci/scripts/build_iceberg.sh $(pwd) ON + run: ci/scripts/build_iceberg.sh $(pwd) ON ON + - name: Show sccache stats + shell: bash + run: sccache --show-stats + - name: Save sccache cache + if: github.ref == 'refs/heads/main' + uses: actions/cache/save@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-test-ubuntu-${{ github.run_id }} - name: Build Example shell: bash env: @@ -72,14 +93,35 @@ jobs: timeout-minutes: 30 strategy: fail-fast: false + env: + SCCACHE_DIR: ${{ github.workspace }}/.sccache + SCCACHE_CACHE_SIZE: "2G" steps: - name: Checkout iceberg-cpp uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 with: persist-credentials: false + - name: Restore sccache cache + uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-test-macos-${{ github.run_id }} + restore-keys: | + sccache-test-macos- + - name: Setup sccache + uses: mozilla-actions/sccache-action@9e7fa8a12102821edf02ca5dbea1acd0f89a2696 # v0.0.10 - name: Build Iceberg shell: bash - run: ci/scripts/build_iceberg.sh $(pwd) + run: ci/scripts/build_iceberg.sh $(pwd) OFF ON + - name: Show sccache stats + shell: bash + run: sccache --show-stats + - name: Save sccache cache + if: github.ref == 'refs/heads/main' + uses: actions/cache/save@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-test-macos-${{ github.run_id }} - name: Build Example shell: bash run: ci/scripts/build_example.sh $(pwd)/example @@ -90,6 +132,9 @@ jobs: timeout-minutes: 60 strategy: fail-fast: false + env: + SCCACHE_DIR: ${{ github.workspace }}/.sccache + SCCACHE_CACHE_SIZE: "2G" steps: - name: Checkout iceberg-cpp uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 @@ -110,17 +155,28 @@ jobs: shell: pwsh run: | vcpkg install zlib:x64-windows nlohmann-json:x64-windows nanoarrow:x64-windows roaring:x64-windows cpr:x64-windows + - name: Restore sccache cache + uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-test-windows-${{ github.run_id }} + restore-keys: | + sccache-test-windows- - name: Setup sccache uses: mozilla-actions/sccache-action@9e7fa8a12102821edf02ca5dbea1acd0f89a2696 # v0.0.10 - name: Build Iceberg shell: pwsh - env: - SCCACHE_GHA_ENABLED: "true" run: | $ErrorActionPreference = "Stop" bash -lc 'ci/scripts/build_iceberg.sh $(pwd) OFF ON' if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } sccache --show-stats + - name: Save sccache cache + if: github.ref == 'refs/heads/main' + uses: actions/cache/save@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-test-windows-${{ github.run_id }} - name: Build Example shell: pwsh run: | @@ -131,6 +187,9 @@ jobs: name: Meson - ${{ matrix.title }} runs-on: ${{ matrix.runs-on }} timeout-minutes: 30 + env: + SCCACHE_DIR: ${{ github.workspace }}/.sccache + SCCACHE_CACHE_SIZE: "2G" strategy: max-parallel: 15 fail-fast: false @@ -159,24 +218,34 @@ jobs: python3 -m pip install --upgrade pip python3 -m pip install -r requirements.txt - name: Set Ubuntu Compilers + # Wrap the compiler with sccache: Meson auto-detects sccache for the + # default compiler (macOS/Windows), but uses an explicit CC/CXX verbatim, + # so the launcher must be prepended here to route Ubuntu compiles through it. if: ${{ startsWith(matrix.runs-on, 'ubuntu') }} run: | - echo "CC=${{ matrix.CC }}" >> $GITHUB_ENV - echo "CXX=${{ matrix.CXX }}" >> $GITHUB_ENV + echo "CC=sccache ${{ matrix.CC }}" >> $GITHUB_ENV + echo "CXX=sccache ${{ matrix.CXX }}" >> $GITHUB_ENV + - name: Restore sccache cache + uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-meson-${{ matrix.runs-on }}-${{ github.run_id }} + restore-keys: | + sccache-meson-${{ matrix.runs-on }}- - name: Setup sccache - if: ${{ startsWith(matrix.runs-on, 'windows') }} uses: mozilla-actions/sccache-action@9e7fa8a12102821edf02ca5dbea1acd0f89a2696 # v0.0.10 - - name: Enable sccache - if: ${{ startsWith(matrix.runs-on, 'windows') }} - shell: bash - run: echo "SCCACHE_GHA_ENABLED=true" >> "$GITHUB_ENV" - name: Build Iceberg run: | meson setup builddir ${{ matrix.meson-setup-args || '' }} meson compile -C builddir - name: Show sccache stats - if: ${{ startsWith(matrix.runs-on, 'windows') }} run: sccache --show-stats + - name: Save sccache cache + if: github.ref == 'refs/heads/main' + uses: actions/cache/save@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ${{ github.workspace }}/.sccache + key: sccache-meson-${{ matrix.runs-on }}-${{ github.run_id }} - name: Test Iceberg run: | meson test -C builddir --timeout-multiplier 0 --print-errorlogs From 959fda6703bb74a0268c7038d48e189427a2f67e Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Tue, 23 Jun 2026 12:47:54 +0800 Subject: [PATCH 10/16] feat: add Iceberg v3 type definitions (#752) Introduce the Iceberg v3 types (variant, geometry, geography), including their schema/JSON serialization and type-system integration (visitors, schema projection, etc.). Reading and writing data of these types is not implemented yet: conversion to/from Arrow, Avro, and Parquet returns an error, as do identity transform binding and scalar validation for them. --- src/iceberg/avro/avro_schema_util.cc | 17 ++ src/iceberg/avro/avro_schema_util_internal.h | 3 + src/iceberg/delete_file_index.cc | 4 +- src/iceberg/json_serde.cc | 122 +++++++--- src/iceberg/metrics_config.cc | 5 +- src/iceberg/parquet/parquet_metrics.cc | 4 + src/iceberg/parquet/parquet_schema_util.cc | 5 + src/iceberg/parquet/parquet_writer.cc | 4 + src/iceberg/schema_internal.cc | 35 +++ src/iceberg/table_metadata.h | 5 +- src/iceberg/test/arrow_test.cc | 14 ++ src/iceberg/test/rest_json_serde_test.cc | 2 +- src/iceberg/test/schema_json_test.cc | 57 +++++ src/iceberg/test/schema_test.cc | 57 +++-- src/iceberg/test/transform_test.cc | 13 ++ src/iceberg/test/type_test.cc | 105 ++++++++- src/iceberg/test/visit_type_test.cc | 40 +++- src/iceberg/transform.cc | 4 +- src/iceberg/transform_function.cc | 4 +- src/iceberg/type.cc | 229 +++++++++++++++++-- src/iceberg/type.h | 170 ++++++++++++-- src/iceberg/type_fwd.h | 15 ++ src/iceberg/update/update_schema.cc | 6 + src/iceberg/util/struct_like_set.cc | 5 + src/iceberg/util/type_util.cc | 37 ++- src/iceberg/util/type_util.h | 4 + src/iceberg/util/visit_type.h | 9 +- src/iceberg/util/visitor_generate.h | 10 + 28 files changed, 859 insertions(+), 126 deletions(-) diff --git a/src/iceberg/avro/avro_schema_util.cc b/src/iceberg/avro/avro_schema_util.cc index c26e2bbc9..14b464cee 100644 --- a/src/iceberg/avro/avro_schema_util.cc +++ b/src/iceberg/avro/avro_schema_util.cc @@ -248,6 +248,18 @@ Status ToAvroNodeVisitor::Visit(const UnknownType&, ::avro::NodePtr* node) { return {}; } +Status ToAvroNodeVisitor::Visit(const VariantType&, ::avro::NodePtr*) { + return NotSupported("Writing Iceberg variant type to Avro is not supported"); +} + +Status ToAvroNodeVisitor::Visit(const GeometryType&, ::avro::NodePtr*) { + return NotSupported("Writing Iceberg geometry type to Avro is not supported"); +} + +Status ToAvroNodeVisitor::Visit(const GeographyType&, ::avro::NodePtr*) { + return NotSupported("Writing Iceberg geography type to Avro is not supported"); +} + Status ToAvroNodeVisitor::Visit(const StructType& type, ::avro::NodePtr* node) { *node = std::make_shared<::avro::NodeRecord>(); @@ -631,6 +643,11 @@ Status ValidateAvroSchemaEvolution(const Type& expected_type, break; case TypeId::kUnknown: return {}; + case TypeId::kVariant: + case TypeId::kGeometry: + case TypeId::kGeography: + return NotSupported("Reading Iceberg type {} from Avro is not supported", + expected_type); default: break; } diff --git a/src/iceberg/avro/avro_schema_util_internal.h b/src/iceberg/avro/avro_schema_util_internal.h index a5bfb989e..342b119a5 100644 --- a/src/iceberg/avro/avro_schema_util_internal.h +++ b/src/iceberg/avro/avro_schema_util_internal.h @@ -59,6 +59,9 @@ class ToAvroNodeVisitor { Status Visit(const FixedType& type, ::avro::NodePtr* node); Status Visit(const BinaryType& type, ::avro::NodePtr* node); Status Visit(const UnknownType&, ::avro::NodePtr*); + Status Visit(const VariantType&, ::avro::NodePtr*); + Status Visit(const GeometryType&, ::avro::NodePtr*); + Status Visit(const GeographyType&, ::avro::NodePtr*); Status Visit(const StructType& type, ::avro::NodePtr* node); Status Visit(const ListType& type, ::avro::NodePtr* node); Status Visit(const MapType& type, ::avro::NodePtr* node); diff --git a/src/iceberg/delete_file_index.cc b/src/iceberg/delete_file_index.cc index 7c8c35032..8c58e861b 100644 --- a/src/iceberg/delete_file_index.cc +++ b/src/iceberg/delete_file_index.cc @@ -56,7 +56,7 @@ Status EqualityDeleteFile::ConvertBoundsIfNeeded() const { } const auto& schema_field = field.value().get(); - if (schema_field.type()->is_nested()) { + if (!schema_field.type()->is_primitive()) { continue; } @@ -103,7 +103,7 @@ Result CanContainEqDeletesForFile(const DataFile& data_file, } const auto& field = found_field.value().get(); - if (field.type()->is_nested()) { + if (!field.type()->is_primitive()) { continue; } diff --git a/src/iceberg/json_serde.cc b/src/iceberg/json_serde.cc index 1f2b8f45c..e137aed1d 100644 --- a/src/iceberg/json_serde.cc +++ b/src/iceberg/json_serde.cc @@ -389,6 +389,12 @@ nlohmann::json ToJson(const Type& type) { return "uuid"; case TypeId::kUnknown: return "unknown"; + case TypeId::kVariant: + return "variant"; + case TypeId::kGeometry: + return type.ToString(); + case TypeId::kGeography: + return type.ToString(); } std::unreachable(); } @@ -459,9 +465,10 @@ Result> ListTypeFromJson(const nlohmann::json& json) { ICEBERG_ASSIGN_OR_RAISE(auto element_required, GetJsonValue(json, kElementRequired)); - return std::make_unique( - SchemaField(element_id, std::string(ListType::kElementName), - std::move(element_type), !element_required)); + ICEBERG_ASSIGN_OR_RAISE(auto type, ListType::Make(SchemaField( + element_id, std::string(ListType::kElementName), + std::move(element_type), !element_required))); + return std::unique_ptr(std::move(type)); } Result> MapTypeFromJson(const nlohmann::json& json) { @@ -478,79 +485,126 @@ Result> MapTypeFromJson(const nlohmann::json& json) { /*optional=*/false); SchemaField value_field(value_id, std::string(MapType::kValueName), std::move(value_type), !value_required); - return std::make_unique(std::move(key_field), std::move(value_field)); + ICEBERG_ASSIGN_OR_RAISE(auto type, + MapType::Make(std::move(key_field), std::move(value_field))); + return std::unique_ptr(std::move(type)); } } // namespace Result> TypeFromJson(const nlohmann::json& json) { if (json.is_string()) { - std::string type_str = json.get(); - if (type_str == "boolean") { + const auto type_name = json.get(); + const auto normalized_type_name = StringUtils::ToLower(type_name); + if (normalized_type_name == "boolean") { return std::make_unique(); - } else if (type_str == "int") { + } else if (normalized_type_name == "int") { return std::make_unique(); - } else if (type_str == "long") { + } else if (normalized_type_name == "long") { return std::make_unique(); - } else if (type_str == "float") { + } else if (normalized_type_name == "float") { return std::make_unique(); - } else if (type_str == "double") { + } else if (normalized_type_name == "double") { return std::make_unique(); - } else if (type_str == "date") { + } else if (normalized_type_name == "date") { return std::make_unique(); - } else if (type_str == "time") { + } else if (normalized_type_name == "time") { return std::make_unique(); - } else if (type_str == "timestamp") { + } else if (normalized_type_name == "timestamp") { return std::make_unique(); - } else if (type_str == "timestamptz") { + } else if (normalized_type_name == "timestamptz") { return std::make_unique(); - } else if (type_str == "timestamp_ns") { + } else if (normalized_type_name == "timestamp_ns") { return std::make_unique(); - } else if (type_str == "timestamptz_ns") { + } else if (normalized_type_name == "timestamptz_ns") { return std::make_unique(); - } else if (type_str == "string") { + } else if (normalized_type_name == "string") { return std::make_unique(); - } else if (type_str == "binary") { + } else if (normalized_type_name == "binary") { return std::make_unique(); - } else if (type_str == "uuid") { + } else if (normalized_type_name == "uuid") { return std::make_unique(); - } else if (type_str == "unknown") { + } else if (normalized_type_name == "unknown") { return std::make_unique(); - } else if (type_str.starts_with("fixed")) { - std::regex fixed_regex(R"(fixed\[\s*(\d+)\s*\])"); + } else if (normalized_type_name == "variant") { + return std::make_unique(); + } else if (normalized_type_name.starts_with("fixed")) { + static const std::regex kFixedRegex(R"(fixed\[\s*(\d+)\s*\])"); std::smatch match; - if (std::regex_match(type_str, match, fixed_regex)) { + if (std::regex_match(normalized_type_name, match, kFixedRegex)) { ICEBERG_ASSIGN_OR_RAISE(auto length, StringUtils::ParseNumber(match[1].str())); return std::make_unique(length); } - return JsonParseError("Invalid fixed type: {}", type_str); - } else if (type_str.starts_with("decimal")) { - std::regex decimal_regex(R"(decimal\(\s*(\d+)\s*,\s*(\d+)\s*\))"); + return JsonParseError("Invalid fixed type: {}", type_name); + } else if (normalized_type_name.starts_with("decimal")) { + static const std::regex kDecimalRegex(R"(decimal\(\s*(\d+)\s*,\s*(\d+)\s*\))"); std::smatch match; - if (std::regex_match(type_str, match, decimal_regex)) { + if (std::regex_match(normalized_type_name, match, kDecimalRegex)) { ICEBERG_ASSIGN_OR_RAISE(auto precision, StringUtils::ParseNumber(match[1].str())); ICEBERG_ASSIGN_OR_RAISE(auto scale, StringUtils::ParseNumber(match[2].str())); return std::make_unique(precision, scale); } - return JsonParseError("Invalid decimal type: {}", type_str); + return JsonParseError("Invalid decimal type: {}", type_name); + } else if (normalized_type_name.starts_with("geometry")) { + static const std::regex kGeometryRegex(R"(geometry\s*(?:\(\s*([^)]*?)\s*\))?)", + std::regex_constants::icase); + std::smatch match; + if (std::regex_match(type_name, match, kGeometryRegex)) { + if (match[1].matched) { + auto crs = match[1].str(); + if (crs.empty()) { + return JsonParseError("Invalid geometry type: {}", type_name); + } + ICEBERG_ASSIGN_OR_RAISE(auto type, GeometryType::Make(std::move(crs))); + return std::unique_ptr(std::move(type)); + } + ICEBERG_ASSIGN_OR_RAISE(auto type, GeometryType::Make()); + return std::unique_ptr(std::move(type)); + } + return JsonParseError("Invalid geometry type: {}", type_name); + } else if (normalized_type_name.starts_with("geography")) { + static const std::regex kGeographyRegex( + R"(geography\s*(?:\(\s*([^,]*?)\s*(?:,\s*(\w*)\s*)?\))?)", + std::regex_constants::icase); + std::smatch match; + if (std::regex_match(type_name, match, kGeographyRegex)) { + auto crs = match[1].str(); + if (match[1].matched && crs.empty()) { + return JsonParseError("Invalid geography type: {}", type_name); + } + if (match[2].matched) { + ICEBERG_ASSIGN_OR_RAISE(auto algorithm, + EdgeAlgorithmFromString(match[2].str())); + ICEBERG_ASSIGN_OR_RAISE(auto type, + GeographyType::Make(std::move(crs), algorithm)); + return std::unique_ptr(std::move(type)); + } + if (match[1].matched) { + ICEBERG_ASSIGN_OR_RAISE(auto type, GeographyType::Make(std::move(crs))); + return std::unique_ptr(std::move(type)); + } + ICEBERG_ASSIGN_OR_RAISE(auto type, GeographyType::Make()); + return std::unique_ptr(std::move(type)); + } + return JsonParseError("Invalid geography type: {}", type_name); } else { - return JsonParseError("Unknown primitive type: {}", type_str); + return JsonParseError("Cannot parse type string: {}", type_name); } } // For complex types like struct, list, and map - ICEBERG_ASSIGN_OR_RAISE(auto type_str, GetJsonValue(json, kType)); - if (type_str == kStruct) { + ICEBERG_ASSIGN_OR_RAISE(auto complex_type_name, GetJsonValue(json, kType)); + if (complex_type_name == kStruct) { return StructTypeFromJson(json); - } else if (type_str == kList) { + } else if (complex_type_name == kList) { return ListTypeFromJson(json); - } else if (type_str == kMap) { + } else if (complex_type_name == kMap) { return MapTypeFromJson(json); } else { - return JsonParseError("Unknown complex type: {}", type_str); + return JsonParseError("Unknown complex type: {}", complex_type_name); } } diff --git a/src/iceberg/metrics_config.cc b/src/iceberg/metrics_config.cc index 95d1a1fe1..f5e30ace9 100644 --- a/src/iceberg/metrics_config.cc +++ b/src/iceberg/metrics_config.cc @@ -197,6 +197,8 @@ Result> MetricsConfig::LimitFieldIds(const Schema& s Status Visit(const Type& type) { if (type.is_nested()) { return VisitNested(internal::checked_cast(type)); + } else if (type.is_variant()) { + return {}; } else { return VisitPrimitive(internal::checked_cast(type)); } @@ -207,8 +209,7 @@ Result> MetricsConfig::LimitFieldIds(const Schema& s if (!ShouldContinue()) { break; } - // TODO(zhuo.wang): variant type should also be handled here - if (field.type()->is_primitive()) { + if (!field.type()->is_nested()) { ids_.insert(field.field_id()); } } diff --git a/src/iceberg/parquet/parquet_metrics.cc b/src/iceberg/parquet/parquet_metrics.cc index 32dc9fec8..dac9ea6a8 100644 --- a/src/iceberg/parquet/parquet_metrics.cc +++ b/src/iceberg/parquet/parquet_metrics.cc @@ -423,6 +423,10 @@ class CollectMetricsVisitor { Status VisitMap(const MapType& /*type*/, const std::string& /*prefix*/) { return {}; } + Status VisitVariant(const VariantType& /*type*/, const std::string& /*prefix*/) { + return {}; + } + Status VisitPrimitive(const PrimitiveType& /*type*/, const std::string& /*prefix*/) { return {}; } diff --git a/src/iceberg/parquet/parquet_schema_util.cc b/src/iceberg/parquet/parquet_schema_util.cc index 39e321d9f..a5629198f 100644 --- a/src/iceberg/parquet/parquet_schema_util.cc +++ b/src/iceberg/parquet/parquet_schema_util.cc @@ -239,6 +239,11 @@ Status ValidateParquetSchemaEvolution( break; case TypeId::kUnknown: return {}; + case TypeId::kVariant: + case TypeId::kGeometry: + case TypeId::kGeography: + return NotSupported("Reading Iceberg type {} from Parquet is not supported", + expected_type); case TypeId::kStruct: if (arrow_type->id() == ::arrow::Type::STRUCT) { return {}; diff --git a/src/iceberg/parquet/parquet_writer.cc b/src/iceberg/parquet/parquet_writer.cc index c50fb26b1..fea7cd834 100644 --- a/src/iceberg/parquet/parquet_writer.cc +++ b/src/iceberg/parquet/parquet_writer.cc @@ -178,6 +178,10 @@ class FieldMetricsCollector { Status VisitMap(const MapType& /*type*/, const ::arrow::Array& /*array*/) { return {}; } + Status VisitVariant(const VariantType& /*type*/, const ::arrow::Array& /*array*/) { + return {}; + } + Status VisitPrimitive(const PrimitiveType& type, const ::arrow::Array& array) { switch (type.type_id()) { case TypeId::kFloat: diff --git a/src/iceberg/schema_internal.cc b/src/iceberg/schema_internal.cc index c32ceb2a6..792341adf 100644 --- a/src/iceberg/schema_internal.cc +++ b/src/iceberg/schema_internal.cc @@ -19,6 +19,7 @@ #include "iceberg/schema_internal.h" +#include #include #include #include @@ -39,6 +40,33 @@ constexpr const char* kArrowExtensionMetadata = "ARROW:extension:metadata"; constexpr const char* kArrowUuidExtensionName = "arrow.uuid"; constexpr int32_t kUnknownFieldId = -1; +Status CheckArrowCompatible(const Type& type) { + switch (type.type_id()) { + case TypeId::kVariant: + case TypeId::kGeometry: + case TypeId::kGeography: + return NotSupported("Iceberg type {} is not supported by Arrow conversion", + type.ToString()); + case TypeId::kStruct: + for (const auto& field : static_cast(type).fields()) { + ICEBERG_RETURN_UNEXPECTED(CheckArrowCompatible(*field.type())); + } + break; + case TypeId::kList: + ICEBERG_RETURN_UNEXPECTED( + CheckArrowCompatible(*static_cast(type).element().type())); + break; + case TypeId::kMap: { + const auto& map_type = static_cast(type); + ICEBERG_RETURN_UNEXPECTED(CheckArrowCompatible(*map_type.key().type())); + ICEBERG_RETURN_UNEXPECTED(CheckArrowCompatible(*map_type.value().type())); + } break; + default: + break; + } + return {}; +} + // Convert an Iceberg type to Arrow schema. Return value is Nanoarrow error code. ArrowErrorCode ToArrowSchema(const Type& type, bool optional, std::string_view name, std::optional field_id, ArrowSchema* schema) { @@ -153,6 +181,11 @@ ArrowErrorCode ToArrowSchema(const Type& type, bool optional, std::string_view n case TypeId::kUnknown: NANOARROW_RETURN_NOT_OK(ArrowSchemaSetType(schema, NANOARROW_TYPE_NA)); break; + case TypeId::kVariant: + case TypeId::kGeometry: + case TypeId::kGeography: + ArrowBufferReset(&metadata_buffer); + return EINVAL; } if (!name.empty()) { @@ -179,6 +212,8 @@ Status ToArrowSchema(const Schema& schema, ArrowSchema* out) { return InvalidArgument("Output Arrow schema cannot be null"); } + ICEBERG_RETURN_UNEXPECTED(CheckArrowCompatible(schema)); + ArrowSchemaInit(out); if (ArrowErrorCode errorCode = ToArrowSchema(schema, /*optional=*/false, /*name=*/"", diff --git a/src/iceberg/table_metadata.h b/src/iceberg/table_metadata.h index 06d636ef5..fd2c27199 100644 --- a/src/iceberg/table_metadata.h +++ b/src/iceberg/table_metadata.h @@ -79,9 +79,8 @@ struct ICEBERG_EXPORT TableMetadata { static constexpr int64_t kInitialRowId = 0; static inline const std::unordered_map kMinFormatVersions = { - {TypeId::kTimestampNs, 3}, - {TypeId::kTimestampTzNs, 3}, - {TypeId::kUnknown, 3}, + {TypeId::kTimestampNs, 3}, {TypeId::kTimestampTzNs, 3}, {TypeId::kUnknown, 3}, + {TypeId::kVariant, 3}, {TypeId::kGeometry, 3}, {TypeId::kGeography, 3}, }; /// An integer version number for the format diff --git a/src/iceberg/test/arrow_test.cc b/src/iceberg/test/arrow_test.cc index 9f8ce86f5..2a7242e71 100644 --- a/src/iceberg/test/arrow_test.cc +++ b/src/iceberg/test/arrow_test.cc @@ -122,6 +122,20 @@ INSTANTIATE_TEST_SUITE_P( ToArrowSchemaParam{.iceberg_type = iceberg::unknown(), .arrow_type = ::arrow::null()})); +TEST(ToArrowSchemaTest, UnsupportedV3Types) { + const std::vector> unsupported_types = { + iceberg::variant(), iceberg::geometry(), iceberg::geography()}; + + for (const auto& unsupported_type : unsupported_types) { + Schema schema( + {SchemaField::MakeOptional(/*field_id=*/1, "unsupported", unsupported_type)}, + /*schema_id=*/0); + ArrowSchema arrow_schema; + ASSERT_THAT(ToArrowSchema(schema, &arrow_schema), + HasErrorMessage("is not supported by Arrow conversion")); + } +} + namespace { void CheckArrowField(const ::arrow::Field& field, ::arrow::Type::type type_id, diff --git a/src/iceberg/test/rest_json_serde_test.cc b/src/iceberg/test/rest_json_serde_test.cc index 7304831c6..50507dd3a 100644 --- a/src/iceberg/test/rest_json_serde_test.cc +++ b/src/iceberg/test/rest_json_serde_test.cc @@ -1078,7 +1078,7 @@ INSTANTIATE_TEST_SUITE_P( CreateTableRequestInvalidParam{ .test_name = "WrongSchemaType", .invalid_json_str = R"({"name":"my_table","schema":"invalid"})", - .expected_error_message = "Unknown primitive type: invalid"}), + .expected_error_message = "Cannot parse type string: invalid"}), [](const ::testing::TestParamInfo& info) { return info.param.test_name; }); diff --git a/src/iceberg/test/schema_json_test.cc b/src/iceberg/test/schema_json_test.cc index 08275a45c..520a1eb70 100644 --- a/src/iceberg/test/schema_json_test.cc +++ b/src/iceberg/test/schema_json_test.cc @@ -65,6 +65,21 @@ INSTANTIATE_TEST_SUITE_P( SchemaJsonParam{.json = "\"binary\"", .type = iceberg::binary()}, SchemaJsonParam{.json = "\"uuid\"", .type = iceberg::uuid()}, SchemaJsonParam{.json = "\"unknown\"", .type = iceberg::unknown()}, + SchemaJsonParam{.json = "\"variant\"", .type = iceberg::variant()}, + SchemaJsonParam{.json = "\"geometry\"", .type = iceberg::geometry()}, + SchemaJsonParam{.json = "\"geometry(srid:4326)\"", + .type = iceberg::geometry("srid:4326")}, + SchemaJsonParam{.json = "\"geography\"", .type = iceberg::geography()}, + SchemaJsonParam{.json = "\"geography(srid:4326)\"", + .type = iceberg::geography("srid:4326")}, + SchemaJsonParam{ + .json = "\"geography(srid:4326, spherical)\"", + .type = iceberg::geography("srid:4326", EdgeAlgorithm::kSpherical)}, + SchemaJsonParam{ + .json = "\"geography(OGC:CRS84, spherical)\"", + .type = iceberg::geography("OGC:CRS84", EdgeAlgorithm::kSpherical)}, + SchemaJsonParam{.json = "\"geography(srid:4326, karney)\"", + .type = iceberg::geography("srid:4326", EdgeAlgorithm::kKarney)}, SchemaJsonParam{.json = "\"fixed[8]\"", .type = iceberg::fixed(8)}, SchemaJsonParam{.json = "\"decimal(10,2)\"", .type = iceberg::decimal(10, 2)}, SchemaJsonParam{.json = "\"date\"", .type = iceberg::date()}, @@ -111,6 +126,48 @@ TEST(TypeJsonTest, FromJsonWithSpaces) { ASSERT_EQ(decimal->scale(), 2); } +TEST(TypeJsonTest, FromJsonV3TypesWithSpacesAndCase) { + auto variant_result = TypeFromJson(nlohmann::json::parse("\"Variant\"")); + ASSERT_TRUE(variant_result.has_value()); + ASSERT_EQ(*variant_result.value(), *iceberg::variant()); + + auto geometry_result = + TypeFromJson(nlohmann::json::parse("\"GEOMETRY( srid: 3857 )\"")); + ASSERT_TRUE(geometry_result.has_value()); + ASSERT_EQ(*geometry_result.value(), *iceberg::geometry("srid: 3857")); + + auto geography_result = + TypeFromJson(nlohmann::json::parse("\"geography(srid:4269,karney)\"")); + ASSERT_TRUE(geography_result.has_value()); + ASSERT_EQ(*geography_result.value(), + *iceberg::geography("srid:4269", EdgeAlgorithm::kKarney)); +} + +TEST(TypeJsonTest, InvalidV3Types) { + auto invalid_geometry = TypeFromJson(nlohmann::json::parse("\"geometry()\"")); + ASSERT_THAT(invalid_geometry, HasErrorMessage("Invalid geometry type")); + + auto invalid_geometry_with_spaces = + TypeFromJson(nlohmann::json::parse("\"geometry( )\"")); + ASSERT_THAT(invalid_geometry_with_spaces, HasErrorMessage("Invalid geometry type")); + + auto invalid_geography = TypeFromJson(nlohmann::json::parse("\"geography()\"")); + ASSERT_THAT(invalid_geography, HasErrorMessage("Invalid geography type")); + + auto invalid_geography_with_algorithm = + TypeFromJson(nlohmann::json::parse("\"geography( , spherical)\"")); + ASSERT_THAT(invalid_geography_with_algorithm, + HasErrorMessage("Invalid geography type")); + + auto invalid_geography_algorithm = + TypeFromJson(nlohmann::json::parse("\"geography(srid:4269, BadAlgorithm)\"")); + ASSERT_THAT(invalid_geography_algorithm, + HasErrorMessage("Invalid edge interpolation algorithm")); + + auto unknown_type = TypeFromJson(nlohmann::json::parse("\"nonsense\"")); + ASSERT_THAT(unknown_type, HasErrorMessage("Cannot parse type string")); +} + TEST(SchemaJsonTest, RoundTrip) { constexpr std::string_view json = R"({"fields":[{"id":1,"name":"id","required":true,"type":"int"},{"id":2,"name":"name","required":false,"type":"string"}],"schema-id":1,"type":"struct"})"; diff --git a/src/iceberg/test/schema_test.cc b/src/iceberg/test/schema_test.cc index 8f1b20035..db99eb02b 100644 --- a/src/iceberg/test/schema_test.cc +++ b/src/iceberg/test/schema_test.cc @@ -671,6 +671,7 @@ iceberg::SchemaField Id() { return {1, "id", iceberg::int32(), true}; } iceberg::SchemaField Name() { return {2, "name", iceberg::string(), false}; } iceberg::SchemaField Age() { return {3, "age", iceberg::int32(), true}; } iceberg::SchemaField Email() { return {4, "email", iceberg::string(), true}; } +iceberg::SchemaField Payload() { return {5, "payload", iceberg::variant(), true}; } iceberg::SchemaField Street() { return {11, "street", iceberg::string(), true}; } iceberg::SchemaField City() { return {12, "city", iceberg::string(), true}; } iceberg::SchemaField Zip() { return {13, "zip", iceberg::int32(), true}; } @@ -683,6 +684,10 @@ static std::unique_ptr BasicSchema() { return MakeSchema(Id(), Name(), Age(), Email()); } +static std::unique_ptr VariantSchema() { + return MakeSchema(Id(), Payload()); +} + static std::unique_ptr AddressSchema() { auto address_type = MakeStructType(Street(), City(), Zip()); auto address_field = iceberg::SchemaField{14, "address", std::move(address_type), true}; @@ -932,30 +937,36 @@ TEST_P(ProjectParamTest, ProjectFields) { INSTANTIATE_TEST_SUITE_P( ProjectTestCases, ProjectParamTest, - ::testing::Values(ProjectTestParam{.test_name = "ProjectAllFields", - .create_schema = []() { return BasicSchema(); }, - .selected_ids = {1, 2, 3, 4}, - .expected_schema = []() { return BasicSchema(); }, - .should_succeed = true}, - - ProjectTestParam{ - .test_name = "ProjectSingleField", - .create_schema = []() { return BasicSchema(); }, - .selected_ids = {2}, - .expected_schema = []() { return MakeSchema(Name()); }, - .should_succeed = true}, + ::testing::Values( + ProjectTestParam{.test_name = "ProjectAllFields", + .create_schema = []() { return BasicSchema(); }, + .selected_ids = {1, 2, 3, 4}, + .expected_schema = []() { return BasicSchema(); }, + .should_succeed = true}, + + ProjectTestParam{.test_name = "ProjectSingleField", + .create_schema = []() { return BasicSchema(); }, + .selected_ids = {2}, + .expected_schema = []() { return MakeSchema(Name()); }, + .should_succeed = true}, - ProjectTestParam{.test_name = "ProjectNonExistentFieldId", - .create_schema = []() { return BasicSchema(); }, - .selected_ids = {999}, - .expected_schema = []() { return MakeSchema(); }, - .should_succeed = true}, - - ProjectTestParam{.test_name = "ProjectEmptySelection", - .create_schema = []() { return BasicSchema(); }, - .selected_ids = {}, - .expected_schema = []() { return MakeSchema(); }, - .should_succeed = true})); + ProjectTestParam{.test_name = "ProjectVariantField", + .create_schema = []() { return VariantSchema(); }, + .selected_ids = {5}, + .expected_schema = []() { return MakeSchema(Payload()); }, + .should_succeed = true}, + + ProjectTestParam{.test_name = "ProjectNonExistentFieldId", + .create_schema = []() { return BasicSchema(); }, + .selected_ids = {999}, + .expected_schema = []() { return MakeSchema(); }, + .should_succeed = true}, + + ProjectTestParam{.test_name = "ProjectEmptySelection", + .create_schema = []() { return BasicSchema(); }, + .selected_ids = {}, + .expected_schema = []() { return MakeSchema(); }, + .should_succeed = true})); INSTANTIATE_TEST_SUITE_P(ProjectNestedTestCases, ProjectParamTest, ::testing::Values(ProjectTestParam{ diff --git a/src/iceberg/test/transform_test.cc b/src/iceberg/test/transform_test.cc index 8d7cd880d..d3eae5971 100644 --- a/src/iceberg/test/transform_test.cc +++ b/src/iceberg/test/transform_test.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -50,6 +51,18 @@ TEST(TransformTest, Transform) { ASSERT_TRUE(identity_transform); } +TEST(TransformTest, IdentityDoesNotSupportV3Types) { + const auto transform = Transform::Identity(); + const std::vector> unsupported_types = { + iceberg::variant(), iceberg::geometry(), iceberg::geography()}; + + for (const auto& type : unsupported_types) { + EXPECT_FALSE(transform->CanTransform(*type)); + EXPECT_THAT(transform->Bind(type), + HasErrorMessage("is not a valid input type for identity transform")); + } +} + TEST(TransformFunctionTest, CreateBucketTransform) { constexpr int32_t bucket_count = 8; auto transform = Transform::Bucket(bucket_count); diff --git a/src/iceberg/test/type_test.cc b/src/iceberg/test/type_test.cc index d405cccc1..ac188f229 100644 --- a/src/iceberg/test/type_test.cc +++ b/src/iceberg/test/type_test.cc @@ -38,6 +38,7 @@ struct TypeTestCase { std::shared_ptr type; iceberg::TypeId type_id; bool primitive; + bool nested = false; std::string repr; }; @@ -61,20 +62,31 @@ TEST_P(TypeTest, IsPrimitive) { const auto* primitive = dynamic_cast(test_case.type.get()); ASSERT_NE(nullptr, primitive); + } else { + ASSERT_FALSE(test_case.type->is_primitive()); } } TEST_P(TypeTest, IsNested) { const auto& test_case = GetParam(); - if (!test_case.primitive) { - ASSERT_FALSE(test_case.type->is_primitive()); + if (test_case.nested) { ASSERT_TRUE(test_case.type->is_nested()); const auto* nested = dynamic_cast(test_case.type.get()); ASSERT_NE(nullptr, nested); + } else { + ASSERT_FALSE(test_case.type->is_nested()); } } +TEST_P(TypeTest, TypeKindPredicates) { + const auto& test_case = GetParam(); + ASSERT_EQ(test_case.type_id == iceberg::TypeId::kStruct, test_case.type->is_struct()); + ASSERT_EQ(test_case.type_id == iceberg::TypeId::kList, test_case.type->is_list()); + ASSERT_EQ(test_case.type_id == iceberg::TypeId::kMap, test_case.type->is_map()); + ASSERT_EQ(test_case.type_id == iceberg::TypeId::kVariant, test_case.type->is_variant()); +} + TEST_P(TypeTest, ReflexiveEquality) { const auto& test_case = GetParam(); ASSERT_EQ(*test_case.type, *test_case.type); @@ -90,7 +102,7 @@ TEST_P(TypeTest, StdFormat) { ASSERT_EQ(test_case.repr, std::format("{}", *test_case.type)); } -const static std::array kPrimitiveTypes = {{ +const static std::array kPrimitiveTypes = {{ { .name = "boolean", .type = iceberg::boolean(), @@ -224,14 +236,37 @@ const static std::array kPrimitiveTypes = {{ .primitive = true, .repr = "unknown", }, + { + .name = "geometry", + .type = iceberg::geometry(), + .type_id = iceberg::TypeId::kGeometry, + .primitive = true, + .repr = "geometry", + }, + { + .name = "geography", + .type = iceberg::geography(), + .type_id = iceberg::TypeId::kGeography, + .primitive = true, + .repr = "geography", + }, }}; +const static TypeTestCase kVariantType = { + .name = "variant", + .type = iceberg::variant(), + .type_id = iceberg::TypeId::kVariant, + .primitive = false, + .repr = "variant", +}; + const static std::array kNestedTypes = {{ { .name = "list_int", .type = std::make_shared(1, iceberg::int32(), true), .type_id = iceberg::TypeId::kList, .primitive = false, + .nested = true, .repr = "list", }, { @@ -240,6 +275,7 @@ const static std::array kNestedTypes = {{ 1, std::make_shared(2, iceberg::int32(), true), false), .type_id = iceberg::TypeId::kList, .primitive = false, + .nested = true, .repr = "list (required)>", }, { @@ -249,6 +285,7 @@ const static std::array kNestedTypes = {{ iceberg::SchemaField::MakeRequired(2, "value", iceberg::string())), .type_id = iceberg::TypeId::kMap, .primitive = false, + .nested = true, .repr = "map", }, { @@ -259,6 +296,7 @@ const static std::array kNestedTypes = {{ }), .type_id = iceberg::TypeId::kStruct, .primitive = false, + .nested = true, .repr = R"(struct< foo (1): long (required) bar (2): string (optional) @@ -269,6 +307,9 @@ const static std::array kNestedTypes = {{ INSTANTIATE_TEST_SUITE_P(Primitive, TypeTest, ::testing::ValuesIn(kPrimitiveTypes), TypeTestCaseToString); +INSTANTIATE_TEST_SUITE_P(Variant, TypeTest, ::testing::Values(kVariantType), + TypeTestCaseToString); + INSTANTIATE_TEST_SUITE_P(Nested, TypeTest, ::testing::ValuesIn(kNestedTypes), TypeTestCaseToString); @@ -277,6 +318,7 @@ TEST(TypeTest, Equality) { for (const auto& test_case : kPrimitiveTypes) { alltypes.push_back(test_case.type); } + alltypes.push_back(kVariantType.type); for (const auto& test_case : kNestedTypes) { alltypes.push_back(test_case.type); } @@ -294,6 +336,33 @@ TEST(TypeTest, Equality) { } } +TEST(TypeTest, GeographyExplicitDefaultAlgorithm) { + ASSERT_NE(*iceberg::geography("srid:4326"), + *iceberg::geography("srid:4326", iceberg::EdgeAlgorithm::kSpherical)); + ASSERT_NE(*iceberg::geography(), + *iceberg::geography("OGC:CRS84", iceberg::EdgeAlgorithm::kSpherical)); + ASSERT_EQ( + "geography(srid:4326, spherical)", + iceberg::geography("srid:4326", iceberg::EdgeAlgorithm::kSpherical)->ToString()); + ASSERT_EQ( + "geography(OGC:CRS84, spherical)", + iceberg::geography("OGC:CRS84", iceberg::EdgeAlgorithm::kSpherical)->ToString()); + ASSERT_NE(*iceberg::geography("srid:4326"), + *iceberg::geography("srid:4326", iceberg::EdgeAlgorithm::kKarney)); +} + +TEST(TypeTest, GeometryMakeRejectsEmptyCrs) { + auto result = iceberg::GeometryType::Make(""); + ASSERT_THAT(result, IsError(iceberg::ErrorKind::kInvalidArgument)); + ASSERT_THAT(result, iceberg::HasErrorMessage("GeometryType: CRS cannot be empty")); +} + +TEST(TypeTest, GeographyMakeRejectsEmptyCrs) { + auto result = iceberg::GeographyType::Make(""); + ASSERT_THAT(result, IsError(iceberg::ErrorKind::kInvalidArgument)); + ASSERT_THAT(result, iceberg::HasErrorMessage("GeographyType: CRS cannot be empty")); +} + TEST(TypeTest, Decimal) { { iceberg::DecimalType decimal(38, 2); @@ -359,11 +428,17 @@ TEST(TypeTest, List) { } ASSERT_THAT( []() { - iceberg::ListType list( - iceberg::SchemaField(1, "wrongname", iceberg::boolean(), true)); + iceberg::list(iceberg::SchemaField(1, "wrongname", iceberg::boolean(), true)); }, ::testing::ThrowsMessage( ::testing::HasSubstr("child field name should be 'element', was 'wrongname'"))); + + auto make_result = iceberg::ListType::Make( + iceberg::SchemaField(1, "wrongname", iceberg::boolean(), true)); + ASSERT_THAT(make_result, IsError(iceberg::ErrorKind::kInvalidArgument)); + ASSERT_THAT(make_result, + iceberg::HasErrorMessage( + "ListType: child field name should be 'element', was 'wrongname'")); } TEST(TypeTest, Map) { @@ -397,7 +472,7 @@ TEST(TypeTest, Map) { []() { iceberg::SchemaField key(5, "notkey", iceberg::int32(), true); iceberg::SchemaField value(7, "value", iceberg::string(), true); - iceberg::MapType map(key, value); + iceberg::map(key, value); }, ::testing::ThrowsMessage( ::testing::HasSubstr("key field name should be 'key', was 'notkey'"))); @@ -405,10 +480,26 @@ TEST(TypeTest, Map) { []() { iceberg::SchemaField key(5, "key", iceberg::int32(), true); iceberg::SchemaField value(7, "notvalue", iceberg::string(), true); - iceberg::MapType map(key, value); + iceberg::map(key, value); }, ::testing::ThrowsMessage( ::testing::HasSubstr("value field name should be 'value', was 'notvalue'"))); + + auto invalid_key_result = + iceberg::MapType::Make(iceberg::SchemaField(5, "notkey", iceberg::int32(), true), + iceberg::SchemaField(7, "value", iceberg::string(), true)); + ASSERT_THAT(invalid_key_result, IsError(iceberg::ErrorKind::kInvalidArgument)); + ASSERT_THAT( + invalid_key_result, + iceberg::HasErrorMessage("MapType: key field name should be 'key', was 'notkey'")); + + auto invalid_value_result = iceberg::MapType::Make( + iceberg::SchemaField(5, "key", iceberg::int32(), true), + iceberg::SchemaField(7, "notvalue", iceberg::string(), true)); + ASSERT_THAT(invalid_value_result, IsError(iceberg::ErrorKind::kInvalidArgument)); + ASSERT_THAT(invalid_value_result, + iceberg::HasErrorMessage( + "MapType: value field name should be 'value', was 'notvalue'")); } TEST(TypeTest, Struct) { diff --git a/src/iceberg/test/visit_type_test.cc b/src/iceberg/test/visit_type_test.cc index f038f906f..a6bd9f8c6 100644 --- a/src/iceberg/test/visit_type_test.cc +++ b/src/iceberg/test/visit_type_test.cc @@ -46,6 +46,7 @@ struct TypeTestCase { std::shared_ptr type; iceberg::TypeId type_id; bool primitive; + bool nested = false; std::string repr; }; @@ -53,7 +54,7 @@ std::string TypeTestCaseToString(const ::testing::TestParamInfo& i return info.param.name; } -const static std::array kPrimitiveTypes = {{ +const static std::array kPrimitiveTypes = {{ { .name = "boolean", .type = iceberg::boolean(), @@ -187,14 +188,37 @@ const static std::array kPrimitiveTypes = {{ .primitive = true, .repr = "unknown", }, + { + .name = "geometry", + .type = iceberg::geometry(), + .type_id = iceberg::TypeId::kGeometry, + .primitive = true, + .repr = "geometry", + }, + { + .name = "geography", + .type = iceberg::geography(), + .type_id = iceberg::TypeId::kGeography, + .primitive = true, + .repr = "geography", + }, }}; +const static TypeTestCase kVariantType = { + .name = "variant", + .type = iceberg::variant(), + .type_id = iceberg::TypeId::kVariant, + .primitive = false, + .repr = "variant", +}; + const static std::array kNestedTypes = {{ { .name = "list_int", .type = std::make_shared(1, iceberg::int32(), true), .type_id = iceberg::TypeId::kList, .primitive = false, + .nested = true, .repr = "list", }, { @@ -203,6 +227,7 @@ const static std::array kNestedTypes = {{ 1, std::make_shared(2, iceberg::int32(), true), false), .type_id = iceberg::TypeId::kList, .primitive = false, + .nested = true, .repr = "list (required)>", }, { @@ -212,6 +237,7 @@ const static std::array kNestedTypes = {{ iceberg::SchemaField::MakeRequired(2, "value", iceberg::string())), .type_id = iceberg::TypeId::kMap, .primitive = false, + .nested = true, .repr = "map", }, { @@ -222,6 +248,7 @@ const static std::array kNestedTypes = {{ }), .type_id = iceberg::TypeId::kStruct, .primitive = false, + .nested = true, .repr = R"(struct< foo (1): long (required) bar (2): string (optional) @@ -236,6 +263,9 @@ class VisitTypeTest : public ::testing::TestWithParam {}; INSTANTIATE_TEST_SUITE_P(Primitive, VisitTypeTest, ::testing::ValuesIn(kPrimitiveTypes), TypeTestCaseToString); +INSTANTIATE_TEST_SUITE_P(Variant, VisitTypeTest, ::testing::Values(kVariantType), + TypeTestCaseToString); + INSTANTIATE_TEST_SUITE_P(Nested, VisitTypeTest, ::testing::ValuesIn(kNestedTypes), TypeTestCaseToString); @@ -261,12 +291,12 @@ TEST_P(VisitTypeTest, VisitTypeReturnNestedTypeId) { const auto& test_case = GetParam(); auto result = VisitType(*test_case.type, visitor); - if (test_case.primitive) { - ASSERT_THAT(result, IsError(ErrorKind::kNotImplemented)); - ASSERT_THAT(result, HasErrorMessage("Type is not a nested type")); - } else { + if (test_case.nested) { ASSERT_THAT(result, IsOk()); ASSERT_EQ(result.value(), test_case.type_id); + } else { + ASSERT_THAT(result, IsError(ErrorKind::kNotImplemented)); + ASSERT_THAT(result, HasErrorMessage("Type is not a nested type")); } } diff --git a/src/iceberg/transform.cc b/src/iceberg/transform.cc index 453941c95..f6d5c0c20 100644 --- a/src/iceberg/transform.cc +++ b/src/iceberg/transform.cc @@ -158,7 +158,9 @@ std::shared_ptr Transform::ResultType( bool Transform::CanTransform(const Type& source_type) const { switch (transform_type_) { case TransformType::kIdentity: - if (!source_type.is_primitive()) [[unlikely]] { + if (source_type.is_variant() || source_type.type_id() == TypeId::kGeometry || + source_type.type_id() == TypeId::kGeography || !source_type.is_primitive()) + [[unlikely]] { return false; } return true; diff --git a/src/iceberg/transform_function.cc b/src/iceberg/transform_function.cc index 4325c53d1..1e8e30bb5 100644 --- a/src/iceberg/transform_function.cc +++ b/src/iceberg/transform_function.cc @@ -40,7 +40,9 @@ std::shared_ptr IdentityTransform::ResultType() const { return source_type Result> IdentityTransform::Make( std::shared_ptr const& source_type) { - if (!source_type || !source_type->is_primitive()) { + if (!source_type || source_type->is_variant() || + source_type->type_id() == TypeId::kGeometry || + source_type->type_id() == TypeId::kGeography || !source_type->is_primitive()) { return NotSupported("{} is not a valid input type for identity transform", source_type ? source_type->ToString() : "null"); } diff --git a/src/iceberg/type.cc b/src/iceberg/type.cc index 057dcf513..fe48e6a99 100644 --- a/src/iceberg/type.cc +++ b/src/iceberg/type.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include "iceberg/exception.h" @@ -142,12 +143,16 @@ StructType::InitFieldByLowerCaseName(const StructType& self) { return field_by_lowercase_name; } -ListType::ListType(SchemaField element) : element_(std::move(element)) { - ICEBERG_CHECK_OR_DIE(element_.name() == kElementName, - "ListType: child field name should be '{}', was '{}'", - kElementName, element_.name()); +Result> ListType::Make(SchemaField element) { + if (element.name() != kElementName) { + return InvalidArgument("ListType: child field name should be '{}', was '{}'", + kElementName, element.name()); + } + return std::make_unique(std::move(element)); } +ListType::ListType(SchemaField element) : element_(std::move(element)) {} + ListType::ListType(int32_t field_id, std::shared_ptr type, bool optional) : element_(field_id, std::string(kElementName), std::move(type), optional) {} @@ -198,16 +203,21 @@ bool ListType::Equals(const Type& other) const { return element_ == list.element_; } -MapType::MapType(SchemaField key, SchemaField value) - : fields_{std::move(key), std::move(value)} { - ICEBERG_CHECK_OR_DIE(this->key().name() == kKeyName, - "MapType: key field name should be '{}', was '{}'", kKeyName, - this->key().name()); - ICEBERG_CHECK_OR_DIE(this->value().name() == kValueName, - "MapType: value field name should be '{}', was '{}'", kValueName, - this->value().name()); +Result> MapType::Make(SchemaField key, SchemaField value) { + if (key.name() != kKeyName) { + return InvalidArgument("MapType: key field name should be '{}', was '{}'", kKeyName, + key.name()); + } + if (value.name() != kValueName) { + return InvalidArgument("MapType: value field name should be '{}', was '{}'", + kValueName, value.name()); + } + return std::make_unique(std::move(key), std::move(value)); } +MapType::MapType(SchemaField key, SchemaField value) + : fields_{std::move(key), std::move(value)} {} + const SchemaField& MapType::key() const { return fields_[0]; } const SchemaField& MapType::value() const { return fields_[1]; } TypeId MapType::type_id() const { return kTypeId; } @@ -264,6 +274,10 @@ bool MapType::Equals(const Type& other) const { return fields_ == map.fields_; } +TypeId VariantType::type_id() const { return kTypeId; } +std::string VariantType::ToString() const { return "variant"; } +bool VariantType::Equals(const Type& other) const { return other.type_id() == kTypeId; } + TypeId BooleanType::type_id() const { return kTypeId; } std::string BooleanType::ToString() const { return "boolean"; } bool BooleanType::Equals(const Type& other) const { return other.type_id() == kTypeId; } @@ -354,6 +368,97 @@ TypeId UnknownType::type_id() const { return kTypeId; } std::string UnknownType::ToString() const { return "unknown"; } bool UnknownType::Equals(const Type& other) const { return other.type_id() == kTypeId; } +Result> GeometryType::Make() { + return std::unique_ptr(new GeometryType()); +} + +Result> GeometryType::Make(std::string crs) { + if (crs.empty()) { + return InvalidArgument("GeometryType: CRS cannot be empty"); + } + return std::unique_ptr(new GeometryType(std::move(crs))); +} + +GeometryType::GeometryType(std::string crs) { + if (StringUtils::ToLower(crs) != StringUtils::ToLower(kDefaultCrs)) { + crs_ = std::move(crs); + } +} + +std::string_view GeometryType::crs() const { + return crs_.empty() ? kDefaultCrs : std::string_view(crs_); +} +TypeId GeometryType::type_id() const { return kTypeId; } +std::string GeometryType::ToString() const { + if (crs_.empty()) { + return "geometry"; + } + return std::format("geometry({})", crs_); +} +bool GeometryType::Equals(const Type& other) const { + if (other.type_id() != kTypeId) { + return false; + } + const auto& geometry = static_cast(other); + return crs_ == geometry.crs_; +} + +Result> GeographyType::Make() { + return std::unique_ptr(new GeographyType()); +} + +Result> GeographyType::Make(std::string crs) { + if (crs.empty()) { + return InvalidArgument("GeographyType: CRS cannot be empty"); + } + return std::unique_ptr(new GeographyType(std::move(crs))); +} + +Result> GeographyType::Make(std::string crs, + EdgeAlgorithm algorithm) { + if (crs.empty()) { + return InvalidArgument("GeographyType: CRS cannot be empty"); + } + return std::unique_ptr(new GeographyType(std::move(crs), algorithm)); +} + +GeographyType::GeographyType(std::string crs) { + if (StringUtils::ToLower(crs) != StringUtils::ToLower(kDefaultCrs)) { + crs_ = std::move(crs); + } +} + +GeographyType::GeographyType(std::string crs, EdgeAlgorithm algorithm) + : algorithm_(algorithm) { + if (StringUtils::ToLower(crs) != StringUtils::ToLower(kDefaultCrs)) { + crs_ = std::move(crs); + } +} + +std::string_view GeographyType::crs() const { + return crs_.empty() ? kDefaultCrs : std::string_view(crs_); +} +EdgeAlgorithm GeographyType::algorithm() const { + return algorithm_.value_or(kDefaultAlgorithm); +} +TypeId GeographyType::type_id() const { return kTypeId; } +std::string GeographyType::ToString() const { + if (algorithm_.has_value()) { + return std::format("geography({}, {})", crs(), iceberg::ToString(*algorithm_)); + } + if (!crs_.empty()) { + return std::format("geography({})", crs_); + } + return "geography"; +} +bool GeographyType::Equals(const Type& other) const { + if (other.type_id() != kTypeId) { + return false; + } + const auto& geography = static_cast(other); + return crs_ == geography.crs_ && algorithm_ == geography.algorithm_; +} + FixedType::FixedType(int32_t length) : length_(length) { ICEBERG_CHECK_OR_DIE(length >= 0, "FixedType: length must be >= 0, was {}", length); } @@ -374,7 +479,7 @@ std::string BinaryType::ToString() const { return "binary"; } bool BinaryType::Equals(const Type& other) const { return other.type_id() == kTypeId; } // ---------------------------------------------------------------------- -// Factory functions for creating primitive data types +// Factory functions for creating data types #define TYPE_FACTORY(NAME, KLASS) \ const std::shared_ptr& NAME() { \ @@ -397,9 +502,28 @@ TYPE_FACTORY(binary, BinaryType) TYPE_FACTORY(string, StringType) TYPE_FACTORY(uuid, UuidType) TYPE_FACTORY(unknown, UnknownType) +TYPE_FACTORY(variant, VariantType) #undef TYPE_FACTORY +const std::shared_ptr& geometry() { + static const std::shared_ptr result = [] { + auto type = GeometryType::Make(); + ICEBERG_CHECK_OR_DIE(type.has_value(), "Failed to create default geometry type"); + return std::shared_ptr(std::move(type.value())); + }(); + return result; +} + +const std::shared_ptr& geography() { + static const std::shared_ptr result = [] { + auto type = GeographyType::Make(); + ICEBERG_CHECK_OR_DIE(type.has_value(), "Failed to create default geography type"); + return std::shared_ptr(std::move(type.value())); + }(); + return result; +} + std::shared_ptr decimal(int32_t precision, int32_t scale) { return std::make_shared(precision, scale); } @@ -408,12 +532,44 @@ std::shared_ptr fixed(int32_t length) { return std::make_shared(length); } +std::shared_ptr geometry(std::string crs) { + auto type = GeometryType::Make(std::move(crs)); + if (!type.has_value()) { + throw IcebergError(type.error().message); + } + return std::move(type.value()); +} + +std::shared_ptr geography(std::string crs) { + auto type = GeographyType::Make(std::move(crs)); + if (!type.has_value()) { + throw IcebergError(type.error().message); + } + return std::move(type.value()); +} + +std::shared_ptr geography(std::string crs, EdgeAlgorithm algorithm) { + auto type = GeographyType::Make(std::move(crs), algorithm); + if (!type.has_value()) { + throw IcebergError(type.error().message); + } + return std::move(type.value()); +} + std::shared_ptr map(SchemaField key, SchemaField value) { - return std::make_shared(key, value); + auto type = MapType::Make(std::move(key), std::move(value)); + if (!type.has_value()) { + throw IcebergError(type.error().message); + } + return std::move(type.value()); } std::shared_ptr list(SchemaField element) { - return std::make_shared(std::move(element)); + auto type = ListType::Make(std::move(element)); + if (!type.has_value()) { + throw IcebergError(type.error().message); + } + return std::move(type.value()); } std::shared_ptr struct_(std::vector fields) { @@ -462,9 +618,52 @@ std::string_view ToString(TypeId id) { return "binary"; case TypeId::kUnknown: return "unknown"; + case TypeId::kVariant: + return "variant"; + case TypeId::kGeometry: + return "geometry"; + case TypeId::kGeography: + return "geography"; + } + + std::unreachable(); +} + +std::string_view ToString(EdgeAlgorithm algorithm) { + switch (algorithm) { + case EdgeAlgorithm::kSpherical: + return "spherical"; + case EdgeAlgorithm::kVincenty: + return "vincenty"; + case EdgeAlgorithm::kThomas: + return "thomas"; + case EdgeAlgorithm::kAndoyer: + return "andoyer"; + case EdgeAlgorithm::kKarney: + return "karney"; } std::unreachable(); } +Result EdgeAlgorithmFromString(std::string_view name) { + auto lower_name = StringUtils::ToLower(name); + if (lower_name == "spherical") { + return EdgeAlgorithm::kSpherical; + } + if (lower_name == "vincenty") { + return EdgeAlgorithm::kVincenty; + } + if (lower_name == "thomas") { + return EdgeAlgorithm::kThomas; + } + if (lower_name == "andoyer") { + return EdgeAlgorithm::kAndoyer; + } + if (lower_name == "karney") { + return EdgeAlgorithm::kKarney; + } + return InvalidArgument("Invalid edge interpolation algorithm: {}", name); +} + } // namespace iceberg diff --git a/src/iceberg/type.h b/src/iceberg/type.h index c0966759e..41484333d 100644 --- a/src/iceberg/type.h +++ b/src/iceberg/type.h @@ -46,20 +46,32 @@ class ICEBERG_EXPORT Type : public iceberg::util::Formattable { ~Type() override = default; /// \brief Get the type ID. - [[nodiscard]] virtual TypeId type_id() const = 0; + virtual TypeId type_id() const = 0; /// \brief Is this a primitive type (may not have child fields)? - [[nodiscard]] virtual bool is_primitive() const = 0; + virtual bool is_primitive() const = 0; /// \brief Is this a nested type (may have child fields)? - [[nodiscard]] virtual bool is_nested() const = 0; + virtual bool is_nested() const = 0; + + /// \brief Is this a struct type? + bool is_struct() const { return type_id() == TypeId::kStruct; } + + /// \brief Is this a list type? + bool is_list() const { return type_id() == TypeId::kList; } + + /// \brief Is this a map type? + bool is_map() const { return type_id() == TypeId::kMap; } + + /// \brief Is this a variant type? + bool is_variant() const { return type_id() == TypeId::kVariant; } /// \brief Compare two types for equality. friend bool operator==(const Type& lhs, const Type& rhs) { return lhs.Equals(rhs); } protected: /// \brief Compare two types for equality. - [[nodiscard]] virtual bool Equals(const Type& other) const = 0; + virtual bool Equals(const Type& other) const = 0; }; /// \brief A data type that does not have child fields. @@ -76,28 +88,27 @@ class ICEBERG_EXPORT NestedType : public Type { bool is_nested() const override { return true; } /// \brief Get a view of the child fields. - [[nodiscard]] virtual std::span fields() const = 0; + virtual std::span fields() const = 0; using SchemaFieldConstRef = std::reference_wrapper; /// \brief Get a field by field ID. /// /// \note This is O(1) complexity. - [[nodiscard]] virtual Result> GetFieldById( + virtual Result> GetFieldById( int32_t field_id) const = 0; /// \brief Get a field by index. /// /// \note This is O(1) complexity. - [[nodiscard]] virtual Result> GetFieldByIndex( + virtual Result> GetFieldByIndex( int32_t index) const = 0; /// \brief Get a field by name. Return an error Status if /// the field name is not unique; prefer GetFieldById or GetFieldByIndex /// when possible. /// /// \note This is O(1) complexity. - [[nodiscard]] virtual Result> GetFieldByName( + virtual Result> GetFieldByName( std::string_view name, bool case_sensitive) const = 0; /// \brief Get a field by name (case-sensitive). - [[nodiscard]] Result> GetFieldByName( - std::string_view name) const; + Result> GetFieldByName(std::string_view name) const; }; /// \defgroup type-nested Nested Types @@ -147,8 +158,11 @@ class ICEBERG_EXPORT ListType : public NestedType { constexpr static const TypeId kTypeId = TypeId::kList; constexpr static const std::string_view kElementName = "element"; - /// \brief Construct a list of the given element. The name of the child - /// field should be "element". + static Result> Make(SchemaField element); + + /// \brief Construct a list of the given element. + /// + /// Use Make or list to validate that the element field name is "element". explicit ListType(SchemaField element); /// \brief Construct a list of the given element type. ListType(int32_t field_id, std::shared_ptr type, bool optional); @@ -180,8 +194,11 @@ class ICEBERG_EXPORT MapType : public NestedType { constexpr static const std::string_view kKeyName = "key"; constexpr static const std::string_view kValueName = "value"; - /// \brief Construct a map of the given key/value fields. The field names - /// should be "key" and "value", respectively. + static Result> Make(SchemaField key, SchemaField value); + + /// \brief Construct a map of the given key/value fields. + /// + /// Use Make or map to validate that the field names are "key" and "value". explicit MapType(SchemaField key, SchemaField value); ~MapType() override = default; @@ -208,6 +225,30 @@ class ICEBERG_EXPORT MapType : public NestedType { /// @} +/// \defgroup type-semi-structured Semi-structured Types +/// Semi-structured types may contain values whose structure varies across rows. +/// @{ + +/// \brief A semi-structured type whose structure may vary across rows. +class ICEBERG_EXPORT VariantType : public Type { + public: + constexpr static const TypeId kTypeId = TypeId::kVariant; + + VariantType() = default; + ~VariantType() override = default; + + bool is_primitive() const override { return false; } + bool is_nested() const override { return false; } + + TypeId type_id() const override; + std::string ToString() const override; + + protected: + bool Equals(const Type& other) const override; +}; + +/// @} + /// \defgroup type-primitive Primitive Types /// Primitive types do not have nested fields. /// @{ @@ -296,14 +337,15 @@ class ICEBERG_EXPORT DecimalType : public PrimitiveType { constexpr static const int32_t kMaxPrecision = 38; /// \brief Construct a decimal type with the given precision and scale. + /// \throws IcebergError if precision is outside the supported range. DecimalType(int32_t precision, int32_t scale); ~DecimalType() override = default; /// \brief Get the precision (the number of decimal digits). - [[nodiscard]] int32_t precision() const; + int32_t precision() const; /// \brief Get the scale (essentially, the number of decimal digits after /// the decimal point; precisely, the value is scaled by $$10^{-s}$$.). - [[nodiscard]] int32_t scale() const; + int32_t scale() const; TypeId type_id() const override; std::string ToString() const override; @@ -353,9 +395,9 @@ class ICEBERG_EXPORT TimeType : public PrimitiveType { class ICEBERG_EXPORT TimestampBase : public PrimitiveType { public: /// \brief Is this type zoned or naive? - [[nodiscard]] virtual bool is_zoned() const = 0; + virtual bool is_zoned() const = 0; /// \brief The time resolution. - [[nodiscard]] virtual TimeUnit time_unit() const = 0; + virtual TimeUnit time_unit() const = 0; }; /// \brief A data type representing a timestamp in microseconds without @@ -471,11 +513,12 @@ class ICEBERG_EXPORT FixedType : public PrimitiveType { constexpr static const TypeId kTypeId = TypeId::kFixed; /// \brief Construct a fixed type with the given length. + /// \throws IcebergError if length is negative. explicit FixedType(int32_t length); ~FixedType() override = default; /// \brief The length (the number of bytes to store). - [[nodiscard]] int32_t length() const; + int32_t length() const; TypeId type_id() const override; std::string ToString() const override; @@ -518,11 +561,67 @@ class ICEBERG_EXPORT UnknownType : public PrimitiveType { bool Equals(const Type& other) const override; }; +/// \brief A data type representing OGC geometry in WKB format. +class ICEBERG_EXPORT GeometryType : public PrimitiveType { + public: + constexpr static const TypeId kTypeId = TypeId::kGeometry; + constexpr static std::string_view kDefaultCrs = "OGC:CRS84"; + + static Result> Make(); + static Result> Make(std::string crs); + ~GeometryType() override = default; + + std::string_view crs() const; + + TypeId type_id() const override; + std::string ToString() const override; + + protected: + bool Equals(const Type& other) const override; + + private: + GeometryType() = default; + explicit GeometryType(std::string crs); + + std::string crs_; +}; + +/// \brief A data type representing OGC geography in WKB format. +class ICEBERG_EXPORT GeographyType : public PrimitiveType { + public: + constexpr static const TypeId kTypeId = TypeId::kGeography; + constexpr static std::string_view kDefaultCrs = "OGC:CRS84"; + constexpr static EdgeAlgorithm kDefaultAlgorithm = EdgeAlgorithm::kSpherical; + + static Result> Make(); + static Result> Make(std::string crs); + static Result> Make(std::string crs, + EdgeAlgorithm algorithm); + ~GeographyType() override = default; + + std::string_view crs() const; + EdgeAlgorithm algorithm() const; + + TypeId type_id() const override; + std::string ToString() const override; + + protected: + bool Equals(const Type& other) const override; + + private: + GeographyType() = default; + explicit GeographyType(std::string crs); + GeographyType(std::string crs, EdgeAlgorithm algorithm); + + std::string crs_; + std::optional algorithm_; +}; + /// @} -/// \defgroup type-factories Factory functions for creating primitive data types +/// \defgroup type-factories Factory functions for creating data types /// -/// Factory functions for creating primitive data types +/// Factory functions for creating data types /// @{ /// \brief Return a BooleanType instance. @@ -555,18 +654,39 @@ ICEBERG_EXPORT const std::shared_ptr& string(); ICEBERG_EXPORT const std::shared_ptr& uuid(); /// \brief Return an UnknownType instance. ICEBERG_EXPORT const std::shared_ptr& unknown(); +/// \brief Return a VariantType instance. +ICEBERG_EXPORT const std::shared_ptr& variant(); +/// \brief Return the default GeometryType instance. +ICEBERG_EXPORT const std::shared_ptr& geometry(); +/// \brief Return the default GeographyType instance. +ICEBERG_EXPORT const std::shared_ptr& geography(); /// \brief Create a DecimalType with the given precision and scale. /// \param precision The number of decimal digits (max 38). /// \param scale The number of decimal digits after the decimal point. /// \return A shared pointer to the DecimalType instance. +/// \throws IcebergError if precision is outside the supported range. ICEBERG_EXPORT std::shared_ptr decimal(int32_t precision, int32_t scale); /// \brief Create a FixedType with the given length. /// \param length The number of bytes to store (must be >= 0). /// \return A shared pointer to the FixedType instance. +/// \throws IcebergError if length is negative. ICEBERG_EXPORT std::shared_ptr fixed(int32_t length); +/// \brief Create a GeometryType with the given CRS. +/// \throws IcebergError if crs is empty. +ICEBERG_EXPORT std::shared_ptr geometry(std::string crs); + +/// \brief Create a GeographyType with the given CRS. +/// \throws IcebergError if crs is empty. +ICEBERG_EXPORT std::shared_ptr geography(std::string crs); + +/// \brief Create a GeographyType with the given CRS and edge algorithm. +/// \throws IcebergError if crs is empty. +ICEBERG_EXPORT std::shared_ptr geography(std::string crs, + EdgeAlgorithm algorithm); + /// \brief Create a StructType with the given fields. /// \param fields The fields of the struct. /// \return A shared pointer to the StructType instance. @@ -575,12 +695,14 @@ ICEBERG_EXPORT std::shared_ptr struct_(std::vector fiel /// \brief Create a ListType with the given element field. /// \param element The element field of the list. /// \return A shared pointer to the ListType instance. +/// \throws IcebergError if element's name is not "element". ICEBERG_EXPORT std::shared_ptr list(SchemaField element); /// \brief Create a MapType with the given key and value fields. /// \param key The key field of the map. /// \param value The value field of the map. /// \return A shared pointer to the MapType instance. +/// \throws IcebergError if the key or value field has an invalid name. ICEBERG_EXPORT std::shared_ptr map(SchemaField key, SchemaField value); /// @} @@ -594,4 +716,10 @@ ICEBERG_EXPORT std::shared_ptr map(SchemaField key, SchemaField value); /// \return A string_view containing the lowercase type name ICEBERG_EXPORT std::string_view ToString(TypeId id); +/// \brief Get the lowercase string representation of an EdgeAlgorithm. +ICEBERG_EXPORT std::string_view ToString(EdgeAlgorithm algorithm); + +/// \brief Parse a lowercase edge algorithm name. +ICEBERG_EXPORT Result EdgeAlgorithmFromString(std::string_view name); + } // namespace iceberg diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index 6c34d3a8d..0320f24ea 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -53,6 +53,9 @@ enum class TypeId { kFixed, kBinary, kUnknown, + kVariant, + kGeometry, + kGeography, }; /// \brief The time unit. In Iceberg V3 nanoseconds are also supported. @@ -61,6 +64,15 @@ enum class TimeUnit { kNanosecond, }; +/// \brief The algorithm used to interpolate geography edges. +enum class EdgeAlgorithm { + kSpherical, + kVincenty, + kThomas, + kAndoyer, + kKarney, +}; + /// \brief Data type family. class BinaryType; class BooleanType; @@ -86,6 +98,9 @@ class TimestampTzNsType; class Type; class UnknownType; class UuidType; +class VariantType; +class GeographyType; +class GeometryType; /// \brief Data values. class Decimal; diff --git a/src/iceberg/update/update_schema.cc b/src/iceberg/update/update_schema.cc index 0e7f147b0..5c50ee41d 100644 --- a/src/iceberg/update/update_schema.cc +++ b/src/iceberg/update/update_schema.cc @@ -181,6 +181,12 @@ class ApplyChangesVisitor { return base_type; } + Result> VisitVariant(const VariantType& variant_type, + const std::shared_ptr& base_type, + int32_t parent_id) { + return base_type; + } + private: Result> ProcessField( const SchemaField& field, const std::shared_ptr& field_type_result) { diff --git a/src/iceberg/util/struct_like_set.cc b/src/iceberg/util/struct_like_set.cc index 12648ea5e..35a0f9e28 100644 --- a/src/iceberg/util/struct_like_set.cc +++ b/src/iceberg/util/struct_like_set.cc @@ -342,6 +342,11 @@ Status ValidateScalarAgainstType(const Scalar& scalar, const Type& type) { return ValidateMapLikeAgainstType(*map, internal::checked_cast(type)); } + case TypeId::kVariant: + case TypeId::kGeometry: + case TypeId::kGeography: + return NotSupported("Scalar validation for type {} is not supported", + type.ToString()); } std::unreachable(); diff --git a/src/iceberg/util/type_util.cc b/src/iceberg/util/type_util.cc index cb01be08f..1f1e02747 100644 --- a/src/iceberg/util/type_util.cc +++ b/src/iceberg/util/type_util.cc @@ -37,6 +37,8 @@ IdToFieldVisitor::IdToFieldVisitor( Status IdToFieldVisitor::Visit(const PrimitiveType& type) { return {}; } +Status IdToFieldVisitor::Visit(const VariantType& type) { return {}; } + Status IdToFieldVisitor::Visit(const NestedType& type) { const auto& nested = internal::checked_cast(type); const auto& fields = nested.fields(); @@ -64,7 +66,7 @@ Status NameToIdVisitor::Visit(const ListType& type, const std::string& path, const auto& field = type.fields()[0]; std::string new_path = BuildPath(path, field.name(), case_sensitive_); std::string new_short_path; - if (field.type()->type_id() == TypeId::kStruct) { + if (field.type()->is_struct()) { new_short_path = short_path; } else { new_short_path = BuildPath(short_path, field.name(), case_sensitive_); @@ -86,8 +88,7 @@ Status NameToIdVisitor::Visit(const MapType& type, const std::string& path, const auto& fields = type.fields(); for (const auto& field : fields) { new_path = BuildPath(path, field.name(), case_sensitive_); - if (field.name() == MapType::kValueName && - field.type()->type_id() == TypeId::kStruct) { + if (field.name() == MapType::kValueName && field.type()->is_struct()) { new_short_path = short_path; } else { new_short_path = BuildPath(short_path, field.name(), case_sensitive_); @@ -128,6 +129,11 @@ Status NameToIdVisitor::Visit(const PrimitiveType& type, const std::string& path return {}; } +Status NameToIdVisitor::Visit(const VariantType& type, const std::string& path, + const std::string& short_path) { + return {}; +} + std::string NameToIdVisitor::BuildPath(std::string_view prefix, std::string_view field_name, bool case_sensitive) { std::string quoted_name; @@ -168,6 +174,20 @@ Status PositionPathVisitor::Visit(const PrimitiveType& type) { return {}; } +Status PositionPathVisitor::Visit(const VariantType& type) { + if (current_field_id_ == kUnassignedFieldId) { + return InvalidSchema("Current field id is not assigned, type: {}", type.ToString()); + } + + if (auto ret = position_path_.try_emplace(current_field_id_, current_path_); + !ret.second) { + return InvalidSchema("Duplicate field id found: {}, prev path: {}, curr path: {}", + current_field_id_, ret.first->second, current_path_); + } + + return {}; +} + Status PositionPathVisitor::Visit(const StructType& type) { for (size_t i = 0; i < type.fields().size(); ++i) { const auto& field = type.fields()[i]; @@ -208,8 +228,8 @@ Result> PruneColumnVisitor::Visit( Result> PruneColumnVisitor::Visit(const SchemaField& field) const { if (selected_ids_.contains(field.field_id())) { - return (select_full_types_ || field.type()->is_primitive()) ? field.type() - : Visit(field.type()); + return (select_full_types_ || !field.type()->is_nested()) ? field.type() + : Visit(field.type()); } return Visit(field.type()); } @@ -278,6 +298,8 @@ GetProjectedIdsVisitor::GetProjectedIdsVisitor(bool include_struct_ids) Status GetProjectedIdsVisitor::Visit(const Type& type) { if (type.is_nested()) { return VisitNested(internal::checked_cast(type)); + } else if (type.is_variant()) { + return {}; } else { return VisitPrimitive(internal::checked_cast(type)); } @@ -288,9 +310,8 @@ Status GetProjectedIdsVisitor::VisitNested(const NestedType& type) { ICEBERG_RETURN_UNEXPECTED(Visit(*field.type())); } for (auto& field : type.fields()) { - // TODO(zhuo.wang) or is_variant - if ((include_struct_ids_ && field.type()->type_id() == TypeId::kStruct) || - field.type()->is_primitive()) { + if ((include_struct_ids_ && field.type()->is_struct()) || + !field.type()->is_nested()) { ids_.insert(field.field_id()); } } diff --git a/src/iceberg/util/type_util.h b/src/iceberg/util/type_util.h index 8fd5ef19f..8623ad254 100644 --- a/src/iceberg/util/type_util.h +++ b/src/iceberg/util/type_util.h @@ -45,6 +45,7 @@ class IdToFieldVisitor { std::unordered_map>& id_to_field); Status Visit(const PrimitiveType& type); + Status Visit(const VariantType& type); Status Visit(const NestedType& type); private: @@ -67,6 +68,8 @@ class NameToIdVisitor { const std::string& short_path); Status Visit(const PrimitiveType& type, const std::string& path, const std::string& short_path); + Status Visit(const VariantType& type, const std::string& path, + const std::string& short_path); void Finish(); private: @@ -85,6 +88,7 @@ class NameToIdVisitor { class PositionPathVisitor { public: Status Visit(const PrimitiveType& type); + Status Visit(const VariantType& type); Status Visit(const StructType& type); Status Visit(const ListType& type); Status Visit(const MapType& type); diff --git a/src/iceberg/util/visit_type.h b/src/iceberg/util/visit_type.h index bf52d2e9a..73fbbb5fd 100644 --- a/src/iceberg/util/visit_type.h +++ b/src/iceberg/util/visit_type.h @@ -127,21 +127,24 @@ inline Status VisitTypeIdInline(TypeId id, VISITOR* visitor, ARGS&&... args) { /// \brief Visit a type using a categorical visitor pattern /// /// This function provides a simplified visitor interface that groups Iceberg types into -/// four categories based on their structural properties: +/// five categories based on their structural properties: /// /// - **Struct types**: Complex types with named fields (StructType) /// - **List types**: Sequential container types (ListType) /// - **Map types**: Key-value container types (MapType) -/// - **Primitive types**: All leaf types without nested structure (14 primitive types) +/// - **Variant type**: Semi-structured type that is neither nested nor primitive +/// (VariantType) +/// - **Primitive types**: All leaf types without nested structure (primitive types) /// /// This grouping is useful for algorithms that need to distinguish between container /// types and leaf types, but don't require separate handling for each primitive type /// variant (e.g., Int vs Long vs String). /// -/// \tparam VISITOR Visitor class that must implement four Visit methods: +/// \tparam VISITOR Visitor class that must implement five Visit methods: /// - `VisitStruct(const StructType&, ARGS...)` for struct types /// - `VisitList(const ListType&, ARGS...)` for list types /// - `VisitMap(const MapType&, ARGS...)` for map types +/// - `VisitVariant(const VariantType&, ARGS...)` for the variant type /// - `VisitPrimitive(const PrimitiveType&, ARGS...)` for all primitive types /// \tparam ARGS Additional argument types forwarded to Visit methods /// \param type The type to visit diff --git a/src/iceberg/util/visitor_generate.h b/src/iceberg/util/visitor_generate.h index a5b0c2ced..ad6f5eb2d 100644 --- a/src/iceberg/util/visitor_generate.h +++ b/src/iceberg/util/visitor_generate.h @@ -39,6 +39,9 @@ namespace iceberg { ACTION(Fixed); \ ACTION(Binary); \ ACTION(Unknown); \ + ACTION(Variant); \ + ACTION(Geometry); \ + ACTION(Geography); \ ACTION(Struct); \ ACTION(List); \ ACTION(Map); @@ -49,7 +52,12 @@ namespace iceberg { /// - Struct types -> calls ACTION with Struct /// - List types -> calls ACTION with List /// - Map types -> calls ACTION with Map +/// - Variant type -> calls ACTION with Variant /// - All primitive types (default) -> calls ACTION with Primitive +/// +/// Variant is dispatched explicitly because it is neither a nested nor a primitive +/// type, so it must not be routed into the primitive default (which would cast it to +/// PrimitiveType). #define ICEBERG_TYPE_SWITCH_WITH_PRIMITIVE_DEFAULT(ACTION) \ case ::iceberg::TypeId::kStruct: \ ACTION(Struct) \ @@ -57,6 +65,8 @@ namespace iceberg { ACTION(List) \ case ::iceberg::TypeId::kMap: \ ACTION(Map) \ + case ::iceberg::TypeId::kVariant: \ + ACTION(Variant) \ default: \ ACTION(Primitive) From 5ac524624552c37faca652f9412ae89be3aa20ed Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Tue, 23 Jun 2026 13:32:46 +0800 Subject: [PATCH 11/16] fix: allow historical sort orders with dropped fields (#762) This fix is aligned with https://github.com/apache/iceberg/pull/16521 --- src/iceberg/json_serde.cc | 44 ++++++++++++----- src/iceberg/test/metadata_serde_test.cc | 63 +++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 13 deletions(-) diff --git a/src/iceberg/json_serde.cc b/src/iceberg/json_serde.cc index e137aed1d..a4810d88f 100644 --- a/src/iceberg/json_serde.cc +++ b/src/iceberg/json_serde.cc @@ -273,8 +273,14 @@ Result> SortFieldFromJson(const nlohmann::json& json) null_order); } -Result> SortOrderFromJson( - const nlohmann::json& json, const std::shared_ptr& current_schema) { +namespace { + +struct ParsedSortOrder { + int32_t order_id; + std::vector fields; +}; + +Result ParseSortOrder(const nlohmann::json& json) { ICEBERG_ASSIGN_OR_RAISE(auto order_id, GetJsonValue(json, kOrderId)); ICEBERG_ASSIGN_OR_RAISE(auto fields, GetJsonValue(json, kFields)); @@ -283,19 +289,30 @@ Result> SortOrderFromJson( ICEBERG_ASSIGN_OR_RAISE(auto sort_field, SortFieldFromJson(field_json)); sort_fields.push_back(std::move(*sort_field)); } - return SortOrder::Make(*current_schema, order_id, std::move(sort_fields)); + return ParsedSortOrder{.order_id = order_id, .fields = std::move(sort_fields)}; } -Result> SortOrderFromJson(const nlohmann::json& json) { - ICEBERG_ASSIGN_OR_RAISE(auto order_id, GetJsonValue(json, kOrderId)); - ICEBERG_ASSIGN_OR_RAISE(auto fields, GetJsonValue(json, kFields)); +} // namespace - std::vector sort_fields; - for (const auto& field_json : fields) { - ICEBERG_ASSIGN_OR_RAISE(auto sort_field, SortFieldFromJson(field_json)); - sort_fields.push_back(std::move(*sort_field)); +Result> SortOrderFromJson( + const nlohmann::json& json, const std::shared_ptr& current_schema, + int32_t default_sort_order_id) { + ICEBERG_ASSIGN_OR_RAISE(auto parsed, ParseSortOrder(json)); + if (parsed.order_id == default_sort_order_id) { + return SortOrder::Make(*current_schema, parsed.order_id, std::move(parsed.fields)); } - return SortOrder::Make(order_id, std::move(sort_fields)); + return SortOrder::Make(parsed.order_id, std::move(parsed.fields)); +} + +Result> SortOrderFromJson( + const nlohmann::json& json, const std::shared_ptr& current_schema) { + ICEBERG_ASSIGN_OR_RAISE(auto parsed, ParseSortOrder(json)); + return SortOrder::Make(*current_schema, parsed.order_id, std::move(parsed.fields)); +} + +Result> SortOrderFromJson(const nlohmann::json& json) { + ICEBERG_ASSIGN_OR_RAISE(auto parsed, ParseSortOrder(json)); + return SortOrder::Make(parsed.order_id, std::move(parsed.fields)); } nlohmann::json ToJson(const SchemaField& field) { @@ -1161,8 +1178,9 @@ Status ParseSortOrders(const nlohmann::json& json, int8_t format_version, ICEBERG_ASSIGN_OR_RAISE(auto sort_order_array, GetJsonValue(json, kSortOrders)); for (const auto& sort_order_json : sort_order_array) { - ICEBERG_ASSIGN_OR_RAISE(auto sort_order, - SortOrderFromJson(sort_order_json, current_schema)); + ICEBERG_ASSIGN_OR_RAISE( + auto sort_order, + SortOrderFromJson(sort_order_json, current_schema, default_sort_order_id)); sort_orders.push_back(std::move(sort_order)); } } else { diff --git a/src/iceberg/test/metadata_serde_test.cc b/src/iceberg/test/metadata_serde_test.cc index 48f88f2bc..ba47f312d 100644 --- a/src/iceberg/test/metadata_serde_test.cc +++ b/src/iceberg/test/metadata_serde_test.cc @@ -89,6 +89,50 @@ void AssertSnapshotById(const TableMetadata& metadata, int64_t snapshot_id, EXPECT_EQ(*snapshot.value(), expected_snapshot); } +nlohmann::json HistoricalSortOrderWithDroppedFieldMetadataJson( + int32_t default_sort_order_id) { + nlohmann::json metadata_json = R"({ + "format-version": 2, + "table-uuid": "test-uuid-1234", + "location": "s3://bucket/test", + "last-sequence-number": 0, + "last-updated-ms": 0, + "last-column-id": 2, + "schemas": [ + { + "type": "struct", + "schema-id": 1, + "fields": [ + {"id": 1, "name": "id", "type": "int", "required": true} + ] + } + ], + "current-schema-id": 1, + "partition-specs": [{"spec-id": 0, "fields": []}], + "default-spec-id": 0, + "last-partition-id": 999, + "sort-orders": [ + {"order-id": 1, "fields": [ + {"transform": "identity", "source-id": 1, "direction": "asc", "null-order": "nulls-first"}, + {"transform": "identity", "source-id": 2, "direction": "asc", "null-order": "nulls-first"} + ]}, + {"order-id": 2, "fields": [ + {"transform": "identity", "source-id": 1, "direction": "asc", "null-order": "nulls-first"} + ]} + ], + "properties": {}, + "current-snapshot-id": null, + "refs": {}, + "snapshots": [], + "statistics": [], + "partition-statistics": [], + "snapshot-log": [], + "metadata-log": [] + })"_json; + metadata_json["default-sort-order-id"] = default_sort_order_id; + return metadata_json; +} + } // namespace TEST(MetadataSerdeTest, DeserializeV1Valid) { @@ -486,6 +530,25 @@ TEST(MetadataSerdeTest, DeserializeV2MissingSortOrder) { "sort-orders must exist"); } +TEST(MetadataSerdeTest, DeserializeHistoricalSortOrderWithDroppedField) { + auto metadata = + TableMetadataFromJson(HistoricalSortOrderWithDroppedFieldMetadataJson(2)); + ASSERT_THAT(metadata, IsOk()); + ASSERT_EQ(metadata.value()->sort_orders.size(), 2); + EXPECT_EQ(metadata.value()->sort_orders[0]->order_id(), 1); + ASSERT_EQ(metadata.value()->sort_orders[0]->fields().size(), 2); + EXPECT_EQ(metadata.value()->sort_orders[0]->fields()[0].source_id(), 1); + EXPECT_EQ(metadata.value()->sort_orders[0]->fields()[1].source_id(), 2); + EXPECT_EQ(metadata.value()->sort_orders[1]->order_id(), 2); +} + +TEST(MetadataSerdeTest, DeserializeDefaultSortOrderWithDroppedFieldFails) { + auto metadata = + TableMetadataFromJson(HistoricalSortOrderWithDroppedFieldMetadataJson(1)); + ASSERT_THAT(metadata, IsError(ErrorKind::kInvalidArgument)); + EXPECT_THAT(metadata, HasErrorMessage("Cannot find source column for sort field")); +} + TEST(MetadataSerdeTest, EncryptionKeysRoundTrip) { nlohmann::json metadata_json = R"({ "format-version": 2, From af4b76f2f806f026d921e9a8dea2b8ffbee30dc1 Mon Sep 17 00:00:00 2001 From: Zehua Zou Date: Tue, 23 Jun 2026 17:00:41 +0800 Subject: [PATCH 12/16] feat: add StoredLength method to PositionOutputStream (#773) --- src/iceberg/arrow/arrow_io.cc | 10 ++++++++++ src/iceberg/file_io.h | 6 ++++++ src/iceberg/puffin/puffin_writer.cc | 2 +- src/iceberg/test/arrow_io_test.cc | 15 +++++++++++++++ src/iceberg/test/std_io.h | 12 ++++++++++++ 5 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/iceberg/arrow/arrow_io.cc b/src/iceberg/arrow/arrow_io.cc index 6b159ca89..4c795badf 100644 --- a/src/iceberg/arrow/arrow_io.cc +++ b/src/iceberg/arrow/arrow_io.cc @@ -378,6 +378,13 @@ class ArrowPositionOutputStream : public PositionOutputStream { return position; } + Result StoredLength() const override { + if (!output_->closed()) { + return Position(); + } + return closed_position_; + } + Status Write(std::span data) override { ICEBERG_ASSIGN_OR_RAISE(auto size, ToInt64Length(data.size())); ICEBERG_ARROW_RETURN_NOT_OK(output_->Write(data.data(), size)); @@ -393,12 +400,15 @@ class ArrowPositionOutputStream : public PositionOutputStream { if (output_->closed()) { return {}; } + ICEBERG_ASSIGN_OR_RAISE(auto position, Position()); ICEBERG_ARROW_RETURN_NOT_OK(output_->Close()); + closed_position_ = position; return {}; } private: std::shared_ptr<::arrow::io::OutputStream> output_; + int64_t closed_position_ = 0; }; class ArrowInputFile : public InputFile { diff --git a/src/iceberg/file_io.h b/src/iceberg/file_io.h index 1f91fb0c1..ba6f0129a 100644 --- a/src/iceberg/file_io.h +++ b/src/iceberg/file_io.h @@ -66,6 +66,12 @@ class ICEBERG_EXPORT PositionOutputStream { /// \brief Return the current write position. virtual Result Position() const = 0; + /// \brief Return the current stored length of the output. + /// + /// This can differ from the current position for encrypting streams, and for other + /// non-length-preserving streams. + virtual Result StoredLength() const { return Position(); } + /// \brief Write all bytes in data at the current position. virtual Status Write(std::span data) = 0; diff --git a/src/iceberg/puffin/puffin_writer.cc b/src/iceberg/puffin/puffin_writer.cc index db749117f..9c173ab52 100644 --- a/src/iceberg/puffin/puffin_writer.cc +++ b/src/iceberg/puffin/puffin_writer.cc @@ -155,7 +155,7 @@ Status PuffinWriter::Finish() { footer_written_ = true; ICEBERG_RETURN_UNEXPECTED(stream_->Flush()); ICEBERG_RETURN_UNEXPECTED(stream_->Close()); - ICEBERG_ASSIGN_OR_RAISE(file_size_, stream_->Position()); + ICEBERG_ASSIGN_OR_RAISE(file_size_, stream_->StoredLength()); finished_ = true; return {}; } diff --git a/src/iceberg/test/arrow_io_test.cc b/src/iceberg/test/arrow_io_test.cc index 4ac83469c..a30e2da93 100644 --- a/src/iceberg/test/arrow_io_test.cc +++ b/src/iceberg/test/arrow_io_test.cc @@ -410,6 +410,21 @@ TEST_F(LocalFileIOTest, StdReadKeepsPositionAvailableAtEof) { EXPECT_THAT(stream->Position(), HasValue(::testing::Eq(3))); } +TEST(ArrowFileIOTest, OutputStoredLengthAfterClose) { + auto file_io = arrow::ArrowFileSystemFileIO::MakeMockFileIO(); + ICEBERG_UNWRAP_OR_FAIL(auto output_file, file_io->NewOutputFile("output")); + ICEBERG_UNWRAP_OR_FAIL(auto output, output_file->Create()); + + std::array data = {std::byte{'a'}, std::byte{'b'}, std::byte{'c'}}; + ASSERT_THAT(output->Write(data), IsOk()); + ASSERT_THAT(output->Close(), IsOk()); + + auto position = output->Position(); + ASSERT_FALSE(position.has_value()); + EXPECT_THAT(position.error().message, ::testing::HasSubstr("closed")); + EXPECT_THAT(output->StoredLength(), HasValue(::testing::Eq(3))); +} + TEST_F(LocalFileIOTest, ResolvesForeignSchemeToUnderlyingPath) { ASSERT_THAT(file_io_->WriteFile(temp_filepath_, "hello world"), IsOk()); diff --git a/src/iceberg/test/std_io.h b/src/iceberg/test/std_io.h index 3866bddf0..725fc7ba5 100644 --- a/src/iceberg/test/std_io.h +++ b/src/iceberg/test/std_io.h @@ -170,6 +170,18 @@ class StdPositionOutputStream : public PositionOutputStream { return static_cast(position); } + Result StoredLength() const override { + if (file_.is_open()) { + return Position(); + } + std::error_code ec; + auto size = std::filesystem::file_size(location_, ec); + if (ec) { + return IOError("Failed to get file size for {}: {}", location_, ec.message()); + } + return detail::ToInt64FileSize(size, location_); + } + Status Write(std::span data) override { if (data.empty()) { return {}; From 3a37c2480b344dd4ac3e6e1f5d9d5e4a6c4b5851 Mon Sep 17 00:00:00 2001 From: Manu Zhang Date: Tue, 9 Jun 2026 23:03:18 +0800 Subject: [PATCH 13/16] feat: add row delta update Implements the RowDelta update builder, table and transaction factory methods, and focused tests for row-level add/delete flows. Co-authored-by: Codex --- src/iceberg/CMakeLists.txt | 1 + src/iceberg/meson.build | 1 + src/iceberg/table.cc | 15 + src/iceberg/table.h | 7 + src/iceberg/test/CMakeLists.txt | 1 + src/iceberg/test/row_delta_test.cc | 388 +++++++++++++++++++ src/iceberg/test/table_test.cc | 2 + src/iceberg/transaction.cc | 8 + src/iceberg/transaction.h | 3 + src/iceberg/type_fwd.h | 1 + src/iceberg/update/merging_snapshot_update.h | 14 +- src/iceberg/update/meson.build | 1 + src/iceberg/update/row_delta.cc | 191 +++++++++ src/iceberg/update/row_delta.h | 104 +++++ 14 files changed, 731 insertions(+), 6 deletions(-) create mode 100644 src/iceberg/test/row_delta_test.cc create mode 100644 src/iceberg/update/row_delta.cc create mode 100644 src/iceberg/update/row_delta.h diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index ea76c641a..9a0dc68b7 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -106,6 +106,7 @@ set(ICEBERG_SOURCES update/merge_append.cc update/merging_snapshot_update.cc update/pending_update.cc + update/row_delta.cc update/set_snapshot.cc update/snapshot_manager.cc update/snapshot_update.cc diff --git a/src/iceberg/meson.build b/src/iceberg/meson.build index 7bd2e052c..ab514be87 100644 --- a/src/iceberg/meson.build +++ b/src/iceberg/meson.build @@ -131,6 +131,7 @@ iceberg_sources = files( 'update/merge_append.cc', 'update/merging_snapshot_update.cc', 'update/pending_update.cc', + 'update/row_delta.cc', 'update/set_snapshot.cc', 'update/snapshot_manager.cc', 'update/snapshot_update.cc', diff --git a/src/iceberg/table.cc b/src/iceberg/table.cc index 817e5917c..afa626964 100644 --- a/src/iceberg/table.cc +++ b/src/iceberg/table.cc @@ -35,6 +35,7 @@ #include "iceberg/update/expire_snapshots.h" #include "iceberg/update/fast_append.h" #include "iceberg/update/merge_append.h" +#include "iceberg/update/row_delta.h" #include "iceberg/update/set_snapshot.h" #include "iceberg/update/snapshot_manager.h" #include "iceberg/update/update_location.h" @@ -231,6 +232,12 @@ Result> Table::NewDeleteFiles() { return DeleteFiles::Make(name().name, std::move(ctx)); } +Result> Table::NewRowDelta() { + ICEBERG_ASSIGN_OR_RAISE( + auto ctx, TransactionContext::Make(shared_from_this(), TransactionKind::kUpdate)); + return RowDelta::Make(name().name, std::move(ctx)); +} + Result> Table::NewUpdateStatistics() { ICEBERG_ASSIGN_OR_RAISE( auto ctx, TransactionContext::Make(shared_from_this(), TransactionKind::kUpdate)); @@ -334,6 +341,14 @@ Result> StaticTable::NewMergeAppend() { return NotSupported("Cannot create a merge append for a static table"); } +Result> StaticTable::NewDeleteFiles() { + return NotSupported("Cannot create delete files for a static table"); +} + +Result> StaticTable::NewRowDelta() { + return NotSupported("Cannot create a row delta for a static table"); +} + Result> StaticTable::NewSnapshotManager() { return NotSupported("Cannot create a snapshot manager for a static table"); } diff --git a/src/iceberg/table.h b/src/iceberg/table.h index b71a1ddbc..c8f6ded08 100644 --- a/src/iceberg/table.h +++ b/src/iceberg/table.h @@ -182,6 +182,9 @@ class ICEBERG_EXPORT Table : public std::enable_shared_from_this
{ /// \brief Create a new DeleteFiles to delete data files and commit the changes. virtual Result> NewDeleteFiles(); + /// \brief Create a new RowDelta to add rows and row-level deletes. + virtual Result> NewRowDelta(); + /// \brief Create a new SnapshotManager to manage snapshots and snapshot references. virtual Result> NewSnapshotManager(); @@ -251,6 +254,10 @@ class ICEBERG_EXPORT StaticTable : public Table { Result> NewMergeAppend() override; + Result> NewDeleteFiles() override; + + Result> NewRowDelta() override; + Result> NewSnapshotManager() override; private: diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index c681de8c9..e528b1333 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -230,6 +230,7 @@ if(ICEBERG_BUILD_BUNDLE) merge_append_test.cc merging_snapshot_update_test.cc name_mapping_update_test.cc + row_delta_test.cc snapshot_manager_test.cc transaction_test.cc update_location_test.cc diff --git a/src/iceberg/test/row_delta_test.cc b/src/iceberg/test/row_delta_test.cc new file mode 100644 index 000000000..40453d345 --- /dev/null +++ b/src/iceberg/test/row_delta_test.cc @@ -0,0 +1,388 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/update/row_delta.h" + +#include +#include +#include +#include + +#include +#include + +#include "iceberg/avro/avro_register.h" +#include "iceberg/manifest/manifest_entry.h" +#include "iceberg/partition_spec.h" +#include "iceberg/row/partition_values.h" +#include "iceberg/schema.h" +#include "iceberg/snapshot.h" +#include "iceberg/table.h" +#include "iceberg/table_metadata.h" +#include "iceberg/test/matchers.h" +#include "iceberg/test/update_test_base.h" +#include "iceberg/update/delete_files.h" +#include "iceberg/update/fast_append.h" + +namespace iceberg { + +class RowDeltaTest : public MinimalUpdateTestBase { + protected: + static void SetUpTestSuite() { avro::RegisterAll(); } + + void SetUp() override { + MinimalUpdateTestBase::SetUp(); + + ICEBERG_UNWRAP_OR_FAIL(spec_, table_->spec()); + ICEBERG_UNWRAP_OR_FAIL(schema_, table_->schema()); + + file_a_ = MakeDataFile("/data/file_a.parquet", /*partition_x=*/1L); + file_b_ = MakeDataFile("/data/file_b.parquet", /*partition_x=*/2L); + } + + std::shared_ptr MakeDataFile(const std::string& path, int64_t partition_x) { + auto file = std::make_shared(); + file->content = DataFile::Content::kData; + file->file_path = table_location_ + path; + file->file_format = FileFormatType::kParquet; + file->partition = PartitionValues(std::vector{Literal::Long(partition_x)}); + file->file_size_in_bytes = 1024; + file->record_count = 100; + file->partition_spec_id = spec_->spec_id(); + return file; + } + + std::shared_ptr MakeDeleteFile(const std::string& path, int64_t partition_x) { + auto file = MakeDataFile(path, partition_x); + file->content = DataFile::Content::kPositionDeletes; + file->file_size_in_bytes = 256; + file->record_count = 7; + return file; + } + + std::shared_ptr MakeDeletionVector(const std::string& path, + const std::string& referenced_data_file, + int64_t partition_x, + int64_t content_offset = 0) { + auto file = MakeDeleteFile(path, partition_x); + file->file_format = FileFormatType::kPuffin; + file->referenced_data_file = referenced_data_file; + file->content_offset = content_offset; + file->content_size_in_bytes = 10; + return file; + } + + void CommitFileA() { + ICEBERG_UNWRAP_OR_FAIL(auto fast_append, table_->NewFastAppend()); + fast_append->AppendFile(file_a_); + EXPECT_THAT(fast_append->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + } + + void SetTableFormatVersion(int8_t format_version) { + table_->metadata()->format_version = format_version; + } + + std::shared_ptr spec_; + std::shared_ptr schema_; + std::shared_ptr file_a_; + std::shared_ptr file_b_; +}; + +TEST_F(RowDeltaTest, AddRowsCommitsAppendOperation) { + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->AddRows(file_a_); + + EXPECT_THAT(row_delta->Commit(), IsOk()); + + EXPECT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_EQ(snapshot->Operation(), std::make_optional(DataOperation::kAppend)); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedDataFiles), "1"); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedRecords), "100"); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedFileSize), "1024"); +} + +TEST_F(RowDeltaTest, AddDeletesCommitsDeleteOperation) { + auto delete_file = MakeDeleteFile("/delete/file_a_pos_deletes.parquet", + /*partition_x=*/1L); + + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->AddDeletes(delete_file); + + EXPECT_THAT(row_delta->Commit(), IsOk()); + + EXPECT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_EQ(snapshot->Operation(), std::make_optional(DataOperation::kDelete)); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedDeleteFiles), "1"); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedPosDeleteFiles), "1"); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedPosDeletes), "7"); +} + +TEST_F(RowDeltaTest, RemoveRowsCommitsOverwriteOperation) { + CommitFileA(); + + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->RemoveRows(file_a_); + + EXPECT_THAT(row_delta->Commit(), IsOk()); + + EXPECT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_EQ(snapshot->Operation(), std::make_optional(DataOperation::kOverwrite)); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kDeletedDataFiles), "1"); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kDeletedRecords), "100"); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kRemovedFileSize), "1024"); +} + +TEST_F(RowDeltaTest, RemoveRowsAndAddDeletesCommitsDeleteOperation) { + CommitFileA(); + + auto delete_file = MakeDeleteFile("/delete/file_a_pos_deletes.parquet", + /*partition_x=*/1L); + + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->RemoveRows(file_a_); + row_delta->AddDeletes(delete_file); + + EXPECT_THAT(row_delta->Commit(), IsOk()); + + EXPECT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_EQ(snapshot->Operation(), std::make_optional(DataOperation::kDelete)); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kDeletedDataFiles), "1"); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedDeleteFiles), "1"); +} + +TEST_F(RowDeltaTest, AddRowsAndRemoveDeletesCommitsAppendOperation) { + auto delete_file = MakeDeleteFile("/delete/file_a_pos_deletes.parquet", + /*partition_x=*/1L); + { + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->AddDeletes(delete_file); + EXPECT_THAT(row_delta->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + } + + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->AddRows(file_a_); + row_delta->RemoveDeletes(delete_file); + + EXPECT_THAT(row_delta->Commit(), IsOk()); + + EXPECT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_EQ(snapshot->Operation(), std::make_optional(DataOperation::kAppend)); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedDataFiles), "1"); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kRemovedDeleteFiles), "1"); +} + +TEST_F(RowDeltaTest, AddDeletesAndRemoveDeletesCommitsDeleteOperation) { + auto old_delete_file = MakeDeleteFile("/delete/file_a_pos_deletes.parquet", + /*partition_x=*/1L); + { + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->AddDeletes(old_delete_file); + EXPECT_THAT(row_delta->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + } + + auto new_delete_file = MakeDeleteFile("/delete/file_b_pos_deletes.parquet", + /*partition_x=*/2L); + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->AddDeletes(new_delete_file); + row_delta->RemoveDeletes(old_delete_file); + + EXPECT_THAT(row_delta->Commit(), IsOk()); + + EXPECT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_EQ(snapshot->Operation(), std::make_optional(DataOperation::kDelete)); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedDeleteFiles), "1"); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kRemovedDeleteFiles), "1"); +} + +TEST_F(RowDeltaTest, ValidateNoConflictingDataFilesFailsForConcurrentAppend) { + CommitFileA(); + ICEBERG_UNWRAP_OR_FAIL(auto starting_snapshot, table_->current_snapshot()); + + ICEBERG_UNWRAP_OR_FAIL(auto concurrent_append, table_->NewFastAppend()); + concurrent_append->AppendFile(file_b_); + EXPECT_THAT(concurrent_append->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + + auto file_c = MakeDataFile("/data/file_c.parquet", /*partition_x=*/3L); + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->ValidateFromSnapshot(starting_snapshot->snapshot_id); + row_delta->ValidateNoConflictingDataFiles(); + row_delta->AddRows(file_c); + + auto result = row_delta->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("Found conflicting files")); + EXPECT_THAT(result, HasErrorMessage(file_b_->file_path)); +} + +TEST_F(RowDeltaTest, ValidateNoConflictingDeleteFilesFailsForConcurrentDelete) { + CommitFileA(); + ICEBERG_UNWRAP_OR_FAIL(auto starting_snapshot, table_->current_snapshot()); + + auto delete_file = MakeDeleteFile("/delete/file_a_pos_deletes.parquet", + /*partition_x=*/1L); + std::shared_ptr concurrent_delta; + ICEBERG_UNWRAP_OR_FAIL(concurrent_delta, table_->NewRowDelta()); + concurrent_delta->AddDeletes(delete_file); + EXPECT_THAT(concurrent_delta->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + + auto file_c = MakeDataFile("/data/file_c.parquet", /*partition_x=*/3L); + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->ValidateFromSnapshot(starting_snapshot->snapshot_id); + row_delta->ValidateNoConflictingDeleteFiles(); + row_delta->AddRows(file_c); + + auto result = row_delta->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("Found new conflicting delete files")); + EXPECT_THAT(result, HasErrorMessage(delete_file->file_path)); +} + +TEST_F(RowDeltaTest, ValidateDataFilesExistFailsForConcurrentDelete) { + CommitFileA(); + ICEBERG_UNWRAP_OR_FAIL(auto starting_snapshot, table_->current_snapshot()); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); + delete_files->DeleteFile(file_a_); + EXPECT_THAT(delete_files->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + + auto delete_file = MakeDeleteFile("/delete/file_a_pos_deletes.parquet", + /*partition_x=*/1L); + delete_file->referenced_data_file = file_a_->file_path; + + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->ValidateFromSnapshot(starting_snapshot->snapshot_id); + std::vector referenced_files{file_a_->file_path}; + row_delta->ValidateDataFilesExist(referenced_files); + row_delta->AddDeletes(delete_file); + + auto result = row_delta->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("Cannot commit, missing data files")); + EXPECT_THAT(result, HasErrorMessage(file_a_->file_path)); +} + +TEST_F(RowDeltaTest, CannotRemoveReferencedDataFile) { + CommitFileA(); + + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + std::vector referenced_files{file_a_->file_path}; + row_delta->ValidateDataFilesExist(referenced_files); + row_delta->RemoveRows(file_a_); + + auto result = row_delta->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("Cannot delete data files")); + EXPECT_THAT(result, HasErrorMessage(file_a_->file_path)); +} + +TEST_F(RowDeltaTest, AddDeleteFileForRemovedDataFileCommitsDeleteOperation) { + CommitFileA(); + + auto delete_file = MakeDeleteFile("/delete/file_a_pos_deletes.parquet", + /*partition_x=*/1L); + delete_file->referenced_data_file = file_a_->file_path; + + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->RemoveRows(file_a_); + row_delta->AddDeletes(delete_file); + + EXPECT_THAT(row_delta->Commit(), IsOk()); + + EXPECT_THAT(table_->Refresh(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto snapshot, table_->current_snapshot()); + EXPECT_EQ(snapshot->Operation(), std::make_optional(DataOperation::kDelete)); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kDeletedDataFiles), "1"); + EXPECT_EQ(snapshot->summary.at(SnapshotSummaryFields::kAddedDeleteFiles), "1"); +} + +TEST_F(RowDeltaTest, ValidateDeletedFilesAllowsMissingRowsOnEmptyTable) { + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->ValidateDeletedFiles(); + row_delta->RemoveRows(file_a_); + + EXPECT_THAT(row_delta->Commit(), IsOk()); +} + +TEST_F(RowDeltaTest, ValidateDeletedFilesAllowsMissingDeletesOnEmptyTable) { + auto delete_file = MakeDeleteFile("/delete/file_a_pos_deletes.parquet", + /*partition_x=*/1L); + + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->ValidateDeletedFiles(); + row_delta->RemoveDeletes(delete_file); + + EXPECT_THAT(row_delta->Commit(), IsOk()); +} + +TEST_F(RowDeltaTest, AddDeletionVectorValidatesConcurrentDVs) { + CommitFileA(); + ICEBERG_UNWRAP_OR_FAIL(auto starting_snapshot, table_->current_snapshot()); + SetTableFormatVersion(3); + + auto concurrent_dv = + MakeDeletionVector("/delete/concurrent-dv-a.puffin", file_a_->file_path, + /*partition_x=*/1L, /*content_offset=*/0); + std::shared_ptr concurrent_delta; + ICEBERG_UNWRAP_OR_FAIL(concurrent_delta, table_->NewRowDelta()); + concurrent_delta->AddDeletes(concurrent_dv); + EXPECT_THAT(concurrent_delta->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + SetTableFormatVersion(3); + + auto dv = MakeDeletionVector("/delete/dv-a.puffin", file_a_->file_path, + /*partition_x=*/1L, /*content_offset=*/10); + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->ValidateFromSnapshot(starting_snapshot->snapshot_id); + row_delta->AddDeletes(dv); + + auto result = row_delta->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("Found concurrently added DV")); + EXPECT_THAT(result, HasErrorMessage(file_a_->file_path)); +} + +} // namespace iceberg diff --git a/src/iceberg/test/table_test.cc b/src/iceberg/test/table_test.cc index 881c4fdd0..d7ebe4a0a 100644 --- a/src/iceberg/test/table_test.cc +++ b/src/iceberg/test/table_test.cc @@ -161,6 +161,8 @@ TEST(StaticTableTest, NewMutatingOperationsAreNotSupported) { EXPECT_THAT(table->NewUpdatePartitionStatistics(), IsError(ErrorKind::kNotSupported)); EXPECT_THAT(table->NewFastAppend(), IsError(ErrorKind::kNotSupported)); EXPECT_THAT(table->NewMergeAppend(), IsError(ErrorKind::kNotSupported)); + EXPECT_THAT(table->NewDeleteFiles(), IsError(ErrorKind::kNotSupported)); + EXPECT_THAT(table->NewRowDelta(), IsError(ErrorKind::kNotSupported)); EXPECT_THAT(table->NewSnapshotManager(), IsError(ErrorKind::kNotSupported)); } diff --git a/src/iceberg/transaction.cc b/src/iceberg/transaction.cc index ac1f08241..e911a61dc 100644 --- a/src/iceberg/transaction.cc +++ b/src/iceberg/transaction.cc @@ -37,6 +37,7 @@ #include "iceberg/update/fast_append.h" #include "iceberg/update/merge_append.h" #include "iceberg/update/pending_update.h" +#include "iceberg/update/row_delta.h" #include "iceberg/update/set_snapshot.h" #include "iceberg/update/snapshot_manager.h" #include "iceberg/update/snapshot_update.h" @@ -505,6 +506,13 @@ Result> Transaction::NewDeleteFiles() { return delete_files; } +Result> Transaction::NewRowDelta() { + ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr row_delta, + RowDelta::Make(ctx_->table->name().name, ctx_)); + ICEBERG_RETURN_UNEXPECTED(AddUpdate(row_delta)); + return row_delta; +} + Result> Transaction::NewUpdateStatistics() { ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr update_statistics, UpdateStatistics::Make(ctx_)); diff --git a/src/iceberg/transaction.h b/src/iceberg/transaction.h index 52a0605c6..34ca78bd7 100644 --- a/src/iceberg/transaction.h +++ b/src/iceberg/transaction.h @@ -112,6 +112,9 @@ class ICEBERG_EXPORT Transaction : public std::enable_shared_from_this> NewDeleteFiles(); + /// \brief Create a new RowDelta to add rows and row-level deletes. + Result> NewRowDelta(); + /// \brief Create a new SnapshotManager to manage snapshots. Result> NewSnapshotManager(); diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index 0320f24ea..f29bc4a1a 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -243,6 +243,7 @@ class ExpireSnapshots; class FastAppend; class MergeAppend; class PendingUpdate; +class RowDelta; class SetSnapshot; class SnapshotManager; class SnapshotUpdate; diff --git a/src/iceberg/update/merging_snapshot_update.h b/src/iceberg/update/merging_snapshot_update.h index 879403222..fc3987ee1 100644 --- a/src/iceberg/update/merging_snapshot_update.h +++ b/src/iceberg/update/merging_snapshot_update.h @@ -288,6 +288,14 @@ class ICEBERG_EXPORT MergingSnapshotUpdate : public SnapshotUpdate { const std::shared_ptr& parent, std::shared_ptr io, bool case_sensitive = true); + /// \brief Return an error if a staged deletion vector conflicts with a deletion + /// vector added since starting_snapshot_id. + Status ValidateAddedDVs(const TableMetadata& metadata, + std::optional starting_snapshot_id, + std::shared_ptr conflict_filter, + const std::shared_ptr& parent, + std::shared_ptr io) const; + private: struct PendingDeleteFile { std::shared_ptr file; @@ -324,12 +332,6 @@ class ICEBERG_EXPORT MergingSnapshotUpdate : public SnapshotUpdate { Status AddDeleteFile(std::shared_ptr file, std::optional data_sequence_number); - Status ValidateAddedDVs(const TableMetadata& metadata, - std::optional starting_snapshot_id, - std::shared_ptr conflict_filter, - const std::shared_ptr& parent, - std::shared_ptr io) const; - Status ManagersReady() const; void SetSummaryProperty(const std::string& property, const std::string& value) override; diff --git a/src/iceberg/update/meson.build b/src/iceberg/update/meson.build index 9f950e8d0..4f594a06e 100644 --- a/src/iceberg/update/meson.build +++ b/src/iceberg/update/meson.build @@ -23,6 +23,7 @@ install_headers( 'merge_append.h', 'merging_snapshot_update.h', 'pending_update.h', + 'row_delta.h', 'set_snapshot.h', 'snapshot_manager.h', 'snapshot_update.h', diff --git a/src/iceberg/update/row_delta.cc b/src/iceberg/update/row_delta.cc new file mode 100644 index 000000000..0d3fdf5ec --- /dev/null +++ b/src/iceberg/update/row_delta.cc @@ -0,0 +1,191 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/update/row_delta.h" + +#include +#include +#include +#include +#include + +#include "iceberg/expression/expressions.h" +#include "iceberg/manifest/manifest_entry.h" +#include "iceberg/snapshot.h" +#include "iceberg/table.h" +#include "iceberg/table_metadata.h" +#include "iceberg/transaction.h" +#include "iceberg/util/error_collector.h" +#include "iceberg/util/formatter_internal.h" +#include "iceberg/util/macros.h" +#include "iceberg/util/snapshot_util_internal.h" + +namespace iceberg { + +Result> RowDelta::Make( + std::string table_name, std::shared_ptr ctx) { + ICEBERG_PRECHECK(!table_name.empty(), "Table name cannot be empty"); + ICEBERG_PRECHECK(ctx != nullptr, "Cannot create RowDelta without a context"); + return std::unique_ptr(new RowDelta(std::move(table_name), std::move(ctx))); +} + +RowDelta::RowDelta(std::string table_name, std::shared_ptr ctx) + : MergingSnapshotUpdate(std::move(table_name), std::move(ctx)), + conflict_detection_filter_(Expressions::AlwaysTrue()) {} + +RowDelta& RowDelta::AddRows(const std::shared_ptr& inserts) { + ICEBERG_BUILDER_RETURN_IF_ERROR(AddDataFile(inserts)); + return *this; +} + +RowDelta& RowDelta::AddDeletes(const std::shared_ptr& deletes) { + ICEBERG_BUILDER_RETURN_IF_ERROR(AddDeleteFile(deletes)); + return *this; +} + +RowDelta& RowDelta::RemoveRows(const std::shared_ptr& file) { + ICEBERG_BUILDER_RETURN_IF_ERROR(DeleteDataFile(file)); + removed_data_files_.insert(file); + return *this; +} + +RowDelta& RowDelta::RemoveDeletes(const std::shared_ptr& deletes) { + ICEBERG_BUILDER_RETURN_IF_ERROR(DeleteDeleteFile(deletes)); + return *this; +} + +RowDelta& RowDelta::ValidateFromSnapshot(int64_t snapshot_id) { + starting_snapshot_id_ = snapshot_id; + return *this; +} + +RowDelta& RowDelta::CaseSensitive(bool case_sensitive) { + MergingSnapshotUpdate::CaseSensitive(case_sensitive); + return *this; +} + +RowDelta& RowDelta::ValidateDataFilesExist( + std::span referenced_files) { + for (const auto& file : referenced_files) { + referenced_data_files_.insert(file); + } + return *this; +} + +RowDelta& RowDelta::ValidateDeletedFiles() { + validate_deletes_ = true; + return *this; +} + +RowDelta& RowDelta::ConflictDetectionFilter(std::shared_ptr filter) { + ICEBERG_BUILDER_CHECK(filter != nullptr, "Conflict detection filter cannot be null"); + conflict_detection_filter_ = std::move(filter); + return *this; +} + +RowDelta& RowDelta::ValidateNoConflictingDataFiles() { + validate_new_data_files_ = true; + return *this; +} + +RowDelta& RowDelta::ValidateNoConflictingDeleteFiles() { + validate_new_delete_files_ = true; + return *this; +} + +std::string RowDelta::operation() { + if (AddsDataFiles() && !AddsDeleteFiles() && !DeletesDataFiles()) { + return DataOperation::kAppend; + } + + if (AddsDeleteFiles() && !AddsDataFiles()) { + return DataOperation::kDelete; + } + + return DataOperation::kOverwrite; +} + +Status RowDelta::Validate(const TableMetadata& current_metadata, + const std::shared_ptr& snapshot) { + if (snapshot == nullptr) { + return {}; + } + + if (validate_deletes_) { + FailMissingDeletePaths(); + } + + if (starting_snapshot_id_.has_value()) { + ICEBERG_ASSIGN_OR_RAISE(bool is_ancestor, SnapshotUtil::IsAncestorOf( + current_metadata, snapshot->snapshot_id, + starting_snapshot_id_.value())); + ICEBERG_CHECK(is_ancestor, "Snapshot {} is not an ancestor of {}", + starting_snapshot_id_.value(), snapshot->snapshot_id); + } + + auto io = ctx_->table->io(); + if (!referenced_data_files_.empty()) { + ICEBERG_RETURN_UNEXPECTED(MergingSnapshotUpdate::ValidateDataFilesExist( + current_metadata, starting_snapshot_id_, referenced_data_files_, + /*skip_deletes=*/false, conflict_detection_filter_, snapshot, io, + IsCaseSensitive())); + } + + if (validate_new_data_files_) { + ICEBERG_RETURN_UNEXPECTED(MergingSnapshotUpdate::ValidateAddedDataFiles( + current_metadata, starting_snapshot_id_, conflict_detection_filter_, snapshot, io, + IsCaseSensitive())); + } + + if (validate_new_delete_files_) { + if (!removed_data_files_.empty()) { + ICEBERG_RETURN_UNEXPECTED(MergingSnapshotUpdate::ValidateNoNewDeletesForDataFiles( + current_metadata, starting_snapshot_id_, conflict_detection_filter_, + removed_data_files_, snapshot, io, IsCaseSensitive())); + } + + ICEBERG_RETURN_UNEXPECTED(MergingSnapshotUpdate::ValidateNoNewDeleteFiles( + current_metadata, starting_snapshot_id_, conflict_detection_filter_, snapshot, io, + IsCaseSensitive())); + } + + ICEBERG_RETURN_UNEXPECTED(ValidateNoConflictingFileAndPositionDeletes()); + + return MergingSnapshotUpdate::ValidateAddedDVs( + current_metadata, starting_snapshot_id_, conflict_detection_filter_, snapshot, io); +} + +Status RowDelta::ValidateNoConflictingFileAndPositionDeletes() const { + std::vector conflicting_files; + for (const auto& file : removed_data_files_) { + if (file != nullptr && referenced_data_files_.contains(file->file_path)) { + conflicting_files.push_back(file->file_path); + } + } + + if (!conflicting_files.empty()) { + return ValidationFailed( + "Cannot delete data files {} that are referenced by new delete files", + FormatRange(conflicting_files, ", ", "[", "]")); + } + + return {}; +} + +} // namespace iceberg diff --git a/src/iceberg/update/row_delta.h b/src/iceberg/update/row_delta.h new file mode 100644 index 000000000..5d859edd5 --- /dev/null +++ b/src/iceberg/update/row_delta.h @@ -0,0 +1,104 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +/// \file iceberg/update/row_delta.h + +#include +#include +#include +#include +#include +#include + +#include "iceberg/iceberg_export.h" +#include "iceberg/result.h" +#include "iceberg/type_fwd.h" +#include "iceberg/update/merging_snapshot_update.h" +#include "iceberg/util/data_file_set.h" + +namespace iceberg { + +/// \brief Row-level delta operation for adding rows and delete files. +/// +/// RowDelta is the C++ counterpart of Java BaseRowDelta. It can add data files, +/// add delete files, remove data/delete files, and validate conflicts against +/// snapshots committed after a configured starting snapshot. +class ICEBERG_EXPORT RowDelta : public MergingSnapshotUpdate { + public: + /// \brief Create a new RowDelta instance. + static Result> Make(std::string table_name, + std::shared_ptr ctx); + + /// \brief Add a data file containing inserted rows. + RowDelta& AddRows(const std::shared_ptr& inserts); + + /// \brief Add a delete file. + RowDelta& AddDeletes(const std::shared_ptr& deletes); + + /// \brief Remove a data file from the table. + RowDelta& RemoveRows(const std::shared_ptr& file); + + /// \brief Remove a delete file from the table. + RowDelta& RemoveDeletes(const std::shared_ptr& deletes); + + /// \brief Validate against snapshots committed after snapshot_id. + RowDelta& ValidateFromSnapshot(int64_t snapshot_id); + + /// \brief Set case sensitivity for conflict detection. + RowDelta& CaseSensitive(bool case_sensitive); + + /// \brief Validate that referenced data files still exist. + RowDelta& ValidateDataFilesExist(std::span referenced_files); + + /// \brief Fail if any requested data/delete-file removal is missing from + /// manifests when the table has a current snapshot. + RowDelta& ValidateDeletedFiles(); + + /// \brief Set the conflict detection filter used by validation methods. + RowDelta& ConflictDetectionFilter(std::shared_ptr filter); + + /// \brief Validate that no matching data files were concurrently added. + RowDelta& ValidateNoConflictingDataFiles(); + + /// \brief Validate that no matching delete files were concurrently added. + RowDelta& ValidateNoConflictingDeleteFiles(); + + std::string operation() override; + + protected: + Status Validate(const TableMetadata& current_metadata, + const std::shared_ptr& snapshot) override; + + private: + explicit RowDelta(std::string table_name, std::shared_ptr ctx); + + Status ValidateNoConflictingFileAndPositionDeletes() const; + + std::optional starting_snapshot_id_; + std::unordered_set referenced_data_files_; + DataFileSet removed_data_files_; + bool validate_deletes_ = false; + std::shared_ptr conflict_detection_filter_; + bool validate_new_data_files_ = false; + bool validate_new_delete_files_ = false; +}; + +} // namespace iceberg From 842d725f72d8a6cbc9ff23c9e3168c45182be586 Mon Sep 17 00:00:00 2001 From: manuzhang Date: Tue, 23 Jun 2026 17:53:45 +0800 Subject: [PATCH 14/16] fix row delta delete validation Co-authored-by: Codex --- src/iceberg/test/row_delta_test.cc | 26 +++++++++++++++++++++++++- src/iceberg/update/row_delta.cc | 2 +- src/iceberg/update/row_delta.h | 10 ++++++++-- 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/src/iceberg/test/row_delta_test.cc b/src/iceberg/test/row_delta_test.cc index 40453d345..5d64f079c 100644 --- a/src/iceberg/test/row_delta_test.cc +++ b/src/iceberg/test/row_delta_test.cc @@ -274,7 +274,7 @@ TEST_F(RowDeltaTest, ValidateNoConflictingDeleteFilesFailsForConcurrentDelete) { EXPECT_THAT(result, HasErrorMessage(delete_file->file_path)); } -TEST_F(RowDeltaTest, ValidateDataFilesExistFailsForConcurrentDelete) { +TEST_F(RowDeltaTest, ValidateDataFilesExistSkipsConcurrentDeleteByDefault) { CommitFileA(); ICEBERG_UNWRAP_OR_FAIL(auto starting_snapshot, table_->current_snapshot()); @@ -294,6 +294,30 @@ TEST_F(RowDeltaTest, ValidateDataFilesExistFailsForConcurrentDelete) { row_delta->ValidateDataFilesExist(referenced_files); row_delta->AddDeletes(delete_file); + EXPECT_THAT(row_delta->Commit(), IsOk()); +} + +TEST_F(RowDeltaTest, ValidateDataFilesExistFailsForConcurrentDeleteWithValidateDeletedFiles) { + CommitFileA(); + ICEBERG_UNWRAP_OR_FAIL(auto starting_snapshot, table_->current_snapshot()); + + ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); + delete_files->DeleteFile(file_a_); + EXPECT_THAT(delete_files->Commit(), IsOk()); + EXPECT_THAT(table_->Refresh(), IsOk()); + + auto delete_file = MakeDeleteFile("/delete/file_a_pos_deletes.parquet", + /*partition_x=*/1L); + delete_file->referenced_data_file = file_a_->file_path; + + std::shared_ptr row_delta; + ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); + row_delta->ValidateFromSnapshot(starting_snapshot->snapshot_id); + std::vector referenced_files{file_a_->file_path}; + row_delta->ValidateDataFilesExist(referenced_files); + row_delta->ValidateDeletedFiles(); + row_delta->AddDeletes(delete_file); + auto result = row_delta->Commit(); EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); EXPECT_THAT(result, HasErrorMessage("Cannot commit, missing data files")); diff --git a/src/iceberg/update/row_delta.cc b/src/iceberg/update/row_delta.cc index 0d3fdf5ec..d2b331572 100644 --- a/src/iceberg/update/row_delta.cc +++ b/src/iceberg/update/row_delta.cc @@ -143,7 +143,7 @@ Status RowDelta::Validate(const TableMetadata& current_metadata, if (!referenced_data_files_.empty()) { ICEBERG_RETURN_UNEXPECTED(MergingSnapshotUpdate::ValidateDataFilesExist( current_metadata, starting_snapshot_id_, referenced_data_files_, - /*skip_deletes=*/false, conflict_detection_filter_, snapshot, io, + /*skip_deletes=*/!validate_deletes_, conflict_detection_filter_, snapshot, io, IsCaseSensitive())); } diff --git a/src/iceberg/update/row_delta.h b/src/iceberg/update/row_delta.h index 5d859edd5..6d50deb7d 100644 --- a/src/iceberg/update/row_delta.h +++ b/src/iceberg/update/row_delta.h @@ -66,10 +66,16 @@ class ICEBERG_EXPORT RowDelta : public MergingSnapshotUpdate { RowDelta& CaseSensitive(bool case_sensitive); /// \brief Validate that referenced data files still exist. + /// + /// By default, this validation checks overwrite and replace commits. To apply + /// validation to delete commits, call ValidateDeletedFiles(). RowDelta& ValidateDataFilesExist(std::span referenced_files); - /// \brief Fail if any requested data/delete-file removal is missing from - /// manifests when the table has a current snapshot. + /// \brief Enable validation for missing delete paths and delete-operation conflicts. + /// + /// This fails if any requested data/delete-file removal is missing from + /// manifests when the table has a current snapshot. It also makes + /// ValidateDataFilesExist() check delete-operation snapshots. RowDelta& ValidateDeletedFiles(); /// \brief Set the conflict detection filter used by validation methods. From ec5d51ef99f8a55542fb94033823e3782b0e538c Mon Sep 17 00:00:00 2001 From: manuzhang Date: Tue, 23 Jun 2026 18:23:09 +0800 Subject: [PATCH 15/16] fix row delta test formatting Co-authored-by: Codex --- src/iceberg/test/row_delta_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/iceberg/test/row_delta_test.cc b/src/iceberg/test/row_delta_test.cc index 5d64f079c..ce5721255 100644 --- a/src/iceberg/test/row_delta_test.cc +++ b/src/iceberg/test/row_delta_test.cc @@ -297,7 +297,8 @@ TEST_F(RowDeltaTest, ValidateDataFilesExistSkipsConcurrentDeleteByDefault) { EXPECT_THAT(row_delta->Commit(), IsOk()); } -TEST_F(RowDeltaTest, ValidateDataFilesExistFailsForConcurrentDeleteWithValidateDeletedFiles) { +TEST_F(RowDeltaTest, + ValidateDataFilesExistFailsForConcurrentDeleteWithValidateDeletedFiles) { CommitFileA(); ICEBERG_UNWRAP_OR_FAIL(auto starting_snapshot, table_->current_snapshot()); From d184b955617fe4a7385825fc75523049e01e10b4 Mon Sep 17 00:00:00 2001 From: Gang Wu Date: Tue, 23 Jun 2026 22:28:52 +0800 Subject: [PATCH 16/16] add more comments --- src/iceberg/test/row_delta_test.cc | 20 +++--- src/iceberg/update/row_delta.cc | 2 + src/iceberg/update/row_delta.h | 104 +++++++++++++++++++++++------ 3 files changed, 97 insertions(+), 29 deletions(-) diff --git a/src/iceberg/test/row_delta_test.cc b/src/iceberg/test/row_delta_test.cc index ce5721255..5906e7e39 100644 --- a/src/iceberg/test/row_delta_test.cc +++ b/src/iceberg/test/row_delta_test.cc @@ -88,7 +88,7 @@ class RowDeltaTest : public MinimalUpdateTestBase { return file; } - void CommitFileA() { + void AppendFileAToTable() { ICEBERG_UNWRAP_OR_FAIL(auto fast_append, table_->NewFastAppend()); fast_append->AppendFile(file_a_); EXPECT_THAT(fast_append->Commit(), IsOk()); @@ -139,7 +139,7 @@ TEST_F(RowDeltaTest, AddDeletesCommitsDeleteOperation) { } TEST_F(RowDeltaTest, RemoveRowsCommitsOverwriteOperation) { - CommitFileA(); + AppendFileAToTable(); std::shared_ptr row_delta; ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); @@ -156,7 +156,7 @@ TEST_F(RowDeltaTest, RemoveRowsCommitsOverwriteOperation) { } TEST_F(RowDeltaTest, RemoveRowsAndAddDeletesCommitsDeleteOperation) { - CommitFileA(); + AppendFileAToTable(); auto delete_file = MakeDeleteFile("/delete/file_a_pos_deletes.parquet", /*partition_x=*/1L); @@ -228,7 +228,7 @@ TEST_F(RowDeltaTest, AddDeletesAndRemoveDeletesCommitsDeleteOperation) { } TEST_F(RowDeltaTest, ValidateNoConflictingDataFilesFailsForConcurrentAppend) { - CommitFileA(); + AppendFileAToTable(); ICEBERG_UNWRAP_OR_FAIL(auto starting_snapshot, table_->current_snapshot()); ICEBERG_UNWRAP_OR_FAIL(auto concurrent_append, table_->NewFastAppend()); @@ -250,7 +250,7 @@ TEST_F(RowDeltaTest, ValidateNoConflictingDataFilesFailsForConcurrentAppend) { } TEST_F(RowDeltaTest, ValidateNoConflictingDeleteFilesFailsForConcurrentDelete) { - CommitFileA(); + AppendFileAToTable(); ICEBERG_UNWRAP_OR_FAIL(auto starting_snapshot, table_->current_snapshot()); auto delete_file = MakeDeleteFile("/delete/file_a_pos_deletes.parquet", @@ -275,7 +275,7 @@ TEST_F(RowDeltaTest, ValidateNoConflictingDeleteFilesFailsForConcurrentDelete) { } TEST_F(RowDeltaTest, ValidateDataFilesExistSkipsConcurrentDeleteByDefault) { - CommitFileA(); + AppendFileAToTable(); ICEBERG_UNWRAP_OR_FAIL(auto starting_snapshot, table_->current_snapshot()); ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); @@ -299,7 +299,7 @@ TEST_F(RowDeltaTest, ValidateDataFilesExistSkipsConcurrentDeleteByDefault) { TEST_F(RowDeltaTest, ValidateDataFilesExistFailsForConcurrentDeleteWithValidateDeletedFiles) { - CommitFileA(); + AppendFileAToTable(); ICEBERG_UNWRAP_OR_FAIL(auto starting_snapshot, table_->current_snapshot()); ICEBERG_UNWRAP_OR_FAIL(auto delete_files, table_->NewDeleteFiles()); @@ -326,7 +326,7 @@ TEST_F(RowDeltaTest, } TEST_F(RowDeltaTest, CannotRemoveReferencedDataFile) { - CommitFileA(); + AppendFileAToTable(); std::shared_ptr row_delta; ICEBERG_UNWRAP_OR_FAIL(row_delta, table_->NewRowDelta()); @@ -341,7 +341,7 @@ TEST_F(RowDeltaTest, CannotRemoveReferencedDataFile) { } TEST_F(RowDeltaTest, AddDeleteFileForRemovedDataFileCommitsDeleteOperation) { - CommitFileA(); + AppendFileAToTable(); auto delete_file = MakeDeleteFile("/delete/file_a_pos_deletes.parquet", /*partition_x=*/1L); @@ -383,7 +383,7 @@ TEST_F(RowDeltaTest, ValidateDeletedFilesAllowsMissingDeletesOnEmptyTable) { } TEST_F(RowDeltaTest, AddDeletionVectorValidatesConcurrentDVs) { - CommitFileA(); + AppendFileAToTable(); ICEBERG_UNWRAP_OR_FAIL(auto starting_snapshot, table_->current_snapshot()); SetTableFormatVersion(3); diff --git a/src/iceberg/update/row_delta.cc b/src/iceberg/update/row_delta.cc index d2b331572..dd3f50c58 100644 --- a/src/iceberg/update/row_delta.cc +++ b/src/iceberg/update/row_delta.cc @@ -154,12 +154,14 @@ Status RowDelta::Validate(const TableMetadata& current_metadata, } if (validate_new_delete_files_) { + // validate that explicitly deleted files have not had added deletes if (!removed_data_files_.empty()) { ICEBERG_RETURN_UNEXPECTED(MergingSnapshotUpdate::ValidateNoNewDeletesForDataFiles( current_metadata, starting_snapshot_id_, conflict_detection_filter_, removed_data_files_, snapshot, io, IsCaseSensitive())); } + // validate that previous deletes do not conflict with added deletes ICEBERG_RETURN_UNEXPECTED(MergingSnapshotUpdate::ValidateNoNewDeleteFiles( current_metadata, starting_snapshot_id_, conflict_detection_filter_, snapshot, io, IsCaseSensitive())); diff --git a/src/iceberg/update/row_delta.h b/src/iceberg/update/row_delta.h index 6d50deb7d..ddb54d836 100644 --- a/src/iceberg/update/row_delta.h +++ b/src/iceberg/update/row_delta.h @@ -36,55 +36,121 @@ namespace iceberg { -/// \brief Row-level delta operation for adding rows and delete files. +/// \brief API for encoding row-level changes to a table. /// -/// RowDelta is the C++ counterpart of Java BaseRowDelta. It can add data files, -/// add delete files, remove data/delete files, and validate conflicts against -/// snapshots committed after a configured starting snapshot. +/// This API accumulates data and delete file changes, produces a new Snapshot +/// of the table, and commits that snapshot as current. +/// +/// When committing, these changes are applied to the latest table snapshot. +/// Commit conflicts are resolved by applying the changes to the new latest +/// snapshot and reattempting the commit. class ICEBERG_EXPORT RowDelta : public MergingSnapshotUpdate { public: /// \brief Create a new RowDelta instance. static Result> Make(std::string table_name, std::shared_ptr ctx); - /// \brief Add a data file containing inserted rows. + /// \brief Add a data file to the table. + /// + /// \param inserts A data file of rows to insert. + /// \return This RowDelta for method chaining. RowDelta& AddRows(const std::shared_ptr& inserts); - /// \brief Add a delete file. + /// \brief Add a delete file to the table. + /// + /// \param deletes A delete file of rows to delete. + /// \return This RowDelta for method chaining. RowDelta& AddDeletes(const std::shared_ptr& deletes); /// \brief Remove a data file from the table. + /// + /// \param file A data file. + /// \return This RowDelta for method chaining. RowDelta& RemoveRows(const std::shared_ptr& file); - /// \brief Remove a delete file from the table. + /// \brief Remove a rewritten delete file from the table. + /// + /// \param deletes A delete file that can be removed from the table. + /// \return This RowDelta for method chaining. RowDelta& RemoveDeletes(const std::shared_ptr& deletes); - /// \brief Validate against snapshots committed after snapshot_id. + /// \brief Set the snapshot ID used in any reads for this operation. + /// + /// Validations check changes after this snapshot ID. If the from snapshot is + /// not set, all ancestor snapshots through the table's initial snapshot are + /// validated. + /// + /// \param snapshot_id A snapshot ID. + /// \return This RowDelta for method chaining. RowDelta& ValidateFromSnapshot(int64_t snapshot_id); - /// \brief Set case sensitivity for conflict detection. + /// \brief Enable or disable case-sensitive expression binding for validations. + /// + /// \param case_sensitive Whether expression binding should be case sensitive. + /// \return This RowDelta for method chaining. RowDelta& CaseSensitive(bool case_sensitive); - /// \brief Validate that referenced data files still exist. + /// \brief Add data file paths that must not be removed by conflicting commits. + /// + /// If any path has been removed by a conflicting commit in the table since + /// the snapshot passed to ValidateFromSnapshot(), the operation fails. + /// + /// By default, this validation checks only rewrite and overwrite commits. To + /// apply validation to delete commits, call ValidateDeletedFiles(). /// - /// By default, this validation checks overwrite and replace commits. To apply - /// validation to delete commits, call ValidateDeletedFiles(). + /// \param referenced_files File paths that are referenced by a position + /// delete file. + /// \return This RowDelta for method chaining. RowDelta& ValidateDataFilesExist(std::span referenced_files); - /// \brief Enable validation for missing delete paths and delete-operation conflicts. + /// \brief Enable validation that referenced data files were not deleted. /// - /// This fails if any requested data/delete-file removal is missing from - /// manifests when the table has a current snapshot. It also makes - /// ValidateDataFilesExist() check delete-operation snapshots. + /// If a data file has a row deleted using a position delete file, rewriting + /// or overwriting the data file concurrently would un-delete the row. Deleting + /// the data file is normally allowed, but a delete may be part of a + /// transaction that reads and re-appends a row. This method is used to + /// validate deletes for the transaction case. + /// + /// \return This RowDelta for method chaining. RowDelta& ValidateDeletedFiles(); - /// \brief Set the conflict detection filter used by validation methods. + /// \brief Set a conflict detection filter used to validate added files. + /// + /// If not called, a true literal is used as the conflict detection filter. + /// + /// \param filter An expression on rows in the table. + /// \return This RowDelta for method chaining. RowDelta& ConflictDetectionFilter(std::shared_ptr filter); - /// \brief Validate that no matching data files were concurrently added. + /// \brief Enable validation that concurrent data files do not conflict. + /// + /// This method should be called when the table is queried to determine which + /// files to delete or append. If a concurrent operation commits a new file + /// after the data was read and that file might contain rows matching the + /// conflict detection filter, this operation detects that during retries and + /// fails. + /// + /// Calling this method is required to maintain serializable isolation for + /// update/delete operations. Otherwise, the isolation level is snapshot + /// isolation. + /// + /// Validation uses the filter passed to ConflictDetectionFilter() and applies + /// to operations after the snapshot passed to ValidateFromSnapshot(). + /// + /// \return This RowDelta for method chaining. RowDelta& ValidateNoConflictingDataFiles(); - /// \brief Validate that no matching delete files were concurrently added. + /// \brief Enable validation that concurrent delete files do not conflict. + /// + /// This method must be called when the table is queried to produce a row + /// delta for UPDATE and MERGE operations independently of the isolation level. + /// Calling this method is not required for DELETE operations because it is OK + /// to delete a record that is also deleted concurrently. + /// + /// Validation uses the filter passed to ConflictDetectionFilter() and applies + /// to operations after the snapshot passed to ValidateFromSnapshot(). + /// + /// \return This RowDelta for method chaining. RowDelta& ValidateNoConflictingDeleteFiles(); std::string operation() override;