Apply the ring array's slice offset, and correct the record

An Arrow list's offsets are logical indices into its child, so the child's
own slice offset has to be added before its offsets buffer is read in turn.
NativeReader did that for the outermost list and for the coordinate array,
but not for the ring array in between: a polygon column whose rings child
carries a slice offset was read from the wrong rings, silently, because
nothing in the outer array says so and geoarrow-c bounds-checks nothing.
Reachable from pyarrow by building a ListArray over an already-sliced
ListArray - the test does exactly that.

Fixed by routing every index into offsets[level] through offsetAt, which is
the one place the level's own offset is added, so no level can be forgotten
again.

This also corrects a claim made in ba046f9 and repeated in the code:
geoarrow-c does not read a sliced coordinate array from the wrong place.
It records each level's offset in the array view and deliberately leaves
its coordinate pointers unshifted for the consumer to apply - which its own
visitors do, per geometry, at array_view.c's VisitNativeLinestring and
VisitNativePolygon. NativeReader::setArray applies the innermost one to the
base pointers instead, which comes to the same thing and costs nothing per
piece. That was always what the compensation was doing; the comment saying
the library had it wrong was mine, and it was wrong. Reading those callers
rather than the setter alone is what turned up the ring-level gap above.

Tests, both confirmed failing before the fix:
- a Catch2 case building a polygon column with a decoy ring sliced off the
  front, which read the decoy (2 vertices) rather than the real ring (3)
- a Python case doing the same through pyarrow, which split to no pieces
  at all because the decoy ring has no area

Verified: 137 Python tests and 50 C++ cases (36,085 assertions); pieces
bit-identical against the golden arrays, since unsliced input takes the
same path; ru_maxrss flat across fourteen stream lifecycles; clean under
-Wall -Wextra -Wpedantic on gcc and clang.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SqdWshmD4AUHhrqeMh86GS

Author: claude

extension/src/geoarrow_arrays.cpp

@@ -187,11 +187,13 @@ void NativeReader::setArray(const ArrowArray *array) {
         std::string("Could not read a batch of geometries: ") + error.message);
   }
 
