You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zeroshade (via GitHub)" <gi...@apache.org> on 2023/02/16 21:01:12 UTC
[GitHub] [arrow-adbc] zeroshade opened a new pull request, #467: feat(go/adbc/driver/flightsql): add transaction and substrait support
zeroshade opened a new pull request, #467:
URL: https://github.com/apache/arrow-adbc/pull/467
Relies on https://github.com/apache/arrow/pull/34220 for the tests to work (that PR adds Transaction Support to the example SQLite flight sql server)
--
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
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #467: feat(go/adbc/driver/flightsql): add transaction and substrait support
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #467:
URL: https://github.com/apache/arrow-adbc/pull/467#discussion_r1110215855
##########
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:
Gotcha, i'll update this here to check GetSqlInfo on startup and return NotImplemented if that's the case.
--
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
[GitHub] [arrow-adbc] lidavidm commented on a diff in pull request #467: feat(go/adbc/driver/flightsql): add transaction and substrait support
Posted by "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
[GitHub] [arrow-adbc] lidavidm merged pull request #467: feat(go/adbc/driver/flightsql): add transaction and substrait support
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #467:
URL: https://github.com/apache/arrow-adbc/pull/467
--
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
[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #467: feat(go/adbc/driver/flightsql): add transaction and substrait support
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #467:
URL: https://github.com/apache/arrow-adbc/pull/467#discussion_r1110324299
##########
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:
updated
--
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