Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions cpp/src/parquet/arrow/arrow_schema_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "parquet/arrow/reader.h"
#include "parquet/arrow/reader_internal.h"
#include "parquet/arrow/schema.h"
#include "parquet/column_reader.h"
#include "parquet/file_reader.h"
#include "parquet/schema.h"
#include "parquet/schema_internal.h"
Expand Down Expand Up @@ -2171,6 +2172,31 @@ TEST(TestFromParquetSchema, UndefinedLogicalType) {
*::arrow::field("column with unknown type", ::arrow::binary()));
}

TEST(TestFromParquetSchema, IncompatibleLogicalTypeDropped) {
// A file with INT32 annotated as UUID. The reader should succeed and ignore the logical
// type and stats.
auto path = test::get_data_file("int32_with_uuid_logical_type.parquet");
std::unique_ptr<parquet::ParquetFileReader> reader =
parquet::ParquetFileReader::OpenFile(path);

const auto* pq_schema = reader->metadata()->schema();
ASSERT_EQ(pq_schema->num_columns(), 1);

const auto* col_desc = pq_schema->Column(0);
ASSERT_EQ(col_desc->physical_type(), parquet::Type::INT32);
ASSERT_FALSE(col_desc->logical_type()->is_valid());
ASSERT_FALSE(col_desc->can_use_min_max());

auto row_group = reader->RowGroup(0);
auto col_reader = std::static_pointer_cast<parquet::Int32Reader>(row_group->Column(0));
const auto num_rows = 10;
std::vector<int32_t> values(num_rows);
int64_t values_read = 0;
col_reader->ReadBatch(num_rows, nullptr, nullptr, values.data(), &values_read);
ASSERT_EQ(values_read, num_rows);
for (int32_t i = 0; i < num_rows; ++i) ASSERT_EQ(values[i], i);
}

//
// Test LevelInfo computation from a Parquet schema
// (for Parquet -> Arrow reading).
Expand Down
19 changes: 15 additions & 4 deletions cpp/src/parquet/schema.cc
Original file line number Diff line number Diff line change
Expand Up @@ -453,10 +453,21 @@ 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_applicable(physical_type, element->type_length)) {
ARROW_LOG(WARNING) << "Dropping unsupported logical type "
<< logical_type->ToString() << " on physical type "
<< TypeToString(physical_type) << " for column '"
<< element->name << "'";
logical_type = UndefinedLogicalType::Make();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The java implementation seems to log in this branch any reason you aren't doing the same? it is a larger change but at some point we might want to make this configurable?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
primitive_node = std::unique_ptr<PrimitiveNode>(new PrimitiveNode(
element->name, LoadEnumSafe(&element->repetition_type), std::move(logical_type),
physical_type, element->type_length, field_id));
} else if (element->__isset.converted_type) {
// legacy writer with converted type present
primitive_node = std::unique_ptr<PrimitiveNode>(new PrimitiveNode(
Expand Down
2 changes: 1 addition & 1 deletion cpp/submodules/parquet-testing

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in this file are due to pinning to a parquet-testing commit that contains the new file from apache/parquet-testing#122. The only file we actually use from this set is data/int32_with_uuid_logical_type.parquet.

Submodule parquet-testing updated 44 files
+ bad_data/ARROW-GH-47662.parquet
+30 −0 bad_data/README.md
+ bad_data/variants/duplicate_field_offsets.parquet
+ bad_data/variants/field_id_out_of_range.parquet
+ bad_data/variants/int_overflow_in_bounds_check.parquet
+ bad_data/variants/malformed_child_inside_well_formed_parent.parquet
+ bad_data/variants/negative_dictionary_size.parquet
+ bad_data/variants/out_of_range_child_offset.parquet
+ bad_data/variants/out_of_range_dictionary_size.parquet
+ bad_data/variants/out_of_range_element_count.parquet
+ bad_data/variants/over_deep_nested_children.parquet
+ bad_data/variants/oversized_primitive_size.parquet
+ bad_data/variants/short_string_length_exceeds_buffer.parquet
+ bad_data/variants/truncated_primitive_size.parquet
+ bad_data/variants/unknown_primitive_type.parquet
+ bad_data/variants/variant_version_2_header.parquet
+121 −1 data/README.md
+ data/aes256/encrypt_columns_and_footer.parquet.encrypted
+ data/aes256/encrypt_columns_and_footer_ctr.parquet.encrypted
+ data/aes256/encrypt_columns_and_footer_disable_aad_storage.parquet.encrypted
+ data/aes256/encrypt_columns_plaintext_footer.parquet.encrypted
+ data/aes256/uniform_encryption.parquet.encrypted
+ data/bson.parquet
+14 −14 data/fixed_length_byte_array.md
+ data/fixed_length_byte_array.parquet
+ data/floating_orders_nan_count.parquet
+24 −0 data/geospatial/README.md
+ data/geospatial/geography-lines.parquet
+ data/geospatial/geography-points.parquet
+ data/geospatial/geography-polygons.parquet
+185 −0 data/geospatial/geospatial-gen-geography.py
+ data/int32_with_uuid_logical_type.parquet
+77 −0 data/int96_timestamp_order.md
+ data/int96_timestamp_order.parquet
+ data/json.parquet
+ shredded_variant/case-041-INVALID.parquet
+ shredded_variant/case-041-INVALID_row-0.variant.bin
+ shredded_variant/case-131-INVALID.parquet
+ shredded_variant/case-131-INVALID_row-0.variant.bin
+ shredded_variant/case-132-INVALID.parquet
+ shredded_variant/case-132-INVALID_row-0.variant.bin
+ shredded_variant/case-138-INVALID.parquet
+ shredded_variant/case-138-INVALID_row-0.variant.bin
+12 −8 shredded_variant/cases.json