-
Notifications
You must be signed in to change notification settings - Fork 60
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
sql: support prepared statements #180
Conversation
e2d11e0
to
7e5c57c
Compare
I am not sure about this part of API: func (conn *Connection) Prepare(expr string) (resp *Response, err error) Maybe we should return func (conn *Connection) Prepare(expr string) (uint64, err error) The 1st option is considered due to its consistency with other connector's methods. |
7e5c57c
to
8f8f5f3
Compare
What can be useful to do with a response? If the response is needed only to get the StmtID, then it is better to choose the second one. Also, would it be better to create |
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.
Thank you for the pull request. It seems to me that there is a missing method Unprepare
. Also, I don't like using the method Connection.Execute
for two cases based on an argument type (but it less critical).
8f8f5f3
to
f42b5a8
Compare
5a117ab
to
14bc31e
Compare
14bc31e
to
2f5dd73
Compare
Good news! You can rebase to the master branch. |
e825bbf
to
6b7fe6b
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.
Thank you for your work! It seems to me that now the API is not consistent with request objects API. Could you do that, please?
7234485
to
8a538fa
Compare
Purposes:
A public API proposal: // PreparedStatement
type PreparedStatement {
Conn Connection
StatementID PreparedStatementID
MetaData []ColumnMetaData
ParamCount uint64
}
func NewPreparedStatement(conn Connection, StatementID PreparedStatementID, MetaData []ColumntMetaData, ParamCount uint64) (*PreparedStatement, error)
func NewPreparedStatementFromResponse(resp Response) *PreparedStatement
// Connection
func (conn *Connection) Prepare(expr string) (resp *Response, err error)
func (conn *Connection) PrepareTyped(expr string, stmt interface{}) err error // ?
func (conn *Connection) PrepareAsync(expr string) *Future
// Request objects
type PrepareRequest {
baseRequest
}
type PreparedStatementRequest interface {
Request
GetPreparedStatement() *PreparedStatement
}
type ExecutePreparedStatementRequest {
PrepearedStatementRequest
}
type UnpreparePreparedStatementRequest {
PrepearedStatementRequest
} Usage example: // stmt := conn.Do(NewPrepareRequest("statement string")).Get().Data.(PreparedStatement)
resp, err := conn.Do(NewPrepareRequest("statement string")).Get()
stmt, err := NewPreparedStatementFromResponse(resp)
conn.Do(NewExecutePrepareStatementRequest(stmt))
conn.Do(NewUnpreparePreparedStatementRequest(stmt))
// stmt := conn.Do(NewPrepareRequest("statement string")).Get().Data.(PreparedStatement)
resp, err := pool.Do(NewPrepareRequest("statement string")).Get()
stmt, err := NewPreparedStatementFromResponse(resp)
pool.Do(NewExecutePrepareStatementRequest(stmt, args))
pool.Do(NewUnpreparePreparedStatementRequest(stmt)) We can create a PreparedStatement object here: Lines 68 to 69 in 988edce
We can add a wrappers for PreparedStatement. it's a bit redundant, but it's looks like a Lua interface: func (stmt *PreparedStatement) Execute(args interface{}) (resp Response, err error)
func (stmt *PreparedStatement) ExecuteTyped(args interface{}, tup interface{}) (err error)
func (stmt *PreparedStatement) ExecuteAsync(args interface{}) (*Future)
func (stmt *PreparedStatement) Unprepare() (resp Response, err error)
func (stmt *PreparedStatement) UnprepareTyped(tup interface{}) (err error) // ?
func (stmt *PreparedStatement) UnprepareAsync() (*Future) In addition, we can replace As an alternative we can add a method: func (stmt *PreparedStatement) Do(req PreparedStatementRequest) (*Future) The approach (only Connection.Do, or ConnectionDo() + PreparedStatement.Do()) should be synchronized with #185 . See: |
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.
I have no critical complaints. Thanks for the great work done!
LGTM after resolving all my active conversations.
b372429
to
bf8c321
Compare
bf8c321
to
0faa7fd
Compare
Added ExecuteTyped/ExecuteAsync implementation to multi package. CHANGELOG.md updated. Follows up #62
e4bbf22
to
afc5571
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.
We're almost there, final push!
490e5f1
to
383422c
Compare
3ae8598
to
2397473
Compare
This patch adds the support of prepared statements. Added a new type for handling prepared statements. Added new IPROTO-constants for support of prepared statements in const.go. Added benchmarks for SQL-select prepared statement. Added examples of using Prepare in example_test.go. Fixed some grammar inconsistencies for the method Execute. Added a test helper for checking if SQL is supported in connected Tarantool. Updated CHANGELOG.md. Added mock for ConnectedRequest interface for tests. Follows up #62 Closes #117
2397473
to
334aaf4
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.
Thank you for the great work done!
I apologize for being too picky, and not being accurate in my suggestions. I will try to improve the review process in the future. But I believe we have achieved the goal - users will like the result.
Thank you for detailed review! |
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.
LGTM!
What has been done? Why? What problem is being solved?
This patch adds the support of prepared statements.
Added a new type for handling prepared statements.
Added new methods for connector's interface in connector.go.
Added new IPROTO-constants for support of prepared statements
in const.go. Updated multi-package for corresponding it to connector's
interface.
Added a new function for checking the count of prepared statements in
config.lua in tests for Prepare and Unprepare.
Added benchmarks for SQL-select prepared statement.
Added examples of using Prepare in example_test.go.
Fixed some grammar inconsistencies for the method Execute.
Updated CHANGELOG.md.
I didn't forget about (remove if it is not applicable):
Related issues:
Follows up #62
Closes #117
Considered variants of API
v1
Prepared Statement is just an object only having a constructor.
All requests get constructed with request objects.
Single connection usage:
For connection pool it is complicated to use without any method that returns
a connection from pool explicitly.
Consider the example:
Implementation:
Advantages:
Disadvantages:
v2
Prepared Statement object is mapped from lua interface
with specific extensions for connectors interface.
It has own methods for execute and unprepare actions.
Connector interface:
Single connection usage:
Connection pool usage:
Implementation:
request.go
:connection.go
:Advantages;
Disadvantages:
v3 - the final one
Try to mix both approaches.
We leave the option for constructing prepared statement manually with request objects.
(But it doesn't work well with connection_pool without some method like pool.GetConnectionFromPool())
We give a user more friendly API with wrappers for request objects.
Connector interface:
Single connection Usage
Single connection usage:
Connection pool usage:
Advantages:
Disadvantages:
v4 - the proposed one
Purposes:
A public API proposal:
Usage example:
In addition, we can replace
PreparedStatement
with a shortest naming. Maybe justPrepared
?See: