You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2025/04/30 20:09:57 UTC

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

paleolimbot opened a new pull request, #46275:
URL: https://github.com/apache/arrow/pull/46275

   ### 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!
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

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

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #46275:
URL: https://github.com/apache/arrow/pull/46275#issuecomment-2843159685

   :warning: GitHub issue #46205 **has been automatically assigned in GitHub** to PR creator.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

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

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #46275: GH-46205: [C++][Parquet][WIP] Read/Write null count statistics for UNKNOWN sort order
URL: https://github.com/apache/arrow/pull/46275


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

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

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #46275:
URL: https://github.com/apache/arrow/pull/46275#discussion_r2069381360


##########
cpp/src/parquet/statistics.cc:
##########
@@ -963,6 +963,89 @@ std::shared_ptr<Comparator> DoMakeComparator(Type::type physical_type,
   return nullptr;
 }
 
+template <typename DType>
+class UnsortedTypedStatisticsImpl : public TypedStatistics<DType> {

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



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

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

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #46275:
URL: https://github.com/apache/arrow/pull/46275#discussion_r2069376548


##########
cpp/src/parquet/column_writer.cc:
##########
@@ -1250,8 +1250,11 @@ class TypedColumnWriterImpl : public ColumnWriterImpl,
         page_statistics_ = MakeStatistics<ParquetType>(descr_, allocator_);
         chunk_statistics_ = MakeStatistics<ParquetType>(descr_, allocator_);
       }
+
       if (descr_->logical_type() != nullptr && descr_->logical_type()->is_geometry()) {
         chunk_geospatial_statistics_ = std::make_shared<geospatial::GeoStatistics>();
+        page_statistics_ = MakeStatistics<ParquetType>(descr_, allocator_);
+        chunk_statistics_ = MakeStatistics<ParquetType>(descr_, allocator_);

Review Comment:
   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?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

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

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #46275:
URL: https://github.com/apache/arrow/pull/46275#discussion_r2069380251


##########
cpp/src/parquet/statistics.cc:
##########
@@ -963,6 +963,89 @@ std::shared_ptr<Comparator> DoMakeComparator(Type::type physical_type,
   return nullptr;
 }
 
+template <typename DType>
+class UnsortedTypedStatisticsImpl : public TypedStatistics<DType> {

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



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

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

Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #46275:
URL: https://github.com/apache/arrow/pull/46275#discussion_r2083852072


##########
cpp/src/parquet/metadata.cc:
##########
@@ -1552,8 +1554,8 @@ bool ApplicationVersion::HasCorrectStatistics(Type::type col_type,
     return true;
   }
 
-  // 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)) {

Review Comment:
   Should we just check geometry & geography types here?



##########
cpp/src/parquet/statistics.cc:
##########
@@ -963,6 +963,89 @@ std::shared_ptr<Comparator> DoMakeComparator(Type::type physical_type,
   return nullptr;
 }
 
+template <typename DType>
+class UnsortedTypedStatisticsImpl : public TypedStatistics<DType> {
+ public:
+  using T = typename DType::c_type;
+
+  explicit UnsortedTypedStatisticsImpl(const ColumnDescriptor* descr) : descr_(descr) {}
+
+  UnsortedTypedStatisticsImpl(const ColumnDescriptor* descr, int64_t num_values,
+                              int64_t null_count)
+      : descr_(descr), num_values_(num_values), null_count_(null_count) {}
+
+  bool HasDistinctCount() const override { return false; };
+  bool HasMinMax() const override { return false; }
+  bool HasNullCount() const override { return true; };
+
+  int64_t null_count() const override { return null_count_; }
+
+  int64_t distinct_count() const override { return num_values_; }

Review Comment:
   Should we throw or return -1?



##########
cpp/src/parquet/statistics.cc:
##########
@@ -963,6 +963,89 @@ std::shared_ptr<Comparator> DoMakeComparator(Type::type physical_type,
   return nullptr;
 }
 
+template <typename DType>
+class UnsortedTypedStatisticsImpl : public TypedStatistics<DType> {

Review Comment:
   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.



##########
cpp/src/parquet/metadata.cc:
##########
@@ -307,8 +307,10 @@ class ColumnChunkMetaData::ColumnChunkMetaDataImpl {
     DCHECK(writer_version_ != nullptr);
     // If the column statistics don't exist or column sort order is unknown
     // we cannot use the column stats
+    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)) {

Review Comment:
   According to the [spec](https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift#L1045), 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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

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

Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #46275:
URL: https://github.com/apache/arrow/pull/46275#discussion_r2069378772


##########
cpp/src/parquet/metadata.cc:
##########
@@ -307,8 +307,10 @@ class ColumnChunkMetaData::ColumnChunkMetaDataImpl {
     DCHECK(writer_version_ != nullptr);
     // If the column statistics don't exist or column sort order is unknown
     // we cannot use the column stats
+    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)) {

Review Comment:
   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?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

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

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #46275:
URL: https://github.com/apache/arrow/pull/46275#issuecomment-4459485060

   Thank you for your contribution. Unfortunately, this pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label or comment below, or this PR will be closed in 14 days.  Feel free to re-open this if it has been closed in error. If you do not have repository permissions to reopen the PR, please tag a maintainer.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org