Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions vortex-geo/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ geoarrow-cast = { workspace = true }
prost = { workspace = true }
vortex-array = { workspace = true }
vortex-arrow = { workspace = true }
vortex-buffer = { workspace = true }
vortex-error = { workspace = true }
vortex-mask = { workspace = true }
vortex-session = { workspace = true }
wkb = { workspace = true }

Expand Down
34 changes: 32 additions & 2 deletions vortex-geo/src/aggregate_fn/aabb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,20 @@ impl AggregateFnVTable for GeometryAabb {
Columnar::Canonical(canonical) => canonical.clone().into_array(),
Columnar::Constant(constant) => constant.clone().into_array(),
};
// Min/max the raw x/y buffers directly - cheap, and avoids `to_geometry`'s panic on empty
// points (which decoding each geometry would hit).
// Drop null rows before reading coordinates: a null geometry's storage holds placeholder
// coordinates (e.g. `(0, 0)`) that would otherwise widen the zone box and drag it toward
// the origin. `filter` collapses an all-true mask back to the input, so a null-free batch
// passes through unchanged.
//
// TODO(perf): on nullable data this `filter` compacts the whole column even for a single
// null before we min/max it. A validity-aware min/max straight over the raw x/y buffers
// would skip that copy. Left as-is for now: this is a write-time zone stat, and the common
// non-nullable case already costs nothing (the all-true mask makes `filter` a no-op).
let valid = array.validity()?.execute_mask(array.len(), ctx)?;
let array = array.filter(valid)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be expensive.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filter(valid) you flagged is in the AABB zone-stat accumulate, so it runs once at write time building the zone map, not on the read path.

On null-free columns (all of SpatialBench, and the common case) filter(all_true) is a no-op; with actual nulls it's a one-time bulk compaction amortized across all reads, I add a TODO here.

Read-side, base-vs-PR is identical across SF 1/3/10 — the per-row kernel isn't even hit here, since DuckDB evaluates ST_* and Vortex only prunes via the stored box.

// Null rows are gone, so every coordinate below belongs to a present geometry — the
// `unmasked_field_by_name` reads are therefore safe. Min/max the raw x/y buffers directly:
// cheap, and avoids `to_geometry`'s panic on empty points (which decoding would hit).
let coords = flatten_coordinates(&array, ctx)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This memory copy looks slow too. Might be worth nothing with a todo this is likely slow and should be fixed in needed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I think the memory copy is the filter(valid) line (one above), not flatten_coordinates. flatten_coordinates just structurally unwraps the extension to its Struct<x,y>.

The filter only copies when nulls are actually present (null-free = no-op), it's a one-time write-side cost.

let xs = coords
.unmasked_field_by_name("x")?
Expand Down Expand Up @@ -254,6 +266,7 @@ mod tests {
use crate::test_harness::multilinestring_column;
use crate::test_harness::multipoint_column;
use crate::test_harness::multipolygon_column;
use crate::test_harness::nullable_point_column;
use crate::test_harness::point_column;
use crate::test_harness::polygon_column;

Expand Down Expand Up @@ -304,6 +317,23 @@ mod tests {
Ok(())
}

/// Null rows contribute no extent: their placeholder coordinates must not widen the box toward
/// the origin (matching min/max, which skip nulls).
#[test]
fn null_rows_do_not_widen_aabb() -> VortexResult<()> {
let session = vortex_array::array_session();
let mut ctx = session.create_execution_ctx();

// The valid points sit far from the origin; the null row's storage placeholder is (0, 0).
let column = nullable_point_column(vec![Some((5.0, 6.0)), None, Some((7.0, 8.0))])?;
let mut acc = Accumulator::try_new(GeometryAabb, EmptyOptions, column.dtype().clone())?;
acc.accumulate(&column, &mut ctx)?;

// The box covers only the two valid points, never (0, 0).
assert_eq!(aabb(&acc.finish()?)?, (5.0, 6.0, 7.0, 8.0));
Ok(())
}

/// The AABB of a Polygon column unions every ring vertex - exercising the `List<List<Struct>>`
/// unwrap, not just the bare Point struct.
#[test]
Expand Down
11 changes: 5 additions & 6 deletions vortex-geo/src/extension/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,18 +72,17 @@ pub(crate) fn is_native_geometry(dtype: &DType) -> bool {
})
}

