WZhuo commented on code in PR #386:
URL: https://github.com/apache/iceberg-cpp/pull/386#discussion_r2618385210


##########
src/iceberg/table_metadata.h:
##########
@@ -76,6 +83,8 @@ struct ICEBERG_EXPORT TableMetadata {
   static constexpr int64_t kInvalidSequenceNumber = -1;
   static constexpr int64_t kInitialRowId = 0;
 
+  /// The location of the table metadata file
+  std::string metadata_file_location;

Review Comment:
   If seperate metadata_file_locatin field from TableMetadata, we need to 
convert more fields in some function
   ```
   Status TableMetadataUtil::Write(FileIO& io, const TableMetadata* base, 
TableMetadata& metadata)
   =>
   Status TableMetadataUtil::Write(FileIO& io, const TableMetadata* base, const 
std::string& base_location, TableMetadata& metadata, std::string& new_location)
   ```
   This filed is highly correlative with TableMetadata.BTW, it's also a filed 
of TableMetadata in Java impl.
   



##########
src/iceberg/table_metadata.cc:
##########
@@ -258,13 +291,88 @@ Result<std::unique_ptr<TableMetadata>> 
TableMetadataUtil::Read(
   return TableMetadataFromJson(json);
 }
 
+Status TableMetadataUtil::Write(FileIO& io, const TableMetadata* base,
+                                TableMetadata& metadata) {
+  int version = -1;
+  if (base != nullptr && !base->metadata_file_location.empty()) {
+    // parse current version from location
+    version = ParseVersionFromLocation(base->metadata_file_location);
+  }
+
+  ICEBERG_ASSIGN_OR_RAISE(std::string new_file_location,
+                          NewTableMetadataFilePath(metadata, version + 1));
+  ICEBERG_RETURN_UNEXPECTED(Write(io, new_file_location, metadata));
+  metadata.metadata_file_location = std::move(new_file_location);
+  return {};
+}
+
 Status TableMetadataUtil::Write(FileIO& io, const std::string& location,
                                 const TableMetadata& metadata) {
   auto json = ToJson(metadata);
   ICEBERG_ASSIGN_OR_RAISE(auto json_string, ToJsonString(json));
   return io.WriteFile(location, json_string);
 }
 
+void TableMetadataUtil::DeleteRemovedMetadataFiles(FileIO& io, const 
TableMetadata* base,
+                                                   const TableMetadata& 
metadata) {
+  if (!base) {
+    return;
+  }
+
+  bool delete_after_commit = 
TableProperties::kMetadataDeleteAfterCommitEnabled.value();
+  if (auto it = metadata.properties.find(
+          TableProperties::kMetadataDeleteAfterCommitEnabled.key());
+      it != metadata.properties.end()) {
+    delete_after_commit =
+        StringUtils::EqualsIgnoreCase(it->second, "true") || it->second == "1";
+  }
+
+  if (delete_after_commit) {
+    auto current_files =
+        metadata.metadata_log |
+        std::ranges::to<std::unordered_set<MetadataLogEntry, 
MetadataLogEntry::Hasher>>();
+    std::ranges::for_each(
+        base->metadata_log | std::views::filter([&current_files](const auto& 
entry) {
+          return !current_files.contains(entry);
+        }),
+        [&io](const auto& entry) { auto status = 
io.DeleteFile(entry.metadata_file); });
+  }
+}
+
+int TableMetadataUtil::ParseVersionFromLocation(
+    const std::string_view& metadata_location) {
+  size_t version_start = metadata_location.find_last_of('/') + 1;

Review Comment:
   It's Ok if '/' does not exist, `version_end` will be std::string::npos, and 
return -1.



##########
src/iceberg/table_metadata.h:
##########
@@ -58,6 +59,12 @@ struct ICEBERG_EXPORT MetadataLogEntry {
   friend bool operator==(const MetadataLogEntry& lhs, const MetadataLogEntry& 
rhs) {
     return lhs.timestamp_ms == rhs.timestamp_ms && lhs.metadata_file == 
rhs.metadata_file;
   }
+
+  struct Hasher {

Review Comment:
   SnapshotLogEntry not used currently. Has leave a TODO in 
TableMetadataBuilder::Build



-- 
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]

Reply via email to