Review comments

Author: Deepak Majeti

src/parquet/column_writer.cc

@@ -268,8 +268,7 @@ int64_t ColumnWriter::Close() {
 
     EncodedStatistics chunk_statistics = GetChunkStatistics();
     if (chunk_statistics.is_set())
-      metadata_->SetStatistics(SortOrder::SIGNED == GetSortOrder(descr_->logical_type(),
-                                                                 descr_->physical_type()),
+      metadata_->SetStatistics(SortOrder::SIGNED == descr_->sort_order(),
                                chunk_statistics);
     pager_->Close(has_dictionary_, fallback_);
   }
@@ -321,8 +320,7 @@ TypedColumnWriter<Type>::TypedColumnWriter(ColumnChunkMetaDataBuilder* metadata,
   }
 
   if (properties->statistics_enabled(descr_->path()) &&
-      (SortOrder::UNKNOWN !=
-       GetSortOrder(descr_->logical_type(), descr_->physical_type()))) {
+      (SortOrder::UNKNOWN != descr_->sort_order())) {
     page_statistics_ = std::unique_ptr<TypedStats>(new TypedStats(descr_, allocator_));
     chunk_statistics_ = std::unique_ptr<TypedStats>(new TypedStats(descr_, allocator_));
   }

src/parquet/file/metadata.cc

@@ -117,8 +117,7 @@ class ColumnChunkMetaData::ColumnChunkMetaDataImpl {
   inline bool is_stats_set() const {
     DCHECK(writer_version_ != nullptr);
     return column_->meta_data.__isset.statistics &&
-           writer_version_->HasCorrectStatistics(
-               type(), GetSortOrder(descr_->logical_type(), descr_->physical_type()));
+           writer_version_->HasCorrectStatistics(type(), descr_->sort_order());
   }
 
   inline std::shared_ptr<RowGroupStatistics> statistics() const {

src/parquet/schema.h

@@ -332,6 +332,9 @@ class PARQUET_EXPORT ColumnDescriptor {
 
   LogicalType::type logical_type() const { return primitive_node_->logical_type(); }
 
+  SortOrder::type sort_order() const { return GetSortOrder(logical_type(),
+                                                           physical_type()); }
+
   const std::string& name() const { return primitive_node_->name(); }
 
   const std::shared_ptr<schema::ColumnPath> path() const;

src/parquet/statistics.cc

@@ -34,7 +34,6 @@ TypedRowGroupStatistics<DType>::TypedRowGroupStatistics(const ColumnDescriptor*
     : pool_(pool),
       min_buffer_(AllocateBuffer(pool_, 0)),
       max_buffer_(AllocateBuffer(pool_, 0)) {
-  comparator_ = std::static_pointer_cast<CompareDefault<DType> >(Compare::Make(schema));
   SetDescr(schema);
   Reset();
 }
@@ -69,7 +68,6 @@ TypedRowGroupStatistics<DType>::TypedRowGroupStatistics(
   IncrementNullCount(null_count);
   IncrementDistinctCount(distinct_count);
 
-  comparator_ = std::static_pointer_cast<CompareDefault<DType> >(Compare::Make(schema));
   SetDescr(schema);
 
   if (!encoded_min.empty()) {
@@ -86,6 +84,11 @@ bool TypedRowGroupStatistics<DType>::HasMinMax() const {
   return has_min_max_;
 }
 
+template <typename DType>
+void TypedRowGroupStatistics<DType>::SetComparator() {
+  comparator_ = std::static_pointer_cast<CompareDefault<DType> >(Compare::Make(descr_));
+}
+
 template <typename DType>
 void TypedRowGroupStatistics<DType>::Reset() {
   ResetCounts();

src/parquet/statistics.h

@@ -98,13 +98,19 @@ class PARQUET_EXPORT RowGroupStatistics
 
   virtual EncodedStatistics Encode() = 0;
 
+  // Set the Corresponding Comparator
+  virtual void SetComparator() = 0;
+
   virtual ~RowGroupStatistics() {}
 
   Type::type physical_type() const { return descr_->physical_type(); }
 
  protected:
   const ColumnDescriptor* descr() const { return descr_; }
-  void SetDescr(const ColumnDescriptor* schema) { descr_ = schema; }
+  void SetDescr(const ColumnDescriptor* schema) {
+    descr_ = schema;
+    SetComparator();
+  }
 
   void IncrementNullCount(int64_t n) { statistics_.null_count += n; }
 
@@ -147,6 +153,7 @@ class TypedRowGroupStatistics : public RowGroupStatistics {
 
   bool HasMinMax() const override;
   void Reset() override;
+  void SetComparator() override;
   void Merge(const TypedRowGroupStatistics<DType>& other);
 
   void Update(const T* values, int64_t num_not_null, int64_t num_null);

src/parquet/util/comparison.cc

@@ -25,7 +25,7 @@
 namespace parquet {
 
 std::shared_ptr<Compare> Compare::Make(const ColumnDescriptor* descr) {
-  if (SortOrder::SIGNED == GetSortOrder(descr->logical_type(), descr->physical_type())) {
+  if (SortOrder::SIGNED == descr->sort_order()) {
     switch (descr->physical_type()) {
       case Type::BOOLEAN:
         return std::make_shared<CompareDefaultBoolean>();
@@ -46,8 +46,7 @@ std::shared_ptr<Compare> Compare::Make(const ColumnDescriptor* descr) {
       default:
         ParquetException::NYI("Signed Compare not implemented");
     }
-  } else if (SortOrder::UNSIGNED ==
-             GetSortOrder(descr->logical_type(), descr->physical_type())) {
+  } else if (SortOrder::UNSIGNED == descr->sort_order()) {
     switch (descr->physical_type()) {
       case Type::INT32:
         return std::make_shared<CompareUnsignedInt32>();