zeroshade commented on code in PR #4754: URL: https://github.com/apache/arrow-adbc/pull/4754#discussion_r4065803315
########## go/adbc/pkg/_tmpl/driver.go.tmpl: ########## @@ -24,7 +24,7 @@ package main // #cgo CFLAGS: -DADBC_EXPORTING // #cgo CXXFLAGS: -std=c++17 -DADBC_EXPORTING -// #include "../../drivermgr/arrow-adbc/adbc.h" +// #include "../../../adbc/drivermgr/arrow-adbc/adbc.h" Review Comment: **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/[email protected]`. 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`. ########## .gitattributes: ########## @@ -25,7 +25,7 @@ go/adbc/drivermgr/adbc_driver_manager_driver_loading.cc linguist-generated go/adbc/drivermgr/adbc_driver_manager_internal.h linguist-generated 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 Review Comment: 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. ```suggestion go/driver/flightsql/pkg/* linguist-generated go/driver/panicdummy/pkg/* linguist-generated ``` ########## go/driver/flightsql/get_objects.go: ########## @@ -15,33 +15,19 @@ // specific language governing permissions and limitations // under the License. -package internal +package flightsql import ( "context" "regexp" "strconv" "strings" - "time" "github.com/apache/arrow-adbc/go/adbc" + "github.com/apache/arrow-adbc/go/driver/internal/driverbase" "github.com/apache/arrow-go/v18/arrow" "github.com/apache/arrow-go/v18/arrow/array" "github.com/apache/arrow-go/v18/arrow/memory" - "go.opentelemetry.io/otel/attribute" - "go.opentelemetry.io/otel/codes" - semconv "go.opentelemetry.io/otel/semconv/v1.30.0" - "go.opentelemetry.io/otel/trace" -) - -const ( - Unique = "UNIQUE" - PrimaryKey = "PRIMARY KEY" - ForeignKey = "FOREIGN KEY" -) - -var ( - AcceptAll = regexp.MustCompile(".*") ) type CatalogAndSchema struct { Review Comment: 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.) ########## r/tools/bootstrap-go.R: ########## @@ -19,13 +19,13 @@ # directory. Technically this copies all go drivers but this is easier # than remembering the internal dependency structure of the go sources. files_to_vendor <- list.files( - "../../go/adbc", + "../../go", Review Comment: 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. ########## dev/release/post-04-go.sh: ########## @@ -34,12 +34,30 @@ main() { header "Tagging Go release ${VERSION_NATIVE}" version_tag="apache-arrow-adbc-${RELEASE}" - go_arrow_tag="go/adbc/v${VERSION_NATIVE}" + go_adbc_tag="go/adbc/v${VERSION_NATIVE}" + go_driver_tag="go/driver/v${VERSION_NATIVE}" - git tag "${go_arrow_tag}" "${version_tag}" - echo "Created tag ${go_arrow_tag}" + git tag "${go_adbc_tag}" "${version_tag}" + echo "Created tag ${go_adbc_tag}" echo "Please verify and push the tag:" - echo git push apache "${go_arrow_tag}" + echo git push apache "${go_adbc_tag}" + + read -p "After pushing the tag, press ENTER to continue..." ignored + + git switch -c "go-driver-${VERSION_NATIVE}" "${version_tag}" + pushd go/driver + go get -u github.com/apache/arrow-adbc/go/adbc@"${VERSION_NATIVE}" Review Comment: Two problems on this line: 1. **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`. 2. **`-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. ```suggestion 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? ########## go/driver/go.mod: ########## @@ -0,0 +1,80 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +module github.com/apache/arrow-adbc/go/driver + +go 1.26.0 + +toolchain go1.26.8 + +require ( + github.com/apache/arrow-adbc/go/adbc v1.12.0 + github.com/apache/arrow-go/v18 v18.8.0 + github.com/bluele/gcache v0.0.2 + github.com/golang/protobuf v1.5.4 + github.com/google/uuid v1.6.0 + github.com/stretchr/testify v1.12.1 + go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.71.0 + go.opentelemetry.io/otel v1.46.0 + go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.46.0 + go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.46.0 + go.opentelemetry.io/otel/exporters/stdout/stdouttrace v1.46.0 + 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 Review Comment: 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. ########## go/adbc/pkg/Makefile: ########## @@ -47,13 +47,13 @@ DRIVERS := $(addsuffix .$(SUFFIX),$(addprefix libadbc_driver_,$(MANAGERS))) .PHONY: all regenerate all: $(DRIVERS) -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 Review Comment: **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`. ```suggestion $(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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
