GH-34351: [C++][Parquet] Statistic: tiny optimization

[mapleFU]

Rationale for this change

This patch does some tiny optimizations on Parquet C++ Statistics. It does:

1. For min-max, using std::string. Because assume the case like that:

EncodedStatistics c1;
// do some operations
EncodedStatistics c2 = c1;
c2.set_max("dasdasdassd");

After c2 set, c1 would be set too. So I use std::string here.

2. Force setting ndv count during merging, and add some comments

What changes are included in this PR?

As we talked above.

Are these changes tested?

Test by unittest.

Are there any user-facing changes?

No

• Closes: [C++][Parquet] Statistics Merge ignore setting has flag #34351

[github-actions]

• Closes: [C++][Parquet] Statistics Merge ignore setting has flag #34351

[github-actions]

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

[mapleFU]

===============================================================================
Omission: parquet::RowGroupMetaData::Equals() isn't stable. [test: #==(TestParquetRowGroupMetadata)]
/Users/runner/work/arrow/arrow/c_glib/test/parquet/test-row-group-metadata.rb:48:in `block in <class:TestParquetRowGroupMetadata>'
===============================================================================
.........F
===============================================================================
Failure: test: #has_n_distinct_values?(TestParquetStatistics):
        @statistics.has_n_distinct_values?
        |           |
        |           false
        #<Parquet::Int32Statistics:0x7f86979871f8 ptr=0x7f8698c1c8d0>
/Users/runner/work/arrow/arrow/c_glib/test/parquet/test-statistics.rb:53:in `block in <class:TestParquetStatistics>'
     50:   end
     51: 
     52:   test("#has_n_distinct_values?") do
  => 53:     assert do
     54:       @statistics.has_n_distinct_values?
     55:     end
     56:   end
===============================================================================
..............O

emmmm. The problem can is also seen here: https://github.com/apache/arrow/pull/34054/files#r1118029417

[mapleFU]

After go through the piece of code, I found that current impl is ok, because we mostly only use statistics on writer(In fact, Statistics should better be renamed to StatisticsBuilder, I think) . But when ExtractStatisticsFromPageHeader or other reader part is in, things will get a bit more complex.

As for now, we can assume that:

1. Writer can assure that if has right null-count ( if it not has any bugs )
2. Currently I found that ndv is never collected. If a user collect ndv in page1, but not collect ndv in page 2, it should be abandon.

For reader:

1. When deserialize, reader should assume that ndv and null_count can be unset ( but currently, it doesn't work like this)
2. Deserialized statistics can not call merge or other mutation methods

Currently, a writer will not has bug on merging. But if a reader, checks has_null_count or ndv, it will get the wrong result

[wgtmac]

IMO, if has_null_count is not used we should remove it to reduce confusion and future misuse. Otherwise, we should respect it from the input argument.

[mapleFU]

I agree with you, but I need other's opinion

[wjones127]

IIUC, this class is only used in the writer and we always write the null count in statistics? If so, then I agree we can remove this.

[mapleFU]

After a weird patch, this class is used in reader, but ndv is not used.

[wjones127]

what is ndv?

[mapleFU]

Sorry for make misunderstanding. NDV means num-different values. Load statistics is introduced in ColumnChunkMetaData, and it's weird that:

1. Writer will never set ndv.
2. Reader should read ndv.

The behavior here is trickey

[wgtmac]

It seems that statistics_ already has a set of hasXXX variables. Can we reuse those directly?

class PARQUET_EXPORT EncodedStatistics {
  std::shared_ptr<std::string> max_, min_;
  bool is_signed_ = false;

 public:
  EncodedStatistics()
      : max_(std::make_shared<std::string>()), min_(std::make_shared<std::string>()) {}


  int64_t null_count = 0;
  int64_t distinct_count = 0;

  bool has_min = false;
  bool has_max = false;
  bool has_null_count = false;
  bool has_distinct_count = false;

[wgtmac]

It would be better to add a case where either side does not have a valid stats (min/max/null_count/distinct_count) meaning that the merged stats is dropped.

[mapleFU]

Sure, I will add them

[mapleFU]

@kou I guess ruby test should not set has ndv, because current parquet writer will never put valid ndv. Let me set it to false later.

As @wgtmac says, maybe it's better to make it "false"

[kou]

OK!

[mapleFU]

@pitrou @emkornfield I need some idea about the code here. The problem is that:

1. When writing statistics, the code is ok.
2. Statistics::Make would create statistics using EncodedStats for read, then the code here would be a disaster

[mapleFU]

This patch is neccessary but not visited for a long time. I'll close and recreate this this weekend.