-
Notifications
You must be signed in to change notification settings - Fork 121
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
Expose conn and stmt for advanced usage #310
Conversation
Hi @fanyang01, thanks for your PR :) I don't quite follow how you call your new functions by exposing the raw driver connection. How do you retrieve the prepared
That would help me better understand why you want to expose both
PRs against duckdb main are always welcome - you could tag me on it. |
In general, I am not opposed to exposing the connection and the statement. |
Tests added :) In short, this PR allows me to do something like:
Sure, stay tuned! |
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.
Hi, @fanyang01, thanks for adding the tests. I've had another more detailed pass over your changes. Thanks also for adding more comments to the existing functions!
One thing I was wondering: this PR is becoming quite involved and touches a lot of critical-ish code paths. Would it be possible to split it into a few smaller PRs? E.g.,
- Expose
Conn
andStmt
and the comments. ParamName
,ParamType
, andStatementType
and their tests.- New exposed low-level API calls for binding and their tests.
That would make it easier for me to review and know what's happening - no worries if it is too much work. Then, we'll work with this PR. :smiling:
if s.closed { | ||
return errors.Join(errCouldNotBind, errClosedCon) | ||
} |
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.
Missing test.
func (s *stmt) QueryContext(ctx context.Context, nargs []driver.NamedValue) (driver.Rows, error) { | ||
res, err := s.execute(ctx, nargs) | ||
if err != nil { | ||
// QueryContext executes a query that may return rows, such as a SELECT. | ||
// It implements the driver.StmtQueryContext interface. | ||
func (s *Stmt) QueryContext(ctx context.Context, nargs []driver.NamedValue) (driver.Rows, error) { | ||
if err := s.Bind(nargs); err != nil { |
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.
Am I missing something here? It looks like we changed the behavior of QueryContext
: Instead of execute
, it now performs Bind
and QueryBound
.
Hi @taniabogatsch, thank you very much for the detailed review! I will split this PR into several smaller ones, as you suggested, later this week. |
Hi @taniabogatsch,
This PR exposes
conn
andstmt
, enabling access according to Go’s visibility rules. Additionally, it makes more information aboutstmt
accessible.The motivation for this comes from the MyDuck Server project, which I’m actively developing. MyDuck Server allows users to run DuckDB as a server that supports MySQL and Postgres wire protocols, leveraging go-duckdb extensively—and it’s been working well so far. Thanks for your continued maintenance!
Currently, I'm implementing prepared statement support for the Postgres protocol, which requires access to parameter and return types for prepared statements (the client sends
Describe
messages to the server to retrieve this info). With the changes in this PR, I would be able to retrieve parameter types using the Conn.Raw method.However, the C API still lacks access to the return types of a prepared statement (though it is available in the C++ API). I’m considering a workaround, like executing the query with
LIMIT 0
to infer return types.Thanks for considering this PR!