add can_use_stats() and revert nullable column index

Author: wgtmac

cpp/src/parquet/metadata.cc

@@ -92,75 +92,25 @@ std::string ParquetVersionToString(ParquetVersion::type ver) {
 
 namespace {
 
-enum class StatsMinMaxMode {
-  // Ignore min/max fields because their ordering is unknown or unsupported.
-  kDiscard,
-  // Use legacy min/max fields for files without column orders.
-  kLegacy,
-  // Use min_value/max_value fields with the column's well-defined order.
-  kNormal,
-};
-
-StatsMinMaxMode GetStatsMinMaxMode(const ColumnDescriptor& descr) {
+StatisticsMinMaxField GetStatisticsMinMaxField(const ColumnDescriptor& descr) {
+  if (descr.sort_order() == SortOrder::UNKNOWN) {
+    return StatisticsMinMaxField::kInvalid;
+  }
   switch (descr.column_order().get_order()) {
     case ColumnOrder::TYPE_DEFINED_ORDER:
-      return descr.sort_order() != SortOrder::UNKNOWN ? StatsMinMaxMode::kNormal
-                                                      : StatsMinMaxMode::kDiscard;
+      return StatisticsMinMaxField::kMinValueMaxValue;
     case ColumnOrder::UNDEFINED:
-      return descr.sort_order() != SortOrder::UNKNOWN ? StatsMinMaxMode::kLegacy
-                                                      : StatsMinMaxMode::kDiscard;
+      return StatisticsMinMaxField::kLegacyMinMax;
     case ColumnOrder::UNKNOWN:
-      return StatsMinMaxMode::kDiscard;
+      return StatisticsMinMaxField::kInvalid;
   }
-  return StatsMinMaxMode::kDiscard;
-}
-
-}  // namespace
-
-static EncodedStatistics EncodedStatisticsFromThrift(const format::Statistics& statistics,
-                                                     StatsMinMaxMode min_max) {
-  EncodedStatistics out;
-
-  switch (min_max) {
-    case StatsMinMaxMode::kNormal:
-      if (statistics.__isset.max_value) {
-        out.set_max(statistics.max_value);
-        if (statistics.__isset.is_max_value_exact) {
-          out.is_max_value_exact = statistics.is_max_value_exact;
-        }
-      }
-      if (statistics.__isset.min_value) {
-        out.set_min(statistics.min_value);
-        if (statistics.__isset.is_min_value_exact) {
-          out.is_min_value_exact = statistics.is_min_value_exact;
-        }
-      }
-      break;
-    case StatsMinMaxMode::kLegacy:
-      if (statistics.__isset.max) {
-        out.set_max(statistics.max);
-      }
-      if (statistics.__isset.min) {
-        out.set_min(statistics.min);
-      }
-      break;
-    case StatsMinMaxMode::kDiscard:
-      break;
-  }
-  if (statistics.__isset.null_count) {
-    out.set_null_count(statistics.null_count);
-  }
-  if (statistics.__isset.distinct_count) {
-    out.set_distinct_count(statistics.distinct_count);
-  }
-
-  return out;
+  return StatisticsMinMaxField::kInvalid;
 }
 
 template <typename DType>
-static std::shared_ptr<Statistics> MakeTypedColumnStats(
-    const format::ColumnMetaData& metadata, const ColumnDescriptor* descr,
-    ::arrow::MemoryPool* pool) {
+std::shared_ptr<Statistics> MakeTypedColumnStats(const format::ColumnMetaData& metadata,
+                                                 const ColumnDescriptor* descr,
+                                                 ::arrow::MemoryPool* pool) {
   const auto& statistics = metadata.statistics;
   const std::string kEmpty = "";
   const std::string* encoded_min = &kEmpty;
@@ -169,8 +119,8 @@ static std::shared_ptr<Statistics> MakeTypedColumnStats(
   std::optional<bool> min_exact = std::nullopt;
   std::optional<bool> max_exact = std::nullopt;
 
-  switch (GetStatsMinMaxMode(*descr)) {
-    case StatsMinMaxMode::kNormal:
+  switch (GetStatisticsMinMaxField(*descr)) {
+    case StatisticsMinMaxField::kMinValueMaxValue:
       encoded_min = &statistics.min_value;
       encoded_max = &statistics.max_value;
       has_min_max = statistics.__isset.max_value && statistics.__isset.min_value;
@@ -181,12 +131,12 @@ static std::shared_ptr<Statistics> MakeTypedColumnStats(
                       ? std::optional<bool>(statistics.is_max_value_exact)
                       : std::nullopt;
       break;
-    case StatsMinMaxMode::kLegacy:
+    case StatisticsMinMaxField::kLegacyMinMax:
       encoded_min = &statistics.min;
       encoded_max = &statistics.max;
       has_min_max = statistics.__isset.max && statistics.__isset.min;
       break;
-    case StatsMinMaxMode::kDiscard:
+    case StatisticsMinMaxField::kInvalid:
       break;
   }
 
@@ -197,8 +147,6 @@ static std::shared_ptr<Statistics> MakeTypedColumnStats(
       max_exact, pool);
 }
 
-namespace {
-
 std::shared_ptr<geospatial::GeoStatistics> MakeColumnGeometryStats(
     const format::ColumnMetaData& metadata, const ColumnDescriptor* descr) {
   if (metadata.__isset.geospatial_statistics) {
@@ -411,9 +359,8 @@ class ColumnChunkMetaData::ColumnChunkMetaDataImpl {
     {
       const std::lock_guard<std::mutex> guard(stats_mutex_);
       if (possible_encoded_stats_ == nullptr) {
-        possible_encoded_stats_ =
-            std::make_shared<EncodedStatistics>(EncodedStatisticsFromThrift(
-                column_metadata_->statistics, GetStatsMinMaxMode(*descr_)));
+        possible_encoded_stats_ = std::make_shared<EncodedStatistics>(
+            FromThrift(column_metadata_->statistics, GetStatisticsMinMaxField(*descr_)));
       }
     }
     return writer_version_->HasCorrectStatistics(type(), *possible_encoded_stats_,

cpp/src/parquet/page_index.cc

@@ -38,12 +38,6 @@ namespace parquet {
 
 namespace {
 
-bool CanTrustPageIndexMinMax(const ColumnDescriptor& descr) {
-  const auto column_order = descr.column_order().get_order();
-  return column_order != ColumnOrder::UNKNOWN && column_order != ColumnOrder::UNDEFINED &&
-         descr.sort_order() != SortOrder::UNKNOWN;
-}
-
 template <typename DType>
 void Decode(std::unique_ptr<typename EncodingTraits<DType>::Decoder>& decoder,
             const std::string& input, std::vector<typename DType::c_type>* output,
@@ -979,9 +973,6 @@ std::unique_ptr<ColumnIndex> ColumnIndex::Make(const ColumnDescriptor& descr,
     // Guard against UB when moving column_index
     throw ParquetException("Invalid ColumnIndex boundary_order");
   }
-  if (!CanTrustPageIndexMinMax(descr)) {
-    return nullptr;
-  }
   switch (descr.physical_type()) {
     case Type::BOOLEAN:
       return std::make_unique<TypedColumnIndexImpl<BooleanType>>(descr,

cpp/src/parquet/page_index_test.cc

@@ -561,58 +561,6 @@ void TestWriteTypedColumnIndex(schema::NodePtr node,
   }
 }
 
-template <typename T>
-std::shared_ptr<Buffer> SerializedColumnIndex(const T& min, const T& max) {
-  auto encode = [](const T& value) {
-    return std::string(reinterpret_cast<const char*>(&value), sizeof(value));
-  };
-  format::ColumnIndex column_index;
-  column_index.__set_null_pages({false});
-  column_index.__set_min_values({encode(min)});
-  column_index.__set_max_values({encode(max)});
-  column_index.__set_boundary_order(format::BoundaryOrder::UNORDERED);
-
-  auto sink = CreateOutputStream();
-  ThriftSerializer{}.Serialize(&column_index, sink.get());
-  PARQUET_ASSIGN_OR_THROW(auto buffer, sink->Finish());
-  return buffer;
-}
-
-void AssertColumnIndexIgnored(const ColumnDescriptor& read_descr,
-                              const std::shared_ptr<Buffer>& buffer) {
-  auto column_index =
-      ColumnIndex::Make(read_descr, buffer->data(), static_cast<uint32_t>(buffer->size()),
-                        default_reader_properties());
-  ASSERT_EQ(nullptr, column_index);
-}
-
-void AssertColumnIndexIgnoredWithColumnOrder(ColumnOrder column_order) {
-  auto node = schema::Int32("c1");
-  auto buffer = SerializedColumnIndex<int32_t>(/*min=*/1, /*max=*/2);
-
-  std::static_pointer_cast<schema::PrimitiveNode>(node)->SetColumnOrder(column_order);
-  ColumnDescriptor read_descr(node, /*max_definition_level=*/1,
-                              /*max_repetition_level=*/0);
-  AssertColumnIndexIgnored(read_descr, buffer);
-}
-
-TEST(PageIndex, ReadColumnIndexWithUnsupportedColumnOrder) {
-  AssertColumnIndexIgnoredWithColumnOrder(ColumnOrder::unknown_);
-  AssertColumnIndexIgnoredWithColumnOrder(ColumnOrder::undefined_);
-}
-
-TEST(PageIndex, ReadColumnIndexWithUnknownSortOrder) {
-  auto node = schema::PrimitiveNode::Make("c1", Repetition::REQUIRED, Type::INT96);
-  ColumnDescriptor descr(node, /*max_definition_level=*/0, /*max_repetition_level=*/0);
-  ASSERT_EQ(SortOrder::UNKNOWN, descr.sort_order());
-
-  Int96 min{{1, 2, 3}};
-  Int96 max{{4, 5, 6}};
-  auto buffer = SerializedColumnIndex(min, max);
-
-  AssertColumnIndexIgnored(descr, buffer);
-}
-
 TEST(PageIndex, WriteInt32ColumnIndex) {
   auto encode = [=](int32_t value) {
     return std::string(reinterpret_cast<const char*>(&value), sizeof(int32_t));

cpp/src/parquet/schema.h

@@ -382,6 +382,12 @@ class PARQUET_EXPORT ColumnDescriptor {
     return la ? GetSortOrder(la, pt) : GetSortOrder(converted_type(), pt);
   }
 
+  // Whether ColumnOrder-governed min/max values have a supported ordering.
+  bool can_use_stats() const {
+    return column_order().get_order() == ColumnOrder::TYPE_DEFINED_ORDER &&
+           sort_order() != SortOrder::UNKNOWN;
+  }
+
   const std::string& name() const { return primitive_node_->name(); }
 
   const std::shared_ptr<schema::ColumnPath> path() const;

cpp/src/parquet/schema_test.cc

@@ -660,6 +660,22 @@ TEST(TestColumnDescriptor, TestAttrs) {
   ASSERT_EQ(expected_descr, descr2.ToString());
 }
 
+TEST(TestColumnDescriptor, CanUseStats) {
+  NodePtr node = Int32("name");
+  ColumnDescriptor descr(node, 0, 0);
+  EXPECT_TRUE(descr.can_use_stats());
+
+  auto primitive_node = std::static_pointer_cast<PrimitiveNode>(node);
+  primitive_node->SetColumnOrder(ColumnOrder::undefined_);
+  EXPECT_FALSE(ColumnDescriptor(node, 0, 0).can_use_stats());
+
+  primitive_node->SetColumnOrder(ColumnOrder::unknown_);
+  EXPECT_FALSE(ColumnDescriptor(node, 0, 0).can_use_stats());
+
+  node = PrimitiveNode::Make("name", Repetition::REQUIRED, Type::INT96);
+  EXPECT_FALSE(ColumnDescriptor(node, 0, 0).can_use_stats());
+}
+
 class TestSchemaDescriptor : public ::testing::Test {
  public:
   void setUp() {}

cpp/src/parquet/thrift_internal.h

@@ -252,12 +252,21 @@ static inline AadMetadata FromThrift(format::AesGcmCtrV1 aesGcmCtrV1) {
                      aesGcmCtrV1.supply_aad_prefix};
 }
 
-static inline EncodedStatistics FromThrift(const format::Statistics& stats) {
+// Selects which thrift Statistics min/max fields should populate EncodedStatistics.
+enum class StatisticsMinMaxField {
+  // Do not populate min/max, because the ordering is undefined or unsupported.
+  kInvalid,
+  // Populate min/max from the min_value/max_value fields.
+  kMinValueMaxValue,
+  // Populate min/max from the legacy min/max fields.
+  kLegacyMinMax,
+};
+
+static inline EncodedStatistics FromThrift(const format::Statistics& stats,
+                                           StatisticsMinMaxField min_max) {
   EncodedStatistics out;
 
-  // Use the new V2 min-max statistics over the former one if it is filled
-  if (stats.__isset.max_value || stats.__isset.min_value) {
-    // TODO: check if the column_order is TYPE_DEFINED_ORDER.
+  if (min_max == StatisticsMinMaxField::kMinValueMaxValue) {
     if (stats.__isset.max_value) {
       out.set_max(stats.max_value);
       if (stats.__isset.is_max_value_exact) {
@@ -270,9 +279,7 @@ static inline EncodedStatistics FromThrift(const format::Statistics& stats) {
         out.is_min_value_exact = stats.is_min_value_exact;
       }
     }
-  } else if (stats.__isset.max || stats.__isset.min) {
-    // TODO: check created_by to see if it is corrupted for some types.
-    // TODO: check if the sort_order is SIGNED.
+  } else if (min_max == StatisticsMinMaxField::kLegacyMinMax) {
     if (stats.__isset.max) {
       out.set_max(stats.max);
     }
@@ -290,6 +297,14 @@ static inline EncodedStatistics FromThrift(const format::Statistics& stats) {
   return out;
 }
 
+static inline EncodedStatistics FromThrift(const format::Statistics& stats) {
+  // Use the new V2 min-max statistics over the former one if it is filled.
+  if (stats.__isset.max_value || stats.__isset.min_value) {
+    return FromThrift(stats, StatisticsMinMaxField::kMinValueMaxValue);
+  }
+  return FromThrift(stats, StatisticsMinMaxField::kLegacyMinMax);
+}
+
 static inline geospatial::EncodedGeoStatistics FromThrift(
     const format::GeospatialStatistics& geo_stats) {
   geospatial::EncodedGeoStatistics out;