GH-46205: [C++][Parquet][WIP] Read/Write null count statistics for UNKNOWN sort order #46275

paleolimbot · 2025-04-30T20:09:54Z

Rationale for this change

Minimum and maximum values are not useful in the context of an unsorted converted or loigcal type; however, null counts are! Geometry is one example of an unsorted type where this type of information might be useful; however, there are others as well!

What changes are included in this PR?

Early work-in-progress explorations to find the relevant pieces of code.

Are these changes tested?

They will be once a general direction is decided on!

Are there any user-facing changes?

Possibly!

GitHub Issue: [Parquet][C++] Logical types with sort order UNKNOWN are missing null_count statistics #46205

github-actions · 2025-04-30T20:10:20Z

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

paleolimbot · 2025-04-30T20:12:33Z

cpp/src/parquet/column_writer.cc

+        page_statistics_ = MakeStatistics<ParquetType>(descr_, allocator_);
+        chunk_statistics_ = MakeStatistics<ParquetType>(descr_, allocator_);


The actual version of this should maybe modify the if (SortOrder::UNKNOWN != descr_->sort_order()) { check just above. Perhaps there needs to be a descr_->can_write_statistics() to separate the sortedness from whether or not we can write anything?

paleolimbot · 2025-04-30T20:14:29Z

cpp/src/parquet/metadata.cc

+    bool is_geometry =
+        descr_->logical_type() != nullptr && descr_->logical_type()->is_geometry();
    if (!column_metadata_->__isset.statistics ||
-        descr_->sort_order() == SortOrder::UNKNOWN) {
+        (descr_->sort_order() == SortOrder::UNKNOWN && !is_geometry)) {


Again, maybe we need a descr_->can_read_statistics? There's a HasCorrectStatistics(), too, and maybe the check needs to be there. I wonder whether the types currently marked as unsorted had null counts written reliably by other implementations or whether we have to ignore those?

According to the spec, only INT96 primitive type and INTERVAL logical type have undefined column order (except complex types like map, list, variant, and geo types).

INT96: use BINARY_AS_SIGNED_INTEGER_COMPARATOR: https://github.com/apache/parquet-java/blob/5f079b98e63c814535e8709ab5c6fb672c2aedc5/parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveType.java#L366

INTERVAL: use UNSIGNED_LEXICOGRAPHICAL_BINARY_COMPARATOR :https://github.com/apache/parquet-java/blob/5f079b98e63c814535e8709ab5c6fb672c2aedc5/parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveType.java#L406

However, I think parquet-java does not cleanly implement the column order semantics.

paleolimbot · 2025-04-30T20:15:51Z

cpp/src/parquet/statistics.cc

+template <typename DType>
+class UnsortedTypedStatisticsImpl : public TypedStatistics<DType> {


I'm not sure this is the answer here...I just needed a TypedStatistics<> to make this work in the ColumnWriter

Perhaps we can reuse the existing typed one and just ignore the stats on write? (Seems inefficient but may be more compact?)

Yes, the Java impl did this because the spec advises that min/max values are undefined and should not be used in this case. If we go with this approach, perhaps we need to disable page index (at least the column index) to reduce file size.

wgtmac · 2025-05-12T05:13:39Z

cpp/src/parquet/statistics.cc

+
+  int64_t null_count() const override { return null_count_; }
+
+  int64_t distinct_count() const override { return num_values_; }


Should we throw or return -1?

wgtmac · 2025-05-12T05:16:05Z

cpp/src/parquet/statistics.cc

+template <typename DType>
+class UnsortedTypedStatisticsImpl : public TypedStatistics<DType> {


Yes, the Java impl did this because the spec advises that min/max values are undefined and should not be used in this case. If we go with this approach, perhaps we need to disable page index (at least the column index) to reduce file size.

wgtmac · 2025-05-12T05:33:57Z

cpp/src/parquet/metadata.cc

+    bool is_geometry =
+        descr_->logical_type() != nullptr && descr_->logical_type()->is_geometry();
    if (!column_metadata_->__isset.statistics ||
-        descr_->sort_order() == SortOrder::UNKNOWN) {
+        (descr_->sort_order() == SortOrder::UNKNOWN && !is_geometry)) {


According to the spec, only INT96 primitive type and INTERVAL logical type have undefined column order (except complex types like map, list, variant, and geo types).

INT96: use BINARY_AS_SIGNED_INTEGER_COMPARATOR: https://github.com/apache/parquet-java/blob/5f079b98e63c814535e8709ab5c6fb672c2aedc5/parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveType.java#L366

INTERVAL: use UNSIGNED_LEXICOGRAPHICAL_BINARY_COMPARATOR :https://github.com/apache/parquet-java/blob/5f079b98e63c814535e8709ab5c6fb672c2aedc5/parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveType.java#L406

However, I think parquet-java does not cleanly implement the column order semantics.

wgtmac · 2025-05-12T05:37:36Z

cpp/src/parquet/metadata.cc

-  // Unknown sort order has incorrect stats
-  if (SortOrder::UNKNOWN == sort_order) {
+  // Unknown sort order has incorrect stats if the min or the max are specified
+  if (SortOrder::UNKNOWN == sort_order && (statistics.has_min || statistics.has_max)) {


Should we just check geometry & geography types here?

mapleFU

UnsortedTypedStatisticsImpl general LGTM. But can we use same way to create UnsortedTypedStatisticsImpl? I found some code checks logical_type, other case just ignore and checks min-max

Besides, I think for other types without min-max (maybe like json or other logical types?) This is also usable?

mapleFU · 2025-05-14T09:08:34Z

cpp/src/parquet/statistics.cc

+
+  EncodedStatistics Encode() override {
+    EncodedStatistics out;
+    out.set_null_count(null_count_);


Do we need all_null_value here?

paleolimbot · 2025-05-14T14:04:53Z

Thanks for these! I'm mostly away from keyboard this week but will circle back to clean this up on Monday.

paleolimbot added 2 commits April 30, 2025 11:26

start on stats for unsorted things

229ec5e

hack to make it work

5f4080c

github-actions bot added Component: Parquet Component: C++ awaiting committer review Awaiting committer review labels Apr 30, 2025

paleolimbot commented Apr 30, 2025

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 30, 2025

paleolimbot commented Apr 30, 2025

View reviewed changes

paleolimbot mentioned this pull request Apr 30, 2025

[Parquet][C++] Logical types with sort order UNKNOWN are missing null_count statistics #46205

Open

wgtmac reviewed May 12, 2025

View reviewed changes

mapleFU reviewed May 14, 2025

View reviewed changes

paleolimbot mentioned this pull request Jun 2, 2025

GH-46270: [C++][Parquet] Clarify GeoStatistics docstring #46649

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

GH-46205: [C++][Parquet][WIP] Read/Write null count statistics for UNKNOWN sort order #46275

GH-46205: [C++][Parquet][WIP] Read/Write null count statistics for UNKNOWN sort order #46275

Uh oh!

paleolimbot commented Apr 30, 2025 •

edited by github-actions bot

Loading

Uh oh!

github-actions bot commented Apr 30, 2025

Uh oh!

paleolimbot Apr 30, 2025

Uh oh!

paleolimbot Apr 30, 2025

Uh oh!

wgtmac May 12, 2025

Uh oh!

paleolimbot Apr 30, 2025

Uh oh!

paleolimbot Apr 30, 2025

Uh oh!

wgtmac May 12, 2025

Uh oh!

wgtmac May 12, 2025

Uh oh!

wgtmac May 12, 2025

Uh oh!

wgtmac May 12, 2025

Uh oh!

wgtmac May 12, 2025

Uh oh!

mapleFU left a comment

Uh oh!

mapleFU May 14, 2025

Uh oh!

paleolimbot commented May 14, 2025

Uh oh!

Uh oh!

		page_statistics_ = MakeStatistics<ParquetType>(descr_, allocator_);
		chunk_statistics_ = MakeStatistics<ParquetType>(descr_, allocator_);

		template <typename DType>
		class UnsortedTypedStatisticsImpl : public TypedStatistics<DType> {


		int64_t null_count() const override { return null_count_; }

		int64_t distinct_count() const override { return num_values_; }

GH-46205: [C++][Parquet][WIP] Read/Write null count statistics for UNKNOWN sort order #46275

Are you sure you want to change the base?

GH-46205: [C++][Parquet][WIP] Read/Write null count statistics for UNKNOWN sort order #46275

Uh oh!

Conversation

paleolimbot commented Apr 30, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Apr 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mapleFU left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paleolimbot commented May 14, 2025

Uh oh!

Uh oh!

paleolimbot commented Apr 30, 2025 •

edited by github-actions bot

Loading