From 0335f0deb9efe3bbf1f08a4d4a8dc5ad46d24174 Mon Sep 17 00:00:00 2001 From: Junwang Zhao Date: Thu, 25 Dec 2025 22:15:43 +0800 Subject: [PATCH] chore: refactor some obvious non optimal coding style --- src/iceberg/json_internal.cc | 4 ++-- src/iceberg/manifest/manifest_reader.cc | 4 ++-- src/iceberg/manifest/manifest_reader.h | 3 --- src/iceberg/schema.cc | 1 - src/iceberg/schema.h | 3 +++ src/iceberg/table_metadata.cc | 8 ++++---- src/iceberg/util/snapshot_util.cc | 4 ++-- src/iceberg/util/timepoint.cc | 9 +++------ src/iceberg/util/timepoint.h | 2 +- 9 files changed, 17 insertions(+), 21 deletions(-) diff --git a/src/iceberg/json_internal.cc b/src/iceberg/json_internal.cc index 82cb8ee71..9d955d46b 100644 --- a/src/iceberg/json_internal.cc +++ b/src/iceberg/json_internal.cc @@ -787,9 +787,9 @@ nlohmann::json ToJson(const TableMetadata& table_metadata) { // write properties map json[kProperties] = table_metadata.properties.configs(); - if (std::ranges::find_if(table_metadata.snapshots, [&](const auto& snapshot) { + if (std::ranges::any_of(table_metadata.snapshots, [&](const auto& snapshot) { return snapshot->snapshot_id == table_metadata.current_snapshot_id; - }) != table_metadata.snapshots.cend()) { + })) { json[kCurrentSnapshotId] = table_metadata.current_snapshot_id; } else { json[kCurrentSnapshotId] = nlohmann::json::value_t::null; diff --git a/src/iceberg/manifest/manifest_reader.cc b/src/iceberg/manifest/manifest_reader.cc index 2ba377fa2..ba1ff860b 100644 --- a/src/iceberg/manifest/manifest_reader.cc +++ b/src/iceberg/manifest/manifest_reader.cc @@ -570,7 +570,7 @@ bool RequireStatsProjection(const std::shared_ptr& row_filter, return false; } const std::unordered_set selected(columns.cbegin(), columns.cend()); - if (selected.contains(ManifestReader::kAllColumns)) { + if (selected.contains(Schema::kAllColumns)) { return false; } if (std::ranges::all_of(kStatsColumns, [&selected](const std::string& col) { @@ -594,7 +594,7 @@ Result> ProjectSchema(std::shared_ptr schema, std::vector ManifestReader::WithStatsColumns( const std::vector& columns) { - if (std::ranges::contains(columns, ManifestReader::kAllColumns)) { + if (std::ranges::contains(columns, Schema::kAllColumns)) { return columns; } else { std::vector updated_columns{columns}; diff --git a/src/iceberg/manifest/manifest_reader.h b/src/iceberg/manifest/manifest_reader.h index a35c1fb94..748d0e33d 100644 --- a/src/iceberg/manifest/manifest_reader.h +++ b/src/iceberg/manifest/manifest_reader.h @@ -36,9 +36,6 @@ namespace iceberg { /// \brief Read manifest entries from a manifest file. class ICEBERG_EXPORT ManifestReader { public: - /// \brief Special value to select all columns from manifest files. - static constexpr std::string_view kAllColumns = "*"; - virtual ~ManifestReader() = default; /// \brief Read all manifest entries in the manifest file. diff --git a/src/iceberg/schema.cc b/src/iceberg/schema.cc index 414219f86..4b4b6c269 100644 --- a/src/iceberg/schema.cc +++ b/src/iceberg/schema.cc @@ -163,7 +163,6 @@ Result> Schema::GetAccessorById( Result> Schema::Select(std::span names, bool case_sensitive) const { - const std::string kAllColumns = "*"; if (std::ranges::find(names, kAllColumns) != names.end()) { auto struct_type = ToStructType(*this); return FromStructType(std::move(*struct_type), std::nullopt); diff --git a/src/iceberg/schema.h b/src/iceberg/schema.h index bb9839625..410fa59ed 100644 --- a/src/iceberg/schema.h +++ b/src/iceberg/schema.h @@ -48,6 +48,9 @@ class ICEBERG_EXPORT Schema : public StructType { static constexpr int32_t kInitialSchemaId = 0; static constexpr int32_t kInvalidColumnId = -1; + /// \brief Special value to select all columns from manifest files. + static constexpr std::string_view kAllColumns = "*"; + explicit Schema(std::vector fields, std::optional schema_id = std::nullopt, std::vector identifier_field_ids = {}); diff --git a/src/iceberg/table_metadata.cc b/src/iceberg/table_metadata.cc index ba5b8f328..46b7f92dc 100644 --- a/src/iceberg/table_metadata.cc +++ b/src/iceberg/table_metadata.cc @@ -548,12 +548,12 @@ Result TableMetadataBuilder::Impl::AddSortOrder(const SortOrder& order) // is now the last) bool is_new_order = last_added_order_id_.has_value() && - std::ranges::find_if(changes_, [new_order_id](const auto& change) { + std::ranges::any_of(changes_, [new_order_id](const auto& change) { return change->kind() == TableUpdate::Kind::kAddSortOrder && internal::checked_cast(*change) .sort_order() ->order_id() == new_order_id; - }) != changes_.cend(); + }); last_added_order_id_ = is_new_order ? std::make_optional(new_order_id) : std::nullopt; return new_order_id; } @@ -613,12 +613,12 @@ Result TableMetadataBuilder::Impl::AddPartitionSpec(const PartitionSpec // is now the last) bool is_new_spec = last_added_spec_id_.has_value() && - std::ranges::find_if(changes_, [new_spec_id](const auto& change) { + std::ranges::any_of(changes_, [new_spec_id](const auto& change) { return change->kind() == TableUpdate::Kind::kAddPartitionSpec && internal::checked_cast(*change) .spec() ->spec_id() == new_spec_id; - }) != changes_.cend(); + }); last_added_spec_id_ = is_new_spec ? std::make_optional(new_spec_id) : std::nullopt; return new_spec_id; } diff --git a/src/iceberg/util/snapshot_util.cc b/src/iceberg/util/snapshot_util.cc index 1243a1093..d2d083ea7 100644 --- a/src/iceberg/util/snapshot_util.cc +++ b/src/iceberg/util/snapshot_util.cc @@ -124,7 +124,7 @@ Result>> SnapshotUtil::OldestAncestorAft } // the first ancestor after the given time can't be determined - return NotFound("Cannot find snapshot older than {}", FormatTimestamp(timestamp_ms)); + return NotFound("Cannot find snapshot older than {}", FormatTimePointMs(timestamp_ms)); } Result> SnapshotUtil::SnapshotIdsBetween(const Table& table, @@ -232,7 +232,7 @@ Result SnapshotUtil::SnapshotIdAsOfTime(const Table& table, TimePointMs timestamp_ms) { auto snapshot_id = OptionalSnapshotIdAsOfTime(table, timestamp_ms); ICEBERG_CHECK(snapshot_id.has_value(), "Cannot find a snapshot older than {}", - FormatTimestamp(timestamp_ms)); + FormatTimePointMs(timestamp_ms)); return snapshot_id.value(); } diff --git a/src/iceberg/util/timepoint.cc b/src/iceberg/util/timepoint.cc index a8bc77080..0381e90a6 100644 --- a/src/iceberg/util/timepoint.cc +++ b/src/iceberg/util/timepoint.cc @@ -45,12 +45,9 @@ int64_t UnixNsFromTimePointNs(TimePointNs time_point_ns) { .count(); } -std::string FormatTimestamp(TimePointMs time_point_ns) { - // Convert TimePointMs to system_clock::time_point - auto unix_ms = UnixMsFromTimePointMs(time_point_ns); - auto time_point = - std::chrono::system_clock::time_point(std::chrono::milliseconds(unix_ms)); - auto time_t = std::chrono::system_clock::to_time_t(time_point); +std::string FormatTimePointMs(TimePointMs time_point_ms) { + auto unix_ms = UnixMsFromTimePointMs(time_point_ms); + auto time_t = std::chrono::system_clock::to_time_t(time_point_ms); // Format as ISO 8601-like string: YYYY-MM-DD HH:MM:SS std::ostringstream oss; diff --git a/src/iceberg/util/timepoint.h b/src/iceberg/util/timepoint.h index 48e630ae4..6052c94ae 100644 --- a/src/iceberg/util/timepoint.h +++ b/src/iceberg/util/timepoint.h @@ -47,6 +47,6 @@ ICEBERG_EXPORT Result TimePointNsFromUnixNs(int64_t unix_ns); ICEBERG_EXPORT int64_t UnixNsFromTimePointNs(TimePointNs time_point_ns); /// \brief Returns a human-readable string representation of a TimePointMs -ICEBERG_EXPORT std::string FormatTimestamp(TimePointMs time_point_ns); +ICEBERG_EXPORT std::string FormatTimePointMs(TimePointMs time_point_ms); } // namespace iceberg