Skip to content

Commit

Permalink
Remove Cow around scalar buffers (#720)
Browse files Browse the repository at this point in the history
Originally scalars were always references onto external buffers. Then in
#119 that was changed to try
to better support bindings that can't have lifetime references. (E.g.
neither python nor JS allow exports with lifetime references, because
they can't verify when data will still be valid in those environments).

But over time it turned out that we really needed fully separate structs
for these (e.g. `OwnedPoint`) that don't have their own lifetime
parameters. This means that we never actually needed to make `Point`
_itself_ owned. We need to have separate structs anyways for the
bindings.

Having Cow on the scalars only serves to make coordinate access slower,
because there's an indirection on access to the coordinate buffer.

### Change list

- Remove `Cow` wrappers on the internal buffers on scalars.

Closes #472

Closes #449
  • Loading branch information
kylebarron authored Aug 26, 2024
1 parent 470ac60 commit cae9d35
Show file tree
Hide file tree
Showing 32 changed files with 441 additions and 713 deletions.
489 changes: 287 additions & 202 deletions js/Cargo.lock

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions js/src/scalar/linestring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ use wasm_bindgen::prelude::*;
#[wasm_bindgen]
pub struct LineString(pub(crate) OwnedLineString<i32, 2>);

impl<'a> From<LineString> for geoarrow::scalar::LineString<'a, i32, 2> {
fn from(value: LineString) -> Self {
value.0.into()
impl<'a> From<&'a LineString> for geoarrow::scalar::LineString<'a, i32, 2> {
fn from(value: &'a LineString) -> Self {
(&value.0).into()
}
}

Expand Down
6 changes: 3 additions & 3 deletions js/src/scalar/multilinestring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ use wasm_bindgen::prelude::*;
#[wasm_bindgen]
pub struct MultiLineString(pub(crate) OwnedMultiLineString<i32, 2>);

impl<'a> From<MultiLineString> for geoarrow::scalar::MultiLineString<'a, i32, 2> {
fn from(value: MultiLineString) -> Self {
value.0.into()
impl<'a> From<&'a MultiLineString> for geoarrow::scalar::MultiLineString<'a, i32, 2> {
fn from(value: &'a MultiLineString) -> Self {
(&value.0).into()
}
}

Expand Down
6 changes: 3 additions & 3 deletions js/src/scalar/multipoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ use wasm_bindgen::prelude::*;
#[wasm_bindgen]
pub struct MultiPoint(pub(crate) OwnedMultiPoint<i32, 2>);

impl<'a> From<MultiPoint> for geoarrow::scalar::MultiPoint<'a, i32, 2> {
fn from(value: MultiPoint) -> Self {
value.0.into()
impl<'a> From<&'a MultiPoint> for geoarrow::scalar::MultiPoint<'a, i32, 2> {
fn from(value: &'a MultiPoint) -> Self {
(&value.0).into()
}
}

Expand Down
6 changes: 3 additions & 3 deletions js/src/scalar/multipolygon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ use wasm_bindgen::prelude::*;
#[wasm_bindgen]
pub struct MultiPolygon(pub(crate) OwnedMultiPolygon<i32, 2>);

impl<'a> From<MultiPolygon> for geoarrow::scalar::MultiPolygon<'a, i32, 2> {
fn from(value: MultiPolygon) -> Self {
value.0.into()
impl<'a> From<&'a MultiPolygon> for geoarrow::scalar::MultiPolygon<'a, i32, 2> {
fn from(value: &'a MultiPolygon) -> Self {
(&value.0).into()
}
}

Expand Down
6 changes: 3 additions & 3 deletions js/src/scalar/point.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ use wasm_bindgen::prelude::*;
#[wasm_bindgen]
pub struct Point(pub(crate) OwnedPoint<2>);

impl<'a> From<Point> for geoarrow::scalar::Point<'a, 2> {
fn from(value: Point) -> Self {
value.0.into()
impl<'a> From<&'a Point> for geoarrow::scalar::Point<'a, 2> {
fn from(value: &'a Point) -> Self {
(&value.0).into()
}
}

Expand Down
6 changes: 3 additions & 3 deletions js/src/scalar/polygon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ use wasm_bindgen::prelude::*;
#[wasm_bindgen]
pub struct Polygon(pub(crate) OwnedPolygon<i32, 2>);

impl<'a> From<Polygon> for geoarrow::scalar::Polygon<'a, i32, 2> {
fn from(value: Polygon) -> Self {
value.0.into()
impl<'a> From<&'a Polygon> for geoarrow::scalar::Polygon<'a, i32, 2> {
fn from(value: &'a Polygon) -> Self {
(&value.0).into()
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/array/binary/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ impl<'a, O: OffsetSizeTrait> GeometryArrayAccessor<'a> for WKBArray<O> {
type ItemGeo = geo::Geometry;

unsafe fn value_unchecked(&'a self, index: usize) -> Self::Item {
WKB::new_borrowed(&self.array, index)
WKB::new(&self.array, index)
}
}

Expand Down
6 changes: 5 additions & 1 deletion src/array/linestring/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,10 @@ impl<O: OffsetSizeTrait, const D: usize> LineStringArray<O, D> {
&self.coords
}

pub fn into_inner(self) -> (CoordBuffer<D>, OffsetBuffer<O>, Option<NullBuffer>) {
(self.coords, self.geom_offsets, self.validity)
}

pub fn geom_offsets(&self) -> &OffsetBuffer<O> {
&self.geom_offsets
}
Expand Down Expand Up @@ -288,7 +292,7 @@ impl<'a, O: OffsetSizeTrait, const D: usize> GeometryArrayAccessor<'a> for LineS
type ItemGeo = geo::LineString;

unsafe fn value_unchecked(&'a self, index: usize) -> Self::Item {
LineString::new_borrowed(&self.coords, &self.geom_offsets, index)
LineString::new(&self.coords, &self.geom_offsets, index)
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/array/multilinestring/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ impl<'a, O: OffsetSizeTrait, const D: usize> GeometryArrayAccessor<'a>
type ItemGeo = geo::MultiLineString;

unsafe fn value_unchecked(&'a self, index: usize) -> Self::Item {
MultiLineString::new_borrowed(&self.coords, &self.geom_offsets, &self.ring_offsets, index)
MultiLineString::new(&self.coords, &self.geom_offsets, &self.ring_offsets, index)
}
}

Expand Down
6 changes: 5 additions & 1 deletion src/array/multipoint/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,10 @@ impl<O: OffsetSizeTrait, const D: usize> MultiPointArray<O, D> {
&self.coords
}

pub fn into_inner(self) -> (CoordBuffer<D>, OffsetBuffer<O>, Option<NullBuffer>) {
(self.coords, self.geom_offsets, self.validity)
}

pub fn geom_offsets(&self) -> &OffsetBuffer<O> {
&self.geom_offsets
}
Expand Down Expand Up @@ -278,7 +282,7 @@ impl<'a, O: OffsetSizeTrait, const D: usize> GeometryArrayAccessor<'a> for Multi
type ItemGeo = geo::MultiPoint;

unsafe fn value_unchecked(&'a self, index: usize) -> Self::Item {
MultiPoint::new_borrowed(&self.coords, &self.geom_offsets, index)
MultiPoint::new(&self.coords, &self.geom_offsets, index)
}
}

Expand Down
18 changes: 17 additions & 1 deletion src/array/multipolygon/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,22 @@ impl<O: OffsetSizeTrait, const D: usize> MultiPolygonArray<O, D> {
&self.coords
}

pub fn into_inner(
self,
) -> (
CoordBuffer<D>,
OffsetBuffer<O>,
OffsetBuffer<O>,
OffsetBuffer<O>,
) {
(
self.coords,
self.geom_offsets,
self.polygon_offsets,
self.ring_offsets,
)
}

pub fn geom_offsets(&self) -> &OffsetBuffer<O> {
&self.geom_offsets
}
Expand Down Expand Up @@ -375,7 +391,7 @@ impl<'a, O: OffsetSizeTrait, const D: usize> GeometryArrayAccessor<'a> for Multi
type ItemGeo = geo::MultiPolygon;

unsafe fn value_unchecked(&'a self, index: usize) -> Self::Item {
MultiPolygon::new_borrowed(
MultiPolygon::new(
&self.coords,
&self.geom_offsets,
&self.polygon_offsets,
Expand Down
2 changes: 1 addition & 1 deletion src/array/point/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ impl<'a, const D: usize> GeometryArrayAccessor<'a> for PointArray<D> {
type ItemGeo = geo::Point;

unsafe fn value_unchecked(&'a self, index: usize) -> Self::Item {
Point::new_borrowed(&self.coords, index)
Point::new(&self.coords, index)
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/array/polygon/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ impl<'a, O: OffsetSizeTrait, const D: usize> GeometryArrayAccessor<'a> for Polyg
type ItemGeo = geo::Polygon;

unsafe fn value_unchecked(&'a self, index: usize) -> Self::Item {
Polygon::new_borrowed(&self.coords, &self.geom_offsets, &self.ring_offsets, index)
Polygon::new(&self.coords, &self.geom_offsets, &self.ring_offsets, index)
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/array/rect/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ impl<'a, const D: usize> GeometryArrayAccessor<'a> for RectArray<D> {
type ItemGeo = geo::Rect;

unsafe fn value_unchecked(&'a self, index: usize) -> Self::Item {
Rect::new_borrowed(&self.lower, &self.upper, index)
Rect::new(&self.lower, &self.upper, index)
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/io/geos/scalar/linestring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ impl<'a, O: OffsetSizeTrait, const D: usize> TryFrom<&'a LineString<'_, O, D>> f
) -> std::result::Result<geos::Geometry, geos::Error> {
let (start, end) = value.geom_offsets.start_end(value.geom_index);

let sliced_coords = value.coords.clone().to_mut().slice(start, end - start);
let sliced_coords = value.coords.clone().slice(start, end - start);

geos::Geometry::create_line_string(sliced_coords.try_into()?)
}
Expand All @@ -34,7 +34,7 @@ impl<O: OffsetSizeTrait, const D: usize> LineString<'_, O, D> {
pub fn to_geos_linear_ring(&self) -> std::result::Result<geos::Geometry, geos::Error> {
let (start, end) = self.geom_offsets.start_end(self.geom_index);

let sliced_coords = self.coords.clone().to_mut().slice(start, end - start);
let sliced_coords = self.coords.clone().slice(start, end - start);

geos::Geometry::create_linear_ring(sliced_coords.try_into()?)
}
Expand Down
8 changes: 1 addition & 7 deletions src/scalar/binary/owned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,9 @@ impl<O: OffsetSizeTrait> OwnedWKB<O> {
}
}

impl<'a, O: OffsetSizeTrait> From<OwnedWKB<O>> for WKB<'a, O> {
fn from(value: OwnedWKB<O>) -> Self {
Self::new_owned(value.arr, value.geom_index)
}
}

impl<'a, O: OffsetSizeTrait> From<&'a OwnedWKB<O>> for WKB<'a, O> {
fn from(value: &'a OwnedWKB<O>) -> Self {
Self::new_borrowed(&value.arr, value.geom_index)
Self::new(&value.arr, value.geom_index)
}
}

Expand Down
21 changes: 3 additions & 18 deletions src/scalar/binary/scalar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,38 +3,23 @@ use crate::trait_::GeometryScalarTrait;
use arrow_array::{GenericBinaryArray, OffsetSizeTrait};
use geo::BoundingRect;
use rstar::{RTreeObject, AABB};
use std::borrow::Cow;

/// An Arrow equivalent of a Point
#[derive(Debug, Clone)]
pub struct WKB<'a, O: OffsetSizeTrait> {
pub(crate) arr: Cow<'a, GenericBinaryArray<O>>,
pub(crate) arr: &'a GenericBinaryArray<O>,
pub(crate) geom_index: usize,
}

impl<'a, O: OffsetSizeTrait> WKB<'a, O> {
pub fn new(arr: Cow<'a, GenericBinaryArray<O>>, geom_index: usize) -> Self {
pub fn new(arr: &'a GenericBinaryArray<O>, geom_index: usize) -> Self {
Self { arr, geom_index }
}

pub fn new_borrowed(arr: &'a GenericBinaryArray<O>, geom_index: usize) -> Self {
Self {
arr: Cow::Borrowed(arr),
geom_index,
}
}

pub fn new_owned(arr: GenericBinaryArray<O>, geom_index: usize) -> Self {
Self {
arr: Cow::Owned(arr),
geom_index,
}
}

pub fn into_owned_inner(self) -> (GenericBinaryArray<O>, usize) {
// TODO: hard slice?
// let owned = self.into_owned();
(self.arr.into_owned(), self.geom_index)
(self.arr.clone(), self.geom_index)
}
}

Expand Down
10 changes: 2 additions & 8 deletions src/scalar/linestring/owned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,23 +25,17 @@ impl<O: OffsetSizeTrait, const D: usize> OwnedLineString<O, D> {
}
}

impl<'a, O: OffsetSizeTrait, const D: usize> From<OwnedLineString<O, D>> for LineString<'a, O, D> {
fn from(value: OwnedLineString<O, D>) -> Self {
Self::new_owned(value.coords, value.geom_offsets, value.geom_index)
}
}

impl<'a, O: OffsetSizeTrait, const D: usize> From<&'a OwnedLineString<O, D>>
for LineString<'a, O, D>
{
fn from(value: &'a OwnedLineString<O, D>) -> Self {
Self::new_borrowed(&value.coords, &value.geom_offsets, value.geom_index)
Self::new(&value.coords, &value.geom_offsets, value.geom_index)
}
}

impl<O: OffsetSizeTrait> From<OwnedLineString<O, 2>> for geo::LineString {
fn from(value: OwnedLineString<O, 2>) -> Self {
let geom = LineString::from(value);
let geom = LineString::from(&value);
geom.into()
}
}
Expand Down
54 changes: 11 additions & 43 deletions src/scalar/linestring/scalar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,14 @@ use crate::trait_::{GeometryArraySelfMethods, GeometryScalarTrait};
use arrow_array::OffsetSizeTrait;
use arrow_buffer::OffsetBuffer;
use rstar::{RTreeObject, AABB};
use std::borrow::Cow;

/// An Arrow equivalent of a LineString
#[derive(Debug, Clone)]
pub struct LineString<'a, O: OffsetSizeTrait, const D: usize> {
pub(crate) coords: Cow<'a, CoordBuffer<D>>,
pub(crate) coords: &'a CoordBuffer<D>,

/// Offsets into the coordinate array where each geometry starts
pub(crate) geom_offsets: Cow<'a, OffsetBuffer<O>>,
pub(crate) geom_offsets: &'a OffsetBuffer<O>,

pub(crate) geom_index: usize,

Expand All @@ -26,8 +25,8 @@ pub struct LineString<'a, O: OffsetSizeTrait, const D: usize> {

impl<'a, O: OffsetSizeTrait, const D: usize> LineString<'a, O, D> {
pub fn new(
coords: Cow<'a, CoordBuffer<D>>,
geom_offsets: Cow<'a, OffsetBuffer<O>>,
coords: &'a CoordBuffer<D>,
geom_offsets: &'a OffsetBuffer<O>,
geom_index: usize,
) -> Self {
let (start_offset, _) = geom_offsets.start_end(geom_index);
Expand All @@ -39,47 +38,16 @@ impl<'a, O: OffsetSizeTrait, const D: usize> LineString<'a, O, D> {
}
}

pub fn new_borrowed(
coords: &'a CoordBuffer<D>,
geom_offsets: &'a OffsetBuffer<O>,
geom_index: usize,
) -> Self {
Self::new(
Cow::Borrowed(coords),
Cow::Borrowed(geom_offsets),
geom_index,
)
}

pub fn new_owned(
coords: CoordBuffer<D>,
geom_offsets: OffsetBuffer<O>,
geom_index: usize,
) -> Self {
Self::new(Cow::Owned(coords), Cow::Owned(geom_offsets), geom_index)
}

/// Extracts the owned data.
///
/// Clones the data if it is not already owned.
pub fn into_owned(self) -> Self {
pub fn into_owned_inner(self) -> (CoordBuffer<D>, OffsetBuffer<O>, usize) {
let arr = LineStringArray::new(
self.coords.into_owned(),
self.geom_offsets.into_owned(),
self.coords.clone(),
self.geom_offsets.clone(),
None,
Default::default(),
);
let sliced_arr = arr.owned_slice(self.geom_index, 1);
Self::new_owned(sliced_arr.coords, sliced_arr.geom_offsets, 0)
}

pub fn into_owned_inner(self) -> (CoordBuffer<D>, OffsetBuffer<O>, usize) {
let owned = self.into_owned();
(
owned.coords.into_owned(),
owned.geom_offsets.into_owned(),
owned.geom_index,
)
let (coords, geom_offsets, _validity) = sliced_arr.into_inner();
(coords, geom_offsets, 0)
}
}

Expand Down Expand Up @@ -114,7 +82,7 @@ impl<'a, O: OffsetSizeTrait, const D: usize> LineStringTrait for LineString<'a,
}

unsafe fn coord_unchecked(&self, i: usize) -> Self::ItemType<'_> {
Point::new(self.coords.clone(), self.start_offset + i)
Point::new(self.coords, self.start_offset + i)
}
}

Expand All @@ -132,7 +100,7 @@ impl<'a, O: OffsetSizeTrait, const D: usize> LineStringTrait for &'a LineString<
}

unsafe fn coord_unchecked(&self, i: usize) -> Self::ItemType<'_> {
Point::new(self.coords.clone(), self.start_offset + i)
Point::new(self.coords, self.start_offset + i)
}
}

Expand Down
Loading

0 comments on commit cae9d35

Please sign in to comment.