perf(rust): point-coordinate fast paths for ST_Distance/ST_DWithin/ST_Azimuth#1008
perf(rust): point-coordinate fast paths for ST_Distance/ST_DWithin/ST_Azimuth#1008james-willis wants to merge 3 commits into
Conversation
fa894dd to
f44a839
Compare
paleolimbot
left a comment
There was a problem hiding this comment.
Cool!
I know you're still working here, but we have something called a GeometryFactory (parameterized by executor) that might be a good fit here...it handles the "preparation" of scalars to ensure we don't parse WKB more than needed. Maybe a PointXYGeometryFactory / GenericExecutor<PointXYGeometryFactory> would make this trivial to scale out to other places.
…Distance/ST_DWithin Add PointXYGeometryFactory to sedona-functions: a GeometryFactory whose Geom is a PointOrWkb — a finite Point decoded straight from its fixed WKB offset (no full parse), or a parsed Wkb for anything else (incl POINT EMPTY). The GenericExecutor parses each scalar once, so kernels get parse-once-for-scalars for free. Adopt it in ST_Distance (Point/Point computes Euclidean directly; mixed Point/geometry uses a geo_types::Point so the Point is never re-parsed) and ST_DWithin (shares the same backend, distance <= bound). Replaces the hand-rolled PreparedScalar scalar-prep. Adds point/point ST_DWithin benchmarks. ST_X/ST_Y deliberately stay on their direct read_point_xy fast path: the factory enum adds per-element overhead that regresses the already-optimal unary case (~+30% in benchmarks), with no parse savings to offset it.
f44a839 to
7f337d1
Compare
paleolimbot
left a comment
There was a problem hiding this comment.
Thank you!
The low level WkbPointLayout would benefit from a quick test covering the type of points it supports (especially errors resulting from incomplete buffers, including EWKB), with apologies if I missed this test.
| impl WkbPointLayout { | ||
| /// Inspects a WKB header, returning the Point layout or `Ok(None)` if `buf` | ||
| /// is not a Point. Reads only the byte order and type code (recognizing the | ||
| /// EWKB SRID prefix) — never a coordinate. Errors only on a missing/invalid | ||
| /// byte order or a truncated type code. Accepts both ISO (with Z/M dimension | ||
| /// flags) and EWKB (with Z/M/SRID flags) encodings. | ||
| #[inline] | ||
| pub fn try_from_wkb(buf: &[u8]) -> Result<Option<Self>, SedonaGeometryError> { |
There was a problem hiding this comment.
This is great!
Because it's doing some byte parsing / is indexing into buffers in a way that could panic for an out of bounds read, I think it needs at least one test to cover the branches. Hopefully there are some in this file to draw from (if not, there are some fixtures in sedona-testing).
Introduces
PointXYGeometryFactory(insedona-functions): aGeometryFactorywhoseGeomis aPointOrWkb— a finite Point decoded straight from its fixed WKB offset (no full parse), or a parsedWkbfor anything else (incl.POINT EMPTY). BecauseGenericExecutorparses each scalar once, kernels built on it get parse-once-for-scalars for free. Replaces the hand-rolledPreparedScalarscalar-prep that was here before.Adopted in the planar point-coordinate distance functions:
geo_types::Pointso the Point is never re-parsed.distance <= bound); this also speeds the spatial-join distance predicate transitively.Results vs
main(criterion):(The mixed Point/geometry cases improve too, since the factory computes point-to-geometry without re-parsing the Point.)
ST_Azimuth applies the same "skip the parse for points" idea but lands on the lighter primitive. It is defined only for two non-empty POINTs (non-Point and
POINT EMPTYare errors, not a fallback), so it is coordinate-extraction-shaped rather than a distance kernel: it reads each operand's(x, y)viaread_point_xyon aWkbBytesExecutor— no parse, no enum. Benchmarked head-to-head against the factory, the accessor wins the dominant two-point-column shape:mainBoth variants beat the full-parse baseline everywhere; the accessor is chosen for the ArrayArray win (the largest absolute saving and the canonical origin→destination shape) and to keep the split consistent — accessors read via
read_point_xy, distance/fallback kernels use the factory.ST_X / ST_Y are deliberately left on their existing
read_point_xyfast path. Migrating them to the factory regressed them ~30% in benchmarks: thePointOrWkbenum (sized to itsWkbvariant) adds per-element overhead that, for the already-optimal unary case, isn't offset by any parse savings. So the factory is the right tool for the binary distance kernels but not for the unary accessors.Geography distance/dwithin/maxdistance (s2geography) are a separate follow-up.