feat(kernel): SEA-via-kernel backend (opt-in, behind build tag) by mani-mathur-arch · Pull Request #393 · databricks/databricks-sql-go

mani-mathur-arch · 2026-07-09T08:18:13Z

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 is
opt-in behind WithUseKernel(true) + the databricks_kernel build tag, so the
default build stays pure-Go (CGO_ENABLED=0) and is unchanged — the only new
user-facing surface is WithUseKernel.

The user API is otherwise identical to Thrift: the kernel backend reads the same
config.Config the Thrift backend reads and routes those options to the kernel
internally (the kernel setters are never exposed to the user), mirroring how the
kernel's pyo3 / napi bindings work.

Testing & CI. The default pure-Go build (CGO_ENABLED=0) and its CI are
unaffected and green. The kernel path is verified locally (built with the kernel
lib via CGO_CFLAGS/CGO_LDFLAGS) and against a live staging warehouse. A tagged
CI job that builds/links the kernel is a distribution follow-up PR — the
pure-Go leaf packages it shares (arrowscan, decimalfmt, the error classifier,
config validation) already run in the default matrix here.

What's implemented

Verified with unit tests + live e2e against a staging warehouse, and a
Thrift-parity check asserting both backends render results identically:

Auth: PAT
Query execution: SQL read, Direct Results (inline), Cloud Fetch, ctx
cancellation (watcher goroutine → the kernel's detached statement canceller),
server statement timeout, query tags
Data types: primitives, binary, timestamp/date (in the session time zone),
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).
Connectivity: TLS skip-verify, HTTP proxy (from the standard proxy env,
same decision Thrift makes), SPOG org routing (via the http path ?o=)
Observability: opt-in debug logging via DBSQL_KERNEL_DEBUG, which turns on
the 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 (target databricks::sql::kernel).
Errors: kernel error → the driver's error surface with sqlstate; the
server queryId is logged on failure. A session-unusable status on the
session-lifecycle path maps to driver.ErrBadConn so the pool retries
connect; on the execute/read path it never returns ErrBadConn (so a
statement is never silently re-run) but marks the session dead so the dead conn
is evicted from the pool
Inherited from the kernel (verified, not re-implemented): retry, backoff,
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:

Initial namespace (catalog/schema) — the kernel C ABI has no setter yet
(needs a kernel change)
Metric-view metadata — maps to a server session conf; deferred to route
backend-neutrally rather than duplicate a Thrift-specific literal
Richer TLS setup — only InsecureSkipVerify is honored today (mapped to
both kernel relaxations: chain + hostname, matching crypto/tls semantics and
the 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 and
is deferred to a follow-up.
Bound query parameters and staging operations (volume PUT/GET/REMOVE)
— rejected with a clear error at execute time (both are per-statement, not
connect-time). Bound params were previously silently dropped; staging previously
reported success with no file transferred, which is why it's now detected from the
SQL and rejected up front.
WithTimeout (server query timeout) and WithRetries used to disable
retries — rejected at connect (the kernel C ABI has no timeout setter and
retries internally). WithMaxRows / positive retry tuning are accepted but
kernel-managed (documented, not applied).
Non-default WithPort / non-https protocol / custom WithTransport —
rejected at connect. The kernel connects over https:443 with its own HTTP stack
and has no port/scheme setter, and it never sees a Go RoundTripper, so each
would otherwise be silently ignored. Every one of these rejections wraps
errors.ErrNotSupportedByKernel, so a caller can detect the "kernel can't honor
this" case with errors.Is (e.g. to fall back to the default backend) instead of
matching message text.

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 the
authenticator). Metadata is issued as ordinary SQL and runs like any other query.

Structure

internal/backend/kernel/ (//go:build cgo && databricks_kernel): the cgo
binding — 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; delegates
cell rendering to internal/arrowscan).
internal/arrowscan/: pure-Go Arrow cell → driver.Value rendering, including
the nested List/Map/Struct → JSON grammar (native float32, exact decimals,
time.Time formatting) that must stay byte-identical to Thrift. Untagged, so
its rendering tests run in the default CGO_ENABLED=0 matrix rather than being
dead 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 paths
so a #274-class precision fix lands in both at once. Untagged, so its unit
test runs in the default CGO_ENABLED=0 matrix.
Pure-Go side: WithUseKernel / WithWarehouseID options, UseKernel /
WarehouseID config + DSN parsing, a build-tagged stub that returns a clear
error when the kernel backend isn't compiled in, and the factory branch in
connector.Connect.

Test plan

Code review

Hardened over several code_review_squad rounds. The early single-perspective
passes converged the design (28 → 87/100, "no High or Critical"); later full
9-reviewer passes then found and closed a further set — most notably a nested-value
offset bug (List/Map indexed via the un-sliced Offsets()[row] instead of the
offset-aware ValueOffsets, wrong/panicking for sliced arrays; now fixed and
guarded by sliced-array parity cases), a proxy silently dropped in warehouse-id
addressing mode, a custom WithTransport silently ignored, and a double-negative in
the rejection messages. Highlights across all rounds: fail-loud rejection of every
unsupported option (no silent divergence from Thrift), each wrapping
errors.ErrNotSupportedByKernel for programmatic detection; AffectedRows and
cross-backend JSON/decimal/float32/int rendering made byte-identical to Thrift and
guarded by an untagged parity test (all rows, multi-entry maps, sliced arrays);
execute-path errors never return driver.ErrBadConn (no silent DML replay) while
still evicting a dead session; and the safety-critical error classifier moved to an
untagged package so it runs in default CI. Full round-by-round detail lives in the
working docs, not here.

Follow-ups (documented, not silently dropped):

CI — a CGO_ENABLED=1 -tags databricks_kernel test job needs the kernel lib
linked in CI, which is the distribution follow-up above; go.yml documents the
gap. The pure-Go renderers (arrowscan, decimalfmt) are already covered by the
default matrix.
Mid-fetch cancellation — real interruption of an in-flight CloudFetch batch
needs the execute-path watcher applied to the read path (or a kernel stream-level
abort); today it is batch-boundary only.
Full renderer single-sourcing — the Thrift arrowbased path still has its
own nested renderer (built on its columnValues container abstraction); having
it delegate to arrowscan too is a separate, higher-risk refactor of the primary
production 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.
Statement-id telemetry + execution-error typing — StatementID() is "" (no
kernel C-ABI success-path accessor), so per-statement metrics don't fire; the
server queryId from the error path is logged for failures. Grouped with it:
ExecutionError currently returns the bare *KernelError, so
errors.Is(err, dbsqlerr.ExecutionError) and the DBExecutionError interface
don't match kernel failures the way they do on Thrift — the fix (wrap via
NewExecutionErrorWithState) lands with the statement-id work so the error-path
changes stay together.
Session-lifecycle deadlines — OpenSession / CloseSession block in an
uninterruptible cgo call and can't honor a ctx deadline mid-connect/close (the
kernel C ABI exposes no deadline/cancel on those calls). Same class and fix as
mid-fetch cancellation above; both are tracked kernel C-ABI asks.
Integer widths → int64 (driver-wide) — both backends currently return DB
integers as their native Go width (TINYINT→int8, …), matching each other but
technically off the driver.Value spec (which names only int64). Making both
spec-correct means changing the production Thrift path's return type, so it's
a separate PR needing maintainer sign-off; matching Thrift keeps the two backends
identical until then.

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>

github-actions · 2026-07-10T14:13:03Z

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

github-actions · 2026-07-10T14:40:52Z

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

… 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>

github-actions · 2026-07-10T20:58:49Z

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: 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>

vikrantpuppala · 2026-07-13T10:40:25Z

+
+// KernelDebugEnabled reports whether binding-level debug logging is on. Tests
+// assert the flag wiring; benchmarks assert it is false before measuring.
+func KernelDebugEnabled() bool { return kdebug }


KernelDebugEnabled() is exported dead code (0 callers) with a doc comment claiming tests/benchmarks assert it — none exist in the PR.

Fix: drop it until the benchmark that needs it lands, or add the asserting test now.

_{Posted by code-review-squad · /full-review · feedback: #code-review-squad-feedback}

vikrantpuppala · 2026-07-13T10:40:30Z

@@ -0,0 +1,180 @@
+//go:build cgo && databricks_kernel


klog and the installed kernel Rust subscriber emit unstructured lines to os.Stderr, not through logger.Logger, so they carry no connId/corrId/queryId and can't be correlated in a multi-conn process.

Fix: route klog through logger.Logger.Debug() or include k.sessionID in each line.

_{Posted by code-review-squad · /full-review · feedback: #code-review-squad-feedback}

…ctions Harden the kernel backend per a code-review pass on PR #393: - Staging (PUT/GET/REMOVE volume ops): reject at execute time instead of returning success with no file moved. The kernel path can't do the local file transfer and the C ABI exposes no IsStagingOperation signal, so IsStaging returning false was a silent no-op. New untagged isStagingStatement detects them from the SQL (stripping leading whitespace + -- / block comments so a comment-prefixed command can't slip through); documented in doc.go. - Status-drift guard: the [a-b]struct{} array-size assertion only failed when the C value exceeded the Go const, so a downward enum renumber went undetected and could silently misclassify errors. Switched to a bidirectional uint(a-b)|uint(b-a) assertion that fails on drift in either direction. - Typed rejections: add errors.ErrNotSupportedByKernel and wrap every kernel "unsupported option/feature" rejection with it, so callers can errors.Is the case instead of substring-matching the message. - evictIfSessionFatal: use errors.As, not a bare type assertion, so it still fires if a caller wraps the KernelError. - Session id: mint a process-unique atomic counter instead of fmt.Sprintf with the session handle pointer (a freed address can be reused, colliding telemetry/log correlation across connections). - Comment fixes: correct the stale cStatusCodeAssertions reference and the KernelDebugEnabled doc that claimed a nonexistent test. Untagged staging + config tests run under CGO_ENABLED=0. Verified: default suite green, kernel-tagged unit tests pass, gofmt clean. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>

github-actions · 2026-07-13T12:11:46Z

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: cbcd20a

mani-mathur-arch · 2026-07-13T12:23:08Z

Thanks for the thorough review, @vikrantpuppala. I've worked through all of it. Grouping by disposition:

Fixed on this branch (pushed)

Staging silent no-op (operation.go IsStaging) — the kernel path can't perform the local file transfer and the C ABI exposes no IsStagingOperation signal, so returning false was a silent success with no data moved. Now rejected
at execute time via a new isStagingStatement detector (strips leading whitespace + --//* */ comments so a comment-prefixed command can't slip through), with a doc.go caveat. Untagged + tested under CGO_ENABLED=0.
One-directional drift guard (cgo.go) — the [a-b]struct{} assertion only failed when the C value exceeded the Go const; a downward enum renumber went undetected. Switched to a bidirectional uint(a-b) | uint(b-a) assertion that
fails on drift in either direction.
Untyped rejections (kernel_config.go, bound-params in backend.go) — added errors.ErrNotSupportedByKernel and wrapped every "unsupported option/feature" rejection with it, so callers can errors.Is the case instead of
substring-matching. Added an errors.Is test.
evictIfSessionFatal bare type assertion — switched to errors.As so it still fires on a wrapped KernelError.
sessionID from handle pointer (kernel-%p) — replaced with a process-unique atomic.Uint64 counter; a freed pointer address can be reused and collide log/telemetry correlation.
Stale/incorrect comments — corrected the cStatusCodeAssertions reference (no such symbol; it's the assertion block in cgo.go) and the KernelDebugEnabled doc that claimed a test which doesn't exist yet.

These changes passed a clean Isaac Review pass (0 findings after validation).

Already addressed in stacked follow-up branches

Telemetry blind spot (StatementID() returns "" → no per-query metrics) — a stacked branch surfaces the server query id via kernel_executed_statement_query_id, which populates StatementID() and engages the existing telemetry
gate.
No CGO_ENABLED=1 -tags databricks_kernel CI job — this is the acknowledged H5 follow-up (blocked on the kernel-lib distribution work); the 2-mode build recipe + tagged CI job land in a stacked branch alongside that.

Deferred / tracked (with rationale)

Blocking CloseSession with no deadline — tracked as the fire-and-forget-close C-ABI gap; the clean fix needs a kernel_session_close_blocking (or watchdog) and is grouped with the other kernel C-ABI asks.
klog not routed through logger.Logger (no connId/corrId correlation) — tracked follow-up to unify kernel logging onto the driver's existing DATABRICKS_LOG_LEVEL knob rather than the separate DBSQL_KERNEL_DEBUG flag.
Repeated-map-key JSON (writeMapJSON) — this mirrors the existing Thrift renderer (parity holds); changing the emitted grammar is a cross-backend contract decision, so I'd rather do it deliberately in its own change than diverge the
two paths here.
Parity/escaping test strengthening (multi-entry maps, multi-row, independent escaped-literal assertion) — fair; queued as test-hardening follow-ups. The parity contract is guarded today, these widen the coverage.
WithWarehouseID godoc (inert without WithUseKernel) — minor doc clarification, will fold into the doc pass.

github-actions · 2026-07-13T12:56:41Z

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

vikrantpuppala · 2026-07-13T16:45:45Z

+// ExecutionError wraps cause as the driver's execution error. The kernel error
+// already carries the sqlstate (see KernelError), so this returns cause as-is
+// (nil when cause is nil), matching the neutral contract.
+func (o *kernelOp) ExecutionError(ctx context.Context, cause error) error {


kernelOp.ExecutionError returns toStatementError(cause) — the bare *KernelError — while the Thrift path wraps the cause via NewExecutionError, whose Is matches dbsqlerr.ExecutionError/DatabricksError and which implements DBExecutionError (SqlState(), QueryId()). KernelError has no Is method and exposes SQLState/QueryID only as struct fields.

Impact: an agent (or any caller) writing backend-agnostic error handling — errors.Is(err, dbsqlerr.ExecutionError), errors.As(err, &dbExecErr) to read SqlState() — sees it work on Thrift and silently fail on the kernel backend, despite comments claiming "matching the Thrift error surface." SQLState/QueryID become unreachable via the public API even though KernelError holds them.

Suggested fix: have kernelOp.ExecutionError wrap the cause in the same NewExecutionError (or an equivalent carrying KernelError.SQLState/QueryID) so kernel errors match the same sentinels and expose the same DBExecutionError methods as Thrift.

_{Posted by code-review-squad · /full-review · feedback: #code-review-squad-feedback}

vikrantpuppala · 2026-07-13T16:46:44Z

Code Review Squad — Re-review (round 2)

Score: 53/100 — HIGH RISK (was 40/100)

The "address review" commits genuinely closed 5 prior findings including 2 Highs (staging now rejected up front, drift guard made bidirectional, deferred rejections typed). This fresh pass at cbcd20a7 surfaced 8 new findings — including one High parity break the first round missed (kernel exec errors don't satisfy the public errors.Is/As taxonomy). Three prior Highs/Mediums remain open on their existing threads (telemetry blind spot, multi-entry map parity, CloseSession timeout). None block the default CGO_ENABLED=0 release; all matter before running WithUseKernel(true) in production.

…/doc/test gaps Second review pass on PR #393: - Nested-value offset bug (correctness): writeJSON indexed List/Map via the raw, un-sliced Offsets()[row] instead of the offset-aware ValueOffsets, so a sliced array — or a List/Map field of a struct that carries a logical offset (Struct.Field re-slices preserving data.offset) — read the wrong element range or panicked, breaking byte-parity with the Thrift path. Use ValueOffsets for List/Map; add data.offset by hand for LargeList (arrow-go's LargeList.ValueOffsets omits it) and FixedSizeList ((row+offset)*n). - Parity test hardening: render every row (not just row 0), and add multi-entry map, multi-row list, and sliced List/Map/struct-of-list cases. Confirmed these fail on the pre-fix indexing and pass after — a real regression guard for the offset fix. - Log-level noise: lastError logged every non-Success kernel status at Warn, including user faults (SqlError/InvalidArgument). Add an untagged isUserFault predicate and log user faults at Debug, reserving Warn for infra codes, so a fat-fingered query can't inflate the WARN rate on-call alerts key on. - doc.go: list the connect-time WithPort/non-https rejections alongside the other rejected options. - Staging wiring test: tagged TestExecuteRejectsStaging drives Execute with a staging statement (not just the detector in isolation), asserting an ErrNotSupportedByKernel-wrapped error and a handle-less op — guards the detector→Execute wiring against a refactor reopening the silent-no-op path. - Spurious cancel: after ctx.Done() the watcher re-checks the done signal before firing, so a query completing exactly at the deadline no longer dispatches a cancel RPC against an already-terminal statement. - Comments: pin the kernel_session_open ownership contract to the header's documented "CONSUMES config on both success and failure" guarantee, and document the process-wide first-call-wins kernel log subscriber. Verified: default suite green, kernel-tagged unit tests pass (incl. the new staging + isUserFault tests), gofmt clean. Isaac review clean (0 findings). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>

github-actions · 2026-07-13T17:23:57Z

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: 0a14eeb

Add "Deferred (tracked)" markers at the three deferred sites so a reviewer reading the source sees the rationale in place, instead of re-flagging them (the second review pass re-raised these precisely because the deferrals lived only in out-of-tree planning notes): - CloseSession: no-deadline blocking close (needs kernel close_blocking / a Go-side watchdog; grouped with the kernel C-ABI follow-ups). - klog: writes raw to stderr, not through logger.Logger, and gates on DBSQL_KERNEL_DEBUG rather than DATABRICKS_LOG_LEVEL (logging-unification follow-up). - writeMapJSON: duplicate map keys render non-unique JSON, matching the Thrift path; changing it is a cross-backend contract decision for its own PR. Comment-only; no behavior change. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>

mani-mathur-arch · 2026-07-13T17:36:31Z

Thanks again @vikrantpuppala. Here's a consolidated status across round 1 and round 2. Everything below is pushed to this branch;

Fixed on this branch

Correctness

Nested-value offset bug (round 2, High) — writeJSON indexed List/Map via the raw un-sliced Offsets()[row] instead of the offset-aware ValueOffsets, so a sliced array (or a List/Map field of a struct carrying a logical offset) read
the wrong element range or panicked. Now uses ValueOffsets for List/Map, manual +data.offset for LargeList (arrow-go omits it there), and (row+offset)*n for FixedSizeList. Verified against the arrow-go v12.0.1 source.
Staging silent no-op (round 1, High) — IsStaging returning false meant a volume PUT/GET/REMOVE reported success with no file moved. Now rejected at execute time via a SQL detector (strips leading whitespace + --//* */ comments
so a comment-prefixed command can't slip through).
One-directional drift guard (round 1, High) — the [a-b]struct{} assertion only failed when the C value exceeded the Go const; a downward enum renumber went undetected. Switched to a bidirectional uint(a-b) | uint(b-a) assertion.

Error/observability

Untyped rejections — added errors.ErrNotSupportedByKernel; every kernel "unsupported option/feature" rejection now wraps it so callers can errors.Is instead of substring-matching.
evictIfSessionFatal — errors.As instead of a bare type assertion (survives wrapped errors).
sessionID from handle pointer — replaced kernel-%p with a process-unique atomic.Uint64 counter (a freed address can be reused and collide correlation).
WARN log noise (round 2) — lastError now logs user faults (SqlError/InvalidArgument) at Debug and reserves Warn for infra codes, via a new isUserFault predicate.
Spurious cancel (round 2) — the cancel watcher re-checks the done signal after ctx.Done(), so a query completing exactly at the deadline no longer fires a cancel RPC against an already-terminal statement.

Tests / docs

Parity test now renders every row and adds multi-entry map, multi-row list, and sliced List/Map/struct cases — confirmed to fail on the pre-fix indexing and pass after (a real regression guard for the offset fix).
Escaping test gained an independent literal assertion (was only cached == uncached).
Tagged TestExecuteRejectsStaging drives Execute end-to-end (not just the detector in isolation).
isUserFault unit test.
doc.go: added WithPort/non-https to the connect-time reject list; WithWarehouseID godoc now states it's inert without WithUseKernel.
Corrected the stale cStatusCodeAssertions reference and the KernelDebugEnabled doc comment; pinned the kernel_session_open ownership contract to the header's documented "CONSUMES config on both success and failure" guarantee.

All of the above passed a clean Isaac Review pass (0 findings after validation), and the full default + kernel-tagged suites pass locally (incl. -race on the kernel package).

Planned for follow-up work (not in this PR)

These are on the roadmap for the follow-up work that builds on this backend; calling them out here so they're not lost, but they aren't part of this PR:

ExecutionError typing (round 2, High) — wrapping kernel exec errors so errors.Is(dbsqlerr.ExecutionError) / errors.As(&DBExecutionError) and SqlState()/QueryId() behave like the Thrift path.
Telemetry (StatementID() empty → no per-statement metrics) — surfacing the server query id to engage the telemetry gate, plus wiring OnClose for the CLOSE_STATEMENT metric.
Tagged CI job (CGO_ENABLED=1 -tags databricks_kernel) — depends on the kernel-lib distribution work (the acknowledged H5 follow-up).

If it'd help, I can link these once the follow-up changes are up for review.

Deferred / tracked (now marked inline in the source)

Because the second pass re-flagged items whose rationale lived only in planning notes, each deferred site now carries a Deferred (tracked) comment in the code itself:

CloseSession no-deadline blocking close — needs a kernel close_blocking (with deadline) or a Go-side watchdog; grouped with the kernel C-ABI follow-ups.
klog not routed through logger.Logger (no connId/corrId; own DBSQL_KERNEL_DEBUG knob) — logging-unification follow-up; also covers the process-wide first-call-wins subscriber note.
Duplicate map keys render non-unique JSON (writeMapJSON) — mirrors the Thrift path exactly (parity holds); dedup-last-vs-error is a cross-backend contract decision for its own PR.
Multi-entry/multi-row map parity — the parity test now covers this (see above), so this thread is addressed.

… fixes Full 9-reviewer review pass (75/100) at the prior head. Eight findings; seven fixed here, one documented as a tracked follow-up: - F9 (double-negative messages): the sentinel is "not supported by the kernel backend", so composing "is/are not yet %w" surfaced "not yet not supported" — literally asserting the feature IS supported. Dropped "not yet" at the five composing sites so %w reads correctly. - F7 (sentinel asserted on 1 of 8 branches): table-drove the rejection subtests so every one asserts errors.Is(ErrNotSupportedByKernel) (would have caught F9); same assertion added to TestExecuteRejectsParams. - F3 (WithTransport silently dropped): reject a custom Transport in validateKernelConfig (the kernel uses its own HTTP stack, never sees a Go RoundTripper) instead of classifying it inert; reclassified in the drop-guard map and documented. - F6 (proxy dropped in warehouse-id mode): proxyForEndpointFunc used ToEndpointURL(), which errors when HTTPPath=="" (warehouse-id addressing) and silently returned "" (direct), ignoring HTTPS_PROXY. Build the endpoint from scheme+host directly (only host matters for NO_PROXY); regression test added. - F10 (StructKeyCache unbounded intra-Rows growth): the cache is keyed by the per-batch-fresh *StructType, so it grew one never-evicted entry per batch over the Rows lifetime. Added StructKeyCache.Reset(), called at each nextBatch; the intra-batch memoization win is preserved. - F12 (comment artifact): removed the "round-2 N1" review-round id from the StructKeyCache doc. - F13 (sentinel undocumented): documented ErrNotSupportedByKernel + the errors.Is fallback recipe in doc.go's Errors section. - F1 (ExecutionError typing, High): documented as a tracked follow-up. It returns the bare *KernelError, so errors.Is(ExecutionError)/DBExecutionError silently no-op for kernel failures; the fix (wrap via NewExecutionErrorWithState) lands with the telemetry/statement-id follow-up. Replaced the misleading "neutral contract" comment with an accurate Deferred (tracked) marker. Also added a Deferred (tracked) marker to OpenSession: like CloseSession it blocks in an uninterruptible cgo call and can't honor a ctx deadline mid-connect (same kernel C-ABI gap, same fix). Verified: default + kernel-tagged suites pass, -race clean on the kernel and arrowscan packages, gofmt clean. Isaac review clean (0 findings). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>

mani-mathur-arch added 9 commits July 9, 2026 05:52

Merge remote-tracking branch 'origin/main' into mani/sea-kernel-backend

d87f614

mani-mathur-arch marked this pull request as ready for review July 10, 2026 14:12

mani-mathur-arch added the integration-test-full Preview the full passthrough integration suite (live warehouse) label Jul 10, 2026

mani-mathur-arch added integration-test-full Preview the full passthrough integration suite (live warehouse) and removed integration-test-full Preview the full passthrough integration suite (live warehouse) labels Jul 10, 2026

mani-mathur-arch added 2 commits July 10, 2026 20:48

github-actions Bot removed the integration-test-full Preview the full passthrough integration suite (live warehouse) label Jul 10, 2026

mani-mathur-arch added 11 commits July 10, 2026 21:08

ci(kernel): trim the kernel-path CI deferral comment

a0d1b47

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>