Merge remote-tracking branch 'upstream/main' into geoarrow-writer

.github/workflows/run-tests.yaml

@@ -19,12 +19,13 @@ jobs:
         uses: actions/checkout@v4
 
       - name: Setup mambaforge
-        uses: conda-incubator/setup-miniconda@v2
+        uses: conda-incubator/setup-miniconda@v3
         with:
-          miniforge-variant: Mambaforge
+          miniforge-variant: Miniforge3
           miniforge-version: latest
           activate-environment: s2geography-dev
           use-mamba: true
+          mamba-version: "*"
 
       - name: Get Date
         id: get-date

CMakeLists.txt

@@ -48,7 +48,9 @@ endif()
 
 # --- s2geometry
 
-if(DEFINED ENV{CONDA_PREFIX})
+if(TARGET s2::s2)
+  set(S2GEOGRAPHY_S2_SOURCE_DEFAULT "NONE")
+elseif(DEFINED ENV{CONDA_PREFIX})
   set(S2GEOGRAPHY_S2_SOURCE_DEFAULT "CONDA")
 else()
   set(S2GEOGRAPHY_S2_SOURCE_DEFAULT "AUTO")
@@ -108,8 +110,8 @@ macro(build_s2)
 
   FetchContent_MakeAvailable(s2)
 
-  # silent warnings in s2 headers
-  # (note: the S2_USE_SYSTEM_INCLUDES=ON option doesn't seem to worl with FetchContent)
+  # silence warnings in s2 headers
+  # (note: the S2_USE_SYSTEM_INCLUDES=ON option doesn't seem to work with FetchContent)
   # (TODO: remove this and instead add SYSTEM in FetchContent_Declare - requires CMake >=3.25)
   get_target_property(S2_IID s2 INTERFACE_INCLUDE_DIRECTORIES)
   set_target_properties(s2 PROPERTIES INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "${S2_IID}")
@@ -147,18 +149,6 @@ elseif(${S2_SOURCE} STREQUAL "SYSTEM")
   endif()
 endif()
 
-if (MSVC AND NOT ${S2_SOURCE} STREQUAL "BUNDLED")
-  # used in s2geometry's CMakeLists.txt but not defined in target
-  target_compile_definitions(s2::s2 INTERFACE _USE_MATH_DEFINES)
-  target_compile_definitions(s2::s2 INTERFACE NOMINMAX)
-  target_compile_options(s2::s2 INTERFACE /J)
-endif()
-
-# this might be needed since s2geometry includes it in general
-# but not for any target explicilty?
-find_package(OpenSSL REQUIRED)
-target_include_directories(${s2_NOALIAS_TARGET} INTERFACE ${OPENSSL_INCLUDE_DIR})
-
 # --- Abseil (bundled build not supported)
 
 find_package(absl REQUIRED)
@@ -221,8 +211,9 @@ add_library(s2geography
     src/vendored/ryu/d2s.c
     src/vendored/geoarrow/double_parse_fast_float.cc)
 
