refactor: extract Backend/Operation seam for SEA integration by mani-mathur-arch · Pull Request #382 · databricks/databricks-sql-go

mani-mathur-arch · 2026-07-05T15:41:16Z

Summary

Introduce a pluggable execution-backend abstraction (internal/backend) and fold today's Thrift/HiveServer2 code into it as the sole implementation (internal/backend/thrift). conn no longer talks to the generated Thrift client directly — it holds one backend.Backend and drives session lifecycle and statement execution through the neutral Backend / Operation interfaces. No behavior change, no cgo, no new user-facing API.

This is Layer 1 of adding SEA (Statement Execution API) support: it draws the seam so a second, SEA-via-kernel backend can slot in behind a connect-time flag later, without touching conn, stmt, or any public API. Row decoding is not part of this neutral seam: internal/rows is coupled to Thrift result types today, so each backend supplies driver.Rows through its own construction path. A follow-up PR will decouple the rows layer before or with Layer 2.

Why

The driver is migrating to the SEA protocol, which we plan to add via the Rust kernel. Wiring a second protocol into conn as it stands would mean threading Thrift and SEA types through every database/sql method. Drawing the seam first — with Thrift as the only implementation — keeps that future change to a single factory branch and makes this PR a pure, verifiable refactor. It is the regression firewall the kernel PRs build on.

What changes

internal/backend — neutral Backend / Operation interfaces + ExecRequest / Param request types. Only a backend implementation imports its protocol's client (the Thrift backend imports internal/cli_service); nothing else in the driver does for the execute/poll/close/session path.
internal/backend/thrift — the Thrift execute/poll/close/session logic moved here behind the interfaces; thriftOperation wraps the execute + status responses and exposes only neutral accessors. The existing Thrift-mechanism tests moved with it.
conn / connector — drop the client + session fields for a single backend field; the 9 database/sql methods keep their signatures and delegate. The connector factory builds the Thrift backend (a future flag will select the kernel backend at that same point).
parameters.go — split the parameter conversion at a neutral backend.Param boundary (type inference stays in dbsql, wire-mapping moves to the backend) to avoid a dbsql↔backend import cycle. internal/querytags extracted for the same reason; the public dbsql.SerializeQueryTags forwards to it, unchanged.
internal/debuglog — a step-level debug tracer for the upcoming FFI work. It is a thin helper over the driver's existing logger, not a second logger: every step is an ordinary level=debug line on the same sink, in the same JSON/pretty format as every other driver log, so Go and (later) kernel-side stderr logs read as one interleaved stream in execution order. Turn it on the way you turn on any driver logging — SetLogLevel("debug") or DATABRICKS_LOG_LEVEL=debug (off by default; near-zero cost when off). Each step carries its function name, an enter/done phase (the enter is emitted before the step runs, so a step that hangs at the FFI edge is still visible), the elapsed time on done, and the conn/correlation/query ids from the context.

The data plane (internal/rows / rowscanner) was already backend-agnostic and is intentionally untouched.

Test plan

go test ./... -count=1 passes — unit tests + the moved Thrift-mechanism tests
Real-warehouse e2e (TestE2EArrowBatchesSurviveQueryContextCancellation, TestE2ECloudFetchExactRowCount) passes against a staging warehouse
CGO_ENABLED=0 go build ./... still works — the pure-Go default is preserved
go vet ./... and gofmt -s clean; golangci-lint clean; internal/debuglog and internal/backend/thrift race-clean under -race
Reviewed with isaac review and the code-review squad; all findings fixed (see below)

Review notes

Reviewed with isaac review (clean) and a 9-reviewer squad. All findings that touched behavior or coverage were fixed in this PR:

Cached the session/statement id — the refactor was recomputing SprintGuid per call vs. the old cached conn.id.
Added coverage for the operation-close skip branches (a subtest dropped in the test move).
Collapsed a redundant duplicate result method (Results/StagingResults were byte-identical).
Threaded the connect context into the client-init error (thrift.New) so the correlation id isn't lost.
De-duplicated a doubly-wrapped staging-metadata error.
Restored the queryId enrichment semantics (commit 2): the refactor had changed the execute path from fill-if-empty to overwrite-always and stopped firing the QueryIdCallback on the no-handle path. Both are now preserved and pinned by TestEnrichQueryId.
internal/debuglog emits through the existing logger, not a second one. An earlier revision added a trace-level sibling logger gated by its own DBSQL_DEBUG_LOG env var; @gopalldb flagged a data race on that sibling (SetLogOutput swapped it while query goroutines read it). Rather than patch it with an atomic.Pointer, the step tracer now writes through the single logger.Logger at debug level, gated by the existing SetLogLevel / DATABRICKS_LOG_LEVEL. This removes the second logger and its separate knob entirely, so the race is gone by construction — logger.go reverts to its pre-refactor shape — and there is one logger for maintainers to reason about, one format on the wire, and one place to redirect output. Net −230 lines.
Code-review squad follow-up (later commit): documented the exec-path vs query-path close-ownership split on Operation (query path hands teardown to Rows.Close), made thriftOperation.Close truly idempotent via a closed memo, restored the end-to-end composed-error assertion the move dropped, pinned the "Execute returns a non-nil Operation on error" contract, added StatementID / AffectedRows accessor tests, collapsed a byte-identical duplicate test constructor, and rewrote a handful of comments that narrated change history instead of stating the contract.

Deferred to a follow-up (no behavior change — internal only): the parameter conversion makes an extra intermediate pass ([]backend.Param) that the old single-pass build didn't. The output is byte-identical (proven by the shared param tests); it's a few extra allocations per parameterized query, inherent to the neutral seam, and fusing it would re-entangle the dbsql/backend split. Can be optimized later if it ever matters.

Also unrelated to this PR: the test named `"ExecContext currently does not support query parameters"` has been stale since parameters landed in 2023 (worth a cleanup ticket).

This pull request and its description were written with assistance from Claude Code.

Introduce a pluggable execution-backend abstraction (internal/backend) and fold today's Thrift/HiveServer2 code into it as the sole implementation (internal/backend/thrift). conn no longer talks to the generated Thrift client directly; it holds one backend.Backend and drives session lifecycle and statement execution through the neutral Backend/Operation interfaces. No behavior change, no cgo, no new user-facing API. ## Why The driver is migrating to the SEA (Statement Execution API) protocol, which we plan to add via the Rust kernel behind a connect-time flag. Wiring a second protocol into conn as it stands would mean threading Thrift and SEA types through every database/sql method. This change draws the seam first — with Thrift as the only implementation — so the kernel backend can slot in later without touching conn, stmt, the rows layer, or any public API. It is the regression firewall the kernel PRs build on. ## What changes - internal/backend: neutral Backend / Operation interfaces + ExecRequest / Param request types. Only a backend implementation imports its protocol's client (the Thrift backend imports internal/cli_service); nothing else in the driver does for the execute/poll/close/session path. - internal/backend/thrift: the Thrift execute/poll/close/session logic moved here behind the interfaces; thriftOperation wraps the execute + status responses and exposes only neutral accessors. - conn / connector: drop the client + session fields for a single backend field; the 9 database/sql methods keep their signatures and delegate. The connector factory builds the Thrift backend (a future flag selects the kernel backend here). - parameters.go: split the parameter conversion at a neutral backend.Param boundary (inference stays in dbsql, wire-mapping moves to the backend) to avoid a dbsql<->backend import cycle. internal/querytags extracted for the same reason; dbsql.SerializeQueryTags forwards to it, unchanged. - internal/debuglog: a step-level debug tracer for the upcoming FFI work, emitted through the existing logger as a trace-level sibling and gated by the DBSQL_DEBUG_LOG env var (off by default). Every backend step is instrumented so a failing or slow step is visible in one ordered stream. ## Test plan - [x] go test ./... -count=1 passes (unit + moved Thrift-mechanism tests) - [x] Real-warehouse e2e (cancellation + CloudFetch) passes against a staging warehouse - [x] CGO_ENABLED=0 go build ./... still works (pure-Go default preserved) - [x] go vet, gofmt -s clean; internal/debuglog and internal/backend/thrift race-clean under -race - [x] Reviewed with isaac review and the code-review-squad; findings fixed Deferred to a follow-up (noted in the PR): the queryId enrichment on the execute path is now overwrite-always rather than fill-if-empty (affects only app-set query ids), and the parameter conversion makes an extra intermediate pass (inherent to the neutral seam). This pull request and its description were written with assistance from Claude Code. Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>