/// Validate the operands of a geo scalar function: each must be a native geometry type (so the
/// kernel can decode it) and non-nullable (geometry arrays never carry nulls).
/// Validate the operands of a geo scalar function: each must be a native geometry type so the
/// kernel can decode it. The two operands need not share a geometry type — e.g. a `Point` against
/// a `Polygon` is valid, since distance/containment/intersection across types is meaningful.
/// Nullable operands are allowed; the kernels propagate nulls (a null geometry input yields a null
/// result) rather than decoding null rows.
pub(crate) fn validate_geometry_operands(dtypes: &[DType]) -> VortexResult<()> {
for dtype in dtypes {
vortex_ensure!(
is_native_geometry(dtype),
"geo: operand {dtype} is not a native geometry type"
);
vortex_ensure!(
!dtype.is_nullable(),
"geo: nullable operand {dtype} is unsupported"
);
}
Ok(())
}
Expand Down
16 changes: 16 additions & 0 deletions vortex-geo/src/prune/distance.rs

@joseph-isaacs joseph-isaacs Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we add a null in the points too? With a min/max geo

Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,22 @@ mod tests {
GeoDistancePrune.falsify(&predicate, &StatsRewriteCtx::new(&session, &scope))
}

/// A null geometry literal (`ST_Distance(geom, NULL) <= r`) declines cleanly instead of
/// erroring in the stats rewrite: the all-null predicate can never prune.
#[test]
fn null_literal_is_not_pruned() -> VortexResult<()> {
let session = geo_session();

let scope = point_column(vec![0.0], vec![0.0])?.dtype().clone();
let null_query = Scalar::null(scope.as_nullable());
let distance = GeoDistance.new_expr(EmptyOptions, [root(), lit(null_query)]);
let predicate = Binary.new_expr(Operator::Lte, [distance, lit(0.5f64)]);

let ctx = StatsRewriteCtx::new(&session, &scope);
assert!(GeoDistancePrune.falsify(&predicate, &ctx)?.is_none());
Ok(())
}

/// All four distance comparisons prune (`<=`/`<` via min-distance, `>=`/`>` via max-distance);
/// `==`/`!=` are left to the scan.
#[rstest]
Expand Down
16 changes: 16 additions & 0 deletions vortex-geo/src/prune/intersects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ mod tests {
use vortex_array::expr::Expression;
use vortex_array::expr::lit;
use vortex_array::expr::root;
use vortex_array::scalar::Scalar;
use vortex_array::scalar_fn::EmptyOptions;
use vortex_array::scalar_fn::ScalarFnVTableExt;
use vortex_array::stats::rewrite::StatsRewriteCtx;
Expand Down Expand Up @@ -114,6 +115,21 @@ mod tests {
Ok(())
}

/// A null geometry literal (`ST_Intersects(geom, NULL)`) declines cleanly instead of erroring
/// in the stats rewrite: the all-null predicate can never prune.
#[test]
fn null_literal_is_not_pruned() -> VortexResult<()> {
let session = geo_session();

let scope = point_column(vec![0.0], vec![0.0])?.dtype().clone();
let null_query = Scalar::null(scope.as_nullable());
let predicate = GeoIntersects.new_expr(EmptyOptions, [root(), lit(null_query)]);

let ctx = StatsRewriteCtx::new(&session, &scope);
assert!(GeoIntersectsPrune.falsify(&predicate, &ctx)?.is_none());
Ok(())
}

/// End-to-end: a zone strictly separated from the query is skipped; zones containing or merely
/// touching the query must scan, touching geometries intersect under OGC semantics.
#[test]
Expand Down
4 changes: 4 additions & 0 deletions vortex-geo/src/prune/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ fn geometry_and_constant<'a>(
/// Prove claims against this box rather than the constant itself: the constant lies inside it,
/// so whatever holds for the box holds for the geometry.
fn query_aabb(constant: &Scalar, ctx: &StatsRewriteCtx<'_>) -> VortexResult<Option<GeoRect<f64>>> {
// A null geometry literal has no extent to prove against, so it can never prune.
if constant.is_null() {
return Ok(None);
}
// Decoding the constant into a concrete geometry runs through the compute stack, which needs
// an execution context.
let mut exec = ctx.session().create_execution_ctx();
Expand Down
Loading
Loading