feat(go/adbc): add IngestStream helper for one-call ingestion and add TestIngestStream #3150

Mandukhai-Alimaa · 2025-07-14T02:20:52Z

Changes:

New freestanding function: func IngestStream(ctx context.Context, cnxn Connection, reader array.RecordReader, opts map[string]string) (int64, error)
TestIngestStream added to drivermgr_test

lidavidm

Would this make more sense as a freestanding function? It can be implemented entirely in terms of existing APIs anyways, and that way we don't have to make a breaking change to the core API

zeroshade · 2025-07-14T02:35:44Z

I was thinking the same as what @lidavidm suggested, a free function would be better in this case to avoid a breaking change to the interface

Wrap the ingestion steps into a single, freestanding `adbc.IngestStream` function for easier Arrow data ingestion. Add `TestIngestStream` in drivermgr_test.go to verify end-to-end functionality. Closes apache#3142

lidavidm · 2025-07-14T07:23:51Z

go/adbc/adbc.go

+// the five-step boilerplate of NewStatement, SetOption, Bind,
+// Execute, and Close.
+//
+// This is not part of the ADBC API specification.


Can we move this to ext.go? https://github.com/apache/arrow-adbc/blob/main/go/adbc/ext.go

lidavidm · 2025-07-14T07:24:11Z

go/adbc/drivermgr/wrapper_sqlite_test.go

+	// 1) Create the target table
+	st, err := dm.conn.NewStatement()
+	dm.Require().NoError(err)
+	defer validation.CheckedClose(dm.T(), st)
+
+	dm.NoError(st.SetSqlQuery(`
+        CREATE TABLE IF NOT EXISTS ingest_test (
+            col1 INTEGER,
+            col2 TEXT
+        )
+    `))
+	n, err := st.ExecuteUpdate(dm.ctx)
+	dm.NoError(err)
+	dm.Equal(int64(0), n, "CREATE TABLE should report 0 rows affected")


Ingest can create the table itself, so this isn't necessarily necessary

lidavidm · 2025-07-14T07:24:22Z

go/adbc/drivermgr/wrapper_sqlite_test.go

+	defer b.Release()
+
+	// first batch: 3 rows
+	b.Field(0).(*array.Int64Builder).AppendValues([]int64{1, 2, 3}, nil)


ArrayFromJSON might make this easier

lidavidm · 2025-07-14T07:25:14Z

go/adbc/adbc.go

+// Execute, and Close.
+//
+// This is not part of the ADBC API specification.
+func IngestStream(ctx context.Context, cnxn Connection, reader array.RecordReader, opts map[string]string) (int64, error) {


Maybe we should accept things like target table directly as parameters? Or have an explicit parameters struct? (We can keep the map for any other options but I think the idea ought to be that we make the common parameters into formal parameters instead of requiring the option)

lidavidm · 2025-07-14T07:26:16Z

go/adbc/adbc.go

+	if err != nil {
+		return -1, fmt.Errorf("IngestStream: NewStatement: %w", err)
+	}
+	defer stmt.Close()


Close is fallible, so you have to handle the error. You need something like this (with a named error return value)

defer func() { err = errors.Join(err, stmt.Close()) }()

Wrap the ingestion steps into a single, freestanding `adbc.IngestStream` function for easier Arrow data ingestion. Add `TestIngestStream` in drivermgr_test to verify end-to-end functionality. Closes apache#3142

zeroshade · 2025-07-14T21:11:34Z

go/adbc/ext.go

+	// 1) Create a new statement
+	stmt, err := cnxn.NewStatement()
+	if err != nil {
+		return -1, fmt.Errorf("IngestStream: NewStatement: %w", err)


the convention for golang error strings is that they do not start with a capitalized letter. Perhaps something more like:

fmt.Errorf("error during ingestion: NewStatement: %w", err)

zeroshade · 2025-07-14T21:12:41Z

go/adbc/ext.go

+	}
+
+	// Set required options
+	if err := stmt.SetOption(OptionKeyIngestTargetTable, opt.TargetTable); err != nil {


remove the :, you're shadowing the err from before so the errors.Join isn't going to work this way.

zeroshade · 2025-07-14T21:14:50Z

go/adbc/ext.go

+	}
+
+	// 4) Execute the update
+	count, err := stmt.ExecuteUpdate(ctx)


same comment as before, you're shadowing the err var here. Usually not a problem, but will be an issue with the defer from the top

zeroshade · 2025-07-14T21:16:51Z

go/adbc/ext.go

+type IngestStreamOption struct {
+	TargetTable string            // required
+	IngestMode  string            // required, e.g. adbc.OptionValueIngestModeCreateAppend, or OptionValueIngestModeReplace
+	Extra       map[string]string // any other stmt.SetOption(...) args


since these are required, should we make them explicit parameters instead of behind this struct? i.e.

func IngestStream(ctx context.Context, cnxn Connection, reader array.RecordReader, target, mode string, extra map[string]string) (int64, error)

what do you think? @lidavidm thoughts?

I could go either way, but the struct may make it easier to add new parameters in the future? That said I don't mind table/mode being just formal parameters and having struct fields for things like temporary/catalog/schema, and keeping Extra for any driver-specific options

Wrap the ingestion steps into a single, freestanding `adbc.IngestStream` function for easier Arrow data ingestion. Add `TestIngestStream` in drivermgr_test to verify end-to-end functionality. Closes apache#3142

zeroshade

Overall this looks really good, just a couple comments.

zeroshade · 2025-07-17T20:57:11Z

go/adbc/ext.go

+
+// IngestStreamOption bundles the IngestStream options.
+// Driver specific options can go into Extra.
+type IngestStreamOption struct {


I would pluralize this to be IngestStreamOptions personally.

Alternately, we could potentially do something like:

type IngestStreamOption func(adbc.Statement) error func ingestSetOption(name, value string) IngestStreamOption { return func(st adbc.Statement) { return st.SetOption(name, cat) } } func WithIngestCatalog(cat string) IngestStreamOption { return ingestSetOption(adbc.OptionValueIngestTargetCatalog, cat) } func WithIngestTemp() IngestStreamOption { return ingestSetOption(adbc.OptionValueIngestTemporary, adbc.OptionValueEnabled) } ... func IngestStream(ctx context.Context, cnxn Connection, reader array.RecordReader, targetTable, ingestMode string, opts ...IngestStreamOption) (count int64, err error) { ... for _, o := range opts { if err = o(stmt); err != nil { err = fmt.Errorf("error during ingestion: %w", err) return } } .... }

which would not only allow for the options we provide but make it fairly easy for users to create their own options if they like and want to add, while still making this extensible.

I'm not saying we definitely should do this, just putting it out as a suggestion. Thoughts?

I think it's fine as-is in terms of extensibility

lidavidm · 2025-07-17T23:57:54Z

go/adbc/ext.go

+
+// IngestStreamOption bundles the IngestStream options.
+// Driver specific options can go into Extra.
+type IngestStreamOption struct {


I think it's fine as-is in terms of extensibility

lidavidm · 2025-07-18T00:13:08Z

go/adbc/ext.go

+
+	// Set required options
+	if err = stmt.SetOption(OptionKeyIngestTargetTable, targetTable); err != nil {
+		return 0, fmt.Errorf("error during ingestion: SetOption(target_table=%s): %w", targetTable, err)


nit: why do we go from returning -1 to 0 as the sentinel value? (Not that it matters either way, but I think we usually use -1)

Fixed it. I've switched the sentinel back to −1. My pair programmer (AI assistant) must have accidentally used 0, and I overlooked it. Thanks for catching that!

Wrap the ingestion steps into a single, freestanding `adbc.IngestStream` function for easier Arrow data ingestion. Add `TestIngestStream` in drivermgr_test to verify end-to-end functionality. Closes apache#3142

add IngestStream helper for one-call ingestion and add TestIngestStream

5ffc499

lidavidm reviewed Jul 14, 2025

View reviewed changes

lidavidm changed the title ~~add IngestStream helper for one-call ingestion and add TestIngestStream~~ feat(go/adbc): add IngestStream helper for one-call ingestion and add TestIngestStream Jul 14, 2025

MANDY Alimaa added 2 commits July 13, 2025 22:39

feat(adbc): add IngestStream helper and TestIngestStream

0b4e52e

Wrap the ingestion steps into a single, freestanding `adbc.IngestStream` function for easier Arrow data ingestion. Add `TestIngestStream` in drivermgr_test.go to verify end-to-end functionality. Closes apache#3142

feat(adbc): add IngestStream helper and TestIngestStream

2d16f00

Wrap the ingestion steps into a single, freestanding `adbc.IngestStream` function for easier Arrow data ingestion. Add `TestIngestStream` in drivermgr_test.go to verify end-to-end functionality. Closes apache#3142

lidavidm reviewed Jul 14, 2025

View reviewed changes

MANDY Alimaa added 3 commits July 14, 2025 13:59

feat(adbc): add IngestStream helper and TestIngestStream

51f93d8

Wrap the ingestion steps into a single, freestanding `adbc.IngestStream` function for easier Arrow data ingestion. Add `TestIngestStream` in drivermgr_test to verify end-to-end functionality. Closes apache#3142

feat(adbc): add IngestStream helper and TestIngestStream

18ff7b9

Wrap the ingestion steps into a single, freestanding `adbc.IngestStream` function for easier Arrow data ingestion. Add `TestIngestStream` in drivermgr_test to verify end-to-end functionality. Closes apache#3142

feat(adbc): add IngestStream helper and TestIngestStream

73c91da

Wrap the ingestion steps into a single, freestanding `adbc.IngestStream` function for easier Arrow data ingestion. Add `TestIngestStream` in drivermgr_test to verify end-to-end functionality. Closes apache#3142

zeroshade reviewed Jul 14, 2025

View reviewed changes

feat(adbc): add IngestStream helper and TestIngestStream

831fcb3

Wrap the ingestion steps into a single, freestanding `adbc.IngestStream` function for easier Arrow data ingestion. Add `TestIngestStream` in drivermgr_test to verify end-to-end functionality. Closes apache#3142

Mandukhai-Alimaa marked this pull request as ready for review July 15, 2025 16:47

github-actions bot added this to the ADBC Libraries 20 milestone Jul 15, 2025

zeroshade reviewed Jul 17, 2025

View reviewed changes

lidavidm approved these changes Jul 18, 2025

View reviewed changes

MANDY Alimaa added 2 commits July 17, 2025 19:25

feat(adbc): add IngestStream helper and TestIngestStream

bc9a7d8

Wrap the ingestion steps into a single, freestanding `adbc.IngestStream` function for easier Arrow data ingestion. Add `TestIngestStream` in drivermgr_test to verify end-to-end functionality. Closes apache#3142

feat(adbc): add IngestStream helper and TestIngestStream

1cc003e

Wrap the ingestion steps into a single, freestanding `adbc.IngestStream` function for easier Arrow data ingestion. Add `TestIngestStream` in drivermgr_test to verify end-to-end functionality. Closes apache#3142

feat(go/adbc): add IngestStream helper for one-call ingestion and add TestIngestStream #3150

Are you sure you want to change the base?

feat(go/adbc): add IngestStream helper for one-call ingestion and add TestIngestStream #3150

Uh oh!

Conversation

Mandukhai-Alimaa commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

zeroshade commented Jul 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Mandukhai-Alimaa commented Jul 14, 2025 •

edited

Loading