SteNicholas commented on code in PR #222:
URL: https://github.com/apache/paimon-cpp/pull/222#discussion_r3954189749
##########
src/paimon/core/schema/schema_validation.cpp:
##########
@@ -134,27 +143,146 @@ bool SchemaValidation::IsComplexType(const
std::shared_ptr<arrow::Field>& field)
BlobUtils::IsBlobField(field));
}
-Status SchemaValidation::ValidateTableSchema(const TableSchema& schema) {
- const auto& field_names = schema.FieldNames();
- PAIMON_RETURN_NOT_OK(ValidateNoDuplicateField(schema.BucketKeys(), "bucket
key"));
- PAIMON_RETURN_NOT_OK(ValidateNoDuplicateField(schema.PrimaryKeys(),
"primary key"));
- PAIMON_RETURN_NOT_OK(ValidateNoDuplicateField(schema.PartitionKeys(),
"partition key"));
- PAIMON_RETURN_NOT_OK(
- Preconditions::CheckState(ObjectUtils::ContainsAll(field_names,
schema.PartitionKeys()),
- "Table column {} should include all
partition fields {}",
- field_names, schema.PartitionKeys()));
- PAIMON_RETURN_NOT_OK(
- Preconditions::CheckState(ObjectUtils::ContainsAll(field_names,
schema.PrimaryKeys()),
- "Table column {} should include all primary
key constraint {}",
- field_names, schema.PrimaryKeys()));
-
- PAIMON_RETURN_NOT_OK(
- ValidateOnlyContainPrimitiveType(schema.Fields(),
schema.PrimaryKeys(), "primary key"));
- PAIMON_RETURN_NOT_OK(
- ValidateOnlyContainPrimitiveType(schema.Fields(),
schema.PartitionKeys(), "partition"));
+Status SchemaValidation::ValidateGenericSchema(const std::vector<DataField>&
fields,
+ const std::vector<std::string>&
bucket_keys,
+ const std::vector<std::string>&
primary_keys,
+ const std::vector<std::string>&
partition_keys) {
+ std::vector<std::string> field_names;
+ field_names.reserve(fields.size());
+ for (const DataField& field : fields) {
+ field_names.push_back(field.Name());
+ }
+ PAIMON_RETURN_NOT_OK(ValidateNoDuplicateField(bucket_keys, "bucket key"));
+ PAIMON_RETURN_NOT_OK(ValidateNoDuplicateField(primary_keys, "primary
key"));
+ PAIMON_RETURN_NOT_OK(ValidateNoDuplicateField(partition_keys, "partition
key"));
+ PAIMON_RETURN_NOT_OK(Preconditions::CheckState(
+ ObjectUtils::ContainsAll(field_names, partition_keys),
+ "Table column {} should include all partition fields {}", field_names,
partition_keys));
+ PAIMON_RETURN_NOT_OK(Preconditions::CheckState(
+ ObjectUtils::ContainsAll(field_names, primary_keys),
+ "Table column {} should include all primary key constraint {}",
field_names, primary_keys));
+ for (const auto& field_name : field_names) {
+ if (SpecialFields::IsSystemField(field_name)) {
+ return Status::Invalid(
+ fmt::format("field name '{}' in schema cannot be special
field.", field_name));
+ }
+ }
+ PAIMON_RETURN_NOT_OK(ValidateOnlyContainPrimitiveType(fields,
primary_keys, "primary key"));
+ PAIMON_RETURN_NOT_OK(ValidateOnlyContainPrimitiveType(fields,
partition_keys, "partition"));
// TODO(lisizhuo.lsz): C++ Paimon do not support timestamp & decimal &
float & double type in
// partition keys for now.
- PAIMON_RETURN_NOT_OK(ValidateNotContainSpecificType(schema.Fields(),
schema.PartitionKeys()));
+ PAIMON_RETURN_NOT_OK(ValidateNotContainSpecificType(fields,
partition_keys));
+ return Status::OK();
+}
+
+Status SchemaValidation::ValidateNewTableSchema(const TableSchema& schema) {
+ const std::map<std::string, std::string>& options = schema.Options();
+ PAIMON_ASSIGN_OR_RAISE(TableType table_type,
TableTypeDefine::FromOptions(options));
+ if (table_type != TableType::TABLE && table_type !=
TableType::MATERIALIZED_TABLE &&
+ table_type != TableType::FORMAT_TABLE) {
+ // Quoted back rather than re-rendered from `table_type`, so the
message says what was
+ // actually asked for.
+ auto type_iter = options.find(Options::TYPE);
+ return Status::NotImplemented(fmt::format(
+ "Cannot create a table whose '{}' is '{}': paimon-cpp does not
implement this table "
+ "type.",
+ Options::TYPE, type_iter == options.end() ? std::string() :
type_iter->second));
+ }
+ if (table_type == TableType::FORMAT_TABLE) {
+ PAIMON_RETURN_NOT_OK(ValidateGenericTableSchema(schema));
+ // At creation the schema's own options are the only ones there are,
and `file-system` is
+ // resolved from them like every other option.
+ return ValidateFormatTableSchema(schema, schema.Options(),
/*file_system=*/nullptr);
Review Comment:
Done. `SchemaValidation::ValidateNewTableSchema()` now takes the caller's
already resolved `FileSystem`, and `SchemaManager::CreateTable()` passes its
own `file_system_` (`schema_manager.cpp:116`). `ValidateFormatTableSchema()`
takes it too, so reading the options through `CoreOptions` there does not
resolve `file-system` a second time — which would fail for a caller that handed
its own file system over rather than naming one.
##########
src/paimon/core/table/format/format_path_validation.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 <map>
+#include <memory>
+#include <string>
+
+#include "paimon/result.h"
+#include "paimon/status.h"
+#include "paimon/table/format/format_table.h"
+
+namespace paimon {
+
+/// Checks that a path a caller handed over really names something of this
format table.
+///
+/// A split and a commit message both come back through an interface that
takes the base type, so
+/// what arrives may belong to another plan, another table or a plan made
before the files moved,
+/// and nothing further down re-checks the paths they name.
+class FormatPathValidation {
+ public:
+ FormatPathValidation() = delete;
+ ~FormatPathValidation() = delete;
+
+ /// Fails when `path` is not a file inside `location`. By path component,
not by string
+ /// prefix: `<location>/../victim` starts with the location and still
resolves outside it.
+ /// Only the path text is checked, so a symbolic link pointing out of the
table is not caught.
+ static Status ValidatePathUnderLocation(const std::string& path, const
std::string& location,
+ const std::string& what);
Review Comment:
Done. Both sides are normalized before being compared.
`ResolveComparablePath()` parses each with `PathUtil::ToPath()`, treats an
empty scheme and `file` as the same file system, compares the scheme
case-insensitively and the authority separately, and resolves a relative local
path against the working directory exactly as `LocalFile` does when it opens
one. The containment check then runs on the normalized inner path by whole path
components, so `<location>/../victim` is still refused.
`/tmp/table` therefore holds `file:///tmp/table/data.parquet`, and a table
located at `./table` matches the absolute paths its own listing hands back.
Covered by `TestTheTableLocationIsRecognisedHoweverItIsWritten` and
`TestATableAtARelativeLocationIsWrittenAndReadBack`.
##########
src/paimon/core/table/format/format_table_commit.h:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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 <memory>
+#include <string>
+#include <vector>
+
+#include "paimon/core/table/format/format_commit_message.h"
+#include "paimon/result.h"
+#include "paimon/status.h"
+#include "paimon/table/format/format_table.h"
+
+namespace paimon {
+
+/// Publishes the files a `FormatTableWrite` produced, by renaming each out of
the `_temporary`
+/// directory it was staged in.
+///
+/// A directory has no metadata to switch, so a commit is not atomic across
files: it renames them
+/// one at a time, and a reader scanning midway sees the ones renamed so far.
Each rename does
+/// guarantee that a file becomes visible whole, never half-written.
+///
+/// A commit that fails partway tries to remove the files it had already
renamed, on a best-effort
+/// basis: a file that cannot be removed is reported in the log and stays. An
overwriting commit is
+/// further limited - the data it replaces is deleted before the new files are
published and cannot
+/// be brought back, so a failure there leaves the table without the replaced
data.
+///
+/// Only the messages this job's own writers produced may be passed in. The
checks here can tell
+/// that a message's path belongs to this table, sits in the partition it
declares, and is staged
+/// rather than already published - not whose staged file it is.
+///
+/// Not thread-safe. Separate commits may add to one table at once, each
publishing only the files
+/// its own messages name; two overwriting commits over the same directory
race, since an overwrite
+/// clears everything committed there before publishing anything.
+class FormatTableCommit {
+ public:
+ /// @param table Table to commit to.
+ /// @param overwrite Whether the commit replaces the data already in the
directories it writes
+ /// to, instead of adding to it. Without a static partition that
means every partition
+ /// the commit touches; with one it means the partitions that spec
covers, whether or
+ /// not this commit wrote to them.
+ /// @param static_partition Partition the commit writes to, keyed by
partition field name. It
+ /// may name only the leading partition keys, in which case it
stands for every
+ /// partition below that prefix. Empty means the partitions are
whatever the written
+ /// files say they are.
+ static Result<std::unique_ptr<FormatTableCommit>> Create(
+ const std::shared_ptr<FormatTable>& table, bool overwrite,
+ const std::map<std::string, std::string>& static_partition);
+
+ ~FormatTableCommit();
+
+ /// Renames every written file into place, first clearing what it replaces
when the commit
+ /// overwrites.
+ Status Commit(const std::vector<FormatCommitMessage>& commit_messages);
Review Comment:
Supported rather than refused. `FormatTableCommit` reads
`dynamic-partition-overwrite` through `CoreOptions` once at `Create()`, and
`ReplacesOnlyWrittenPartitions()` is true only when the option is on **and**
the table is partitioned. Otherwise an overwrite clears everything the table
holds — the condition Java's `FormatTableCommit` applies — so an overwrite
whose query returns nothing still empties the table instead of leaving stale
data behind.
Both branches have tests, including the empty-message case and a table whose
data directory has not been created yet.
##########
src/paimon/core/table/format/format_table_file_store_commit.h:
##########
@@ -0,0 +1,109 @@
+/*
+ * 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 <memory>
+#include <optional>
+#include <string>
+#include <vector>
+
+#include "paimon/core/table/format/format_commit_message.h"
+#include "paimon/defs.h"
+#include "paimon/file_store_commit.h"
+#include "paimon/result.h"
+#include "paimon/status.h"
+
+namespace paimon {
+
+class FormatTable;
+class Metrics;
+
+/// Commits a format table through the `FileStoreCommit` interface, so that a
caller holding a
+/// table path commits it the way it commits any other table. Java Paimon does
the same through
+/// `FormatTable.newBatchWriteBuilder()`.
+///
+/// Most of `FileStoreCommit` is about snapshots and manifests, which a format
table has none of:
+/// expiring them, rolling back to one, and filtering by a commit identifier
recorded in one all
+/// refer to state this table does not keep. Each is refused rather than
quietly doing nothing, so
+/// a caller moving between table types finds out at the call rather than from
a table that did
+/// not change. `RowIdCheckConflict()` is the one exception, since it returns
a reference and has
+/// no way to report a refusal.
+///
+/// What is left is what a directory of files can do: `Commit()`,
`Overwrite()` and `Abort()`.
+class FormatTableFileStoreCommit : public FileStoreCommit {
+ public:
+ static Result<std::unique_ptr<FormatTableFileStoreCommit>> Create(
+ const std::shared_ptr<FormatTable>& table);
+
+ ~FormatTableFileStoreCommit() override;
+
+ Status Commit(const std::vector<std::shared_ptr<CommitMessage>>&
commit_messages,
+ int64_t commit_identifier = BATCH_WRITE_COMMIT_IDENTIFIER,
+ std::optional<int64_t> watermark = std::nullopt) override;
Review Comment:
Done here, and applied to the rest of the new surface: the two public
`FormatTable::Create()` overloads in
`include/paimon/table/format/format_table.h` no longer default
`location_carries_paimon_metadata` or `dynamic_options`, and every caller —
production and test — passes them explicitly.
--
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]
