Remove owned slice (#891)

Closes #889 When I was originally implementing scalars, I didn't really understand how cheap `Arc`s are. I was thinking that for scalars we don't necessarily want to hold on to the full geometry array just to keep one scalar around. While that's sometimes true, we also don't necessarily want to make a full memcopy of the scalar whenever we access it. Sometimes it's better to just clone the `Arc` and maintain the geometry offset. `owned_slice` was an implementation of this full memcopy for the given offset and length. It was never fully implemented and at this point it's better to just remove it.
geoarrow · Dec 3, 2024 · 5f4421c · 5f4421c
1 parent c0a0fee
commit 5f4421c
Show file tree

Hide file tree

Showing 26 changed files with 20 additions and 540 deletions.
diff --git a/rust/geoarrow/src/array/binary/array.rs b/rust/geoarrow/src/array/binary/array.rs
@@ -7,14 +7,13 @@ use crate::array::{CoordType, WKBBuilder};
 use crate::datatypes::{NativeType, SerializedType};
 use crate::error::{GeoArrowError, Result};
 use crate::scalar::WKB;
-use geo_traits::GeometryTrait;
-// use crate::util::{owned_slice_offsets, owned_slice_validity};
 use crate::trait_::{ArrayAccessor, ArrayBase, IntoArrow, SerializedArray};
 use arrow::array::AsArray;
 use arrow_array::OffsetSizeTrait;
 use arrow_array::{Array, BinaryArray, GenericBinaryArray, LargeBinaryArray};
 use arrow_buffer::NullBuffer;
 use arrow_schema::{DataType, Field};
+use geo_traits::GeometryTrait;
 
 /// An immutable array of WKB geometries using GeoArrow's in-memory representation.
 ///
@@ -94,31 +93,6 @@ impl<O: OffsetSizeTrait> WKBArray<O> {
         }
     }
 
-    pub fn owned_slice(&self, _offset: usize, _length: usize) -> Self {
-        todo!()
-        // assert!(
-        //     offset + length <= self.len(),
-        //     "offset + length may not exceed length of array"
-        // );
-        // assert!(length >= 1, "length must be at least 1");
-
-        // // Find the start and end of the ring offsets
-        // let (start_idx, _) = self.array.offsets().start_end(offset);
-        // let (_, end_idx) = self.array.offsets().start_end(offset + length - 1);
-
-        // let new_offsets = owned_slice_offsets(self.array.offsets(), offset, length);
-
-        // let mut values = self.array.slice(start_idx, end_idx - start_idx);
-
-        // let validity = owned_slice_validity(self.array.nulls(), offset, length);
-
-        // Self::new(GenericBinaryArray::new(
-        //     new_offsets,
-        //     values.as_slice().to_vec().into(),
-        //     validity,
-        // ))
-    }
-
     pub fn with_metadata(&self, metadata: Arc<ArrayMetadata>) -> Self {
         let mut arr = self.clone();
         arr.metadata = metadata;

diff --git a/rust/geoarrow/src/array/coord/combined/array.rs b/rust/geoarrow/src/array/coord/combined/array.rs
@@ -37,15 +37,6 @@ impl CoordBuffer {
         }
     }
 
-    pub fn owned_slice(&self, offset: usize, length: usize) -> Self {
-        match self {
-            CoordBuffer::Interleaved(cb) => {
-                CoordBuffer::Interleaved(cb.owned_slice(offset, length))
-            }
-            CoordBuffer::Separated(cb) => CoordBuffer::Separated(cb.owned_slice(offset, length)),
-        }
-    }
-
     pub fn coord_type(&self) -> CoordType {
         match self {
             CoordBuffer::Interleaved(cb) => cb.coord_type(),

diff --git a/rust/geoarrow/src/array/coord/interleaved/array.rs b/rust/geoarrow/src/array/coord/interleaved/array.rs
@@ -89,11 +89,6 @@ impl InterleavedCoordBuffer {
         }
     }
 
-    pub fn owned_slice(&self, offset: usize, length: usize) -> Self {
-        let buffer = self.slice(offset, length);
-        Self::new(buffer.coords.to_vec().into(), self.dim)
-    }
-
     pub fn into_array_ref(self) -> Arc<dyn Array> {
         Arc::new(self.into_arrow())
     }

diff --git a/rust/geoarrow/src/array/coord/separated/array.rs b/rust/geoarrow/src/array/coord/separated/array.rs
@@ -143,24 +143,6 @@ impl SeparatedCoordBuffer {
         }
     }
 
-    pub fn owned_slice(&self, offset: usize, length: usize) -> Self {
-        assert!(
-            offset + length <= self.len(),
-            "offset + length may not exceed length of array"
-        );
-
-        // Initialize array with existing buffers, then overwrite them
-        let mut sliced_buffers = self.buffers.clone();
-        for (i, buffer) in self.buffers.iter().enumerate() {
-            sliced_buffers[i] = buffer.slice(offset, length).to_vec().into();
-        }
-
-        Self {
-            buffers: sliced_buffers,
-            dim: self.dim,
-        }
-    }
-
     pub fn storage_type(&self) -> DataType {
         coord_type_to_data_type(CoordType::Separated, self.dim)
     }

diff --git a/rust/geoarrow/src/array/dynamic.rs b/rust/geoarrow/src/array/dynamic.rs
@@ -133,10 +133,6 @@ impl NativeArray for NativeArrayDyn {
     fn slice(&self, offset: usize, length: usize) -> Arc<dyn NativeArray> {
         self.0.slice(offset, length)
     }
-
-    fn owned_slice(&self, offset: usize, length: usize) -> Arc<dyn NativeArray> {
-        self.0.owned_slice(offset, length)
-    }
 }
 
 impl Display for NativeArrayDyn {

diff --git a/rust/geoarrow/src/array/geometry/array.rs b/rust/geoarrow/src/array/geometry/array.rs
@@ -230,26 +230,6 @@ impl<O: OffsetSizeTrait> GeometryArraySelfMethods for GeometryArray<O> {
             GeometryArray::Rect(arr) => GeometryArray::Rect(arr.slice(offset, length)),
         }
     }
-
-    fn owned_slice(&self, offset: usize, length: usize) -> Self {
-        match self {
-            GeometryArray::Point(arr) => GeometryArray::Point(arr.owned_slice(offset, length)),
-            GeometryArray::LineString(arr) => {
-                GeometryArray::LineString(arr.owned_slice(offset, length))
-            }
-            GeometryArray::Polygon(arr) => GeometryArray::Polygon(arr.owned_slice(offset, length)),
-            GeometryArray::MultiPoint(arr) => {
-                GeometryArray::MultiPoint(arr.owned_slice(offset, length))
-            }
-            GeometryArray::MultiLineString(arr) => {
-                GeometryArray::MultiLineString(arr.owned_slice(offset, length))
-            }
-            GeometryArray::MultiPolygon(arr) => {
-                GeometryArray::MultiPolygon(arr.owned_slice(offset, length))
-            }
-            GeometryArray::Rect(arr) => GeometryArray::Rect(arr.owned_slice(offset, length)),
-        }
-    }
 }
 
 impl<'a, O: OffsetSizeTrait> ArrayAccessor<'a> for GeometryArray<O> {

diff --git a/rust/geoarrow/src/array/geometrycollection/array.rs b/rust/geoarrow/src/array/geometrycollection/array.rs
@@ -115,10 +115,6 @@ impl GeometryCollectionArray {
         }
     }
 
-    pub fn owned_slice(&self, _offset: usize, _length: usize) -> Self {
-        todo!()
-    }
-
     pub fn to_coord_type(&self, coord_type: CoordType) -> Self {
         self.clone().into_coord_type(coord_type)
     }
@@ -204,10 +200,6 @@ impl NativeArray for GeometryCollectionArray {
     fn slice(&self, offset: usize, length: usize) -> Arc<dyn NativeArray> {
         Arc::new(self.slice(offset, length))
     }
-
-    fn owned_slice(&self, offset: usize, length: usize) -> Arc<dyn NativeArray> {
-        Arc::new(self.owned_slice(offset, length))
-    }
 }
 
 impl GeometryArraySelfMethods for GeometryCollectionArray {

diff --git a/rust/geoarrow/src/array/linestring/array.rs b/rust/geoarrow/src/array/linestring/array.rs
@@ -14,7 +14,6 @@ use crate::datatypes::{Dimension, NativeType};
 use crate::error::{GeoArrowError, Result};
 use crate::scalar::{Geometry, LineString};
 use crate::trait_::{ArrayAccessor, GeometryArraySelfMethods, IntoArrow, NativeGeometryAccessor};
-use crate::util::{owned_slice_offsets, owned_slice_validity};
 use crate::{ArrayBase, NativeArray};
 use arrow::array::AsArray;
 use arrow_array::{Array, ArrayRef, GenericListArray, OffsetSizeTrait};
@@ -172,28 +171,6 @@ impl LineStringArray {
         }
     }
 
-    pub fn owned_slice(&self, offset: usize, length: usize) -> Self {
-        assert!(
-            offset + length <= self.len(),
-            "offset + length may not exceed length of array"
-        );
-        assert!(length >= 1, "length must be at least 1");
-
-        // Find the start and end of the coord buffer
-        let (start_coord_idx, _) = self.geom_offsets.start_end(offset);
-        let (_, end_coord_idx) = self.geom_offsets.start_end(offset + length - 1);
-
-        let geom_offsets = owned_slice_offsets(&self.geom_offsets, offset, length);
-
-        let coords = self
-            .coords
-            .owned_slice(start_coord_idx, end_coord_idx - start_coord_idx);
-
-        let validity = owned_slice_validity(self.nulls(), offset, length);
-
-        Self::new(coords, geom_offsets, validity, self.metadata())
-    }
-
     pub fn to_coord_type(&self, coord_type: CoordType) -> Self {
         self.clone().into_coord_type(coord_type)
     }
