Skip to content

fix(flightsql): reject unsupported transaction options - #1152

Merged
zeroshade merged 3 commits into
apache:mainfrom
fallintoplace:fix/flightsql-tx-options
Aug 12, 2026
Merged

fix(flightsql): reject unsupported transaction options#1152
zeroshade merged 3 commits into
apache:mainfrom
fallintoplace:fix/flightsql-tx-options

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

What

  • Connection.BeginTx used sql.TxOptions instead of driver.TxOptions.
  • Because of that, Connection did not implement driver.ConnBeginTx and database/sql never called the method.
  • Use driver.TxOptions so database/sql forwards the caller context and transaction options.
  • Return ErrNotSupported for non-default isolation levels and read-only transactions because FlightSQL does not support configuring them.
  • Add a compile-time interface check and test the behavior through sql.DB.BeginTx.

Test

  • go test ./arrow/flight/flightsql/driver -run TestBeginTxRejectsUnsupportedOptions -count=1
  • go test -race ./arrow/flight/flightsql/driver -run TestBeginTxRejectsUnsupportedOptions -count=1
  • go test ./arrow/flight/flightsql/... -count=1
  • go test ./arrow/flight/... -count=1
  • go vet ./arrow/flight/flightsql/driver

@fallintoplace
fallintoplace marked this pull request as ready for review August 8, 2026 15:52

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The option validation itself is correct, but the method signature prevents database/sql from dispatching to it. This leaves the intended integration behavior unchanged and makes the new test cover only direct concrete-method calls.

I ran the focused test, focused race test, FlightSQL subtree, and broader Flight package on head 4c51edded1ab; all passed. The explicit driver.ConnBeginTx compile-time assertion fails as described inline. CI is still running.


This review was drafted by an AI-assisted tool and confirmed by an Apache Arrow Go maintainer. The maintainer posting this review has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.

More on how Apache Arrow Go handles maintainer review: CONTRIBUTING.md.

}

func (c *Connection) BeginTx(ctx context.Context, opts sql.TxOptions) (driver.Tx, error) {
if opts.Isolation != sql.LevelDefault || opts.ReadOnly {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is not reached through database/sql: Connection.BeginTx accepts sql.TxOptions, but driver.ConnBeginTx requires driver.TxOptions. Consequently, *Connection does not implement driver.ConnBeginTx; database/sql bypasses this method, rejects non-default options with its own generic errors, and falls back to Begin() for default transactions without forwarding the caller’s context. Please expose the proper interface hook—directly or through an adapter—add a compile-time interface assertion, and exercise the behavior through sql.DB.BeginTx.

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The driver.ConnBeginTx dispatch issue is fixed, but the implementation changes the signature of an exported v18 method and breaks existing concrete callers. See the inline comment on driver.go.

The corrected integration behavior is otherwise sound. The focused test passed 100 repetitions, race passed 20 repetitions, and the driver, FlightSQL, 32-bit, and vet checks all passed. CI is 23/23 green.


This review was drafted by an AI-assisted tool and confirmed by an Apache Arrow Go maintainer. After you've addressed the point above and pushed an update, an Apache Arrow Go maintainer — a real person — will take the next look at the PR. If you think the finding is mis-applied, please reply on the PR and a maintainer will weigh in.

More on how Apache Arrow Go handles maintainer review: CONTRIBUTING.md.

Comment thread arrow/flight/flightsql/driver/driver.go Outdated
}

func (c *Connection) BeginTx(ctx context.Context, opts sql.TxOptions) (driver.Tx, error) {
func (c *Connection) BeginTx(ctx context.Context, opts driver.TxOptions) (driver.Tx, error) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this exported method from sql.TxOptions to driver.TxOptions is source-incompatible within v18. A downstream call that compiled previously now fails:

cannot use sql.TxOptions{} as driver.TxOptions in argument to
(*Connection).BeginTx

Please avoid silently breaking the concrete API. One option is a private adapter returned by Connector.Connect that implements driver.ConnBeginTx and translates its options into the existing Connection.BeginTx method. That approach changes the returned concrete type, so whichever compatibility tradeoff is chosen should be explicit.

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both prior blockers are resolved. The private connBeginTx adapter preserves the exported Connection.BeginTx(context.Context, sql.TxOptions) API while correctly implementing driver.ConnBeginTx for connections returned through database/sql.

The external compatibility probe, repeated integration and race tests, FlightSQL subtree, 32-bit build, vet, and all 23 CI checks pass. I found no new issues in the updated implementation.


This review was drafted by an AI-assisted tool and confirmed by an Apache Arrow Go maintainer. The maintainer approving this PR has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.

More on how Apache Arrow Go handles maintainer review: CONTRIBUTING.md.

@zeroshade
zeroshade merged commit 986b027 into apache:main Aug 12, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants