Skip to content

feat!(go): split Flight SQL into its own module - #4754

Open
lidavidm wants to merge 1 commit into
apache:mainfrom
lidavidm:gh-4623
Open

lidavidm wants to merge 1 commit into
apache:mainfrom
lidavidm:gh-4623

Conversation

@lidavidm

@lidavidm lidavidm commented Sep 4, 2026 •

Copy link
Copy Markdown
Member

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

Closes #4623.

@lidavidm
lidavidm force-pushed the gh-4623 branch 6 times, most recently from ed14bd0 to 259a74f Compare September 4, 2026 05:17
@lidavidm
lidavidm marked this pull request as ready for review September 4, 2026 06:12
@lidavidm

lidavidm commented Sep 4, 2026 •

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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

lidavidm commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

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

@lidavidm

lidavidm commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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

lidavidm commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

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.

@lidavidm

Copy link
Copy Markdown
Member Author

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

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

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

Comment thread go/adbc/pkg/Makefile Outdated
Comment thread go/adbc/pkg/_tmpl/driver.go.tmpl Outdated
Comment thread .gitattributes Outdated
Comment thread dev/release/post-04-go.sh Outdated
Comment thread go/driver/go.mod
Comment thread go/driver/flightsql/get_objects.go
Comment thread r/tools/bootstrap-go.R
# than remembering the internal dependency structure of the go sources.
files_to_vendor <- list.files(
"../../go/adbc",
"../../go",

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.

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
lidavidm force-pushed the gh-4623 branch 3 times, most recently from e05ddde to 9a71e7b Compare September 28, 2026 07:02

This branch has not been deployed

No deployments
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.

go/adbc/driver/flightsql: separate from the main project

2 participants