Skip to content

GH-50692: [C++][Parquet] treat negative null_count and distinct_count values as missing #50694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
HuaHuaY wants to merge 1 commit into from
Closed

GH-50692: [C++][Parquet] treat negative null_count and distinct_count values as missing #50694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 16 additions & 14 deletions cpp/src/parquet/metadata.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,11 @@ namespace {

template <typename DType>
std::shared_ptr<Statistics> 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;

@wgtmac wgtmac Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why can't we directly use metadata.statistics?

@HuaHuaY HuaHuaY Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because the values which metadata.statistics contains are the negative values. I want to use the values ​​modified in FromThrift.

const std::string kEmpty = "";
const std::string* encoded_min = &kEmpty;
const std::string* encoded_max = &kEmpty;
Expand Down Expand Up @@ -126,10 +128,9 @@ std::shared_ptr<Statistics> MakeTypedColumnStats(const format::ColumnMetaData& m
}

return MakeStatistics<DType>(
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<geospatial::GeoStatistics> MakeColumnGeometryStats(
Expand All @@ -144,6 +145,7 @@ std::shared_ptr<geospatial::GeoStatistics> MakeColumnGeometryStats(
}

std::shared_ptr<Statistics> MakeColumnStats(const format::ColumnMetaData& meta_data,
const EncodedStatistics& encoded_stats,
const ColumnDescriptor* descr,
::arrow::MemoryPool* pool) {
auto metadata_type = LoadEnumSafe(&meta_data.type);
Expand All @@ -154,21 +156,21 @@ std::shared_ptr<Statistics> MakeColumnStats(const format::ColumnMetaData& meta_d
}
switch (metadata_type) {
case Type::BOOLEAN:
return MakeTypedColumnStats<BooleanType>(meta_data, descr, pool);
return MakeTypedColumnStats<BooleanType>(meta_data, encoded_stats, descr, pool);
case Type::INT32:
return MakeTypedColumnStats<Int32Type>(meta_data, descr, pool);
return MakeTypedColumnStats<Int32Type>(meta_data, encoded_stats, descr, pool);
case Type::INT64:
return MakeTypedColumnStats<Int64Type>(meta_data, descr, pool);
return MakeTypedColumnStats<Int64Type>(meta_data, encoded_stats, descr, pool);
case Type::INT96:
return MakeTypedColumnStats<Int96Type>(meta_data, descr, pool);
return MakeTypedColumnStats<Int96Type>(meta_data, encoded_stats, descr, pool);
case Type::DOUBLE:
return MakeTypedColumnStats<DoubleType>(meta_data, descr, pool);
return MakeTypedColumnStats<DoubleType>(meta_data, encoded_stats, descr, pool);
case Type::FLOAT:
return MakeTypedColumnStats<FloatType>(meta_data, descr, pool);
return MakeTypedColumnStats<FloatType>(meta_data, encoded_stats, descr, pool);
case Type::BYTE_ARRAY:
return MakeTypedColumnStats<ByteArrayType>(meta_data, descr, pool);
return MakeTypedColumnStats<ByteArrayType>(meta_data, encoded_stats, descr, pool);
case Type::FIXED_LEN_BYTE_ARRAY:
return MakeTypedColumnStats<FLBAType>(meta_data, descr, pool);
return MakeTypedColumnStats<FLBAType>(meta_data, encoded_stats, descr, pool);
case Type::UNDEFINED:
break;
}
Expand Down Expand Up @@ -370,8 +372,8 @@ class ColumnChunkMetaData::ColumnChunkMetaDataImpl {
if (is_stats_set()) {
const std::lock_guard<std::mutex> 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_;
}
Expand Down
20 changes: 20 additions & 0 deletions cpp/src/parquet/metadata_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
8 changes: 8 additions & 0 deletions cpp/src/parquet/page_index.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// specific language governing permissions and limitations
// under the License.

#include <algorithm>
#include <limits>
#include <numeric>

Expand Down Expand Up @@ -978,6 +979,13 @@ std::unique_ptr<ColumnIndex> 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<TypedColumnIndexImpl<BooleanType>>(descr,
Expand Down
20 changes: 20 additions & 0 deletions cpp/src/parquet/page_index_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint32_t>(buffer->size()),
default_reader_properties());

EXPECT_FALSE(column_index->has_null_counts());
}

struct PageIndexRanges {
int64_t column_index_offset;
int64_t column_index_length;
Expand Down
4 changes: 2 additions & 2 deletions cpp/src/parquet/thrift_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

@wgtmac wgtmac Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This silently changes the public behavior that corrupted values are no longer visible to users from reading a file. I think we have a few options w.r.t. values in the stats and page indexes:

  1. Reject any corrupted values by throwing an exception (@etseidl has proposed this in another thread)
  2. Regard corrupted values as unset (this PR does).
  3. Preserve corrupted values but provide a Validate function or a function to fix the corrupted values.
  4. Do nothing so it is users' responsibility to check corrupted values and take action (ignore them or error out).

My understanding is that users have to validate these values before consumption anyway. In that case, an unset or corrupted value does not make too much difference. Users may still want to read the file even when there are corrupted stats (or even columns/row groups), e.g. recover corrupted/legacy files. So I think we shouldn't error out on invalid stats. Preserving corrupted values still bring some values like help inspecting the files so here might be the best place to fix this issue?

WDYT? @pitrou @mapleFU @emkornfield

@HuaHuaY HuaHuaY Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

FYI:

@etseidl etseidl Jul 29, 2026

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.

I see this was closed, but would just like to say that the situation here is a little different from arrow-rs. It seems arrow-cpp keeps the sign for null_count and nan_count, so returning the actual encoded value is probably the best thing to do. Let users decide what to make of that. arrow-rs, for unknown historical reasons, changes these fields to unsigned, so when a conversion to unsigned is not possible, the IMO most reasonable thing to do is return an error to let the user know something is wrong with the metadata. I don't think silently ignoring the error in that case is an option.

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);
}

Expand Down
Loading