divjotarora commented on code in PR #50909:
URL: https://github.com/apache/arrow/pull/50909#discussion_r3823195466
##########
cpp/src/parquet/schema.cc:
##########
@@ -453,10 +453,17 @@ std::unique_ptr<Node> PrimitiveNode::FromParquet(const
void* opaque_element) {
std::unique_ptr<PrimitiveNode> primitive_node;
if (element->__isset.logicalType) {
// updated writer with logical type present
- primitive_node = std::unique_ptr<PrimitiveNode>(
- new PrimitiveNode(element->name,
LoadEnumSafe(&element->repetition_type),
- LogicalType::FromThrift(element->logicalType),
- LoadEnumSafe(&element->type), element->type_length,
field_id));
+ auto physical_type = LoadEnumSafe(&element->type);
+ auto logical_type = LogicalType::FromThrift(element->logicalType);
+ // Tolerate unrecognized logical/physical type combinations by dropping
the logical type
+ // annotation.
+ if (logical_type && !logical_type->is_nested() &&
Review Comment:
Dropped. I previously thought this was needed for correctness but I
cross-checked the Java implementation and doesn't seem it is
##########
cpp/src/parquet/schema.cc:
##########
@@ -453,10 +453,17 @@ std::unique_ptr<Node> PrimitiveNode::FromParquet(const
void* opaque_element) {
std::unique_ptr<PrimitiveNode> primitive_node;
if (element->__isset.logicalType) {
// updated writer with logical type present
- primitive_node = std::unique_ptr<PrimitiveNode>(
- new PrimitiveNode(element->name,
LoadEnumSafe(&element->repetition_type),
- LogicalType::FromThrift(element->logicalType),
- LoadEnumSafe(&element->type), element->type_length,
field_id));
+ auto physical_type = LoadEnumSafe(&element->type);
+ auto logical_type = LogicalType::FromThrift(element->logicalType);
+ // Tolerate unrecognized logical/physical type combinations by dropping
the logical type
+ // annotation.
+ if (logical_type && !logical_type->is_nested() &&
+ !logical_type->is_applicable(physical_type, element->type_length)) {
+ logical_type = UndefinedLogicalType::Make();
Review Comment:
Added a log here as well.
> it is a larger change but at some point we might want to make this
configurable?
I'm not sure we need this to be configurable. It's not in parquet-java and
likely would not be in arrow-rs based on previous changes made there. Is it
standard to add such flags in this implementation?
--
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]
