From 9b97d6b4910765c0eb90b9ccf542826addaac08f Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Tue, 23 Dec 2025 18:37:02 +0800 Subject: [PATCH 1/8] feat: replace sort order and refactor transaction / pending update --- src/iceberg/CMakeLists.txt | 1 + src/iceberg/expression/term.h | 2 + src/iceberg/meson.build | 1 + src/iceberg/sort_order.cc | 4 +- src/iceberg/table.cc | 7 + src/iceberg/table.h | 4 + src/iceberg/table_metadata.h | 2 +- src/iceberg/table_update.h | 7 +- src/iceberg/test/CMakeLists.txt | 3 +- src/iceberg/test/replace_sort_order_test.cc | 272 ++++++++++++++++++++ src/iceberg/test/table_requirements_test.cc | 2 +- src/iceberg/test/table_update_test.cc | 2 +- src/iceberg/test/transaction_test.cc | 10 - src/iceberg/test/update_properties_test.cc | 63 +++-- src/iceberg/transaction.cc | 34 ++- src/iceberg/transaction.h | 6 +- src/iceberg/type_fwd.h | 1 + src/iceberg/update/meson.build | 2 +- src/iceberg/update/pending_update.cc | 7 +- src/iceberg/update/pending_update.h | 15 +- src/iceberg/update/replace_sort_order.cc | 100 +++++++ src/iceberg/update/replace_sort_order.h | 75 ++++++ src/iceberg/update/update_properties.cc | 27 +- src/iceberg/update/update_properties.h | 13 +- 24 files changed, 558 insertions(+), 102 deletions(-) create mode 100644 src/iceberg/test/replace_sort_order_test.cc create mode 100644 src/iceberg/update/replace_sort_order.cc create mode 100644 src/iceberg/update/replace_sort_order.h diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index 0579c67d2..70937996b 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -77,6 +77,7 @@ set(ICEBERG_SOURCES transform_function.cc type.cc update/pending_update.cc + update/replace-sort-order.cc update/update_properties.cc util/bucket_util.cc util/conversions.cc diff --git a/src/iceberg/expression/term.h b/src/iceberg/expression/term.h index 5e834af51..b50ea9242 100644 --- a/src/iceberg/expression/term.h +++ b/src/iceberg/expression/term.h @@ -151,6 +151,8 @@ class ICEBERG_EXPORT BoundReference std::shared_ptr type() const override { return field_.type(); } + int32_t field_id() const { return field_.field_id(); } + bool MayProduceNull() const override { return field_.optional(); } bool Equals(const BoundTerm& other) const override; diff --git a/src/iceberg/meson.build b/src/iceberg/meson.build index 850f65905..5e0bcbf07 100644 --- a/src/iceberg/meson.build +++ b/src/iceberg/meson.build @@ -99,6 +99,7 @@ iceberg_sources = files( 'transform_function.cc', 'type.cc', 'update/pending_update.cc', + 'update/replace_sort_order.cc', 'update/update_properties.cc', 'util/bucket_util.cc', 'util/conversions.cc', diff --git a/src/iceberg/sort_order.cc b/src/iceberg/sort_order.cc index 71e5e18df..355437e6c 100644 --- a/src/iceberg/sort_order.cc +++ b/src/iceberg/sort_order.cc @@ -111,7 +111,9 @@ Result> SortOrder::Make(const Schema& schema, int32_t } if (fields.empty() && sort_id != kUnsortedOrderId) [[unlikely]] { - return InvalidArgument("Sort order must have at least one sort field"); + return InvalidArgument( + "Sorted order must have at least one sort field. Use SortOrder::Unsorted() to " + "create an unsorted order"); } auto sort_order = std::unique_ptr(new SortOrder(sort_id, std::move(fields))); diff --git a/src/iceberg/table.cc b/src/iceberg/table.cc index b5a1e582e..2f963d19a 100644 --- a/src/iceberg/table.cc +++ b/src/iceberg/table.cc @@ -154,6 +154,13 @@ Result> Table::NewUpdateProperties() { return transaction->NewUpdateProperties(); } +Result> Table::NewReplaceSortOrder() { + ICEBERG_ASSIGN_OR_RAISE( + auto transaction, Transaction::Make(shared_from_this(), Transaction::Kind::kUpdate, + /*auto_commit=*/true)); + return transaction->NewReplaceSortOrder(); +} + Result> StagedTable::Make( TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, diff --git a/src/iceberg/table.h b/src/iceberg/table.h index efe175828..c65f435fa 100644 --- a/src/iceberg/table.h +++ b/src/iceberg/table.h @@ -132,6 +132,10 @@ class ICEBERG_EXPORT Table : public std::enable_shared_from_this { /// changes. virtual Result> NewUpdateProperties(); + /// \brief Create a new ReplaceSortOrder to replace the table sort order and commit the + /// changes. + virtual Result> NewReplaceSortOrder(); + protected: Table(TableIdentifier identifier, std::shared_ptr metadata, std::string metadata_location, std::shared_ptr io, diff --git a/src/iceberg/table_metadata.h b/src/iceberg/table_metadata.h index ce7975c6e..daaada6e6 100644 --- a/src/iceberg/table_metadata.h +++ b/src/iceberg/table_metadata.h @@ -394,7 +394,7 @@ class ICEBERG_EXPORT TableMetadataBuilder : public ErrorCollector { /// /// \param removed Set of property keys to remove /// \return Reference to this builder for method chaining - TableMetadataBuilder& RemoveProperties(const std::vector& removed); + TableMetadataBuilder& RemoveProperties(const std::unordered_set& removed); /// \brief Set the table location /// diff --git a/src/iceberg/table_update.h b/src/iceberg/table_update.h index a040cb36c..71db517b8 100644 --- a/src/iceberg/table_update.h +++ b/src/iceberg/table_update.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include "iceberg/iceberg_export.h" @@ -379,10 +380,10 @@ class ICEBERG_EXPORT SetProperties : public TableUpdate { /// \brief Represents removing table properties class ICEBERG_EXPORT RemoveProperties : public TableUpdate { public: - explicit RemoveProperties(std::vector removed) + explicit RemoveProperties(std::unordered_set removed) : removed_(std::move(removed)) {} - const std::vector& removed() const { return removed_; } + const std::unordered_set& removed() const { return removed_; } void ApplyTo(TableMetadataBuilder& builder) const override; @@ -391,7 +392,7 @@ class ICEBERG_EXPORT RemoveProperties : public TableUpdate { Kind kind() const override { return Kind::kRemoveProperties; } private: - std::vector removed_; + std::unordered_set removed_; }; /// \brief Represents setting the table location diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index fef63efee..0c5820485 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -154,7 +154,8 @@ if(ICEBERG_BUILD_BUNDLE) USE_BUNDLE SOURCES transaction_test.cc - update_properties_test.cc) + update_properties_test.cc + replace_sort_order_test.cc) endif() diff --git a/src/iceberg/test/replace_sort_order_test.cc b/src/iceberg/test/replace_sort_order_test.cc new file mode 100644 index 000000000..0a54eef11 --- /dev/null +++ b/src/iceberg/test/replace_sort_order_test.cc @@ -0,0 +1,272 @@ +/* + * 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/replace_sort_order.h" + +#include +#include + +#include +#include + +#include "iceberg/expression/term.h" +#include "iceberg/result.h" +#include "iceberg/schema.h" +#include "iceberg/schema_field.h" +#include "iceberg/sort_field.h" +#include "iceberg/table_metadata.h" +#include "iceberg/test/matchers.h" +#include "iceberg/test/update_test_base.h" +#include "iceberg/transaction.h" +#include "iceberg/transform.h" + +namespace iceberg { + +class ReplaceSortOrderTest : public UpdateTestBase {}; + +TEST_F(ReplaceSortOrderTest, AddSingleSortFieldAscending) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); + auto ref = NamedReference::Make("x").value(); + auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); + + update->AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst); + EXPECT_THAT(update->Commit(), IsOk()); + + // Verify the sort order was set + ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); + ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->metadata()->SortOrder()); + ASSERT_NE(sort_order, nullptr); + EXPECT_EQ(sort_order->fields().size(), 1); + + const auto& field = sort_order->fields()[0]; + EXPECT_EQ(field.source_id(), 1); + EXPECT_EQ(field.direction(), SortDirection::kAscending); + EXPECT_EQ(field.null_order(), NullOrder::kFirst); +} + +TEST_F(ReplaceSortOrderTest, AddSingleSortFieldDescending) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); + auto ref = NamedReference::Make("y").value(); + auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); + + update->AddSortField(std::move(term), SortDirection::kDescending, NullOrder::kLast); + EXPECT_THAT(update->Commit(), IsOk()); + + // Verify the sort order was set + ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); + ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->metadata()->SortOrder()); + ASSERT_NE(sort_order, nullptr); + EXPECT_EQ(sort_order->fields().size(), 1); + + const auto& field = sort_order->fields()[0]; + EXPECT_EQ(field.source_id(), 2); + EXPECT_EQ(field.direction(), SortDirection::kDescending); + EXPECT_EQ(field.null_order(), NullOrder::kLast); +} + +TEST_F(ReplaceSortOrderTest, AddMultipleSortFields) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); + auto ref1 = NamedReference::Make("y").value(); + auto term1 = UnboundTransform::Make(std::move(ref1), Transform::Identity()).value(); + + auto ref2 = NamedReference::Make("x").value(); + auto term2 = UnboundTransform::Make(std::move(ref2), Transform::Identity()).value(); + + update->AddSortField(std::move(term1), SortDirection::kAscending, NullOrder::kFirst) + .AddSortField(std::move(term2), SortDirection::kDescending, NullOrder::kLast); + + EXPECT_THAT(update->Commit(), IsOk()); + + // Verify the sort order was set with multiple fields + ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); + ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->metadata()->SortOrder()); + ASSERT_NE(sort_order, nullptr); + EXPECT_EQ(sort_order->fields().size(), 2); + + // Check first field (y field) + const auto& field1 = sort_order->fields()[0]; + EXPECT_EQ(field1.source_id(), 2); + EXPECT_EQ(field1.direction(), SortDirection::kAscending); + EXPECT_EQ(field1.null_order(), NullOrder::kFirst); + + // Check second field (x field) + const auto& field2 = sort_order->fields()[1]; + EXPECT_EQ(field2.source_id(), 1); + EXPECT_EQ(field2.direction(), SortDirection::kDescending); + EXPECT_EQ(field2.null_order(), NullOrder::kLast); +} + +TEST_F(ReplaceSortOrderTest, AddSortFieldWithTruncateTransform) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); + + auto ref = NamedReference::Make("x").value(); + auto term = UnboundTransform::Make(std::move(ref), Transform::Truncate(10)).value(); + + update->AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst); + EXPECT_THAT(update->Commit(), IsOk()); + + // Verify the sort order uses truncate transform + ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); + ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->metadata()->SortOrder()); + ASSERT_NE(sort_order, nullptr); + EXPECT_EQ(sort_order->fields().size(), 1); + + const auto& field = sort_order->fields()[0]; + EXPECT_EQ(field.source_id(), 1); + EXPECT_EQ(field.transform()->ToString(), "truncate[10]"); + EXPECT_EQ(field.direction(), SortDirection::kAscending); +} + +TEST_F(ReplaceSortOrderTest, AddSortFieldWithBucketTransform) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); + + auto ref = NamedReference::Make("y").value(); + auto term = UnboundTransform::Make(std::move(ref), Transform::Bucket(10)).value(); + + update->AddSortField(std::move(term), SortDirection::kDescending, NullOrder::kLast); + EXPECT_THAT(update->Commit(), IsOk()); + + // Verify the sort order uses bucket transform + ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); + ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->metadata()->SortOrder()); + ASSERT_NE(sort_order, nullptr); + EXPECT_EQ(sort_order->fields().size(), 1); + + const auto& field = sort_order->fields()[0]; + EXPECT_EQ(field.source_id(), 2); + EXPECT_EQ(field.transform()->ToString(), "bucket[10]"); + EXPECT_EQ(field.direction(), SortDirection::kDescending); +} + +TEST_F(ReplaceSortOrderTest, AddSortFieldNullTerm) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); + + update->AddSortField(nullptr, SortDirection::kAscending, NullOrder::kFirst); + + auto result = update->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("Term cannot be null")); +} + +TEST_F(ReplaceSortOrderTest, AddSortFieldInvalidTransform) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); + + // Try to apply day transform to a long field (invalid - day requires date/timestamp) + auto ref = NamedReference::Make("x").value(); + auto term = UnboundTransform::Make(std::move(ref), Transform::Day()).value(); + + update->AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst); + + auto result = update->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("not a valid input type")); +} + +TEST_F(ReplaceSortOrderTest, AddSortFieldNonExistentField) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); + + auto ref = NamedReference::Make("nonexistent").value(); + auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); + + update->AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst); + + auto result = update->Commit(); + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); + EXPECT_THAT(result, HasErrorMessage("Cannot find")); +} + +TEST_F(ReplaceSortOrderTest, CaseSensitiveTrue) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); + + auto ref = NamedReference::Make("X").value(); // Uppercase + auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); + + update->CaseSensitive(true).AddSortField(std::move(term), SortDirection::kAscending, + NullOrder::kFirst); + + auto result = update->Commit(); + // Should fail because schema has "x" (lowercase) + EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); +} + +TEST_F(ReplaceSortOrderTest, CaseSensitiveFalse) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); + auto ref = NamedReference::Make("X").value(); // Uppercase + auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); + + update->CaseSensitive(false).AddSortField(std::move(term), SortDirection::kAscending, + NullOrder::kFirst); + + auto result = update->Commit(); + // Should succeed because case-insensitive matching + EXPECT_THAT(result, IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); + ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->metadata()->SortOrder()); + ASSERT_NE(sort_order, nullptr); + EXPECT_EQ(sort_order->fields().size(), 1); + EXPECT_EQ(sort_order->fields()[0].source_id(), 1); +} + +TEST_F(ReplaceSortOrderTest, ReplaceExistingSortOrder) { + // First, set an initial sort order + ICEBERG_UNWRAP_OR_FAIL(auto update1, table_->NewReplaceSortOrder()); + + auto ref1 = NamedReference::Make("x").value(); + auto term1 = UnboundTransform::Make(std::move(ref1), Transform::Identity()).value(); + update1->AddSortField(std::move(term1), SortDirection::kAscending, NullOrder::kFirst); + EXPECT_THAT(update1->Commit(), IsOk()); + + // Now replace with a different sort order + ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); + + ICEBERG_UNWRAP_OR_FAIL(auto update2, reloaded->NewReplaceSortOrder()); + auto ref2 = NamedReference::Make("y").value(); + auto term2 = UnboundTransform::Make(std::move(ref2), Transform::Identity()).value(); + update2->AddSortField(std::move(term2), SortDirection::kDescending, NullOrder::kLast); + EXPECT_THAT(update2->Commit(), IsOk()); + + // Verify the sort order was replaced + ICEBERG_UNWRAP_OR_FAIL(auto final_table, catalog_->LoadTable(table_ident_)); + ICEBERG_UNWRAP_OR_FAIL(auto sort_order, final_table->metadata()->SortOrder()); + ASSERT_NE(sort_order, nullptr); + EXPECT_EQ(sort_order->fields().size(), 1); + + const auto& field = sort_order->fields()[0]; + EXPECT_EQ(field.source_id(), 2); + EXPECT_EQ(field.direction(), SortDirection::kDescending); + EXPECT_EQ(field.null_order(), NullOrder::kLast); +} + +TEST_F(ReplaceSortOrderTest, EmptySortOrder) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); + + // Don't add any sort fields + auto result = update->Commit(); + + // Should succeed with an unsorted order + EXPECT_THAT(result, IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); + ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->metadata()->SortOrder()); + ASSERT_NE(sort_order, nullptr); + EXPECT_TRUE(sort_order->fields().empty()); +} + +} // namespace iceberg diff --git a/src/iceberg/test/table_requirements_test.cc b/src/iceberg/test/table_requirements_test.cc index 041b44dd1..3278eb883 100644 --- a/src/iceberg/test/table_requirements_test.cc +++ b/src/iceberg/test/table_requirements_test.cc @@ -939,7 +939,7 @@ TEST(TableRequirementsTest, RemoveProperties) { std::vector> updates; updates.push_back( - std::make_unique(std::vector{"test"})); + std::make_unique(std::unordered_set{"test"})); auto result = TableRequirements::ForUpdateTable(*metadata, updates); ASSERT_THAT(result, IsOk()); diff --git a/src/iceberg/test/table_update_test.cc b/src/iceberg/test/table_update_test.cc index afa70894c..041cfcd23 100644 --- a/src/iceberg/test/table_update_test.cc +++ b/src/iceberg/test/table_update_test.cc @@ -175,7 +175,7 @@ INSTANTIATE_TEST_SUITE_P( .update_factory = [] { return std::make_unique( - std::vector{"key"}); + std::unordered_set{"key"}); }, .expected_existing_table_count = 0, .validator = nullptr}, diff --git a/src/iceberg/test/transaction_test.cc b/src/iceberg/test/transaction_test.cc index b8a55d83c..97cf84cb2 100644 --- a/src/iceberg/test/transaction_test.cc +++ b/src/iceberg/test/transaction_test.cc @@ -19,8 +19,6 @@ #include "iceberg/transaction.h" -#include "iceberg/table.h" -#include "iceberg/table_update.h" #include "iceberg/test/matchers.h" #include "iceberg/test/update_test_base.h" #include "iceberg/update/update_properties.h" @@ -35,14 +33,6 @@ TEST_F(TransactionTest, CreateTransaction) { EXPECT_EQ(txn->table(), table_); } -TEST_F(TransactionTest, UpdatePropertiesInTransaction) { - ICEBERG_UNWRAP_OR_FAIL(auto txn, table_->NewTransaction()); - ICEBERG_UNWRAP_OR_FAIL(auto update, txn->NewUpdateProperties()); - - update->Set("key1", "value1"); - EXPECT_THAT(update->Apply(), IsOk()); -} - TEST_F(TransactionTest, CommitEmptyTransaction) { ICEBERG_UNWRAP_OR_FAIL(auto txn, table_->NewTransaction()); EXPECT_THAT(txn->Commit(), IsOk()); diff --git a/src/iceberg/test/update_properties_test.cc b/src/iceberg/test/update_properties_test.cc index 5350af422..6dac47bcb 100644 --- a/src/iceberg/test/update_properties_test.cc +++ b/src/iceberg/test/update_properties_test.cc @@ -19,7 +19,6 @@ #include "iceberg/update/update_properties.h" -#include "iceberg/table_update.h" #include "iceberg/test/matchers.h" #include "iceberg/test/update_test_base.h" @@ -27,13 +26,22 @@ namespace iceberg { class UpdatePropertiesTest : public UpdateTestBase {}; +TEST_F(UpdatePropertiesTest, EmptyUpdate) { + // commit an empty update, should succeed + ICEBERG_UNWRAP_OR_FAIL(auto empty_update, table_->NewUpdateProperties()); + EXPECT_THAT(empty_update->Commit(), IsOk()); +} + TEST_F(UpdatePropertiesTest, SetProperty) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); update->Set("key1", "value1").Set("key2", "value2"); + EXPECT_THAT(update->Commit(), IsOk()); - ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); - EXPECT_EQ(result.updates.size(), 1); - EXPECT_EQ(result.updates[0]->kind(), table::SetProperties::Kind::kSetProperties); + // Verify the properties were set + ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); + const auto& props = reloaded->properties().configs(); + EXPECT_EQ(props.at("key1"), "value1"); + EXPECT_EQ(props.at("key2"), "value2"); } TEST_F(UpdatePropertiesTest, RemoveProperty) { @@ -47,16 +55,20 @@ TEST_F(UpdatePropertiesTest, RemoveProperty) { ICEBERG_UNWRAP_OR_FAIL(auto update, reloaded->NewUpdateProperties()); update->Remove("key1").Remove("key2"); - ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); - EXPECT_EQ(result.updates.size(), 1); - EXPECT_EQ(result.updates[0]->kind(), table::RemoveProperties::Kind::kRemoveProperties); + EXPECT_THAT(update->Commit(), IsOk()); + + // Verify the properties were removed + ICEBERG_UNWRAP_OR_FAIL(auto final_table, catalog_->LoadTable(table_ident_)); + const auto& props = final_table->properties().configs(); + EXPECT_FALSE(props.contains("key1")); + EXPECT_FALSE(props.contains("key2")); } TEST_F(UpdatePropertiesTest, SetThenRemoveSameKey) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); update->Set("key1", "value1").Remove("key1"); - auto result = update->Apply(); + auto result = update->Commit(); EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); EXPECT_THAT(result, HasErrorMessage("already marked for update")); } @@ -65,7 +77,7 @@ TEST_F(UpdatePropertiesTest, RemoveThenSetSameKey) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); update->Remove("key1").Set("key1", "value1"); - auto result = update->Apply(); + auto result = update->Commit(); EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); EXPECT_THAT(result, HasErrorMessage("already marked for removal")); } @@ -83,19 +95,19 @@ TEST_F(UpdatePropertiesTest, SetAndRemoveDifferentKeys) { TEST_F(UpdatePropertiesTest, UpgradeFormatVersionValid) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); - update->Set("format-version", "2"); + update->Set("format-version", "3"); + EXPECT_THAT(update->Commit(), IsOk()); - ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); - EXPECT_EQ(result.updates.size(), 1); - EXPECT_EQ(result.updates[0]->kind(), - table::UpgradeFormatVersion::Kind::kUpgradeFormatVersion); + // Verify the format version was upgraded + ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); + EXPECT_EQ(reloaded->metadata()->format_version, 3); } TEST_F(UpdatePropertiesTest, UpgradeFormatVersionInvalidString) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); update->Set("format-version", "invalid"); - auto result = update->Apply(); + auto result = update->Commit(); EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); EXPECT_THAT(result, HasErrorMessage("Invalid format version")); } @@ -104,7 +116,7 @@ TEST_F(UpdatePropertiesTest, UpgradeFormatVersionOutOfRange) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); update->Set("format-version", "5000000000"); - auto result = update->Apply(); + auto result = update->Commit(); EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); EXPECT_THAT(result, HasErrorMessage("out of range")); } @@ -114,26 +126,9 @@ TEST_F(UpdatePropertiesTest, UpgradeFormatVersionUnsupported) { update->Set("format-version", std::to_string(TableMetadata::kSupportedTableFormatVersion + 1)); - auto result = update->Apply(); + auto result = update->Commit(); EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); EXPECT_THAT(result, HasErrorMessage("unsupported format version")); } -TEST_F(UpdatePropertiesTest, CommitSuccess) { - ICEBERG_UNWRAP_OR_FAIL(auto empty_update, table_->NewUpdateProperties()); - EXPECT_THAT(empty_update->Commit(), IsOk()); - - ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); - update->Set("new.property", "new.value"); - update->Set("format-version", "3"); - - EXPECT_THAT(update->Commit(), IsOk()); - - ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); - const auto& props = reloaded->properties().configs(); - EXPECT_EQ(props.at("new.property"), "new.value"); - const auto& format_version = reloaded->metadata()->format_version; - EXPECT_EQ(format_version, 3); -} - } // namespace iceberg diff --git a/src/iceberg/transaction.cc b/src/iceberg/transaction.cc index c8cf42d36..848e52204 100644 --- a/src/iceberg/transaction.cc +++ b/src/iceberg/transaction.cc @@ -19,12 +19,16 @@ */ #include "iceberg/transaction.h" +#include + #include "iceberg/catalog.h" #include "iceberg/table.h" #include "iceberg/table_metadata.h" #include "iceberg/table_requirement.h" #include "iceberg/table_requirements.h" #include "iceberg/table_update.h" +#include "iceberg/update/pending_update.h" +#include "iceberg/update/replace_sort_order.h" #include "iceberg/update/update_properties.h" #include "iceberg/util/macros.h" @@ -60,9 +64,26 @@ Status Transaction::AddUpdate(const std::shared_ptr& update) { return {}; } -Status Transaction::Apply(std::vector> updates) { - for (const auto& update : updates) { - update->ApplyTo(*metadata_builder_); +Status Transaction::Apply(PendingUpdate& update) { + switch (update.kind()) { + case PendingUpdate::Kind::kUpdateProperties: { + auto& update_properties = static_cast(update); + ICEBERG_ASSIGN_OR_RAISE(UpdateProperties::ApplyResult result, + update_properties.Apply()); + metadata_builder_->SetProperties(std::move(result.updates_)); + metadata_builder_->RemoveProperties(std::move(result.removals_)); + if (result.format_version_.has_value()) { + metadata_builder_->UpgradeFormatVersion(result.format_version_.value()); + } + } break; + case PendingUpdate::Kind::kReplaceSortOrder: { + auto& replace_sort_order = static_cast(update); + ICEBERG_ASSIGN_OR_RAISE(ReplaceSortOrder::ApplyResult result, + replace_sort_order.Apply()); + metadata_builder_->SetDefaultSortOrder(result.sort_order_); + } break; + default: + return InvalidArgument("Unsupported pending update kind"); } last_update_committed_ = true; @@ -119,4 +140,11 @@ Result> Transaction::NewUpdateProperties() { return update_properties; } +Result> Transaction::NewReplaceSortOrder() { + ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr replace_sort_order, + ReplaceSortOrder::Make(shared_from_this())); + ICEBERG_RETURN_UNEXPECTED(AddUpdate(replace_sort_order)); + return replace_sort_order; +} + } // namespace iceberg diff --git a/src/iceberg/transaction.h b/src/iceberg/transaction.h index 73c346833..27ffe5f00 100644 --- a/src/iceberg/transaction.h +++ b/src/iceberg/transaction.h @@ -60,13 +60,17 @@ class ICEBERG_EXPORT Transaction : public std::enable_shared_from_this> NewUpdateProperties(); + /// \brief Create a new ReplaceSortOrder to replace the table sort order and commit the + /// changes. + Result> NewReplaceSortOrder(); + private: Transaction(std::shared_ptr
table, Kind kind, bool auto_commit); Status AddUpdate(const std::shared_ptr& update); /// \brief Apply the pending changes to current table. - Status Apply(std::vector> updates); + Status Apply(PendingUpdate& updates); friend class PendingUpdate; // Need to access the Apply method. diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index 133a7043c..16228eaa9 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -178,6 +178,7 @@ class Transaction; /// \brief Update family. class PendingUpdate; class UpdateProperties; +class ReplaceSortOrder; /// ---------------------------------------------------------------------------- /// TODO: Forward declarations below are not added yet. diff --git a/src/iceberg/update/meson.build b/src/iceberg/update/meson.build index 38502b14e..41f5055df 100644 --- a/src/iceberg/update/meson.build +++ b/src/iceberg/update/meson.build @@ -16,6 +16,6 @@ # under the License. install_headers( - ['pending_update.h', 'update_properties.h'], + ['pending_update.h', 'replace_sort_order.h', 'update_properties.h'], subdir: 'iceberg/update', ) diff --git a/src/iceberg/update/pending_update.cc b/src/iceberg/update/pending_update.cc index 4dbc67884..535e7b41c 100644 --- a/src/iceberg/update/pending_update.cc +++ b/src/iceberg/update/pending_update.cc @@ -19,9 +19,7 @@ #include "iceberg/update/pending_update.h" -#include "iceberg/table_update.h" #include "iceberg/transaction.h" -#include "iceberg/util/macros.h" namespace iceberg { @@ -30,9 +28,6 @@ PendingUpdate::PendingUpdate(std::shared_ptr transaction) PendingUpdate::~PendingUpdate() = default; -Status PendingUpdate::Commit() { - ICEBERG_ASSIGN_OR_RAISE(auto apply_result, Apply()); - return transaction_->Apply(std::move(apply_result.updates)); -} +Status PendingUpdate::Commit() { return transaction_->Apply(*this); } } // namespace iceberg diff --git a/src/iceberg/update/pending_update.h b/src/iceberg/update/pending_update.h index c4618400d..d072e201a 100644 --- a/src/iceberg/update/pending_update.h +++ b/src/iceberg/update/pending_update.h @@ -43,25 +43,12 @@ class ICEBERG_EXPORT PendingUpdate : public ErrorCollector { public: enum class Kind : uint8_t { kUpdateProperties, + kReplaceSortOrder, }; /// \brief Return the kind of this pending update. virtual Kind kind() const = 0; - struct ApplyResult { - std::vector> updates; - }; - - /// \brief Apply the pending changes and return the uncommitted changes for validation. - /// - /// \note This does not result in a permanent update. - /// \return The uncommitted changes that would be committed by calling Commit(), or an - /// error: - /// - ValidationFailed: the pending changes cannot be applied to the current - /// metadata - /// - InvalidArgument: if pending changes are conflicting or invalid - virtual Result Apply() = 0; - /// \brief Apply the pending changes and commit. /// /// \return An OK status if the commit was successful, or an error: diff --git a/src/iceberg/update/replace_sort_order.cc b/src/iceberg/update/replace_sort_order.cc new file mode 100644 index 000000000..73903b2a1 --- /dev/null +++ b/src/iceberg/update/replace_sort_order.cc @@ -0,0 +1,100 @@ +/* + * 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/replace_sort_order.h" + +#include +#include +#include + +#include "iceberg/expression/term.h" +#include "iceberg/result.h" +#include "iceberg/sort_field.h" +#include "iceberg/sort_order.h" +#include "iceberg/table_metadata.h" +#include "iceberg/transaction.h" +#include "iceberg/util/checked_cast.h" +#include "iceberg/util/error_collector.h" +#include "iceberg/util/macros.h" + +namespace iceberg { + +Result> ReplaceSortOrder::Make( + std::shared_ptr transaction) { + if (!transaction) [[unlikely]] { + return InvalidArgument("Cannot create UpdateProperties without a transaction"); + } + return std::shared_ptr(new ReplaceSortOrder(std::move(transaction))); +} + +ReplaceSortOrder::ReplaceSortOrder(std::shared_ptr transaction) + : PendingUpdate(std::move(transaction)) {} + +ReplaceSortOrder::~ReplaceSortOrder() = default; + +ReplaceSortOrder& ReplaceSortOrder::AddSortField(std::shared_ptr term, + SortDirection direction, + NullOrder null_order) { + if (!term) { + return AddError(ErrorKind::kInvalidArgument, "Term cannot be null"); + } + if (term->kind() != Term::Kind::kTransform) { + return AddError(ErrorKind::kInvalidArgument, "Term must be a transform term"); + } + if (!term->is_unbound()) { + return AddError(ErrorKind::kInvalidArgument, "Term must be unbound"); + } + // use checked-cast to get UnboundTransform + auto unbound_transform = internal::checked_pointer_cast(term); + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto schema, transaction_->current().Schema()); + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto bound_term, + unbound_transform->Bind(*schema, case_sensitive_)); + + int32_t source_id = bound_term->reference()->field_id(); + sort_fields_.emplace_back(source_id, unbound_transform->transform(), direction, + null_order); + return *this; +} + +ReplaceSortOrder& ReplaceSortOrder::CaseSensitive(bool case_sensitive) { + case_sensitive_ = case_sensitive; + return *this; +} + +Result ReplaceSortOrder::Apply() { + ICEBERG_RETURN_UNEXPECTED(CheckErrors()); + + // If no sort fields are specified, return an unsorted order (ID = 0). + std::shared_ptr order; + if (sort_fields_.empty()) { + order = SortOrder::Unsorted(); + } else { + // Use kInitialSortOrderId (1) as a placeholder for non-empty sort orders. + // The actual sort order ID will be assigned by TableMetadataBuilder when + // the AddSortOrder update is applied. + ICEBERG_ASSIGN_OR_RAISE( + order, SortOrder::Make(SortOrder::kInitialSortOrderId, sort_fields_)); + } + + ICEBERG_ASSIGN_OR_RAISE(auto schema, transaction_->current().Schema()); + ICEBERG_RETURN_UNEXPECTED(order->Validate(*schema)); + return ApplyResult{std::move(order)}; +} + +} // namespace iceberg diff --git a/src/iceberg/update/replace_sort_order.h b/src/iceberg/update/replace_sort_order.h new file mode 100644 index 000000000..4b0f92cac --- /dev/null +++ b/src/iceberg/update/replace_sort_order.h @@ -0,0 +1,75 @@ +/* + * 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 "iceberg/expression/term.h" +#include "iceberg/iceberg_export.h" +#include "iceberg/sort_field.h" +#include "iceberg/sort_order.h" +#include "iceberg/type_fwd.h" +#include "iceberg/update/pending_update.h" + +namespace iceberg { + +/// \brief Replacing table sort order with a newly created order. +class ICEBERG_EXPORT ReplaceSortOrder : public PendingUpdate { + public: + static Result> Make( + std::shared_ptr transaction); + + ~ReplaceSortOrder() override; + + struct ApplyResult { + std::shared_ptr sort_order_; + }; + + /// \brief Add a sort field to the sort order. + /// + /// \param term A transform term referencing the field + /// \param direction The sort direction (ascending or descending) + /// \param null_order The null order (first or last) + /// \return Reference to this ReplaceSortOrder for chaining + ReplaceSortOrder& AddSortField(std::shared_ptr term, SortDirection direction, + NullOrder null_order); + + /// \brief Set case sensitivity of sort column name resolution. + /// + /// \param case_sensitive When true, column name resolution is case-sensitive + /// \return Reference to this ReplaceSortOrder for chaining + ReplaceSortOrder& CaseSensitive(bool case_sensitive); + + Kind kind() const final { return Kind::kReplaceSortOrder; } + + private: + Result Apply(); + + friend class Transaction; + + explicit ReplaceSortOrder(std::shared_ptr transaction); + + std::vector sort_fields_; + bool case_sensitive_ = true; + std::shared_ptr sort_order_; +}; + +} // namespace iceberg diff --git a/src/iceberg/update/update_properties.cc b/src/iceberg/update/update_properties.cc index 9cdd7b5a7..2dc1d3eb6 100644 --- a/src/iceberg/update/update_properties.cc +++ b/src/iceberg/update/update_properties.cc @@ -28,7 +28,6 @@ #include "iceberg/result.h" #include "iceberg/table_metadata.h" #include "iceberg/table_properties.h" -#include "iceberg/table_update.h" #include "iceberg/transaction.h" #include "iceberg/util/error_collector.h" #include "iceberg/util/macros.h" @@ -70,7 +69,7 @@ UpdateProperties& UpdateProperties::Remove(const std::string& key) { return *this; } -Result UpdateProperties::Apply() { +Result UpdateProperties::Apply() { ICEBERG_RETURN_UNEXPECTED(CheckErrors()); const auto& current_props = transaction_->current().properties.configs(); std::unordered_map new_properties; @@ -111,27 +110,9 @@ Result UpdateProperties::Apply() { ICEBERG_RETURN_UNEXPECTED( MetricsConfig::VerifyReferencedColumns(new_properties, *schema.value())); } - - ApplyResult result; - if (!updates_.empty()) { - result.updates.emplace_back(std::make_unique(updates_)); - } - if (!removals_.empty()) { - for (const auto& key : removals_) { - if (current_props.contains(key)) { - removals.push_back(key); - } - } - if (!removals.empty()) { - result.updates.emplace_back(std::make_unique(removals)); - } - } - if (format_version_.has_value()) { - result.updates.emplace_back( - std::make_unique(format_version_.value())); - }; - - return result; + return ApplyResult{.updates_ = std::move(new_properties), + .removals_ = std::move(removals_), + .format_version_ = format_version_}; } } // namespace iceberg diff --git a/src/iceberg/update/update_properties.h b/src/iceberg/update/update_properties.h index e0f578bea..0a94ad954 100644 --- a/src/iceberg/update/update_properties.h +++ b/src/iceberg/update/update_properties.h @@ -26,6 +26,7 @@ #include #include "iceberg/iceberg_export.h" +#include "iceberg/result.h" #include "iceberg/type_fwd.h" #include "iceberg/update/pending_update.h" @@ -39,6 +40,12 @@ class ICEBERG_EXPORT UpdateProperties : public PendingUpdate { ~UpdateProperties() override; + struct ApplyResult { + std::unordered_map updates_; + std::unordered_set removals_; + std::optional format_version_; + }; + /// \brief Sets a property key to a specified value. /// /// The key must not have been previously marked for removal and reserved property keys @@ -57,9 +64,11 @@ class ICEBERG_EXPORT UpdateProperties : public PendingUpdate { Kind kind() const final { return Kind::kUpdateProperties; } - Result Apply() final; - private: + Result Apply(); + + friend class Transaction; + explicit UpdateProperties(std::shared_ptr transaction); std::unordered_map updates_; From fc393eb11db69018a94c3e55992469326dbca992 Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Wed, 24 Dec 2025 10:48:26 +0800 Subject: [PATCH 2/8] 1 --- src/iceberg/CMakeLists.txt | 2 +- src/iceberg/update/replace_sort_order.h | 3 +++ src/iceberg/update/update_properties.h | 3 +++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index 70937996b..65f796e2b 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -77,7 +77,7 @@ set(ICEBERG_SOURCES transform_function.cc type.cc update/pending_update.cc - update/replace-sort-order.cc + update/replace_sort_order.cc update/update_properties.cc util/bucket_util.cc util/conversions.cc diff --git a/src/iceberg/update/replace_sort_order.h b/src/iceberg/update/replace_sort_order.h index 4b0f92cac..22c454ed5 100644 --- a/src/iceberg/update/replace_sort_order.h +++ b/src/iceberg/update/replace_sort_order.h @@ -29,6 +29,9 @@ #include "iceberg/type_fwd.h" #include "iceberg/update/pending_update.h" +/// \file iceberg/update/replace_sort_order.h +/// \brief Replaces the table sort order. + namespace iceberg { /// \brief Replacing table sort order with a newly created order. diff --git a/src/iceberg/update/update_properties.h b/src/iceberg/update/update_properties.h index 0a94ad954..b2397d6a6 100644 --- a/src/iceberg/update/update_properties.h +++ b/src/iceberg/update/update_properties.h @@ -30,6 +30,9 @@ #include "iceberg/type_fwd.h" #include "iceberg/update/pending_update.h" +/// \file iceberg/update/update_properties.h +/// \brief Updates table properties. + namespace iceberg { /// \brief Updates table properties. From c7d091405a5791e9ff46064337aa54c2f788a434 Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Wed, 24 Dec 2025 11:04:16 +0800 Subject: [PATCH 3/8] fix ci --- src/iceberg/CMakeLists.txt | 2 +- src/iceberg/meson.build | 2 +- src/iceberg/table.cc | 4 +- src/iceberg/table.h | 4 +- src/iceberg/test/CMakeLists.txt | 2 +- src/iceberg/test/sort_order_test.cc | 2 +- ...rder_test.cc => update_sort_order_test.cc} | 54 +++++++++---------- src/iceberg/transaction.cc | 20 +++---- src/iceberg/transaction.h | 4 +- src/iceberg/type_fwd.h | 2 +- src/iceberg/update/meson.build | 2 +- src/iceberg/update/pending_update.h | 2 +- ...ace_sort_order.cc => update_sort_order.cc} | 22 ++++---- ...place_sort_order.h => update_sort_order.h} | 26 ++++----- 14 files changed, 74 insertions(+), 74 deletions(-) rename src/iceberg/test/{replace_sort_order_test.cc => update_sort_order_test.cc} (84%) rename src/iceberg/update/{replace_sort_order.cc => update_sort_order.cc} (79%) rename src/iceberg/update/{replace_sort_order.h => update_sort_order.h} (70%) diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index 65f796e2b..dcc9372f5 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -77,7 +77,7 @@ set(ICEBERG_SOURCES transform_function.cc type.cc update/pending_update.cc - update/replace_sort_order.cc + update/update_sort_order.cc update/update_properties.cc util/bucket_util.cc util/conversions.cc diff --git a/src/iceberg/meson.build b/src/iceberg/meson.build index 5e0bcbf07..1596a2ead 100644 --- a/src/iceberg/meson.build +++ b/src/iceberg/meson.build @@ -99,8 +99,8 @@ iceberg_sources = files( 'transform_function.cc', 'type.cc', 'update/pending_update.cc', - 'update/replace_sort_order.cc', 'update/update_properties.cc', + 'update/update_sort_order.cc', 'util/bucket_util.cc', 'util/conversions.cc', 'util/decimal.cc', diff --git a/src/iceberg/table.cc b/src/iceberg/table.cc index 2f963d19a..38afdedd0 100644 --- a/src/iceberg/table.cc +++ b/src/iceberg/table.cc @@ -154,11 +154,11 @@ Result> Table::NewUpdateProperties() { return transaction->NewUpdateProperties(); } -Result> Table::NewReplaceSortOrder() { +Result> Table::NewUpdateSortOrder() { ICEBERG_ASSIGN_OR_RAISE( auto transaction, Transaction::Make(shared_from_this(), Transaction::Kind::kUpdate, /*auto_commit=*/true)); - return transaction->NewReplaceSortOrder(); + return transaction->NewUpdateSortOrder(); } Result> StagedTable::Make( diff --git a/src/iceberg/table.h b/src/iceberg/table.h index c65f435fa..317aea018 100644 --- a/src/iceberg/table.h +++ b/src/iceberg/table.h @@ -132,9 +132,9 @@ class ICEBERG_EXPORT Table : public std::enable_shared_from_this
{ /// changes. virtual Result> NewUpdateProperties(); - /// \brief Create a new ReplaceSortOrder to replace the table sort order and commit the + /// \brief Create a new UpdateSortOrder to update the table sort order and commit the /// changes. - virtual Result> NewReplaceSortOrder(); + virtual Result> NewUpdateSortOrder(); protected: Table(TableIdentifier identifier, std::shared_ptr metadata, diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index 0c5820485..0d36f64cb 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -155,7 +155,7 @@ if(ICEBERG_BUILD_BUNDLE) SOURCES transaction_test.cc update_properties_test.cc - replace_sort_order_test.cc) + update_sort_order_test.cc) endif() diff --git a/src/iceberg/test/sort_order_test.cc b/src/iceberg/test/sort_order_test.cc index d2bef580a..8e207317e 100644 --- a/src/iceberg/test/sort_order_test.cc +++ b/src/iceberg/test/sort_order_test.cc @@ -193,7 +193,7 @@ TEST_F(SortOrderTest, MakeInvalidSortOrderEmptyFields) { auto sort_order = SortOrder::Make(*schema_, 1, std::vector{}); EXPECT_THAT(sort_order, IsError(ErrorKind::kInvalidArgument)); EXPECT_THAT(sort_order, - HasErrorMessage("Sort order must have at least one sort field")); + HasErrorMessage("Sorted order must have at least one sort field")); } TEST_F(SortOrderTest, MakeInvalidSortOrderUnsortedId) { diff --git a/src/iceberg/test/replace_sort_order_test.cc b/src/iceberg/test/update_sort_order_test.cc similarity index 84% rename from src/iceberg/test/replace_sort_order_test.cc rename to src/iceberg/test/update_sort_order_test.cc index 0a54eef11..c5fff3c21 100644 --- a/src/iceberg/test/replace_sort_order_test.cc +++ b/src/iceberg/test/update_sort_order_test.cc @@ -17,7 +17,7 @@ * under the License. */ -#include "iceberg/update/replace_sort_order.h" +#include "iceberg/update/update_sort_order.h" #include #include @@ -38,10 +38,10 @@ namespace iceberg { -class ReplaceSortOrderTest : public UpdateTestBase {}; +class UpdateSortOrderTest : public UpdateTestBase {}; -TEST_F(ReplaceSortOrderTest, AddSingleSortFieldAscending) { - ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); +TEST_F(UpdateSortOrderTest, AddSingleSortFieldAscending) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto ref = NamedReference::Make("x").value(); auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); @@ -60,8 +60,8 @@ TEST_F(ReplaceSortOrderTest, AddSingleSortFieldAscending) { EXPECT_EQ(field.null_order(), NullOrder::kFirst); } -TEST_F(ReplaceSortOrderTest, AddSingleSortFieldDescending) { - ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); +TEST_F(UpdateSortOrderTest, AddSingleSortFieldDescending) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto ref = NamedReference::Make("y").value(); auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); @@ -80,8 +80,8 @@ TEST_F(ReplaceSortOrderTest, AddSingleSortFieldDescending) { EXPECT_EQ(field.null_order(), NullOrder::kLast); } -TEST_F(ReplaceSortOrderTest, AddMultipleSortFields) { - ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); +TEST_F(UpdateSortOrderTest, AddMultipleSortFields) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto ref1 = NamedReference::Make("y").value(); auto term1 = UnboundTransform::Make(std::move(ref1), Transform::Identity()).value(); @@ -112,8 +112,8 @@ TEST_F(ReplaceSortOrderTest, AddMultipleSortFields) { EXPECT_EQ(field2.null_order(), NullOrder::kLast); } -TEST_F(ReplaceSortOrderTest, AddSortFieldWithTruncateTransform) { - ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); +TEST_F(UpdateSortOrderTest, AddSortFieldWithTruncateTransform) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto ref = NamedReference::Make("x").value(); auto term = UnboundTransform::Make(std::move(ref), Transform::Truncate(10)).value(); @@ -133,8 +133,8 @@ TEST_F(ReplaceSortOrderTest, AddSortFieldWithTruncateTransform) { EXPECT_EQ(field.direction(), SortDirection::kAscending); } -TEST_F(ReplaceSortOrderTest, AddSortFieldWithBucketTransform) { - ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); +TEST_F(UpdateSortOrderTest, AddSortFieldWithBucketTransform) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto ref = NamedReference::Make("y").value(); auto term = UnboundTransform::Make(std::move(ref), Transform::Bucket(10)).value(); @@ -154,8 +154,8 @@ TEST_F(ReplaceSortOrderTest, AddSortFieldWithBucketTransform) { EXPECT_EQ(field.direction(), SortDirection::kDescending); } -TEST_F(ReplaceSortOrderTest, AddSortFieldNullTerm) { - ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); +TEST_F(UpdateSortOrderTest, AddSortFieldNullTerm) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); update->AddSortField(nullptr, SortDirection::kAscending, NullOrder::kFirst); @@ -164,8 +164,8 @@ TEST_F(ReplaceSortOrderTest, AddSortFieldNullTerm) { EXPECT_THAT(result, HasErrorMessage("Term cannot be null")); } -TEST_F(ReplaceSortOrderTest, AddSortFieldInvalidTransform) { - ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); +TEST_F(UpdateSortOrderTest, AddSortFieldInvalidTransform) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); // Try to apply day transform to a long field (invalid - day requires date/timestamp) auto ref = NamedReference::Make("x").value(); @@ -178,8 +178,8 @@ TEST_F(ReplaceSortOrderTest, AddSortFieldInvalidTransform) { EXPECT_THAT(result, HasErrorMessage("not a valid input type")); } -TEST_F(ReplaceSortOrderTest, AddSortFieldNonExistentField) { - ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); +TEST_F(UpdateSortOrderTest, AddSortFieldNonExistentField) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto ref = NamedReference::Make("nonexistent").value(); auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); @@ -191,8 +191,8 @@ TEST_F(ReplaceSortOrderTest, AddSortFieldNonExistentField) { EXPECT_THAT(result, HasErrorMessage("Cannot find")); } -TEST_F(ReplaceSortOrderTest, CaseSensitiveTrue) { - ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); +TEST_F(UpdateSortOrderTest, CaseSensitiveTrue) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto ref = NamedReference::Make("X").value(); // Uppercase auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); @@ -205,8 +205,8 @@ TEST_F(ReplaceSortOrderTest, CaseSensitiveTrue) { EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); } -TEST_F(ReplaceSortOrderTest, CaseSensitiveFalse) { - ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); +TEST_F(UpdateSortOrderTest, CaseSensitiveFalse) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto ref = NamedReference::Make("X").value(); // Uppercase auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); @@ -224,9 +224,9 @@ TEST_F(ReplaceSortOrderTest, CaseSensitiveFalse) { EXPECT_EQ(sort_order->fields()[0].source_id(), 1); } -TEST_F(ReplaceSortOrderTest, ReplaceExistingSortOrder) { +TEST_F(UpdateSortOrderTest, ReplaceExistingSortOrder) { // First, set an initial sort order - ICEBERG_UNWRAP_OR_FAIL(auto update1, table_->NewReplaceSortOrder()); + ICEBERG_UNWRAP_OR_FAIL(auto update1, table_->NewUpdateSortOrder()); auto ref1 = NamedReference::Make("x").value(); auto term1 = UnboundTransform::Make(std::move(ref1), Transform::Identity()).value(); @@ -236,7 +236,7 @@ TEST_F(ReplaceSortOrderTest, ReplaceExistingSortOrder) { // Now replace with a different sort order ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); - ICEBERG_UNWRAP_OR_FAIL(auto update2, reloaded->NewReplaceSortOrder()); + ICEBERG_UNWRAP_OR_FAIL(auto update2, reloaded->NewUpdateSortOrder()); auto ref2 = NamedReference::Make("y").value(); auto term2 = UnboundTransform::Make(std::move(ref2), Transform::Identity()).value(); update2->AddSortField(std::move(term2), SortDirection::kDescending, NullOrder::kLast); @@ -254,8 +254,8 @@ TEST_F(ReplaceSortOrderTest, ReplaceExistingSortOrder) { EXPECT_EQ(field.null_order(), NullOrder::kLast); } -TEST_F(ReplaceSortOrderTest, EmptySortOrder) { - ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewReplaceSortOrder()); +TEST_F(UpdateSortOrderTest, EmptySortOrder) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); // Don't add any sort fields auto result = update->Commit(); diff --git a/src/iceberg/transaction.cc b/src/iceberg/transaction.cc index 848e52204..427c404ef 100644 --- a/src/iceberg/transaction.cc +++ b/src/iceberg/transaction.cc @@ -28,8 +28,8 @@ #include "iceberg/table_requirements.h" #include "iceberg/table_update.h" #include "iceberg/update/pending_update.h" -#include "iceberg/update/replace_sort_order.h" #include "iceberg/update/update_properties.h" +#include "iceberg/update/update_sort_order.h" #include "iceberg/util/macros.h" namespace iceberg { @@ -76,10 +76,10 @@ Status Transaction::Apply(PendingUpdate& update) { metadata_builder_->UpgradeFormatVersion(result.format_version_.value()); } } break; - case PendingUpdate::Kind::kReplaceSortOrder: { - auto& replace_sort_order = static_cast(update); - ICEBERG_ASSIGN_OR_RAISE(ReplaceSortOrder::ApplyResult result, - replace_sort_order.Apply()); + case PendingUpdate::Kind::kUpdateSortOrder: { + auto& update_sort_order = static_cast(update); + ICEBERG_ASSIGN_OR_RAISE(UpdateSortOrder::ApplyResult result, + update_sort_order.Apply()); metadata_builder_->SetDefaultSortOrder(result.sort_order_); } break; default: @@ -140,11 +140,11 @@ Result> Transaction::NewUpdateProperties() { return update_properties; } -Result> Transaction::NewReplaceSortOrder() { - ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr replace_sort_order, - ReplaceSortOrder::Make(shared_from_this())); - ICEBERG_RETURN_UNEXPECTED(AddUpdate(replace_sort_order)); - return replace_sort_order; +Result> Transaction::NewUpdateSortOrder() { + ICEBERG_ASSIGN_OR_RAISE(std::shared_ptr update_sort_order, + UpdateSortOrder::Make(shared_from_this())); + ICEBERG_RETURN_UNEXPECTED(AddUpdate(update_sort_order)); + return update_sort_order; } } // namespace iceberg diff --git a/src/iceberg/transaction.h b/src/iceberg/transaction.h index 27ffe5f00..05fdea325 100644 --- a/src/iceberg/transaction.h +++ b/src/iceberg/transaction.h @@ -60,9 +60,9 @@ class ICEBERG_EXPORT Transaction : public std::enable_shared_from_this> NewUpdateProperties(); - /// \brief Create a new ReplaceSortOrder to replace the table sort order and commit the + /// \brief Create a new UpdateSortOrder to update the table sort order and commit the /// changes. - Result> NewReplaceSortOrder(); + Result> NewUpdateSortOrder(); private: Transaction(std::shared_ptr
table, Kind kind, bool auto_commit); diff --git a/src/iceberg/type_fwd.h b/src/iceberg/type_fwd.h index 16228eaa9..8ca444ccd 100644 --- a/src/iceberg/type_fwd.h +++ b/src/iceberg/type_fwd.h @@ -178,7 +178,7 @@ class Transaction; /// \brief Update family. class PendingUpdate; class UpdateProperties; -class ReplaceSortOrder; +class UpdateSortOrder; /// ---------------------------------------------------------------------------- /// TODO: Forward declarations below are not added yet. diff --git a/src/iceberg/update/meson.build b/src/iceberg/update/meson.build index 41f5055df..b0ec67066 100644 --- a/src/iceberg/update/meson.build +++ b/src/iceberg/update/meson.build @@ -16,6 +16,6 @@ # under the License. install_headers( - ['pending_update.h', 'replace_sort_order.h', 'update_properties.h'], + ['pending_update.h', 'update_sort_order.h', 'update_properties.h'], subdir: 'iceberg/update', ) diff --git a/src/iceberg/update/pending_update.h b/src/iceberg/update/pending_update.h index d072e201a..c298ba800 100644 --- a/src/iceberg/update/pending_update.h +++ b/src/iceberg/update/pending_update.h @@ -43,7 +43,7 @@ class ICEBERG_EXPORT PendingUpdate : public ErrorCollector { public: enum class Kind : uint8_t { kUpdateProperties, - kReplaceSortOrder, + kUpdateSortOrder, }; /// \brief Return the kind of this pending update. diff --git a/src/iceberg/update/replace_sort_order.cc b/src/iceberg/update/update_sort_order.cc similarity index 79% rename from src/iceberg/update/replace_sort_order.cc rename to src/iceberg/update/update_sort_order.cc index 73903b2a1..36cb349df 100644 --- a/src/iceberg/update/replace_sort_order.cc +++ b/src/iceberg/update/update_sort_order.cc @@ -17,7 +17,7 @@ * under the License. */ -#include "iceberg/update/replace_sort_order.h" +#include "iceberg/update/update_sort_order.h" #include #include @@ -35,22 +35,22 @@ namespace iceberg { -Result> ReplaceSortOrder::Make( +Result> UpdateSortOrder::Make( std::shared_ptr transaction) { if (!transaction) [[unlikely]] { - return InvalidArgument("Cannot create UpdateProperties without a transaction"); + return InvalidArgument("Cannot create UpdateSortOrder without a transaction"); } - return std::shared_ptr(new ReplaceSortOrder(std::move(transaction))); + return std::shared_ptr(new UpdateSortOrder(std::move(transaction))); } -ReplaceSortOrder::ReplaceSortOrder(std::shared_ptr transaction) +UpdateSortOrder::UpdateSortOrder(std::shared_ptr transaction) : PendingUpdate(std::move(transaction)) {} -ReplaceSortOrder::~ReplaceSortOrder() = default; +UpdateSortOrder::~UpdateSortOrder() = default; -ReplaceSortOrder& ReplaceSortOrder::AddSortField(std::shared_ptr term, - SortDirection direction, - NullOrder null_order) { +UpdateSortOrder& UpdateSortOrder::AddSortField(std::shared_ptr term, + SortDirection direction, + NullOrder null_order) { if (!term) { return AddError(ErrorKind::kInvalidArgument, "Term cannot be null"); } @@ -72,12 +72,12 @@ ReplaceSortOrder& ReplaceSortOrder::AddSortField(std::shared_ptr term, return *this; } -ReplaceSortOrder& ReplaceSortOrder::CaseSensitive(bool case_sensitive) { +UpdateSortOrder& UpdateSortOrder::CaseSensitive(bool case_sensitive) { case_sensitive_ = case_sensitive; return *this; } -Result ReplaceSortOrder::Apply() { +Result UpdateSortOrder::Apply() { ICEBERG_RETURN_UNEXPECTED(CheckErrors()); // If no sort fields are specified, return an unsorted order (ID = 0). diff --git a/src/iceberg/update/replace_sort_order.h b/src/iceberg/update/update_sort_order.h similarity index 70% rename from src/iceberg/update/replace_sort_order.h rename to src/iceberg/update/update_sort_order.h index 22c454ed5..4404d6e44 100644 --- a/src/iceberg/update/replace_sort_order.h +++ b/src/iceberg/update/update_sort_order.h @@ -29,18 +29,18 @@ #include "iceberg/type_fwd.h" #include "iceberg/update/pending_update.h" -/// \file iceberg/update/replace_sort_order.h -/// \brief Replaces the table sort order. +/// \file iceberg/update/update_sort_order.h +/// \brief Updates the table sort order. namespace iceberg { -/// \brief Replacing table sort order with a newly created order. -class ICEBERG_EXPORT ReplaceSortOrder : public PendingUpdate { +/// \brief Updating table sort order with a newly created order. +class ICEBERG_EXPORT UpdateSortOrder : public PendingUpdate { public: - static Result> Make( + static Result> Make( std::shared_ptr transaction); - ~ReplaceSortOrder() override; + ~UpdateSortOrder() override; struct ApplyResult { std::shared_ptr sort_order_; @@ -51,24 +51,24 @@ class ICEBERG_EXPORT ReplaceSortOrder : public PendingUpdate { /// \param term A transform term referencing the field /// \param direction The sort direction (ascending or descending) /// \param null_order The null order (first or last) - /// \return Reference to this ReplaceSortOrder for chaining - ReplaceSortOrder& AddSortField(std::shared_ptr term, SortDirection direction, - NullOrder null_order); + /// \return Reference to this UpdateSortOrder for chaining + UpdateSortOrder& AddSortField(std::shared_ptr term, SortDirection direction, + NullOrder null_order); /// \brief Set case sensitivity of sort column name resolution. /// /// \param case_sensitive When true, column name resolution is case-sensitive - /// \return Reference to this ReplaceSortOrder for chaining - ReplaceSortOrder& CaseSensitive(bool case_sensitive); + /// \return Reference to this UpdateSortOrder for chaining + UpdateSortOrder& CaseSensitive(bool case_sensitive); - Kind kind() const final { return Kind::kReplaceSortOrder; } + Kind kind() const final { return Kind::kUpdateSortOrder; } private: Result Apply(); friend class Transaction; - explicit ReplaceSortOrder(std::shared_ptr transaction); + explicit UpdateSortOrder(std::shared_ptr transaction); std::vector sort_fields_; bool case_sensitive_ = true; From e78bbc85fa77f3b4f3158b73854df8bd7d3206eb Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Wed, 24 Dec 2025 11:11:39 +0800 Subject: [PATCH 4/8] 1 --- src/iceberg/test/transaction_test.cc | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/iceberg/test/transaction_test.cc b/src/iceberg/test/transaction_test.cc index 97cf84cb2..5bd1cdd25 100644 --- a/src/iceberg/test/transaction_test.cc +++ b/src/iceberg/test/transaction_test.cc @@ -19,9 +19,12 @@ #include "iceberg/transaction.h" +#include "iceberg/expression/term.h" #include "iceberg/test/matchers.h" #include "iceberg/test/update_test_base.h" +#include "iceberg/transform.h" #include "iceberg/update/update_properties.h" +#include "iceberg/update/update_sort_order.h" namespace iceberg { @@ -57,24 +60,35 @@ TEST_F(TransactionTest, CommitTransactionWithPropertyUpdate) { TEST_F(TransactionTest, MultipleUpdatesInTransaction) { ICEBERG_UNWRAP_OR_FAIL(auto txn, table_->NewTransaction()); - // First update + // First update: set property ICEBERG_UNWRAP_OR_FAIL(auto update1, txn->NewUpdateProperties()); - update1->Set("key1", "value1"); + update1->Set("key1", "value1").Set("key2", "value2"); EXPECT_THAT(update1->Commit(), IsOk()); - // Second update - ICEBERG_UNWRAP_OR_FAIL(auto update2, txn->NewUpdateProperties()); - update2->Set("key2", "value2"); + // Second update: update sort order + ICEBERG_UNWRAP_OR_FAIL(auto update2, txn->NewUpdateSortOrder()); + auto ref = NamedReference::Make("x").value(); + auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); + update2->AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst); EXPECT_THAT(update2->Commit(), IsOk()); // Commit transaction ICEBERG_UNWRAP_OR_FAIL(auto updated_table, txn->Commit()); - // Verify both properties were set + // Verify properties were set ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); const auto& props = reloaded->properties().configs(); EXPECT_EQ(props.at("key1"), "value1"); EXPECT_EQ(props.at("key2"), "value2"); + + // Verify sort order was updated + ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->sort_order()); + EXPECT_FALSE(sort_order->is_unsorted()); + const auto& fields = sort_order->fields(); + ASSERT_EQ(fields.size(), 1); + EXPECT_EQ(fields[0].source_id(), 1); // field id for "x" + EXPECT_EQ(fields[0].direction(), SortDirection::kAscending); + EXPECT_EQ(fields[0].null_order(), NullOrder::kFirst); } } // namespace iceberg From e875ba5b762fdfa75e1093ed60e2474e8be8ab35 Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Wed, 24 Dec 2025 16:13:01 +0800 Subject: [PATCH 5/8] fix review --- src/iceberg/sort_order.cc | 4 +- src/iceberg/test/sort_order_test.cc | 2 +- src/iceberg/test/transaction_test.cc | 19 +- src/iceberg/test/update_properties_test.cc | 74 +++--- src/iceberg/test/update_sort_order_test.cc | 259 +++++++++++---------- src/iceberg/transaction.cc | 28 ++- src/iceberg/update/update_properties.cc | 10 +- src/iceberg/update/update_properties.h | 9 +- src/iceberg/update/update_sort_order.cc | 67 +++--- src/iceberg/update/update_sort_order.h | 22 +- 10 files changed, 266 insertions(+), 228 deletions(-) diff --git a/src/iceberg/sort_order.cc b/src/iceberg/sort_order.cc index 355437e6c..fca138a6c 100644 --- a/src/iceberg/sort_order.cc +++ b/src/iceberg/sort_order.cc @@ -111,9 +111,7 @@ Result> SortOrder::Make(const Schema& schema, int32_t } if (fields.empty() && sort_id != kUnsortedOrderId) [[unlikely]] { - return InvalidArgument( - "Sorted order must have at least one sort field. Use SortOrder::Unsorted() to " - "create an unsorted order"); + return InvalidArgument("Sort order must have at least one sort field."); } auto sort_order = std::unique_ptr(new SortOrder(sort_id, std::move(fields))); diff --git a/src/iceberg/test/sort_order_test.cc b/src/iceberg/test/sort_order_test.cc index 8e207317e..d2bef580a 100644 --- a/src/iceberg/test/sort_order_test.cc +++ b/src/iceberg/test/sort_order_test.cc @@ -193,7 +193,7 @@ TEST_F(SortOrderTest, MakeInvalidSortOrderEmptyFields) { auto sort_order = SortOrder::Make(*schema_, 1, std::vector{}); EXPECT_THAT(sort_order, IsError(ErrorKind::kInvalidArgument)); EXPECT_THAT(sort_order, - HasErrorMessage("Sorted order must have at least one sort field")); + HasErrorMessage("Sort order must have at least one sort field")); } TEST_F(SortOrderTest, MakeInvalidSortOrderUnsortedId) { diff --git a/src/iceberg/test/transaction_test.cc b/src/iceberg/test/transaction_test.cc index 5bd1cdd25..232febc1f 100644 --- a/src/iceberg/test/transaction_test.cc +++ b/src/iceberg/test/transaction_test.cc @@ -19,7 +19,9 @@ #include "iceberg/transaction.h" +#include "iceberg/expression/expressions.h" #include "iceberg/expression/term.h" +#include "iceberg/sort_order.h" #include "iceberg/test/matchers.h" #include "iceberg/test/update_test_base.h" #include "iceberg/transform.h" @@ -67,8 +69,8 @@ TEST_F(TransactionTest, MultipleUpdatesInTransaction) { // Second update: update sort order ICEBERG_UNWRAP_OR_FAIL(auto update2, txn->NewUpdateSortOrder()); - auto ref = NamedReference::Make("x").value(); - auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); + auto term = + UnboundTransform::Make(Expressions::Ref("x"), Transform::Identity()).value(); update2->AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst); EXPECT_THAT(update2->Commit(), IsOk()); @@ -83,12 +85,13 @@ TEST_F(TransactionTest, MultipleUpdatesInTransaction) { // Verify sort order was updated ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->sort_order()); - EXPECT_FALSE(sort_order->is_unsorted()); - const auto& fields = sort_order->fields(); - ASSERT_EQ(fields.size(), 1); - EXPECT_EQ(fields[0].source_id(), 1); // field id for "x" - EXPECT_EQ(fields[0].direction(), SortDirection::kAscending); - EXPECT_EQ(fields[0].null_order(), NullOrder::kFirst); + std::vector expected_fields; + expected_fields.emplace_back(1, Transform::Identity(), SortDirection::kAscending, + NullOrder::kFirst); + ICEBERG_UNWRAP_OR_FAIL( + auto expected_sort_order, + SortOrder::Make(sort_order->order_id(), std::move(expected_fields))); + EXPECT_EQ(*sort_order, *expected_sort_order); } } // namespace iceberg diff --git a/src/iceberg/test/update_properties_test.cc b/src/iceberg/test/update_properties_test.cc index 6dac47bcb..737ba8ff2 100644 --- a/src/iceberg/test/update_properties_test.cc +++ b/src/iceberg/test/update_properties_test.cc @@ -28,20 +28,20 @@ class UpdatePropertiesTest : public UpdateTestBase {}; TEST_F(UpdatePropertiesTest, EmptyUpdate) { // commit an empty update, should succeed - ICEBERG_UNWRAP_OR_FAIL(auto empty_update, table_->NewUpdateProperties()); - EXPECT_THAT(empty_update->Commit(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); + EXPECT_THAT(result.updates.empty(), true); } TEST_F(UpdatePropertiesTest, SetProperty) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); update->Set("key1", "value1").Set("key2", "value2"); - EXPECT_THAT(update->Commit(), IsOk()); - // Verify the properties were set - ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); - const auto& props = reloaded->properties().configs(); - EXPECT_EQ(props.at("key1"), "value1"); - EXPECT_EQ(props.at("key2"), "value2"); + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); + EXPECT_EQ(result.updates.size(), 2); + EXPECT_EQ(result.updates.at("key1"), "value1"); + EXPECT_EQ(result.updates.at("key2"), "value2"); + EXPECT_TRUE(result.removals.empty()); } TEST_F(UpdatePropertiesTest, RemoveProperty) { @@ -55,20 +55,18 @@ TEST_F(UpdatePropertiesTest, RemoveProperty) { ICEBERG_UNWRAP_OR_FAIL(auto update, reloaded->NewUpdateProperties()); update->Remove("key1").Remove("key2"); - EXPECT_THAT(update->Commit(), IsOk()); - - // Verify the properties were removed - ICEBERG_UNWRAP_OR_FAIL(auto final_table, catalog_->LoadTable(table_ident_)); - const auto& props = final_table->properties().configs(); - EXPECT_FALSE(props.contains("key1")); - EXPECT_FALSE(props.contains("key2")); + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); + EXPECT_TRUE(result.updates.empty()); + EXPECT_EQ(result.removals.size(), 2); + EXPECT_TRUE(result.removals.contains("key1")); + EXPECT_TRUE(result.removals.contains("key2")); } TEST_F(UpdatePropertiesTest, SetThenRemoveSameKey) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); update->Set("key1", "value1").Remove("key1"); - auto result = update->Commit(); + auto result = update->Apply(); EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); EXPECT_THAT(result, HasErrorMessage("already marked for update")); } @@ -77,7 +75,7 @@ TEST_F(UpdatePropertiesTest, RemoveThenSetSameKey) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); update->Remove("key1").Set("key1", "value1"); - auto result = update->Commit(); + auto result = update->Apply(); EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); EXPECT_THAT(result, HasErrorMessage("already marked for removal")); } @@ -85,29 +83,30 @@ TEST_F(UpdatePropertiesTest, RemoveThenSetSameKey) { TEST_F(UpdatePropertiesTest, SetAndRemoveDifferentKeys) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); update->Set("key1", "value1").Remove("key2"); - EXPECT_THAT(update->Commit(), IsOk()); - ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); - const auto& props = reloaded->properties().configs(); - EXPECT_EQ(props.at("key1"), "value1"); - EXPECT_FALSE(props.contains("key2")); + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); + EXPECT_EQ(result.updates.size(), 1); + EXPECT_EQ(result.updates.at("key1"), "value1"); + EXPECT_EQ(result.removals.size(), 1); + EXPECT_TRUE(result.removals.contains("key2")); } TEST_F(UpdatePropertiesTest, UpgradeFormatVersionValid) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); update->Set("format-version", "3"); - EXPECT_THAT(update->Commit(), IsOk()); - // Verify the format version was upgraded - ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); - EXPECT_EQ(reloaded->metadata()->format_version, 3); + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); + EXPECT_TRUE(result.updates.empty()); + EXPECT_TRUE(result.removals.empty()); + ASSERT_TRUE(result.format_version.has_value()); + EXPECT_EQ(result.format_version.value(), 3); } TEST_F(UpdatePropertiesTest, UpgradeFormatVersionInvalidString) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); update->Set("format-version", "invalid"); - auto result = update->Commit(); + auto result = update->Apply(); EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); EXPECT_THAT(result, HasErrorMessage("Invalid format version")); } @@ -116,7 +115,7 @@ TEST_F(UpdatePropertiesTest, UpgradeFormatVersionOutOfRange) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); update->Set("format-version", "5000000000"); - auto result = update->Commit(); + auto result = update->Apply(); EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); EXPECT_THAT(result, HasErrorMessage("out of range")); } @@ -126,9 +125,26 @@ TEST_F(UpdatePropertiesTest, UpgradeFormatVersionUnsupported) { update->Set("format-version", std::to_string(TableMetadata::kSupportedTableFormatVersion + 1)); - auto result = update->Commit(); + auto result = update->Apply(); EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument)); EXPECT_THAT(result, HasErrorMessage("unsupported format version")); } +TEST_F(UpdatePropertiesTest, CommitSuccess) { + ICEBERG_UNWRAP_OR_FAIL(auto empty_update, table_->NewUpdateProperties()); + EXPECT_THAT(empty_update->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); + update->Set("new.property", "new.value"); + update->Set("format-version", "3"); + + EXPECT_THAT(update->Commit(), IsOk()); + + ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); + const auto& props = reloaded->properties().configs(); + EXPECT_EQ(props.at("new.property"), "new.value"); + const auto& format_version = reloaded->metadata()->format_version; + EXPECT_EQ(format_version, 3); +} + } // namespace iceberg diff --git a/src/iceberg/test/update_sort_order_test.cc b/src/iceberg/test/update_sort_order_test.cc index c5fff3c21..2c6238985 100644 --- a/src/iceberg/test/update_sort_order_test.cc +++ b/src/iceberg/test/update_sort_order_test.cc @@ -25,15 +25,13 @@ #include #include +#include "iceberg/expression/expressions.h" #include "iceberg/expression/term.h" #include "iceberg/result.h" -#include "iceberg/schema.h" -#include "iceberg/schema_field.h" #include "iceberg/sort_field.h" -#include "iceberg/table_metadata.h" +#include "iceberg/sort_order.h" #include "iceberg/test/matchers.h" #include "iceberg/test/update_test_base.h" -#include "iceberg/transaction.h" #include "iceberg/transform.h" namespace iceberg { @@ -42,116 +40,128 @@ class UpdateSortOrderTest : public UpdateTestBase {}; TEST_F(UpdateSortOrderTest, AddSingleSortFieldAscending) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); - auto ref = NamedReference::Make("x").value(); - auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); + auto term = Expressions::Transform("x", Transform::Identity()); - update->AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst); - EXPECT_THAT(update->Commit(), IsOk()); + update->AddSortField(term, SortDirection::kAscending, NullOrder::kFirst); - // Verify the sort order was set - ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); - ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->metadata()->SortOrder()); - ASSERT_NE(sort_order, nullptr); - EXPECT_EQ(sort_order->fields().size(), 1); - - const auto& field = sort_order->fields()[0]; - EXPECT_EQ(field.source_id(), 1); - EXPECT_EQ(field.direction(), SortDirection::kAscending); - EXPECT_EQ(field.null_order(), NullOrder::kFirst); + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); + EXPECT_FALSE(result.sort_order->is_unsorted()); + + std::vector expected_fields; + expected_fields.emplace_back(1, Transform::Identity(), SortDirection::kAscending, + NullOrder::kFirst); + ICEBERG_UNWRAP_OR_FAIL( + auto expected_sort_order, + SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); + EXPECT_EQ(*result.sort_order, *expected_sort_order); } TEST_F(UpdateSortOrderTest, AddSingleSortFieldDescending) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); - auto ref = NamedReference::Make("y").value(); - auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); + auto term = Expressions::Transform("y", Transform::Identity()); - update->AddSortField(std::move(term), SortDirection::kDescending, NullOrder::kLast); - EXPECT_THAT(update->Commit(), IsOk()); + update->AddSortField(term, SortDirection::kDescending, NullOrder::kLast); - // Verify the sort order was set - ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); - ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->metadata()->SortOrder()); - ASSERT_NE(sort_order, nullptr); - EXPECT_EQ(sort_order->fields().size(), 1); - - const auto& field = sort_order->fields()[0]; - EXPECT_EQ(field.source_id(), 2); - EXPECT_EQ(field.direction(), SortDirection::kDescending); - EXPECT_EQ(field.null_order(), NullOrder::kLast); + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); + + std::vector expected_fields; + expected_fields.emplace_back(2, Transform::Identity(), SortDirection::kDescending, + NullOrder::kLast); + ICEBERG_UNWRAP_OR_FAIL( + auto expected_sort_order, + SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); + EXPECT_EQ(*result.sort_order, *expected_sort_order); } TEST_F(UpdateSortOrderTest, AddMultipleSortFields) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); - auto ref1 = NamedReference::Make("y").value(); - auto term1 = UnboundTransform::Make(std::move(ref1), Transform::Identity()).value(); + auto term1 = Expressions::Transform("y", Transform::Identity()); + auto term2 = Expressions::Transform("x", Transform::Identity()); + + update->AddSortField(term1, SortDirection::kAscending, NullOrder::kFirst) + .AddSortField(term2, SortDirection::kDescending, NullOrder::kLast); + + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); + + std::vector expected_fields; + expected_fields.emplace_back(2, Transform::Identity(), SortDirection::kAscending, + NullOrder::kFirst); + expected_fields.emplace_back(1, Transform::Identity(), SortDirection::kDescending, + NullOrder::kLast); + ICEBERG_UNWRAP_OR_FAIL( + auto expected_sort_order, + SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); + EXPECT_EQ(*result.sort_order, *expected_sort_order); +} - auto ref2 = NamedReference::Make("x").value(); - auto term2 = UnboundTransform::Make(std::move(ref2), Transform::Identity()).value(); +TEST_F(UpdateSortOrderTest, AddSortFieldWithNamedReference) { + // Test that we can directly use NamedReference (kReference) which is treated as + // identity + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); + auto ref = Expressions::Ref("x"); - update->AddSortField(std::move(term1), SortDirection::kAscending, NullOrder::kFirst) - .AddSortField(std::move(term2), SortDirection::kDescending, NullOrder::kLast); + update->AddSortField(ref, SortDirection::kAscending, NullOrder::kFirst); - EXPECT_THAT(update->Commit(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); - // Verify the sort order was set with multiple fields - ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); - ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->metadata()->SortOrder()); - ASSERT_NE(sort_order, nullptr); - EXPECT_EQ(sort_order->fields().size(), 2); - - // Check first field (y field) - const auto& field1 = sort_order->fields()[0]; - EXPECT_EQ(field1.source_id(), 2); - EXPECT_EQ(field1.direction(), SortDirection::kAscending); - EXPECT_EQ(field1.null_order(), NullOrder::kFirst); - - // Check second field (x field) - const auto& field2 = sort_order->fields()[1]; - EXPECT_EQ(field2.source_id(), 1); - EXPECT_EQ(field2.direction(), SortDirection::kDescending); - EXPECT_EQ(field2.null_order(), NullOrder::kLast); + std::vector expected_fields; + expected_fields.emplace_back(1, Transform::Identity(), SortDirection::kAscending, + NullOrder::kFirst); + ICEBERG_UNWRAP_OR_FAIL( + auto expected_sort_order, + SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); + EXPECT_EQ(*result.sort_order, *expected_sort_order); +} + +TEST_F(UpdateSortOrderTest, AddSortFieldByName) { + // Test the convenience method for adding sort field by name + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); + + update->AddSortFieldByName("x", SortDirection::kAscending, NullOrder::kFirst); + + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); + + std::vector expected_fields; + expected_fields.emplace_back(1, Transform::Identity(), SortDirection::kAscending, + NullOrder::kFirst); + ICEBERG_UNWRAP_OR_FAIL( + auto expected_sort_order, + SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); + EXPECT_EQ(*result.sort_order, *expected_sort_order); } TEST_F(UpdateSortOrderTest, AddSortFieldWithTruncateTransform) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); + auto term = Expressions::Truncate("x", 10); - auto ref = NamedReference::Make("x").value(); - auto term = UnboundTransform::Make(std::move(ref), Transform::Truncate(10)).value(); + update->AddSortField(term, SortDirection::kAscending, NullOrder::kFirst); - update->AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst); - EXPECT_THAT(update->Commit(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); - // Verify the sort order uses truncate transform - ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); - ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->metadata()->SortOrder()); - ASSERT_NE(sort_order, nullptr); - EXPECT_EQ(sort_order->fields().size(), 1); - - const auto& field = sort_order->fields()[0]; - EXPECT_EQ(field.source_id(), 1); - EXPECT_EQ(field.transform()->ToString(), "truncate[10]"); - EXPECT_EQ(field.direction(), SortDirection::kAscending); + std::vector expected_fields; + expected_fields.emplace_back(1, Transform::Truncate(10), SortDirection::kAscending, + NullOrder::kFirst); + ICEBERG_UNWRAP_OR_FAIL( + auto expected_sort_order, + SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); + EXPECT_EQ(*result.sort_order, *expected_sort_order); } TEST_F(UpdateSortOrderTest, AddSortFieldWithBucketTransform) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); + auto term = Expressions::Bucket("y", 10); - auto ref = NamedReference::Make("y").value(); - auto term = UnboundTransform::Make(std::move(ref), Transform::Bucket(10)).value(); + update->AddSortField(term, SortDirection::kDescending, NullOrder::kLast); - update->AddSortField(std::move(term), SortDirection::kDescending, NullOrder::kLast); - EXPECT_THAT(update->Commit(), IsOk()); + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); - // Verify the sort order uses bucket transform - ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); - ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->metadata()->SortOrder()); - ASSERT_NE(sort_order, nullptr); - EXPECT_EQ(sort_order->fields().size(), 1); - - const auto& field = sort_order->fields()[0]; - EXPECT_EQ(field.source_id(), 2); - EXPECT_EQ(field.transform()->ToString(), "bucket[10]"); - EXPECT_EQ(field.direction(), SortDirection::kDescending); + std::vector expected_fields; + expected_fields.emplace_back(2, Transform::Bucket(10), SortDirection::kDescending, + NullOrder::kLast); + ICEBERG_UNWRAP_OR_FAIL( + auto expected_sort_order, + SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); + EXPECT_EQ(*result.sort_order, *expected_sort_order); } TEST_F(UpdateSortOrderTest, AddSortFieldNullTerm) { @@ -159,7 +169,7 @@ TEST_F(UpdateSortOrderTest, AddSortFieldNullTerm) { update->AddSortField(nullptr, SortDirection::kAscending, NullOrder::kFirst); - auto result = update->Commit(); + auto result = update->Apply(); EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); EXPECT_THAT(result, HasErrorMessage("Term cannot be null")); } @@ -173,7 +183,7 @@ TEST_F(UpdateSortOrderTest, AddSortFieldInvalidTransform) { update->AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst); - auto result = update->Commit(); + auto result = update->Apply(); EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); EXPECT_THAT(result, HasErrorMessage("not a valid input type")); } @@ -186,7 +196,7 @@ TEST_F(UpdateSortOrderTest, AddSortFieldNonExistentField) { update->AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst); - auto result = update->Commit(); + auto result = update->Apply(); EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); EXPECT_THAT(result, HasErrorMessage("Cannot find")); } @@ -200,7 +210,7 @@ TEST_F(UpdateSortOrderTest, CaseSensitiveTrue) { update->CaseSensitive(true).AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst); - auto result = update->Commit(); + auto result = update->Apply(); // Should fail because schema has "x" (lowercase) EXPECT_THAT(result, IsError(ErrorKind::kValidationFailed)); } @@ -213,60 +223,51 @@ TEST_F(UpdateSortOrderTest, CaseSensitiveFalse) { update->CaseSensitive(false).AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst); - auto result = update->Commit(); + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); // Should succeed because case-insensitive matching - EXPECT_THAT(result, IsOk()); - - ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); - ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->metadata()->SortOrder()); - ASSERT_NE(sort_order, nullptr); - EXPECT_EQ(sort_order->fields().size(), 1); - EXPECT_EQ(sort_order->fields()[0].source_id(), 1); + std::vector expected_fields; + expected_fields.emplace_back(1, Transform::Identity(), SortDirection::kAscending, + NullOrder::kFirst); + ICEBERG_UNWRAP_OR_FAIL( + auto expected_sort_order, + SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); + EXPECT_EQ(*result.sort_order, *expected_sort_order); } -TEST_F(UpdateSortOrderTest, ReplaceExistingSortOrder) { - // First, set an initial sort order - ICEBERG_UNWRAP_OR_FAIL(auto update1, table_->NewUpdateSortOrder()); - - auto ref1 = NamedReference::Make("x").value(); - auto term1 = UnboundTransform::Make(std::move(ref1), Transform::Identity()).value(); - update1->AddSortField(std::move(term1), SortDirection::kAscending, NullOrder::kFirst); - EXPECT_THAT(update1->Commit(), IsOk()); +TEST_F(UpdateSortOrderTest, CommitSuccess) { + // Test empty commit + ICEBERG_UNWRAP_OR_FAIL(auto empty_update, table_->NewUpdateSortOrder()); + EXPECT_THAT(empty_update->Commit(), IsOk()); - // Now replace with a different sort order + // Reload table after first commit ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); - ICEBERG_UNWRAP_OR_FAIL(auto update2, reloaded->NewUpdateSortOrder()); - auto ref2 = NamedReference::Make("y").value(); - auto term2 = UnboundTransform::Make(std::move(ref2), Transform::Identity()).value(); - update2->AddSortField(std::move(term2), SortDirection::kDescending, NullOrder::kLast); - EXPECT_THAT(update2->Commit(), IsOk()); + // Test commit with sort order changes + ICEBERG_UNWRAP_OR_FAIL(auto update, reloaded->NewUpdateSortOrder()); + auto ref = NamedReference::Make("x").value(); + auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); + update->AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst); + + EXPECT_THAT(update->Commit(), IsOk()); - // Verify the sort order was replaced + // Verify the sort order was committed ICEBERG_UNWRAP_OR_FAIL(auto final_table, catalog_->LoadTable(table_ident_)); - ICEBERG_UNWRAP_OR_FAIL(auto sort_order, final_table->metadata()->SortOrder()); - ASSERT_NE(sort_order, nullptr); - EXPECT_EQ(sort_order->fields().size(), 1); - - const auto& field = sort_order->fields()[0]; - EXPECT_EQ(field.source_id(), 2); - EXPECT_EQ(field.direction(), SortDirection::kDescending); - EXPECT_EQ(field.null_order(), NullOrder::kLast); + ICEBERG_UNWRAP_OR_FAIL(auto sort_order, final_table->sort_order()); + + std::vector expected_fields; + expected_fields.emplace_back(1, Transform::Identity(), SortDirection::kAscending, + NullOrder::kFirst); + ICEBERG_UNWRAP_OR_FAIL( + auto expected_sort_order, + SortOrder::Make(sort_order->order_id(), std::move(expected_fields))); + EXPECT_EQ(*sort_order, *expected_sort_order); } TEST_F(UpdateSortOrderTest, EmptySortOrder) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); - - // Don't add any sort fields - auto result = update->Commit(); - + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); // Should succeed with an unsorted order - EXPECT_THAT(result, IsOk()); - - ICEBERG_UNWRAP_OR_FAIL(auto reloaded, catalog_->LoadTable(table_ident_)); - ICEBERG_UNWRAP_OR_FAIL(auto sort_order, reloaded->metadata()->SortOrder()); - ASSERT_NE(sort_order, nullptr); - EXPECT_TRUE(sort_order->fields().empty()); + EXPECT_TRUE(result.sort_order->fields().empty()); } } // namespace iceberg diff --git a/src/iceberg/transaction.cc b/src/iceberg/transaction.cc index 427c404ef..49416cdc0 100644 --- a/src/iceberg/transaction.cc +++ b/src/iceberg/transaction.cc @@ -30,6 +30,7 @@ #include "iceberg/update/pending_update.h" #include "iceberg/update/update_properties.h" #include "iceberg/update/update_sort_order.h" +#include "iceberg/util/checked_cast.h" #include "iceberg/util/macros.h" namespace iceberg { @@ -67,23 +68,26 @@ Status Transaction::AddUpdate(const std::shared_ptr& update) { Status Transaction::Apply(PendingUpdate& update) { switch (update.kind()) { case PendingUpdate::Kind::kUpdateProperties: { - auto& update_properties = static_cast(update); - ICEBERG_ASSIGN_OR_RAISE(UpdateProperties::ApplyResult result, - update_properties.Apply()); - metadata_builder_->SetProperties(std::move(result.updates_)); - metadata_builder_->RemoveProperties(std::move(result.removals_)); - if (result.format_version_.has_value()) { - metadata_builder_->UpgradeFormatVersion(result.format_version_.value()); + auto& update_properties = internal::checked_cast(update); + ICEBERG_ASSIGN_OR_RAISE(auto result, update_properties.Apply()); + if (!result.updates.empty()) { + metadata_builder_->SetProperties(std::move(result.updates)); + } + if (!result.removals.empty()) { + metadata_builder_->RemoveProperties(std::move(result.removals)); + } + if (result.format_version.has_value()) { + metadata_builder_->UpgradeFormatVersion(result.format_version.value()); } } break; case PendingUpdate::Kind::kUpdateSortOrder: { - auto& update_sort_order = static_cast(update); - ICEBERG_ASSIGN_OR_RAISE(UpdateSortOrder::ApplyResult result, - update_sort_order.Apply()); - metadata_builder_->SetDefaultSortOrder(result.sort_order_); + auto& update_sort_order = internal::checked_cast(update); + ICEBERG_ASSIGN_OR_RAISE(auto result, update_sort_order.Apply()); + metadata_builder_->SetDefaultSortOrder(result.sort_order); } break; default: - return InvalidArgument("Unsupported pending update kind"); + return NotSupported("Unsupported pending update: {}", + static_cast(update.kind())); } last_update_committed_ = true; diff --git a/src/iceberg/update/update_properties.cc b/src/iceberg/update/update_properties.cc index 2dc1d3eb6..ce809c437 100644 --- a/src/iceberg/update/update_properties.cc +++ b/src/iceberg/update/update_properties.cc @@ -36,9 +36,8 @@ namespace iceberg { Result> UpdateProperties::Make( std::shared_ptr transaction) { - if (!transaction) [[unlikely]] { - return InvalidArgument("Cannot create UpdateProperties without a transaction"); - } + ICEBERG_PRECHECK(transaction != nullptr, + "Cannot create UpdateProperties without a transaction"); return std::shared_ptr(new UpdateProperties(std::move(transaction))); } @@ -110,9 +109,8 @@ Result UpdateProperties::Apply() { ICEBERG_RETURN_UNEXPECTED( MetricsConfig::VerifyReferencedColumns(new_properties, *schema.value())); } - return ApplyResult{.updates_ = std::move(new_properties), - .removals_ = std::move(removals_), - .format_version_ = format_version_}; + return ApplyResult{ + .updates = updates_, .removals = removals_, .format_version = format_version_}; } } // namespace iceberg diff --git a/src/iceberg/update/update_properties.h b/src/iceberg/update/update_properties.h index b2397d6a6..7aaf93217 100644 --- a/src/iceberg/update/update_properties.h +++ b/src/iceberg/update/update_properties.h @@ -44,9 +44,9 @@ class ICEBERG_EXPORT UpdateProperties : public PendingUpdate { ~UpdateProperties() override; struct ApplyResult { - std::unordered_map updates_; - std::unordered_set removals_; - std::optional format_version_; + std::unordered_map updates; + std::unordered_set removals; + std::optional format_version; }; /// \brief Sets a property key to a specified value. @@ -67,9 +67,10 @@ class ICEBERG_EXPORT UpdateProperties : public PendingUpdate { Kind kind() const final { return Kind::kUpdateProperties; } - private: + /// \brief Apply the pending changes and return the updates and removals. Result Apply(); + private: friend class Transaction; explicit UpdateProperties(std::shared_ptr transaction); diff --git a/src/iceberg/update/update_sort_order.cc b/src/iceberg/update/update_sort_order.cc index 36cb349df..1a4fc7a8a 100644 --- a/src/iceberg/update/update_sort_order.cc +++ b/src/iceberg/update/update_sort_order.cc @@ -24,22 +24,19 @@ #include #include "iceberg/expression/term.h" -#include "iceberg/result.h" -#include "iceberg/sort_field.h" #include "iceberg/sort_order.h" #include "iceberg/table_metadata.h" #include "iceberg/transaction.h" +#include "iceberg/transform.h" #include "iceberg/util/checked_cast.h" -#include "iceberg/util/error_collector.h" #include "iceberg/util/macros.h" namespace iceberg { Result> UpdateSortOrder::Make( std::shared_ptr transaction) { - if (!transaction) [[unlikely]] { - return InvalidArgument("Cannot create UpdateSortOrder without a transaction"); - } + ICEBERG_PRECHECK(transaction != nullptr, + "Cannot create UpdateSortOrder without a transaction"); return std::shared_ptr(new UpdateSortOrder(std::move(transaction))); } @@ -48,30 +45,44 @@ UpdateSortOrder::UpdateSortOrder(std::shared_ptr transaction) UpdateSortOrder::~UpdateSortOrder() = default; -UpdateSortOrder& UpdateSortOrder::AddSortField(std::shared_ptr term, +UpdateSortOrder& UpdateSortOrder::AddSortField(const std::shared_ptr& term, SortDirection direction, NullOrder null_order) { - if (!term) { - return AddError(ErrorKind::kInvalidArgument, "Term cannot be null"); - } - if (term->kind() != Term::Kind::kTransform) { - return AddError(ErrorKind::kInvalidArgument, "Term must be a transform term"); - } - if (!term->is_unbound()) { - return AddError(ErrorKind::kInvalidArgument, "Term must be unbound"); - } - // use checked-cast to get UnboundTransform - auto unbound_transform = internal::checked_pointer_cast(term); + ICEBERG_BUILDER_CHECK(term != nullptr, "Term cannot be null"); + ICEBERG_BUILDER_CHECK(term->is_unbound(), "Term must be unbound"); + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto schema, transaction_->current().Schema()); - ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto bound_term, - unbound_transform->Bind(*schema, case_sensitive_)); - int32_t source_id = bound_term->reference()->field_id(); - sort_fields_.emplace_back(source_id, unbound_transform->transform(), direction, - null_order); + int32_t source_id; + std::shared_ptr transform; + + if (term->kind() == Term::Kind::kReference) { + // kReference is treated as identity transform + auto named_ref = internal::checked_pointer_cast(term); + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto bound_ref, + named_ref->Bind(*schema, case_sensitive_)); + sort_fields_.emplace_back(bound_ref->field_id(), Transform::Identity(), direction, + null_order); + } else { + // kTransform - use the specified transform + auto unbound_transform = internal::checked_pointer_cast(term); + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto bound_term, + unbound_transform->Bind(*schema, case_sensitive_)); + sort_fields_.emplace_back(bound_term->reference()->field_id(), + unbound_transform->transform(), direction, null_order); + } + return *this; } +UpdateSortOrder& UpdateSortOrder::AddSortFieldByName(std::string_view name, + SortDirection direction, + NullOrder null_order) { + ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto named_ref, + NamedReference::Make(std::string(name))); + return AddSortField(std::move(named_ref), direction, null_order); +} + UpdateSortOrder& UpdateSortOrder::CaseSensitive(bool case_sensitive) { case_sensitive_ = case_sensitive; return *this; @@ -85,15 +96,13 @@ Result UpdateSortOrder::Apply() { if (sort_fields_.empty()) { order = SortOrder::Unsorted(); } else { - // Use kInitialSortOrderId (1) as a placeholder for non-empty sort orders. + // Use -1 as a placeholder for non-empty sort orders. // The actual sort order ID will be assigned by TableMetadataBuilder when // the AddSortOrder update is applied. - ICEBERG_ASSIGN_OR_RAISE( - order, SortOrder::Make(SortOrder::kInitialSortOrderId, sort_fields_)); + ICEBERG_ASSIGN_OR_RAISE(order, SortOrder::Make(/*sort_id=*/-1, sort_fields_)); + ICEBERG_ASSIGN_OR_RAISE(auto schema, transaction_->current().Schema()); + ICEBERG_RETURN_UNEXPECTED(order->Validate(*schema)); } - - ICEBERG_ASSIGN_OR_RAISE(auto schema, transaction_->current().Schema()); - ICEBERG_RETURN_UNEXPECTED(order->Validate(*schema)); return ApplyResult{std::move(order)}; } diff --git a/src/iceberg/update/update_sort_order.h b/src/iceberg/update/update_sort_order.h index 4404d6e44..f9b0b15ef 100644 --- a/src/iceberg/update/update_sort_order.h +++ b/src/iceberg/update/update_sort_order.h @@ -20,12 +20,11 @@ #pragma once #include +#include #include #include "iceberg/expression/term.h" -#include "iceberg/iceberg_export.h" #include "iceberg/sort_field.h" -#include "iceberg/sort_order.h" #include "iceberg/type_fwd.h" #include "iceberg/update/pending_update.h" @@ -43,7 +42,7 @@ class ICEBERG_EXPORT UpdateSortOrder : public PendingUpdate { ~UpdateSortOrder() override; struct ApplyResult { - std::shared_ptr sort_order_; + std::shared_ptr sort_order; }; /// \brief Add a sort field to the sort order. @@ -52,8 +51,17 @@ class ICEBERG_EXPORT UpdateSortOrder : public PendingUpdate { /// \param direction The sort direction (ascending or descending) /// \param null_order The null order (first or last) /// \return Reference to this UpdateSortOrder for chaining - UpdateSortOrder& AddSortField(std::shared_ptr term, SortDirection direction, - NullOrder null_order); + UpdateSortOrder& AddSortField(const std::shared_ptr& term, + SortDirection direction, NullOrder null_order); + + /// \brief Add a sort field by field name with identity transform. + /// + /// \param name The name of the field to sort by + /// \param direction The sort direction (ascending or descending) + /// \param null_order The null order (first or last) + /// \return Reference to this UpdateSortOrder for chaining + UpdateSortOrder& AddSortFieldByName(std::string_view name, SortDirection direction, + NullOrder null_order); /// \brief Set case sensitivity of sort column name resolution. /// @@ -63,16 +71,16 @@ class ICEBERG_EXPORT UpdateSortOrder : public PendingUpdate { Kind kind() const final { return Kind::kUpdateSortOrder; } - private: + /// \brief Apply the pending changes and return the new SortOrder. Result Apply(); + private: friend class Transaction; explicit UpdateSortOrder(std::shared_ptr transaction); std::vector sort_fields_; bool case_sensitive_ = true; - std::shared_ptr sort_order_; }; } // namespace iceberg From be01a4d60e02e96d3fd9e6238548942c920d61d3 Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Wed, 24 Dec 2025 16:18:23 +0800 Subject: [PATCH 6/8] 1 --- src/iceberg/test/update_properties_test.cc | 1 - src/iceberg/test/update_sort_order_test.cc | 91 +++++++--------------- 2 files changed, 27 insertions(+), 65 deletions(-) diff --git a/src/iceberg/test/update_properties_test.cc b/src/iceberg/test/update_properties_test.cc index 737ba8ff2..8ac8e5eb2 100644 --- a/src/iceberg/test/update_properties_test.cc +++ b/src/iceberg/test/update_properties_test.cc @@ -27,7 +27,6 @@ namespace iceberg { class UpdatePropertiesTest : public UpdateTestBase {}; TEST_F(UpdatePropertiesTest, EmptyUpdate) { - // commit an empty update, should succeed ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateProperties()); ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); EXPECT_THAT(result.updates.empty(), true); diff --git a/src/iceberg/test/update_sort_order_test.cc b/src/iceberg/test/update_sort_order_test.cc index 2c6238985..e5f5fc0e9 100644 --- a/src/iceberg/test/update_sort_order_test.cc +++ b/src/iceberg/test/update_sort_order_test.cc @@ -36,62 +36,61 @@ namespace iceberg { -class UpdateSortOrderTest : public UpdateTestBase {}; +class UpdateSortOrderTest : public UpdateTestBase { + protected: + // Helper function to apply update and verify the resulting sort order + void ApplyAndExpectSortOrder(UpdateSortOrder* update, + std::vector expected_fields) { + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); + ICEBERG_UNWRAP_OR_FAIL( + auto expected_sort_order, + SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); + EXPECT_EQ(*result.sort_order, *expected_sort_order); + } +}; + +TEST_F(UpdateSortOrderTest, EmptySortOrder) { + ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); + ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); + // Should succeed with an unsorted order + EXPECT_TRUE(result.sort_order->fields().empty()); +} TEST_F(UpdateSortOrderTest, AddSingleSortFieldAscending) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto term = Expressions::Transform("x", Transform::Identity()); - update->AddSortField(term, SortDirection::kAscending, NullOrder::kFirst); - ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); - EXPECT_FALSE(result.sort_order->is_unsorted()); - std::vector expected_fields; expected_fields.emplace_back(1, Transform::Identity(), SortDirection::kAscending, NullOrder::kFirst); - ICEBERG_UNWRAP_OR_FAIL( - auto expected_sort_order, - SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); - EXPECT_EQ(*result.sort_order, *expected_sort_order); + ApplyAndExpectSortOrder(update.get(), std::move(expected_fields)); } TEST_F(UpdateSortOrderTest, AddSingleSortFieldDescending) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto term = Expressions::Transform("y", Transform::Identity()); - update->AddSortField(term, SortDirection::kDescending, NullOrder::kLast); - ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); - std::vector expected_fields; expected_fields.emplace_back(2, Transform::Identity(), SortDirection::kDescending, NullOrder::kLast); - ICEBERG_UNWRAP_OR_FAIL( - auto expected_sort_order, - SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); - EXPECT_EQ(*result.sort_order, *expected_sort_order); + ApplyAndExpectSortOrder(update.get(), std::move(expected_fields)); } TEST_F(UpdateSortOrderTest, AddMultipleSortFields) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto term1 = Expressions::Transform("y", Transform::Identity()); auto term2 = Expressions::Transform("x", Transform::Identity()); - update->AddSortField(term1, SortDirection::kAscending, NullOrder::kFirst) .AddSortField(term2, SortDirection::kDescending, NullOrder::kLast); - ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); - std::vector expected_fields; expected_fields.emplace_back(2, Transform::Identity(), SortDirection::kAscending, NullOrder::kFirst); expected_fields.emplace_back(1, Transform::Identity(), SortDirection::kDescending, NullOrder::kLast); - ICEBERG_UNWRAP_OR_FAIL( - auto expected_sort_order, - SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); - EXPECT_EQ(*result.sort_order, *expected_sort_order); + ApplyAndExpectSortOrder(update.get(), std::move(expected_fields)); } TEST_F(UpdateSortOrderTest, AddSortFieldWithNamedReference) { @@ -99,69 +98,45 @@ TEST_F(UpdateSortOrderTest, AddSortFieldWithNamedReference) { // identity ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto ref = Expressions::Ref("x"); - update->AddSortField(ref, SortDirection::kAscending, NullOrder::kFirst); - ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); - std::vector expected_fields; expected_fields.emplace_back(1, Transform::Identity(), SortDirection::kAscending, NullOrder::kFirst); - ICEBERG_UNWRAP_OR_FAIL( - auto expected_sort_order, - SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); - EXPECT_EQ(*result.sort_order, *expected_sort_order); + ApplyAndExpectSortOrder(update.get(), std::move(expected_fields)); } TEST_F(UpdateSortOrderTest, AddSortFieldByName) { // Test the convenience method for adding sort field by name ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); - update->AddSortFieldByName("x", SortDirection::kAscending, NullOrder::kFirst); - ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); - std::vector expected_fields; expected_fields.emplace_back(1, Transform::Identity(), SortDirection::kAscending, NullOrder::kFirst); - ICEBERG_UNWRAP_OR_FAIL( - auto expected_sort_order, - SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); - EXPECT_EQ(*result.sort_order, *expected_sort_order); + ApplyAndExpectSortOrder(update.get(), std::move(expected_fields)); } TEST_F(UpdateSortOrderTest, AddSortFieldWithTruncateTransform) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto term = Expressions::Truncate("x", 10); - update->AddSortField(term, SortDirection::kAscending, NullOrder::kFirst); - ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); - std::vector expected_fields; expected_fields.emplace_back(1, Transform::Truncate(10), SortDirection::kAscending, NullOrder::kFirst); - ICEBERG_UNWRAP_OR_FAIL( - auto expected_sort_order, - SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); - EXPECT_EQ(*result.sort_order, *expected_sort_order); + ApplyAndExpectSortOrder(update.get(), std::move(expected_fields)); } TEST_F(UpdateSortOrderTest, AddSortFieldWithBucketTransform) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto term = Expressions::Bucket("y", 10); - update->AddSortField(term, SortDirection::kDescending, NullOrder::kLast); - ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); - std::vector expected_fields; expected_fields.emplace_back(2, Transform::Bucket(10), SortDirection::kDescending, NullOrder::kLast); - ICEBERG_UNWRAP_OR_FAIL( - auto expected_sort_order, - SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); - EXPECT_EQ(*result.sort_order, *expected_sort_order); + ApplyAndExpectSortOrder(update.get(), std::move(expected_fields)); } TEST_F(UpdateSortOrderTest, AddSortFieldNullTerm) { @@ -219,19 +194,14 @@ TEST_F(UpdateSortOrderTest, CaseSensitiveFalse) { ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); auto ref = NamedReference::Make("X").value(); // Uppercase auto term = UnboundTransform::Make(std::move(ref), Transform::Identity()).value(); - update->CaseSensitive(false).AddSortField(std::move(term), SortDirection::kAscending, NullOrder::kFirst); - ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); // Should succeed because case-insensitive matching std::vector expected_fields; expected_fields.emplace_back(1, Transform::Identity(), SortDirection::kAscending, NullOrder::kFirst); - ICEBERG_UNWRAP_OR_FAIL( - auto expected_sort_order, - SortOrder::Make(result.sort_order->order_id(), std::move(expected_fields))); - EXPECT_EQ(*result.sort_order, *expected_sort_order); + ApplyAndExpectSortOrder(update.get(), std::move(expected_fields)); } TEST_F(UpdateSortOrderTest, CommitSuccess) { @@ -263,11 +233,4 @@ TEST_F(UpdateSortOrderTest, CommitSuccess) { EXPECT_EQ(*sort_order, *expected_sort_order); } -TEST_F(UpdateSortOrderTest, EmptySortOrder) { - ICEBERG_UNWRAP_OR_FAIL(auto update, table_->NewUpdateSortOrder()); - ICEBERG_UNWRAP_OR_FAIL(auto result, update->Apply()); - // Should succeed with an unsorted order - EXPECT_TRUE(result.sort_order->fields().empty()); -} - } // namespace iceberg From 49a1b72409edeb6dcbaa61e1bb7681d1ce22ad4a Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Wed, 24 Dec 2025 16:29:24 +0800 Subject: [PATCH 7/8] 1 --- src/iceberg/update/update_properties.h | 2 -- src/iceberg/update/update_sort_order.cc | 11 +++++------ src/iceberg/update/update_sort_order.h | 2 -- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/iceberg/update/update_properties.h b/src/iceberg/update/update_properties.h index 7aaf93217..ec9ab796e 100644 --- a/src/iceberg/update/update_properties.h +++ b/src/iceberg/update/update_properties.h @@ -71,8 +71,6 @@ class ICEBERG_EXPORT UpdateProperties : public PendingUpdate { Result Apply(); private: - friend class Transaction; - explicit UpdateProperties(std::shared_ptr transaction); std::unordered_map updates_; diff --git a/src/iceberg/update/update_sort_order.cc b/src/iceberg/update/update_sort_order.cc index 1a4fc7a8a..c31d2ba74 100644 --- a/src/iceberg/update/update_sort_order.cc +++ b/src/iceberg/update/update_sort_order.cc @@ -24,6 +24,7 @@ #include #include "iceberg/expression/term.h" +#include "iceberg/result.h" #include "iceberg/sort_order.h" #include "iceberg/table_metadata.h" #include "iceberg/transaction.h" @@ -52,10 +53,6 @@ UpdateSortOrder& UpdateSortOrder::AddSortField(const std::shared_ptr& term ICEBERG_BUILDER_CHECK(term->is_unbound(), "Term must be unbound"); ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto schema, transaction_->current().Schema()); - - int32_t source_id; - std::shared_ptr transform; - if (term->kind() == Term::Kind::kReference) { // kReference is treated as identity transform auto named_ref = internal::checked_pointer_cast(term); @@ -63,13 +60,15 @@ UpdateSortOrder& UpdateSortOrder::AddSortField(const std::shared_ptr& term named_ref->Bind(*schema, case_sensitive_)); sort_fields_.emplace_back(bound_ref->field_id(), Transform::Identity(), direction, null_order); - } else { - // kTransform - use the specified transform + } else if (term->kind() == Term::Kind::kTransform) { auto unbound_transform = internal::checked_pointer_cast(term); ICEBERG_BUILDER_ASSIGN_OR_RETURN(auto bound_term, unbound_transform->Bind(*schema, case_sensitive_)); sort_fields_.emplace_back(bound_term->reference()->field_id(), unbound_transform->transform(), direction, null_order); + } else { + return AddError(ErrorKind::kNotSupported, "Not supported unbound term: {}", + static_cast(term->kind())); } return *this; diff --git a/src/iceberg/update/update_sort_order.h b/src/iceberg/update/update_sort_order.h index f9b0b15ef..7983513b6 100644 --- a/src/iceberg/update/update_sort_order.h +++ b/src/iceberg/update/update_sort_order.h @@ -75,8 +75,6 @@ class ICEBERG_EXPORT UpdateSortOrder : public PendingUpdate { Result Apply(); private: - friend class Transaction; - explicit UpdateSortOrder(std::shared_ptr transaction); std::vector sort_fields_; From 05ce98c0232a725dd46a43a3e969becf92fa5bc4 Mon Sep 17 00:00:00 2001 From: Feiyang Li Date: Wed, 24 Dec 2025 16:38:11 +0800 Subject: [PATCH 8/8] fix conficts --- src/iceberg/table_metadata.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/iceberg/table_metadata.cc b/src/iceberg/table_metadata.cc index 40dcb03c0..2d77aef43 100644 --- a/src/iceberg/table_metadata.cc +++ b/src/iceberg/table_metadata.cc @@ -29,6 +29,7 @@ #include #include #include +#include #include #include @@ -426,7 +427,7 @@ class TableMetadataBuilder::Impl { Status SetDefaultSortOrder(int32_t order_id); Result AddSortOrder(const SortOrder& order); Status SetProperties(const std::unordered_map& updated); - Status RemoveProperties(const std::vector& removed); + Status RemoveProperties(const std::unordered_set& removed); std::unique_ptr Build(); @@ -590,7 +591,7 @@ Status TableMetadataBuilder::Impl::SetProperties( } Status TableMetadataBuilder::Impl::RemoveProperties( - const std::vector& removed) { + const std::unordered_set& removed) { // If removed is empty, return early (no-op) if (removed.empty()) { return {}; @@ -820,7 +821,7 @@ TableMetadataBuilder& TableMetadataBuilder::SetProperties( } TableMetadataBuilder& TableMetadataBuilder::RemoveProperties( - const std::vector& removed) { + const std::unordered_set& removed) { ICEBERG_BUILDER_RETURN_IF_ERROR(impl_->RemoveProperties(removed)); return *this; }