Skip to content

fix(go/adbc/drivermgr): adjust ingest helper to set target before BindStream - #4308

Merged
lidavidm merged 3 commits into
apache:mainfrom
arnoldwakim:fix/go-adbc-ingest-helper
May 21, 2026
Merged

fix(go/adbc/drivermgr): adjust ingest helper to set target before BindStream#4308
lidavidm merged 3 commits into
apache:mainfrom
arnoldwakim:fix/go-adbc-ingest-helper

Conversation

@arnoldwakim

Copy link
Copy Markdown
Contributor

Summary

Reorder IngestStream and IngestStreamContext to set OptionKeyIngestTargetTable and OptionKeyIngestMode before calling BindStream, matching drivers that require ingest targets up front (e.g., FlightSQL).

Keep all other ingest option handling and execution flow unchanged.

Evidence (FlightSQL requirement):

go/adbc/driver/flightsql/flightsql_statement.go:630-656:

func (s *statement) BindStream(_ context.Context, stream array.RecordReader) error {
	// For bulk ingest, bind to the statement
	if s.targetTable != "" {
		if s.bound != nil {
			s.bound.Release()
			s.bound = nil
		}
		if s.streamBind != nil {
			s.streamBind.Release()
		}
		s.streamBind = stream
		if s.streamBind != nil {
			s.streamBind.Retain()
		}
		return nil
	}

	if s.prepared == nil {
		return adbc.Error{
			Msg:  "[Flight SQL Statement] must call Prepare or set IngestTargetTable before calling Bind",
			Code: adbc.StatusInvalidState}
	}

	// calls retain
	s.prepared.SetRecordReader(stream)
	return nil
}

The current non-patched code, raises the error:

[Flight SQL Statement] must call Prepare or set IngestTargetTable before calling Bind

@arnoldwakim
arnoldwakim requested a review from zeroshade as a code owner May 10, 2026 10:31

@lidavidm lidavidm 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.

I suppose this is reasonable but I feel like we should fix the FlightSQL driver as well, @zeroshade

@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.

I agree, we should fix the FlightSQL driver. But more importantly, can you add a test here?

@arnoldwakim

Copy link
Copy Markdown
Contributor Author

I will, add a test, did you have something particular in mind or just asserting that the stream was bound?

Also, are you expecting me to fix the FlightSQL driver or is it something one of you wants to tackle?

Thanks for the swift review!

@zeroshade

Copy link
Copy Markdown
Member

If you are able and willing to fix the flightSQL driver accordingly, go for it! Otherwise one of us will do so as a follow-up to this.

I didn't have anything in particular in mind for a test, just simply a test that would fail on main and succeeds after this fix so that we don't have any regressions in the future.

@arnoldwakim

Copy link
Copy Markdown
Contributor Author

If you are able and willing to fix the flightSQL driver accordingly, go for it! Otherwise one of us will do so as a follow-up to this.

I didn't have anything in particular in mind for a test, just simply a test that would fail on main and succeeds after this fix so that we don't have any regressions in the future.

I think 0cd718e addresses both.

@@ -650,9 +675,18 @@ func (s *statement) BindStream(_ context.Context, stream array.RecordReader) err
}

if s.prepared == nil {

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.

Can't we combine this clause with the one above?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure let me know if 3c38988 did the job.

@@ -617,9 +633,18 @@ func (s *statement) Bind(_ context.Context, values arrow.RecordBatch) error {
}

if s.prepared == nil {

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.

ditto here

@arnoldwakim
arnoldwakim requested a review from zeroshade May 18, 2026 13:57
@arnoldwakim

Copy link
Copy Markdown
Contributor Author

Oh, the failing CI says it needed an action from me?

@lidavidm

Copy link
Copy Markdown
Member

No, it's just a flake

@lidavidm
lidavidm merged commit 4a07b84 into apache:main May 21, 2026
39 of 41 checks passed
@lidavidm lidavidm added this to the ADBC Libraries 24 milestone Jul 22, 2026
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.

4 participants