Skip to content

feat(vortex-geo): add native geoarrow.box bounding-box type#8746

Merged
HarukiMoriarty merged 3 commits into
developfrom
nemo/geo-box-type
Jul 13, 2026
Merged

feat(vortex-geo): add native geoarrow.box bounding-box type#8746
HarukiMoriarty merged 3 commits into
developfrom
nemo/geo-box-type

Conversation

@HarukiMoriarty

Copy link
Copy Markdown
Contributor

Rationale for this change

Groundwork for spatial chunk-pruning: the upcoming geometry-bounds zone-map needs to store bounding boxes as a first-class geometry so the pruning rules can reuse the existing ST_Distance / ST_Intersects / ST_Contains kernels on them instead of hand-rolled box math.

What changes are included in this PR?

  • New Rect extension type (vortex.geo.box, GeoArrow's geoarrow.box): an axis-aligned box stored as Struct<xmin, ymin[, zmin][, mmin], xmax, ymax[, zmax][, mmax]> of non-nullable f64.

Add the `Rect` extension type (`vortex.geo.box`, GeoArrow's `geoarrow.box`):
an axis-aligned envelope stored as `Struct<xmin, ymin[, zmin][, mmin], xmax,
ymax[, zmax][, mmax]>` of non-nullable f64, supporting all four GeoArrow
dimensions in storage and the arrow round-trip. Named `Rect` (not `Box`) to
match `geo::Rect` / geoarrow-rs `RectArray`.

Wired into `is_native_geometry` and the `geometries()` decode dispatch, so the
existing geo scalar functions (GeoDistance/GeoIntersects/GeoContains) accept a
box operand and reuse their kernels on it — decoding to a 2D
`geo_types::Geometry::Rect` (z/m dropped, as for the other types). This is the
foundation for the geometry-bounds zone-map stat to emit a native box column
and have the pruning rules reuse those kernels.

Signed-off-by: Nemo Yu <zyu379@wisc.edu>
@connortsui20 connortsui20 self-requested a review July 13, 2026 19:59
@connortsui20 connortsui20 added the changelog/feature A new feature label Jul 13, 2026
Comment thread vortex-geo/src/lib.rs
Comment on lines +54 to +55
session.dtypes().register(Rect);
session.arrow().register_exporter(Arc::new(Rect));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

surely we can write a helper functiuon that makes this nice :D

@connortsui20

Copy link
Copy Markdown
Member

@claude please review this

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @connortsui20's task in 1m 42s —— View job


PR Review: feat(vortex-geo): add native geoarrow.box bounding-box type

  • Read changed files & gather context
  • Review rect.rs extension type
  • Review test harness & tests
  • Post review feedback

Overall this is a clean, well-documented addition that closely mirrors the existing Polygon/Point extension pattern. The Arrow import/export vtables, dtype validation, and decode-to-geo_types path all line up with the sibling types, and reusing the shared geometries() decode so the existing ST_* kernels work on a Rect for free is exactly the right call. Nice work. A few notes below, none blocking.


Repetition in initialize() — echoing @connortsui20

The reviewer's comment on lib.rs:55 is well-founded: the three-line register triad is now repeated eight times:

session.dtypes().register(Rect);
session.arrow().register_exporter(Arc::new(Rect));
session.arrow().register_importer(Arc::new(Rect));

A small helper would collapse all of them and make adding the next geometry a one-liner. Since Rect: ExtVTable + ArrowExportVTable + ArrowImportVTable + Clone + 'static, something like:

fn register_geo_ext<T>(session: &VortexSession, ext: T)
where
    T: ExtVTable + ArrowExportVTable + ArrowImportVTable + Clone + Send + Sync + 'static,
{
    session.dtypes().register(ext.clone());
    session.arrow().register_exporter(Arc::new(ext.clone()));
    session.arrow().register_importer(Arc::new(ext));
}

then register_geo_ext(session, Rect); etc. (Exact bounds depend on the register signatures — worth verifying, but the shape is clearly reducible.) This is a pre-existing pattern so it's fair to defer, but the box type is what tips it into "surely a helper" territory. Fix this →


Test doc-comment claims an untested case

In rect.rs:353-374, box_rejects_invalid_storage's doc comment says it rejects three things:

non-struct storage, a struct with the wrong field names, and a struct with a nullable field.

But the test only exercises the first two — the nullable-field case is never constructed or asserted. That path (box_dimension rejecting a Nullable f64 field via the matches!(... NonNullable) check at rect.rs:147-150) is currently untested. Either add the case or trim the comment. Since the non-nullable invariant is load-bearing for the storage contract, I'd add it:

// A struct whose fields are nullable f64 is not valid box storage.
let nullable = DType::Struct(
    StructFields::new(
        ["xmin", "ymin", "xmax", "ymax"].into(),
        std::iter::repeat_n(
            DType::Primitive(PType::F64, Nullability::Nullable), 4,
        ).collect(),
    ),
    Nullability::NonNullable,
);
assert!(ExtDType::<Rect>::try_new(geo_meta(), nullable).is_err());

Fix this →


Arrow interop only exercised for XY

box_validates_every_dimension covers dtype construction for all four dimensions, which is great. But the Arrow export/import round-trip tests (tests/rect.rs) only ever use GeoArrowDimension::XY. Since box_field_names and box_type() claim support for Xyz/Xym/Xyzm, it would be worth one round-trip test through geoarrow's RectArray for a 3D/4D box — that path is where a mismatch between our field ordering (all mins, then all maxes) and geoarrow's BoxType layout would actually surface. Right now nothing verifies that RectArray::try_from((arrow, box_meta)) succeeds for anything but XY. Consider parameterizing roundtrips_through_arrow over dimensions, or at least adding an XYZ case.


Minor / non-blocking

  • rect_array unused metadata (rect.rs:167): uses GeoMetadata::default() and drops CRS when decoding to geo_types. This matches polygon_array and is correct for planar ops, but unlike polygon.rs:118 there's no inline comment explaining why the CRS is intentionally discarded. A one-line note would keep the two symmetric.
  • No RectData/to_wkb equivalent: Polygon has a PolygonData wrapper with to_wkb, and Rect doesn't. That looks intentional (a box isn't a WKB primitive, and it's correctly omitted from native_geometry_scalar_from_wkb), just flagging that the asymmetry is deliberate.
  • box_dimension vs Dimension::from_field_names: the box variant is a separate implementation because the field names differ (xmin/ymin vs x/y), so no reuse is possible — this is fine, just noting it isn't accidental duplication.

I couldn't run cargo clippy/cargo nextest -p vortex-geo in this review pass (read-only review), so the helper-refactor bound suggestion above should be compiled before landing.
nemo/geo-box-type

@codspeed-hq

codspeed-hq Bot commented Jul 13, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚡ 4 improved benchmarks
❌ 4 regressed benchmarks
✅ 1656 untouched benchmarks
⏩ 42 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation chunked_varbinview_opt_canonical_into[(1000, 10)] 168.9 µs 205.6 µs -17.83%
Simulation chunked_varbinview_into_canonical[(1000, 10)] 169.5 µs 205.9 µs -17.67%
Simulation chunked_varbinview_into_canonical[(100, 100)] 272.2 µs 306.7 µs -11.24%
Simulation encode_varbin[(1000, 8)] 139.1 µs 155 µs -10.26%
Simulation bitwise_not_vortex_buffer_mut[128] 244.4 ns 215.3 ns +13.55%
Simulation chunked_varbinview_opt_canonical_into[(100, 100)] 340 µs 305.3 µs +11.37%
Simulation eq_i64_constant 298.2 µs 268.4 µs +11.13%
Simulation bitwise_not_vortex_buffer_mut[1024] 304.7 ns 275.6 ns +10.58%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing nemo/geo-box-type (98bdc0e) with develop (0aa35f4)

Open in CodSpeed

Footnotes

  1. 42 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@connortsui20 connortsui20 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

as stated offline I think this is fine but I think the code could be a bit cleaner if you used the matcher pattern we have (like in vortex-tensor).

Once you fix the rustdoc error I think this is fine to merge

@HarukiMoriarty HarukiMoriarty enabled auto-merge (squash) July 13, 2026 21:10
@HarukiMoriarty HarukiMoriarty merged commit 2f18a4c into develop Jul 13, 2026
76 of 78 checks passed
@HarukiMoriarty HarukiMoriarty deleted the nemo/geo-box-type branch July 13, 2026 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/feature A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants