Repository navigation
Add filter-and-scatter RowFn execution - #9521
Conversation
Merging this PR will regress 1 benchmark
|
2268a7d to
84b879d
Compare
84b879d to
b6c09be
Compare
|
Can you merge this with the next pr so we can see if this is the best approach |
|
@joseph-isaacs this is not about being a better approach, it's that we literally cannot support spatial with rowfn without this let me update the pr description to include more detail |
|
im actually going to hold off on this until we can figure out how we want to deal with @HarukiMoriarty's work |
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
685e0d2 to
2cb8ddb
Compare
|
The downstream consumer is now visible in #9623, which is stacked directly on this PR and contains only spatial changes. |
| "valid-only execution requires direct valid-row support; {} selected an unsupported signature", | ||
| self.id, | ||
| ) | ||
| self.filter_and_scatter(kernel, &valid_rows, ctx) |
There was a problem hiding this comment.
how is this the default here?
There was a problem hiding this comment.
the alternative is much more complicated (per-row decode needs to be defined by each input type). That can maybe be added in the future. This is just the easy way to do it
| DenseWithRetry, | ||
|
|
||
| /// Execute only valid rows over the original inputs. | ||
| /// Execute only valid rows, filtering inputs if direct execution is unavailable. |
There was a problem hiding this comment.
this is likely MUCH slower for most operations
There was a problem hiding this comment.
yes, but its simple and doesnt require adding another 2k LoC (probably more)
## Overview Depends on #9521. Moves the remaining Rust `geo`-backed spatial scalar functions onto RowFn. This removes the bespoke geometry execution adapters and leaves constant handling, strict null propagation, input decoding, output construction, and validity restoration with the shared executor. `GeometryRow` now provides only its ordinary fallible decoder. It does not manufacture placeholder geometries for null rows or expand the shared `InputElement` API. On a partially valid batch, direct valid-row execution declines and #9521 filters every input to the jointly valid rows before geometry decoding. ## Implemented - Migrates area, contains, intersects, convex hull, and length to RowFn. - Preserves constant bounding-box prechecks for contains and intersects. - Keeps length on native LineString and MultiLineString storage. - Adds native MultiLineString support to ST_Length. - Removes the bespoke geo-types execution layer. ## Execution choice A prepared selected-row geometry view was prototyped separately. It was substantially faster for sparse validity, but slower for ordinary nullable predicates and about 45% slower than filter-and-scatter on the dense polygon control. It also needs fallible per-row access for empty geometry representations. The generic fallback is therefore the correctness baseline, while a selected-row view remains a possible optional fast path. The comparison and full rationale are documented in #9521. ## Validation - `cargo nextest run -p vortex-spatial`: 245 passed. - `cargo clippy -p vortex-spatial --all-targets --all-features -- -D warnings`. - `cargo test --doc -p vortex-spatial`. - `cargo +nightly fmt --all -- --check`. --------- Signed-off-by: Nemo Yu <zyu379@wisc.edu>
Summary
Adds a private correctness fallback for partially valid
RowFnbatches. Direct skip-invalid execution remains the first choice. If an input decoder cannot safely decode the original null rows, or an output sink cannot initialize skipped rows, batch execution filters every input to the jointly valid rows, runs the ordinary dense kernel, and scatters the compact output back to the original row domain.Why filter and scatter
The direct path already invokes the row function only for valid rows. However, it must create each Rust-native input view and the full-length output sink before entering that loop. Some representations cannot create a safe view over unspecified payloads behind nulls. Some sinks also have no meaningful value with which to initialize a skipped row. In either case, simply changing the loop to visit rows one at a time does not provide the missing input or output representation.
Calling the general scalar API once per valid row would work, but it would repeat array execution and scalar construction in the hot loop. A prepared, type-specific selected-row view is a better possible fast path for types that can support one. It is not a complete generic fallback because it does not solve skipped output initialization and needs a fallible per-row access contract for representations such as geometry.
Filter and scatter is not treated as an optimization. Filtering can materialize nested or compressed inputs, and scattering allocates original-length take indices. The executor uses it only after direct execution declines.
Directional ARM strategy comparison
Measured on an Apple M4 Max with Rust 1.98.0,
target-cpu=native, the bench profile, and 100 Divan samples. Values are two-run average medians in microseconds. Lower is better.The prototype confirms that selected-row decoding can win for sparse validity. It is also 45% slower than filter/scatter on the dense polygon control and 10% slower on the ordinary nullable polygon case. This supports keeping selected-row decoding as an optional direct capability instead of making it the general execution model.
These are directional ARM results, not x86 runtime measurements.
Validation
vortex-arraydoctests, formatting, and workspace clippy with all targets and features.