feat(kernel): SEA-via-kernel backend (opt-in, behind build tag)#393
Open
mani-mathur-arch wants to merge 29 commits into
Open
feat(kernel): SEA-via-kernel backend (opt-in, behind build tag)#393mani-mathur-arch wants to merge 29 commits into
mani-mathur-arch wants to merge 29 commits into
Conversation
Add a second execution backend that runs statements over the Statement Execution API via the Rust databricks-sql-kernel, reached through a cgo C ABI. It is opt-in behind WithUseKernel + the databricks_kernel build tag, so the default build stays pure-Go (CGO_ENABLED=0) and is unaffected. Pure-Go side (always compiled): - WithUseKernel / WithWarehouseID connector options; UseKernel / WarehouseID on config with DSN parsing (useKernel, warehouseId). - connector.Connect selects the backend via newKernelBackend, which in a build without the tag is a stub returning a clear "not compiled in" error rather than silently falling back to Thrift. Kernel side (//go:build cgo && databricks_kernel): - KernelBackend/kernelOp implement backend.Backend/Operation over the C ABI: PAT session open (warehouse id or http path), blocking execute with out-of-band context cancellation (a watcher goroutine drives the kernel's detached statement canceller), and result streaming. - kernelRows imports Arrow batches zero-copy via the Arrow C Data Interface and scans the scalar types (ints, floats, bool, string, binary, date, timestamp, and top-level decimal as an exact string per #274); unsupported types return an explicit error. KernelError maps to the driver's error surface with sqlstate, and to driver.ErrBadConn for session-unusable statuses. Tests: tagged unit tests for error/status mapping and scalar scanning; live e2e (exercised against a staging warehouse) for select, per-type scanning, CloudFetch, and context cancellation, plus a Thrift-parity check that both backends render scalars identically. The cgo link directives point at a locally built kernel; committing a prebuilt per-platform static lib and adding a tagged CI job are a follow-up, so the kernel path is not yet covered by CI. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Extend the kernel backend's scanner to the complex data types, so the SEA path covers the full scalar-plus-nested type set. Nested Arrow values — list, map, struct, and VARIANT (which arrives as a nested value) — render to a JSON string that is byte-identical to the Thrift arrow path, so a query's result is the same across backends. GEOMETRY arrives as a WKT string and is read by the existing string arm. The renderer (scan_nested.go) recurses into child arrays and mirrors the Thrift marshal() rules: time.Time as a quoted .String(), and nested decimals as float64 (the exact-string decimal applies only to a top-level decimal column, #274). scanCell delegates nested columns here; genuinely unhandled types (interval/ duration) still return an explicit error. Tests: unit tests for list/map/struct/nested-null rendering; the live e2e data types table gains array/map/struct/variant/geometry cases; the Thrift-parity query gains the same, asserting byte-identical output across backends. Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Route the driver's existing connection options through to the kernel session, so a kernel-backed connection honors the same knobs as Thrift with no change to the user-facing API — only WithUseKernel selects the backend. The connector reads the same config the Thrift backend reads and translates it to the kernel's flat connection config: - Session confs (STATEMENT_TIMEOUT, QUERY_TAGS, TIMEZONE, …): the same SessionParams map Thrift forwards, applied one key at a time via set_session_conf. This covers Query Tags and server statement timeout. - TLS: TLSConfig.InsecureSkipVerify (WithSkipTLSHostVerify) maps to the kernel's skip-hostname-verification setter — the one TLS knob the driver actually honors today. - HTTP proxy: resolved via http.ProxyFromEnvironment for the connection's endpoint, the same cached HTTP(S)_PROXY / NO_PROXY decision the Thrift transport makes, then passed to set_proxy. No new dependency or option. - SPOG org routing continues to ride in the http path's ?o= (parsed kernel-side). M2M/OAuth is deliberately not included here: its credentials are held inside the authenticator rather than on config, so wiring it needs a small config change that is better done on its own. Tests: a proxy-resolution unit test; live e2e that reads query tags and statement timeout back from the server via SET (proving they were applied, not just accepted) and that a TLS skip-verify connection still succeeds. Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…leak Address three review findings on the kernel backend. Session timezone: DATE/TIMESTAMP values were rendered in UTC, ignoring the configured location, so a connection with a timezone returned times in a different zone than the Thrift backend. Forward cfg.Location into the kernel config and apply it (.In(loc)) when scanning dates/timestamps, including inside nested/JSON values — matching the Thrift path. nil location keeps UTC. Silently-dropped options: newKernelBackend ignored Catalog/Schema and EnableMetricViewMetadata. Neither is wired for the kernel yet — Catalog/Schema have no kernel C-ABI setter, and metric-view maps to a server session conf we want to route backend-neutrally rather than duplicate the Thrift literal here — so the backend now returns a clear error at connect time when either is set, instead of running with different behavior than Thrift. Both are follow-ups. Handle leak: kernelOp.Results returned without closing the operation when get_result_stream failed — and on the query path nothing else closes it, since the (absent) Rows was to own teardown. Close the operation on that error path. Tests: unit tests for timezone rendering (location applied vs nil=UTC) and the unsupported-option rejections; a live timezone e2e asserting the scanned timestamp carries the configured location. Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
A DECIMAL inside a nested value (struct field, list element, map value/key) was
rendered via ToFloat64, so it emitted a lossy float64 — e.g. a struct decimal
19.99 came out as {"d":19.990000000000002}, diverging from the Thrift path's
{"d":19.99}. The top-level decimal column was already exact (#274); only the
nested path was lossy.
writeJSON now renders a nested Decimal128 with the same exact-string helper the
top-level scan uses, emitted as a raw JSON number literal — mirroring the Thrift
arrow path's marshalScalar → ValueString (databricks-sql-go#253/#274). Map keys
go through scalarForJSON, which now defers to the scalar scan (exact string) as
well, so a decimal key is exact too.
The earlier nested tests missed this because they used float64-exact values
(1.5, 2.5). Add a struct-decimal unit case (19.99) and a nested-decimal column
to the live Thrift-parity query as regression guards.
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…le-free) Adapt the SEA-via-kernel backend to the kernel C ABI as merged (canceller #163 + logging/U2M #162), and fix a latent crash the sync surfaced. - Cancellation: kernel_statement_canceller_cancel now takes a bool* dispatched out-param. Route it through a fireCancel helper and stop the 250ms re-fire loop the moment a cancel actually dispatches (server id observed, RPC sent), instead of firing blindly until execute returns. - Logging: wire kernel_init_logging under a sync.Once, gated on the same DBSQL_KERNEL_DEBUG flag as the binding tracer, so one switch interleaves Go and kernel logs on stderr and both stay off by default / during benchmarks. level=NULL honors RUST_LOG; file=NULL uses stderr. Filter on target databricks::sql::kernel (colons), not the underscore module path. - Double-free: OpenSession freed the session config on the kernel_session_open failure path, but the kernel consumes the config on every path. Set consumed before checking the error so a failed open (e.g. HTTP 403) returns a clean error instead of aborting the process. - doc.go: correct the debug-logging env vars and refresh the supported-features summary (nested/complex types, TLS/proxy/session-conf now supported). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
crypto/tls's InsecureSkipVerify accepts any server cert — it disables both chain validation and the hostname check. The kernel path mapped it to only kernel_session_config_set_tls_skip_hostname_verification, leaving the kernel stricter than the Thrift path it mirrors: a self-signed cert + skip-verify succeeds on Thrift but was rejected by the kernel at chain validation. Also call kernel_session_config_set_tls_allow_self_signed under the same flag so both relax together, matching crypto/tls semantics and the pyo3/napi mapping. Also trim two over-detailed doc comments (call, fireCancel): drop the kernel-out-param aside and the internal "F4 window" name. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The #cgo CFLAGS/LDFLAGS hardcoded an absolute path to a developer's local
kernel checkout, so the package built only on that one machine and leaked a
local filesystem path into the repo. Drop the search paths from the directives
(keep only the library name) and document supplying the header/lib locations at
build time via the standard CGO_CFLAGS / CGO_LDFLAGS env vars. Also drop a
dangling reference to an internal-only design doc. Committing a per-platform
prebuilt static lib at a ${SRCDIR}-relative path + a tagged CI job is a
separate distribution follow-up.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
|
Go integration tests triggered ( |
|
Go integration tests triggered ( |
… tests Close the GA-readiness defects from the #393 review (verified against source). Correctness / doc-contract (High): - AffectedRows: cache the modified-row count at execute time. conn.ExecContext closes the op (nulling exec) before reading AffectedRows, so the previous live read returned 0 for every INSERT/UPDATE/DELETE/MERGE. - OAuth: reject non-PAT authenticators in newKernelBackend. OAuth/token-provider options leave AccessToken empty, so an empty PAT reached the kernel and failed with an opaque Unauthenticated error instead of the documented clear error. - Bound parameters: reject len(req.Params) > 0 in Execute with a clear error (non-nil Operation per the contract); they were silently dropped. doc.go now says params error at execute time, OAuth/metadata/namespace at connect time. - Context: fail fast on an already-cancelled ctx in nextBatch and OpenSession before the blocking C calls (database/sql's Rows.Close watcher still tears down an in-flight fetch). Parity / quality (Medium/Low): - Decimal: hoist the exact fixed-point formatter into internal/decimalfmt, shared by the Thrift and kernel paths so a #274-class fix lands in both. Its unit test runs in the default (untagged) build. - Nested FLOAT: marshal the native float32, not a widened float64, so ARRAY/MAP/STRUCT<FLOAT> match Thrift byte-for-byte (3.14, not 3.140000104904175). Added a nested-float parity test. - Observability: OnChunkFetched now reports a cumulative chunk count; kernel errors log at the driver's default (Warn) level (no SQL/PII) so a failure is visible without DBSQL_KERNEL_DEBUG. - klog logs sql.len, not raw SQL text (PII/secret safety), matching debuglog. - Precompute struct field-name JSON keys once per type instead of per row. - Fix the scan_nested header comment (nested decimals are exact, not lossy) and drop a POC-history comment. Tests: - config: DSN useKernel/warehouseId parse + malformed-useKernel error + both fields in the DeepCopy all-values case. - stub: default build asserts WithUseKernel(true) → Connect errors with a clear not-compiled-in message. - e2e cancellation: assert context.DeadlineExceeded and tighten timing. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The default CI matrix builds CGO_ENABLED=0, so the SEA-via-kernel backend (//go:build cgo && databricks_kernel) is not compiled or tested here. A dedicated CGO_ENABLED=1 -tags databricks_kernel job needs the kernel static library linked in CI, which is the kernel distribution work (pinned-source build or published .a) tracked as a #393 follow-up. Add a comment in go.yml so the gap is explicit rather than silent. The shared pure-Go decimal formatter (internal/decimalfmt) is already covered by the default matrix via its own untagged test. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
|
Integration test approval reset. New commits were pushed to this PR. Label(s) A maintainer must re-review and re-add a label to preview tests again. (The real gate runs in the merge queue.) Latest commit: 5852480 |
Drop the decimalfmt sentence from the go.yml comment; the deferral note only needs to explain why the tagged kernel-path job is absent. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
No behavior change. Addresses the doc/maintainability findings: - doc.go: drop "metadata commands" from the connect-time-error clause — there is no such guard and metadata runs as ordinary SQL (SHOW/DESCRIBE/ information_schema); note that explicitly (N8). - Remove rotting review/PR labels from comments (kernel_test.go, go.yml) while keeping the substantive rationale (N10). - kernelTestDB delegates to kernelTestDBWith instead of duplicating it (N11). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
- Top-level FLOAT columns now scan to a native float32, not a widened float64
(N3). Thrift returns float32 for a bare FLOAT, and database/sql's asString
formats at bit-size 32, so widening rendered CAST(0.1 AS FLOAT) as
"0.10000000149011612" vs Thrift's "0.1". The round-1 M2 fix covered only the
nested path; this closes the top-level scalar arm. Parity test gains a
top-level FLOAT case at a non-exactly-representable value, and the e2e float
case moves from 1.5 (exactly representable, masked the bug) to 0.1.
- close() reports closed=false for a handle-less op (N9). The bound-params error
path returns &kernelOp{} with no handle; conn.ExecContext closes it
unconditionally, and the previous unconditional true recorded a phantom
CLOSE_STATEMENT for a statement that never reached the server. Now matches the
backend.Operation contract (closed=false when there was no handle).
Tests: add float32_native scalar case, top-level FLOAT to the parity query, and
TestExecuteRejectsParams (asserts non-nil op, closed=false, AffectedRows 0).
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
No behavior change; the code is honest about two real limitations now. - nextBatch comment (N2): the previous comment claimed database/sql's cancel watcher tears down an in-flight fetch. It does not — Rows.Close takes closemu.Lock() which blocks until the in-progress Next (holding the RLock) returns, so the stream close waits for the blocking C call to finish. Read-path cancellation is honored only at batch boundaries; a single hung CloudFetch batch is uninterruptible (no per-download timeout). doc.go's "context cancellation" claim is scoped accordingly (real server cancel on execute; batch-boundary on read). - StatementID comment (N4): the kernel C ABI has no success-path statement/query id accessor (query_id is error-path only), so StatementID() is "" and per-statement telemetry (gated on != "") does not fire — there is no session-id fallback, contrary to the old comment. Documented; the accessor is a kernel follow-up. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The old TestProxyForEndpoint discarded the valid-config result and only asserted "" for the bad-config arm — which every no-proxy outcome returns, so a `return ""` stub would have passed. http.ProxyFromEnvironment snapshots the proxy env once per process (sync.Once), so env-based cases can't be driven mid-test. Extract the core into proxyForEndpointFunc(cfg, resolve), keeping proxyForEndpoint(cfg) as the thin production wrapper over http.ProxyFromEnvironment. The test now injects deterministic resolvers to assert all branches: proxy-set→URL, NO_PROXY/nil→direct, resolver-error→direct, and unbuildable-endpoint→direct (resolver never consulted). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The round-1 L4 change memoized struct field-name JSON prefixes in a process- global sync.Map keyed by *arrow.StructType. That leaks without bound: cdata.ImportCRecordBatch → importSchema → arrow.StructOf allocates a FRESH *StructType every batch (no interning), so the key never repeats across batches and the map grows one never-evicted entry per batch — a monotonic leak for a struct/nested-struct column over a large multi-batch CloudFetch result. The intended cross-row win only ever existed within a single batch anyway. Drop the cache and marshal the field name inline in writeStructJSON (the proven pre-L4 form). The per-row json.Marshal is cheap next to the once-per-batch cgo crossing; a correct per-batch precompute can return with the nested-renderer extraction if profiling warrants it. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…scan (N5/N6) Move ScanCell (scalar + nested→JSON grammar: native float32, exact decimals, time.Time formatting, list/map/struct) out of the cgo-tagged kernel package into a new pure-Go internal/arrowscan package. rows.go delegates via arrowscan.ScanCell. The renderer imports no C, so this is a pure move. Why: the rendering rules are the parity-critical contract both backends must agree on, but every kernel-package test is //go:build cgo && databricks_kernel and dead in CI. Relocating the tests to arrowscan makes them run in the default CGO_ENABLED=0 matrix (N6) — TestScanCellScalars/Nested/TimestampLocation now guard the native-float32 (N3/M2) and exact-decimal rendering on every build. The grammar is single-sourced for the kernel side (N5). Kernel-specific tests (error mapping, bad-connection, bound-params rejection) stay in the kernel package. Verified: default suite + arrowscan race pass; kernel unit tests pass; live Thrift-parity + all data types still byte-identical. Note: the Thrift arrowbased path still has its own nested renderer (built on its columnValues container abstraction, not raw arrow.Array). Having it delegate to arrowscan too — the remaining half of N5 — is a separate, higher-risk refactor of the primary production path; deferred. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Moving ScanCell out of the cgo-tagged kernel package into the untagged internal/arrowscan (N5/N6) put it under gosec in the default-build lint, which flags the uint64->int64 conversion (G115). Databricks SQL has no unsigned types, so a Uint64 column never occurs; driver.Value has no uint64 and the driver convention is int64, so the conversion is correct for every reachable value. Annotate the intent and suppress G115 on this unreachable arm, matching the repo's existing #nosec convention (connector.go G402). Verified with golangci-lint v2.12.2 (the CI version): 0 issues repo-wide. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
lastError already parsed the kernel's query_id into KernelError.QueryID, but the always-on Warn log and Error() both omitted it — so a kernel-path failure gave on-call one log line with no queryId and (because StatementID() is "") no metric, leaving no way to pivot to server-side query history. Include the queryId in the Warn log and append it to Error() when non-empty. A query id is a correlation token, not PII; error-path only, so no benchmark impact. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…(M1/M2/M3)
The kernel backend promises "nothing silently ignored", but three options leaked:
- PAT via WithAuthenticator (M1): the guard admits *pat.PATAuth, but the token was
read from cfg.AccessToken — which WithAuthenticator(&pat.PATAuth{AccessToken:...})
leaves empty (only WithAccessToken sets both). So a valid, Thrift-supported PAT
config reached the kernel with an empty token → opaque Unauthenticated. Resolve
the token from the authenticator when cfg.AccessToken is empty, and reject an
empty resolved token loudly.
- WithTimeout (M2): cfg.QueryTimeout maps to a per-statement server timeout on
Thrift (TExecuteStatementReq.QueryTimeout); the kernel C ABI has no equivalent
setter (verified against the header), so reject QueryTimeout > 0 rather than run
with no server-side timeout.
- WithRetries(-1) (M3): explicitly disables retries, but the kernel retries
internally with no toggle — reject the disable request. Positive retry tuning and
WithMaxRows can't be distinguished from defaults and are managed kernel-side, so
they're documented in doc.go as accepted-but-not-applied rather than rejected.
Tests cover all three rejects, the PAT-via-authenticator success path, and the
accepted positive-tuning path.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…st runs in CI (M4) proxyForEndpoint / proxyForEndpointFunc are pure Go (only config + net/http/url, no kernel C symbol), but lived in the cgo-tagged kernel_backend.go — so the now-meaningful TestProxyForEndpoint (four asserted branches) was inert under the only CI job (CGO_ENABLED=0). Split them into an untagged kernel_proxy.go + kernel_proxy_test.go, mirroring the arrowscan/decimalfmt move; newKernelBackend stays gated and calls proxyForEndpoint. The proxy test now runs in the default matrix without a kernel lib. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…kend parity (H1)
The nested-JSON grammar is rendered independently by the kernel path
(internal/arrowscan) and the Thrift path (arrowbased), and the two had diverged
on struct-key escaping: arrowscan does json.Marshal(fieldName) (escaped) while
arrowbased did a raw `"` + name + `"` concat — invalid JSON for a field name
containing a quote/backslash/control char (e.g. `a"b` → `{"a"b":1}`). Thrift was
the buggy one; fix it to json.Marshal the key so both agree on valid escaped JSON.
The only prior guard, TestKernelThriftParity, is build-tagged AND needs a live
warehouse, so it never runs in CI and the divergence shipped unnoticed. Add an
untagged cross-backend parity test (in package arrowbased, calling the private
container factory and importing arrowscan as a pure leaf) that feeds the same
arrow.Array through both renderers and asserts byte-equality — runs in the default
CGO_ENABLED=0 matrix, no kernel lib. Covers special-char struct keys, string/
special/int map keys, float32, and decimal; verified it fails on the pre-fix concat
and passes after. Map-key rendering was confirmed already-identical (different code,
same bytes), so left unchanged.
Full arrowbased single-sourcing (Thrift delegating to arrowscan) remains a separate,
higher-risk follow-up; this closes the correctness + CI-guard gap.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
No behavior change. - doc.go: add WithEnableMetricViewMetadata to the connect-time reject list (L2); note INTERVAL is not handled by the kernel scanner and returns a scan error (L1); caveat that the kernel backend surfaces no per-statement query id, so QueryIdCallback fires with "" and no EXECUTE_STATEMENT metric is emitted (M6). - columnValues.go: add a back-pointer above structValueContainer.Value noting the grammar is mirrored by internal/arrowscan and guarded by the parity test (L3), so a maintainer editing the Thrift renderer gets the signal. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Round 3's struct-key escaping fix (json.Marshal the field name) was correct but ran in structValueContainer.Value, the per-row hot path of the DEFAULT Thrift backend every customer uses — re-marshaling the same constant field names N_rows × N_fields times (~5M marshals + allocs on a 5-field × 1M-row result). Field names are invariant across rows and the container is built once per result (fieldNames is batch-stable, unlike the kernel's per-batch-fresh *StructType), so precompute the JSON-escaped `"name":` prefixes into svc.fieldKeys at construction and concat them in Value. Byte-identical output — the cross-backend parity test still passes — with zero per-row marshal. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…v) (M4)
writeJSONKey's default arm did json.Marshal(fmt.Sprintf("%v", k)), so a []byte
(BINARY) map key rendered as "[97 98 99]" while the Thrift path
(marshalScalar → json.Marshal) renders base64 "YWJj" — a byte-parity violation
for MAP<BINARY,_> (and large-magnitude float keys via %v vs marshal). Marshal the
key value directly (matching Thrift), quote-wrapping only when the result isn't
already a JSON string; keep the time.Time → quoted .String() special-case.
Parity test gains MAP<BINARY,_> and MAP<DATE,_> cases; verified map_binary_key
fails on the old %v path ({"[97 98 99]":9} vs {"YWJj":9}) and passes after.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…apes (M5) The parity test covered the flat surface + escaping/binary-key cases but omitted the shapes where two hand-maintained grammars are most likely to drift. Add recursive nesting (ARRAY<STRUCT>, STRUCT<LIST>, MAP<_,STRUCT>), a nested TIMESTAMP leaf (the time.Time → quoted .String() special-case, previously only tested top-level), and a null leaf inside a struct (vs a null container). Render through both backends under a non-UTC location so the timestamp .In(loc) path is actually exercised rather than a UTC no-op. All agree. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
toDriverError mapped a NetworkError/Unavailable status to driver.ErrBadConn on
every path, including execute and result-read. On the statement path that is
unsafe: a network failure surfaced *after* the SEA statement was sent may have
committed server-side, and driver.ErrBadConn makes database/sql transparently
re-run the whole statement — a silent duplicate write for a non-idempotent
INSERT/UPDATE/MERGE. This violates Go's own ErrBadConn rule ("never return
ErrBadConn if the server might have performed the operation").
Split classification by path:
- toConnError (session lifecycle: open/close/config — nothing executed) keeps the
bad-conn mapping so the pool evicts a dead session. Safe: no statement ran.
- toStatementError (execute + result read) never returns ErrBadConn; it returns
the KernelError unchanged (sqlstate preserved).
This restores the contract the rest of the stack already honors: the kernel itself
classifies ExecuteStatement as NonIdempotent (retried only on connect-phase
failures, per src/client/retry.rs), and the Thrift backend marks ExecuteStatement
non-retryable. The kernel has already exhausted its safe internal retries by the
time Go sees the error, so a second database/sql-level retry was pure hazard.
Tests: TestToStatementErrorNeverBadConn asserts no ErrBadConn for
network/unavailable/unauthenticated on the statement path; TestToConnError keeps
the session-path eviction behavior.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…fig fields (M2/M3) The round-3 auth/timeout/retry guards lived in newKernelBackend (cgo-tagged), and every test asserting them was tagged too — so CI (CGO_ENABLED=0) never compiled or ran them, and a config-field rename would silently delete the guards with make test still green (M2). Extract the pure-Go validation into an untagged kernel_config.go (validateKernelConfig: reject unsupported options + resolve the PAT). The tagged newKernelBackend now calls it, then assembles the cgo kernel.Config. The full reject/auth assertions move to an untagged kernel_config_test.go and run in the default build; the tagged test keeps only a smoke check that the wrapper wires validation + assembly. Add TestKernelConfigFieldsClassified (M3): a reflective test over config.UserConfig that fails if any field is unclassified (forwarded / rejected / inert), so a future WithFoo mapping to a wire setting can't be silently ignored on the kernel path — the divergence class rounds 2–4 closed. It already earned its keep by catching that TLSConfig lives on the outer Config, not UserConfig. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.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
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
Add a second execution backend that runs statements over the Statement Execution
API via the Rust
databricks-sql-kernel, reached through a cgo C ABI. It isopt-in behind
WithUseKernel(true)+ thedatabricks_kernelbuild tag, so thedefault build stays pure-Go (
CGO_ENABLED=0) and is unchanged — the only newuser-facing surface is
WithUseKernel.The user API is otherwise identical to Thrift: the kernel backend reads the same
config.Configthe Thrift backend reads and routes those options to the kernelinternally (the kernel setters are never exposed to the user), mirroring how the
kernel's pyo3 / napi bindings work.
What's implemented
Verified with unit tests + live e2e against a staging warehouse, and a
Thrift-parity check asserting both backends render results identically:
cancellation (watcher goroutine → the kernel's detached statement canceller),
server statement timeout, query tags
and the complex types — array, map, struct, variant — all rendered to JSON
byte-identical to Thrift; geometry (WKT). DECIMAL is rendered exactly, matching
Thrift: a fixed-point string for a top-level column and an exact JSON number
inside a nested value (never a lossy float64).
same decision Thrift makes), SPOG org routing (via the http path
?o=)DBSQL_KERNEL_DEBUG, which turns onthe binding's step tracer and installs the kernel's own Rust log subscriber so
both interleave on stderr; off by default (and during benchmarks). Kernel
verbosity is controlled by
RUST_LOG(targetdatabricks::sql::kernel).session-unusable status on the session-lifecycle path maps to
driver.ErrBadConnso the pool evicts the conn, while the execute/read pathnever returns
ErrBadConn(so a statement is never silently re-run)async HTTP, parallel Cloud Fetch
Deferred (rejected loudly at connect time where applicable)
Options that aren't wired yet return a clear error rather than behaving
differently than Thrift:
(needs a kernel change)
backend-neutrally rather than duplicate a Thrift-specific literal
InsecureSkipVerifyis honored today (mapped toboth kernel relaxations: chain + hostname, matching
crypto/tlssemantics andthe pyo3/napi bindings). The kernel's fuller TLS surface — a trusted-CA bundle,
an independent hostname-only skip, and mTLS client cert/key — is not exposed
yet. Doing so means adding new Go config params/setters (Go's native path
honors only
InsecureSkipVerify), so it's new public API rather than parity andis deferred to a follow-up.
(they arrive per-query, not at connect); previously they were silently dropped.
WithTimeout(server query timeout) andWithRetriesused to disableretries — rejected at connect (the kernel C ABI has no timeout setter and
retries internally).
WithMaxRows/ positive retry tuning are accepted butkernel-managed (documented, not applied).
OAuth (M2M/U2M) is not offered; a non-PAT authenticator is rejected with a clear
error at connect rather than reaching the kernel as an empty PAT. A PAT supplied
via
WithAuthenticator(&pat.PATAuth{...})is honored (token sourced from theauthenticator). Metadata is issued as ordinary SQL and runs like any other query.
Structure
internal/backend/kernel/(//go:build cgo && databricks_kernel): the cgobinding —
cgo.go(FFI-safe call helper + error mapping + logging init),backend.go(session open + config),operation.go(blocking execute +watcher cancel),
rows.go(zero-copy Arrow C Data Interface import; delegatescell rendering to
internal/arrowscan).internal/arrowscan/: pure-Go Arrow cell →driver.Valuerendering, includingthe nested List/Map/Struct → JSON grammar (native float32, exact decimals,
time.Timeformatting) that must stay byte-identical to Thrift. Untagged, soits rendering tests run in the default
CGO_ENABLED=0matrix rather than beingdead behind the kernel build tag.
internal/decimalfmt/: the exact fixed-point DECIMAL formatter, a dependency-free leaf package shared by the Thrift (
arrowbased) and kernel result pathsso a
#274-class precision fix lands in both at once. Untagged, so its unittest runs in the default
CGO_ENABLED=0matrix.WithUseKernel/WithWarehouseIDoptions,UseKernel/WarehouseIDconfig + DSN parsing, a build-tagged stub that returns a clearerror when the kernel backend isn't compiled in, and the factory branch in
connector.Connect.Test plan
CGO_ENABLED=0 go build ./...+ full pure-Go suite pass (default build unchanged)go build -tags databricks_kernel(CGO_ENABLED=1) compiles and linksrendering, proxy resolution, unsupported-option rejection
CloudFetch (1M rows), ctx cancellation, query tags / statement timeout
(read back via
SET), timezone, TLS skip-verifyboth backends renders identical rows
verified live on both the success and connect-failure paths
internal/arrowscancell/nested-JSON rendering(scalars, float32, exact decimals, list/map/struct, timezone), a
cross-backend parity test feeding the same
arrow.Recordthrough botharrowscanand the Thriftarrowbasedrenderer (special-char struct keys,map keys, float32, decimal),
internal/decimalfmtexact formatting, theproxy resolver seam (4 branches), auth/timeout/retry rejects + PAT-via-
authenticator,
useKernel/warehouseIdDSN parse +DeepCopy, and the"kernel not compiled in" stub error — all run under
CGO_ENABLED=0golangci-lint(v2.12.2, the CI version) clean repo-wideisaac reviewand a follow-up review pass (findings addressed— see below)
Code-review remediation
Four review passes surfaced correctness/contract defects (GA-readiness — the
backend has no production callers today, being behind the double opt-in). All
addressed and verified against both the default and kernel-tagged builds, plus
live e2e + Thrift-parity against a staging warehouse.
Round 1 — correctness & contract:
conn.ExecContextcloses the op (nulling the executed handle) before readingAffectedRows, so the previous live read returned 0 for everyINSERT/UPDATE/DELETE/MERGE.
token-provider / external / federated) at connect, and reject bound parameters
at execute, instead of silently sending an empty PAT / dropping the params.
float32, not a widenedfloat64, soARRAY/MAP/STRUCT<FLOAT>match Thrift byte-for-byte (3.14, not3.140000104904175).(
nextBatch) andOpenSessionpaths before entering the blocking C calls.internal/decimalfmtformatter (was duplicatedverbatim across the two backends).
OnChunkFetchedreports a cumulative chunk count; kernelerrors also log at the driver's default (Warn) level (no SQL/PII) so a failure
is visible without
DBSQL_KERNEL_DEBUG;kloglogssql.len, not raw SQL.Round 2 — regressions from round 1, incomplete fixes, and coverage:
prefixes in a process-global map keyed by
*arrow.StructType. The Arrow C Dataimport allocates a fresh
*StructTypeper batch (no interning), so the map grewone never-evicted entry per batch — an unbounded leak on multi-batch struct
results. Removed; the field name is marshaled inline.
FLOATcolumn still widened to
float64, soCAST(0.1 AS FLOAT)rendered0.10000000149011612vs Thrift's0.1. The scalar arm now returns nativefloat32; the parity + e2e tests gain a non-exactly-representable FLOAT case.Close()contract — the bound-params error path returns a handle-less&kernelOp{};Close()now reportsclosed=falsefor it (wastrue), so nophantom CLOSE_STATEMENT telemetry is recorded for a statement that never hit
the server.
internal/arrowscan) — the cell/nested-JSON grammarmoved out of the cgo-tagged kernel package into an untagged leaf package, so its
rendering tests (float32, exact decimals, nested grammar) now run in the default
CGO_ENABLED=0matrix instead of being dead behind the build tag.database/sql's watchertears down an in-flight kernel fetch (it can't —
Rows.closemublocks closeuntil the blocking
Nextreturns; read-path cancellation is batch-boundaryonly); documented that
StatementID()is""because the kernel C ABI has nosuccess-path id accessor, so per-statement telemetry does not fire (tracked as a
kernel follow-up); dropped a stale "metadata commands error at connect" claim
(metadata runs as ordinary SQL).
proxy-set / NO_PROXY / direct outcomes (was passable by a
return ""stub);removed a duplicated test helper and rotting review/PR labels from comments.
Round 3 — grammar divergence, fail-loud holes, and CI reach:
grammar is rendered independently by
arrowscan(kernel) andarrowbased(Thrift), and they had diverged:
arrowscanjson.Marshals struct field-namekeys (escaped), while Thrift did a raw
"+ name +"concat that producesinvalid JSON for a name with a quote/backslash/control char (
a"b→{"a"b":1}). Fixed the Thrift path to escape. The only prior guard(
TestKernelThriftParity) is build-tagged and needs a live warehouse, so itnever ran in CI — added an untagged cross-backend parity test that feeds the
same
arrow.Recordthrough both renderers and asserts byte-equality in thedefault matrix (special-char struct keys, string/special/int map keys, float32,
decimal). Verified it fails on the pre-fix concat and passes after.
WithAuthenticator(&pat.PATAuth{})passed the auth guard but shipped an empty token (the token was read from
cfg.AccessToken, which onlyWithAccessTokensets) → opaqueUnauthenticatedfor a valid, Thrift-supported config; now the token is sourced from the
authenticator and an empty resolved token is rejected.
WithTimeout(a serverquery timeout the kernel C ABI can't set) and
WithRetriesused to disableretries now error at connect instead of being silently dropped;
WithMaxRowsandpositive retry tuning are documented as accepted-but-kernel-managed.
queryIdsurfaced — the kernel error already parsed the serverquery_id;it is now included in the default-level Warn log and
Error(), restoring the onecorrelation handle to server-side query history (
StatementID()is still"").(
kernel_proxy.go), so its four-branch test now runs underCGO_ENABLED=0instead of being trapped behind the kernel tag.
Round 4 — a data-correctness hazard, a self-inflicted regression, and CI reach:
ErrBadConnon the execute path (High) — a network/unavailable statussurfaced after a statement was sent was classified as
driver.ErrBadConn,which makes
database/sqltransparently re-run the statement — a silentduplicate write for a non-idempotent INSERT/UPDATE/MERGE. Split classification:
toConnError(session lifecycle — nothing executed) keeps the bad-conn mappingso the pool evicts a dead session;
toStatementError(execute + result read)never returns
ErrBadConn. This restores the contract the rest of the stackalready honors (the kernel classifies
ExecuteStatementNonIdempotent; Thriftmarks it non-retryable; Go's
ErrBadConndocs forbid it when the server may haveperformed the op).
json.Marshal(fieldName)instructValueContainer.Value, the per-row hot pathof the default Thrift backend, re-marshaling constant field names N_rows ×
N_fields times. Precompute the escaped
"name":prefixes once at containerconstruction (the Thrift
fieldNamesslice is per-result-stable); byte-identicaloutput, zero per-row marshal.
[]byte—arrowscan's map-key path marshaledfmt.Sprintf("%v", key), so aMAP<BINARY,_>key rendered"[97 98 99]"vsThrift's base64
"YWJj". Now marshals the key value directly (matching Thrift);parity test gains binary/date map-key cases + recursive-nesting, nested-timestamp,
and null-leaf shapes.
guards lived in tag-gated code that CI never compiled. Extracted the pure-Go
validation into an untagged
validateKernelConfig(its full reject/auth testsnow run under
CGO_ENABLED=0), plus a reflective test that fails if anyconfig.UserConfigfield is unclassified — so a future option can't be silentlydropped on the kernel path.
WithEnableMetricViewMetadataadded to the connect-time reject list;QueryIdCallbackfires with""on the kernel backend (no per-statement id); aback-pointer on the Thrift struct renderer to its
arrowscanmirror + the parityguard.
Deferred (documented, not silently dropped):
CGO_ENABLED=1 -tags databricks_kerneltest job needs the kernel liblinked in CI, which is the distribution follow-up above;
go.ymldocuments thegap. The pure-Go renderers (
arrowscan,decimalfmt) are already covered by thedefault matrix.
needs the execute-path watcher applied to the read path (or a kernel stream-level
abort); today it is batch-boundary only.
arrowbasedpath still has itsown nested renderer (built on its
columnValuescontainer abstraction); havingit delegate to
arrowscantoo is a separate, higher-risk refactor of the primaryproduction path. The correctness + drift-guard gap is already closed by the
round-3 cross-backend parity test; this is now purely a dedup follow-up.
StatementID()is""(no kernel C-ABI success-pathaccessor), so per-statement metrics don't fire. The server
queryIdfrom theerror path is now logged for failures; full telemetry needs the kernel accessor.
This pull request and its description were written by Isaac.