lidavidm commented on code in PR #467:
URL: https://github.com/apache/arrow-adbc/pull/467#discussion_r1110112842
##########
go/adbc/driver/flightsql/flightsql_adbc.go:
##########
@@ -554,10 +556,44 @@ func (c *cnxn) SetOption(key, value string) error {
}
c.timeouts.updateTimeout = timeout
case adbc.OptionKeyAutoCommit:
- return adbc.Error{
- Msg: "[Flight SQL] transactions not yet supported",
- Code: adbc.StatusNotImplemented,
+ autocommit := true
+ switch value {
+ case adbc.OptionValueEnabled:
+ case adbc.OptionValueDisabled:
+ autocommit = false
+ default:
+ return adbc.Error{
+ Msg: "[Flight SQL] invalid value for option "
+ key + ": " + value,
+ Code: adbc.StatusInvalidArgument,
+ }
}
+
+ if autocommit != c.autocommit {
+ // if we're *enabling* autocommit, then commit any
existing
+ // transaction. If we're *disabling* auto commit, start
a
+ // transaction and store it.
+ ctx :=
metadata.NewOutgoingContext(context.Background(), c.hdrs)
+ var err error
+ if autocommit && c.txn != nil {
+ if err = c.txn.Commit(ctx, c.timeouts); err !=
nil {
+ return adbc.Error{
+ Msg: "[Flight SQL] failed to
update autocommit: " + err.Error(),
+ Code: adbc.StatusIO,
+ }
Review Comment:
The C++ driver polls GetSqlInfo on startup (inside AdbcDatabase) and checks
if the server supports transactions in the first place, if not, it just acts
like before/as if transactions aren't supported (feel free to punt that to
another PR though but I think we'll need it eventually)
##########
go/adbc/driver/flightsql/flightsql_statement.go:
##########
@@ -37,11 +37,17 @@ import (
const (
OptionStatementQueueSize = "adbc.flight.sql.rpc.queue_size"
+ // Explicitly set substrait version for Flight SQL
+ // substrait *does* include the version in the serialized plan
Review Comment:
it wasn't at the time and people suggested we add it in case; now it's
vestigial unfortunately
##########
go/adbc/driver/flightsql/flightsql_adbc.go:
##########
@@ -554,10 +556,44 @@ func (c *cnxn) SetOption(key, value string) error {
}
c.timeouts.updateTimeout = timeout
case adbc.OptionKeyAutoCommit:
- return adbc.Error{
- Msg: "[Flight SQL] transactions not yet supported",
- Code: adbc.StatusNotImplemented,
+ autocommit := true
+ switch value {
+ case adbc.OptionValueEnabled:
+ case adbc.OptionValueDisabled:
+ autocommit = false
+ default:
+ return adbc.Error{
+ Msg: "[Flight SQL] invalid value for option "
+ key + ": " + value,
+ Code: adbc.StatusInvalidArgument,
+ }
}
+
+ if autocommit != c.autocommit {
+ // if we're *enabling* autocommit, then commit any
existing
+ // transaction. If we're *disabling* auto commit, start
a
+ // transaction and store it.
+ ctx :=
metadata.NewOutgoingContext(context.Background(), c.hdrs)
+ var err error
+ if autocommit && c.txn != nil {
+ if err = c.txn.Commit(ctx, c.timeouts); err !=
nil {
+ return adbc.Error{
+ Msg: "[Flight SQL] failed to
update autocommit: " + err.Error(),
+ Code: adbc.StatusIO,
+ }
Review Comment:
(The key thing being that the Python driver manager always tries to toggle
it since DBAPI defaults to autocommit off, so if it gets an error besides
NotImplemented it throws an error to the user)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
