refactor(server): drop duckdb-go from conn.go via DuckDBAppender hook#496
Merged
fuziontech merged 1 commit intomainfrom May 1, 2026
Merged
refactor(server): drop duckdb-go from conn.go via DuckDBAppender hook#496fuziontech merged 1 commit intomainfrom
fuziontech merged 1 commit intomainfrom
Conversation
server/conn.go's appendWithDuckDBAppender was the last block of duckdb-go references in conn.go (used for COPY FROM bulk insert via the duckdb Appender API). This PR factors it out the same way PR #482 split AppendValue: server holds the registration hook, duckdbservice's init() registers the real implementation. Changes: - server/duckdb_appender.go (new): DuckDBAppendFunc type + RegisterDuckDBAppender + a thin (*clientConn).appendWithDuckDBAppender that dispatches to the registered implementation. Returns a sentinel errDuckDBAppenderUnavailable when nothing is registered, so callers can fall back to batchInsertRows. - duckdbservice/duckdb_appender.go (new): the real implementation. Registers in init() so any binary that links duckdbservice gets full Appender support. Mirrors the logic that previously lived inline in conn.go (Raw → driver.Conn → duckdb.NewAppender* → AppendRow loop). - server/conn.go: the inline implementation is removed; the unused "database/sql/driver" and "github.com/duckdb/duckdb-go/v2" imports are dropped. grep duckdb server/conn.go # only an arrowmap import + SQL strings go list -deps ./server/conn.go # no duckdb-go (file-level) The whole server package still links duckdb-go via server.go, querylog.go, and checkpoint.go, but conn.go is fully clean of duckdb-go now. Verified: - go build ./... clean - go build -tags kubernetes ./... clean - go test -short ./server/ ./duckdbservice/... — green Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
fuziontech
added a commit
that referenced
this pull request
May 1, 2026
…497) server/server.go used to blank-import github.com/duckdb/duckdb-go/v2 to register the "duckdb" sql/driver. The all-in-one binary already imports duckdbservice (which itself imports duckdb-go via the appender + value normalizer hooks added in PR #482, #494, #496), so the driver gets registered through that chain — server.go's blank import is redundant in production. server/ package tests already blank-import duckdb-go in the files that need a real DuckDB connection (bundled_extensions_test.go, catalog_test.go, chsql_test.go, transient_test.go, session_database_metadata_test.go, types_test.go), so removing the production blank import doesn't break the test binaries. The integration test binary (tests/integration/) imported `server` but not `duckdbservice`, so it relied on server.go's blank import for driver registration. Added a blank import of duckdbservice in tests/integration/harness.go to compensate — same effect, just routed through the package that's now responsible for registering all duckdb-go hooks (driver, AppendValue handler, value normalizer, COPY appender). After this PR, server/server.go has zero `import` statements that pull duckdb-go into the package's import graph. server still links duckdb-go overall via querylog.go and checkpoint.go (both call sql.Open("duckdb", ...) directly), but that's about runtime use of an already-registered driver rather than a Go-level package import. The follow-up PRs that move querylog and checkpoint into server/exec/ will close out the file-level duckdb-go references in server/. Verified: - go build ./... clean - go build -tags kubernetes ./... clean - go test -short ./server/ ./duckdbservice/... ./controlplane/... ./ — all green - go test -short -timeout 60s ./tests/integration/... — same failures as main (TestFallbackUtilityCommands etc.); confirmed pre-existing via stash + re-run; unrelated to this PR Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
server/conn.go'sappendWithDuckDBAppenderwas the last block ofduckdb-goreferences inconn.go— the COPY FROM bulk-insert path that uses the duckdbAppenderAPI. This PR factors it out the same way PR #482 splitAppendValue:serverholds the registration hook,duckdbservice'sinit()registers the real implementation.Changes
server/duckdb_appender.go(new):DuckDBAppendFunctype +RegisterDuckDBAppender+ a thin(*clientConn).appendWithDuckDBAppenderthat dispatches to the registered implementation. Returns a sentinelerrDuckDBAppenderUnavailablewhen nothing is registered, so callers can fall back tobatchInsertRows.duckdbservice/duckdb_appender.go(new): the real implementation. Registers ininit()so any binary that linksduckdbservicegets full Appender support. Mirrors the logic that previously lived inline inconn.go(Raw→driver.Conn→duckdb.NewAppender*→ AppendRow loop).server/conn.go: the inline implementation is removed; the unused"database/sql/driver"and"github.com/duckdb/duckdb-go/v2"imports are dropped.After this PR
The whole
serverpackage still linksduckdb-goviaserver.go,querylog.go, andcheckpoint.go, butconn.goitself is fully clean. Three down (types.go format/encode, conn.go format, conn.go appender), three to go (server.go's_ "duckdb-go"blank import +sql.Open("duckdb", ...)calls; querylog.go'ssql.Open("duckdb", ...); checkpoint.go'ssql.Open("duckdb", ...)).Test plan
go build ./...cleango build -tags kubernetes ./...cleango test -short ./server/ ./duckdbservice/...— green🤖 Generated with Claude Code