build(kernel): 2-mode kernel build recipe + tagged CI job#395
Draft
mani-mathur-arch wants to merge 39 commits into
Draft
build(kernel): 2-mode kernel build recipe + tagged CI job#395mani-mathur-arch wants to merge 39 commits into
mani-mathur-arch wants to merge 39 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>
… 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>
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>
…5) + comment cleanup (L2-r5)
TestKernelConfigFieldsClassified used reflect NumField(), which reports the
embedded CloudFetchConfig as a single field — so its six promoted fields
(UseCloudFetch, MaxDownloadThreads, …) were invisible to the "no silent drop"
guard. A future WithCloudFetchX would slip through with the test green, the exact
divergence the guard exists to prevent. Recurse into anonymous struct fields so
each promoted field is classified individually; classify the six CloudFetch fields
as inert (the kernel does CloudFetch internally). Verified the guard now fails when
any one is unclassified.
Also drop review-round narration ("round-4", "rounds 2–4") from the file's
comments per the comment-style ruleset, keeping the design rationale.
Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…g permanent auth (H1-r5, L1-r5) H1 (round 4) stopped the statement/read path from returning driver.ErrBadConn — which correctly killed silent DML replay — but ErrBadConn was also the ONLY thing that evicted a conn whose server session died mid-life (nothing on the statement/read path mutates k.valid, and kernelOp/kernelRows held no backend ref). So after a warehouse idle-stop/restart GC'd the session, the pooled conn stayed IsValid()==true and every subsequent query failed identically, with no self-healing (Thrift evicts via "Invalid SessionHandle" → ErrBadConn). Thread the H1 needle: give kernelOp a *KernelBackend back-reference (mirroring thriftOperation's *Backend) and, on a session-fatal status, call markSessionDead() so SessionValid()→conn.IsValid() returns false and database/sql discards the conn on return to the pool — eviction WITHOUT ErrBadConn, so the statement is never re-run. Wired on every statement/read error path (new_statement, set_sql, execute, get_result_stream, get_schema, next_batch). Split the two notions the old isBadConnection conflated: - isBadConnection = TRANSIENT (Unavailable/NetworkError) → produces ErrBadConn on the session-lifecycle path so connect is retried. Unauthenticated dropped from it (L1-r5): a wrong/expired PAT is permanent, so retrying connect 3× is wasteful and can worsen auth rate-limiting — matching Thrift, which only treats an invalid session handle (not a 401) as bad-conn. - isSessionFatal = the broader eviction set (adds Unauthenticated): a dead session from any of these is evicted, but via markSessionDead, never ErrBadConn. Tests: TestIsSessionFatal, TestEvictIfSessionFatal (evicts + asserts not ErrBadConn), and TestIsBadConnection updated to exclude Unauthenticated. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…rnel path (M3-r5) writeStructJSON did json.Marshal(fieldName) per field, reached per row via kernelRows.Next → ScanCell — the exact per-row-marshal antipattern M1 fixed on the Thrift path, on the backend whose reason to exist is throughput. (The old comment claiming it was "cheap relative to once-per-batch" was wrong: it's per row.) Add a caller-owned StructKeyCache and ScanCellCached; kernelRows creates one in newKernelRows and passes it down, so escaped `"name":` keys are computed once per result set. The cache is Rows-scoped and freed with it — deliberately NOT a process-global keyed by *arrow.StructType, which would leak (the C Data import allocates a fresh *StructType per batch — round-2 N1). ScanCell stays as the nil-cache one-shot, so existing callers/tests are unchanged; a nil cache just recomputes inline. TestScanCellCachedMatchesUncached asserts the cached path is byte-identical to the uncached one across rows. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…rendering (M1-r5) Review r5 M1-r5 flagged a top-level DECIMAL divergence (kernel exact string vs Thrift lossy float64). Investigated + verified live against staging: it does NOT occur on any real configuration. In the DEFAULT config UseArrowNativeDecimal is false, so the execute request sets DecimalAsArrow=false and the server sends DECIMAL as a *string* column — no decimal128 arrives, so the container's ToFloat64 path is never reached. With the flag on, the top-level scan uses DecimalStringValue (exact). All three configs (thrift-default, thrift-nativeDecimal, kernel) return the exact string "1234567890123456.7890" (Go string) — confirmed by a live probe. The finding's premise (reasoned from the code path) missed the DecimalAsArrow wire behavior; no code change needed. The legitimate part — the parity test had no top-level scalar cases — is addressed: TestArrowbasedKernelTopLevelScalarParity covers int64/float32/float64/ string/timestamp/date32, and TestTopLevelDecimalRendering pins all three decimal behaviors (kernel exact string, Thrift DecimalStringValue exact, Thrift container Value lossy-float64) with the wire nuance documented, so the distinction can't silently drift into an actual result divergence. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
force-pushed
the
mani/sea-kernel-build-recipe
branch
from
ec1e033 to
de9c139
Compare
…test (L2/L3/L4-r6) - Delete structValueContainer.fieldNames: the round-4 M1 fix replaced it with fieldKeys (the precomputed escaped prefix) and left fieldNames allocated + populated but never read — a wasted per-struct alloc and a latent trap (it holds the raw *unescaped* names, so reaching for it could reintroduce the escaping bug). fieldKeys fully replaces it (L2-r6). - TestTopLevelDecimalRendering: the lossy-path assertion compared holder.Value(0) (a float64) against a string literal — an any-compare across types that's always false, so it only caught a type change, never the value. Assert v.(float64) == 1234567890123456.7890 (L4-r6). Drop the "(review r5 M1-r5)" thread-id from the comment (rots on merge), keeping the WHY (L3-r6). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
… safety classifier, docs - Narrow ints (M1-r6): kernel ScanCell returned int64 for TINYINT/SMALLINT/INT while Thrift returns native int8/int16/int32, so a top-level narrow int differed by Go type across backends (verified live). Return the native width to match Thrift (its prod callers make widening Thrift the breaking option; the kernel has none). Both are technically off the driver.Value spec (int64 only); unifying both on int64 is a tracked driver-wide follow-up (needs team sign-off — see decisions-and-deferrals.md). Parity test gains tinyint/smallint/int; e2e int expectations updated to native widths. - Port/Protocol (M2-r6): the drop-guard classified both "forwarded", but kernel.Config has neither field — the kernel takes a bare host and connects over https:443. So a non-default WithPort / non-https protocol was silently ignored on the kernel path (the exact divergence the "nothing silently ignored" contract forbids). Reject a non-default port/protocol in validateKernelConfig; reclassify as "rejected". Defaults (https/443) still validate. - Safety classifier in CI (M3-r6): isBadConnection/isSessionFatal/toStatementError/ toConnError + KernelError + the status consts enforce the H1 (no statement replay) and L1 (no permanent-auth retry) guarantees, but lived behind the cgo build tag and ran in no CI job. Move them to an untagged errors_classify.go (plain-int consts with compile-time lockstep assertions against the C enum in cgo.go); move their tests to an untagged errors_classify_test.go so they run under CGO_ENABLED=0. A regression now fails default CI instead of shipping green. - Docs (L1-r6): document useKernel/warehouseId DSN params + WithUseKernel/ WithWarehouseID options in doc.go. Verified: default suite + golangci-lint clean; kernel-tagged unit; live e2e + Thrift-parity against staging (all int widths and rejects confirmed). Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch
force-pushed
the
mani/sea-kernel-build-recipe
branch
from
de9c139 to
38c283f
Compare
This was referenced Jul 12, 2026
Strip the internal design-doc/review-round markers (H1, L1, round-2 N1) from kernel comments and the "~10s" inline-wait figure from the cancel-watcher note. The comments keep their meaning — no statement replay, no permanent-auth retry, per-result-set key cache — just without the internal shorthand that doesn't belong in shipped source. Comment-only; no behavior change. Co-authored-by: Isaac Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Add a reproducible build recipe for the opt-in SEA-via-kernel backend and a CI job that exercises it, so the databricks_kernel-tagged path is built and unit-tested in CI instead of being dead code. - KERNEL_REV pins the kernel commit (PR #163 head) to build against. - cgo.go drops the machine-specific link line; per-platform cgo_<os>.go files carry ${SRCDIR}-relative LDFLAGS and force static linking (-l:...a). - build/kernel-lib.sh builds the kernel .a from source at KERNEL_REV (cargo --no-default-features --features tls-rustls), with a KERNEL_LOCAL_A override; Makefile adds kernel-lib / build-kernel / test-kernel and a kernel-lib-download stub for the future published-.a path. - go.yml gains build-and-test-kernel (CGO + rust + cargo-via-JFrog); the pure-Go CGO_ENABLED=0 jobs are untouched. Built artifacts are gitignored. Co-authored-by: Isaac
- build-and-test-kernel: drop the "no warehouse creds → live e2e/parity self-skip" note (goes stale once creds are added to the job). - kernel-token step: retirement trigger is the repo going public, not a published .a — the source clone needs auth regardless of the download path, and the source-build option stays alongside the prebuilt .a. Co-authored-by: Isaac
mani-mathur-arch
force-pushed
the
mani/sea-kernel-build-recipe
branch
from
38c283f to
77e0231
Compare
…chain, job limits Addresses three build-hygiene findings from the PR-395 review: - F1 platform guard: add cgo_unsupported.go, compiled only on OS/arch combos with no LDFLAGS file (e.g. linux/arm64, darwin/amd64). The Makefile host== target guard passes there and builds a host .a, but the link then fails with an opaque "undefined reference to kernel_*". A build-time constant fails earlier, at compile, naming the supported targets. - F2 reproducible .a: pin rustc via rust-toolchain.toml (governs the kernel build under build/kernel-src/; kernel repo pins none), match the CI Rust step to it, pass cargo --locked (build against the kernel's Cargo.lock), and fold rust-toolchain.toml into both kernel cache keys so a toolchain bump busts the stale archive/build-tree caches. - F6 job limits: add timeout-minutes 30 and a per-ref concurrency group with cancel-in-progress to build-and-test-kernel, so a hung cargo can't park a protected-runner slot and repeated pushes don't stack heavy builds. Verified: pure-Go CGO_ENABLED=0 build stays clean; cgo_unsupported.go is excluded on linux/amd64 + darwin/arm64 and compiled on the unsupported combos; go.yml is valid YAML; the recipe script passes a shell syntax check. Co-authored-by: Isaac
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a reproducible build recipe for the opt-in SEA-via-kernel backend and a CI job that exercises it, so the
databricks_kernel-tagged path is built and unit-tested in CI instead of being dead code. This is the distribution follow-up that lets #393 leave draft.KERNEL_REVpins the kernel commit to build against (currently PR 163 head in the kernel, which carries the statement canceller the driver links; re-pin to the merge SHA once 163 lands).cgo.godrops the machine-specific link line; per-platformcgo_<os>.gofiles carry${SRCDIR}-relative LDFLAGS and force static linking (-l:…a). linux/amd64 is real; darwin/windows are documented stubs pending native runners. Acgo_unsupported.goguard fails at compile time (naming the supported targets) on any other OS/arch, instead of an opaqueundefined referenceat link.build/kernel-lib.shbuilds the kernel.afrom source atKERNEL_REV(cargo --release --locked --no-default-features --features tls-rustls), with a rev-stamped cache short-circuit, aKERNEL_LOCAL_Aprebuilt-archive override, and a fail-loud guard against source cross-builds. The Rust toolchain is pinned viarust-toolchain.toml(it governs the checkout underbuild/kernel-src/), so the archive is reproducible under a fixedKERNEL_REV.Makefileaddskernel-lib/build-kernel/test-kerneland akernel-lib-downloadstub for the future published-.apath.go.ymlgainsbuild-and-test-kernel(CGO + rust + cargo-via-JFrog), withtimeout-minutesand a per-refconcurrencygroup so the heavy source build can't park a protected-runner slot or stack redundant runs; the pure-GoCGO_ENABLED=0jobs are untouched. Built artifacts are gitignored.Multi-OS support comes via native per-OS runners (host == target) or staging a prebuilt
.a, not cross-compiling from source — matching the settled distribution design.Known items from review (deferred / pre-merge)
Tracked here rather than as code comments, since these are sequencing/scope notes that would go stale in the source.
Must resolve before merge:
push:trigger.go.yml'sbranches: [main, mani/sea-kernel-build-recipe]exists only to run CI on this stacked branch (its PR base is the feature branch, notmain). It must be removed before merge somaindoesn't carry a dead personal-branch trigger that would mint anINTEGRATION_TEST_APPtoken + clone the private kernel on any recreation of that branch.KERNEL_REVto a merged commit. It currently points at 163's PR-head SHA (9b90406), reachable only viarefs/pull/163/head— GC-able if that PR force-pushes/deletes, which would break cold-cache CI. Re-pin to the 163 merge SHA once it lands, sequenced with the kernel merge. (Merge order overall: feat(kernel): SEA-via-kernel backend (opt-in, behind build tag) #393 → main first, then this PR, since these files live in the kernel package that only exists in feat(kernel): SEA-via-kernel backend (opt-in, behind build tag) #393.)Deferred (out of scope for this PR):
kernel_e2e_test.go,SELECT 1→ scan) is env-gated and self-skips withoutDATABRICKS_HOST/DATABRICKS_HTTP_PATH/DATABRICKS_TOKEN, which this job doesn't set. It passes locally against staging (90 pass / 84 skip). Wiring those three secrets onto the job (un-skipping the e2e test) is deferred to the CI-dispatch / creds follow-up.cgo_darwin.go/cgo_windows.goLDFLAGS are never compiled. M0 is linux; per-OS runners are the multi-OS follow-up.Testing
make kernel-libbuilds the.a(reproducible sha256) from a clean clone, a cache-restored tree, and a leftover-source tree.make test-kernellinks the archive with CGO and passes thedatabricks_kernel-tagged unit tests (ok …/internal/backend/kernel).CGO_ENABLED=0 go build ./...still succeeds — default pure-Go build unaffected.GOOS≠ host) fails loud.cgo_unsupported.goguard verified: excluded on linux/amd64 + darwin/arm64 (kernel builds normally), compiled on the unsupported combos (clear compile error).Co-authored-by: Isaac