[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #467: feat(go/adbc/driver/flightsql): add transaction and substrait support

["lidavidm (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org