lxy-9602 commented on code in PR #222:
URL: https://github.com/apache/paimon-cpp/pull/222#discussion_r3821872858
##########
src/paimon/common/utils/serialization_utils.h:
##########
@@ -47,6 +49,50 @@ class SerializationUtils {
SerializationUtils() = delete;
~SerializationUtils() = delete;
+ /// Longest string this encoding can carry:
`MemorySegmentOutputStream::WriteString` prefixes a
+ /// string with a 16-bit length and `DataInputStream::ReadString` reads it
back unsigned, so a
+ /// longer one would be written in full behind a truncated length and read
back as a different
+ /// string.
+ static constexpr size_t MAX_SERIALIZED_STRING_BYTES = 65535;
+
+ /// Fails when `value` could not be written and read back as itself. Meant
to run before a
+ /// byte is written, so that the failure happens where the bytes came from.
+ ///
+ /// @param what What the string is, for the error message.
+ /// @param container What it is part of, so the message says which
encoding refused it.
+ static Status CheckSerializableString(const std::string& value, const
char* what,
+ const char* container) {
+ if (value.size() > MAX_SERIALIZED_STRING_BYTES) {
+ return Status::Invalid(
+ fmt::format("{} is {} bytes, more than the {} a serialized {}
can carry", what,
+ value.size(), MAX_SERIALIZED_STRING_BYTES,
container));
+ }
+ return Status::OK();
+ }
+
+ /// Rejects an element count that the bytes left to read could not
possibly hold.
+ ///
+ /// The bytes are untrusted, so a negative count, or one the rest of the
stream could not
+ /// encode, must come back as a `Status` rather than reach a `reserve()`
that would throw past
+ /// the error model. `min_bytes_per_element` is what makes the check
tight: an entry costs
+ /// several bytes, so comparing against the bytes left one-for-one would
let a count many
+ /// times larger than the input through.
+ static Status CheckElementCount(int32_t count, int64_t
min_bytes_per_element, const char* what,
+ DataInputStream* in) {
+ if (count < 0) {
Review Comment:
It looks like `GetPos` and `Length` are const methods, so we can probably
just use `const DataInputStream& in` directly.
##########
src/paimon/core/table/table.cpp:
##########
@@ -44,6 +48,28 @@ Result<std::shared_ptr<Table>> Table::Create(const
std::shared_ptr<FileSystem>&
fmt::format("load table schema for {} failed",
identifier.ToString()));
}
+ // Only a managed table is a `Table`; describing another type as one would
hand back snapshots
+ // and manifests it never had. Checked here because this is the one place
every `Table` built
+ // from a table directory passes through, catalogs included.
+ PAIMON_ASSIGN_OR_RAISE(TableType table_type,
+
TableTypeDefine::FromOptions((*latest_schema)->Options()));
+ if (table_type == TableType::FORMAT_TABLE) {
+ return Status::Invalid(
+ fmt::format("Cannot open format table '{}' as a Table, please use "
+ "'Catalog::GetFormatTable' or 'FormatTable::Create'.",
+ identifier.ToString()));
+ }
+ // A materialized table is a managed table that also carries the SQL it
materializes.
+ if (table_type != TableType::TABLE && table_type !=
TableType::MATERIALIZED_TABLE) {
+ const std::map<std::string, std::string>& options =
(*latest_schema)->Options();
+ auto type_iter = options.find(Options::TYPE);
+ return Status::NotImplemented(fmt::format(
+ "Cannot open table '{}': its '{}' is '{}', a table type paimon-cpp
does not "
+ "implement, and opening it as a managed table would look for
snapshots it never had.",
+ identifier.ToString(), Options::TYPE,
+ type_iter == options.end() ? std::string() : type_iter->second));
+ }
Review Comment:
This code is highly similar to `CatalogUtils::CheckManagedTableType`. Please
extract the common logic or call into that implementation directly.
##########
src/paimon/core/table/format/format_file_naming.h:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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 written under a hidden `.{name}.tmp` and takes its real name
only on commit, both
+/// marks a scan and an orphan-file cleaner already treat as uncommitted
output.
+class FormatFileNaming {
+ public:
+ static constexpr char DEFAULT_DATA_FILE_PREFIX[] = "data-";
+ static constexpr char TEMP_FILE_SUFFIX[] = ".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 =
DEFAULT_DATA_FILE_PREFIX);
+
+ FormatFileNaming() = default;
+
+ /// The name the next file takes once committed.
+ std::string NextFileName();
+
+ /// The hidden name a file is written under before it is committed.
+ static std::string ToTempFileName(const std::string& file_name) {
+ return "." + file_name + TEMP_FILE_SUFFIX;
+ }
Review Comment:
The temporary file handling here seems inconsistent with Java. In Java,
there is an additional `_temporary` directory. Is there a reason for this
difference in design?
##########
src/paimon/core/schema/schema_validation.cpp:
##########
@@ -134,27 +143,132 @@ 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()));
+Status SchemaValidation::ValidateKeyFieldNames(const std::vector<std::string>&
field_names,
+ const std::vector<std::string>&
bucket_keys,
+ const std::vector<std::string>&
primary_keys,
+ const std::vector<std::string>&
partition_keys) {
+ 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));
+ }
+ }
+ 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));
+ return ValidateFormatTableSchema(schema);
+ }
+ return ValidateTableSchema(schema);
+}
+
+Status SchemaValidation::ValidateGenericTableSchema(const TableSchema& schema)
{
+ PAIMON_RETURN_NOT_OK(ValidateKeyFieldNames(schema.FieldNames(),
schema.BucketKeys(),
+ schema.PrimaryKeys(),
schema.PartitionKeys()));
PAIMON_RETURN_NOT_OK(
ValidateOnlyContainPrimitiveType(schema.Fields(),
schema.PrimaryKeys(), "primary key"));
PAIMON_RETURN_NOT_OK(
ValidateOnlyContainPrimitiveType(schema.Fields(),
schema.PartitionKeys(), "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()));
+ return Status::OK();
+}
+
+Status SchemaValidation::ValidateGenericDataSchema(const DataSchema& schema) {
+ PAIMON_RETURN_NOT_OK(ValidateKeyFieldNames(schema.FieldNames(),
schema.BucketKeys(),
+ schema.PrimaryKeys(),
schema.PartitionKeys()));
+ // A `DataSchema` carries its field types as an arrow schema, so the type
rules read them from
+ // there. Only the partition and primary keys are looked up, and both were
just checked.
+ PAIMON_ASSIGN_OR_RAISE(std::unique_ptr<::ArrowSchema> c_schema,
schema.GetArrowSchema());
+ ScopeGuard schema_guard([&c_schema]() {
ArrowSchemaRelease(c_schema.get()); });
+ PAIMON_ASSIGN_OR_RAISE_FROM_ARROW(std::shared_ptr<arrow::Schema>
arrow_schema,
+ arrow::ImportSchema(c_schema.get()));
+ PAIMON_RETURN_NOT_OK(
+ ValidateOnlyContainPrimitiveType(*arrow_schema, schema.PrimaryKeys(),
"primary key"));
+ PAIMON_RETURN_NOT_OK(
+ ValidateOnlyContainPrimitiveType(*arrow_schema,
schema.PartitionKeys(), "partition"));
+ PAIMON_RETURN_NOT_OK(ValidateNotContainSpecificType(*arrow_schema,
schema.PartitionKeys()));
+ return Status::OK();
Review Comment:
Could we try removing the overloads of `ValidateOnlyContainPrimitiveType`
and `ValidateNotContainSpecificType` and switch to a more general input type
instead? `DataField` already provides utilities for converting between
`DataField`, `arrow::Field`, and `arrow::Schema`, so this might help reduce
some of the current duplication and make future changes easier to maintain.
##########
src/paimon/core/table/format/format_file_naming.h:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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 written under a hidden `.{name}.tmp` and takes its real name
only on commit, both
+/// marks a scan and an orphan-file cleaner already treat as uncommitted
output.
+class FormatFileNaming {
+ public:
+ static constexpr char DEFAULT_DATA_FILE_PREFIX[] = "data-";
+ static constexpr char TEMP_FILE_SUFFIX[] = ".tmp";
+
Review Comment:
In the new code, please use the `kName` naming convention for static const
variables. The same applies to similar cases elsewhere as well—could you please
update them accordingly?
##########
src/paimon/core/table/format/format_file_listing_test.cpp:
##########
@@ -0,0 +1,162 @@
+/*
+ * 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/table/format/format_file_listing.h"
+
+#include <algorithm>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "gtest/gtest.h"
+#include "paimon/common/utils/path_util.h"
+#include "paimon/fs/file_system.h"
+#include "paimon/fs/local/local_file_system.h"
+#include "paimon/testing/utils/testharness.h"
+
+namespace paimon::test {
+
+namespace {
+
+Status WriteAt(const std::shared_ptr<FileSystem>& file_system, const
std::string& path) {
+ std::string parent = PathUtil::GetParentDirPath(path);
+ PAIMON_RETURN_NOT_OK(file_system->Mkdirs(parent));
+ return file_system->WriteFile(path, "row\n", /*overwrite=*/true);
+}
+
+/// The listed files as paths relative to `root`, sorted so the order of a
listing cannot matter.
+Result<std::vector<std::string>> ListNames(const std::shared_ptr<FileSystem>&
file_system,
+ const std::string& root,
+ const FormatDataFileListingOptions&
options) {
+ std::vector<FormatDataSplit::FileMeta> files;
+ PAIMON_RETURN_NOT_OK(ListFormatDataFiles(file_system, root, options,
&files));
+ std::vector<std::string> names;
+ names.reserve(files.size());
+ for (const FormatDataSplit::FileMeta& file : files) {
+ names.push_back(file.file_path.substr(root.size() + 1));
+ }
+ std::sort(names.begin(), names.end());
+ return names;
+}
+
+} // namespace
+
+TEST(FormatFileListingTest, TestDescendsIntoPlainSubdirectories) {
+ std::unique_ptr<UniqueTestDirectory> dir = UniqueTestDirectory::Create();
+ ASSERT_TRUE(dir);
+ std::shared_ptr<FileSystem> file_system =
std::make_shared<LocalFileSystem>();
+ // `data-file.path-directory`, and other engines, put data files below the
partition directory
+ // rather than directly in it, so stopping at the top level would miss
them.
+ ASSERT_OK(WriteAt(file_system, dir->Str() + "/a.parquet"));
+ ASSERT_OK(WriteAt(file_system, dir->Str() + "/nested/b.parquet"));
+ ASSERT_OK(WriteAt(file_system, dir->Str() + "/nested/deeper/c.parquet"));
+
+ ASSERT_OK_AND_ASSIGN(std::vector<std::string> names,
+ ListNames(file_system, dir->Str(),
FormatDataFileListingOptions{}));
+ ASSERT_EQ(names, (std::vector<std::string>{"a.parquet", "nested/b.parquet",
+ "nested/deeper/c.parquet"}));
+}
+
+TEST(FormatFileListingTest, TestHiddenNamesAreSkippedAndNotDescendedInto) {
+ std::unique_ptr<UniqueTestDirectory> dir = UniqueTestDirectory::Create();
+ ASSERT_TRUE(dir);
+ std::shared_ptr<FileSystem> file_system =
std::make_shared<LocalFileSystem>();
+ // A staging tree holds another job's uncommitted output under ordinary
data file names, so
+ // only the directory above them tells the two apart.
+ ASSERT_OK(WriteAt(file_system, dir->Str() + "/a.parquet"));
+ ASSERT_OK(WriteAt(file_system, dir->Str() + "/.b.parquet.tmp"));
+ ASSERT_OK(WriteAt(file_system, dir->Str() + "/_temporary/c.parquet"));
+ ASSERT_OK(WriteAt(file_system, dir->Str() + "/.hive-staging_1/d.parquet"));
+
+ ASSERT_OK_AND_ASSIGN(std::vector<std::string> names,
+ ListNames(file_system, dir->Str(),
FormatDataFileListingOptions{}));
+ ASSERT_EQ(names, (std::vector<std::string>{"a.parquet"}));
+}
+
+TEST(FormatFileListingTest,
TestDefaultPartitionDirectoryIsTheOneHiddenNameThatIsContent) {
+ std::unique_ptr<UniqueTestDirectory> dir = UniqueTestDirectory::Create();
+ ASSERT_TRUE(dir);
+ std::shared_ptr<FileSystem> file_system =
std::make_shared<LocalFileSystem>();
+ // In the value-only layout a partition directory is the bare value, so a
null partition is
+ // named `__DEFAULT_PARTITION__` - a hidden name that nonetheless holds
table data.
+ ASSERT_OK(WriteAt(file_system, dir->Str() +
"/__DEFAULT_PARTITION__/a.parquet"));
+ ASSERT_OK(WriteAt(file_system, dir->Str() + "/_temporary/b.parquet"));
+
+ FormatDataFileListingOptions options;
+ options.partition_levels = 1;
+ options.only_value_in_path = true;
+ options.default_part_name = "__DEFAULT_PARTITION__";
+ ASSERT_OK_AND_ASSIGN(std::vector<std::string> names,
+ ListNames(file_system, dir->Str(), options));
+ ASSERT_EQ(names,
(std::vector<std::string>{"__DEFAULT_PARTITION__/a.parquet"}));
+
+ // With no partition level below the root, that name is a staging tree
like any other.
+ FormatDataFileListingOptions no_partition_level = options;
+ no_partition_level.partition_levels = 0;
+ ASSERT_OK_AND_ASSIGN(std::vector<std::string> without,
+ ListNames(file_system, dir->Str(),
no_partition_level));
+ ASSERT_TRUE(without.empty());
+}
+
+TEST(FormatFileListingTest,
TestReservedDirectoriesAreSkippedOnlyWhenTheyAreMetadata) {
+ std::unique_ptr<UniqueTestDirectory> dir = UniqueTestDirectory::Create();
+ ASSERT_TRUE(dir);
+ std::shared_ptr<FileSystem> file_system =
std::make_shared<LocalFileSystem>();
+ ASSERT_OK(WriteAt(file_system, dir->Str() + "/a.parquet"));
+ ASSERT_OK(WriteAt(file_system, dir->Str() + "/schema/schema-0"));
+
+ FormatDataFileListingOptions metadata_here;
+ metadata_here.skip_reserved_directories = true;
+ ASSERT_OK_AND_ASSIGN(std::vector<std::string> skipped,
+ ListNames(file_system, dir->Str(), metadata_here));
+ ASSERT_EQ(skipped, (std::vector<std::string>{"a.parquet"}));
+
+ // For a table whose schema lives in a metastore, the location is nothing
but data and a
+ // directory of that name is a partition value or a data subdirectory.
+ ASSERT_OK_AND_ASSIGN(std::vector<std::string> kept,
+ ListNames(file_system, dir->Str(),
FormatDataFileListingOptions{}));
+ ASSERT_EQ(kept, (std::vector<std::string>{"a.parquet",
"schema/schema-0"}));
+}
+
+TEST(FormatFileListingTest,
TestMissingRootIsAnErrorButAVanishedSubdirectoryIsNot) {
+ std::unique_ptr<UniqueTestDirectory> dir = UniqueTestDirectory::Create();
+ ASSERT_TRUE(dir);
+ std::shared_ptr<FileSystem> file_system =
std::make_shared<LocalFileSystem>();
+ // A root that is not there means the location is wrong or the data is
gone. The file systems
+ // here report a missing directory as an empty listing, so this only works
because the root is
+ // asked about outright - passing it off as a table with no rows would
hide a mistyped path.
+ std::vector<FormatDataSplit::FileMeta> files;
+ ASSERT_FALSE(ListFormatDataFiles(file_system, dir->Str() + "/absent",
+ FormatDataFileListingOptions{}, &files)
+ .ok());
+
Review Comment:
ASSERT_NOK？
##########
src/paimon/core/table/format/format_file_naming.h:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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 written under a hidden `.{name}.tmp` and takes its real name
only on commit, both
+/// marks a scan and an orphan-file cleaner already treat as uncommitted
output.
+class FormatFileNaming {
+ public:
+ static constexpr char DEFAULT_DATA_FILE_PREFIX[] = "data-";
+ static constexpr char TEMP_FILE_SUFFIX[] = ".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 =
DEFAULT_DATA_FILE_PREFIX);
+
Review Comment:
Please avoid using default parameter values in production code whenever
possible.
##########
src/paimon/core/table/format/format_file_listing.h:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 <string>
+#include <vector>
+
+#include "paimon/status.h"
+#include "paimon/table/format/format_data_split.h"
+
+namespace paimon {
+
+class FileSystem;
+
+/// Whether `name` is a directory this library keeps under a format table's
location as metadata.
+/// A table whose schema lives in a catalog has none.
+bool IsReservedFormatTableDirectory(const std::string& name);
+
Review Comment:
Could we try to avoid introducing global functions where possible? Would it
make sense to place this in a utility class instead?
##########
src/paimon/core/table/format/format_path_validation.h:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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 {
+
Review Comment:
Please avoid introducing global functions; it would be better to encapsulate
this in a shared utility class instead.
##########
src/paimon/core/table/format/format_file_naming_test.cpp:
##########
@@ -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.
+ */
+
+#include "paimon/core/table/format/format_file_naming.h"
+
+#include <string>
+
+#include "gtest/gtest.h"
+#include "paimon/common/utils/string_utils.h"
+#include "paimon/status.h"
+#include "paimon/testing/utils/testharness.h"
+
+namespace paimon::test {
+
+TEST(FormatFileNamingTest, TestNamesAreDataPrefixedAndNumbered) {
+ ASSERT_OK_AND_ASSIGN(FormatFileNaming naming,
FormatFileNaming::Create("parquet"));
+ std::string first = naming.NextFileName();
+ std::string second = naming.NextFileName();
+
+ ASSERT_TRUE(StringUtils::StartsWith(first, "data-"));
+ ASSERT_TRUE(StringUtils::EndsWith(first, "-0.parquet"));
+ ASSERT_TRUE(StringUtils::EndsWith(second, "-1.parquet"));
+ // Both files of one write share its uuid.
+ ASSERT_EQ(first.substr(0, first.size() - std::string("-0.parquet").size()),
+ second.substr(0, second.size() -
std::string("-1.parquet").size()));
+}
+
+TEST(FormatFileNamingTest, TestTwoWritesDoNotCollide) {
+ ASSERT_OK_AND_ASSIGN(FormatFileNaming first_write,
FormatFileNaming::Create("parquet"));
+ ASSERT_OK_AND_ASSIGN(FormatFileNaming second_write,
FormatFileNaming::Create("parquet"));
+ ASSERT_NE(first_write.NextFileName(), second_write.NextFileName());
+}
+
+TEST(FormatFileNamingTest, TestTempNameIsHidden) {
+ std::string temp_name =
FormatFileNaming::ToTempFileName("data-abc-0.parquet");
+ ASSERT_EQ(temp_name, ".data-abc-0.parquet.tmp");
+ // Both marks are what a scan and an orphan-file cleaner already skip.
+ ASSERT_EQ(temp_name[0], '.');
+ ASSERT_TRUE(StringUtils::EndsWith(temp_name, ".tmp"));
+}
+
+TEST(FormatFileNamingTest, TestPrefixComesFromDataFilePrefix) {
+ ASSERT_OK_AND_ASSIGN(FormatFileNaming naming,
FormatFileNaming::Create("parquet", "part-"));
+ std::string name = naming.NextFileName();
+ ASSERT_TRUE(StringUtils::StartsWith(name, "part-"));
+ ASSERT_TRUE(StringUtils::EndsWith(name, "-0.parquet"));
+}
+
+TEST(FormatFileNamingTest, TestRejectsAPrefixThatIsNotOneFileNameComponent) {
+ // The prefix goes straight into a file name that is joined onto a
directory, and the file is
+ // created before any commit sees it - so a separator or a `..` here would
put data outside the
+ // table with nothing left to stop it.
+ for (const char* prefix :
+ {"nested/", "../outside-", "nested/../../outside-", "a\\b", "..",
"."}) {
+ Result<FormatFileNaming> naming = FormatFileNaming::Create("parquet",
prefix);
+ ASSERT_FALSE(naming.ok()) << prefix;
Review Comment:
ASSERT_NOK
##########
src/paimon/core/schema/schema_validation.cpp:
##########
@@ -134,27 +143,132 @@ 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()));
+Status SchemaValidation::ValidateKeyFieldNames(const std::vector<std::string>&
field_names,
+ const std::vector<std::string>&
bucket_keys,
+ const std::vector<std::string>&
primary_keys,
+ const std::vector<std::string>&
partition_keys) {
+ 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));
+ }
+ }
+ 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));
+ return ValidateFormatTableSchema(schema);
+ }
+ return ValidateTableSchema(schema);
+}
+
+Status SchemaValidation::ValidateGenericTableSchema(const TableSchema& schema)
{
+ PAIMON_RETURN_NOT_OK(ValidateKeyFieldNames(schema.FieldNames(),
schema.BucketKeys(),
+ schema.PrimaryKeys(),
schema.PartitionKeys()));
PAIMON_RETURN_NOT_OK(
ValidateOnlyContainPrimitiveType(schema.Fields(),
schema.PrimaryKeys(), "primary key"));
PAIMON_RETURN_NOT_OK(
ValidateOnlyContainPrimitiveType(schema.Fields(),
schema.PartitionKeys(), "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()));
+ return Status::OK();
+}
+
+Status SchemaValidation::ValidateGenericDataSchema(const DataSchema& schema) {
+ PAIMON_RETURN_NOT_OK(ValidateKeyFieldNames(schema.FieldNames(),
schema.BucketKeys(),
+ schema.PrimaryKeys(),
schema.PartitionKeys()));
+ // A `DataSchema` carries its field types as an arrow schema, so the type
rules read them from
+ // there. Only the partition and primary keys are looked up, and both were
just checked.
+ PAIMON_ASSIGN_OR_RAISE(std::unique_ptr<::ArrowSchema> c_schema,
schema.GetArrowSchema());
+ ScopeGuard schema_guard([&c_schema]() {
ArrowSchemaRelease(c_schema.get()); });
+ PAIMON_ASSIGN_OR_RAISE_FROM_ARROW(std::shared_ptr<arrow::Schema>
arrow_schema,
+ arrow::ImportSchema(c_schema.get()));
+ PAIMON_RETURN_NOT_OK(
+ ValidateOnlyContainPrimitiveType(*arrow_schema, schema.PrimaryKeys(),
"primary key"));
+ PAIMON_RETURN_NOT_OK(
+ ValidateOnlyContainPrimitiveType(*arrow_schema,
schema.PartitionKeys(), "partition"));
+ PAIMON_RETURN_NOT_OK(ValidateNotContainSpecificType(*arrow_schema,
schema.PartitionKeys()));
+ return Status::OK();
+}
+
+Status SchemaValidation::ValidateFormatTableSchema(const DataSchema& schema) {
+ // Runs both when the table is created and when it is opened: creation
alone would let a
+ // schema written elsewhere through, and opening alone would persist a
table nothing can load.
+ if (!schema.PrimaryKeys().empty()) {
+ return Status::Invalid(
+ "Cannot define primary keys for a format table: a directory of
data files records no "
+ "row identity to merge on.");
+ }
+
+ const std::map<std::string, std::string>& options = schema.Options();
+ PAIMON_ASSIGN_OR_RAISE(std::string file_format,
OptionsUtils::GetValueFromMap<std::string>(
+ options,
Options::FILE_FORMAT, "parquet"));
+ // The parsed value is not kept; this only has to fail for a format
nothing here can read.
+ PAIMON_RETURN_NOT_OK(FormatTable::ParseFormat(file_format).status());
Review Comment:
`PAIMON_RETURN_NOT_OK(FormatTable::ParseFormat(file_format).status())` to
`PAIMON_RETURN_NOT_OK(FormatTable::ParseFormat(file_format))`
--
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]
