lishuxu commented on code in PR #418:
URL: https://github.com/apache/iceberg-cpp/pull/418#discussion_r2629250139
##########
src/iceberg/update/update_properties.cc:
##########
@@ -70,65 +75,56 @@ UpdateProperties& UpdateProperties::Remove(const
std::string& key) {
return *this;
}
-Status UpdateProperties::Apply() {
- if (!catalog_) {
- return InvalidArgument("Catalog is required to apply property updates");
- }
- if (!base_metadata_) {
+Result<PendingUpdate::ApplyResult> UpdateProperties::Apply() {
+ ICEBERG_RETURN_UNEXPECTED(CheckErrors());
+
+ const auto* base_metadata = transaction_->base();
+ if (!base_metadata) {
return InvalidArgument("Base table metadata is required to apply property
updates");
}
- ICEBERG_RETURN_UNEXPECTED(CheckErrors());
-
auto iter = updates_.find(TableProperties::kFormatVersion.key());
if (iter != updates_.end()) {
- try {
- int parsed_version = std::stoi(iter->second);
- if (parsed_version > TableMetadata::kSupportedTableFormatVersion) {
- return InvalidArgument(
- "Cannot upgrade table to unsupported format version: v{}
(supported: v{})",
- parsed_version, TableMetadata::kSupportedTableFormatVersion);
- }
- format_version_ = static_cast<int8_t>(parsed_version);
- } catch (const std::invalid_argument& e) {
- return InvalidArgument("Invalid format version '{}': not a valid
integer",
- iter->second);
- } catch (const std::out_of_range& e) {
- return InvalidArgument("Format version '{}' is out of range",
iter->second);
+ int parsed_version = 0;
+ const auto& val = iter->second;
+ auto [ptr, ec] = std::from_chars(val.data(), val.data() + val.size(),
parsed_version);
+
+ if (ec == std::errc::invalid_argument) {
+ return InvalidArgument("Invalid format version '{}': not a valid
integer", val);
+ } else if (ec == std::errc::result_out_of_range) {
+ return InvalidArgument("Format version '{}' is out of range", val);
+ }
+
+ if (parsed_version > TableMetadata::kSupportedTableFormatVersion) {
+ return InvalidArgument(
+ "Cannot upgrade table to unsupported format version: v{} (supported:
v{})",
+ parsed_version, TableMetadata::kSupportedTableFormatVersion);
}
+ format_version_ = static_cast<int8_t>(parsed_version);
updates_.erase(iter);
}
- if (auto schema = base_metadata_->Schema(); schema.has_value()) {
+ if (auto schema = base_metadata->Schema(); schema.has_value()) {
ICEBERG_RETURN_UNEXPECTED(
MetricsConfig::VerifyReferencedColumns(updates_, *schema.value()));
}
- return {};
-}
-
-Status UpdateProperties::Commit() {
- ICEBERG_RETURN_UNEXPECTED(Apply());
- std::vector<std::unique_ptr<TableUpdate>> updates;
+ ApplyResult result;
if (!updates_.empty()) {
-
updates.emplace_back(std::make_unique<table::SetProperties>(std::move(updates_)));
+ result.updates.emplace_back(
+ std::make_unique<table::SetProperties>(std::move(updates_)));
Review Comment:
Should we keep updates_/removals_ intact when producing ApplyResult?
--
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]
