Skip to content
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

Support $ as a parameter prefix #1952

Merged
merged 1 commit into from
Sep 3, 2023
Merged

Support $ as a parameter prefix #1952

merged 1 commit into from
Sep 3, 2023

Conversation

Giorgi
Copy link
Contributor

@Giorgi Giorgi commented Aug 23, 2023

This PR adds support for $ as a parameter prefix. I made the changes as described in this comment: #1687 (comment)

I need support for $ because I'm building ADO.NET Provider for DuckDB and DuckDB uses $: Add support for named parameters in prepared statements

@mgravell
Copy link
Member

mgravell commented Aug 23, 2023

Does the existing pseudo-positional parameter support not work? By that I mean: looking at DuckDB, it seems to use positional rather than named parameters - and the ? isn't a prefix - it is the entire thing. Dapper can't work well with that by itself, because it doesn't know how to order things - however, Dapper supports a syntax variant:

where Name = ?name?

which it rewrites as

where Name = ?

but uses the token name to figure out which parameter to add in that position. This works with multiple providers, and it looks like it should work here, without any other changes?

@mgravell
Copy link
Member

Secondary: is there any integration test possible that would should this proposed change actually working against DuckDB? I don't know how DuckDB deploys in terms of whether it can work on the CI server, but something that can at least run on a dev box is desirable.

@Giorgi
Copy link
Contributor Author

Giorgi commented Aug 23, 2023

By that I mean: looking at DuckDB, it seems to use positional rather than named parameters

This is not correct, the upcoming version of DuckDB (0.9.0, scheduled to be released in September) will support named parameters too. This PR added support for named parameters: Add support for named parameters in prepared statements

As for how to run tests against DuckDB, DuckDB is an embedded database and you don't need to deploy anything. The simplest option is to clone the https://github.com/Giorgi/DuckDB.NET repo, checkout Named-Parameters branch and run dotnet build /p:BuildType=Full BuildType=Full tells the build to fetch necessary DuckDB binaries so you don't need to do anything else.

Inside the ParameterCollectionTests.cs file, you will find this commented assertion:

//connection.Execute(queryStatement, new { foo = 1, bar = "test" }).Should().BeGreaterOrEqualTo(1, "Passing parameters as object should work");

The changes in this PR make that assertion successful.

@mgravell mgravell merged commit 8a68070 into DapperLib:main Sep 3, 2023
1 check passed
@Giorgi
Copy link
Contributor Author

Giorgi commented Oct 2, 2023

Thanks for merging the PR. Do you want me to add test cases to Dapper that use DuckDB and test the new prefix?

@Giorgi
Copy link
Contributor Author

Giorgi commented Oct 2, 2023

I already have a test case for it in my repo: Giorgi/DuckDB.NET@c5c2e5d

@mgravell
Copy link
Member

mgravell commented Oct 2, 2023

Ideally "yes", but the real question is: how do we execute those tests? What is required to spin up a duckdb instance? Can it run in CI, or is it dev machine only?

@Giorgi Giorgi mentioned this pull request Oct 3, 2023
@mgravell
Copy link
Member

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