zjw1111 commented on code in PR #222: URL: https://github.com/apache/paimon-cpp/pull/222#discussion_r3852394974
########## src/paimon/core/table/format/format_file_naming.h: ########## @@ -0,0 +1,73 @@ +/* + * 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 <string> + +#include "paimon/result.h" + +namespace paimon { + +/// Names the data files one format table write produces. +/// +/// `{prefix}{uuid}-{n}.{extension}`, the convention every paimon writer follows: the uuid belongs +/// to this write and the counter to its files, so two concurrent writers cannot collide. +/// +/// A file is staged under `_temporary/.tmp.{uuid}` beside where it will end up and takes its real +/// name only on commit, as Java Paimon's `RenamingTwoPhaseOutputStream` does. Both the directory +/// and the name are hidden, which is the Hive-style convention for output that is not committed +/// table data and is what a scan of this table skips. +class FormatFileNaming { + public: + static constexpr char kDefaultDataFilePrefix[] = "data-"; + /// Directory a staged file waits in, shared with every other writer of the same table. + static constexpr char kTempDirName[] = "_temporary"; + static constexpr char kTempFilePrefix[] = ".tmp."; + + /// @param extension File extension without its dot, which is the format's identifier. + /// @param prefix File name prefix, from `data-file.prefix`. It may not be hidden by the + /// `_` / `.` convention, since a scan skips every such file. + static Result<FormatFileNaming> Create(const std::string& extension, const std::string& prefix); + + FormatFileNaming() = default; Review Comment: `Create()` validates four things here - non-empty extension, no path separator or `..` in either extension or prefix, prefix not hidden by the `_`/`.` convention, and successful UUID generation - so construction can genuinely fail. But the public `FormatFileNaming() = default;` on line 49 lets a caller bypass all of it: a default-constructed instance has an empty `uuid_` and `extension_`, and `NextFileName()` would then produce `data--0.`. It seems to exist only so that `FormatTableWrite::Impl` can hold it as a value member (`format_table_write.cpp:184`, assigned later at `:297`). Could you change `Create()` to return `Result<std::unique_ptr<FormatFileNaming>>` and drop the public default constructor? `Impl::naming` would become a `std::unique_ptr`, and the invariant would stay inside the type. That also matches the `static Create()` + private constructor rule in `docs/code-style.md:187`. ########## src/paimon/core/table/format/format_file_naming.h: ########## @@ -0,0 +1,73 @@ +/* + * 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 <string> + +#include "paimon/result.h" + +namespace paimon { + +/// Names the data files one format table write produces. +/// +/// `{prefix}{uuid}-{n}.{extension}`, the convention every paimon writer follows: the uuid belongs +/// to this write and the counter to its files, so two concurrent writers cannot collide. +/// +/// A file is staged under `_temporary/.tmp.{uuid}` beside where it will end up and takes its real +/// name only on commit, as Java Paimon's `RenamingTwoPhaseOutputStream` does. Both the directory +/// and the name are hidden, which is the Hive-style convention for output that is not committed +/// table data and is what a scan of this table skips. Review Comment: A note on the design rather than something to change here. Mirroring `RenamingTwoPhaseOutputStream` is the right starting point, but it ties publishing to a rename. On an object store a rename is neither atomic nor cheap - it is a server-side copy followed by a delete - so a commit that publishes many files effectively pays for the data twice, and a failure part-way through the rename loop cannot be undone atomically (the rollback in `format_table_commit.cpp:292-308` is best-effort for exactly this reason). Java has a second implementation for this case: `MultiPartUploadTwoPhaseOutputStream` (`paimon-common/src/main/java/org/apache/paimon/fs/MultiPartUploadTwoPhaseOutputStream.java`, with the `OssTwoPhaseOutputStream` / `S3TwoPhaseOutputStream` / `JindoTwoPhaseOutputStream` subclasses). There the data is uploaded straight to its final key and only `CompleteMultipartUpload` is deferred to commit, so there is no copy and the publish is a single atomic call. This PR is already very large, so please don't change it here. Could you just leave a TODO in this comment block noting that only the rename-based semantics are supported today and pointing at `MultiPartUploadTwoPhaseOutputStream` as the follow-up? The object-store path can then be optimised in a later PR. ########## src/paimon/rest/rest_catalog.cpp: ########## @@ -404,8 +410,28 @@ Result<std::shared_ptr<Schema>> RestCatalog::LoadTableSchema(const Identifier& i return checked_pointer_cast<Schema>(schema); } +Result<std::shared_ptr<FormatTable>> RestCatalog::LoadFormatTable( + const Identifier& identifier) const { + PAIMON_ASSIGN_OR_RAISE(bool is_system_table, identifier.IsSystemTable()); + if (is_system_table || CatalogUtils::IsSystemDatabase(identifier.GetDatabaseName())) { + return Status::Invalid(fmt::format("{} is a system table, so it cannot be a format table", + identifier.GetFullName())); + } Review Comment: `CatalogUtils::CheckNotSystemTable` (`src/paimon/core/catalog/catalog_utils.cpp:55`) already wraps exactly this pair of checks, and its comment spells out why the ordering matters: *"The system database is checked first so that an identifier of 'sys' is rejected without being parsed as a table name."* This evaluates `identifier.IsSystemTable()` first, which is the order that comment warns against. `IsSystemTable()` goes through `SplitTableName()` (`identifier.cpp:117-155`), which returns `Status::Invalid("Invalid table name: ...")` for a malformed name - so `sys.<malformed>` surfaces a parse error instead of the system-table refusal. The same pattern appears a few lines up at `:384`. Could you call `CatalogUtils::CheckNotSystemTable(identifier, "load format table")` here instead? If the format-table specific wording is worth keeping, extracting an `IsSystemTableIdentifier()` helper into `CatalogUtils` that preserves the database-first order would let both call sites share one implementation. ########## src/paimon/core/table/format/format_commit_message.h: ########## @@ -0,0 +1,65 @@ +/* + * 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 <string> + +#include "paimon/commit_message.h" + +namespace paimon { + +/// One file a `FormatTableWrite` has written but not yet published. +/// +/// The file is complete on disk under `temp_file_path`, which a scan skips; committing renames it +/// to `file_path`, which is what makes it part of the table. +/// +/// It is a `CommitMessage` so that a format table can be written and committed through +/// `FileStoreWrite` and `FileStoreCommit` like any other table, as Java Paimon's +/// `TwoPhaseCommitMessage` is. It names a staged path rather than files to record in a manifest, +/// so `CommitMessage::Serialize()` refuses it: there is no cross-runtime encoding for one, and a +/// write and its commit belong to the same process. +struct FormatCommitMessage : public CommitMessage { + FormatCommitMessage(const std::string& _temp_file_path, const std::string& _file_path, + const std::map<std::string, std::string>& _partition, int64_t _record_count, + int64_t _file_size) + : temp_file_path(_temp_file_path), + file_path(_file_path), + partition(_partition), + record_count(_record_count), + file_size(_file_size) {} + + ~FormatCommitMessage() override = default; + + std::string ToString() const; Review Comment: `ToString()` is declared here but defined in `format_table_commit.cpp:134`, and there is no `format_commit_message.cpp` in this directory. Any translation unit that includes this header without also linking `format_table_commit.cpp` - a future unit test covering the commit message on its own, for instance - would hit an undefined symbol. Could you define it inline in the header? The body is a single `fmt::format` call, so it would need `fmt/format.h` here, which seems a fair trade for keeping the header's contract and its implementation together. ########## src/paimon/core/utils/partition_path_utils.cpp: ########## @@ -49,8 +50,17 @@ const std::bitset<128>& PartitionPathUtils::CharToEscape() { return bitset; } +Status PartitionPathUtils::ValidatePartitionValueForPath(const std::string& value, + bool only_value) { + if (value.empty() || (only_value && (value == "." || value == ".."))) { + return Status::Invalid("Partition value '" + value + + "' cannot be used as a partition path component."); Review Comment: `docs/code-style.md:308-310` (String Formatting) asks for `fmt::format()` rather than `+` concatenation, and every other `Status::Invalid` added in this PR already follows it. Could you switch this one too? ```cpp return Status::Invalid( fmt::format("Partition value '{}' cannot be used as a partition path component.", value)); ``` -- 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]
