From 38c5fb35b6e5e44450dbee121d7c8d0231703601 Mon Sep 17 00:00:00 2001 From: James Greenhill Date: Fri, 1 May 2026 08:55:25 -0700 Subject: [PATCH] refactor(server): extract SQL/result interfaces into server/sqlcore so flightclient is duckdb-free MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- server/conn.go | 41 ++----------- server/executor.go | 57 +++++-------------- server/exports.go | 5 -- server/flightclient/flight_executor.go | 28 ++++----- server/sqlcore/interfaces.go | 57 +++++++++++++++++++ .../{otelgrpc_filter.go => sqlcore/otel.go} | 2 +- server/sqlcore/query.go | 41 +++++++++++++ server/sqlcore_aliases.go | 13 +++++ 8 files changed, 145 insertions(+), 99 deletions(-) create mode 100644 server/sqlcore/interfaces.go rename server/{otelgrpc_filter.go => sqlcore/otel.go} (97%) create mode 100644 server/sqlcore/query.go create mode 100644 server/sqlcore_aliases.go diff --git a/server/conn.go b/server/conn.go index e5b0f8fe..40f71832 100644 --- a/server/conn.go +++ b/server/conn.go @@ -28,6 +28,7 @@ import ( pg_query "github.com/pganalyze/pg_query_go/v6" "github.com/posthog/duckgres/duckdbservice/arrowmap" "github.com/posthog/duckgres/server/auth" + "github.com/posthog/duckgres/server/sqlcore" "github.com/posthog/duckgres/transpiler" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" @@ -2476,41 +2477,11 @@ func countDollarParams(query string) int { return max } -// isEmptyQuery checks if a query contains only semicolons, whitespace, and/or comments. -// PostgreSQL returns EmptyQueryResponse for queries like ";", ";;;", "-- ping", etc. -func isEmptyQuery(query string) bool { - // Strip SQL comments first (e.g., pgx sends "-- ping" for Ping()) - stripped := stripLeadingComments(query) - for _, r := range stripped { - if r != ';' && r != ' ' && r != '\t' && r != '\n' && r != '\r' { - return false - } - } - return true -} - -// stripLeadingComments removes leading SQL comments from a query. -// Handles both block comments /* ... */ and line comments -- ... -func stripLeadingComments(query string) string { - for { - query = strings.TrimSpace(query) - if strings.HasPrefix(query, "/*") { - end := strings.Index(query, "*/") - if end == -1 { - return query - } - query = query[end+2:] - } else if strings.HasPrefix(query, "--") { - end := strings.Index(query, "\n") - if end == -1 { - return "" - } - query = query[end+1:] - } else { - return query - } - } -} +// isEmptyQuery and stripLeadingComments moved to server/sqlcore so the +// Flight client can call them without importing server. Local thin wrappers +// preserve the unexported call-site spellings used throughout this file. +func isEmptyQuery(query string) bool { return sqlcore.IsEmptyQuery(query) } +func stripLeadingComments(query string) string { return sqlcore.StripLeadingComments(query) } // stripLeadingNoise strips leading whitespace, comments, and parentheses from // a query string in a loop until none remain. This handles cases like diff --git a/server/executor.go b/server/executor.go index 5eaec997..449a9fa4 100644 --- a/server/executor.go +++ b/server/executor.go @@ -4,52 +4,21 @@ import ( "context" "database/sql" "os" -) - -// ColumnTyper provides type name information for a database column. -// *sql.ColumnType satisfies this interface. -type ColumnTyper interface { - DatabaseTypeName() string -} - -// RowSet represents a set of rows from a query result. -type RowSet interface { - Columns() ([]string, error) - ColumnTypes() ([]ColumnTyper, error) - Next() bool - Scan(dest ...any) error - Close() error - Err() error -} - -// ExecResult represents the result of a non-query execution. -type ExecResult interface { - RowsAffected() (int64, error) -} - -// RawConn provides access to the underlying driver connection. -// *sql.Conn satisfies this interface. -type RawConn interface { - Raw(func(any) error) error - Close() error -} -// QueryExecutor abstracts database query execution, allowing both local (*sql.DB) -// and remote (Arrow Flight SQL) backends. -type QueryExecutor interface { - QueryContext(ctx context.Context, query string, args ...any) (RowSet, error) - ExecContext(ctx context.Context, query string, args ...any) (ExecResult, error) - Query(query string, args ...any) (RowSet, error) - Exec(query string, args ...any) (ExecResult, error) - ConnContext(ctx context.Context) (RawConn, error) - PingContext(ctx context.Context) error - Close() error + "github.com/posthog/duckgres/server/sqlcore" +) - // LastProfilingOutput returns the JSON profiling output from the last - // executed query, or "" if profiling is not enabled or not available - // (e.g. Flight SQL mode where the query ran on a remote worker). - LastProfilingOutput() string -} +// The SQL/result interfaces moved to server/sqlcore so the Flight client +// and other duckdb-free callers can implement them without importing +// server. The aliases below preserve the old server.X spellings for the +// dozens of references inside this package and elsewhere. +type ( + ColumnTyper = sqlcore.ColumnTyper + RowSet = sqlcore.RowSet + ExecResult = sqlcore.ExecResult + RawConn = sqlcore.RawConn + QueryExecutor = sqlcore.QueryExecutor +) // LocalExecutor wraps *sql.DB to implement QueryExecutor for local DuckDB access. type LocalExecutor struct { diff --git a/server/exports.go b/server/exports.go index 26d8b6cc..ef25055d 100644 --- a/server/exports.go +++ b/server/exports.go @@ -127,11 +127,6 @@ func GenerateSecretKey() int32 { return generateSecretKey() } -// IsEmptyQuery checks if a query contains only semicolons, whitespace, and/or SQL comments. -// PostgreSQL returns EmptyQueryResponse for queries like ";", "-- ping", "/* */", etc. -func IsEmptyQuery(query string) bool { - return isEmptyQuery(query) -} // SetQueryLogger sets the query logger on a Server. Used by the control plane // to attach a query logger to the minimal server after creation. diff --git a/server/flightclient/flight_executor.go b/server/flightclient/flight_executor.go index b5e69848..b7e080fb 100644 --- a/server/flightclient/flight_executor.go +++ b/server/flightclient/flight_executor.go @@ -20,7 +20,7 @@ import ( "github.com/apache/arrow-go/v18/arrow/flight/flightsql" "github.com/apache/arrow-go/v18/arrow/memory" "github.com/posthog/duckgres/duckdbservice/arrowmap" - "github.com/posthog/duckgres/server" + "github.com/posthog/duckgres/server/sqlcore" "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/metadata" @@ -70,7 +70,7 @@ func NewFlightExecutor(addr, bearerToken, sessionToken string) (*FlightExecutor, // Propagate OTEL trace context across gRPC to worker pods. // Filtered to query RPCs only (GetFlightInfo, DoGet). - dialOpts = append(dialOpts, server.OTELGRPCClientHandler()) + dialOpts = append(dialOpts, sqlcore.OTELGRPCClientHandler()) if bearerToken != "" { dialOpts = append(dialOpts, grpc.WithPerRPCCredentials(&bearerCreds{token: bearerToken})) @@ -156,14 +156,14 @@ func recoverClientPanic(err *error) { } } -func (e *FlightExecutor) QueryContext(ctx context.Context, query string, args ...any) (rs server.RowSet, err error) { +func (e *FlightExecutor) QueryContext(ctx context.Context, query string, args ...any) (rs sqlcore.RowSet, err error) { if e.dead.Load() { return nil, ErrWorkerDead } // Return empty results for queries that are only semicolons, whitespace, // and/or comments. These represent PostgreSQL client pings (e.g., pgx sends "-- ping"). - if server.IsEmptyQuery(query) { + if sqlcore.IsEmptyQuery(query) { return &emptyRowSet{}, nil } @@ -221,13 +221,13 @@ func (e *FlightExecutor) QueryContext(ctx context.Context, query string, args .. }, nil } -func (e *FlightExecutor) ExecContext(ctx context.Context, query string, args ...any) (result server.ExecResult, err error) { +func (e *FlightExecutor) ExecContext(ctx context.Context, query string, args ...any) (result sqlcore.ExecResult, err error) { if e.dead.Load() { return nil, ErrWorkerDead } // Return zero rows affected for empty/comment-only queries. - if server.IsEmptyQuery(query) { + if sqlcore.IsEmptyQuery(query) { return &flightExecResult{rowsAffected: 0}, nil } @@ -252,15 +252,15 @@ func (e *FlightExecutor) ExecContext(ctx context.Context, query string, args ... return &flightExecResult{rowsAffected: affected}, nil } -func (e *FlightExecutor) Query(query string, args ...any) (server.RowSet, error) { +func (e *FlightExecutor) Query(query string, args ...any) (sqlcore.RowSet, error) { return e.QueryContext(context.Background(), query, args...) } -func (e *FlightExecutor) Exec(query string, args ...any) (server.ExecResult, error) { +func (e *FlightExecutor) Exec(query string, args ...any) (sqlcore.ExecResult, error) { return e.ExecContext(context.Background(), query, args...) } -func (e *FlightExecutor) ConnContext(ctx context.Context) (server.RawConn, error) { +func (e *FlightExecutor) ConnContext(ctx context.Context) (sqlcore.RawConn, error) { return nil, fmt.Errorf("ConnContext not supported in Flight mode (use batched INSERT for COPY FROM)") } @@ -341,8 +341,8 @@ func (r *FlightRowSet) Columns() ([]string, error) { return names, nil } -func (r *FlightRowSet) ColumnTypes() ([]server.ColumnTyper, error) { - types := make([]server.ColumnTyper, r.schema.NumFields()) +func (r *FlightRowSet) ColumnTypes() ([]sqlcore.ColumnTyper, error) { + types := make([]sqlcore.ColumnTyper, r.schema.NumFields()) for i := 0; i < r.schema.NumFields(); i++ { types[i] = &arrowColumnType{dt: r.schema.Field(i).Type} } @@ -428,7 +428,7 @@ func (r *FlightRowSet) Err() error { type emptyRowSet struct{} func (e *emptyRowSet) Columns() ([]string, error) { return nil, nil } -func (e *emptyRowSet) ColumnTypes() ([]server.ColumnTyper, error) { return nil, nil } +func (e *emptyRowSet) ColumnTypes() ([]sqlcore.ColumnTyper, error) { return nil, nil } func (e *emptyRowSet) Next() bool { return false } func (e *emptyRowSet) Scan(dest ...any) error { return fmt.Errorf("no rows") } func (e *emptyRowSet) Close() error { return nil } @@ -450,8 +450,8 @@ func (e *emptySchemaRowSet) Columns() ([]string, error) { return cols, nil } -func (e *emptySchemaRowSet) ColumnTypes() ([]server.ColumnTyper, error) { - types := make([]server.ColumnTyper, e.schema.NumFields()) +func (e *emptySchemaRowSet) ColumnTypes() ([]sqlcore.ColumnTyper, error) { + types := make([]sqlcore.ColumnTyper, e.schema.NumFields()) for i := 0; i < e.schema.NumFields(); i++ { types[i] = &arrowColumnType{dt: e.schema.Field(i).Type} } diff --git a/server/sqlcore/interfaces.go b/server/sqlcore/interfaces.go new file mode 100644 index 00000000..ca71f902 --- /dev/null +++ b/server/sqlcore/interfaces.go @@ -0,0 +1,57 @@ +// Package sqlcore holds the duckgres-internal SQL/result interfaces that +// span the wire-protocol/server layer and the Arrow Flight client. It also +// hosts a couple of small helpers (IsEmptyQuery, OTELGRPCClientHandler) +// shared between those layers. +// +// The package has no dependency on github.com/duckdb/duckdb-go, so any +// caller that wants to operate against duckgres without linking libduckdb +// (notably the Flight client and a future control-plane-only binary) can +// import this package without dragging the DuckDB driver in. +package sqlcore + +import "context" + +// ColumnTyper provides type name information for a database column. +// *sql.ColumnType satisfies this interface. +type ColumnTyper interface { + DatabaseTypeName() string +} + +// RowSet represents a set of rows from a query result. +type RowSet interface { + Columns() ([]string, error) + ColumnTypes() ([]ColumnTyper, error) + Next() bool + Scan(dest ...any) error + Close() error + Err() error +} + +// ExecResult represents the result of a non-query execution. +type ExecResult interface { + RowsAffected() (int64, error) +} + +// RawConn provides access to the underlying driver connection. +// *sql.Conn satisfies this interface. +type RawConn interface { + Raw(func(any) error) error + Close() error +} + +// QueryExecutor abstracts database query execution, allowing both local +// (*sql.DB) and remote (Arrow Flight SQL) backends. +type QueryExecutor interface { + QueryContext(ctx context.Context, query string, args ...any) (RowSet, error) + ExecContext(ctx context.Context, query string, args ...any) (ExecResult, error) + Query(query string, args ...any) (RowSet, error) + Exec(query string, args ...any) (ExecResult, error) + ConnContext(ctx context.Context) (RawConn, error) + PingContext(ctx context.Context) error + Close() error + + // LastProfilingOutput returns the JSON profiling output from the last + // executed query, or "" if profiling is not enabled or not available + // (e.g. Flight SQL mode where the query ran on a remote worker). + LastProfilingOutput() string +} diff --git a/server/otelgrpc_filter.go b/server/sqlcore/otel.go similarity index 97% rename from server/otelgrpc_filter.go rename to server/sqlcore/otel.go index 59927275..b6150d7a 100644 --- a/server/otelgrpc_filter.go +++ b/server/sqlcore/otel.go @@ -1,4 +1,4 @@ -package server +package sqlcore import ( "strings" diff --git a/server/sqlcore/query.go b/server/sqlcore/query.go new file mode 100644 index 00000000..8359d519 --- /dev/null +++ b/server/sqlcore/query.go @@ -0,0 +1,41 @@ +package sqlcore + +import "strings" + +// IsEmptyQuery checks if a query contains only semicolons, whitespace, and/or +// SQL comments. PostgreSQL returns EmptyQueryResponse for queries like ";", +// "-- ping", "/* */", etc. +func IsEmptyQuery(query string) bool { + stripped := StripLeadingComments(query) + for _, r := range stripped { + if r != ';' && r != ' ' && r != '\t' && r != '\n' && r != '\r' { + return false + } + } + return true +} + +// StripLeadingComments removes leading SQL comments from a query. +// Handles both block comments /* ... */ and line comments -- ... +func StripLeadingComments(query string) string { + for { + query = strings.TrimSpace(query) + if strings.HasPrefix(query, "/*") { + end := strings.Index(query, "*/") + if end == -1 { + return query + } + query = query[end+2:] + continue + } + if strings.HasPrefix(query, "--") { + nl := strings.IndexByte(query, '\n') + if nl == -1 { + return "" + } + query = query[nl+1:] + continue + } + return query + } +} diff --git a/server/sqlcore_aliases.go b/server/sqlcore_aliases.go new file mode 100644 index 00000000..410991cd --- /dev/null +++ b/server/sqlcore_aliases.go @@ -0,0 +1,13 @@ +package server + +import "github.com/posthog/duckgres/server/sqlcore" + +// Re-exports kept here so existing references to server.IsEmptyQuery and +// server.OTELGRPCClientHandler continue to compile after the helpers +// moved into server/sqlcore. The sqlcore package is duckdb-free; new code +// (notably the Flight client) should import server/sqlcore directly. + +var ( + IsEmptyQuery = sqlcore.IsEmptyQuery + OTELGRPCClientHandler = sqlcore.OTELGRPCClientHandler +)