align with java impl & spec

Author: wgtmac

src/iceberg/metrics/commit_report.cc

@@ -24,48 +24,51 @@
 
 namespace iceberg {
 
-std::unique_ptr<CommitMetrics> CommitMetrics::Of(MetricsContext& context) {
+std::unique_ptr<CommitMetrics> CommitMetrics::Make(MetricsContext& context) {
   auto m = std::unique_ptr<CommitMetrics>(new CommitMetrics());
-  m->total_duration = context.GetTimer("total-duration");
+  m->total_duration = context.GetTimer("total-duration", TimerUnit::kNanoseconds);
   m->attempts = context.GetCounter("attempts");
   return m;
 }
 
 std::unique_ptr<CommitMetrics> CommitMetrics::Noop() {
-  return CommitMetrics::Of(*MetricsContext::Null());
+  return CommitMetrics::Make(*MetricsContext::Noop());
 }
 
-void CommitMetrics::PopulateResult(CommitMetricsResult& result) const {
-  result.total_duration =
-      total_duration ? TimerResult{.unit = std::string(total_duration->Unit()),
-                                   .count = total_duration->Count(),
-                                   .total_duration = total_duration->TotalDuration()}
-                     : TimerResult{};
-  result.attempts =
-      attempts ? CounterResult{.unit = attempts->unit(), .value = attempts->value()}
-               : CounterResult{};
+CommitMetricsResult CommitMetrics::ToResult() const {
+  CommitMetricsResult result;
+  if (total_duration && !total_duration->IsNoop()) {
+    result.total_duration =
+        TimerResult{.unit = std::string(total_duration->Unit()),
+                    .count = total_duration->Count(),
+                    .total_duration = total_duration->TotalDuration()};
+  }
+  if (attempts && !attempts->IsNoop()) {
+    result.attempts = CounterResult{.unit = attempts->unit(), .value = attempts->value()};
+  }
+  return result;
 }
 
 CommitMetricsResult CommitMetricsResult::From(
     const CommitMetrics& live_metrics,
     const std::unordered_map<std::string, std::string>& snapshot_summary) {
-  CommitMetricsResult result;
-  live_metrics.PopulateResult(result);
+  auto result = live_metrics.ToResult();
 
-  // Helpers: parse a summary key and wrap as a typed CounterResult.
-  auto count_field = [&snapshot_summary](const std::string& key) -> CounterResult {
+  auto count_field =
+      [&snapshot_summary](const std::string& key) -> std::optional<CounterResult> {
     auto it = snapshot_summary.find(key);
-    if (it == snapshot_summary.end()) return {};
+    if (it == snapshot_summary.end()) return std::nullopt;
     auto parsed = StringUtils::ParseNumber<int64_t>(it->second);
-    return {.unit = CounterUnit::kCount,
-            .value = parsed.has_value() ? parsed.value() : 0};
+    if (!parsed.has_value()) return std::nullopt;
+    return CounterResult{.unit = CounterUnit::kCount, .value = parsed.value()};
   };
-  auto bytes_field = [&snapshot_summary](const std::string& key) -> CounterResult {
+  auto bytes_field =
+      [&snapshot_summary](const std::string& key) -> std::optional<CounterResult> {
     auto it = snapshot_summary.find(key);
-    if (it == snapshot_summary.end()) return {.unit = CounterUnit::kBytes};
+    if (it == snapshot_summary.end()) return std::nullopt;
     auto parsed = StringUtils::ParseNumber<int64_t>(it->second);
-    return {.unit = CounterUnit::kBytes,
-            .value = parsed.has_value() ? parsed.value() : 0};
+    if (!parsed.has_value()) return std::nullopt;
+    return CounterResult{.unit = CounterUnit::kBytes, .value = parsed.value()};
   };
 
   result.added_data_files = count_field(SnapshotSummaryFields::kAddedDataFiles);
@@ -77,12 +80,12 @@ CommitMetricsResult CommitMetricsResult::From(
   result.added_positional_delete_files =
       count_field(SnapshotSummaryFields::kAddedPosDeleteFiles);
   result.added_dvs = count_field(SnapshotSummaryFields::kAddedDVs);
+  result.removed_delete_files = count_field(SnapshotSummaryFields::kRemovedDeleteFiles);
   result.removed_positional_delete_files =
       count_field(SnapshotSummaryFields::kRemovedPosDeleteFiles);
   result.removed_dvs = count_field(SnapshotSummaryFields::kRemovedDVs);
   result.removed_equality_delete_files =
       count_field(SnapshotSummaryFields::kRemovedEqDeleteFiles);
-  result.removed_delete_files = count_field(SnapshotSummaryFields::kRemovedDeleteFiles);
   result.total_delete_files = count_field(SnapshotSummaryFields::kTotalDeleteFiles);
   result.added_records = count_field(SnapshotSummaryFields::kAddedRecords);
   result.removed_records = count_field(SnapshotSummaryFields::kDeletedRecords);

src/iceberg/metrics/commit_report.h

@@ -20,6 +20,7 @@
 #pragma once
 
 #include <memory>
+#include <optional>
 #include <string>
 #include <unordered_map>
 
@@ -31,112 +32,112 @@
 
 namespace iceberg {
 
-// Forward declaration: CommitMetricsResult is defined later in this header.
-struct CommitMetricsResult;
-
-/// \brief Live commit metrics collected during a table commit operation.
-///
-/// Tracks the overall commit duration and retry count. File/record counts come
-/// from the snapshot summary after the commit succeeds and are stored separately
-/// in CommitMetricsResult.
-class ICEBERG_EXPORT CommitMetrics {
- public:
-  /// \brief Create a CommitMetrics instance backed by the given MetricsContext.
-  static std::unique_ptr<CommitMetrics> Of(MetricsContext& context);
-
-  /// \brief Create a CommitMetrics instance with all-noop timer and counter.
-  static std::unique_ptr<CommitMetrics> Noop();
-
-  /// \brief Snapshot timer and counter values into the corresponding fields of result.
-  ///
-  /// Only total_duration and attempts are written; the caller is responsible for
-  /// populating the remaining snapshot-summary fields.
-  void PopulateResult(CommitMetricsResult& result) const;
-
-  /// \brief Timer measuring total wall-clock time of the commit call.
-  std::shared_ptr<Timer> total_duration;
-
-  /// \brief Counter for the number of commit attempts (including retries).
-  std::shared_ptr<Counter> attempts;
-
- private:
-  CommitMetrics() = default;
-};
+// Forward declaration: CommitMetrics is defined later in this header.
+class CommitMetrics;
 
 /// \brief Immutable snapshot of commit metrics for use in CommitReport.
+///
+/// Populated by CommitMetrics::ToResult() after a commit completes. File and
+/// record counts can also be combined from the snapshot summary with From().
 struct ICEBERG_EXPORT CommitMetricsResult {
   /// \brief Total wall-clock duration of the commit attempt.
-  TimerResult total_duration;
+  std::optional<TimerResult> total_duration;
   /// \brief Number of commit attempts (1 on success without retries).
-  CounterResult attempts;
+  std::optional<CounterResult> attempts;
   /// \brief Number of data files added in this commit.
-  CounterResult added_data_files;
+  std::optional<CounterResult> added_data_files;
   /// \brief Number of data files removed in this commit.
-  CounterResult removed_data_files;
+  std::optional<CounterResult> removed_data_files;
   /// \brief Total live data files after this commit.
-  CounterResult total_data_files;
+  std::optional<CounterResult> total_data_files;
   /// \brief Number of delete files added in this commit.
-  CounterResult added_delete_files;
+  std::optional<CounterResult> added_delete_files;
   /// \brief Equality delete files added.
-  CounterResult added_equality_delete_files;
+  std::optional<CounterResult> added_equality_delete_files;
   /// \brief Positional delete files added.
-  CounterResult added_positional_delete_files;
+  std::optional<CounterResult> added_positional_delete_files;
   /// \brief Deletion vectors added.
-  CounterResult added_dvs;
+  std::optional<CounterResult> added_dvs;
+  /// \brief Number of delete files removed in this commit.
+  std::optional<CounterResult> removed_delete_files;
   /// \brief Positional delete files removed.
-  CounterResult removed_positional_delete_files;
+  std::optional<CounterResult> removed_positional_delete_files;
   /// \brief Deletion vectors removed.
-  CounterResult removed_dvs;
+  std::optional<CounterResult> removed_dvs;
   /// \brief Equality delete files removed.
-  CounterResult removed_equality_delete_files;
-  /// \brief Number of delete files removed in this commit.
-  CounterResult removed_delete_files;
+  std::optional<CounterResult> removed_equality_delete_files;
   /// \brief Total live delete files after this commit.
-  CounterResult total_delete_files;
+  std::optional<CounterResult> total_delete_files;
   /// \brief Number of records added in this commit.
-  CounterResult added_records;
+  std::optional<CounterResult> added_records;
   /// \brief Number of records removed in this commit.
-  CounterResult removed_records;
+  std::optional<CounterResult> removed_records;
   /// \brief Total live records after this commit.
-  CounterResult total_records;
+  std::optional<CounterResult> total_records;
   /// \brief Total byte size of files added.
-  CounterResult added_files_size_bytes;
+  std::optional<CounterResult> added_files_size_bytes;
   /// \brief Total byte size of files removed.
-  CounterResult removed_files_size_bytes;
+  std::optional<CounterResult> removed_files_size_bytes;
   /// \brief Total byte size of all live files after this commit.
-  CounterResult total_files_size_bytes;
+  std::optional<CounterResult> total_files_size_bytes;
   /// \brief Positional delete records added.
-  CounterResult added_positional_deletes;
+  std::optional<CounterResult> added_positional_deletes;
   /// \brief Positional delete records removed.
-  CounterResult removed_positional_deletes;
+  std::optional<CounterResult> removed_positional_deletes;
   /// \brief Total positional delete records after this commit.
-  CounterResult total_positional_deletes;
+  std::optional<CounterResult> total_positional_deletes;
   /// \brief Equality delete records added.
-  CounterResult added_equality_deletes;
+  std::optional<CounterResult> added_equality_deletes;
   /// \brief Equality delete records removed.
-  CounterResult removed_equality_deletes;
+  std::optional<CounterResult> removed_equality_deletes;
   /// \brief Total equality delete records after this commit.
-  CounterResult total_equality_deletes;
+  std::optional<CounterResult> total_equality_deletes;
   /// \brief Manifest files kept unchanged in this commit.
-  CounterResult kept_manifest_count;
+  std::optional<CounterResult> kept_manifest_count;
   /// \brief Manifest files created in this commit.
-  CounterResult created_manifest_count;
+  std::optional<CounterResult> created_manifest_count;
   /// \brief Manifest files replaced in this commit.
-  CounterResult replaced_manifest_count;
+  std::optional<CounterResult> replaced_manifest_count;
   /// \brief Manifest entries processed in this commit.
-  CounterResult processed_manifest_entries_count;
+  std::optional<CounterResult> processed_manifest_entries_count;
 
   bool operator==(const CommitMetricsResult&) const = default;
 
   /// \brief Build a CommitMetricsResult from live metrics and a snapshot summary map.
   ///
   /// Combines timer/retry measurements from \p live_metrics with records parsed
-  /// from \p snapshot_summary.  Missing or unparseable summary keys default to 0.
+  /// from \p snapshot_summary. Missing or unparseable summary keys are omitted.
   static CommitMetricsResult From(
       const CommitMetrics& live_metrics,
       const std::unordered_map<std::string, std::string>& snapshot_summary);
 };
 
+/// \brief Live commit metrics collected during a table commit operation.
+///
+/// Tracks the overall commit duration and retry count. File/record counts come
+/// from the snapshot summary after the commit succeeds and are stored separately
+/// in CommitMetricsResult.
+class ICEBERG_EXPORT CommitMetrics {
+ public:
+  /// \brief Create a CommitMetrics instance backed by the given MetricsContext.
+  static std::unique_ptr<CommitMetrics> Make(MetricsContext& context);
+
+  /// \brief Create a CommitMetrics instance with all-noop timer and counter.
+  static std::unique_ptr<CommitMetrics> Noop();
+
+  /// \brief Snapshot current timer and counter values into a CommitMetricsResult.
+  CommitMetricsResult ToResult() const;
+
+  /// \brief Timer measuring total wall-clock time of the commit call.
+  std::shared_ptr<Timer> total_duration;
+
+  /// \brief Counter for the number of commit attempts (including retries).
+  std::shared_ptr<Counter> attempts;
+
+ private:
+  CommitMetrics() = default;
+};
+
 /// \brief Report generated after a commit operation.
 ///
 /// Contains metrics about the changes made in a commit.

src/iceberg/metrics/counter.cc

@@ -29,7 +29,8 @@ class NoopCounter final : public Counter {
  public:
   using Counter::Increment;
   void Increment(int64_t) override {}
-  int64_t value() const override { return 0; }
+  int64_t value() const override { return -1; }
+  CounterUnit unit() const override { return CounterUnit::kUndefined; }
   bool IsNoop() const override { return true; }
 };
 

src/iceberg/metrics/counter.h

@@ -26,6 +26,8 @@
 #include <utility>
 
 #include "iceberg/iceberg_export.h"
+#include "iceberg/result.h"
+#include "iceberg/util/string_util.h"
 
 namespace iceberg {
 
@@ -52,11 +54,13 @@ ICEBERG_EXPORT constexpr std::string_view ToString(CounterUnit unit) noexcept {
 /// \brief Parse a CounterUnit from a string.
 ///
 /// \param s The string to parse ("count", "bytes", or "undefined").
-/// \return The CounterUnit, or CounterUnit::kCount if unrecognized.
-ICEBERG_EXPORT constexpr CounterUnit CounterUnitFromString(std::string_view s) noexcept {
-  if (s == "bytes") return CounterUnit::kBytes;
-  if (s == "undefined") return CounterUnit::kUndefined;
-  return CounterUnit::kCount;
+/// \return The CounterUnit, or an InvalidArgument error if unrecognized.
+ICEBERG_EXPORT inline Result<CounterUnit> CounterUnitFromString(std::string_view s) {
+  auto unit = StringUtils::ToLower(s);
+  if (unit == "count") return CounterUnit::kCount;
+  if (unit == "bytes") return CounterUnit::kBytes;
+  if (unit == "undefined") return CounterUnit::kUndefined;
+  return InvalidArgument("Invalid unit: {}", s);
 }
 
 /// \brief Abstract counter for tracking event totals.