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

[C-API + CLI] Add support for named parameters in prepared statements #7113

Merged
merged 68 commits into from
Aug 7, 2023

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented Apr 17, 2023

This PR extends the functionality introduced in #5582 and implements the request made in #7076

Implementation changes / API extension

Old way

Previously the named parameter support was implemented by preparing the query, using the PreparedStatements named_parameter map to manually transform the provided data into a vector<Value> that maps the values to the right index.

We then cleared the map, and when it got bound we verify that the map is empty, otherwise we throw an exception saying that named parameters are not supported in this client.

This was only supported in the Python client.

Problems

This put the responsibility on the client to verify all the error conditions, such as the statement expects named values but they weren't provided, some are missing, too many values are provided, etc..
That also means that when this isn't checked, we could end up accepting a regular value even when we expected a named value, which shouldn't happen.

Solution

Now we have a new PendingQuery overload that takes a case_insensitive_map_t<Value> parameter, this enables us to use named parameters.
The verification of the named parameters is moved to core, and is no longer the responsibility of the client.
This change propagates throughout the entire system, meaning we don't store parameters internally as a vector anymore, but as a case_insensitive_map_t (unordered_map<string, ...>)

CLI

While working on this change, I also took the opportunity to add named parameters to the CLI:

This means this works now:

prepare q123 as select $param, $other_name, $param;
execute q123(param := 5, other_name := 3);

Result:

5	3	5

C-API

Similar to SQLite, we now have duckdb_bind_parameter_index, which retrieves the internal mapping from name -> parameter index, this also registers the value as being named, for verification purposes.

/*!
Retrieve the index of the parameter for the prepared statement, identified by name
*/
DUCKDB_API duckdb_state duckdb_bind_parameter_index(duckdb_prepared_statement prepared_statement, idx_t *param_idx_out,
                                                    const char *name);

See test/api/capi/test_capi_prepared.cpp for an example of how this is used

"Test prepared statements with named parameters in C API"

Footnote:

Mixing named and unnamed prepared parameters are still not supported, but the possibility of this has been taken into account while making these changes, so it should still be relatively low hanging fruit to add support for this.

@Tishj
Copy link
Contributor Author

Tishj commented Apr 17, 2023

Hmm we do lose some existing functionality, now that we occupy the ComparisonEqual expression for named parameters, so I've had to edit an existing test.

Mark mentioned using param := 5 but that's currently a syntax error, maybe I should add support to the parser for that so we don't have to lose existing functionality.

EDIT: fixed

Copy link
Collaborator

@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I left some comments below. I wonder if it makes sense to unify the positional and named parameters. Positional parameters could be names that are just numbers internally, e.g. SELECT $1, $2 would translate to the names "1", "2". That might allow for code to be cleaned up to avoid having to think about these as separate things in many layers.

src/include/duckdb/main/capi/capi_internal.hpp Outdated Show resolved Hide resolved
tools/pythonpkg/tests/fast/api/test_duckdb_query.py Outdated Show resolved Hide resolved
src/include/duckdb.h Show resolved Hide resolved
src/main/capi/prepared-c.cpp Outdated Show resolved Hide resolved
@Tishj
Copy link
Contributor Author

Tishj commented Apr 25, 2023

This is still broken currently, dealing with one last hurdle unifying the named and positional parameters.
Since we need to keep the $<number> -> param_idx as a direct mapping for C-API reasons, this complicates the $<name> situation.

We can't assign the named parameters an index until we've seen all positional parameters

I can't come up with a neat solution for this problem,
maybe I could save references to the created parameter expressions, and update their indices after the fact, but that sounds dirty.

I also thought about just adding the name, and only bind them later, but we use the "unbound" parameter index in the C-API.

Maybe we have to keep the limitation that you can't mix positional/named parameters for now

EDIT:
We still don't support mixing named and positional parameters for now

@Tishj Tishj force-pushed the named_prepared_parameters branch from a62ff95 to 99940e2 Compare July 24, 2023 12:21
@Tishj Tishj force-pushed the named_prepared_parameters branch from 9e52b63 to a85b9be Compare July 25, 2023 09:21
@Tishj Tishj marked this pull request as ready for review August 3, 2023 11:49
@github-actions github-actions bot marked this pull request as draft August 3, 2023 15:22
@Tishj Tishj marked this pull request as ready for review August 5, 2023 17:24
@@ -63,7 +63,7 @@ class PreparedStatement {
//! Returns the result names of the prepared statement
DUCKDB_API const vector<string> &GetNames();
//! Returns the map of parameter index to the expected type of parameter
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should be updated

@@ -72,18 +72,86 @@ class PreparedStatement {
return PendingQueryRecursive(values, args...);
}

//! Create a pending query result of the prepared statement with the given set of arguments
DUCKDB_API unique_ptr<PendingQueryResult> PendingQuery(vector<Value> &values, bool allow_stream_result = true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be const references I feel

bool allow_stream_result = true);

//! Execute the prepared statement with the given set of values
DUCKDB_API unique_ptr<QueryResult> Execute(vector<Value> &values, bool allow_stream_result = true);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for these

@Mytherin Mytherin merged commit c140303 into duckdb:master Aug 7, 2023
53 checks passed
@Mytherin
Copy link
Collaborator

Mytherin commented Aug 7, 2023

Thanks! LGTM

hannes added a commit that referenced this pull request Aug 11, 2023
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.

None yet

3 participants