diff --git a/Cargo.lock b/Cargo.lock index 0d3665511c2..c823632cd77 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10067,6 +10067,7 @@ name = "vortex-geo" version = "0.1.0" dependencies = [ "arrow-array 58.4.0", + "arrow-buffer 58.4.0", "arrow-schema 58.4.0", "codspeed-divan-compat", "geo", diff --git a/vortex-geo/Cargo.toml b/vortex-geo/Cargo.toml index fcd13ffa641..f373dd5b240 100644 --- a/vortex-geo/Cargo.toml +++ b/vortex-geo/Cargo.toml @@ -35,6 +35,7 @@ wkb = { workspace = true } _test-harness = [] [dev-dependencies] +arrow-buffer = { workspace = true } divan = { workspace = true } rstest = { workspace = true } vortex-array = { workspace = true, features = ["_test-harness"] } @@ -49,5 +50,9 @@ harness = false name = "predicate_bbox" harness = false +[[bench]] +name = "coordinate_validation" +harness = false + [lints] workspace = true diff --git a/vortex-geo/benches/coordinate_validation.rs b/vortex-geo/benches/coordinate_validation.rs new file mode 100644 index 00000000000..6272680b847 --- /dev/null +++ b/vortex-geo/benches/coordinate_validation.rs @@ -0,0 +1,193 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Microbenchmark for the incremental cost of native geometry validation during Arrow import. +//! +//! Run with `cargo bench -p vortex-geo --bench coordinate_validation`. + +#![expect(clippy::unwrap_used)] + +use std::sync::LazyLock; + +use arrow_array::Array as ArrowArray; +use arrow_array::ArrayRef as ArrowArrayRef; +use arrow_schema::Field; +use divan::Bencher; +use divan::counter::BytesCount; +use vortex_array::ArrayRef; +use vortex_array::IntoArray; +use vortex_array::VortexSessionExecute; +use vortex_array::arrays::ExtensionArray; +use vortex_array::dtype::DType; +use vortex_arrow::ArrowSessionExt; +use vortex_arrow::FromArrowArray; +use vortex_error::VortexResult; +use vortex_geo::test_harness::MultiPolygonRings; +use vortex_geo::test_harness::geo_session; +use vortex_geo::test_harness::multipoint_column; +use vortex_geo::test_harness::multipolygon_column; +use vortex_geo::test_harness::nullable_multipolygon_column; +use vortex_geo::test_harness::nullable_point_column; +use vortex_geo::test_harness::point_column; +use vortex_geo::test_harness::rect_column; +use vortex_geo::test_harness::validate_list_geometry; +use vortex_geo::test_harness::validate_point; +use vortex_geo::test_harness::validate_rect; +use vortex_session::VortexSession; + +fn main() { + divan::main(); +} + +static SESSION: LazyLock = LazyLock::new(geo_session); + +const COORDINATES: usize = 1 << 20; +const COORDINATES_PER_ROW: usize = 32; +const NESTED_ROWS: usize = COORDINATES / COORDINATES_PER_ROW; +const VERTICES_PER_RING: usize = 8; + +fn ordinate(index: usize) -> f64 { + (index.wrapping_mul(2654435761) % 1000) as f64 +} + +struct ImportCase { + array: ArrowArrayRef, + field: Field, + dtype: DType, +} + +#[derive(Debug, Clone, Copy)] +enum Validation { + Without, + With, +} + +type Validator = fn(&dyn ArrowArray) -> VortexResult<()>; + +fn to_arrow(array: ArrayRef) -> ImportCase { + let mut ctx = SESSION.create_execution_ctx(); + let field = SESSION + .arrow() + .to_arrow_field("geometry", array.dtype()) + .unwrap(); + let arrow = SESSION + .arrow() + .execute_arrow(array, Some(&field), &mut ctx) + .unwrap(); + let dtype = SESSION.arrow().from_arrow_field(&field).unwrap(); + ImportCase { + array: arrow, + field, + dtype, + } +} + +fn import_native(case: &ImportCase, validation: Validation, validate: Validator) -> ArrayRef { + if matches!(validation, Validation::With) { + validate(case.array.as_ref()).unwrap(); + } + + // Both modes perform the same zero-copy import and extension wrapping. `Without` deliberately + // skips reading the coordinate buffers, so its reported throughput is not memory bandwidth. + let storage = ArrayRef::from_arrow(case.array.as_ref(), case.field.is_nullable()).unwrap(); + ExtensionArray::try_new(case.dtype.as_extension().clone(), storage) + .unwrap() + .into_array() +} + +fn xy_bytes(coordinates: usize) -> BytesCount { + BytesCount::of_many::(coordinates * 2) +} + +fn multipolygon_row(row: usize) -> MultiPolygonRings { + let ring = |part: usize| { + (0..VERTICES_PER_RING) + .map(|vertex| { + ( + ordinate(row + part + vertex), + ordinate(row + part + vertex + 1), + ) + }) + .collect() + }; + vec![vec![ring(0), ring(1)], vec![ring(2), ring(3)]] +} + +#[divan::bench(args = [Validation::Without, Validation::With])] +fn point(bencher: Bencher, validation: Validation) { + let xs = (0..COORDINATES).map(ordinate).collect(); + let ys = (0..COORDINATES).map(|index| ordinate(index + 1)).collect(); + let case = to_arrow(point_column(xs, ys).unwrap()); + + bencher + .counter(xy_bytes(COORDINATES)) + .bench(|| import_native(&case, validation, validate_point)); +} + +#[divan::bench(args = [Validation::Without, Validation::With])] +fn point_sparse_nulls(bencher: Bencher, validation: Validation) { + let points: Vec<_> = (0..COORDINATES) + .map(|index| (!index.is_multiple_of(10)).then(|| (ordinate(index), ordinate(index + 1)))) + .collect(); + let valid_points = points.iter().filter(|point| point.is_some()).count(); + let case = to_arrow(nullable_point_column(points).unwrap()); + + bencher + .counter(xy_bytes(valid_points)) + .bench(|| import_native(&case, validation, validate_point)); +} + +#[divan::bench(args = [Validation::Without, Validation::With])] +fn rect(bencher: Bencher, validation: Validation) { + let boxes = (0..COORDINATES) + .map(|index| { + let xmin = ordinate(index); + let ymin = ordinate(index + 1); + (xmin, ymin, xmin + 1.0, ymin + 1.0) + }) + .collect(); + let case = to_arrow(rect_column(boxes).unwrap()); + + bencher + .counter(BytesCount::of_many::(COORDINATES * 4)) + .bench(|| import_native(&case, validation, validate_rect)); +} + +#[divan::bench(args = [Validation::Without, Validation::With])] +fn multipoint(bencher: Bencher, validation: Validation) { + let rows: Vec<_> = (0..NESTED_ROWS) + .map(|row| { + (0..COORDINATES_PER_ROW) + .map(|point| (ordinate(row + point), ordinate(row + point + 1))) + .collect() + }) + .collect(); + let case = to_arrow(multipoint_column(rows).unwrap()); + + bencher + .counter(xy_bytes(COORDINATES)) + .bench(|| import_native(&case, validation, validate_list_geometry)); +} + +#[divan::bench(args = [Validation::Without, Validation::With])] +fn multipolygon(bencher: Bencher, validation: Validation) { + let rows = (0..NESTED_ROWS).map(multipolygon_row).collect(); + let case = to_arrow(multipolygon_column(rows).unwrap()); + + bencher + .counter(xy_bytes(COORDINATES)) + .bench(|| import_native(&case, validation, validate_list_geometry)); +} + +#[divan::bench(args = [Validation::Without, Validation::With])] +fn multipolygon_sparse_nulls(bencher: Bencher, validation: Validation) { + let rows: Vec<_> = (0..NESTED_ROWS) + .map(|row| (!row.is_multiple_of(10)).then(|| multipolygon_row(row))) + .collect(); + let valid_coordinates = rows.iter().filter(|row| row.is_some()).count() * COORDINATES_PER_ROW; + let case = to_arrow(nullable_multipolygon_column(rows).unwrap()); + + bencher + .counter(xy_bytes(valid_coordinates)) + .bench(|| import_native(&case, validation, validate_list_geometry)); +} diff --git a/vortex-geo/src/extension/linestring.rs b/vortex-geo/src/extension/linestring.rs index 274d20f28ba..a6a4bb1000c 100644 --- a/vortex-geo/src/extension/linestring.rs +++ b/vortex-geo/src/extension/linestring.rs @@ -53,6 +53,7 @@ use super::coordinate::coordinate_storage_dtype; use super::geo_metadata_from_arrow; use super::geoarrow_metadata; use super::geoarrow_to_wkb; +use super::validation::validate_list_geometry; /// A line string: `geoarrow.linestring`, stored as `List>` (an ordered path /// of vertices). @@ -140,7 +141,7 @@ fn linestring_array(storage: &ArrayRef, ctx: &mut ExecutionCtx) -> VortexResult< .map_err(|e| vortex_err!("failed to construct LineStringArray: {e}")) } -/// A validated `LineString` array (`try_from` checks the extension type). +/// A typed view of a native `LineString` extension array. pub struct LineStringData(ExtensionArray); impl TryFrom for LineStringData { @@ -290,6 +291,7 @@ impl ArrowImportVTable for LineString { { return Ok(ArrowImport::Unsupported(array)); } + validate_list_geometry(array.as_ref())?; let storage = ArrayRef::from_arrow(array.as_ref(), field.is_nullable())?; Ok(ArrowImport::Imported( diff --git a/vortex-geo/src/extension/mod.rs b/vortex-geo/src/extension/mod.rs index b05069f8ca5..1473e391b5a 100644 --- a/vortex-geo/src/extension/mod.rs +++ b/vortex-geo/src/extension/mod.rs @@ -9,6 +9,7 @@ mod multipolygon; mod point; mod polygon; mod rect; +pub(crate) mod validation; mod wkb; use std::fmt::Display; @@ -65,6 +66,9 @@ use vortex_error::vortex_ensure; use vortex_error::vortex_err; pub use wkb::*; +use self::validation::validate_list_geometry; +use self::validation::validate_point; + /// Whether `dtype` is one of the native geometry extension types the geo kernels operate on. pub(crate) fn is_native_geometry(dtype: &DType) -> bool { dtype.as_extension_opt().is_some_and(|ext| { @@ -218,10 +222,14 @@ pub fn native_geometry_scalar_from_wkb(bytes: &[u8]) -> VortexResult VortexResult { + let to_storage = |target: &GeoArrowType, + validate: fn(&dyn arrow_array::Array) -> VortexResult<()>| + -> VortexResult { let native = cast(&wkb, target).map_err(|e| vortex_err!("failed to cast WKB literal: {e}"))?; - ArrayRef::from_arrow(native.to_array_ref().as_ref(), false) + let arrow = native.to_array_ref(); + validate(arrow.as_ref())?; + ArrayRef::from_arrow(arrow.as_ref(), false) }; let scalar = match Wkb::try_from_bytes(bytes)?.geometry_type() { @@ -229,39 +237,42 @@ pub fn native_geometry_scalar_from_wkb(bytes: &[u8]) -> VortexResult { let target = GeoArrowType::LineString( LineStringType::new(Dimension::XY, metadata).with_coord_type(CoordType::Separated), ); - geo_ext_scalar(LineString, to_storage(&target)?)? + geo_ext_scalar(LineString, to_storage(&target, validate_list_geometry)?)? } GeometryType::Polygon => { let target = GeoArrowType::Polygon( PolygonType::new(Dimension::XY, metadata).with_coord_type(CoordType::Separated), ); - geo_ext_scalar(Polygon, to_storage(&target)?)? + geo_ext_scalar(Polygon, to_storage(&target, validate_list_geometry)?)? } GeometryType::MultiPoint => { let target = GeoArrowType::MultiPoint( MultiPointType::new(Dimension::XY, metadata).with_coord_type(CoordType::Separated), ); - geo_ext_scalar(MultiPoint, to_storage(&target)?)? + geo_ext_scalar(MultiPoint, to_storage(&target, validate_list_geometry)?)? } GeometryType::MultiLineString => { let target = GeoArrowType::MultiLineString( MultiLineStringType::new(Dimension::XY, metadata) .with_coord_type(CoordType::Separated), ); - geo_ext_scalar(MultiLineString, to_storage(&target)?)? + geo_ext_scalar( + MultiLineString, + to_storage(&target, validate_list_geometry)?, + )? } GeometryType::MultiPolygon => { let target = GeoArrowType::MultiPolygon( MultiPolygonType::new(Dimension::XY, metadata) .with_coord_type(CoordType::Separated), ); - geo_ext_scalar(MultiPolygon, to_storage(&target)?)? + geo_ext_scalar(MultiPolygon, to_storage(&target, validate_list_geometry)?)? } _ => return Ok(None), }; @@ -352,6 +363,26 @@ mod tests { use super::native_geometry_scalar_from_wkb; use crate::extension::GeoMetadata; + fn point_wkb(x: f64, y: f64) -> Vec { + let mut wkb = vec![1u8]; + wkb.extend_from_slice(&1u32.to_le_bytes()); + wkb.extend_from_slice(&x.to_le_bytes()); + wkb.extend_from_slice(&y.to_le_bytes()); + wkb + } + + fn linestring_wkb(points: &[(f64, f64)]) -> VortexResult> { + let mut wkb = vec![1u8]; + wkb.extend_from_slice(&2u32.to_le_bytes()); + let len = u32::try_from(points.len()).map_err(|e| vortex_err!("{e}"))?; + wkb.extend_from_slice(&len.to_le_bytes()); + for &(x, y) in points { + wkb.extend_from_slice(&x.to_le_bytes()); + wkb.extend_from_slice(&y.to_le_bytes()); + } + Ok(wkb) + } + #[test] fn test_metadata() { let meta = GeoMetadata { @@ -368,10 +399,7 @@ mod tests { /// A little-endian WKB `POINT` literal decodes to the native `Point` extension scalar. #[test] fn decodes_wkb_point_to_native() -> VortexResult<()> { - let mut wkb = vec![1u8]; // little-endian byte order - wkb.extend_from_slice(&1u32.to_le_bytes()); // geometry type: point - wkb.extend_from_slice(&1.0f64.to_le_bytes()); // x - wkb.extend_from_slice(&2.0f64.to_le_bytes()); // y + let wkb = point_wkb(1.0, 2.0); let scalar = native_geometry_scalar_from_wkb(&wkb)?.expect("a point scalar"); let DType::Extension(ext) = scalar.dtype() else { @@ -407,14 +435,7 @@ mod tests { #[test] fn decodes_wkb_linestring_to_native() -> VortexResult<()> { let points = [(0.0, 0.0), (1.0, 1.0)]; - let mut wkb = vec![1u8]; // little-endian byte order - wkb.extend_from_slice(&2u32.to_le_bytes()); // geometry type: linestring - let len = u32::try_from(points.len()).map_err(|e| vortex_err!("{e}"))?; - wkb.extend_from_slice(&len.to_le_bytes()); - for (x, y) in points { - wkb.extend_from_slice(&f64::to_le_bytes(x)); - wkb.extend_from_slice(&f64::to_le_bytes(y)); - } + let wkb = linestring_wkb(&points)?; let scalar = native_geometry_scalar_from_wkb(&wkb)?.expect("a linestring scalar"); let DType::Extension(ext) = scalar.dtype() else { @@ -424,6 +445,26 @@ mod tests { Ok(()) } + #[test] + fn accepts_empty_wkb_point_sentinel() -> VortexResult<()> { + let wkb = point_wkb(f64::NAN, f64::NAN); + assert!(native_geometry_scalar_from_wkb(&wkb)?.is_some()); + Ok(()) + } + + #[test] + fn rejects_partial_nan_wkb_point() { + let wkb = point_wkb(f64::NAN, 5.0); + assert!(native_geometry_scalar_from_wkb(&wkb).is_err()); + } + + #[test] + fn rejects_nan_wkb_linestring_coordinate() -> VortexResult<()> { + let wkb = linestring_wkb(&[(0.0, 0.0), (f64::NAN, 1.0)])?; + assert!(native_geometry_scalar_from_wkb(&wkb).is_err()); + Ok(()) + } + /// A little-endian WKB `MULTIPOINT` literal decodes to the native `MultiPoint` extension scalar. #[test] fn decodes_wkb_multipoint_to_native() -> VortexResult<()> { diff --git a/vortex-geo/src/extension/multilinestring.rs b/vortex-geo/src/extension/multilinestring.rs index 6ba4c4c5324..09463ad00b5 100644 --- a/vortex-geo/src/extension/multilinestring.rs +++ b/vortex-geo/src/extension/multilinestring.rs @@ -54,6 +54,7 @@ use super::coordinate::coordinate_storage_dtype; use super::geo_metadata_from_arrow; use super::geoarrow_metadata; use super::geoarrow_to_wkb; +use super::validation::validate_list_geometry; /// A multilinestring: `geoarrow.multilinestring`, stored as `List>>` /// (line strings of vertices). @@ -147,7 +148,7 @@ fn multilinestring_array( .map_err(|e| vortex_err!("failed to construct MultiLineStringArray: {e}")) } -/// A validated `MultiLineString` array (`try_from` checks the extension type). +/// A typed view of a native `MultiLineString` extension array. pub struct MultiLineStringData(ExtensionArray); impl TryFrom for MultiLineStringData { @@ -300,6 +301,7 @@ impl ArrowImportVTable for MultiLineString { { return Ok(ArrowImport::Unsupported(array)); } + validate_list_geometry(array.as_ref())?; let storage = ArrayRef::from_arrow(array.as_ref(), field.is_nullable())?; Ok(ArrowImport::Imported( diff --git a/vortex-geo/src/extension/multipoint.rs b/vortex-geo/src/extension/multipoint.rs index 7d8e429dcc0..7521f8264cf 100644 --- a/vortex-geo/src/extension/multipoint.rs +++ b/vortex-geo/src/extension/multipoint.rs @@ -54,6 +54,7 @@ use super::coordinate::coordinate_storage_dtype; use super::geo_metadata_from_arrow; use super::geoarrow_metadata; use super::geoarrow_to_wkb; +use super::validation::validate_list_geometry; /// A multipoint: `geoarrow.multipoint`, stored as `List>` (a set of points). #[derive(Debug, Clone, Default, PartialEq, Eq, Hash)] @@ -138,7 +139,7 @@ fn multipoint_array(storage: &ArrayRef, ctx: &mut ExecutionCtx) -> VortexResult< .map_err(|e| vortex_err!("failed to construct MultiPointArray: {e}")) } -/// A validated `MultiPoint` array (`try_from` checks the extension type). +/// A typed view of a native `MultiPoint` extension array. pub struct MultiPointData(ExtensionArray); impl TryFrom for MultiPointData { @@ -285,6 +286,7 @@ impl ArrowImportVTable for MultiPoint { { return Ok(ArrowImport::Unsupported(array)); } + validate_list_geometry(array.as_ref())?; let storage = ArrayRef::from_arrow(array.as_ref(), field.is_nullable())?; Ok(ArrowImport::Imported( diff --git a/vortex-geo/src/extension/multipolygon.rs b/vortex-geo/src/extension/multipolygon.rs index 524e470749c..4384f61a667 100644 --- a/vortex-geo/src/extension/multipolygon.rs +++ b/vortex-geo/src/extension/multipolygon.rs @@ -53,6 +53,7 @@ use super::coordinate::coordinate_storage_dtype; use super::geo_metadata_from_arrow; use super::geoarrow_metadata; use super::geoarrow_to_wkb; +use super::validation::validate_list_geometry; /// A multipolygon (`geoarrow.multipolygon`); a single `Polygon` is a one-element multipolygon. #[derive(Debug, Clone, Default, PartialEq, Eq, Hash)] @@ -148,7 +149,7 @@ fn multipolygon_array( .map_err(|e| vortex_err!("failed to construct MultiPolygonArray: {e}")) } -/// A validated `MultiPolygon` array (`try_from` checks the extension type). +/// A typed view of a native `MultiPolygon` extension array. pub struct MultiPolygonData(ExtensionArray); impl TryFrom for MultiPolygonData { @@ -302,6 +303,7 @@ impl ArrowImportVTable for MultiPolygon { { return Ok(ArrowImport::Unsupported(array)); } + validate_list_geometry(array.as_ref())?; let storage = ArrayRef::from_arrow(array.as_ref(), field.is_nullable())?; Ok(ArrowImport::Imported( diff --git a/vortex-geo/src/extension/point.rs b/vortex-geo/src/extension/point.rs index cfebecee461..56f76242692 100644 --- a/vortex-geo/src/extension/point.rs +++ b/vortex-geo/src/extension/point.rs @@ -53,6 +53,7 @@ use super::coordinate::coordinate_storage_dtype; use super::geo_metadata_from_arrow; use super::geoarrow_metadata; use super::geoarrow_to_wkb; +use super::validation::validate_point; /// A single location: `geoarrow.point`, stored as `Struct` of non-nullable `f64`. #[derive(Debug, Clone, Default, PartialEq, Eq, Hash)] @@ -275,6 +276,7 @@ impl ArrowImportVTable for Point { { return Ok(ArrowImport::Unsupported(array)); } + validate_point(array.as_ref())?; let storage = ArrayRef::from_arrow(array.as_ref(), field.is_nullable())?; Ok(ArrowImport::Imported( diff --git a/vortex-geo/src/extension/polygon.rs b/vortex-geo/src/extension/polygon.rs index 9a74c3ce7b3..1bfa4d276e1 100644 --- a/vortex-geo/src/extension/polygon.rs +++ b/vortex-geo/src/extension/polygon.rs @@ -53,6 +53,7 @@ use super::coordinate::coordinate_storage_dtype; use super::geo_metadata_from_arrow; use super::geoarrow_metadata; use super::geoarrow_to_wkb; +use super::validation::validate_list_geometry; /// A polygon: `geoarrow.polygon`, stored as `List>>` (rings of vertices). #[derive(Debug, Clone, Default, PartialEq, Eq, Hash)] @@ -140,7 +141,7 @@ fn polygon_array(storage: &ArrayRef, ctx: &mut ExecutionCtx) -> VortexResult for PolygonData { @@ -294,6 +295,7 @@ impl ArrowImportVTable for Polygon { { return Ok(ArrowImport::Unsupported(array)); } + validate_list_geometry(array.as_ref())?; let storage = ArrayRef::from_arrow(array.as_ref(), field.is_nullable())?; Ok(ArrowImport::Imported( diff --git a/vortex-geo/src/extension/rect.rs b/vortex-geo/src/extension/rect.rs index 4c373a1807f..c874b1e94be 100644 --- a/vortex-geo/src/extension/rect.rs +++ b/vortex-geo/src/extension/rect.rs @@ -55,6 +55,7 @@ use super::GeoMetadata; use super::coordinate::Dimension; use super::geo_metadata_from_arrow; use super::geoarrow_metadata; +use super::validation::validate_rect; /// An axis-aligned bounding box (`geoarrow.box`), stored as `Struct`. // Named `Rect`, not `Box`: matches `geo::Rect` / geoarrow-rs `RectArray`, and `Box` is a std name. @@ -303,6 +304,7 @@ impl ArrowImportVTable for Rect { return Ok(ArrowImport::Unsupported(array)); } + validate_rect(array.as_ref())?; let storage = ArrayRef::from_arrow(array.as_ref(), field.is_nullable())?; Ok(ArrowImport::Imported( ExtensionArray::try_new(ext_dtype.clone(), storage)?.into_array(), diff --git a/vortex-geo/src/extension/validation.rs b/vortex-geo/src/extension/validation.rs new file mode 100644 index 00000000000..826a3ff76bb --- /dev/null +++ b/vortex-geo/src/extension/validation.rs @@ -0,0 +1,261 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: Copyright the Vortex contributors + +//! Geometry-value validation at the GeoArrow import boundary. +//! +//! The geometry importers validate the schema first, so coordinate and box columns are already in +//! canonical order here. + +use std::ops::Range; + +use arrow_array::Array; +use arrow_array::Float64Array; +use arrow_array::ListArray; +use arrow_array::StructArray; +use vortex_error::VortexResult; +use vortex_error::vortex_bail; +use vortex_error::vortex_ensure; +use vortex_error::vortex_err; + +/// Validate Point coordinates before importing them into native storage. +/// +/// - NaN in every ordinate is the empty-Point sentinel. +/// - Otherwise, X and Y must be finite. +/// - Z and M are attributes and are not part of this 2-D validity check. +/// - Child values of null Point rows are ignored. +pub fn validate_point(array: &dyn Array) -> VortexResult<()> { + let coordinates = array + .as_any() + .downcast_ref::() + .ok_or_else(|| vortex_err!("geo: Point storage must be a coordinate Struct"))?; + let ordinate_columns = coordinates + .columns() + .iter() + .map(|column| { + column + .as_any() + .downcast_ref::() + .ok_or_else(|| vortex_err!("geo: coordinate ordinates must be f64")) + }) + .collect::>>()?; + let [x, y, ..] = ordinate_columns.as_slice() else { + vortex_bail!("geo: coordinates must contain x and y ordinates"); + }; + + for_each_non_null_run(coordinates, |rows| { + let xs = &x.values()[rows.clone()]; + let ys = &y.values()[rows.clone()]; + + let valid = if ordinate_columns.len() == 2 { + // An XY Point is either the all-NaN empty sentinel or a pair of finite ordinates. + xs.iter().zip(ys).fold(true, |all_valid, (&x, &y)| { + let empty = x.is_nan() & y.is_nan(); + let finite = x.is_finite() & y.is_finite(); + all_valid & (empty | finite) + }) + } else { + rows.fold(true, |all_valid, index| { + let empty = ordinate_columns.iter().fold(true, |all_nan, ordinate| { + all_nan & ordinate.value(index).is_nan() + }); + let finite = x.value(index).is_finite() & y.value(index).is_finite(); + all_valid & (empty | finite) + }) + }; + + vortex_ensure!(valid, "geo: native Point contains an invalid coordinate"); + Ok(()) + }) +} + +/// Validate Rect bounds before importing them into native storage. +/// +/// - A non-null Rect must have finite, ordered X/Y bounds. +/// - Z and M bounds are not part of this 2-D validity check. +/// - Child values of null Rect rows are ignored. +pub fn validate_rect(array: &dyn Array) -> VortexResult<()> { + let bounds = array + .as_any() + .downcast_ref::() + .ok_or_else(|| vortex_err!("geo: Rect storage must be a Struct"))?; + let columns = bounds.columns(); + vortex_ensure!( + columns.len() >= 4 && columns.len().is_multiple_of(2), + "geo: Rect storage must contain lower and upper bounds" + ); + let dimensions = columns.len() / 2; + let [xmin, ymin, ..] = columns else { + vortex_bail!("geo: Rect storage must contain x/y bounds"); + }; + let (Some(xmin), Some(ymin), Some(xmax), Some(ymax)) = ( + xmin.as_any().downcast_ref::(), + ymin.as_any().downcast_ref::(), + columns[dimensions].as_any().downcast_ref::(), + columns[dimensions + 1] + .as_any() + .downcast_ref::(), + ) else { + vortex_bail!("geo: Rect bounds must be f64"); + }; + + for_each_non_null_run(bounds, |rows| { + let xmin = &xmin.values()[rows.clone()]; + let ymin = &ymin.values()[rows.clone()]; + let xmax = &xmax.values()[rows.clone()]; + let ymax = &ymax.values()[rows]; + let x_bounds = xmin.iter().zip(xmax); + let y_bounds = ymin.iter().zip(ymax); + let valid = + x_bounds + .zip(y_bounds) + .fold(true, |all_valid, ((&xmin, &xmax), (&ymin, &ymax))| { + all_valid + & xmin.is_finite() + & ymin.is_finite() + & xmax.is_finite() + & ymax.is_finite() + & (xmin <= xmax) + & (ymin <= ymax) + }); + vortex_ensure!(valid, "geo: native Rect contains invalid x/y bounds"); + Ok(()) + }) +} + +/// Validate list-based geometry coordinates before importing them into native storage. +/// +/// Empty geometries use empty lists, so every coordinate reachable from a non-null outer row must +/// have finite X and Y. Z and M are attributes and are not part of this 2-D validity check. +pub fn validate_list_geometry(array: &dyn Array) -> VortexResult<()> { + let mut values = array; + let mut list_levels = Vec::new(); + while let Some(list) = values.as_any().downcast_ref::() { + list_levels.push(list); + values = list.values().as_ref(); + } + + let coordinates = values + .as_any() + .downcast_ref::() + .ok_or_else(|| vortex_err!("geo: geometry storage must end with a coordinate Struct"))?; + let [x, y, ..] = coordinates.columns() else { + vortex_bail!("geo: coordinates must contain x and y ordinates"); + }; + let (Some(x), Some(y)) = ( + x.as_any().downcast_ref::(), + y.as_any().downcast_ref::(), + ) else { + vortex_bail!("geo: coordinate ordinates must be f64"); + }; + let outer = list_levels + .first() + .ok_or_else(|| vortex_err!("geo: geometry storage must begin with a List"))?; + + for_each_non_null_run(*outer, |rows| { + let mut coordinate_rows = rows; + for list in &list_levels { + let offsets = list.value_offsets(); + let start = usize::try_from(offsets[coordinate_rows.start]) + .map_err(|_| vortex_err!("geo: list offset exceeds usize"))?; + let end = usize::try_from(offsets[coordinate_rows.end]) + .map_err(|_| vortex_err!("geo: list offset exceeds usize"))?; + coordinate_rows = start..end; + } + + let xs = &x.values()[coordinate_rows.clone()]; + let ys = &y.values()[coordinate_rows]; + + // Bitwise boolean reduction keeps the buffer scan branch-free and vectorizable. + let valid = xs.iter().zip(ys).fold(true, |all_valid, (&x, &y)| { + all_valid & x.is_finite() & y.is_finite() + }); + vortex_ensure!( + valid, + "geo: native geometry contains a non-finite x/y coordinate" + ); + Ok(()) + }) +} + +/// Apply `validate` to contiguous runs of non-null rows. +/// +/// Arrow child values beneath a null parent are unspecified, so they must not affect validation. +fn for_each_non_null_run( + array: &dyn Array, + mut validate: impl FnMut(Range) -> VortexResult<()>, +) -> VortexResult<()> { + let Some(nulls) = array.nulls() else { + return validate(0..array.len()); + }; + if nulls.null_count() == array.len() { + return Ok(()); + } + if nulls.null_count() == 0 { + return validate(0..array.len()); + } + + nulls + .valid_slices() + .try_for_each(|(start, end)| validate(start..end)) +} + +#[cfg(test)] +mod tests { + use vortex_array::VortexSessionExecute; + use vortex_arrow::ArrowSessionExt; + use vortex_error::VortexResult; + + use super::validate_list_geometry; + use super::validate_rect; + use crate::test_harness::geo_session; + use crate::test_harness::linestring_column; + use crate::test_harness::multilinestring_column; + use crate::test_harness::multipoint_column; + use crate::test_harness::multipolygon_column; + use crate::test_harness::polygon_column; + use crate::test_harness::rect_column; + + #[test] + fn rejects_non_finite_xy_for_every_storage_shape() -> VortexResult<()> { + let invalid = (f64::NAN, 0.0); + let arrays = [ + ("LineString", linestring_column(vec![vec![invalid]])?), + ("MultiPoint", multipoint_column(vec![vec![invalid]])?), + ("Polygon", polygon_column(vec![vec![vec![invalid]]])?), + ( + "MultiLineString", + multilinestring_column(vec![vec![vec![invalid]]])?, + ), + ( + "MultiPolygon", + multipolygon_column(vec![vec![vec![vec![invalid]]]])?, + ), + ]; + let session = geo_session(); + let mut ctx = session.create_execution_ctx(); + + for (geometry_type, array) in arrays { + let arrow = session.arrow().execute_arrow(array, None, &mut ctx)?; + assert!( + validate_list_geometry(arrow.as_ref()).is_err(), + "{geometry_type} accepted a NaN x coordinate" + ); + } + + let rects = rect_column(vec![(f64::NAN, 0.0, 1.0, 1.0)])?; + let arrow = session.arrow().execute_arrow(rects, None, &mut ctx)?; + assert!(validate_rect(arrow.as_ref()).is_err()); + Ok(()) + } + + #[test] + fn rejects_inverted_rect_bounds() -> VortexResult<()> { + let session = geo_session(); + let mut ctx = session.create_execution_ctx(); + let rects = rect_column(vec![(2.0, 0.0, 1.0, 1.0)])?; + let arrow = session.arrow().execute_arrow(rects, None, &mut ctx)?; + + assert!(validate_rect(arrow.as_ref()).is_err()); + Ok(()) + } +} diff --git a/vortex-geo/src/test_harness.rs b/vortex-geo/src/test_harness.rs index 7bec0f90ee9..0dcd335ca09 100644 --- a/vortex-geo/src/test_harness.rs +++ b/vortex-geo/src/test_harness.rs @@ -37,6 +37,9 @@ use crate::extension::multilinestring_storage_dtype; use crate::extension::multipoint_storage_dtype; use crate::extension::multipolygon_storage_dtype; use crate::extension::polygon_storage_dtype; +pub use crate::extension::validation::validate_list_geometry; +pub use crate::extension::validation::validate_point; +pub use crate::extension::validation::validate_rect; /// A fresh session with the geospatial types, functions, and pruning rules registered. pub fn geo_session() -> VortexSession { diff --git a/vortex-geo/src/tests/linestring.rs b/vortex-geo/src/tests/linestring.rs index 46e2be94647..8e2c5077064 100644 --- a/vortex-geo/src/tests/linestring.rs +++ b/vortex-geo/src/tests/linestring.rs @@ -5,6 +5,9 @@ use std::sync::Arc; +use arrow_array::Array; +use arrow_array::ListArray as ArrowListArray; +use arrow_buffer::NullBuffer; use arrow_schema::DataType; use arrow_schema::Field; use arrow_schema::extension::ExtensionType as _; @@ -13,6 +16,7 @@ use geoarrow::datatypes::Crs; use geoarrow::datatypes::Dimension as GeoArrowDimension; use geoarrow::datatypes::LineStringType; use geoarrow::datatypes::Metadata; +use vortex_array::VortexSessionExecute; use vortex_array::dtype::DType; use vortex_array::dtype::Nullability; use vortex_arrow::ArrowSessionExt; @@ -20,6 +24,7 @@ use vortex_error::VortexResult; use super::SESSION; use crate::extension::LineString; +use crate::test_harness::linestring_column; /// A `geoarrow.linestring` Arrow field with separated (struct) XY coordinates. fn linestring_field(name: &str, nullable: bool, crs: Option<&str>) -> Field { @@ -86,3 +91,66 @@ fn export_field_carries_extension() -> VortexResult<()> { ); Ok(()) } + +/// A NaN coordinate in a non-Point geometry is invalid rather than an empty-geometry sentinel. +#[test] +fn rejects_non_finite_coordinate() -> VortexResult<()> { + let mut ctx = SESSION.create_execution_ctx(); + let field = linestring_field("geom", false, Some("EPSG:4326")); + let source = linestring_column(vec![vec![(0.0, 0.0), (f64::NAN, 1.0)]])?; + let arrow = SESSION + .arrow() + .execute_arrow(source, Some(&field), &mut ctx)?; + + assert!(SESSION.arrow().from_arrow_array(arrow, &field).is_err()); + Ok(()) +} + +/// Coordinates outside an Arrow slice are not part of the imported geometry. +#[test] +fn ignores_non_finite_coordinates_outside_slice() -> VortexResult<()> { + let mut ctx = SESSION.create_execution_ctx(); + let field = linestring_field("geom", false, Some("EPSG:4326")); + let source = linestring_column(vec![ + vec![(f64::NAN, 0.0)], + vec![(1.0, 2.0), (3.0, 4.0)], + vec![(5.0, f64::NAN)], + ])?; + let arrow = SESSION + .arrow() + .execute_arrow(source, Some(&field), &mut ctx)? + .slice(1, 1); + + SESSION.arrow().from_arrow_array(arrow, &field)?; + Ok(()) +} + +/// Child values of a null geometry row are unspecified and must not be validated. +#[test] +fn ignores_non_finite_coordinates_under_null_row() -> VortexResult<()> { + let mut ctx = SESSION.create_execution_ctx(); + let field = linestring_field("geom", true, Some("EPSG:4326")); + let source = linestring_column(vec![vec![(f64::NAN, 0.0)], vec![(1.0, 2.0)]])?; + let arrow = SESSION + .arrow() + .execute_arrow(source, Some(&field), &mut ctx)?; + let lists = arrow + .as_any() + .downcast_ref::() + .ok_or_else(|| vortex_error::vortex_err!("expected Arrow ListArray"))?; + let DataType::List(element) = lists.data_type() else { + vortex_error::vortex_bail!("expected Arrow ListArray") + }; + let nullable = ArrowListArray::try_new( + Arc::clone(element), + lists.offsets().clone(), + Arc::clone(lists.values()), + Some(NullBuffer::from(vec![false, true])), + ) + .map_err(|error| vortex_error::vortex_err!("failed to build nullable list: {error}"))?; + + SESSION + .arrow() + .from_arrow_array(Arc::new(nullable), &field)?; + Ok(()) +} diff --git a/vortex-geo/src/tests/point.rs b/vortex-geo/src/tests/point.rs index 61a2e98dbe7..d603262b9c3 100644 --- a/vortex-geo/src/tests/point.rs +++ b/vortex-geo/src/tests/point.rs @@ -10,6 +10,7 @@ use arrow_array::Float64Array; use arrow_array::StructArray as ArrowStructArray; use arrow_array::cast::AsArray; use arrow_array::types::Float64Type; +use arrow_buffer::NullBuffer; use arrow_schema::DataType; use arrow_schema::Field; use arrow_schema::Fields; @@ -34,28 +35,51 @@ use crate::test_harness::point_column; /// A `geoarrow.point` Arrow field with separated (struct) XY coordinates. fn point_field(name: &str, nullable: bool, crs: Option<&str>) -> Field { + point_field_with_dimension(name, nullable, crs, GeoArrowDimension::XY) +} + +/// A `geoarrow.point` Arrow field with separated coordinates in the requested dimension. +fn point_field_with_dimension( + name: &str, + nullable: bool, + crs: Option<&str>, + dimension: GeoArrowDimension, +) -> Field { let crs = crs .map(|crs| Crs::from_unknown_crs_type(crs.to_string())) .unwrap_or_default(); let metadata = Arc::new(Metadata::new(crs, None)); - PointType::new(GeoArrowDimension::XY, metadata).to_field(name, nullable) + PointType::new(dimension, metadata).to_field(name, nullable) } /// An Arrow `Struct` point array with non-nullable `Float64` children. fn arrow_point_struct(xs: Vec, ys: Vec) -> ArrowStructArray { - let fields: Fields = vec![ - Field::new("x", DataType::Float64, false), - Field::new("y", DataType::Float64, false), - ] - .into(); - ArrowStructArray::new( - fields, - vec![ - Arc::new(Float64Array::from(xs)) as ArrowArrayRef, - Arc::new(Float64Array::from(ys)), - ], - None, - ) + arrow_point_struct_with_ordinates([("x", xs), ("y", ys)]) +} + +/// An Arrow `Struct` Point array with non-nullable `Float64` children. +fn arrow_point_struct_xyz(xs: Vec, ys: Vec, zs: Vec) -> ArrowStructArray { + arrow_point_struct_with_ordinates([("x", xs), ("y", ys), ("z", zs)]) +} + +/// An Arrow `Struct` Point array with non-nullable `Float64` children. +fn arrow_point_struct_xym(xs: Vec, ys: Vec, ms: Vec) -> ArrowStructArray { + arrow_point_struct_with_ordinates([("x", xs), ("y", ys), ("m", ms)]) +} + +fn arrow_point_struct_with_ordinates( + ordinates: [(&str, Vec); N], +) -> ArrowStructArray { + let fields: Fields = ordinates + .iter() + .map(|(name, _)| Field::new(*name, DataType::Float64, false)) + .collect::>() + .into(); + let columns = ordinates + .into_iter() + .map(|(_, values)| Arc::new(Float64Array::from(values)) as ArrowArrayRef) + .collect(); + ArrowStructArray::new(fields, columns, None) } /// The exported Arrow field carries the `geoarrow.point` extension over the separated @@ -161,6 +185,85 @@ fn imports_from_struct() -> VortexResult<()> { Ok(()) } +/// GeoArrow's all-NaN Point sentinel is an empty Point and remains importable. +#[test] +fn imports_empty_point_sentinel() -> VortexResult<()> { + let arrow: ArrowArrayRef = Arc::new(arrow_point_struct(vec![f64::NAN], vec![f64::NAN])); + let field = point_field("loc", false, Some("EPSG:4326")); + + SESSION.arrow().from_arrow_array(arrow, &field)?; + Ok(()) +} + +/// A Point with only one non-finite XY ordinate is malformed, not GeoArrow's empty-Point +/// sentinel, and is rejected before it enters native Vortex storage. +#[test] +fn rejects_partial_nan_point() { + let arrow: ArrowArrayRef = Arc::new(arrow_point_struct(vec![f64::NAN], vec![5.0])); + let field = point_field("loc", false, Some("EPSG:4326")); + assert!(SESSION.arrow().from_arrow_array(arrow, &field).is_err()); +} + +/// Child values of a null Point row are unspecified and must not be validated. +#[test] +fn ignores_invalid_point_under_null_row() -> VortexResult<()> { + let mut ctx = SESSION.create_execution_ctx(); + let field = point_field("loc", true, Some("EPSG:4326")); + let source = point_column(vec![f64::NAN, 1.0], vec![0.0, 2.0])?; + let arrow = SESSION + .arrow() + .execute_arrow(source, Some(&field), &mut ctx)?; + let points = arrow.as_struct(); + let nullable = ArrowStructArray::try_new( + points.fields().clone(), + points.columns().to_vec(), + Some(NullBuffer::from(vec![false, true])), + ) + .map_err(|error| vortex_err!("failed to build nullable points: {error}"))?; + + SESSION + .arrow() + .from_arrow_array(Arc::new(nullable), &field)?; + Ok(()) +} + +/// A higher-dimensional Point is empty only when every ordinate is NaN. A NaN Z value on a +/// finite XY coordinate remains an attribute value, because native geometry validity is 2-D. +#[test] +fn xyz_empty_point_uses_every_ordinate() -> VortexResult<()> { + let field = point_field_with_dimension("loc", false, Some("EPSG:4326"), GeoArrowDimension::XYZ); + + let empty: ArrowArrayRef = Arc::new(arrow_point_struct_xyz( + vec![f64::NAN], + vec![f64::NAN], + vec![f64::NAN], + )); + SESSION.arrow().from_arrow_array(empty, &field)?; + + let partial: ArrowArrayRef = Arc::new(arrow_point_struct_xyz( + vec![f64::NAN], + vec![f64::NAN], + vec![1.0], + )); + assert!(SESSION.arrow().from_arrow_array(partial, &field).is_err()); + + let finite_xy: ArrowArrayRef = + Arc::new(arrow_point_struct_xyz(vec![1.0], vec![2.0], vec![f64::NAN])); + SESSION.arrow().from_arrow_array(finite_xy, &field)?; + Ok(()) +} + +/// M is carried as an attribute ordinate, so a NaN M value does not invalidate finite XY. +#[test] +fn xym_point_keeps_nan_measure() -> VortexResult<()> { + let field = point_field_with_dimension("loc", false, Some("EPSG:4326"), GeoArrowDimension::XYM); + let point: ArrowArrayRef = + Arc::new(arrow_point_struct_xym(vec![1.0], vec![2.0], vec![f64::NAN])); + + SESSION.arrow().from_arrow_array(point, &field)?; + Ok(()) +} + /// A point column exported to Arrow and imported back is unchanged, including the CRS. #[test] fn roundtrips_through_arrow() -> VortexResult<()> { diff --git a/vortex-geo/src/tests/rect.rs b/vortex-geo/src/tests/rect.rs index 865892493e7..d61e2270e54 100644 --- a/vortex-geo/src/tests/rect.rs +++ b/vortex-geo/src/tests/rect.rs @@ -5,6 +5,9 @@ use std::sync::Arc; +use arrow_array::Array; +use arrow_array::StructArray as ArrowStructArray; +use arrow_buffer::NullBuffer; use arrow_schema::DataType; use arrow_schema::Field; use arrow_schema::extension::ExtensionType as _; @@ -110,6 +113,54 @@ fn roundtrips_through_arrow() -> VortexResult<()> { Ok(()) } +/// Non-null boxes must contain finite, ordered X/Y bounds before entering native storage. +#[test] +fn rejects_invalid_box_values() -> VortexResult<()> { + let field = box_field("bbox", GeoArrowDimension::XY, false, Some("EPSG:4326")); + let invalid_boxes = [ + (f64::NAN, 0.0, 1.0, 1.0), + (0.0, 0.0, f64::INFINITY, 1.0), + (2.0, 0.0, 1.0, 1.0), + (0.0, 2.0, 1.0, 1.0), + ]; + + for invalid_box in invalid_boxes { + let mut ctx = SESSION.create_execution_ctx(); + let source = rect_column(vec![invalid_box])?; + let arrow = SESSION + .arrow() + .execute_arrow(source, Some(&field), &mut ctx)?; + assert!(SESSION.arrow().from_arrow_array(arrow, &field).is_err()); + } + Ok(()) +} + +/// Bounds beneath a null box are unspecified and must not affect validation. +#[test] +fn ignores_invalid_bounds_under_null_box() -> VortexResult<()> { + let mut ctx = SESSION.create_execution_ctx(); + let field = box_field("bbox", GeoArrowDimension::XY, true, Some("EPSG:4326")); + let source = rect_column(vec![(f64::NAN, 2.0, 1.0, 1.0)])?; + let arrow = SESSION + .arrow() + .execute_arrow(source, Some(&field), &mut ctx)?; + let rects = arrow + .as_any() + .downcast_ref::() + .ok_or_else(|| vortex_err!("expected Arrow StructArray"))?; + let nullable = ArrowStructArray::try_new( + rects.fields().clone(), + rects.columns().to_vec(), + Some(NullBuffer::from(vec![false])), + ) + .map_err(|error| vortex_err!("failed to build nullable Rect: {error}"))?; + + SESSION + .arrow() + .from_arrow_array(Arc::new(nullable), &field)?; + Ok(()) +} + /// The existing geo scalar functions run on a `Rect` operand via the shared `geometries()` decode, /// producing the same results as the equivalent polygon: a box `(0,0)-(10,10)` against interior /// point `(5,5)` and exterior point `(20,20)`.