Skip to content

Commit 6b67b8e

Browse files
refactor(vortex-geo): address review - GeometryAabb rename + AABB terminology sweep
- Rename GeometryBounds -> GeometryAabb (id vortex.geo.bounds -> vortex.geo.aabb), BoundsPartial -> AabbPartial, aggregate_fn/bounds.rs -> aabb.rs: the statistic is specifically the axis-aligned bounding box, leaving room for other bbox kinds later - Rename GeoDistanceBoundsPrune -> GeoDistancePrune; the AABB is an implementation detail carried by the doc comment - Sweep docs, comments, and test names to axis-aligned bounding box (AABB) terminology - Document what zone_stat_default is for and note zone_stat_defaults' linear registry scan (intended once per column at writer open) - Present GeometryAabb as a plain aggregate whose zone-stat role is an application - Collapse AabbPartial::merge to map_or - Comment which other operators could prune in the future (==) or cannot (!=), and why the radius try_from fall-through is a sound decline rather than an error - Extract test_harness::geo_session() to replace repeated session+initialize in tests Signed-off-by: Nemo Yu <zyu379@wisc.edu>
1 parent 322e91d commit 6b67b8e

9 files changed

Lines changed: 167 additions & 153 deletions

File tree

vortex-array/src/aggregate_fn/plugin.rs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,15 @@ pub trait AggregateFnPlugin: 'static + Send + Sync {
3030
fn deserialize(&self, metadata: &[u8], session: &VortexSession)
3131
-> VortexResult<AggregateFnRef>;
3232

33-
/// The default per-chunk zone statistic to store for a column of `input_dtype`, or `None` if
34-
/// this aggregate isn't one.
33+
/// The default zone statistic (per-chunk) for a column of `input_dtype`, or `None` if the
34+
/// dtype is not supported (or this aggregate is not a zone statistic at all).
35+
///
36+
/// This is how a registered aggregate volunteers itself as a per-chunk statistic: when a
37+
/// zoned layout writer opens a column, it collects every plugin's default via
38+
/// [`AggregateFnSession::zone_stat_defaults`], so new statistics can be added without the
39+
/// writer knowing about them.
40+
///
41+
/// [`AggregateFnSession::zone_stat_defaults`]: crate::aggregate_fn::session::AggregateFnSession::zone_stat_defaults
3542
fn zone_stat_default(&self, _input_dtype: &DType) -> Option<AggregateFnRef> {
3643
None
3744
}

vortex-array/src/aggregate_fn/session.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,9 @@ impl AggregateFnSession {
138138

139139
/// The default per-chunk zone statistics for a column of `input_dtype`, collected from every
140140
/// registered aggregate's `zone_stat_default`.
141+
///
142+
/// Each call scans the whole plugin registry, so this is intended to be called once per
143+
/// column when a zoned writer is opened, not per chunk or per row.
141144
pub fn zone_stat_defaults(&self, input_dtype: &DType) -> Vec<AggregateFnRef> {
142145
self.registry.read(|registry| {
143146
let mut fns: Vec<AggregateFnRef> = registry
Lines changed: 73 additions & 75 deletions
Large diffs are not rendered by default.

vortex-geo/src/aggregate_fn/mod.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,6 @@
33

44
//! Aggregate functions over geometry columns.
55
6-
mod bounds;
6+
mod aabb;
77

8-
pub use bounds::*;
8+
pub use aabb::*;

vortex-geo/src/extension/rect.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ use super::coordinate::Dimension;
5656
use super::geo_metadata_from_arrow;
5757
use super::geoarrow_metadata;
5858

59-
/// A bounding box (`geoarrow.box`), stored as `Struct<xmin, ymin[, ..], xmax, ymax[, ..]>`.
59+
/// An axis-aligned bounding box (`geoarrow.box`), stored as `Struct<xmin, ymin[, ..], xmax, ymax[, ..]>`.
6060
// Named `Rect`, not `Box`: matches `geo::Rect` / geoarrow-rs `RectArray`, and `Box` is a std name.
6161
#[derive(Debug, Clone, Default, PartialEq, Eq, Hash)]
6262
pub struct Rect;

vortex-geo/src/lib.rs

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use vortex_array::stats::session::StatsSessionExt;
1010
use vortex_arrow::ArrowSessionExt;
1111
use vortex_session::VortexSession;
1212

13-
use crate::aggregate_fn::GeometryBounds;
13+
use crate::aggregate_fn::GeometryAabb;
1414
use crate::extension::LineString;
1515
use crate::extension::MultiLineString;
1616
use crate::extension::MultiPoint;
@@ -19,7 +19,7 @@ use crate::extension::Point;
1919
use crate::extension::Polygon;
2020
use crate::extension::Rect;
2121
use crate::extension::WellKnownBinary;
22-
use crate::prune::GeoDistanceBoundsPrune;
22+
use crate::prune::GeoDistancePrune;
2323
use crate::scalar_fn::contains::GeoContains;
2424
use crate::scalar_fn::distance::GeoDistance;
2525
use crate::scalar_fn::intersects::GeoIntersects;
@@ -66,9 +66,10 @@ pub fn initialize(session: &VortexSession) {
6666
session.scalar_fns().register(GeoDistance);
6767
session.scalar_fns().register(GeoIntersects);
6868

69-
// The bounding-box aggregate; self-declares as a per-chunk zone stat for geometry columns.
70-
session.aggregate_fns().register(GeometryBounds);
69+
// The axis-aligned bounding-box (AABB) aggregate; self-declares as a per-chunk zone stat for
70+
// geometry columns.
71+
session.aggregate_fns().register(GeometryAabb);
7172

72-
// Register the spatial pruning rule that uses that bounding box.
73-
session.stats().register_rewrite(GeoDistanceBoundsPrune);
73+
// Register the spatial pruning rule that uses that AABB.
74+
session.stats().register_rewrite(GeoDistancePrune);
7475
}

vortex-geo/src/prune.rs

Lines changed: 63 additions & 62 deletions
Large diffs are not rendered by default.

vortex-geo/src/test_harness.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ use vortex_array::scalar::Scalar;
1717
use vortex_array::validity::Validity;
1818
use vortex_error::VortexResult;
1919
use vortex_error::vortex_err;
20+
use vortex_session::VortexSession;
2021

2122
use crate::extension::GeoMetadata;
2223
use crate::extension::LineString;
@@ -36,6 +37,13 @@ use crate::extension::multipoint_storage_dtype;
3637
use crate::extension::multipolygon_storage_dtype;
3738
use crate::extension::polygon_storage_dtype;
3839

40+
/// A fresh session with the geospatial types, functions, and pruning rules registered.
41+
pub(crate) fn geo_session() -> VortexSession {
42+
let session = vortex_array::array_session();
43+
crate::initialize(&session);
44+
session
45+
}
46+
3947
/// The WGS 84 (`EPSG:4326`) metadata tagged onto test geometry columns.
4048
fn wgs84() -> GeoMetadata {
4149
GeoMetadata {

vortex-geo/src/tests/mod.rs

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,4 @@ use std::sync::LazyLock;
1717
use vortex_session::VortexSession;
1818

1919
/// A session with the geospatial types and functions registered.
20-
static SESSION: LazyLock<VortexSession> = LazyLock::new(|| {
21-
let session = vortex_array::array_session();
22-
crate::initialize(&session);
23-
session
24-
});
20+
static SESSION: LazyLock<VortexSession> = LazyLock::new(crate::test_harness::geo_session);

0 commit comments

Comments
 (0)