gty404 commented on code in PR #268:
URL: https://github.com/apache/iceberg-cpp/pull/268#discussion_r2448836232
##########
src/iceberg/table_metadata.cc:
##########
@@ -201,13 +203,46 @@ Status TableMetadataUtil::Write(FileIO& io, const
std::string& location,
// TableMetadataBuilder implementation
-struct TableMetadataBuilder::Impl {};
+struct TableMetadataBuilder::Impl {
+ // Base metadata (nullptr for new tables)
+ const TableMetadata* base;
+
+ // Working metadata copy
+ TableMetadata metadata;
+
+ // Change tracking
+ std::vector<std::unique_ptr<TableUpdate>> changes;
+
+ // Error collection (since methods return *this and cannot throw)
+ std::vector<Status> errors;
+
+ // Metadata location tracking
+ std::optional<std::string> metadata_location;
+ std::optional<std::string> previous_metadata_location;
+
+ // Constructor for new table
+ explicit Impl(int8_t format_version) : base(nullptr), metadata{} {
+ metadata.format_version = format_version;
+ metadata.last_sequence_number = TableMetadata::kInitialSequenceNumber;
+ metadata.last_updated_ms = TimePointMs{std::chrono::milliseconds(0)};
Review Comment:
Let's follow the Java pattern and directly define the metadata fields in the
builder, rather than the metadata object, although it doesn't look as concise.
##########
src/iceberg/table_metadata.cc:
##########
@@ -378,11 +436,50 @@ TableMetadataBuilder&
TableMetadataBuilder::RemoveEncryptionKey(std::string_view
}
TableMetadataBuilder& TableMetadataBuilder::DiscardChanges() {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ // Clear all changes and errors
+ impl_->changes.clear();
+ impl_->errors.clear();
+
+ // Reset metadata to base state
+ if (impl_->base != nullptr) {
+ impl_->metadata = *impl_->base;
+ } else {
+ // Reset to initial state for new table
+ *impl_ = Impl(impl_->metadata.format_version);
+ }
+
+ return *this;
}
Result<std::unique_ptr<TableMetadata>> TableMetadataBuilder::Build() {
- return NotImplemented("TableMetadataBuilder::Build not implemented");
+ // 1. Check for accumulated errors
+ if (!impl_->errors.empty()) {
+ std::string error_msg = "Failed to build TableMetadata due to validation
errors:\n";
+ for (const auto& error : impl_->errors) {
+ error_msg += " - " + error.error().message + "\n";
+ }
+ return CommitFailed("{}", error_msg);
+ }
+
+ // 2. Validate metadata consistency
+
+ // Validate UUID exists for format version > 1
+ if (impl_->metadata.format_version > 1 &&
impl_->metadata.table_uuid.empty()) {
+ return InvalidArgument("UUID is required for format version {}",
+ impl_->metadata.format_version);
+ }
+
+ // 3. Update last_updated_ms if there are changes
+ if (!impl_->changes.empty() && impl_->base != nullptr) {
+ impl_->metadata.last_updated_ms =
+ TimePointMs{std::chrono::duration_cast<std::chrono::milliseconds>(
+ std::chrono::system_clock::now().time_since_epoch())};
+ }
Review Comment:
The check exists in the constructor of TableMetadata.
##########
src/iceberg/table_requirements.cc:
##########
@@ -26,11 +26,11 @@
namespace iceberg {
void TableUpdateContext::AddRequirement(std::unique_ptr<TableRequirement>
requirement) {
- throw IcebergError("TableUpdateContext::AddRequirement not implemented");
+ requirements_.emplace_back(std::move(requirement));
Review Comment:
Yes, when updating an existing table, it is necessary to verify if the uuid
has changed. This assertion is actively added by
ForReplaceTable/ForUpdateTable, not generated from TableUpdate.
##########
src/iceberg/table_metadata.cc:
##########
@@ -378,11 +436,50 @@ TableMetadataBuilder&
TableMetadataBuilder::RemoveEncryptionKey(std::string_view
}
TableMetadataBuilder& TableMetadataBuilder::DiscardChanges() {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ // Clear all changes and errors
+ impl_->changes.clear();
+ impl_->errors.clear();
+
+ // Reset metadata to base state
+ if (impl_->base != nullptr) {
+ impl_->metadata = *impl_->base;
+ } else {
+ // Reset to initial state for new table
+ *impl_ = Impl(impl_->metadata.format_version);
+ }
+
+ return *this;
}
Result<std::unique_ptr<TableMetadata>> TableMetadataBuilder::Build() {
- return NotImplemented("TableMetadataBuilder::Build not implemented");
+ // 1. Check for accumulated errors
Review Comment:
Okay, so the logic for Build might be to construct the final TableMetadata
and then call Validate to verify its validity.
##########
src/iceberg/table_metadata.cc:
##########
@@ -378,11 +436,50 @@ TableMetadataBuilder&
TableMetadataBuilder::RemoveEncryptionKey(std::string_view
}
TableMetadataBuilder& TableMetadataBuilder::DiscardChanges() {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ // Clear all changes and errors
+ impl_->changes.clear();
+ impl_->errors.clear();
+
+ // Reset metadata to base state
Review Comment:
Maybe I misunderstood the semantics of discard, and I think the
implementation of java is problematic. I'll read the context of this piece
carefully again to get more understanding
##########
src/iceberg/table_update.cc:
##########
@@ -21,18 +21,32 @@
#include "iceberg/exception.h"
#include "iceberg/table_metadata.h"
+#include "iceberg/table_requirement.h"
#include "iceberg/table_requirements.h"
namespace iceberg::table {
// AssignUUID
void AssignUUID::ApplyTo(TableMetadataBuilder& builder) const {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ builder.AssignUUID(uuid_);
}
Status AssignUUID::GenerateRequirements(TableUpdateContext& context) const {
Review Comment:
It is not currently in use. The expected usage scenario is to use
TableMetadataBuilder to construct the final TableMetadata object and obtain a
list of TableUpdate, then call GenerateRequirements to generate a list of
TableRequirement. Finally, use the list of TableUpdate and TableRequirement to
call the UpdateTable interface of the catalog.
--
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]
