apilloud commented on code in PR #50892:
URL: https://github.com/apache/arrow/pull/50892#discussion_r4499084981
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -1150,6 +1193,21 @@ Result<bool> ApplyOriginalStorageMetadata(const Field&
origin_field,
return modified;
}
+bool FileStorageTypesCompatible(const std::shared_ptr<::arrow::DataType>&
origin_type,
+ const std::shared_ptr<::arrow::DataType>&
inferred_type) {
+ if (origin_type->num_fields() != inferred_type->num_fields()) {
Review Comment:
Are you sure you want to be this strict? It might be useful to be able to
write a reader with all 6 fields and have that work regardless of what
(optional) fields are in the file. So possibly use something like
`origin_type->num_fields() <= inferred_type->num_fields()`?
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -6079,5 +6081,84 @@ TEST(TestArrowReadWrite, AllNulls) {
ASSERT_TRUE(expected_table->Equals(*read_table));
}
+struct FileRoundtripTestCase {
+ std::shared_ptr<::arrow::DataType> storage_type;
+
+ friend std::ostream& operator<<(std::ostream& os,
+ const FileRoundtripTestCase& test_case) {
+ return os << "{storage_type=" << test_case.storage_type->ToString() << "}";
+ }
+};
+
+class TestArrowReadWriteFileType
+ : public ::testing::TestWithParam<FileRoundtripTestCase> {
+ protected:
+ ::arrow::Result<std::unique_ptr<FileReader>> OpenReader(
+ const std::shared_ptr<::arrow::Buffer>& buffer,
+ const std::shared_ptr<::arrow::DataType>& file_type) {
+ extension_guard_.emplace(::arrow::DataTypeVector{file_type});
+ ArrowReaderProperties reader_properties;
+ reader_properties.set_arrow_extensions_enabled(true);
+ std::unique_ptr<FileReader> reader;
+ FileReaderBuilder builder;
+ RETURN_NOT_OK(builder.Open(std::make_shared<BufferReader>(buffer)));
+ RETURN_NOT_OK(builder.properties(reader_properties)->Build(&reader));
+ return reader;
+ }
+
+ private:
+ std::optional<::arrow::ExtensionTypeGuard> extension_guard_;
+};
+
+TEST_P(TestArrowReadWriteFileType, FileExtensionRoundtrip) {
Review Comment:
Maybe add a test case where the order is different? `(size, offset, uri)`
##########
cpp/src/arrow/extension/parquet_file_test.cc:
##########
Review Comment:
Maybe add some tests validating FileExtensionType::Make in the happy path?
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -1150,6 +1193,21 @@ Result<bool> ApplyOriginalStorageMetadata(const Field&
origin_field,
return modified;
}
+bool FileStorageTypesCompatible(const std::shared_ptr<::arrow::DataType>&
origin_type,
+ const std::shared_ptr<::arrow::DataType>&
inferred_type) {
+ if (origin_type->num_fields() != inferred_type->num_fields()) {
+ return false;
+ }
+ const auto& inferred_struct_type =
+ checked_cast<const ::arrow::StructType&>(*inferred_type);
+ for (const auto& origin_field : origin_type->fields()) {
+ if (inferred_struct_type.GetFieldByName(origin_field->name()) == nullptr) {
Review Comment:
Should type match be validated too?
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -1150,6 +1193,21 @@ Result<bool> ApplyOriginalStorageMetadata(const Field&
origin_field,
return modified;
}
+bool FileStorageTypesCompatible(const std::shared_ptr<::arrow::DataType>&
origin_type,
+ const std::shared_ptr<::arrow::DataType>&
inferred_type) {
+ if (origin_type->num_fields() != inferred_type->num_fields()) {
+ return false;
+ }
+ const auto& inferred_struct_type =
+ checked_cast<const ::arrow::StructType&>(*inferred_type);
Review Comment:
This seems to still exists? Should there be a struct type check at the
begining of this function like in IsSupportedStorageType?
```
if (!origin_type || !inferred_type ||
origin_type->id() != ::arrow::Type::STRUCT ||
inferred_type->id() != ::arrow::Type::STRUCT) {
return false;
}
```
--
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]
