wgtmac commented on code in PR #345:
URL: https://github.com/apache/iceberg-cpp/pull/345#discussion_r2583488816
##########
src/iceberg/table_requirements.h:
##########
@@ -68,27 +68,19 @@ class ICEBERG_EXPORT TableUpdateContext {
/// \brief Build and return the list of requirements
Result<std::vector<std::unique_ptr<TableRequirement>>> Build();
- // Getters for deduplication flags
- bool added_last_assigned_field_id() const { return
added_last_assigned_field_id_; }
- bool added_current_schema_id() const { return added_current_schema_id_; }
- bool added_last_assigned_partition_id() const {
- return added_last_assigned_partition_id_;
- }
- bool added_default_spec_id() const { return added_default_spec_id_; }
- bool added_default_sort_order_id() const { return
added_default_sort_order_id_; }
-
- // Setters for deduplication flags
- void set_added_last_assigned_field_id(bool value) {
- added_last_assigned_field_id_ = value;
- }
- void set_added_current_schema_id(bool value) { added_current_schema_id_ =
value; }
- void set_added_last_assigned_partition_id(bool value) {
- added_last_assigned_partition_id_ = value;
- }
- void set_added_default_spec_id(bool value) { added_default_spec_id_ = value;
}
- void set_added_default_sort_order_id(bool value) {
- added_default_sort_order_id_ = value;
- }
+ // Helper methods to add requirements with deduplication
Review Comment:
```suggestion
// Helper methods to deduplicate requirements to add.
```
##########
src/iceberg/table_metadata.cc:
##########
@@ -42,6 +45,21 @@
namespace iceberg {
+#define BUILDER_RETURN_IF_ERROR(result) \
Review Comment:
What about moving these macros to `iceberg/util/error_collector.h`?
##########
src/iceberg/table_metadata.cc:
##########
@@ -406,16 +447,96 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveSchemas(
TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(
std::shared_ptr<SortOrder> order) {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(order));
+ return SetDefaultSortOrder(order_id);
}
TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(int32_t
order_id) {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ if (order_id == -1) {
Review Comment:
```suggestion
if (order_id == SortOrder::kInitialSortOrderId) {
```
##########
src/iceberg/table_requirements.h:
##########
@@ -97,11 +89,11 @@ class ICEBERG_EXPORT TableUpdateContext {
std::vector<std::unique_ptr<TableRequirement>> requirements_;
// flags to avoid adding duplicate requirements
- bool added_last_assigned_field_id_ = false;
- bool added_current_schema_id_ = false;
- bool added_last_assigned_partition_id_ = false;
- bool added_default_spec_id_ = false;
- bool added_default_sort_order_id_ = false;
+ bool asserted_last_assigned_field_id_ = false;
Review Comment:
TBH, I prefer names on the left.
##########
src/iceberg/table_metadata.cc:
##########
@@ -406,16 +447,96 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveSchemas(
TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(
std::shared_ptr<SortOrder> order) {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(order));
+ return SetDefaultSortOrder(order_id);
}
TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(int32_t
order_id) {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ if (order_id == -1) {
+ if (!impl_->last_added_order_id.has_value()) {
+ return AddError(ErrorKind::kInvalidArgument,
+ "Cannot set last added sort order: no sort order has
been added");
+ }
+ return SetDefaultSortOrder(impl_->last_added_order_id.value());
+ }
+
+ if (order_id == impl_->metadata.default_sort_order_id) {
+ return *this;
+ }
+ impl_->metadata.default_sort_order_id = order_id;
+
Review Comment:
```suggestion
impl_->metadata.default_sort_order_id = order_id;
```
##########
src/iceberg/test/table_update_test.cc:
##########
@@ -56,35 +67,377 @@ std::unique_ptr<TableMetadata> CreateBaseMetadata() {
metadata->location = "s3://bucket/test";
metadata->last_sequence_number = 0;
metadata->last_updated_ms = TimePointMs{std::chrono::milliseconds(1000)};
- metadata->last_column_id = 0;
+ metadata->last_column_id = 3;
+ metadata->current_schema_id = 0;
+ metadata->schemas.push_back(CreateTestSchema());
metadata->default_spec_id = PartitionSpec::kInitialSpecId;
metadata->last_partition_id = 0;
metadata->current_snapshot_id = Snapshot::kInvalidSnapshotId;
metadata->default_sort_order_id = SortOrder::kInitialSortOrderId;
+ metadata->sort_orders.push_back(SortOrder::Unsorted());
metadata->next_row_id = TableMetadata::kInitialRowId;
return metadata;
}
} // namespace
-// Test GenerateRequirements for AssignUUID update
+// Test AssignUUID
+TEST(TableMetadataBuilderTest, AssignUUIDApplyUpdate) {
+ auto base = CreateBaseMetadata();
+ auto builder = TableMetadataBuilder::BuildFrom(base.get());
+
+ // Apply AssignUUID update
+ table::AssignUUID uuid_update("apply-uuid");
+ uuid_update.ApplyTo(*builder);
+
+ ICEBERG_UNWRAP_OR_FAIL(auto metadata, builder->Build());
+ EXPECT_EQ(metadata->table_uuid, "apply-uuid");
+}
+
TEST(TableUpdateTest, AssignUUIDGenerateRequirements) {
table::AssignUUID update("new-uuid");
// New table - no requirements (AssignUUID doesn't generate requirements)
auto new_table_reqs = GenerateRequirements(update, nullptr);
EXPECT_TRUE(new_table_reqs.empty());
- // Existing table - AssignUUID doesn't generate requirements anymore
- // The UUID assertion is added by ForUpdateTable/ForReplaceTable methods
+ // Existing table - no requirements
+ auto base = CreateBaseMetadata();
+ auto existing_table_reqs = GenerateRequirements(update, base.get());
+ EXPECT_TRUE(existing_table_reqs.empty());
+}
+
+// Test UpgradeFormatVersion
+TEST(TableUpdateTest, UpgradeFormatVersionGenerateRequirements) {
Review Comment:
I think this PR has mixed two features: 1) add sort order to table metadata
builder and 2) implement table requirement context. It would be better to split
it into two to make the git commit history cleaner.
BTW, some test cases like this belong to the table_requirement_test.cc and
should be moved there. I think we need port `TestUpdateRequirements.java` from
java for better coverage.
##########
src/iceberg/table_metadata.cc:
##########
@@ -406,16 +447,96 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveSchemas(
TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(
std::shared_ptr<SortOrder> order) {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(order));
Review Comment:
```suggestion
BUILDER_ASSIGN_OR_RETURN(auto order_id,
AddSortOrderInternal(std::move(order)));
```
##########
src/iceberg/table_update.cc:
##########
@@ -135,16 +133,16 @@ void AddSnapshot::ApplyTo(TableMetadataBuilder& builder)
const {
throw IcebergError(std::format("{} not implemented", __FUNCTION__));
}
-Status AddSnapshot::GenerateRequirements(TableUpdateContext& context) const {
- return NotImplemented("AddTableSnapshot::GenerateRequirements not
implemented");
+void AddSnapshot::GenerateRequirements(TableUpdateContext& context) const {
+ // AddSnapshot doesn't generate any requirements
}
// RemoveSnapshots
void RemoveSnapshots::ApplyTo(TableMetadataBuilder& builder) const {}
-Status RemoveSnapshots::GenerateRequirements(TableUpdateContext& context)
const {
- return NotImplemented("RemoveTableSnapshots::GenerateRequirements not
implemented");
+void RemoveSnapshots::GenerateRequirements(TableUpdateContext& context) const {
+ throw NotImplemented("RemoveTableSnapshots::GenerateRequirements not
implemented");
Review Comment:
This is wrong. `NotImplemented` is not an exception type. We can throw
`IcebergError` instead.
##########
src/iceberg/table_metadata.cc:
##########
@@ -406,16 +447,96 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveSchemas(
TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(
std::shared_ptr<SortOrder> order) {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(order));
+ return SetDefaultSortOrder(order_id);
}
TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(int32_t
order_id) {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ if (order_id == -1) {
+ if (!impl_->last_added_order_id.has_value()) {
+ return AddError(ErrorKind::kInvalidArgument,
+ "Cannot set last added sort order: no sort order has
been added");
+ }
+ return SetDefaultSortOrder(impl_->last_added_order_id.value());
+ }
+
+ if (order_id == impl_->metadata.default_sort_order_id) {
+ return *this;
+ }
+ impl_->metadata.default_sort_order_id = order_id;
+
+ if (impl_->last_added_order_id.has_value() &&
+ impl_->last_added_order_id.value() == order_id) {
+ impl_->changes.push_back(std::make_unique<table::SetDefaultSortOrder>(-1));
+ } else {
+
impl_->changes.push_back(std::make_unique<table::SetDefaultSortOrder>(order_id));
+ }
+ return *this;
+}
+
+Result<int32_t> TableMetadataBuilder::AddSortOrderInternal(
+ std::shared_ptr<SortOrder> order) {
Review Comment:
It seems that this `order` is never moved away or reused. Perhaps we should
use `const SortOrder& order`.
##########
src/iceberg/table_metadata.cc:
##########
@@ -406,16 +447,96 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveSchemas(
TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(
std::shared_ptr<SortOrder> order) {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(order));
+ return SetDefaultSortOrder(order_id);
}
TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(int32_t
order_id) {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ if (order_id == -1) {
+ if (!impl_->last_added_order_id.has_value()) {
+ return AddError(ErrorKind::kInvalidArgument,
+ "Cannot set last added sort order: no sort order has
been added");
+ }
+ return SetDefaultSortOrder(impl_->last_added_order_id.value());
+ }
+
+ if (order_id == impl_->metadata.default_sort_order_id) {
+ return *this;
+ }
+ impl_->metadata.default_sort_order_id = order_id;
+
+ if (impl_->last_added_order_id.has_value() &&
+ impl_->last_added_order_id.value() == order_id) {
+ impl_->changes.push_back(std::make_unique<table::SetDefaultSortOrder>(-1));
Review Comment:
Define `kLastAdded = -1`
##########
src/iceberg/table_metadata.cc:
##########
@@ -406,16 +447,96 @@ TableMetadataBuilder& TableMetadataBuilder::RemoveSchemas(
TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(
std::shared_ptr<SortOrder> order) {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ BUILDER_ASSIGN_OR_RETURN(auto order_id, AddSortOrderInternal(order));
+ return SetDefaultSortOrder(order_id);
}
TableMetadataBuilder& TableMetadataBuilder::SetDefaultSortOrder(int32_t
order_id) {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ if (order_id == -1) {
+ if (!impl_->last_added_order_id.has_value()) {
+ return AddError(ErrorKind::kInvalidArgument,
+ "Cannot set last added sort order: no sort order has
been added");
+ }
+ return SetDefaultSortOrder(impl_->last_added_order_id.value());
+ }
+
+ if (order_id == impl_->metadata.default_sort_order_id) {
+ return *this;
+ }
+ impl_->metadata.default_sort_order_id = order_id;
+
+ if (impl_->last_added_order_id.has_value() &&
+ impl_->last_added_order_id.value() == order_id) {
Review Comment:
```suggestion
if (impl_->last_added_order_id == std::make_optional(order_id)) {
```
It can be a one-liner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
