-
Notifications
You must be signed in to change notification settings - Fork 190
feat(vortex-geo): null-propagating scalar functions #8803
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f81df62
45f0a48
eea991e
8fe6c0e
57a1eb3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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)?; | ||
| // 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)?; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm, I think the memory copy is the 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")? | ||
|
|
@@ -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; | ||
|
|
||
|
|
@@ -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] | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be expensive.
There was a problem hiding this comment.
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-stataccumulate, 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.