feat!(go): split Flight SQL into its own module by lidavidm · Pull Request #4754 · apache/arrow-adbc

lidavidm · 2026-09-04T02:53:30Z

This way we don't leak CVEs/dependencies from the driver into the core ADBC package.

Breaking changes:

package go/adbc/driver/flightsql => go/driver/flightsql
package go/adbc/sqldriver/flightsql => go/driver/flightsql/sqldriver

TODOs

CI, build
Get OpenTelemetry (or at least the exporters) out of the core, too
Regenerate licenses
Update regenerate command in Makefile

Closes #4623.

lidavidm · 2026-09-04T07:23:39Z

I wonder if I should just move driverbase to be part of flightsql (since we don't really expect to export it anyways) to get the OpenTelemetry dependency out of core, or even use the thirdparty driverbase fork

zeroshade · 2026-09-04T20:09:00Z

I wonder if I should just move driverbase to be part of flightsql (since we don't really expect to export it anyways) to get the OpenTelemetry dependency out of core, or even use the thirdparty driverbase fork

I think that's fine since we have we have https://github.com/adbc-drivers/driverbase-go

lidavidm · 2026-09-07T01:34:38Z

Ah, but we still have panicdummy. Maybe the compromise can be go/adbc and go/drivers...

lidavidm · 2026-09-07T03:01:54Z

Alright, that gets gRPC entirely out of the core API...we still depend on the core/trace OTel packages, but they seem to have minimal dependencies, and we still have Protobuf due to some error metadata definitions, but that should also hopefully be OK.

zeroshade · 2026-09-08T18:36:16Z

You're going to need to update the release scripts to add a new tag when we do releases in addition to the go/adbc/vX.Y.Z tag. You'll need to separately also add go/driver/vX.Y.Z as a tag for releases

lidavidm · 2026-09-09T00:49:04Z

Hmm, how is this supposed to work with the go.mod override? Rust/Cargo handles this properly with workspaces, but I suppose for a release we need to remove the override and point at the new tag?

lidavidm · 2026-09-14T02:05:14Z

Ok, I think I'll go with using a Go workspace, and updating scripts/instructions so we auto-publish go/adbc, and then we need a second maintainer step to update and publish go/driver.

This way we don't leak CVEs/dependencies from the driver into the core ADBC package. Breaking changes: - package go/adbc/driver/flightsql => go/flightsql - package go/adbc/sqldriver/flightsql => go/flightsql/sqldriver Also, `driverbase` is now exposed. Closes apache#4623.

lidavidm · 2026-09-14T03:41:08Z

I think for now, I'm not gonna fiddle with go.work, although it means the Flight SQL driver technically uses older API definitions, but I think that's OK since we moved all the driver code anyways. Perhaps at some point we should consider splitting the Flight SQL driver into a separate Apache repository

zeroshade

The split itself is well-executed: clean renames, go mod tidy is a no-op for both modules, both build and vet clean, the shared_utils.go split has zero duplication, and license regeneration looks right (only test-only deps dropped, otelgrpc added). The CVE-isolation goal is achieved.

Two issues are worth blocking on, both from references to the old paths that the rename didn't catch. Details inline; summarizing the two big ones here since one of them is in a file this PR doesn't touch.

1. Driver version stamping is silently broken

go/adbc/pkg/Makefile:51 and c/cmake_modules/GoUtils.cmake:209 both still inject:

-X github.com/apache/arrow-adbc/go/adbc/driver/internal/driverbase.infoDriverVersion=...

but driverbase moved to go/driver/internal/driverbase. The Go linker silently ignores -X for symbols it can't resolve — no warning, no error. Verified by building ./flightsql/pkg with both symbol paths set:

v8.8.8-CORRECT          <- new path, injected
(v9.9.9-STALE absent)   <- old path, silently dropped

So driver.go:97's if infoDriverVersion != "" is now always false and ADBC_INFO_DRIVER_VERSION is never registered. Shipped Flight SQL libraries stop reporting their version via AdbcConnectionGetInfo.

GoUtils.cmake isn't in this PR's diff so I can't comment inline, but it's the path the CMake/release build actually uses — it needs the same fix.

2. `pkg` packages no longer build from the Go module cache

See the inline note on _tmpl/driver.go.tmpl. The cgo include now escapes the module boundary into the sibling go/adbc module, so it only resolves in a source checkout.

Also worth fixing

go/adbc/pkg/doc.go go:generate directives are stale (file isn't in the diff, so no inline). The Makefile regenerate target was updated but these weren't:
```
//go:generate go run ./gen -prefix "FlightSQL" -driver ../driver/flightsql -o flightsql
//go:generate go run ./gen -prefix "PanicDummy" -driver ../driver/panicdummy -o panicdummy
```
../driver/flightsql no longer exists (it's ../../driver/flightsql) and -o should be ../../driver/*/pkg. Probably the remaining half of the unchecked "Update regenerate command in Makefile" TODO.
I couldn't run make regenerate to confirm the checked-in generated files are in sync — no clang-format available on my machine.

Question on the overall design

No replace / go.work — intentional? go/driver resolves go/adbc v1.12.0 from the module proxy, so in-tree changes to the core are never exercised by go/driver CI. A breaking core change merges green and surfaces only after release. post-04-go.sh clearly handles the tag ordering, so I assume this is deliberate — but a go.work would restore "CI tests the tree as it is" without changing published module metadata.

Not yours, but adjacent

TestDefaultDriver/TestCustomizedDriver (driverbase) and TestADBCFlightSQLWithHeader/TestMetadataGetInfo fail on this branch — DriverArrowVersion expects (unknown or development build), gets v18.8.0. I ran both against the merge base (86667c4d7) and they fail identically there, so this is pre-existing. Worth a separate issue, though note it's adjacent to finding #1.

Minor: the golangci-lint bump (v2.9.0 → v2.13.2) plus a new root .golangci.toml is unrelated churn — no golangci config existed before, so this changes lint behavior for go/adbc too.

zeroshade · 2026-09-21T19:48:33Z

-libadbc_driver_%.$(SUFFIX): % ../driver/% ../go.mod ../go.sum
-	$(GO_BUILD) -buildvcs=true -tags driverlib -o $@ -buildmode=c-shared -ldflags "-X github.com/apache/arrow-adbc/go/adbc/driver/internal/driverbase.infoDriverVersion=$(VERSION)" ./$*
+libadbc_driver_%.$(SUFFIX): ../../driver/% ../../driver/%/pkg ../../driver/go.mod ../../driver/go.sum
+	$(GO_BUILD) -C ../../driver/$* -buildvcs=true -tags driverlib -o $(CURDIR)/$@ -buildmode=c-shared -ldflags "-X github.com/apache/arrow-adbc/go/adbc/driver/internal/driverbase.infoDriverVersion=$(VERSION)" ./pkg


Blocking: stale -X symbol path. driverbase moved to go/driver/internal/driverbase in this PR, but this still targets go/adbc/driver/internal/driverbase.

The Go linker silently no-ops -X on an unresolvable symbol, so this fails without any diagnostic and infoDriverVersion stays "" — meaning driver.go:97 never registers ADBC_INFO_DRIVER_VERSION.

Suggested change

$(GO_BUILD) -C ../../driver/$* -buildvcs=true -tags driverlib -o $(CURDIR)/$@ -buildmode=c-shared -ldflags "-X github.com/apache/arrow-adbc/go/adbc/driver/internal/driverbase.infoDriverVersion=$(VERSION)" ./pkg

$(GO_BUILD) -C ../../driver/$* -buildvcs=true -tags driverlib -o $(CURDIR)/$@ -buildmode=c-shared -ldflags "-X github.com/apache/arrow-adbc/go/driver/internal/driverbase.infoDriverVersion=$(VERSION)" ./pkg

c/cmake_modules/GoUtils.cmake:209 has the identical problem and isn't touched by this PR.

zeroshade · 2026-09-21T19:48:33Z

 // #cgo CFLAGS: -DADBC_EXPORTING
 // #cgo CXXFLAGS: -std=c++17 -DADBC_EXPORTING
-// #include "../../drivermgr/arrow-adbc/adbc.h"
+// #include "../../../adbc/drivermgr/arrow-adbc/adbc.h"


Blocking: this include now escapes the module boundary. It used to be ../../drivermgr/arrow-adbc/adbc.h, which stayed inside go/adbc; now it reaches sideways into a different module.

That resolves only when the sibling directory is literally named adbc — true in a source checkout, false in the module cache, where it's go/adbc@v1.12.0. Reproduced with a versioned-directory layout:

flightsql/pkg/init.go:24:11: fatal error: ../../../adbc/drivermgr/arrow-adbc/adbc.h: No such file or directory

So go build -buildmode=c-shared github.com/apache/arrow-adbc/go/driver/flightsql/pkg is now broken for anyone outside a checkout. Vendoring adbc.h into go/driver/ would keep the include module-local. Same issue in utils.h.tmpl.

zeroshade · 2026-09-21T19:48:33Z

 go/adbc/drivermgr/adbc_driver_manager_profiles.cc linguist-generated
 go/adbc/drivermgr/current_arch.h linguist-generated
-go/adbc/pkg/flightsql/* linguist-generated
+go/adbc/flightsql/pkg/* linguist-generated


This path doesn't exist anywhere in the tree — looks like driver → adbc got transposed. And line 29 (go/adbc/pkg/panicdummy/*) is stale for the same move. Both generated dirs stop being marked as such.

Suggested change

go/adbc/flightsql/pkg/* linguist-generated

go/driver/flightsql/pkg/* linguist-generated

go/driver/panicdummy/pkg/* linguist-generated

zeroshade · 2026-09-21T19:48:33Z

+
+    git switch -c "go-driver-${VERSION_NATIVE}" "${version_tag}"
+    pushd go/driver
+    go get -u github.com/apache/arrow-adbc/go/adbc@"${VERSION_NATIVE}"


Two problems on this line:

Missing v prefix. The tag created above is go/adbc/v${VERSION_NATIVE}, so the module version is v1.12.0 — but this queries @1.12.0.

-u upgrades every transitive dep to latest, not just go/adbc. Bumping the whole dependency graph at tag-cut time seems like the last thing you'd want during a release.

Suggested change

go get -u github.com/apache/arrow-adbc/go/adbc@"${VERSION_NATIVE}"

go get github.com/apache/arrow-adbc/go/adbc@"v${VERSION_NATIVE}"

Separately: the script ends on the go-driver-${VERSION_NATIVE} branch and never says to push or merge it, so main's go/driver/go.mod stays a release behind and that commit is reachable only via the tag. Intentional?

zeroshade · 2026-09-21T19:48:34Z

+	go.opentelemetry.io/otel/sdk v1.46.0
+	go.opentelemetry.io/otel/trace v1.46.0
+	golang.org/x/exp v0.0.0-20260908205506-85c1c2202aba
+	golang.org/x/oauth2 v0.36.0


This is a downgrade — go/adbc was on golang.org/x/oauth2 v0.37.0 before the split. For a PR whose stated point is keeping driver CVEs out of the core, the driver is the module that most wants the newer oauth2 (it's what flightsql_oauth.go uses).

genproto/googleapis/{api,rpc} moved backwards too (20260908 → 20260819/20260825). Looks like the go.sum was generated before the last round of core bumps landed.

zeroshade · 2026-09-21T19:48:34Z

-	AcceptAll = regexp.MustCompile(".*")
 )

 type CatalogAndSchema struct {


Heads up that this move promotes a lot of previously-internal machinery into public API. These were in go/adbc/driver/internal/ specifically so they wouldn't be; landing them in package flightsql makes GetObjects, PatternToRegexp, TableInfo, CatalogAndSchema, DefaultXdbcMetadataBuilder and friends importable surface you're then on the hook for.

Since flightsql is the only consumer now, either unexporting them or putting them in go/driver/flightsql/internal/ would preserve the original intent.

(The split itself is clean, for what it's worth — I diffed the symbol lists and every name from the old shared_utils.go lands in exactly one of the two new files, no duplication.)

zeroshade · 2026-09-21T19:48:34Z

 # than remembering the internal dependency structure of the go sources.
 files_to_vendor <- list.files(
-  "../../go/adbc",
+  "../../go",


Vendoring widened from go/adbc to all of go/, but since go/driver resolves go/adbc from the proxy rather than from disk, the vendored src/go/adbc is now dead weight — shipped source that isn't what actually gets built.

Not breaking (the build already needs network for arrow-go etc.), but it does mean the source package no longer contains the core that the resulting binary is built against.

lidavidm force-pushed the gh-4623 branch 6 times, most recently from ed14bd0 to 259a74f Compare September 4, 2026 05:17

lidavidm marked this pull request as ready for review September 4, 2026 06:12

lidavidm requested review from kou, paleolimbot and zeroshade as code owners September 4, 2026 06:12

lidavidm requested a review from Mandukhai-Alimaa September 4, 2026 06:13

lidavidm force-pushed the gh-4623 branch from 259a74f to 306d86a Compare September 7, 2026 01:50

lidavidm force-pushed the gh-4623 branch from 3e342dd to f0005ca Compare September 14, 2026 02:26

lidavidm added this to the ADBC Libraries 25 milestone Sep 21, 2026

zeroshade reviewed Sep 21, 2026

View reviewed changes

	$(GO_BUILD) -C ../../driver/$* -buildvcs=true -tags driverlib -o $(CURDIR)/$@ -buildmode=c-shared -ldflags "-X github.com/apache/arrow-adbc/go/adbc/driver/internal/driverbase.infoDriverVersion=$(VERSION)" ./pkg
	$(GO_BUILD) -C ../../driver/$* -buildvcs=true -tags driverlib -o $(CURDIR)/$@ -buildmode=c-shared -ldflags "-X github.com/apache/arrow-adbc/go/driver/internal/driverbase.infoDriverVersion=$(VERSION)" ./pkg

	go/adbc/flightsql/pkg/* linguist-generated
	go/driver/flightsql/pkg/* linguist-generated
	go/driver/panicdummy/pkg/* linguist-generated

	go get -u github.com/apache/arrow-adbc/go/adbc@"${VERSION_NATIVE}"
	go get github.com/apache/arrow-adbc/go/adbc@"v${VERSION_NATIVE}"

Conversation

lidavidm commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lidavidm commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeroshade commented Sep 4, 2026

Uh oh!

lidavidm commented Sep 7, 2026

Uh oh!

lidavidm commented Sep 7, 2026

Uh oh!

zeroshade commented Sep 8, 2026

Uh oh!

lidavidm commented Sep 9, 2026

Uh oh!

lidavidm commented Sep 14, 2026

Uh oh!

lidavidm commented Sep 14, 2026

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

1. Driver version stamping is silently broken

2. pkg packages no longer build from the Go module cache

Also worth fixing

Question on the overall design

Not yours, but adjacent

Uh oh!

zeroshade Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

zeroshade Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

zeroshade Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

zeroshade Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

zeroshade Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

zeroshade Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

zeroshade Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lidavidm commented Sep 4, 2026 •

edited

Loading

lidavidm commented Sep 4, 2026 •

edited

Loading

2. `pkg` packages no longer build from the Go module cache