-set_target_properties(s2geography PROPERTIES
-    POSITION_INDEPENDENT_CODE ${BUILD_SHARED_LIBS})
+if(NOT BUILD_SHARED_LIBS)
+  set_target_properties(s2geography PROPERTIES POSITION_INDEPENDENT_CODE ON)
+endif()
 
 target_include_directories(
   s2geography
@@ -243,6 +234,13 @@ target_compile_definitions(
   GEOARROW_USE_RYU=1
   GEOARROW_NAMESPACE=S2Geography)
 
+if (MSVC)
+  # used in s2geometry's CMakeLists.txt but not defined in target
+  target_compile_definitions(s2geography PUBLIC _USE_MATH_DEFINES)
+  target_compile_definitions(s2geography PUBLIC NOMINMAX)
+  target_compile_options(s2geography PUBLIC /J)
+endif()
+
 target_link_libraries(
   s2geography
   PUBLIC
@@ -256,6 +254,7 @@ if(S2GEOGRAPHY_BUILD_TESTS)
 
   add_executable(distance_test src/s2geography/distance_test.cc)
   add_executable(geoarrow_test src/s2geography/geoarrow_test.cc)
+  add_executable(geography_test src/s2geography/geography_test.cc)
   add_executable(wkt_writer_test src/s2geography/wkt-writer_test.cc)
 
   if (S2GEOGRAPHY_CODE_COVERAGE)
@@ -266,11 +265,13 @@ if(S2GEOGRAPHY_BUILD_TESTS)
 
   target_link_libraries(distance_test s2geography GTest::gtest_main)
   target_link_libraries(geoarrow_test s2geography nanoarrow GTest::gtest_main)
+  target_link_libraries(geography_test s2geography nanoarrow GTest::gtest_main)
   target_link_libraries(wkt_writer_test s2geography GTest::gtest_main)
 
   include(GoogleTest)
   gtest_discover_tests(distance_test)
   gtest_discover_tests(geoarrow_test)
+  gtest_discover_tests(geography_test)
   gtest_discover_tests(wkt_writer_test)
 endif()
 

src/s2geography/geoarrow.cc

@@ -61,6 +61,12 @@ static inline int8_t GeoArrowBitmapReaderNextIsNull(
   return (bitmap_reader->byte & (1 << bitmap_reader->bit_i)) == 0;
 }
 
+/// \brief Construct Geography objects while visiting a GeoArrow array.
+///
+/// This class implements the visitor callbacks that construct Geography
+/// objects. This visitor gets called by the Reader, which iterates over the
+/// features in a GeoArrow ArrowArray (that can be either in WKT or WKB
+/// serialized format or in native format).
 class Constructor {
  public:
   Constructor(const ImportOptions& options) : options_(options) {
@@ -577,17 +583,12 @@ class ReaderImpl {
  public:
   ReaderImpl() {
     error_.message[0] = '\0';
-    wkt_reader_.private_data = nullptr;
-    wkb_reader_.private_data = nullptr;
+    reader_.private_data = nullptr;
   }
 
   ~ReaderImpl() {
-    if (wkt_reader_.private_data != nullptr) {
-      GeoArrowWKTReaderReset(&wkt_reader_);
-    }
-
-    if (wkb_reader_.private_data != nullptr) {
-      GeoArrowWKBReaderReset(&wkb_reader_);
+    if (reader_.private_data != nullptr) {
+      GeoArrowArrayReaderReset(&reader_);
     }
   }
 
@@ -612,13 +613,7 @@ class ReaderImpl {
     constructor_->InitVisitor(&visitor_);
     visitor_.error = &error_;
 
-    if (array_view_.schema_view.type == GEOARROW_TYPE_WKT) {
-      GeoArrowWKTReaderInit(&wkt_reader_);
-    }
-
-    if (array_view_.schema_view.type == GEOARROW_TYPE_WKB) {
-      GeoArrowWKBReaderInit(&wkb_reader_);
-    }
+    GeoArrowArrayReaderInit(&reader_);
   }
 
   void ReadGeography(const ArrowArray* array, int64_t offset, int64_t length,
@@ -631,83 +626,18 @@ class ReaderImpl {
     }
 
     constructor_->SetOutput(out);
-
-    switch (array_view_.schema_view.type) {
-      case GEOARROW_TYPE_WKT:
-        code = VisitWKT(offset, length);
-        break;
-      case GEOARROW_TYPE_WKB:
-        code = VisitWKB(offset, length);
-        break;
-      default:
-        code = GeoArrowArrayViewVisit(&array_view_, offset, length, &visitor_);
-        break;
-    }
-
+    code = GeoArrowArrayReaderVisit(&reader_, &array_view_, offset, length, &visitor_);
     ThrowNotOk(code);
   }
 
  private:
   ImportOptions options_;
   std::unique_ptr<FeatureConstructor> constructor_;
   GeoArrowArrayView array_view_;
-  GeoArrowWKTReader wkt_reader_;
-  GeoArrowWKBReader wkb_reader_;
+  GeoArrowArrayReader reader_;
   GeoArrowVisitor visitor_;
   GeoArrowError error_;
 
-  int VisitWKT(int64_t offset, int64_t length) {
-    offset += array_view_.offset[0];
-    const uint8_t* validity = array_view_.validity_bitmap;
-    const int32_t* offsets = array_view_.offsets[0];
-    const char* data = reinterpret_cast<const char*>(array_view_.data);
-    GeoArrowStringView item;
-
-    GeoArrowBitmapReader bitmap;
-    GeoArrowBitmapReaderInit(&bitmap, validity, offset);
-
-    for (int64_t i = 0; i < length; i++) {
-      if (GeoArrowBitmapReaderNextIsNull(&bitmap)) {
-        GEOARROW_RETURN_NOT_OK(visitor_.feat_start(&visitor_));
-        GEOARROW_RETURN_NOT_OK(visitor_.null_feat(&visitor_));
-        GEOARROW_RETURN_NOT_OK(visitor_.feat_end(&visitor_));
-      } else {
-        item.size_bytes = offsets[offset + i + 1] - offsets[offset + i];
-        item.data = data + offsets[offset + i];
-        GEOARROW_RETURN_NOT_OK(
-            GeoArrowWKTReaderVisit(&wkt_reader_, item, &visitor_));
-      }
-    }
-
-    return GEOARROW_OK;
-  }
-
-  int VisitWKB(int64_t offset, int64_t length) {
-    offset += array_view_.offset[0];
-    const uint8_t* validity = array_view_.validity_bitmap;
-    const int32_t* offsets = array_view_.offsets[0];
-    const uint8_t* data = array_view_.data;
-    GeoArrowBufferView item;
-
-    GeoArrowBitmapReader bitmap;
-    GeoArrowBitmapReaderInit(&bitmap, validity, offset);
-
-    for (int64_t i = 0; i < length; i++) {
-      if (GeoArrowBitmapReaderNextIsNull(&bitmap)) {
-        GEOARROW_RETURN_NOT_OK(visitor_.feat_start(&visitor_));
-        GEOARROW_RETURN_NOT_OK(visitor_.null_feat(&visitor_));
-        GEOARROW_RETURN_NOT_OK(visitor_.feat_end(&visitor_));
-      } else {
-        item.size_bytes = offsets[offset + i + 1] - offsets[offset + i];
-        item.data = data + offsets[offset + i];
-        GEOARROW_RETURN_NOT_OK(
-            GeoArrowWKBReaderVisit(&wkb_reader_, item, &visitor_));
-      }
-    }
-
-    return GEOARROW_OK;
-  }
-
   void ThrowNotOk(int code) {
     if (code != GEOARROW_OK) {
       throw Exception(error_.message);

src/s2geography/geoarrow.h

@@ -55,6 +55,10 @@ class ImportOptions : public TessellationOptions {
 
 class ReaderImpl;
 
+/// \brief Array reader for any GeoArrow extension array
+///
+/// This class is used to convert an ArrowArray with geoarrow data (serialized
+/// or native) into a vector of Geography objects.
 class Reader {
  public:
   enum class InputType { kWKT, kWKB };

src/s2geography/geography.cc

@@ -23,7 +23,7 @@ using namespace s2geography;
 // s2/s2shapeutil_coding.cc.
 class S2ShapeWrapper : public S2Shape {
  public:
-  S2ShapeWrapper(S2Shape* shape) : shape_(shape) {}
+  S2ShapeWrapper(const S2Shape* shape) : shape_(shape) {}
   int num_edges() const { return shape_->num_edges(); }
   Edge edge(int edge_id) const { return shape_->edge(edge_id); }
   int dimension() const { return shape_->dimension(); }
@@ -40,7 +40,7 @@ class S2ShapeWrapper : public S2Shape {
   }
 
  private:
-  S2Shape* shape_;
+  const S2Shape* shape_;
 };
 
 // Just like the S2ShapeWrapper, the S2RegionWrapper helps reconcile the
@@ -171,7 +171,7 @@ int ShapeIndexGeography::num_shapes() const {
 }
 
 std::unique_ptr<S2Shape> ShapeIndexGeography::Shape(int id) const {
-  S2Shape* shape = shape_index_.shape(id);
+  const S2Shape* shape = shape_index_.shape(id);
   return std::unique_ptr<S2Shape>(new S2ShapeWrapper(shape));
 }
 

src/s2geography/geography.h

@@ -117,7 +117,7 @@ class PolylineGeography : public Geography {
 // perspective).
 class PolygonGeography : public Geography {
  public:
-  PolygonGeography() {}
+  PolygonGeography() : polygon_(std::make_unique<S2Polygon>()) {}
   PolygonGeography(std::unique_ptr<S2Polygon> polygon)
       : polygon_(std::move(polygon)) {}
 

src/s2geography/geography_test.cc

@@ -0,0 +1,12 @@
+#include <gtest/gtest.h>
+
+#include "s2geography.h"
+#include "s2geography/geography.h"
+
+using namespace s2geography;
+
+TEST(Geography, EmptyPolygon) {
+  PolygonGeography geog;
+
+  EXPECT_TRUE(geog.Polygon()->is_empty());
+}

src/vendored/geoarrow/double_parse_fast_float.cc

@@ -5,9 +5,14 @@
 
 #include "fast_float/fast_float.h"
 
+// When building a DuckDB extension, this is duckdb_fast_float
+#if !defined(GEOARROW_FAST_FLOAT_NAMESPACE)
+#define GEOARROW_FAST_FLOAT_NAMESPACE fast_float
+#endif
+
 extern "C" GeoArrowErrorCode GeoArrowFromChars(const char* first, const char* last,
                                                double* out) {
-  auto answer = fast_float::from_chars(first, last, *out);
+  auto answer = GEOARROW_FAST_FLOAT_NAMESPACE::from_chars(first, last, *out);
   if (answer.ec != std::errc()) {
     return EINVAL;
   } else {