refactor(server): extract Arrow Flight client into server/flightclient#486
Merged
fuziontech merged 1 commit intofeature/binary-split-pr4-querylog-checkpointfrom May 1, 2026
Conversation
fuziontech
force-pushed
the
feature/binary-split-pr5-flight
branch
from
d1908fe to
d8676f2
Compare
fuziontech
force-pushed
the
feature/binary-split-pr4-querylog-checkpoint
branch
from
e53dc96 to
f898455
Compare
fuziontech
force-pushed
the
feature/binary-split-pr5-flight
branch
from
d8676f2 to
0451c12
Compare
5 tasks
fuziontech
force-pushed
the
feature/binary-split-pr4-querylog-checkpoint
branch
from
f898455 to
c2df749
Compare
Step 5 of the binary-split plan. Moves the Arrow Flight SQL client glue
(FlightExecutor, FlightRowSet, MaxGRPCMessageSize, the value extraction
helpers) out of server/flight_executor.go into a focused
server/flightclient subpackage.
The package is named flightclient (rather than just flight) to avoid a
name collision with arrow-go's github.com/apache/arrow-go/v18/arrow/flight
package, which is imported alongside it in flightsqlingress,
duckdbservice/service.go, and the controlplane.
Files moved:
server/flight_executor.go → server/flightclient/flight_executor.go
server/flight_executor_test.go → server/flightclient/flight_executor_test.go
server/flight_executor_arrow_test.go → server/flightclient/flight_executor_arrow_test.go
The previously private intervalValue type moved into arrowmap as
IntervalValue (alongside OrderedMapValue) so the result formatters in
server/conn.go and server/types.go can switch on it without importing
the flightclient subpackage (which would create an import cycle since
flightclient imports server for the RowSet/ExecResult/RawConn/ColumnTyper
interfaces).
External callers updated to import server/flightclient directly:
- server/flightsqlingress/ingress.go (and its test)
- duckdbservice/service.go
- controlplane/{control,session_mgr,flight_ingress,worker_mgr,k8s_pool}.go
- controlplane/{control_cancel,session_mgr}_test.go
- tests/perf/drivers/flight/driver.go
No back-compat aliases in server/ for FlightExecutor / MaxGRPCMessageSize:
adding them would create the same import cycle (server -> flightclient
-> server). Callers update to flightclient.X directly. There are only a
handful, so the migration is mechanical.
The formatOrderedMapValue tests moved into a new
server/format_ordered_map_test.go because the function under test
(server/conn.go) calls into formatValue, which switches on duckdb-go's
driver value types — it can't move to a duckdb-free subpackage. The
arrowTypeToDuckDB / extractArrowValue tests stayed with their subjects
in flightclient.
What this PR does NOT achieve: server/flightclient is duckdb-free at
the source level, but its transitive import graph still includes
duckdb-go because it imports server (for the executor interfaces) and
server still links libduckdb. Making server itself duckdb-free is the
load-bearing structural work in PR #6+.
Verified:
- go build ./... clean
- go build -tags kubernetes ./... clean
- go test -short ./server/flightclient/... ./server/...
./controlplane/... ./duckdbservice/... — all green (pre-existing
testcontainer Postgres + integration test failures unrelated)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fuziontech
force-pushed
the
feature/binary-split-pr5-flight
branch
from
0451c12 to
bc069e4
Compare
fuziontech
merged commit 2696222
into
feature/binary-split-pr4-querylog-checkpoint
5 checks passed
5 tasks
fuziontech
added a commit
that referenced
this pull request
May 1, 2026
* refactor(server): extract TLS / ACME helpers into server/tlscert
Step 4 of the binary-split plan. Moves the TLS-cert and ACME (HTTP-01,
DNS-01) helpers out of the server/ package root into a focused
server/tlscert subpackage with no dependency on github.com/duckdb/duckdb-go.
Files moved:
server/certs.go → server/tlscert/certs.go
server/acme.go → server/tlscert/acme.go
server/acme_dns.go → server/tlscert/acme_dns.go
server/acme_test.go → server/tlscert/acme_test.go
server/acme_dns_test.go → server/tlscert/acme_dns_test.go
Backward compatibility preserved via type aliases and re-export `var`s in
server/tlscert_aliases.go:
server.ACMEManager is now an alias for tlscert.ACMEManager
server.ACMEDNSManager is now an alias for tlscert.ACMEDNSManager
server.NewACMEManager / NewACMEDNSManager / EnsureCertificates
are re-export `var`s pointing at tlscert.X
So existing references in main.go, tests/integration/harness.go,
controlplane/control.go, and the Server struct's acmeManager /
acmeDNSManager fields all compile unchanged. New code should import
server/tlscert directly.
The previously-private generateSelfSignedCert is now exported as
tlscert.GenerateSelfSignedCert because server_test.go calls it
directly to generate a cert pair without going through the
EnsureCertificates file-existence check.
PR #4 originally targeted querylog + checkpoint extraction. That hit
a chicken-and-egg with DuckLakeConfig: those constructors take the
server.Config struct, and the duckdb-bound config types haven't moved
to a leaf package yet on this branch (PR #1.5 does that on a parallel
branch). Pivoted to TLS/ACME as a self-contained alternative — same
shape as PR #3, no cycle issues. The querylog / checkpoint move will
land in PR #5 once the config types are sorted.
Verified:
- go build ./... clean
- go build -tags kubernetes ./... clean
- go test -short ./server/tlscert/... ./server/... ./controlplane/... ./
all green
- go list -deps ./server/tlscert | grep duckdb-go is empty
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(server): extract Arrow Flight client into server/flightclient (#486)
Step 5 of the binary-split plan. Moves the Arrow Flight SQL client glue
(FlightExecutor, FlightRowSet, MaxGRPCMessageSize, the value extraction
helpers) out of server/flight_executor.go into a focused
server/flightclient subpackage.
The package is named flightclient (rather than just flight) to avoid a
name collision with arrow-go's github.com/apache/arrow-go/v18/arrow/flight
package, which is imported alongside it in flightsqlingress,
duckdbservice/service.go, and the controlplane.
Files moved:
server/flight_executor.go → server/flightclient/flight_executor.go
server/flight_executor_test.go → server/flightclient/flight_executor_test.go
server/flight_executor_arrow_test.go → server/flightclient/flight_executor_arrow_test.go
The previously private intervalValue type moved into arrowmap as
IntervalValue (alongside OrderedMapValue) so the result formatters in
server/conn.go and server/types.go can switch on it without importing
the flightclient subpackage (which would create an import cycle since
flightclient imports server for the RowSet/ExecResult/RawConn/ColumnTyper
interfaces).
External callers updated to import server/flightclient directly:
- server/flightsqlingress/ingress.go (and its test)
- duckdbservice/service.go
- controlplane/{control,session_mgr,flight_ingress,worker_mgr,k8s_pool}.go
- controlplane/{control_cancel,session_mgr}_test.go
- tests/perf/drivers/flight/driver.go
No back-compat aliases in server/ for FlightExecutor / MaxGRPCMessageSize:
adding them would create the same import cycle (server -> flightclient
-> server). Callers update to flightclient.X directly. There are only a
handful, so the migration is mechanical.
The formatOrderedMapValue tests moved into a new
server/format_ordered_map_test.go because the function under test
(server/conn.go) calls into formatValue, which switches on duckdb-go's
driver value types — it can't move to a duckdb-free subpackage. The
arrowTypeToDuckDB / extractArrowValue tests stayed with their subjects
in flightclient.
What this PR does NOT achieve: server/flightclient is duckdb-free at
the source level, but its transitive import graph still includes
duckdb-go because it imports server (for the executor interfaces) and
server still links libduckdb. Making server itself duckdb-free is the
load-bearing structural work in PR #6+.
Verified:
- go build ./... clean
- go build -tags kubernetes ./... clean
- go test -short ./server/flightclient/... ./server/...
./controlplane/... ./duckdbservice/... — all green (pre-existing
testcontainer Postgres + integration test failures unrelated)
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(server): extract SQL/result interfaces into server/sqlcore so flightclient is duckdb-free
Step 6 of the binary-split plan. Carves the SQL/result interfaces and
two pure helpers out of server/ into a new server/sqlcore subpackage so
the Flight client can talk to those abstractions without importing the
rest of server/ (which still links libduckdb).
Symbols moved:
- Interfaces: RowSet, ExecResult, RawConn, ColumnTyper, QueryExecutor
(from server/executor.go)
- IsEmptyQuery + the previously private isEmptyQuery /
stripLeadingComments helpers (from server/conn.go and
server/exports.go) — now sqlcore.IsEmptyQuery and
sqlcore.StripLeadingComments
- OTELGRPCClientHandler (from server/otelgrpc_filter.go)
Backward compatibility preserved via type aliases and re-export `var`s
in server/executor.go and a new server/sqlcore_aliases.go. Existing
references to server.RowSet / server.QueryExecutor / server.IsEmptyQuery
/ server.OTELGRPCClientHandler / etc. compile unchanged.
The Flight client (server/flightclient) is the first consumer to drop
its server import entirely. After this PR:
go list -deps ./server/sqlcore | grep duckdb-go # empty
go list -deps ./server/flightclient | grep duckdb-go # empty
This means a hypothetical cmd/duckgres-controlplane that imports
flightclient (and arrowmap, auth, sysinfo, tlscert, ducklake from
earlier PRs) would not link libduckdb at all. The control plane's
remaining duckdb-go linkage comes from its server import for the
larger Server / clientConn / Config surface, which PR #7+ tackles.
LocalExecutor / PinnedExecutor / LocalRowSet stay in server/executor.go
intentionally — they're standalone-mode-only adapters around *sql.DB
and aren't needed by flightclient or the control plane.
Verified:
- go build ./... clean
- go build -tags kubernetes ./... clean
- go test -short ./server/sqlcore/... ./server/flightclient/...
./server/... ./controlplane/... — all green (existing
isEmptyQuery / stripLeadingComments tests in conn_test.go pass via
the unexported wrappers)
- go list -deps ./server/sqlcore | grep duckdb-go is empty
- go list -deps ./server/flightclient | grep duckdb-go is empty
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
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
FlightExecutor,FlightRowSet,MaxGRPCMessageSize, value extractors) out ofserver/flight_executor.gointo a newserver/flightclient/subpackageintervalValuetype moves intoarrowmap.IntervalValue(alongsideOrderedMapValue) so result formatters inserver/conn.goandserver/types.gocan switch on it without importing flightclient (which would create an import cycle)Why "flightclient" and not "flight"?
arrow-go'sgithub.com/apache/arrow-go/v18/arrow/flightpackage is imported alongside the new package inflightsqlingress,duckdbservice/service.go, and the controlplane. Naming the new packageflightwould force every consumer to alias one of the two, which is more disruptive than a longer name.What this PR does NOT achieve
server/flightclientis duckdb-free at the source level — no directduckdb-goimports — but its transitive import graph still includesduckdb-gobecause it importsserver(for theRowSet/ExecResult/RawConn/ColumnTyperinterfaces) andserverstill links libduckdb via its other files. Makingserveritself duckdb-free is the load-bearing structural work in PR #6+.Why no back-compat aliases for
server.FlightExecutor?Adding them in
server/would create the same import cycle (server → flightclient → server). External callers (controlplane, flightsqlingress, duckdbservice, perf-test driver) update toflightclient.Xdirectly. There were only ~20 call sites to update — mechanical.Test plan
go build ./...cleango build -tags kubernetes ./...cleango test -short ./server/flightclient/... ./server/... ./controlplane/... ./duckdbservice/...— all greenStack
server.go/conn.go/types.go's duckdb-bound code intoserver/exec/soserveritself stops linking duckdb-gocmd/duckgres-controlplaneandcmd/duckgres-workerbinaries🤖 Generated with Claude Code