feat: support ST_DWithin pushdown in vortex#8625
Conversation
Merging this PR will improve performance by 11.89%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | chunked_varbinview_into_canonical[(100, 100)] |
272.2 µs | 306.9 µs | -11.31% |
| ⚡ | Simulation | chunked_varbinview_opt_canonical_into[(1000, 10)] |
206.4 µs | 169.8 µs | +21.57% |
| ⚡ | Simulation | chunked_varbinview_into_canonical[(1000, 10)] |
205.9 µs | 169.5 µs | +21.45% |
| ⚡ | Simulation | chunked_varbinview_opt_into_canonical[(1000, 10)] |
219.9 µs | 183.6 µs | +19.79% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[128] |
273.6 ns | 244.4 ns | +11.93% |
| ⚡ | Simulation | encode_varbin[(1000, 32)] |
162.4 µs | 145.3 µs | +11.79% |
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-native-pushdown (20efa07) with develop (ee2cd67)
Footnotes
-
4 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. ↩
HarukiMoriarty
force-pushed
the
nemo/duckdb-native-geometry
branch
2 times, most recently
from
bc74778 to
8e0738a
Compare
HarukiMoriarty
force-pushed
the
nemo/geo-native-pushdown
branch
from
94674e0 to
c87704d
Compare
Single-table `ST_DWithin(col, literal, radius)` filters over native-geometry Vortex scans now push down as `vortex.geo.distance(..) <= radius`, evaluated natively via a `GeoDistance` point fast path — with no query rewriting; any query using `ST_DWithin` benefits. Genuine `ST_DWithin` cannot push as-is: spatial's bind folds the radius into opaque bind data, while its SPATIAL_JOIN optimization requires exactly that folded 2-argument form. The pieces: - `duckdb_vx_register_st_dwithin_override` shadows `ST_DWithin` in the user catalog with a copy whose bind is cleared, so bound calls keep the radius as `children[2]`. Unpushed occurrences still execute correctly through spatial's own 3-column code path. - The expression converter lowers the 3-argument `st_dwithin` (and `st_distance`) to `GeoDistance`, guarded by the scan's fields: geometry columns must be native (`vortex.geo.wkb` columns fall back to DuckDB). `can_push_expression` dry-runs the lowering so DuckDB never installs an ExpressionFilter the scan would later drop. - `RestoreStDWithin`, in the existing Vortex optimizer pass, rebinds every remaining 3-argument `st_dwithin` (join conditions, unpushed filters) through spatial's original entry, restoring the folded form its spatial-join optimization requires. duckdb-bench registers the override once per connection, after `LOAD spatial`; it is a no-op when spatial is absent. SF1.0: Q1 47ms (parquet) / 16ms (vortex WKB) / 5.4ms (native, pushed); Q8 keeps SPATIAL_JOIN at ~89ms on the native lane. Signed-off-by: Nemo Yu <zyu379@wisc.edu>
HarukiMoriarty
force-pushed
the
nemo/geo-native-pushdown
branch
from
c87704d to
7f6e388
Compare
|
|
||
| static void VortexOptimizeFunction(OptimizerExtensionInput &input, unique_ptr<LogicalOperator> &plan) { | ||
| plan = TryPushdownScalarFunctions(input.context, std::move(plan)); | ||
| // Runs before spatial's own optimizer pass: this extension registers at database open, |
There was a problem hiding this comment.
Do we guarantee it runs before extension's optimizer pass? Do we have a total order on that? If not, I'd suggest using a pre_optimize_function in vortex extension (second parameter to OptimizerExtension) and put this pass there.
There was a problem hiding this comment.
Done as suggested.
| } | ||
|
|
||
| ExpressionPtr VisitReplace(BoundFunctionExpression &expr, ExpressionPtr *) override { | ||
| if (expr.children.size() != 3 || !StringUtil::CIEquals(expr.function.name, "st_dwithin")) { |
There was a problem hiding this comment.
Duckdb already compares identifiers case-insensitively, we can use == here.
- Move the ST_DWithin restore pass to the optimizer extension's pre_optimize_function. DuckDB runs all extensions' pre-optimize hooks before any post-optimize pass, so this guarantees the restore precedes spatial's spatial-join optimization without relying on registration order. Since it now runs before filter pushdown, restrict it to join conditions so filters keep the visible radius they need to push. - Compare the function name with `==`; DuckDB identifiers are already case-insensitive. - Terser doc comment on `duckdb_vx_register_st_dwithin_override`. - Drop a debug print from the geo function converter. Signed-off-by: Nemo Yu <zyu379@wisc.edu>
Signed-off-by: Nemo Yu <zyu379@wisc.edu>
| self.connection().query(stmt)?; | ||
| } | ||
| // After `LOAD spatial`, shadow `ST_DWithin` so radius filters push. No-op without it. | ||
| self.db |
There was a problem hiding this comment.
Do we want to register geo-overrides only for the bench or for users in general?
If it's the latter, we should move the initialization out of benchmark
There was a problem hiding this comment.
Let's make it internal for benchmark only for now, later I will systematically applied into vortex-duckdb.
| * Rebind 3-argument `st_dwithin` join conditions (bound via the Vortex override; see expr.h) | ||
| * back through spatial's original entry, whose folded 2-argument form its spatial-join | ||
| * optimization requires. Runs in the pre-optimize hook, which is guaranteed to precede every | ||
| * extension's post-optimize pass; filters are left untouched so they can push into Vortex scans. |
There was a problem hiding this comment.
To be honest, I don't understand this comment. Maybe we should add an example showing what's done somewhere (i.e. what function gets overriden and restored), and reference it in other places?
There was a problem hiding this comment.
Let me add one example. The root cause here is that the DuckDB Spatial extension optimizer only recognizes the two-argument version of ST_DWithin as optimizable, allowing it to use a spatial index. If we keep the manually written three-argument version, DuckDB will fail to apply this optimization and will instead execute Query 8 using brute force.
| }); | ||
| } catch (const std::exception &) { | ||
| // No `spatial` loaded, so there is no `ST_DWithin` to override; nothing to register. | ||
| return DuckDBSuccess; |
There was a problem hiding this comment.
What if ModifyDatabase or other calls failed? We want to propagate an error to the user
| struct ConvertCtx<'a> { | ||
| /// Substituted for `BoundRef` references when converting scan-scoped table filters. | ||
| col_sub: Option<&'a Expression>, | ||
| /// The scan's fields, when known; geo lowering requires them to verify a geometry column. |
There was a problem hiding this comment.
Nit: I think the user can infer why this field is needed, let's remove the "why" from the comment
…comments Address PR review comments: - Treat only a missing `st_dwithin` entry (no `spatial` loaded) as a no-op; real registration failures now log the message and return an error. - Rewrite the `RestoreStDWithin` comment with an override/restore example and reference it from expr.h. - Trim the `ConvertCtx::fields` doc comment. Signed-off-by: Nemo Yu <zyu379@wisc.edu>
3 participants
Summary
Insert non-throwing geo predicate
vortex_dwithinin DuckDB, which later pushdown into vortex, calldistancescalar function on scanning, significantly improve Q1/Q3 performance in SpatialBench.What changes are included in this PR?
vortex_dwithinin DuckDB, which is non-throwing and can be pushdown.ST_dwithinis text rewritten intovortex_dwithin.Performance
Takeaways: Q1 and Q3 is significantly improved due to the single table geo predicate is pushdown into vortex scan. Q1 is more benefit from the manually fast path without going through
geocrate for calculation. Q3 is also potentially can be benefit from manually calculation.