[C++][Parquet] Support min_value and max_value Statistics

[pitrou]

Describe the enhancement requested

The Statistics structure in Parquet files provides two ways of specifying lower and upper bounds for a data page:

• min and max are legacy fields for compatibility with older writers, with ill-defined comparison semantics in most cases except for signed integers
• min_value and max_value are "new" fields (introduced in 2017! - see apache/parquet-format@041708d and apache/parquet-format@bef5438) with well-defined comparison semantics depending on the logical type

Currently Parquet C++ supports only the legacy fields min and max. We should add support for reading and writing the newer ones, with the appropriate semantics on the write path.

Component(s)

Parquet

[pitrou]

cc @fatemehp @wgtmac

[wgtmac]

AFAIK, the statistics in the data page header has been deprecated after column index (part of the page index) has been introduced.

CMIW, parquet cpp is in good state with the parquet spec:

• On the read path, the statistics in the ColumnChunkMetaData already consumes min_value & max_value from the thrift object if available: https://github.com/apache/arrow/blob/master/cpp/src/parquet/metadata.cc#L97
• On the write path, we have written both min_value and min for backward-compatibility: https://github.com/apache/arrow/blob/master/cpp/src/parquet/thrift_internal.h#L292

[wgtmac]

AFAIK, the statistics in the data page header has been deprecated after column index (part of the page index) has been introduced.

CMIW, parquet cpp is in good state with the parquet spec:

• On the read path, the statistics in the ColumnChunkMetaData already consumes min_value & max_value from the thrift object if available: https://github.com/apache/arrow/blob/master/cpp/src/parquet/metadata.cc#L97
• On the write path, we have written both min_value and min for backward-compatibility: https://github.com/apache/arrow/blob/master/cpp/src/parquet/thrift_internal.h#L292

The confusing thing is that both Statistics and EncodedStatistics use min and max for both old and new meanings.

[pitrou]

CMIW, parquet cpp is in good state with the parquet spec:

Uh, I had missed that. I agree that things seem fine.

[wgtmac]

@pitrou Could you please reopen this issue? I found that there is a missing part: The C++ parquet reader does not parse column statistics correctly here: https://github.com/apache/arrow/blob/master/cpp/src/parquet/column_reader.cc#L214

// Extracts encoded statistics from V1 and V2 data page headers
template <typename H>
EncodedStatistics ExtractStatsFromHeader(const H& header) {
  EncodedStatistics page_statistics;
  if (!header.__isset.statistics) {
    return page_statistics;
  }
  const format::Statistics& stats = header.statistics;
  if (stats.__isset.max) {
    page_statistics.set_max(stats.max);
  }
  if (stats.__isset.min) {
    page_statistics.set_min(stats.min);
  }
  if (stats.__isset.null_count) {
    page_statistics.set_null_count(stats.null_count);
  }
  if (stats.__isset.distinct_count) {
    page_statistics.set_distinct_count(stats.distinct_count);
  }
  return page_statistics;
}

It should check __isset.min_value and __isset.max_value first which is similar to parquet-mr: https://github.com/apache/parquet-mr/blob/5290bd5e0ee5dc30db0576e2bfc6eea335c465cf/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java#L797

[pitrou]

@wgtmac You should probably open a new issue instead.