feat(vortex-geo): null-propagating scalar functions#8803
Conversation
Polar Signals Profiling ResultsLatest Run
Previous Runs (9)
Powered by Polar Signals Cloud |
Merging this PR will improve performance by 11.17%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
Benchmarks: Vortex queries 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.050x ➖, 0↑ 0↓)
datafusion / parquet (0.978x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.997x ➖, 0↑ 0↓)
duckdb / parquet (0.996x ➖, 0↑ 0↓)
No file size changes detected. |
|
@claude review this PR, and review it also with the context of |
|
Claude finished @connortsui20's task in 3m 58s —— View job Claude PR Review —
|
9a50270 to
0716c2c
Compare
|
@claude review again, again check for consistency within vortex-geo and between vortex-tensor, and generally look for null semantics bugs |
|
Claude finished @connortsui20's task in 3m 16s —— View job Claude PR Review — round 2 (consistency + null semantics)Re-reviewed the updated diff against
Verdict: The round-1 feedback landed cleanly — Null-semantics audit (no bugs found)Traced every path in
1. Test-coverage inconsistency —
|
| null path | distance |
contains |
intersects |
|---|---|---|---|
| col-vs-const null rows | ✅ | ✅ | ✅ |
| constant-null → all null | ✅ | ✅ | ✅ |
two-col pair nulls (eval_column_pair) |
✅ | ✅ | ❌ |
all-null column (valid.all_false() in eval_column) |
❌ | ✅ | ❌ |
two-col all-null (valid.all_false() in eval_column_pair) |
❌ | ✅ | ❌ |
ST_Intersects never runs eval_column_pair with nulls at all — the entire two-nullable-column path is untested for it. Since intersection is symmetric it's cheap to add, and it's the highest-value gap. Adding the all-false early-return tests to distance/intersects (mirroring contains_all_null_column_is_all_null / contains_column_pair_all_null) would bring all three to parity. Fix this →
2. is_fallible divergence from vortex-tensor is correct — leave as-is
vortex-tensor sets is_fallible = false; the geo functions keep the true default. This is the right call and should not be copied: geometry decoding (geometries → WKB/geoarrow decode) can genuinely fail on malformed storage, whereas the tensor math kernels cannot. Flagging only to confirm the asymmetry is intentional, not an oversight.
3. Minor — binary_result_validity and the dead AllOr::None arms
- The shared
binary_result_validityhelper is a nice consolidation, and @connortsui20's inline note (mod.rs:142) about promoting it to a general array-level helper is a reasonable follow-up — many vtablevalidity()impls compute exactlyand(child validities). Out of scope for this PR, but worth a tracking issue. - The
unreachable!on theAllOr::Nonearms in bothbuild_arrayimpls is now clearly documented as guarded by theall_false()short-circuits — good. The earlierBitBuffer::from_iter(vec![false; len])concern is gone since that arm no longer allocates. ✅
Consistency with vortex-tensor — summary
| aspect | tensor | geo (this PR) | consistent? |
|---|---|---|---|
return_dtype nullability |
any operand nullable |
any operand nullable |
✅ |
validity() |
and(child0, child1) |
and(child0, child1) via helper |
✅ |
is_null_sensitive |
false |
false |
✅ |
is_fallible |
false |
true (default) |
✅ justified (decode can fail) |
| operand type check | eq_ignore_nullability (same type) |
each is some geometry | ✅ justified (cross-type geo ops are valid) |
I ran no build/test commands (read-only review); the coverage table was derived by reading the test modules. Nice, well-documented change overall.
· branch nemo/geo-null-propagation
connortsui20
left a comment
connortsui20
left a comment
There was a problem hiding this comment.
Must fix
1. A null geometry literal now breaks zone pruning. Before this PR a nullable operand was rejected at type-check, so the prune rules never saw a null literal. Now ST_Intersects(geom, NULL) executes fine (all-null result, tested), but query_aabb (prune/mod.rs:84-89) calls single_geometry on the constant with no is_null check, hits "geo: null geometry is not supported", and the error propagates out of the stats rewrite driver (vortex-array/src/stats/rewrite.rs:148), failing the whole rewrite. Minimal fix: decline on scalar.is_null() (return Ok(None)); optimal: rewrite to lit(true) since an all-NULL predicate matches nothing — cf. how the null radius literal is already declined in prune/distance.rs:68-73. Needs a prune test either way.
2. Zero-length inputs with non-nullable operands produce the wrong dtype. Mask::AllTrue(0) has true_count() == 0, so all_false() is vacuously true, and eval_column / eval_column_pair check all_false() before all_true() (scalar_fn/mod.rs:220-223, 252-254). A len-0 non-nullable execution therefore returns the Nullable all_null_array while return_dtype declared NonNullable — tripping the debug-build result-dtype assertion (vortex-array/src/scalar_fn/typed.rs:149-158), silently mismatched in release. Untested; worth a test.
Suggested simplification (also fixes bug 2)
GeoOutput can drop from 3 methods to 2: make build_array handle AllOr::None by returning an all-invalid array instead of unreachable! (scalar_fn/mod.rs:87, 122). Then null_dtype(), all_null_array, and the two all_false() early-returns all become unnecessary — the const-null paths can route through T::build_array(len, &Mask::new_false(len), vec![], Nullable). That removes the panic and its cross-function invariant, and the len-0 case falls out correct for free (AllTrue(0) → indices() is All → correctly-typed empty array).
Also minor: the if q.scalar().is_null() { return all_null } block is repeated 3× in execute_null_propagating (mod.rs:170-191) — one loop over both operands before the match does it once.
Worth a note in this PR
Nullable columns now silently degrade AABB pruning: GeometryAabb::accumulate reads the unmasked coordinate buffers (aggregate_fn/aabb.rs:207-216), so null-row placeholder coords (e.g. (0,0)) get unioned into the zone box. Safe direction — only ever under-prunes — but a placeholder at the origin drags a distant chunk's box across the plane. Deserves a comment at accumulate, and there's no test covering a nullable column through the aggregate or pruning end-to-end.
|
| let valid = array.validity()?.execute_mask(array.len(), ctx)?; | ||
| let array = if valid.all_true() { | ||
| array | ||
| } else { | ||
| array.filter(valid)? | ||
| }; |
There was a problem hiding this comment.
I remember (maybe incorrectly) that it used to be the case you didnt need this if statement, .filter should do this check for you. @joseph-isaacs is this still true? tracing through the code its hard for me to tell
There was a problem hiding this comment.
Confirmed. .filter collapses an all-true mask: Filter's reduce rule (filter/rules.rs:43) returns the child, and ArrayRef::filter runs .optimize(). So the if all_true guard was redundant; removed it here.
| // 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. |
There was a problem hiding this comment.
so do any of the functions below need a precondition that it probably shouldnt take nulls? I also think that converting nulls to (0, 0) might be a footgun in general
There was a problem hiding this comment.
Yes, both functions have an implicit "no nulls" precondition, now documented. I think we should never converts nulls to (0, 0).
| .execute::<PrimitiveArray>(ctx)?; | ||
| if let Some(rect) = aabb_of(xs.as_slice::<f64>(), ys.as_slice::<f64>()) { |
There was a problem hiding this comment.
Basically it would be good to add some comments here explaining that because nulls have been removed we are fine to use all the values.
But that brings up a separate question: did you look at the perf of computing this merge on very sparsely null data? for example, if there is only a single null value, it would be way faster to not execute the filter above and instead just merge if the value is not null.
There was a problem hiding this comment.
Commented. I think it is fine, it's uniform across all geometry types, and the hot/common path (non-nullable) already costs nothing. The sparse-null compaction is an acceptable tradeoff for a write-time stat; if profiling ever flags nullable-geometry stat writes, revisit with a coordinate-validity-aware min/max.
Add expr::union_child_validities — the conjunction of an expression's child validities (result null iff any operand is null), the common ScalarFnVTable validity() for null-propagating kernels. Migrate vortex-tensor's four scalar functions (InnerProduct, CosineSimilarity, L2Denorm, L2Norm) onto it, replacing their inline and(child validities). Signed-off-by: Nemo Yu <zyu379@wisc.edu>
Make the binary geo scalar functions (ST_Distance, ST_Intersects, ST_Contains) null-propagating: a nullable geometry operand is allowed, and any row whose geometry input is null yields a null result (null in -> null out), matching SQL/OGC and the other Vortex binary kernels. - validate_geometry_operands no longer rejects nullable operands - return_dtype mirrors operand nullability; validity() uses the shared vortex_array::expr::union_child_validities (#8829); is_null_sensitive = false - shared execute module filters null rows before decoding, computes over the rows valid in both operands, and scatters results back under the combined mask - prune: query_aabb declines a null geometry literal instead of erroring - GeometryAabb::accumulate skips null rows so placeholders don't widen the box - tests for nullable columns, constant-null, column/column, empty input Signed-off-by: Nemo Yu <zyu379@wisc.edu>
cb2af19 to
45f0a48
Compare
ArrayRef::filter collapses an all-true mask back to the input (Filter's reduce rule + optimize), so the if valid.all_true() guards in GeometryAabb::accumulate and the execute helpers were redundant. Filter unconditionally; all-valid inputs pass through unchanged. Signed-off-by: Nemo Yu <zyu379@wisc.edu>
connortsui20
left a comment
connortsui20
left a comment
There was a problem hiding this comment.
I think this looks good, but I would like if someone else has eyes on this as well. @joseph-isaacs?
| // the origin. `filter` collapses an all-true mask back to the input, so a null-free batch | ||
| // passes through unchanged. | ||
| let valid = array.validity()?.execute_mask(array.len(), ctx)?; | ||
| let array = array.filter(valid)?; |
There was a problem hiding this comment.
This could be expensive.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
shall we add a null in the points too? With a min/max geo
| // 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)?; |
There was a problem hiding this comment.
This memory copy looks slow too. Might be worth nothing with a todo this is likely slow and should be fixed in needed
There was a problem hiding this comment.
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.
| fn eval_column<T, F>( | ||
| column: &ArrayRef, | ||
| f: F, | ||
| nullability: Nullability, | ||
| ctx: &mut ExecutionCtx, | ||
| ) -> VortexResult<ArrayRef> | ||
| where | ||
| T: GeoOutput, | ||
| F: Fn(&Geometry<f64>) -> T, | ||
| { | ||
| let len = column.len(); | ||
| let valid = column.validity()?.execute_mask(len, ctx)?; | ||
| // Drop the null rows before decoding, since a null row has no geometry to decode. `filter` | ||
| // collapses an all-true mask, so an all-valid column passes through unchanged. | ||
| let decoded = geometries(&column.filter(valid.clone())?, ctx)?; | ||
| let values = decoded.iter().map(f).collect(); | ||
| Ok(T::build_array(len, &valid, values, nullability)) | ||
| } | ||
|
|
||
| /// Evaluate `compute` over each row where both geometry columns are valid, propagating the nulls | ||
| /// of either column. | ||
| fn eval_column_pair<T, F>( | ||
| a: &ArrayRef, | ||
| b: &ArrayRef, | ||
| compute: F, | ||
| nullability: Nullability, | ||
| ctx: &mut ExecutionCtx, | ||
| ) -> VortexResult<ArrayRef> | ||
| where | ||
| T: GeoOutput, | ||
| F: Fn(&Geometry<f64>, &Geometry<f64>) -> T, | ||
| { | ||
| let len = a.len(); | ||
| let a_present = a.validity()?.execute_mask(len, ctx)?; | ||
| let b_present = b.validity()?.execute_mask(len, ctx)?; | ||
| // A row survives only where both columns are present. | ||
| let valid = &a_present & &b_present; | ||
| // Keep only the rows valid in both columns, so decoding never sees a null geometry. `filter` | ||
| // collapses an all-true mask, so all-valid columns pass through unchanged. | ||
| let ag = geometries(&a.filter(valid.clone())?, ctx)?; | ||
| let bg = geometries(&b.filter(valid.clone())?, ctx)?; | ||
| let values = ag.iter().zip(&bg).map(|(x, y)| compute(x, y)).collect(); | ||
| Ok(T::build_array(len, &valid, values, nullability)) | ||
| } |
There was a problem hiding this comment.
Have you benchmarked this code at all (its on the read path correct?).
geometries looks slow due to both memory copies and usage of a enum
There was a problem hiding this comment.
Generally speaking I think a lot of things in the geo crate are kind of slow / not optimized because we are prioritizing features instead of speed here... And the fact that we store things via columns instead of rows is already huge.
So imo it would be better to merge things and then if we see perf is slow in e2e benchmarks we can optimize.
That being said... yes it would be good to see some numbers @HarukiMoriarty
There was a problem hiding this comment.
SF = 1
| Query | parquet base | parquet PR | vortex base | vortex PR | geo-native base | geo-native PR |
|---|---|---|---|---|---|---|
| 1 | 45.6 | 44.4 | 13.1 | 13.6 | 7.0 | 7.0 |
| 2 | 176.1 | 161.4 | 40.4 | 42.3 | 54.0 | 56.7 |
| 3 | 77.0 | 75.9 | 35.7 | 25.0 | 6.4 | 9.4 |
| 4 | 630.0 | 639.0 | 80.4 | 61.3 | 139.3 | 129.6 |
| 5 | 434.5 | 374.4 | 408.7 | 297.1 | 310.9 | 315.7 |
| 6 | 737.1 | 735.3 | 126.7 | 113.3 | 169.6 | 150.6 |
| 7 | 186.9 | 205.2 | 86.7 | 157.4 | 102.5 | 102.3 |
| 8 | 159.8 | 160.6 | 62.0 | 71.7 | 69.0 | 79.3 |
| 9 | 19.3 | 22.6 | 18.5 | 19.0 | 19.9 | 22.1 |
SF = 3
| Query | parquet base | parquet PR | vortex base | vortex PR | geo-native base | geo-native PR |
|---|---|---|---|---|---|---|
| 1 | 50.3 | 52.6 | 45.8 | 32.6 | 11.1 | 12.7 |
| 2 | 177.7 | 186.7 | 67.1 | 65.5 | 112.2 | 139.5 |
| 3 | 119.4 | 131.4 | 82.1 | 73.5 | 10.3 | 13.3 |
| 4 | 583.0 | 622.4 | 71.8 | 76.8 | 114.9 | 121.7 |
| 5 | 1120.0 | 1140.0 | 962.1 | 999.8 | 941.8 | 985.0 |
| 6 | 743.5 | 775.7 | 134.2 | 125.4 | 230.2 | 223.2 |
| 7 | 469.5 | 387.7 | 385.7 | 469.1 | 414.5 | 384.4 |
| 8 | 200.2 | 263.6 | 237.0 | 198.1 | 232.7 | 301.8 |
| 9 | 30.9 | 34.0 | 30.1 | 36.4 | 28.3 | 42.2 |
SF = 10
| Query | parquet base | parquet PR | vortex base | vortex PR | geo-native base | geo-native PR |
|---|---|---|---|---|---|---|
| 1 | 176.0 | 174.3 | 107.4 | 105.0 | 22.7 | 21.5 |
| 2 | 362.6 | 350.3 | 190.7 | 184.9 | 319.6 | 307.0 |
| 3 | 289.2 | 287.5 | 230.9 | 248.2 | 24.4 | 23.6 |
| 4 | 911.3 | 891.1 | 108.4 | 106.3 | 144.2 | 140.3 |
| 5 | 3660.0 | 3280.0 | 3120.0 | 3000.0 | 3120.0 | 3000.0 |
| 6 | 1250.0 | 1230.0 | 330.9 | 282.6 | 437.0 | 426.2 |
| 7 | 1290.0 | 1270.0 | 1100.0 | 1080.0 | 946.0 | 930.9 |
| 8 | 709.5 | 722.1 | 542.0 | 526.1 | 666.2 | 649.6 |
| 9 | 32.1 | 32.7 | 31.1 | 31.9 | 41.3 | 40.0 |
There was a problem hiding this comment.
Q1 Q3 and Q6 actually fire the code you mentioned, and all within run-to-tun noise.
When would this path actually cost anything?
- the column actually contains nulls, otherwise
filter(all_true)is a no-op andbuild_arraytakes the all-valid fast path, i.e. zero added work; - the nulls are sparse. That's the worst case: we pay the filter-compaction + scatter over ~all rows but only skip decoding a handful of null rows. (With dense nulls you filter most rows away and save decode time; with no nulls it's free.)
That regime is uncommon: geometry columns are almost always fully populated (null geometries are unusual and typically dropped at ingest, so SpatialBench's zero-null case is representative).
I think it should be fine?
There was a problem hiding this comment.
I am not sure what this shows?
…umulate The GeometryAabb accumulate filters out null rows before min/max-ing the raw coordinate buffers, which compacts the whole column even for a single null. Record a validity-aware min/max over the raw x/y buffers as the future optimization; the common non-nullable case is already a no-op. Signed-off-by: Nemo Yu <zyu379@wisc.edu>
## Rationale for this change - Closes: #8807 Three benchmarks stayed flaky after #8742, flipping between the same two values on PRs that can't affect them. Same root causes and fixes as #8742: | Benchmark | Seen flaky on | Why | Fix | | --- | --- | --- | --- | | `true_count_vortex_buffer[128]` | ±11.17% on 9 unrelated PRs (#8805, #8811, #8812, #8820, #8843, #8803, …) | a 128-bit popcount measures harness overhead and code layout, not the count | drop the 128 size | | runend `compress[(100000, 4)]` | ±11.9% on #8805, #8750, #8856 | allocates in the timed region; glibc malloc differs across runner images | mimalloc as global allocator | | `cast_decimal` `copy_*[65536]` | identical flags on #8838 and #8724 | same glibc-malloc cause (512 KB alloc per iteration) | mimalloc as global allocator | Left alone: `compact_sliced[(4096, 90)]` (single sighting) and the CUDA walltime benches (hosted-runner walltime noise, a runner config issue). The allocator swap shifts every benchmark in the two touched binaries once — see the comment below. Needs a one-time CodSpeed acknowledgment, like #8742. ## What changes are included in this PR? One commit per benchmark; bench files only. Ran `cargo check` + `clippy` on the three bench targets, smoke-ran the binaries, `cargo +nightly fmt`. --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
3 participants
What
Make the binary geo scalar functions (
ST_Distance,ST_Intersects,ST_Contains) null-propagating. A nullable geometry operand is now allowed, and any row whose geometry input is null yields aNULLresult.Why
Spiral needs the geo predicates to accept nullable inputs to run SpatialBench.
How
A geo kernel decodes each operand into a
geo_typesgeometry, and a null row has no geometry to decode, so it can't compute over every row and mask the nulls afterwards the way the numeric kernels do.Instead, the shared
execute_null_propagatingdispatch drops the null rows, computes only the rows valid in both operands, and scatters the results back under the combined null mask.return_dtypeis nullable iff an operand is, andvalidate_geometry_operandsno longer rejects nullable operands.