You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "zjw1111 (via GitHub)" <gi...@apache.org> on 2026/06/09 03:42:42 UTC
[PR] feat(core): introduce storage metadata components [paimon-cpp]
zjw1111 opened a new pull request, #62:
URL: https://github.com/apache/paimon-cpp/pull/62
### Purpose
Linked issue: close #xxx
Introduce core storage metadata components migrated from Alibaba Paimon C++:
- Disk IO channel and IO manager helpers.
- Writer memory manager.
- Partition metadata/statistics models.
- Tag metadata model and JSON serialization helpers.
- View metadata model.
Migration scope:
- Requested and migrated files: 15 files under `src/paimon/core/disk/`, `src/paimon/core/memory/`, `src/paimon/core/partition/`, `src/paimon/core/tag/`, and `src/paimon/core/view/`.
- Additional dependency files migrated in this PR: none.
- Requested files skipped: none.
- Known dependencies intentionally left for later migration batches include `paimon/core/snapshot.h`, `paimon/core/utils/batch_writer.h`, `paimon/core/io/compact_increment.h`, `paimon/core/io/data_increment.h`, and `paimon/core/casting/cast_executor.h`.
License and contributor handling:
- Converted Alibaba-owned source headers to ASF license headers.
- No third-party license or NOTICE updates were required.
- External contributor analysis found per-file threshold hits in `src/paimon/core/tag/*` and `src/paimon/core/view/view.h`; no `Co-authored-by` trailers were added per migration decision.
### Tests
- `git diff --check`
- `git diff --no-index --check <empty-dir> src/paimon/core/disk src/paimon/core/memory src/paimon/core/partition src/paimon/core/tag src/paimon/core/view`
- `clang-format --dry-run --Werror src/paimon/core/disk/file_channel_manager.h src/paimon/core/disk/file_io_channel.cpp src/paimon/core/disk/file_io_channel.h src/paimon/core/disk/io_manager.h src/paimon/core/disk/io_manager_test.cpp src/paimon/core/memory/writer_memory_manager.cpp src/paimon/core/memory/writer_memory_manager.h src/paimon/core/memory/writer_memory_manager_test.cpp src/paimon/core/partition/partition_info.h src/paimon/core/partition/partition_statistics.h src/paimon/core/partition/partition_statistics_test.cpp src/paimon/core/tag/tag.cpp src/paimon/core/tag/tag.h src/paimon/core/tag/tag_test.cpp src/paimon/core/view/view.h`
### API and Format
This PR adds source files under `src/paimon/core`. It does not change public headers under `include/` or storage formats by itself.
### Documentation
No documentation updates.
### Generative AI tooling
Migrated-by: OpenAI Codex
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): introduce storage metadata components [paimon-cpp]
Posted by "leaves12138 (via GitHub)" <gi...@apache.org>.
leaves12138 merged PR #62:
URL: https://github.com/apache/paimon-cpp/pull/62
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): introduce storage metadata components [paimon-cpp]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #62:
URL: https://github.com/apache/paimon-cpp/pull/62#discussion_r3377768119
##########
src/paimon/core/memory/writer_memory_manager.cpp:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "paimon/core/memory/writer_memory_manager.h"
+
+#include <cassert>
+
+#include "fmt/format.h"
+#include "paimon/core/utils/batch_writer.h"
+
+namespace paimon {
+
+void WriterMemoryManager::RegisterWriter(BatchWriter* writer) {
+ UpdateWriterMemory(writer);
+}
+
+void WriterMemoryManager::UnregisterWriter(BatchWriter* writer) {
+ auto iter = writer_memory_.find(writer);
+ if (iter == writer_memory_.end()) {
+ return;
+ }
+
+ assert(total_memory_ >= iter->second);
+ total_memory_ -= iter->second;
+ writer_memory_.erase(iter);
+}
+
+void WriterMemoryManager::RefreshWriterMemory(BatchWriter* writer) {
+ UpdateWriterMemory(writer);
+}
+
+Status WriterMemoryManager::OnWriteCompleted(BatchWriter* writer) {
+ UpdateWriterMemory(writer);
+ if (total_memory_ < memory_limit_) {
+ return Status::OK();
+ }
+
+ return ShrinkToLimit();
+}
+
+void WriterMemoryManager::UpdateWriterMemory(BatchWriter* writer) {
+ uint64_t current_memory_usage = writer->GetMemoryUsage();
+ auto [iter, inserted] = writer_memory_.emplace(writer, current_memory_usage);
+ if (inserted) {
+ total_memory_ += current_memory_usage;
+ } else {
+ uint64_t previous_memory = iter->second;
+ if (current_memory_usage >= previous_memory) {
+ total_memory_ += (current_memory_usage - previous_memory);
+ } else {
+ assert(total_memory_ >= previous_memory - current_memory_usage);
+ total_memory_ -= (previous_memory - current_memory_usage);
+ }
+ iter->second = current_memory_usage;
+ }
+}
+
+WriterMemoryManager::Candidate WriterMemoryManager::PickLargest() const {
+ Candidate candidate;
+ for (const auto& [writer, memory] : writer_memory_) {
+ if (memory > candidate.memory) {
+ candidate = {writer, memory};
+ }
+ }
+ return candidate;
+}
+
+Status WriterMemoryManager::ShrinkToLimit() {
+ while (true) {
+ if (total_memory_ < memory_limit_) {
+ return Status::OK();
+ }
Review Comment:
Same boundary issue as `OnWriteCompleted`: if `total_memory_ == memory_limit_`, this loop will attempt to flush even though it is not above the limit (per the class documentation). Changing the condition to `total_memory_ <= memory_limit_` avoids extra flushing and reduces the chance of hitting the 'unable to release memory' error when already at the limit.
##########
src/paimon/core/memory/writer_memory_manager.cpp:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "paimon/core/memory/writer_memory_manager.h"
+
+#include <cassert>
+
+#include "fmt/format.h"
+#include "paimon/core/utils/batch_writer.h"
+
+namespace paimon {
+
+void WriterMemoryManager::RegisterWriter(BatchWriter* writer) {
+ UpdateWriterMemory(writer);
+}
+
+void WriterMemoryManager::UnregisterWriter(BatchWriter* writer) {
+ auto iter = writer_memory_.find(writer);
+ if (iter == writer_memory_.end()) {
+ return;
+ }
+
+ assert(total_memory_ >= iter->second);
+ total_memory_ -= iter->second;
+ writer_memory_.erase(iter);
+}
+
+void WriterMemoryManager::RefreshWriterMemory(BatchWriter* writer) {
+ UpdateWriterMemory(writer);
+}
+
+Status WriterMemoryManager::OnWriteCompleted(BatchWriter* writer) {
+ UpdateWriterMemory(writer);
+ if (total_memory_ < memory_limit_) {
+ return Status::OK();
+ }
+
+ return ShrinkToLimit();
+}
Review Comment:
The header comment says the manager flushes when memory usage *exceeds* the limit, but the current check flushes even when `total_memory_ == memory_limit_`. Consider using `<=` here (and similarly in `ShrinkToLimit`) to avoid unnecessary flushes when exactly at the configured limit.
##########
src/paimon/core/tag/tag.h:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <limits>
+#include <map>
+#include <memory>
+#include <optional>
+#include <string>
Review Comment:
`Tag` uses `std::vector<int64_t>` and `double_t` in its public API, but this header does not include `<vector>` (for `std::vector`) nor `<cmath>` (for `double_t`). This can break builds depending on include order; add the missing standard headers (or switch `double_t` to `double`).
##########
src/paimon/core/partition/partition_statistics.h:
##########
@@ -0,0 +1,140 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <map>
+#include <sstream>
+#include <string>
+#include <utility>
+
+#include "paimon/common/utils/jsonizable.h"
+#include "paimon/common/utils/rapidjson_util.h"
+#include "rapidjson/allocators.h"
+#include "rapidjson/document.h"
+#include "rapidjson/rapidjson.h"
+
+namespace paimon {
+
+class PartitionStatistics : public Jsonizable<PartitionStatistics> {
+ public:
+ using SpecType = std::map<std::string, std::string>;
+ PartitionStatistics(const SpecType& spec, int64_t record_count, int64_t file_size_in_bytes,
+ int64_t file_count, int64_t last_file_creation_time, int32_t total_buckets)
+ : spec_(spec),
+ record_count_(record_count),
+ file_size_in_bytes_(file_size_in_bytes),
+ file_count_(file_count),
+ last_file_creation_time_(last_file_creation_time),
+ total_buckets_(total_buckets) {}
+
+ const SpecType& Spec() const {
+ return spec_;
+ }
+ int64_t RecordCount() const {
+ return record_count_;
+ }
+ int64_t FileSizeInBytes() const {
+ return file_size_in_bytes_;
+ }
+ int64_t FileCount() const {
+ return file_count_;
+ }
+ int64_t LastFileCreationTime() const {
+ return last_file_creation_time_;
+ }
+ int32_t TotalBuckets() const {
+ return total_buckets_;
+ }
+
+ rapidjson::Value ToJson(rapidjson::Document::AllocatorType* allocator) const
+ noexcept(false) override {
+ rapidjson::Value obj(rapidjson::kObjectType);
+ obj.AddMember(rapidjson::StringRef(FIELD_SPEC),
+ RapidJsonUtil::SerializeValue(spec_, allocator).Move(), *allocator);
+ obj.AddMember(rapidjson::StringRef(FIELD_RECORD_COUNT),
+ RapidJsonUtil::SerializeValue(record_count_, allocator).Move(), *allocator);
+ obj.AddMember(rapidjson::StringRef(FIELD_FILE_SIZE_IN_BYTES),
+ RapidJsonUtil::SerializeValue(file_size_in_bytes_, allocator).Move(),
+ *allocator);
+ obj.AddMember(rapidjson::StringRef(FIELD_FILE_COUNT),
+ RapidJsonUtil::SerializeValue(file_count_, allocator).Move(), *allocator);
+ obj.AddMember(rapidjson::StringRef(FIELD_LAST_FILE_CREATION_TIME),
+ RapidJsonUtil::SerializeValue(last_file_creation_time_, allocator).Move(),
+ *allocator);
+ obj.AddMember(rapidjson::StringRef(FIELD_TOTAL_BUCKETS),
+ RapidJsonUtil::SerializeValue(total_buckets_, allocator).Move(), *allocator);
+ return obj;
+ }
+
+ void FromJson(const rapidjson::Value& obj) noexcept(false) override {
+ spec_ = RapidJsonUtil::DeserializeKeyValue<SpecType>(obj, FIELD_SPEC);
+ record_count_ = RapidJsonUtil::DeserializeKeyValue<int64_t>(obj, FIELD_RECORD_COUNT);
+ file_size_in_bytes_ =
+ RapidJsonUtil::DeserializeKeyValue<int64_t>(obj, FIELD_FILE_SIZE_IN_BYTES);
+ file_count_ = RapidJsonUtil::DeserializeKeyValue<int64_t>(obj, FIELD_FILE_COUNT);
+ last_file_creation_time_ =
+ RapidJsonUtil::DeserializeKeyValue<int64_t>(obj, FIELD_LAST_FILE_CREATION_TIME);
+ total_buckets_ = RapidJsonUtil::DeserializeKeyValue<int32_t>(obj, FIELD_TOTAL_BUCKETS);
+ }
+
+ bool operator==(const PartitionStatistics& rhs) const {
+ return record_count_ == rhs.record_count_ &&
+ file_size_in_bytes_ == rhs.file_size_in_bytes_ && file_count_ == rhs.file_count_ &&
+ last_file_creation_time_ == rhs.last_file_creation_time_ &&
+ total_buckets_ == rhs.total_buckets_ && spec_ == rhs.spec_;
+ }
+
+ std::string ToString() const {
+ std::ostringstream oss;
+ oss << "{spec={";
+ bool first = true;
+ for (const auto& kv : spec_) {
+ if (!first) oss << ", ";
+ first = false;
+ oss << kv.first << ":" << kv.second;
+ }
+ oss << "}, recordCount=" << record_count_ << ", fileSizeInBytes=" << file_size_in_bytes_
+ << ", fileCount=" << file_count_
+ << ", lastFileCreationTime=" << last_file_creation_time_
+ << ", totalBuckets=" << total_buckets_ << "}";
+ return oss.str();
+ }
+
+ private:
+ JSONIZABLE_FRIEND_AND_DEFAULT_CTOR(PartitionStatistics);
+
+ private:
+ static constexpr const char* FIELD_SPEC = "spec";
Review Comment:
There are two consecutive `private:` sections with no access change in between. Removing the duplicate `private:` improves readability and avoids noise in this header.
##########
src/paimon/core/memory/writer_memory_manager.cpp:
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "paimon/core/memory/writer_memory_manager.h"
+
+#include <cassert>
+
+#include "fmt/format.h"
+#include "paimon/core/utils/batch_writer.h"
+
+namespace paimon {
+
+void WriterMemoryManager::RegisterWriter(BatchWriter* writer) {
+ UpdateWriterMemory(writer);
+}
+
+void WriterMemoryManager::UnregisterWriter(BatchWriter* writer) {
+ auto iter = writer_memory_.find(writer);
+ if (iter == writer_memory_.end()) {
+ return;
+ }
+
+ assert(total_memory_ >= iter->second);
+ total_memory_ -= iter->second;
+ writer_memory_.erase(iter);
+}
+
+void WriterMemoryManager::RefreshWriterMemory(BatchWriter* writer) {
+ UpdateWriterMemory(writer);
+}
+
+Status WriterMemoryManager::OnWriteCompleted(BatchWriter* writer) {
+ UpdateWriterMemory(writer);
+ if (total_memory_ < memory_limit_) {
+ return Status::OK();
+ }
Review Comment:
There is no test covering the exact-boundary case `total_memory_ == memory_limit_`. Adding a test for this would validate the intended semantics (flush on `>` vs `>=`) and prevent behavior changes that cause unnecessary flushes.
##########
src/paimon/core/tag/tag.cpp:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "paimon/core/tag/tag.h"
+
+#include <cassert>
+
+#include "paimon/common/utils/rapidjson_util.h"
+#include "paimon/fs/file_system.h"
+#include "paimon/result.h"
+#include "paimon/status.h"
+#include "rapidjson/document.h"
+#include "rapidjson/rapidjson.h"
+
+namespace paimon {
+
+Tag::Tag(const std::optional<int32_t>& version, const int64_t id, const int64_t schema_id,
+ const std::string& base_manifest_list,
+ const std::optional<int64_t>& base_manifest_list_size,
+ const std::string& delta_manifest_list,
+ const std::optional<int64_t>& delta_manifest_list_size,
+ const std::optional<std::string>& changelog_manifest_list,
+ const std::optional<int64_t>& changelog_manifest_list_size,
+ const std::optional<std::string>& index_manifest, const std::string& commit_user,
+ const int64_t commit_identifier, const CommitKind commit_kind, const int64_t time_millis,
+ const std::optional<std::map<int32_t, int64_t>>& log_offsets,
+ const std::optional<int64_t>& total_record_count,
+ const std::optional<int64_t>& delta_record_count,
+ const std::optional<int64_t>& changelog_record_count,
+ const std::optional<int64_t>& watermark, const std::optional<std::string>& statistics,
+ const std::optional<std::map<std::string, std::string>>& properties,
+ const std::optional<int64_t>& next_row_id,
+ const std::optional<std::vector<int64_t>>& tag_create_time,
+ const std::optional<double_t>& tag_time_retained)
+ : Snapshot(version, id, schema_id, base_manifest_list, base_manifest_list_size,
+ delta_manifest_list, delta_manifest_list_size, changelog_manifest_list,
+ changelog_manifest_list_size, index_manifest, commit_user, commit_identifier,
+ commit_kind, time_millis, log_offsets, total_record_count, delta_record_count,
+ changelog_record_count, watermark, statistics, properties, next_row_id),
+ tag_create_time_(tag_create_time),
+ tag_time_retained_(tag_time_retained) {}
+
+bool Tag::operator==(const Tag& other) const {
+ if (this == &other) {
+ return true;
+ }
+ return Snapshot::operator==(other) && tag_create_time_ == other.tag_create_time_ &&
+ tag_time_retained_ == other.tag_time_retained_;
+}
+
+bool Tag::TEST_Equal(const Tag& other) const {
+ if (this == &other) {
+ return true;
+ }
+
+ return Snapshot::TEST_Equal(other) && tag_create_time_ == other.tag_create_time_ &&
+ tag_time_retained_ == other.tag_time_retained_;
+}
+
+Result<Snapshot> Tag::TrimToSnapshot() const {
+ return Snapshot(Version(), Id(), SchemaId(), BaseManifestList(), BaseManifestListSize(),
+ DeltaManifestList(), DeltaManifestListSize(), ChangelogManifestList(),
+ ChangelogManifestListSize(), IndexManifest(), CommitUser(), CommitIdentifier(),
+ GetCommitKind(), TimeMillis(), LogOffsets(), TotalRecordCount(),
+ DeltaRecordCount(), ChangelogRecordCount(), Watermark(), Statistics(),
+ Properties(), NextRowId());
+}
Review Comment:
`TrimToSnapshot()` is new behavior but isn't exercised by the added tests. Adding a unit test that verifies the returned `Snapshot` preserves the base `Snapshot` fields and drops `tagCreateTime` / `tagTimeRetained` would catch regressions in field mapping.
##########
src/paimon/core/view/view.h:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include <map>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "paimon/result.h"
+#include "paimon/schema/schema.h"
+#include "paimon/status.h"
+#include "paimon/type_fwd.h"
+#include "paimon/visibility.h"
+
+struct ArrowSchema;
Review Comment:
This new header appears to include and forward-declare several unused dependencies (`<map>`, `<vector>`, `paimon/result.h`, `paimon/status.h`, `paimon/type_fwd.h`, and `struct ArrowSchema`). Since this is a public interface header, trimming includes (and potentially forward-declaring `class Schema;` instead of including `schema.h`) will reduce compile-time coupling.
##########
src/paimon/core/disk/file_channel_manager.h:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include <memory>
+#include <mutex>
+#include <random>
+#include <string>
+
+#include "paimon/common/utils/path_util.h"
+#include "paimon/common/utils/uuid.h"
+#include "paimon/core/disk/file_io_channel.h"
+#include "paimon/fs/local/local_file_system.h"
+#include "paimon/result.h"
Review Comment:
This header depends on `paimon/fs/local/local_file_system.h` even though the API accepts a generic `FileSystem` and should not require the local FS implementation. Prefer including the abstract filesystem header (and any direct dependencies like `paimon/status.h` if `Status` is referenced) to avoid unnecessary coupling via transitive includes.
##########
src/paimon/core/tag/tag.cpp:
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "paimon/core/tag/tag.h"
+
+#include <cassert>
+
+#include "paimon/common/utils/rapidjson_util.h"
+#include "paimon/fs/file_system.h"
+#include "paimon/result.h"
+#include "paimon/status.h"
+#include "rapidjson/document.h"
+#include "rapidjson/rapidjson.h"
+
+namespace paimon {
+
+Tag::Tag(const std::optional<int32_t>& version, const int64_t id, const int64_t schema_id,
+ const std::string& base_manifest_list,
+ const std::optional<int64_t>& base_manifest_list_size,
+ const std::string& delta_manifest_list,
+ const std::optional<int64_t>& delta_manifest_list_size,
+ const std::optional<std::string>& changelog_manifest_list,
+ const std::optional<int64_t>& changelog_manifest_list_size,
+ const std::optional<std::string>& index_manifest, const std::string& commit_user,
+ const int64_t commit_identifier, const CommitKind commit_kind, const int64_t time_millis,
+ const std::optional<std::map<int32_t, int64_t>>& log_offsets,
+ const std::optional<int64_t>& total_record_count,
+ const std::optional<int64_t>& delta_record_count,
+ const std::optional<int64_t>& changelog_record_count,
+ const std::optional<int64_t>& watermark, const std::optional<std::string>& statistics,
+ const std::optional<std::map<std::string, std::string>>& properties,
+ const std::optional<int64_t>& next_row_id,
+ const std::optional<std::vector<int64_t>>& tag_create_time,
+ const std::optional<double_t>& tag_time_retained)
+ : Snapshot(version, id, schema_id, base_manifest_list, base_manifest_list_size,
+ delta_manifest_list, delta_manifest_list_size, changelog_manifest_list,
+ changelog_manifest_list_size, index_manifest, commit_user, commit_identifier,
+ commit_kind, time_millis, log_offsets, total_record_count, delta_record_count,
+ changelog_record_count, watermark, statistics, properties, next_row_id),
+ tag_create_time_(tag_create_time),
+ tag_time_retained_(tag_time_retained) {}
+
+bool Tag::operator==(const Tag& other) const {
+ if (this == &other) {
+ return true;
+ }
+ return Snapshot::operator==(other) && tag_create_time_ == other.tag_create_time_ &&
+ tag_time_retained_ == other.tag_time_retained_;
+}
+
+bool Tag::TEST_Equal(const Tag& other) const {
+ if (this == &other) {
+ return true;
+ }
+
+ return Snapshot::TEST_Equal(other) && tag_create_time_ == other.tag_create_time_ &&
+ tag_time_retained_ == other.tag_time_retained_;
+}
+
+Result<Snapshot> Tag::TrimToSnapshot() const {
+ return Snapshot(Version(), Id(), SchemaId(), BaseManifestList(), BaseManifestListSize(),
+ DeltaManifestList(), DeltaManifestListSize(), ChangelogManifestList(),
+ ChangelogManifestListSize(), IndexManifest(), CommitUser(), CommitIdentifier(),
+ GetCommitKind(), TimeMillis(), LogOffsets(), TotalRecordCount(),
+ DeltaRecordCount(), ChangelogRecordCount(), Watermark(), Statistics(),
+ Properties(), NextRowId());
+}
+
+rapidjson::Value Tag::ToJson(rapidjson::Document::AllocatorType* allocator) const noexcept(false) {
+ rapidjson::Value obj(rapidjson::kObjectType);
+ obj = Snapshot::ToJson(allocator);
Review Comment:
The local `obj` is constructed and then immediately overwritten by `Snapshot::ToJson(allocator)`. Initializing `obj` directly from the base implementation avoids redundant work and can prevent accidental extra copies/moves in RapidJSON values.
##########
src/paimon/core/tag/tag_test.cpp:
##########
@@ -0,0 +1,247 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#include "paimon/core/tag/tag.h"
+
+#include "gtest/gtest.h"
+#include "paimon/common/utils/string_utils.h"
+#include "paimon/fs/local/local_file_system.h"
+#include "paimon/result.h"
+#include "paimon/status.h"
+#include "paimon/testing/utils/testharness.h"
+
+namespace paimon::test {
+
+class TagTest : public testing::Test {
+ public:
+ static std::string ReplaceAll(const std::string& str, const bool serialized) {
+ std::string replaced_str = StringUtils::Replace(str, " ", "");
+ replaced_str = StringUtils::Replace(replaced_str, "\t", "");
+ replaced_str = StringUtils::Replace(replaced_str, "\n", "");
+ if (serialized) {
+ replaced_str = StringUtils::Replace(replaced_str, ".0", ".000000000");
+ }
+ return replaced_str;
+ }
Review Comment:
The `.0` → `.000000000` normalization is overly broad and can corrupt already-serialized values (e.g., `2.000000000` would become `2.000000000000000000`). This makes the JSON round-trip assertions fragile against serializer formatting changes; consider making the normalization target only exact single-decimal forms (e.g., regex/bounded replacements) or compare parsed JSON structures instead of raw strings.
##########
src/paimon/core/partition/partition_info.h:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#pragma once
+
+#include <cstdint>
+#include <memory>
+#include <vector>
+
+#include "arrow/type.h"
Review Comment:
`arrow/type.h` is not referenced anywhere in this header as shown. Since this file only defines schema/field mapping structs, removing this include would reduce compile-time dependencies (and avoids pulling Arrow headers into unrelated compilation units).
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): introduce storage metadata components [paimon-cpp]
Posted by "zjw1111 (via GitHub)" <gi...@apache.org>.
zjw1111 commented on PR #62:
URL: https://github.com/apache/paimon-cpp/pull/62#issuecomment-4655865857
Thanks @SteNicholas, @ChaomingZhangCN for your previous contributions to paimon-cpp. This migration PR carries that work forward into Apache Paimon C++.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org