-  // geoarrow-c points its coordinate pointers past the innermost child's
-  // slice offset but not past the coordinate array's own - the fixed-size
-  // list's, or the struct's for separated coordinates - so an array sliced
-  // there would be read from the wrong place. A list's offsets count from
-  // the coordinate array's start, so that offset has to be applied.
+  // The innermost level needs the same shift offsetAt applies to the list
+  // levels, and geoarrow-c leaves it to the consumer in the same way: its
+  // coordinate pointers stop at the innermost double child's offset, and
+  // the coordinate array's own - the fixed-size list's, or the struct's for
+  // separated coordinates - is recorded in the array view for us to add.
+  // Its own visitors add it per geometry; applying it once to the base
+  // pointers here comes to the same thing and costs nothing per piece.
   const ArrowArray *coordinates = array;
   for (int32_t level = 0; level < view.n_offsets; level++) {
     coordinates = coordinates->children[0];
@@ -206,8 +208,13 @@ void NativeReader::setArray(const ArrowArray *array) {
 
 int64_t NativeReader::length() const { return view.length[0]; }
 
-/// Where element i of the list at `level` begins. geoarrow-c leaves the
-/// offsets buffer unshifted, so a slice's own start is added here.
+/// Where element i of the list at `level` begins.
+///
+/// An Arrow list's offsets are logical indices into its child, so the
+/// child's own slice offset has to be added before its offsets buffer is
+/// read in turn. geoarrow-c records each level's offset in the array view
+/// for exactly this, and every index into `offsets[level]` goes through
+/// here so that no level can be forgotten.
 int64_t NativeReader::offsetAt(int level, int64_t i) const {
   return view.offsets[level][view.offset[level] + i];
 }
@@ -262,8 +269,7 @@ void NativeReader::rings(int64_t i, std::vector<operations::CoordSpan> &out,
   const int64_t last = offsetAt(0, i + 1);
   if (contiguous()) {
     for (int64_t r = first; r < last; r++) {
-      // the polygon offsets already count from the ring array's own start
-      out.push_back(run(view.offsets[1][r], view.offsets[1][r + 1]));
+      out.push_back(run(offsetAt(1, r), offsetAt(1, r + 1)));
     }
     return;
   }
@@ -275,8 +281,8 @@ void NativeReader::rings(int64_t i, std::vector<operations::CoordSpan> &out,
     // the ring buffers keep their capacity from the previous polygon
     linestr &ring = scratch[used++];
     ring.clear();
-    const int64_t begin = view.offsets[1][r];
-    const int64_t end = view.offsets[1][r + 1];
+    const int64_t begin = offsetAt(1, r);
+    const int64_t end = offsetAt(1, r + 1);
     ring.reserve(static_cast<std::size_t>(end - begin));
     for (int64_t v = begin; v < end; v++) {
       ring.push_back(at(v));

extension/tests/tests_geoarrow.cpp

@@ -397,15 +397,107 @@ class NativeColumn {
   ArrowArray *array_children[1]{}, *vertices_children[1]{};
 };
 
+/// A geoarrow.polygon array built by hand, so that a test can put a slice
+/// offset on the *rings* array - a level pyarrow will not slice on its own
+class NativePolygonColumn {
+public:
+  /// `pad` decoy rings of two vertices each are written at the front and
+  /// then sliced off by giving the ring list an offset. The polygon offsets
+  /// count from the ring array's own logical start, so they still begin at
+  /// 0, and a reader that forgets the offset lands on the decoys.
+  NativePolygonColumn(
+      const std::vector<std::vector<std::pair<double, double>>> &rings_in,
+      int64_t pad) {
+    ArrowSchemaInit(schema.get());
+    REQUIRE(ArrowSchemaSetType(schema.get(), NANOARROW_TYPE_LIST) ==
+            NANOARROW_OK);
+    ArrowSchema *ring_field = schema->children[0];
+    REQUIRE(ArrowSchemaSetType(ring_field, NANOARROW_TYPE_LIST) ==
+            NANOARROW_OK);
+    REQUIRE(ArrowSchemaSetName(ring_field, "rings") == NANOARROW_OK);
+    ArrowSchema *vertex_field = ring_field->children[0];
+    REQUIRE(ArrowSchemaSetTypeFixedSize(
+                vertex_field, NANOARROW_TYPE_FIXED_SIZE_LIST, 2) ==
+            NANOARROW_OK);
+    REQUIRE(ArrowSchemaSetName(vertex_field, "vertices") == NANOARROW_OK);
+    REQUIRE(ArrowSchemaSetType(vertex_field->children[0],
+                               NANOARROW_TYPE_DOUBLE) == NANOARROW_OK);
+    REQUIRE(ArrowSchemaSetName(vertex_field->children[0], "xy") ==
+            NANOARROW_OK);
+
+    ring_offsets.push_back(0);
+    for (int64_t i = 0; i < pad; i++) {
+      xy.insert(xy.end(), {-999.0, -999.0, -999.0, -999.0});
+      ring_offsets.push_back(static_cast<int32_t>(ring_offsets.back() + 2));
+    }
+    for (const auto &ring : rings_in) {
+      for (const auto &point : ring) {
+        xy.push_back(point.first);
+        xy.push_back(point.second);
+      }
+      ring_offsets.push_back(
+          static_cast<int32_t>(ring_offsets.back() + ring.size()));
+    }
+    // one polygon holding every real ring, counted from the slice's start
+    polygon_offsets = {0, static_cast<int32_t>(rings_in.size())};
+
+    xy_child.length = static_cast<int64_t>(xy.size());
+    xy_child.n_buffers = 2;
+    xy_buffers[1] = xy.data();
+    xy_child.buffers = xy_buffers;
+    xy_child.null_count = 0;
+
+    vertices.length = static_cast<int64_t>(xy.size() / 2);
+    vertices.n_buffers = 1;
+    vertices.buffers = vertices_buffers;
+    vertices.n_children = 1;
+    vertices_children[0] = &xy_child;
+    vertices.children = vertices_children;
+    vertices.null_count = 0;
+
+    rings.length = static_cast<int64_t>(rings_in.size());
+    rings.offset = pad;
+    rings.n_buffers = 2;
+    rings_buffers[1] = ring_offsets.data();
+    rings.buffers = rings_buffers;
+    rings.n_children = 1;
+    rings_children[0] = &vertices;
+    rings.children = rings_children;
+    rings.null_count = 0;
+
+    array.length = 1;
+    array.n_buffers = 2;
+    array_buffers[1] = polygon_offsets.data();
+    array.buffers = array_buffers;
+    array.n_children = 1;
+    array_children[0] = &rings;
+    array.children = array_children;
+    array.null_count = 0;
+  }
+
+  const ArrowSchema *arrowSchema() const { return schema.get(); }
+  const ArrowArray *arrowArray() const { return &array; }
+
+private:
+  nanoarrow::UniqueSchema schema;
+  std::vector<double> xy;
+  std::vector<int32_t> ring_offsets, polygon_offsets;
+  ArrowArray array{}, rings{}, vertices{}, xy_child{};
+  const void *array_buffers[2]{}, *rings_buffers[2]{}, *vertices_buffers[1]{},
+      *xy_buffers[2]{};
+  ArrowArray *array_children[1]{}, *rings_children[1]{},
+      *vertices_children[1]{};
+};
+
 } // namespace
 
 TEST_CASE("A sliced coordinate array is read from the right place",
           "[geoarrow]") {
-  // The list's offsets count from the coordinate array's own start, so a
-  // fixed-size list carrying a slice offset of its own has to be accounted
-  // for. geoarrow-c's array view applies only the innermost double child's
-  // offset, not the fixed-size list's, so this is compensated on our side -
-  // and this is the case that says whether it still is.
+  // A list's offsets are logical indices into its child, so the child's own
+  // slice offset has to be added to them. geoarrow-c records each level's
+  // offset in the array view and leaves its coordinate pointers unshifted
+  // for the consumer to apply; NativeReader::setArray applies the innermost
+  // one to the pointers themselves, and this is the case that says so.
   NativeReader reader;
   NativeColumn column({{{0.5, 0.5}, {1.5, 1.5}, {2.5, 2.5}}}, /*pad=*/2);
   reader.init(column.arrowSchema(), GeometryType::linestring);
@@ -419,3 +511,25 @@ TEST_CASE("A sliced coordinate array is read from the right place",
   REQUIRE(vertices[2].x == 2.5);
   REQUIRE(vertices[2].y == 2.5);
 }
+
+TEST_CASE("A sliced ring array is read from the right rings", "[geoarrow]") {
+  // The same rule one level up: a polygon's offsets index the ring array
+  // logically, so the ring array's own slice offset has to be added before
+  // its offsets buffer is read. Forgetting it lands on the decoy rings.
+  NativeReader reader;
+  NativePolygonColumn column({{{0.5, 0.5}, {2.5, 0.5}, {2.5, 2.5}}}, /*pad=*/1);
+  reader.init(column.arrowSchema(), GeometryType::polygon);
+  reader.setArray(column.arrowArray());
+
+  REQUIRE(reader.length() == 1);
+  std::vector<snail::operations::CoordSpan> rings;
+  std::vector<snail::geoarrow::linestr> scratch;
+  reader.rings(0, rings, scratch);
+
+  REQUIRE(rings.size() == 1);
+  REQUIRE(rings[0].size() == 3);
+  REQUIRE(rings[0][0].x == 0.5);
+  REQUIRE(rings[0][0].y == 0.5);
+  REQUIRE(rings[0][2].x == 2.5);
+  REQUIRE(rings[0][2].y == 2.5);
+}

tests/test_geoarrow.py

@@ -406,6 +406,32 @@ def test_sliced_arrays(self, many_linestrings, polygons):
 
             assert_same_pieces(actual, expected)
 
+    def test_polygon_over_a_sliced_ring_array(self):
+        """The slice can sit on an inner level too. A polygon's offsets index
+        its ring array logically, so a ring array that is itself a slice
+        shifts which rings a polygon is made of - and nothing in the outer
+        array says so."""
+
+        def polygon_of(xy, ring_offsets, decoy_rings):
+            rings = pa.ListArray.from_arrays(
+                pa.array(ring_offsets, type=pa.int32()),
+                pa.FixedSizeListArray.from_arrays(pa.array(xy, type=pa.float64()), 2),
+            ).slice(decoy_rings)
+            return pa.ListArray.from_arrays(
+                pa.array([0, len(rings)], type=pa.int32()), rings
+            )
+
+        ring = [0.5, 0.5, 2.5, 0.5, 2.5, 2.5]
+        sliced = polygon_of([-9.0, -9.0, -9.0, -9.0] + ring, [0, 2, 5], 1)
+        assert sliced.values.offset == 1
+        plain = polygon_of(ring, [0, 3], 0)
+
+        actual = geometry_of(core_split_polygons(sliced, NROWS, NCOLS, TRANSFORM))
+        expected = geometry_of(core_split_polygons(plain, NROWS, NCOLS, TRANSFORM))
+
+        assert len(actual) > 0
+        assert_same_pieces(actual, expected)
+
 
 class TestMixed:
     """split_geometries takes each geometry on its own terms, and gives the