-
Notifications
You must be signed in to change notification settings - Fork 208
Implements stmt.named_params #642
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: main
Are you sure you want to change the base?
Conversation
@flavorjones mind taking a look? |
3d265d3
to
8412fb0
Compare
8412fb0
to
c73faa9
Compare
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.
@captn3m0 Thanks for this! I've kicked off CI.
I'd like to see a test for the behavior of this code when given ?NNN
parameters. Do you think the method should return those parameters? I'm not sure what happens if you try to bind a variable to a name like ?13
.
VALUE params = rb_ary_new2(param_count); | ||
|
||
// The first host parameter has an index of 1, not 0. | ||
for (int i = 1; i <= param_count; i++) { |
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.
Just a note for posterity: When given a parameter like ?32760
this code is going to iterate over the numbers in the gap calling sqlite3_bind_parameter_name
for each one. That's pretty inefficient, but I don't see any way to avoid it given the set of functions SQLite provides.
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.
Can add a note about it to the docs.
https://sqlite.org/forum/forumpost/533576942b17bf66 is the best explanation I could find.
Nothing special about 3 digits either, ?1 binds as the first param, and ?1111 happily binds as the 1111th parameter.
stmt = SQLite3::Statement.new(@db, "select ?1")
stmt.bind_param(1, "foo")
assert_equal ["1"], stmt.named_params
stmt.each do |row|
assert_equal ["foo"], row
end My first thought is that these aren't really named parameters and we should just drop them. But then, if you were using a statement like: "select ?2, ?3", you'd be back to the same problem this PR is trying to solve for these One way would be to return a mixed array? [2, "foo", "bar", 3], so that the values in the array can be directly used against |
Spent a bit more time at the problem, and it looks like the And there's no way to differentiate and get a result that says [1,2,50].
I wrote this implementation, which works well as long as params are either numbered or named. Once you add anonymous params to the mix, it goes haywire. Suggestion: |
numeric unused params are undistinguishable from numeric bindable parameters. So a simple loop from 1-bind_parameter_count is the best you can do. This means .named_params can be focused on the truly named parameters only, which is what This commit does by dropping numeric parameters
Pushed a change to ignore numeric and anonymous parameters: stmt = SQLite3::Statement.new(@db, "select ?1, :foo, ?, $bar, @zed, ?250, @999")
assert_equal ["foo", "bar", "zed"], stmt.named_params |
Closes #627
Implemented
named_params
directly in C. Not a C programmer, mistakes are mine. Learned about@
and$
prefixes while reading the docs, so added them to the test as well.However, this implementation ignores
sqlite3_bind_parameter_index
entirely, which is only used internally so shouldn't be an issue(?). As of now, the index of the parameter in the result might not match the value returned bysqlite3_bind_parameter_index
in some cases.But this feels better than having to pad the result with nulls.
Once this is finalized, I can update the changelog if needed