-
Notifications
You must be signed in to change notification settings - Fork 180
feat: support ST_DWithin pushdown in vortex
#8625
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
base: nemo/duckdb-native-geometry
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,15 @@ | |
| #include "duckdb/planner/expression/bound_operator_expression.hpp" | ||
| #include "duckdb/planner/expression/bound_conjunction_expression.hpp" | ||
|
|
||
| #include "duckdb/catalog/catalog.hpp" | ||
| #include "duckdb/catalog/catalog_entry/scalar_function_catalog_entry.hpp" | ||
| #include "duckdb/main/capi/capi_internal.hpp" | ||
| #include "duckdb/main/client_context.hpp" | ||
| #include "duckdb/main/connection.hpp" | ||
| #include "duckdb/parser/parsed_data/create_scalar_function_info.hpp" | ||
|
|
||
| #include <exception> | ||
|
|
||
| using namespace duckdb; | ||
|
|
||
| extern "C" const char *duckdb_vx_sfunc_name(duckdb_vx_sfunc ffi_func) { | ||
|
|
@@ -21,6 +30,40 @@ extern "C" const char *duckdb_vx_sfunc_name(duckdb_vx_sfunc ffi_func) { | |
| return func->name.c_str(); | ||
| } | ||
|
|
||
| extern "C" duckdb_state duckdb_vx_register_geo_aliases(duckdb_database ffi_db) { | ||
| if (!ffi_db) { | ||
| return DuckDBError; | ||
| } | ||
| const DatabaseWrapper &wrapper = *reinterpret_cast<DatabaseWrapper *>(ffi_db); | ||
| try { | ||
| Connection conn(*wrapper.database->instance); | ||
|
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. Why do we need a transaction here? Can we register the function in the global catalog without it?
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. It looks like we cannot not. DuckDB's catalog is MVCC, so |
||
| ClientContext &context = *conn.context; | ||
| context.RunFunctionInTransaction([&]() { | ||
| auto &catalog = Catalog::GetSystemCatalog(context); | ||
| auto &entry = catalog.GetEntry<ScalarFunctionCatalogEntry>( | ||
| context, DEFAULT_SCHEMA, "st_dwithin"); | ||
| // Copy each ST_DWithin overload to a non-throwing `vortex_dwithin` so DuckDB will push it. | ||
| ScalarFunctionSet set("vortex_dwithin"); | ||
| for (const auto &overload : entry.functions.functions) { | ||
| ScalarFunction copy = overload; | ||
| copy.name = "vortex_dwithin"; | ||
| copy.SetErrorMode(FunctionErrors::CANNOT_ERROR); | ||
| // Clear the bind so the radius stays as children[2] for the Vortex converter | ||
| // (ST_DWithin's bind folds it into bind_data). vortex_dwithin is only pushed, never run. | ||
| copy.bind = nullptr; | ||
| set.AddFunction(copy); | ||
| } | ||
| CreateScalarFunctionInfo info(std::move(set)); | ||
| info.on_conflict = OnCreateConflict::IGNORE_ON_CONFLICT; | ||
| catalog.CreateFunction(context, info); | ||
| }); | ||
| } catch (const std::exception &) { | ||
| // No `spatial` loaded, so there is no `ST_DWithin` to alias; nothing to register. | ||
| return DuckDBSuccess; | ||
| } | ||
| return DuckDBSuccess; | ||
| } | ||
|
|
||
| extern "C" const char *duckdb_vx_expr_to_string(duckdb_vx_expr ffi_expr) { | ||
| if (!ffi_expr) { | ||
| return nullptr; | ||
|
|
||
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.
I think CREATE MACRO in init.sql is an easier approach than rewriting the query manually.
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.
Here the queries call
ST_GeomFromWKB/ST_DWithin, which the spatial extension already defines, and you can't shadow those:CREATE MACRO ST_DWithin(...)->Catalog Error: Macro Function with name "ST_DWithin" already exists(same forST_GeomFromWKB, both tested).Uh oh!
There was an error while loading. Please reload this page.
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.
But this means we won't be able to use this function on real queries since you don't have access to the query string in Duckdb. Let's think of another way. Maybe we can delete the spatial function and then create ours? This won't survive extension re-LOAD, but is a temporary good solution.
Otherwise we can write a small pass replacing ST_DWithin with vortex_DWIthin in the graph, but doing nothing else, this should be a single Visitor override.