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

[HuaHuaY]

Rationale for this change

To prevent negative null_count and distinct_count values ​​from leading to incorrect statistics-based pruning, we had better to check for negative values.

What changes are included in this PR?

Modify ColumnIndex::Makeat cpp/src/parquet/page_index.cc and MakeTypedColumnStats at cpp/src/parquet/metadata.cc. When reading null_count and distinct_count from a file, we no longer silently assume that the values ​​are natural numbers.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

• GitHub Issue: [C++][Parquet] Be better to check null_count, distinct_count and nan_count are negative when reading files #50692

[github-actions]

⚠️ GitHub issue #50692 has been automatically assigned in GitHub to PR creator.

[wgtmac]

Why can't we directly use metadata.statistics?

[HuaHuaY]

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

[wgtmac]

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]

FYI:

• A brief discussion about option 1 and 2 Implement PARQUET-2249: Introduce IEEE 754 total order arrow-rs#9619 (comment)
• For option 4, we may need to fix ParquetFileFragment::EvaluateStatisticsAsExpression at cpp/src/arrow/dataset/file_parquet.cc. It uses bool may_have_null = !statistics.HasNullCount() || statistics.null_count() > 0; which regards negative count as zero.

[etseidl]

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.

[pitrou]

Why do we care about this? The null_count could be incorrect in other ways (for example larger than num_values or inconsistent with the actual def levels). Writing a negative value is quite unlikely compared to the subtler ways of writing an incorrect value.

The bottom line is that an incorrect statistic should not lead to a crash or UB, but it could lead to incorrect results when reading. There's nothing that we can reasonably do against that (the whole point of a statistic is to avoid reading all values, but verifying that a statistic is correct would require reading all values, defeating the point of a statistic).

[HuaHuaY]

Why do we care about this?

When I implement nan_count, I had to decide how to handle negative numbers. I checked the arrow-rs implementation and saw that they include error handling for negative numbers, which prompted this PR. I just wanted to clarify the expected behavior for negative numbers; I'm also fine with doing nothing about them.