The Layer-1 refactor replaced client.LoggerAndContext (which enriched the context queryId with fill-if-empty semantics) with an inline "if op.StatementID() != \"\" { NewContextWithQueryId(...) }". That changed two observable behaviors for callers who manage their own query id via driverctx: - it overwrote a caller-set queryId with the handle GUID (old code kept the caller's value), and - it skipped NewContextWithQueryId entirely on the no-handle path, so a registered QueryIdCallback never fired (old code always called it, even with an empty id). Restore both via a small enrichQueryId helper used at the ExecContext and QueryContext call sites, and pin the behavior with TestEnrichQueryId. This makes the refactor's "zero behavior change" claim hold on this path. Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>

gopalldb · 2026-07-06T05:54:26Z

⚠️ [MAJOR] · bug · logger/logger.go:98

Data race on debugLogger between SetLogOutput and debuglog step tracing

Non-atomic reassignment of the global debugLogger races with concurrent step tracing. The newly added debugLogger (logger.go:50) is a plain zerolog.Logger var, and DebugLogger() (logger.go:59) returns &debugLogger — a pointer to the mutable global. The public SetLogOutput overwrites the entire struct with a plain store at logger.go:98 (debugLogger = newDebugLogger(w)). Meanwhile, whenever debug tracing is enabled (DBSQL_DEBUG_LOG / SetEnabled), debuglog.event() reads that same struct through the pointer on every step (debuglog.go:143, logger.DebugLogger().Log()). A caller invoking the public SetLogOutput while queries are emitting debug traces is an unsynchronized read/write of a multi-field struct — a data race that -race will flag and that can surface a torn logger value in a long-running process. Notably, debuglog's own sibling globals (enabled, seq, clockOverride) are all atomic precisely because they are touched concurrently with logging; debugLogger was left unprotected.

SetLogOutput reassigned the package-global debugLogger (a plain zerolog.Logger) while the step tracer read it via DebugLogger() on every event from query goroutines — an unsynchronized read/write of a multi-field struct that -race flags and that can surface a torn logger value. Hold debugLogger in an atomic.Pointer[zerolog.Logger] instead: SetLogOutput stores a fresh logger, DebugLogger() loads a stable snapshot. Matches the atomic treatment already given to the sibling globals (enabled, seq, clockOverride). Add TestConcurrentSetLogOutputIsRaceFree, which reproduces the race (concurrent SetLogOutput during active tracing) and fails under -race on the plain-var version. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>

mani-mathur-arch · 2026-07-06T06:23:38Z

Thanks @gopalldb — fixed in e24e903.

[MAJOR] Data race on debugLogger between SetLogOutput and step tracing. You're right — the sibling gates (enabled, seq, clockOverride) were made atomic precisely because they're touched concurrently with logging, and debugLogger was left as a plain zerolog.Logger var that SetLogOutput overwrote wholesale while debuglog.event() read it through the DebugLogger() pointer on every step. -race flags it and a torn multi-field read is possible in a long-running process. Now held in an atomic.Pointer[zerolog.Logger]: the three assignment sites go through a small storeDebugLogger helper that Stores a fresh logger, and DebugLogger() returns debugLogger.Load() — a stable snapshot, so a concurrent SetLogOutput swaps in a new logger without mutating the one already handed out. This matches the atomic treatment the sibling globals already get.

Added TestConcurrentSetLogOutputIsRaceFree, which drives concurrent SetLogOutput against active tracing; I confirmed it reports DATA RACE under -race on the plain-var version and passes with the fix.

gofmt / go vet clean; go build ./... and the full internal/debuglog suite pass under -race.

…ogger The step tracer wrote through a second, trace-level logger (logger.DebugLogger) gated by its own DBSQL_DEBUG_LOG env var. That was a parallel logging knob other maintainers would have to learn, and it forced the atomic.Pointer swap in SetLogOutput to keep the sibling race-free. Route internal/debuglog through the existing logger.Logger at debug level instead, gated by the driver's own SetLogLevel / DATABRICKS_LOG_LEVEL. Step lines are now ordinary level=debug lines on the one sink, in the same JSON/pretty format as every other driver log, so Go and kernel-side stderr logs read as one stream. The helper keeps its real value: the before-the-step enter line (so a step that hangs at the FFI edge is still visible), enter/done elapsed timing, and the fn/phase/conn/corr/query fields. This removes the second logger entirely, so the debugLogger data race is gone by construction rather than patched: logger.go reverts to its pre-refactor shape (no atomic.Pointer, no shared out var), and the concurrent-SetLogOutput race test is dropped as moot. Also drops the now-unused seq counter and clock override (seq only ordered Go-vs-Go lines; cross-boundary interleave rides the shared logger's wall-clock time field). Net -230 lines. go test ./... green; -race clean on debuglog/thrift/logger; CGO_ENABLED=0 build preserved. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>

Resolve conflicts from #383 (native Arrow decimal handling) against the Backend/Operation refactor. #383 moved the authoritative useArrowNativeDecimal flag onto ArrowConfig (promoted as cfg.UseArrowNativeDecimal) and changed the in-conn executeStatement to read cfg.ArrowConfig.UseArrowNativeDecimal. This branch had already moved executeStatement/pollOperation out of connection.go into internal/backend/thrift, so the two conflicting hunks in connection.go and connection_test.go were main re-touching code that no longer lives there. Resolution: - connection.go / connection_test.go: keep this branch's version (the moved-out bodies are dead here). The decimal semantics #383 wanted are already satisfied in internal/backend/thrift/backend.go, which reads the promoted b.cfg.UseArrowNativeDecimal selector — Config embeds ArrowConfig and the DSN carrier is named UseArrowNativeDecimalDSN, so the selector is unambiguous. - connector.go / config.go and the rest of #383 (DSN bridge, WithArrowNativeDecimal, columnValues decimal128 path) auto-merged and are kept. go build ./... and go test ./... green; -race clean on backend/debuglog; CGO_ENABLED=0 build preserved. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>

