Giorgi / DuckDB.NET

Bindings and ADO.NET Provider for DuckDB
https://duckdb.net
MIT License
343 stars 62 forks source link

Query Parameters #48

Closed unconverged closed 1 year ago

unconverged commented 1 year ago

This pull request brings support for query parameters. With this PR we can now use queries like SELECT ?; and bind a value to '?'.

Parameter placeholders support

It appears that DuckDb does not support named parameters. I.e. we could not use queries like SELECT ?test;. It only supports either anonymous placeholders like ? or indexed placeholders like ?1 or $1. In contrast with SQLite DuckDb does not provide an API to resolve parameter index by its name (SQLite API, list of DuckDb APIs, we do not see anything similar in DuckDb APIs). As a result, this PR does not support named parameters either.

Dapper notes

On the other hand, Dapper tends to prefer named parameters over indexed, because it makes it simpler to use Dapper's object-based parameters magic. Fortunately, Dapper also supports index-based params, so we can use Dapper parameters with DuckDb.

However, there are some restrictions here. DuckDb supports three methods to define a parameter placeholder: ?, ?1 and $1. Dapper fully supports only the first one. That means, that only using ? placeholders we can use both object-based and DynamicParameters-based Dapper parameters. Placeholders like ?1 and $1 work only for DynamicParameter-based. That is because Dapper has special handling for placeholders like ?, considering them OleDb-like placeholders. In case of ?1 Dapper looks for a property with name 1, which cannot exist in a C# object.

Therefore, the simples approach is just to use ? as placeholders, like this:

connection.Execute("INSERT INTO test VALUES (?, ?);", new {x = 1, y = 2});

Parameter ordering

Since DuckDb only supports index-based parameters, we have to put the parameter values in the correct order, so that parameter value matches the corrsponding placeholder. In case of DynamicParameters object, we need to .Add the values in the correct order.

var dp = new DynamicParameters();
dp.Add("param2", 1); // will match the first ? or ?1 or $1
dp.Add("param1", "test"); // will match the second ? or ?2 or $2

In case of an object, we should declare the fields in the correct order.

connection.Execute("INSERT INTO test VALUES (?, ?);", new {x = 1, y = 2}); // `x` will match the first ? and `y` will match the second ?
Giorgi commented 1 year ago

Also, the DuckDBParameter class needs strongly typed constructors too.

unconverged commented 1 year ago

Reimplemented query preparation & execution. Now these are two separate actions, so we can prepare a command and then reuse it with another params.

Giorgi commented 1 year ago

Thanks! Can you edit the PR so that the target branch is develop? I want to review it once again and make some changes before merging to main.

Giorgi commented 1 year ago

@IvaYan By the way, tests are failing locally too.

Giorgi commented 1 year ago

I think you aren't disposing handles returned from DuckDB and that's causing the db file to remain locked.

unconverged commented 1 year ago

Possibly so. That's what I was investigating before you've merged the PR. I guess, you shouldn't have merged it until after I make the tests pass. As a wild guess -- it seems that Prepare() does not dispose the existing prepared statement and simple overwrites it, even it should not prepare anything.

Giorgi commented 1 year ago

I fixed the prepared statement not being disposed. The tests are now passing locally but for some reason still failing on GH

unconverged commented 1 year ago

As far as I can see, tests fail with a message 'Expected scalar to be 42, but found "42".'

I've faced this problem before and that's why I've changed the expected value to string from an int. You changed it back here. DuckDB indeed returns a string there. The root cause is that both of us use DuckDB 0.4 on out local machines but for some reason 0.3.4 is used on the CI. Once I have replaced DuckDB 0.4 with 0.3.4, DuckDb returned a string and the test failed locally.

unconverged commented 1 year ago

Probably we should decide which DuckDb version we actually support and use it everywhere.

Giorgi commented 1 year ago

I updated tests to use v0.4.0. We support only the latest release until DuckDB hits a stable release.

unconverged commented 1 year ago

And the tests have finally passed.