diff --git a/cpp/src/parquet/metadata.cc b/cpp/src/parquet/metadata.cc index 98f60df63dd4..ace49731fc62 100644 --- a/cpp/src/parquet/metadata.cc +++ b/cpp/src/parquet/metadata.cc @@ -94,9 +94,11 @@ namespace { template std::shared_ptr MakeTypedColumnStats(const format::ColumnMetaData& metadata, + const EncodedStatistics& encoded_stats, const ColumnDescriptor* descr, ::arrow::MemoryPool* pool) { const auto& statistics = metadata.statistics; + const int64_t null_count = encoded_stats.has_null_count ? encoded_stats.null_count : 0; const std::string kEmpty = ""; const std::string* encoded_min = &kEmpty; const std::string* encoded_max = &kEmpty; @@ -126,10 +128,9 @@ std::shared_ptr MakeTypedColumnStats(const format::ColumnMetaData& m } return MakeStatistics( - descr, *encoded_min, *encoded_max, metadata.num_values - statistics.null_count, - statistics.null_count, statistics.distinct_count, has_min_max, - statistics.__isset.null_count, statistics.__isset.distinct_count, min_exact, - max_exact, pool); + descr, *encoded_min, *encoded_max, metadata.num_values - null_count, null_count, + encoded_stats.distinct_count, has_min_max, encoded_stats.has_null_count, + encoded_stats.has_distinct_count, min_exact, max_exact, pool); } std::shared_ptr MakeColumnGeometryStats( @@ -144,6 +145,7 @@ std::shared_ptr MakeColumnGeometryStats( } std::shared_ptr MakeColumnStats(const format::ColumnMetaData& meta_data, + const EncodedStatistics& encoded_stats, const ColumnDescriptor* descr, ::arrow::MemoryPool* pool) { auto metadata_type = LoadEnumSafe(&meta_data.type); @@ -154,21 +156,21 @@ std::shared_ptr MakeColumnStats(const format::ColumnMetaData& meta_d } switch (metadata_type) { case Type::BOOLEAN: - return MakeTypedColumnStats(meta_data, descr, pool); + return MakeTypedColumnStats(meta_data, encoded_stats, descr, pool); case Type::INT32: - return MakeTypedColumnStats(meta_data, descr, pool); + return MakeTypedColumnStats(meta_data, encoded_stats, descr, pool); case Type::INT64: - return MakeTypedColumnStats(meta_data, descr, pool); + return MakeTypedColumnStats(meta_data, encoded_stats, descr, pool); case Type::INT96: - return MakeTypedColumnStats(meta_data, descr, pool); + return MakeTypedColumnStats(meta_data, encoded_stats, descr, pool); case Type::DOUBLE: - return MakeTypedColumnStats(meta_data, descr, pool); + return MakeTypedColumnStats(meta_data, encoded_stats, descr, pool); case Type::FLOAT: - return MakeTypedColumnStats(meta_data, descr, pool); + return MakeTypedColumnStats(meta_data, encoded_stats, descr, pool); case Type::BYTE_ARRAY: - return MakeTypedColumnStats(meta_data, descr, pool); + return MakeTypedColumnStats(meta_data, encoded_stats, descr, pool); case Type::FIXED_LEN_BYTE_ARRAY: - return MakeTypedColumnStats(meta_data, descr, pool); + return MakeTypedColumnStats(meta_data, encoded_stats, descr, pool); case Type::UNDEFINED: break; } @@ -370,8 +372,8 @@ class ColumnChunkMetaData::ColumnChunkMetaDataImpl { if (is_stats_set()) { const std::lock_guard guard(stats_mutex_); if (possible_stats_ == nullptr) { - possible_stats_ = - MakeColumnStats(*column_metadata_, descr_, properties_.memory_pool()); + possible_stats_ = MakeColumnStats(*column_metadata_, *possible_encoded_stats_, + descr_, properties_.memory_pool()); } return possible_stats_; } diff --git a/cpp/src/parquet/metadata_test.cc b/cpp/src/parquet/metadata_test.cc index ac45be1fac38..bbd8335b7374 100644 --- a/cpp/src/parquet/metadata_test.cc +++ b/cpp/src/parquet/metadata_test.cc @@ -361,6 +361,26 @@ TEST(Metadata, UnknownColumnOrderIgnoresMinMax) { AssertColumnChunkHasNoMinMax(*metadata, ColumnOrder::UNKNOWN); } +TEST(Metadata, NegativeCounts) { + format::FileMetaData thrift_metadata = SingleInt32MetadataWithStats(); + auto& column_metadata = thrift_metadata.row_groups[0].columns[0].meta_data; + column_metadata.__set_num_values(10); + column_metadata.statistics.__set_null_count(-1); + column_metadata.statistics.__set_distinct_count(-1); + + auto metadata = ParseMetadata(SerializeMetadata(thrift_metadata)); + auto column = GetOnlyColumnChunk(*metadata, ColumnOrder::TYPE_DEFINED_ORDER); + auto encoded_statistics = column->encoded_statistics(); + ASSERT_NE(nullptr, encoded_statistics); + EXPECT_FALSE(encoded_statistics->has_null_count); + EXPECT_FALSE(encoded_statistics->has_distinct_count); + auto statistics = column->statistics(); + ASSERT_NE(nullptr, statistics); + EXPECT_FALSE(statistics->HasNullCount()); + EXPECT_FALSE(statistics->HasDistinctCount()); + EXPECT_EQ(10, statistics->num_values()); +} + TEST(Metadata, MissingColumnOrderUsesLegacyMinMax) { format::FileMetaData thrift_metadata = SingleInt32MetadataWithStats(); thrift_metadata.column_orders.clear(); diff --git a/cpp/src/parquet/page_index.cc b/cpp/src/parquet/page_index.cc index 1d2faebd251c..540cd7b860f2 100644 --- a/cpp/src/parquet/page_index.cc +++ b/cpp/src/parquet/page_index.cc @@ -15,6 +15,7 @@ // specific language governing permissions and limitations // under the License. +#include #include #include @@ -978,6 +979,13 @@ std::unique_ptr ColumnIndex::Make(const ColumnDescriptor& descr, // Guard against UB when moving column_index throw ParquetException("Invalid ColumnIndex boundary_order"); } + if (column_index.__isset.null_counts && + column_index.null_counts.size() == column_index.null_pages.size() && + std::ranges::any_of(column_index.null_counts, + [](int64_t null_count) { return null_count < 0; })) { + column_index.__isset.null_counts = false; + column_index.null_counts.clear(); + } switch (descr.physical_type()) { case Type::BOOLEAN: return std::make_unique>(descr, diff --git a/cpp/src/parquet/page_index_test.cc b/cpp/src/parquet/page_index_test.cc index 3a7308c1c6bc..531b16a5134b 100644 --- a/cpp/src/parquet/page_index_test.cc +++ b/cpp/src/parquet/page_index_test.cc @@ -259,6 +259,26 @@ TEST(PageIndex, ReadColumnIndexWithNullPage) { null_pages, min_values, max_values, has_null_counts, null_counts); } +TEST(PageIndex, ReadColumnIndexWithNegativeNullCount) { + format::ColumnIndex thrift_index; + thrift_index.__set_null_pages({true, true}); + thrift_index.__set_min_values({"", ""}); + thrift_index.__set_max_values({"", ""}); + thrift_index.__set_boundary_order(format::BoundaryOrder::UNORDERED); + thrift_index.__set_null_counts({1, -1}); + + auto sink = CreateOutputStream(); + ThriftSerializer{}.Serialize(&thrift_index, sink.get()); + PARQUET_ASSIGN_OR_THROW(auto buffer, sink->Finish()); + ColumnDescriptor descr(schema::Int32("c1"), /*max_definition_level=*/1, + /*max_repetition_level=*/0); + auto column_index = + ColumnIndex::Make(descr, buffer->data(), static_cast(buffer->size()), + default_reader_properties()); + + EXPECT_FALSE(column_index->has_null_counts()); +} + struct PageIndexRanges { int64_t column_index_offset; int64_t column_index_length; diff --git a/cpp/src/parquet/thrift_internal.h b/cpp/src/parquet/thrift_internal.h index 971e6ccebc9d..fd9f1cf6fd38 100644 --- a/cpp/src/parquet/thrift_internal.h +++ b/cpp/src/parquet/thrift_internal.h @@ -306,10 +306,10 @@ static inline EncodedStatistics FromThrift(const format::Statistics& stats, out.set_min(stats.min); } } - if (stats.__isset.null_count) { + if (stats.__isset.null_count && stats.null_count >= 0) { out.set_null_count(stats.null_count); } - if (stats.__isset.distinct_count) { + if (stats.__isset.distinct_count && stats.distinct_count >= 0) { out.set_distinct_count(stats.distinct_count); }