Address review comments in apache#43196

Kontinuation · Sep 6, 2024 · fb134d3 · fb134d3
1 parent e89962f
commit fb134d3
Show file tree

Hide file tree

Showing 8 changed files with 50 additions and 6 deletions.
diff --git a/cpp/src/parquet/CMakeLists.txt b/cpp/src/parquet/CMakeLists.txt
@@ -379,7 +379,7 @@ add_parquet_test(internals-test
                  public_api_test.cc
                  types_test.cc)
 
-add_parquet_test(geometry-test SOURCES geometry_util_test.cc)
+add_parquet_test(geometry-test SOURCES geometry_util_internal_test.cc)
 
 set_source_files_properties(public_api_test.cc PROPERTIES SKIP_PRECOMPILE_HEADERS ON
                                                           SKIP_UNITY_BUILD_INCLUSION ON)

diff --git a/cpp/src/parquet/column_writer_test.cc b/cpp/src/parquet/column_writer_test.cc
@@ -398,7 +398,7 @@ class TestPrimitiveWriter : public PrimitiveTypedTest<TestType> {
         ColumnChunkMetaData::Make(metadata_->contents(), this->descr_);
     return metadata_accessor->key_value_metadata();
   }
-  
+
   EncodedStatistics metadata_encoded_stats() {
     ApplicationVersion app_version(this->writer_properties_->created_by());
     auto metadata_accessor = ColumnChunkMetaData::Make(
@@ -1780,7 +1780,7 @@ TEST_F(TestInt32Writer, WriteKeyValueMetadataEndToEnd) {
   ASSERT_OK_AND_ASSIGN(auto value, key_value_metadata->Get("foo"));
   ASSERT_EQ("bar", value);
 }
-  
+
 // Test writing and reading geometry columns
 class TestGeometryValuesWriter : public TestPrimitiveWriter<ByteArrayType> {
  public:

diff --git a/cpp/src/parquet/geometry_util.h → cpp/src/parquet/geometry_util_internal.h b/cpp/src/parquet/geometry_util.h → cpp/src/parquet/geometry_util_internal.h
diff --git a/cpp/src/parquet/geometry_util_test.cc → ...rc/parquet/geometry_util_internal_test.cc b/cpp/src/parquet/geometry_util_test.cc → ...rc/parquet/geometry_util_internal_test.cc
@@ -20,7 +20,7 @@
 
 #include "arrow/testing/gtest_compat.h"
 
-#include "parquet/geometry_util.h"
+#include "parquet/geometry_util_internal.h"
 
 namespace parquet::geometry {
 

diff --git a/cpp/src/parquet/statistics.cc b/cpp/src/parquet/statistics.cc
@@ -37,7 +37,7 @@
 #include "arrow/visit_data_inline.h"
 #include "parquet/encoding.h"
 #include "parquet/exception.h"
-#include "parquet/geometry_util.h"
+#include "parquet/geometry_util_internal.h"
 #include "parquet/platform.h"
 #include "parquet/schema.h"
 #include "parquet/types.h"
@@ -109,6 +109,28 @@ class GeometryStatisticsImpl {
     }
   }
 
+  void UpdateSpaced(const ByteArray* values, const uint8_t* valid_bits,
+                    int64_t valid_bits_offset, int64_t num_spaced_values,
+                    int64_t num_values, int64_t null_count) {
+    DCHECK_GT(num_spaced_values, 0);
+
+    geometry::WKBBuffer buf;
+    try {
+      ::arrow::internal::VisitSetBitRunsVoid(
+          valid_bits, valid_bits_offset, num_spaced_values,
+          [&](int64_t position, int64_t length) {
+            for (int64_t i = 0; i < num_spaced_values; i++) {
+              ByteArray item = SafeLoad(values + i + position);
+              buf.Init(item.ptr, item.len);
+              bounder_.ReadGeometry(&buf);
+            }
+          });
+      bounder_.Flush();
+    } catch (ParquetException& e) {
+      is_valid_ = false;
+    }
+  }
+
   EncodedGeometryStatistics Encode() const {
     const double* mins = bounder_.Bounds().min;
     const double* maxes = bounder_.Bounds().max;
@@ -1093,6 +1115,16 @@ void TypedStatisticsImpl<DType>::UpdateSpaced(const T* values, const uint8_t* va
   if (num_values == 0) return;
   SetMinMaxPair(comparator_->GetMinMaxSpaced(values, num_spaced_values, valid_bits,
                                              valid_bits_offset));
+
+  if constexpr (std::is_same<T, ByteArray>::value) {
+    if (logical_type_ == LogicalType::Type::GEOMETRY) {
+      if (geometry_statistics_ == nullptr) {
+        geometry_statistics_ = std::make_unique<GeometryStatistics>();
+      }
+      geometry_statistics_->UpdateSpaced(values, valid_bits, valid_bits_offset,
+                                         num_spaced_values, num_values, null_count);
+    }
+  }
 }
 
 template <typename DType>

diff --git a/cpp/src/parquet/statistics.h b/cpp/src/parquet/statistics.h
@@ -153,6 +153,10 @@ class PARQUET_EXPORT GeometryStatistics {
 
   void Update(const ByteArray* values, int64_t num_values, int64_t null_count);
 
+  void UpdateSpaced(const ByteArray* values, const uint8_t* valid_bits,
+                    int64_t valid_bits_offset, int64_t num_spaced_values,
+                    int64_t num_values, int64_t null_count);
+
   EncodedGeometryStatistics Encode();
 
   bool is_valid() const;

diff --git a/cpp/src/parquet/test_util.h b/cpp/src/parquet/test_util.h
@@ -39,7 +39,7 @@
 #include "parquet/column_reader.h"
 #include "parquet/column_writer.h"
 #include "parquet/encoding.h"
-#include "parquet/geometry_util.h"
+#include "parquet/geometry_util_internal.h"
 #include "parquet/platform.h"
 
 // https://github.com/google/googletest/pull/2904 might not be available

diff --git a/cpp/src/parquet/types.cc b/cpp/src/parquet/types.cc
@@ -474,12 +474,20 @@ std::shared_ptr<const LogicalType> LogicalType::FromThrift(
       edges = LogicalType::GeometryEdges::PLANAR;
     } else if (type.GEOMETRY.edges == format::Edges::SPHERICAL) {
       edges = LogicalType::GeometryEdges::SPHERICAL;
+    } else {
+      std::stringstream ss;
+      ss << "Unknown value for geometry edges: " << type.GEOMETRY.edges;
+      throw ParquetException(ss.str());
     }
 
     LogicalType::GeometryEncoding::geometry_encoding encoding =
         LogicalType::GeometryEncoding::UNKNOWN;
     if (type.GEOMETRY.encoding == format::GeometryEncoding::WKB) {
       encoding = LogicalType::GeometryEncoding::WKB;
+    } else {
+      std::stringstream ss;
+      ss << "Unknown value for geometry encoding: " << type.GEOMETRY.edges;
+      throw ParquetException(ss.str());
     }
 
     std::string metadata;