@@ -287,10 +264,6 @@ impl NativeArray for LineStringArray {
     fn slice(&self, offset: usize, length: usize) -> Arc<dyn NativeArray> {
         Arc::new(self.slice(offset, length))
     }
-
-    fn owned_slice(&self, offset: usize, length: usize) -> Arc<dyn NativeArray> {
-        Arc::new(self.owned_slice(offset, length))
-    }
 }
 
 impl GeometryArraySelfMethods for LineStringArray {
@@ -586,20 +559,6 @@ mod test {
         assert_eq!(sliced.get_as_geo(0), Some(ls1()));
     }
 
-    #[test]
-    fn owned_slice() {
-        let arr: LineStringArray = (vec![ls0(), ls1()].as_slice(), Dimension::XY).into();
-        let sliced = arr.owned_slice(1, 1);
-
-        // assert!(
-        //     !sliced.geom_offsets.buffer().is_sliced(),
-        //     "underlying offsets should not be sliced"
-        // );
-        assert_eq!(arr.len(), 2);
-        assert_eq!(sliced.len(), 1);
-        assert_eq!(sliced.get_as_geo(0), Some(ls1()));
-    }
-
     #[test]
     fn parse_wkb_geoarrow_interleaved_example() {
         let linestring_arr = example_linestring_interleaved();

diff --git a/rust/geoarrow/src/array/mixed/array.rs b/rust/geoarrow/src/array/mixed/array.rs
@@ -281,10 +281,6 @@ impl MixedGeometryArray {
         }
     }
 
-    pub fn owned_slice(&self, _offset: usize, _length: usize) -> Self {
-        todo!()
-    }
-
     pub fn to_coord_type(&self, coord_type: CoordType) -> Self {
         self.clone().into_coord_type(coord_type)
     }
@@ -376,10 +372,6 @@ impl NativeArray for MixedGeometryArray {
     fn slice(&self, offset: usize, length: usize) -> Arc<dyn NativeArray> {
         Arc::new(self.slice(offset, length))
     }
-
-    fn owned_slice(&self, offset: usize, length: usize) -> Arc<dyn NativeArray> {
-        Arc::new(self.owned_slice(offset, length))
-    }
 }
 
 impl GeometryArraySelfMethods for MixedGeometryArray {

diff --git a/rust/geoarrow/src/array/multilinestring/array.rs b/rust/geoarrow/src/array/multilinestring/array.rs
@@ -12,7 +12,6 @@ use crate::datatypes::{Dimension, NativeType};
 use crate::error::{GeoArrowError, Result};
 use crate::scalar::{Geometry, MultiLineString};
 use crate::trait_::{ArrayAccessor, GeometryArraySelfMethods, IntoArrow, NativeGeometryAccessor};
-use crate::util::{owned_slice_offsets, owned_slice_validity};
 use crate::{ArrayBase, NativeArray};
 use arrow::array::AsArray;
 use arrow_array::{Array, GenericListArray, OffsetSizeTrait};
@@ -185,43 +184,6 @@ impl MultiLineStringArray {
         }
     }
 
-    pub fn owned_slice(&self, offset: usize, length: usize) -> Self {
-        assert!(
-            offset + length <= self.len(),
-            "offset + length may not exceed length of array"
-        );
-        assert!(length >= 1, "length must be at least 1");
-
-        // Find the start and end of the ring offsets
-        let (start_ring_idx, _) = self.geom_offsets.start_end(offset);
-        let (_, end_ring_idx) = self.geom_offsets.start_end(offset + length - 1);
-
-        // Find the start and end of the coord buffer
-        let (start_coord_idx, _) = self.ring_offsets.start_end(start_ring_idx);
-        let (_, end_coord_idx) = self.ring_offsets.start_end(end_ring_idx - 1);
-
-        // Slice the geom_offsets
-        let geom_offsets = owned_slice_offsets(&self.geom_offsets, offset, length);
-        let ring_offsets = owned_slice_offsets(
-            &self.ring_offsets,
-            start_ring_idx,
-            end_ring_idx - start_ring_idx,
-        );
-        let coords = self
-            .coords
-            .owned_slice(start_coord_idx, end_coord_idx - start_coord_idx);
-
-        let validity = owned_slice_validity(self.nulls(), offset, length);
-
-        Self::new(
-            coords,
-            geom_offsets,
-            ring_offsets,
-            validity,
-            self.metadata(),
-        )
-    }
-
     pub fn to_coord_type(&self, coord_type: CoordType) -> Self {
         self.clone().into_coord_type(coord_type)
     }
@@ -307,10 +269,6 @@ impl NativeArray for MultiLineStringArray {
     fn slice(&self, offset: usize, length: usize) -> Arc<dyn NativeArray> {
         Arc::new(self.slice(offset, length))
     }
-
-    fn owned_slice(&self, offset: usize, length: usize) -> Arc<dyn NativeArray> {
-        Arc::new(self.owned_slice(offset, length))
-    }
 }
 
 impl GeometryArraySelfMethods for MultiLineStringArray {
@@ -621,24 +579,6 @@ mod test {
         assert_eq!(sliced.get_as_geo(0), Some(ml1()));
     }
 
-    #[test]
-    fn owned_slice() {
-        let arr: MultiLineStringArray = (vec![ml0(), ml1()].as_slice(), Dimension::XY).into();
-        let sliced = arr.owned_slice(1, 1);
-
-        // assert!(
-        //     !sliced.geom_offsets.buffer().is_sliced(),
-        //     "underlying offsets should not be sliced"
-        // );
-        assert_eq!(arr.len(), 2);
-        assert_eq!(sliced.len(), 1);
-        assert_eq!(sliced.get_as_geo(0), Some(ml1()));
-
-        // // Offset is 0 because it's copied to an owned buffer
-        // assert_eq!(*sliced.geom_offsets.first(), 0);
-        // assert_eq!(*sliced.ring_offsets.first(), 0);
-    }
-
     #[test]
     fn parse_wkb_geoarrow_interleaved_example() {
         let geom_arr = example_multilinestring_interleaved();