[GitHub] [arrow] mapleFU commented on a diff in pull request #35989: GH-34351: [C++][Parquet] Statistic: tiny optimization and specification

["mapleFU (via GitHub)" <gi...@apache.org>]

mapleFU commented on code in PR #35989:
URL: https://github.com/apache/arrow/pull/35989#discussion_r1229429805


##########
cpp/src/parquet/statistics.cc:
##########
@@ -472,34 +474,40 @@ class TypedStatisticsImpl : public TypedStatistics<DType> {
     comparator_ = std::static_pointer_cast<TypedComparator<DType>>(comp);
     TypedStatisticsImpl::Reset();
     has_null_count_ = true;
-    has_distinct_count_ = true;
+    has_distinct_count_ = false;

Review Comment:
   Yes this change the syntax for `has_distinct_count_`, but currently `has_distinct_count_` is never used by writer...



##########
cpp/src/parquet/statistics.cc:
##########
@@ -552,12 +560,19 @@ class TypedStatisticsImpl : public TypedStatistics<DType> {
 
   void Merge(const TypedStatistics<DType>& other) override {
     this->num_values_ += other.num_values();
-    if (other.HasNullCount()) {
+    // Merge always runs when Merge builder's page statistics
+    // into column chunk statistics, so it tent to have null count.
+    if (ARROW_PREDICT_TRUE(other.HasNullCount())) {
       this->statistics_.null_count += other.null_count();
+    } else {
+      this->has_null_count_ = false;
     }
-    if (other.HasDistinctCount()) {
-      this->statistics_.distinct_count += other.distinct_count();
+    // Distinct count cannot be merged.
+    if (has_distinct_count_) {
+      has_distinct_count_ = false;
     }
+    // If !other.HasMinMax, might be all-nulls or nulls and nan,

Review Comment:
   The interesting thing is, currently, "merge" is only called by writer, and writer is always the case (1)...
   @pitrou Do you have any idea here? Since `Statistics` is already public.



##########
cpp/src/parquet/statistics.cc:
##########
@@ -552,12 +560,19 @@ class TypedStatisticsImpl : public TypedStatistics<DType> {
 
   void Merge(const TypedStatistics<DType>& other) override {
     this->num_values_ += other.num_values();
-    if (other.HasNullCount()) {
+    // Merge always runs when Merge builder's page statistics
+    // into column chunk statistics, so it tent to have null count.
+    if (ARROW_PREDICT_TRUE(other.HasNullCount())) {

Review Comment:
   if `!this->has_null_count_`, adding a number is non-sense, so I think it's ok to keep it



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