golangci-lint's gosimple flagged time.Now().Sub(start) in the Track elapsed computation. Use time.Since(start), which is identical in behavior. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>

github-actions · 2026-07-07T07:05:24Z

Go integration tests triggered (passthrough). View workflow runs.

mani-mathur-arch · 2026-07-07T12:50:14Z

Code Review Squad — Review

Score: 77/100 — MODERATE RISK

A clean, genuinely behavior-preserving refactor on the control plane — security, language, ops, and performance reviewers found nothing at bar after reading the moved code against the pre-PR original, and no correctness or security regression ships here. The deductions are one architectural scoping gap (the rows/data plane stays Thrift-bound), two test-coverage regressions the move introduced, and interface-doc gaps that would bite the next PR (the SEA backend) more than this one. One devil's-advocate finding (QueryIdCallback allegedly stops firing on the param-error path) was dropped as a verified false positive — client.LoggerAndContext(ctx, nil) still fires it at the top of ExecContext/QueryContext.

Doc + test-only response to the multi-perspective review; no seam-behavior change beyond Close idempotency. - backend.Operation: doc close-ownership split (exec path uses Close, query path uses Rows.Close) and require both to be independently idempotent; carve row decoding out of the package-doc neutrality claim. - thriftOperation.Close: add a closed memo so a second call is a no-op (never re-issues CloseOperation, never re-fires CLOSE_STATEMENT). - Restore the end-to-end composed-error assertion the move dropped (unexpected-state + sqlstate across ERROR/CANCELED/TIMEDOUT/CLOSED). - Pin the Execute non-nil-Operation-on-error contract (pre-handle and post-handle terminal-error branches). - Add StatementID and AffectedRows accessor tests. - Collapse the byte-identical newTestBackend into NewForTest (~37 sites). - Rewrite "removed LoggerAndContext" comments to state the contract directly instead of narrating history. Co-authored-by: Isaac

github-actions · 2026-07-07T15:03:10Z

Integration test approval reset.

New commits were pushed to this PR. Label(s) integration-test-full were removed for security.

A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.)

Latest commit: 4b24a0e

github-actions · 2026-07-07T15:04:49Z

Go integration tests triggered (passthrough). View workflow runs.

Doc + test-only response to the multi-perspective review; no seam-behavior change beyond Close idempotency. - backend.Operation: doc close-ownership split (exec path uses Close, query path uses Rows.Close) and require both to be independently idempotent; carve row decoding out of the package-doc neutrality claim. - thriftOperation.Close: add a closed memo so a second call is a no-op (never re-issues CloseOperation, never re-fires CLOSE_STATEMENT). - Restore the end-to-end composed-error assertion the move dropped (unexpected-state + sqlstate across ERROR/CANCELED/TIMEDOUT/CLOSED). - Pin the Execute non-nil-Operation-on-error contract (pre-handle and post-handle terminal-error branches). - Add StatementID and AffectedRows accessor tests. - Collapse the byte-identical newTestBackend into NewForTest (~37 sites). - Rewrite "removed LoggerAndContext" comments to state the contract directly instead of narrating history. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>

github-actions · 2026-07-07T19:12:42Z

Integration test approval reset.

New commits were pushed to this PR. Label(s) integration-test-full were removed for security.

A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.)

Latest commit: 4efba05

github-actions · 2026-07-07T19:15:45Z

Go integration tests triggered (passthrough). View workflow runs.

vikrantpuppala

lgtm - almost perfect pr, thank you!

vikrantpuppala · 2026-07-08T16:20:50Z

let's name stuff as kernel rather than sea (in future prs)

Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>

github-actions · 2026-07-08T17:12:23Z

Integration test approval reset.

New commits were pushed to this PR. Label(s) integration-test-full were removed for security.

A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.)

Latest commit: fc8870a

github-actions · 2026-07-08T17:13:25Z

Go integration tests triggered (replay). View workflow runs.

mani-mathur-arch added 2 commits July 5, 2026 15:39

mani-mathur-arch marked this pull request as ready for review July 5, 2026 19:44

mani-mathur-arch requested a review from msrathore-db July 5, 2026 19:44

mani-mathur-arch requested a review from gopalldb July 6, 2026 06:23

mani-mathur-arch added 3 commits July 6, 2026 13:03

mani-mathur-arch added integration-test Preview the Go integration replay suite on this PR integration-test-full Preview the full passthrough integration suite (live warehouse) and removed integration-test Preview the Go integration replay suite on this PR labels Jul 6, 2026