Skip to content

Conversation

@sgrebnov
Copy link

@sgrebnov sgrebnov commented Dec 20, 2024

Which issue does this PR close?

Allow adding custom scalar to sql overrides for DuckDB, for example

let dialect = DuckDBDialect::new().with_custom_scalar_overrides(vec![
      (
          "cosine_distance",
          Box::new(cosine_distance_to_sql) as ScalarFnToSqlHandler
      ),
  ]);

Concern

  1. This is a breaking change for DuckDB as we are adding new field so DuckDBDialect {} must be replaced with DuckDBDialect::new() constructor, I've tried to avoid this by using Default trait but it does not help, with this change we require to update the way DuckDBDialect is created. Note: DuckDB was added by us so most likely only us are affected by this change.

  2. I was thinking to add fn get_custom_scalar_overrides()->Option<&HashMap<String, ScalarFnToSqlHandler>> to Dialect Trait as an example of what Dialects should use to provide custom overrides but I don't think we should restrict how Dialects should implement this. We have Type ScalarFnToSqlHandler = Box<dyn Fn(&Unparser, &[Expr]) -> Result<Option<ast::Expr>> + Send + Sync> that specifies the signature and this should be enough for dialects IMO.

@sgrebnov sgrebnov self-assigned this Dec 20, 2024
@sgrebnov sgrebnov merged commit 37f0f14 into spiceai-43 Dec 20, 2024
@sgrebnov sgrebnov deleted the sgrebnov/duckdb-dialect-update branch December 20, 2024 06:07
Sevenannn pushed a commit that referenced this pull request Jan 25, 2025
…pache#13915)

* Allow adding custom scalar to sql overrides for DuckDB (#68)

* Add unit test: custom_scalar_overrides_duckdb

* Move `with_custom_scalar_overrides` definition on `Dialect` trait level
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants