Copilot commented on code in PR #50892:
URL: https://github.com/apache/arrow/pull/50892#discussion_r4005713062
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -1161,28 +1219,42 @@ Result<bool> ApplyOriginalMetadata(const Field&
origin_field, SchemaField* infer
if (origin_type->id() == ::arrow::Type::EXTENSION) {
const auto& origin_extension_type =
checked_cast<const ::arrow::ExtensionType&>(*origin_type);
+ std::string origin_extension_name = origin_extension_type.extension_name();
+
+ // Whether or not the inferred type is also an extension type. This can
occur when
+ // arrow_extensions_enabled is true in the ArrowReaderProperties.
Extension types
+ // are not currently inferred for any other reason.
+ bool arrow_extension_inferred =
+ inferred->field->type()->id() == ::arrow::Type::EXTENSION;
+
+ bool restore_file_extension = false;
+ if (origin_extension_name == kFileExtensionName &&
arrow_extension_inferred) {
+ const auto& inferred_extension_type =
+ checked_cast<const
::arrow::ExtensionType&>(*inferred->field->type());
+ if (FileStorageTypesCompatible(origin_extension_type.storage_type(),
+ inferred_extension_type.storage_type())) {
Review Comment:
FILE children are only compared by name when `arrow_extension_inferred` is
true. If a File extension is registered but `arrow_extensions_enabled` is
false, `GroupToStruct` produces a plain struct (the test above expects metadata
to restore the extension in this mode), so a valid FILE schema whose children
are reordered is compared positionally by the later
`storage_type()->Equals(...)` check. That loses the extension and can apply
child metadata to the wrong fields. Compare the origin File storage with the
inferred struct by name even when inference did not create an extension, and
use that result for the recursive restoration.
##########
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:
`FileStorageTypesCompatible` assumes `inferred_type` is a `StructType` and
uses `checked_cast` without checking its type id. The inferred storage type
comes from the registered extension, so a mismatched registration/name
collision can provide a non-struct; in release builds this cast is unchecked
and causes undefined behavior before the FILE extension is restored. Guard both
inputs with `id() == STRUCT` and return `false` before the cast.
--
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]
