wgtmac commented on code in PR #511:
URL: https://github.com/apache/iceberg-cpp/pull/511#discussion_r2706864142
##########
src/iceberg/table_update.cc:
##########
@@ -446,4 +447,56 @@ std::unique_ptr<TableUpdate> SetLocation::Clone() const {
return std::make_unique<SetLocation>(location_);
}
+// SetStatistics
+
+int64_t SetStatistics::snapshot_id() const { return
statistics_file_->snapshot_id; }
+
+void SetStatistics::ApplyTo(TableMetadataBuilder& builder) const {
+ builder.SetStatistics(statistics_file_);
+}
+
+void SetStatistics::GenerateRequirements(TableUpdateContext& context) const {
+ // SetStatistics doesn't generate any requirements
+}
+
+bool SetStatistics::Equals(const TableUpdate& other) const {
+ if (other.kind() != Kind::kSetStatistics) {
+ return false;
+ }
+ const auto& other_set = static_cast<const SetStatistics&>(other);
Review Comment:
use checked_cast
##########
src/iceberg/json_internal.cc:
##########
@@ -1399,6 +1401,18 @@ nlohmann::json ToJson(const TableUpdate& update) {
json[kLocation] = u.location();
break;
}
+ case TableUpdate::Kind::kSetStatistics: {
+ const auto& u = internal::checked_cast<const
table::SetStatistics&>(update);
+ json[kAction] = kActionSetStatistics;
+ json[kStatistics] = ToJson(*u.statistics_file());
Review Comment:
check if `u.statistics_file()` is null before calling ToJson
##########
src/iceberg/table_metadata.cc:
##########
@@ -1173,6 +1177,45 @@ Status TableMetadataBuilder::Impl::SetRef(const
std::string& name,
return {};
}
+Status TableMetadataBuilder::Impl::SetStatistics(
+ const std::shared_ptr<StatisticsFile>& statistics_file) {
Review Comment:
This is not resolved yet.
##########
src/iceberg/table_metadata.cc:
##########
@@ -1173,6 +1177,40 @@ Status TableMetadataBuilder::Impl::SetRef(const
std::string& name,
return {};
}
+Status TableMetadataBuilder::Impl::SetStatistics(
+ const std::shared_ptr<StatisticsFile>& statistics_file) {
+ ICEBERG_PRECHECK(statistics_file != nullptr, "Cannot set null statistics
file");
+
+ // Find and replace existing statistics for the same snapshot_id, or add new
one
+ auto it = std::ranges::find_if(
+ metadata_.statistics,
+ [snapshot_id = statistics_file->snapshot_id](const auto& stat) {
+ return stat && stat->snapshot_id == snapshot_id;
+ });
+
+ if (it != metadata_.statistics.end()) {
+ *it = statistics_file;
+ } else {
+ metadata_.statistics.push_back(statistics_file);
+ }
+
+
changes_.push_back(std::make_unique<table::SetStatistics>(std::move(statistics_file)));
+ return {};
+}
+
+Status TableMetadataBuilder::Impl::RemoveStatistics(int64_t snapshot_id) {
+ auto removed_count =
+ std::erase_if(metadata_.statistics, [snapshot_id](const auto& stat) {
+ return stat && stat->snapshot_id == snapshot_id;
+ });
+ if (removed_count == 0) {
+ return {};
+ }
+
+ changes_.push_back(std::make_unique<table::RemoveStatistics>(snapshot_id));
Review Comment:
```suggestion
if (removed_count != 0) {
changes_.push_back(std::make_unique<table::RemoveStatistics>(snapshot_id));
}
```
##########
src/iceberg/table_metadata.cc:
##########
@@ -1038,6 +1041,7 @@ void
TableMetadataBuilder::Impl::SetLocation(std::string_view location) {
}
metadata_.location = std::string(location);
changes_.push_back(std::make_unique<table::SetLocation>(std::string(location)));
+ return;
Review Comment:
```suggestion
```
##########
src/iceberg/table_update.cc:
##########
@@ -446,4 +447,56 @@ std::unique_ptr<TableUpdate> SetLocation::Clone() const {
return std::make_unique<SetLocation>(location_);
}
+// SetStatistics
+
+int64_t SetStatistics::snapshot_id() const { return
statistics_file_->snapshot_id; }
+
+void SetStatistics::ApplyTo(TableMetadataBuilder& builder) const {
+ builder.SetStatistics(statistics_file_);
+}
+
+void SetStatistics::GenerateRequirements(TableUpdateContext& context) const {
+ // SetStatistics doesn't generate any requirements
+}
+
+bool SetStatistics::Equals(const TableUpdate& other) const {
+ if (other.kind() != Kind::kSetStatistics) {
+ return false;
+ }
+ const auto& other_set = static_cast<const SetStatistics&>(other);
+ if (!statistics_file_ != !other_set.statistics_file_) {
+ return false;
+ }
+ if (statistics_file_ && !(*statistics_file_ == *other_set.statistics_file_))
{
+ return false;
+ }
+ return true;
+}
+
+std::unique_ptr<TableUpdate> SetStatistics::Clone() const {
+ return std::make_unique<SetStatistics>(statistics_file_);
+}
+
+// RemoveStatistics
+
+void RemoveStatistics::ApplyTo(TableMetadataBuilder& builder) const {
+ builder.RemoveStatistics(snapshot_id_);
+}
+
+void RemoveStatistics::GenerateRequirements(TableUpdateContext& context) const
{
+ // RemoveStatistics doesn't generate any requirements
+}
+
+bool RemoveStatistics::Equals(const TableUpdate& other) const {
+ if (other.kind() != Kind::kRemoveStatistics) {
+ return false;
+ }
+ const auto& other_remove = static_cast<const RemoveStatistics&>(other);
Review Comment:
ditto
##########
src/iceberg/transaction.cc:
##########
@@ -189,6 +191,17 @@ Status Transaction::Apply(PendingUpdate& update) {
ICEBERG_ASSIGN_OR_RAISE(auto location, update_location.Apply());
metadata_builder_->SetLocation(location);
} break;
+ case PendingUpdate::Kind::kUpdateStatistics: {
+ auto& update_statistics =
internal::checked_cast<UpdateStatistics&>(update);
+ ICEBERG_ASSIGN_OR_RAISE(auto result, update_statistics.Apply());
+ // Apply statistics changes to the metadata builder
Review Comment:
```suggestion
```
##########
src/iceberg/table_metadata.cc:
##########
@@ -1589,12 +1627,14 @@ TableMetadataBuilder&
TableMetadataBuilder::SuppressHistoricalSnapshots() {
}
TableMetadataBuilder& TableMetadataBuilder::SetStatistics(
- const std::shared_ptr<StatisticsFile>& statistics_file) {
- throw IcebergError(std::format("{} not implemented", __FUNCTION__));
+ std::shared_ptr<StatisticsFile> statistics_file) {
+ ICEBERG_BUILDER_RETURN_IF_ERROR(impl_->SetStatistics(statistics_file));
Review Comment:
```suggestion
ICEBERG_BUILDER_RETURN_IF_ERROR(impl_->SetStatistics(std::move(statistics_file)));
```
--
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]
