Skip to content

feat(kernel): consolidated DAIS gap-closure + PuPr feature set + nightly workflows for e2e tests#399

Merged
mani-mathur-arch merged 28 commits into
mainfrom
mani/sea-kernel-consolidated
Jul 17, 2026
Merged

feat(kernel): consolidated DAIS gap-closure + PuPr feature set + nightly workflows for e2e tests#399
mani-mathur-arch merged 28 commits into
mainfrom
mani/sea-kernel-consolidated

Conversation

@mani-mathur-arch

@mani-mathur-arch mani-mathur-arch commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

What

Extends the SEA-via-kernel backend for the Go driver with the full DAIS gap-closure

  • PuPr feature set: authentication, session setup, type rendering, query execution,
    telemetry, richer TLS, kernel-log routing, and CI.

This PR consolidates what was a 5-PR stack (#400 richer-TLS, #401 log-unify,
#402 nightly-e2e, #403 c2-dispatch) into a single reviewable PR. The stack was linear,
so the branches were fast-forwarded into this one — every change is preserved as a
distinct, logically-scoped commit (23 commits), so it reads commit-by-commit rather
than as one blob. #400#403 now show as merged (their commits live here).

Features

Authentication

  • OAuth M2M / U2M — the kernel drives its own auth flow from cfg.Authenticator
    (mirroring pyo3/napi). Adds a kernel.Auth descriptor + resolveKernelAuth, wired
    through the set_auth_pat / _m2m / _u2m C-ABI setters. cfg.Authenticator is the
    single source of truth (last-writer-wins, matching Thrift).
  • Unsupported auth is sentinel-wrapped — token-provider / external / federated
    authenticators reject with %w-wrapped ErrNotSupportedByKernel, the same
    programmatic-fallback contract every other unsupported kernel option follows.

Session setup

  • Initial namespace — post-connect USE CATALOG / USE SCHEMA with identifier
    quoting (the kernel C ABI has no namespace setter).
  • Metric-view metadataconfig.EffectiveSessionParams() folds the server conf in
    backend-neutrally, so the kernel path sends the identical conf Thrift does.

Type rendering

  • INTERVAL day-time & year-month rendering in internal/arrowscan (kernel returns
    native arrow values; formatted Go-side to the Thrift path's string form). Handles the
    math.MinInt64 / math.MinInt32 negation bound without overflow.
  • Parity coverage for TIMESTAMP vs TIMESTAMP_NTZ and VARIANT / GEOMETRY
    (live-probed on both backends).

Query execution & telemetry

  • Bound query parameters via the kernel raw-param C ABI
    (kernel_statement_bind_parameter). The positional/named + SQL-NULL/empty-string
    decision is a pure paramBindArg seam, unit-tested under CGO_ENABLED=0.
  • Kernel errors surfaced as DBExecutionError carrying sqlstate + server query id,
    so errors.AsSqlState() / QueryId() works on the kernel path. The execute
    path deliberately reports non-retryable (a sent statement may have committed — no
    double-write), matching Thrift.
  • Server query id via a real StatementID() accessor
    (kernel_executed_statement_query_id), threaded into EXECUTE_STATEMENT telemetry, plus
    CLOSE_STATEMENT telemetry on the result-read path.
  • Cancelled execute evicts a session-fatal conn and preserves the kernel error
    metadata (multi-%w: errors.Is(DeadlineExceeded) AND errors.As(*KernelError) both
    hold).

Richer TLS (Go options)

  • CA-bundle + independent hostname-skip setters mapped to the kernel's own TLS knobs
    (defensive PEM copy; the wholesale custom WithTransport stays rejected).

Kernel logging

  • Kernel log output routed through the driver log level with connId/corrId/queryId
    correlation; allocation-free above Debug (guards the Arrow-batch hot path).
  • scope note (please read before flagging):
  • This unifies the control knob, not the sink, and that boundary is intentional.
    DATABRICKS_LOG_LEVEL/SetLogLevel now drives both the Go binding lines and the
    kernel's Rust lines from one level. But they stay two physical streams: Go lines go
    through logger.Logger (and honor logger.SetLogOutput), while the kernel's Rust
    lines go straight to stderr — the kernel C ABI exposes no sink/callback hook, so
    there is currently no way to route them through the Go logger. Consequences a
    reviewer will notice, all expected:
  • A custom SetLogOutput/file captures only the Go lines; Rust lines stay on stderr.
  • With the default sink both land on stderr but only race there — not a single
    ordered, merged stream.
  • The Rust subscriber's level is sampled once, at the first kernel connect.

Fully merging both into one ordered sink requires a kernel log callback and is
tracked separately (); it's out of scope here because it's a kernel-side
ABI change, not a driver one.

CI

  • A scheduled + dispatchable nightly E2E workflow that runs the credential-gated
    e2e + Thrift-vs-kernel parity suites against a real warehouse (the ordinary CI injects
    no warehouse secrets, so those tests otherwise skip). SEA-via-kernel integration leg
    gated behind a kernel label.

Notable behavior

  • Bound query parameters are now supported (previously rejected at connect).
  • Staging operations remain rejected — the kernel path can't perform the local file
    transfer and the C ABI surfaces no staging signal.

Testing

  • Default pure-Go (CGO_ENABLED=0): go build / go vet / go test ./... — 24
    packages ok.
  • Tagged kernel path (CGO_ENABLED=1 -tags databricks_kernel, linked against a
    locally-built kernel .a at KERNEL_REV): 24 packages ok.
  • Live e2e + parity against a real warehouse: all pass — data types (incl.
    variant/geometry/decimal), NTZ, params-vs-Thrift (10/10), CloudFetch, cancellation,
    connection pool (40/40), StatementID, initial namespace, and M2M (OAuth
    client-credentials).
  • gofmt clean; Isaac Review clean (0 final comments).

Merge gate

  • Re-pin KERNEL_REV at merge time. It currently points at the kernel dependency's
    PR-head SHA (GC-able once that kernel change merges); bump it to the resulting kernel
    main SHA, re-sync the cgo drift assertions in cgo.go if any signatures changed, and
    run make test-kernel against the new rev.

This pull request and its description were written by Isaac.

@mani-mathur-arch
mani-mathur-arch force-pushed the mani/sea-kernel-consolidated branch from 543c7a2 to ba466d3 Compare July 14, 2026 15:36
Base automatically changed from mani/sea-kernel-backend to main July 14, 2026 17:52
@mani-mathur-arch
mani-mathur-arch force-pushed the mani/sea-kernel-consolidated branch 3 times, most recently from d0c999e to 2caa354 Compare July 14, 2026 18:53
@mani-mathur-arch mani-mathur-arch changed the title feat(kernel): consolidated DAIS gap-closure + PuPr feature set (supersedes #395/#396/#397) feat(kernel): OAuth, initial namespace, metric-view, param binding, extended-type rendering & query-id telemetry for the SEA-via-kernel backend Jul 15, 2026
@mani-mathur-arch mani-mathur-arch changed the title feat(kernel): OAuth, initial namespace, metric-view, param binding, extended-type rendering & query-id telemetry for the SEA-via-kernel backend feat(kernel): consolidated DAIS gap-closure + PuPr feature set Jul 15, 2026
Comment thread internal/arrowscan/arrowscan.go Outdated
Comment thread kernel_config.go Outdated
Comment thread kernel_parity_test.go
@mani-mathur-arch

Copy link
Copy Markdown
Collaborator Author

Two additional review findings from the PR pass could not be anchored inline because GitHub does not expose those exact lines in this PR diff:

  1. Kernel retryability is still dropped from DBExecutionError. KernelError.Retryable is copied from the C ABI, but kernelOp.ExecutionError only forwards sqlstate/query id into NewExecutionErrorWithState; DBError.IsRetryable() is derived from internal/errors.RetryableError in the cause chain. A retryable kernel execution failure will therefore surface as non-retryable unless the cause is wrapped when ke.Retryable is true, with a test asserting DBExecutionError.IsRetryable().

  2. The cancelled execute branch skips session-fatal eviction. If ctx.Err() != nil while kernel_statement_execute returns a session-fatal KernelError, the code returns only ctx.Err() and bypasses k.evictIfSessionFatal(execErr), leaving a dead session potentially valid in the pool and dropping kernel metadata from the returned error. It should still classify/evict on execErr while preserving cancellation semantics.

mani-mathur-arch added a commit that referenced this pull request Jul 15, 2026
…ction, bind-mapping test

Addresses the review pass on this PR (comments left on #399). Fixes the
actionable set; a follow-up Isaac re-review then flagged that one of the
requested changes (surfacing the kernel Retryable flag) was itself unsafe, so
that one is intentionally NOT made — see below.

- Interval negation overflow (High): formatDayTimeInterval / formatYearMonthInterval
  negated the full magnitude up front (v = -v). At math.MinInt64 (day-time µs) /
  math.MinInt32 (year-month) that wraps back negative, so every component came out
  negative AND a '-' was prepended — doubly-negated garbage — and both are
  representable Spark interval bounds. Now derive each component from the signed
  value and take its magnitude when formatting (abs64); widen year-month to int64
  before negating. Adds MinInt64 (µs + ns) and MinInt32 regression cases.

- Cancelled-execute skipped session-fatal eviction (Med): when execErr raced a ctx
  cancel, execute returned ctx.Err() without calling evictIfSessionFatal, leaving a
  dead conn marked valid in the pool. Hoist the evict above the ctx-cancelled
  branch so it fires on both paths (drained watcher first, so no race). Also wrap
  BOTH the ctx error and the kernel error with two %w verbs so
  errors.Is(context.DeadlineExceeded) still matches AND the *KernelError
  (sqlstate/queryId) stays reachable via errors.As instead of being dropped.

- Bind mapping had no executing coverage (Med): the live Param-binding proof
  (TestKernelParamsVsThrift) needs a warehouse and only runs in the credentialed
  nightly job, so the positional/named + SQL-NULL/empty-string decision shipped
  untested at PR time. Extract that pure decision into an untagged
  paramBindArg (bindparams.go), consumed by the cgo bindParams, and unit-test it
  under CGO_ENABLED=0 (TestParamBindArg). Rename a comment's dead
  TestKernelE2EParams reference to the real tests.

- Stale StatementID() comments (Low): both said StatementID() is "" on this
  backend, but this PR made it return the real server id on the success path.
  Scope the empty-id claim to the execute-error path.

NOT changed (Isaac re-review, MAJOR): surfacing the kernel's Retryable flag on
kernelOp.ExecutionError. That is exclusively the post-submission path, where a
network/unavailable failure may have already committed a non-idempotent
INSERT/UPDATE/MERGE — reporting IsRetryable()==true would invite an app to
double-write, and it diverges from the Thrift path (always non-retryable here).
This mirrors toStatementError refusing driver.ErrBadConn for the same reason.
sqlState/queryId extraction is unchanged; TestExecutionErrorNeverRetryable pins
that a Retryable KernelError still reports IsRetryable()==false. The connect-phase
path (toConnError), where the retryable signal IS safe, is unaffected.

Verified: default (CGO_ENABLED=0) + kernel-tagged suites pass, gofmt clean, Isaac
review clean (0 final comments).

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

Copy link
Copy Markdown
Collaborator Author

Both points addressed in f5ea652 — one fixed, one deliberately declined:

1. Retryability — intentionally NOT changed. A follow-up Isaac review flagged wrapping the kernel Retryable flag here as MAJOR: kernelOp.ExecutionError is exclusively the post-submission (execute / result-read) path, where a network/unavailable failure may have already committed a non-idempotent INSERT/UPDATE/MERGE. Surfacing IsRetryable() == true there would invite an app keying retry on it (the doc.go recipe) to double-write, and it diverges from the Thrift path, which always builds a non-retryable execution error here. This mirrors toStatementError refusing driver.ErrBadConn for the same reason. Added TestExecutionErrorNeverRetryable pinning that a Retryable KernelError still reports IsRetryable() == false; sqlState/queryId extraction is unchanged. The connect-phase path (toConnError), where the retryable signal is safe, is unaffected.

2. Cancelled-execute eviction — fixed. Hoisted k.evictIfSessionFatal(execErr) above the ctx.Err() branch so a session-fatal failure racing a cancel still evicts the dead conn from the pool. Also wrapped both the ctx error and the kernel error with two %w verbs, so errors.Is(err, context.DeadlineExceeded/Canceled) still matches and the *KernelError (sqlstate/queryId) stays reachable via errors.As instead of being dropped.

…initial namespace

Close the four DAIS-scope rows the SEA-via-kernel backend previously rejected at
connect time. All are Go-side only (no new kernel dependency): the OAuth setters
are on merged kernel #162, metric-view is an existing session conf, and the
namespace uses plain SQL.

- Metric view: config.EffectiveSessionParams() derives the server conf
  (spark.sql.thriftserver.metadata.metricview.enabled) once, backend-neutrally, so
  both backends send the identical conf. The Thrift OpenSession special-case is
  removed (behaviour-preserving); the kernel forwards it via SessionConf. Reject
  dropped; reclassified forwarded.
- Initial namespace: applied post-connect via USE CATALOG / USE SCHEMA (the OSS
  ODBC workaround) since the kernel C ABI has no catalog/schema setter. quoteIdent
  (untagged) backtick-quotes identifiers; a USE failure fails connect and closes
  the session. Reject dropped; reclassified forwarded.
- OAuth M2M/U2M: the kernel drives its own OAuth flow from raw credentials
  (mirroring pyo3/napi and the Node/Python kernel bindings), read off
  cfg.Authenticator — the single source of truth (last-writer-wins, matching
  Thrift). The m2m/u2m authenticators expose auth.M2MCredentialsProvider /
  auth.U2MCredentialsProvider; resolveKernelAuth type-switches them and returns a
  *kernelAuth descriptor. KernelBackend.setAuth branches to set_auth_pat /
  set_auth_m2m / set_auth_u2m; U2M uses Go's cloud-inferred client id (kernel
  defaults for scopes/port). No new config fields.

Verified: default CGO_ENABLED=0 suite + golangci-lint v2.12.2 clean; tagged
databricks_kernel unit tests (auth-mode -> setter mapping, quoteIdent); live
staging e2e for initial namespace (current_catalog/current_schema) and metric-view
(session opens + queries; the conf is not SET-introspectable on either backend).
M2M/U2M covered by unit tests (no staging service principal; U2M is interactive).

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
… single-source auth, coverage + docs

Remediation of the code-review pass on the DAIS gap-closure work. Verified against
source; full default + tagged suites, golangci-lint v2.12.2, and live staging e2e
all green.

- Move the OAuth credential-provider interfaces (M2MCredentialsProvider /
  U2MCredentialsProvider) out of the public auth package into internal/backend/kernel,
  so the secret-reading capability is not part of the driver's public API. The
  unexported m2m/u2m authenticators satisfy them structurally.
- Collapse the duplicate auth descriptor: validateKernelConfig/resolveKernelAuth now
  return kernel.Auth directly (its type is in an untagged file, so the default build
  builds it cgo-free); dropped dbsql.kernelAuth, kernelAuthMode, and toKernelAuth
  (which also removed a stale build-tag comment).
- Route the initial-namespace failure-path session close through call() so a failed
  close is logged (via lastError's Warn), mirroring CloseSession.
- Add an env-guarded live M2M e2e (TestKernelE2EM2M, skips without
  DATABRICKS_CLIENT_ID/_SECRET) and a last-writer-wins auth regression test; the
  resolveKernelAuth -> kernel.Auth path is table-tested for M2M/U2M.
- Document: U2M is interactive (browser on cache-miss, blocks up to the kernel's
  ~120s callback timeout, connect ctx deadline not honored during that window, no
  C-ABI override — use PAT/M2M for headless); the U2M Scopes/RedirectPort fields are
  dormant-but-wired (no Go option feeds them yet); the metric-view e2e is a
  deliberate connect-smoke (routing asserted in TestEffectiveSessionParams).

Custom M2M scopes remain unforwardable over the C ABI (no scopes arg on
set_auth_m2m) — a kernel gap shared with ODBC, no authz impact (all-apis always
requested); tracked in the kernel-gaps notes rather than worked around.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The kernel backend returns INTERVAL columns as native arrow duration
(day-time) and month-interval (year-month) values, whereas the Thrift path
receives them pre-formatted from the server (its native-interval config is
off in prod, so it never scans a duration/month-interval array). Format them
Go-side in the shared untagged arrowscan package to the same strings the
Thrift path returns — "D HH:MM:SS.nnnnnnnnn" and "years-months", negatives
signed — so a query's result is identical across backends.

Replaces the fail-loud "intervals are not yet handled" default arm with the
two type arms; golden-string unit tests (day/day-to-sec/seconds-unit/negative,
year/year-month/months/negative) run in the default CGO_ENABLED=0 build.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Over Arrow the kernel delivers TIMESTAMP with a tz ("UTC") and TIMESTAMP_NTZ
with an empty tz, but — like the Thrift path — the driver ignores that field
and renders both via ToTime + .In(loc). The LTZ-vs-NTZ difference is carried
entirely by the instant the server sends, not by the client inspecting the
tz, so no arrowscan change is needed: the existing code already matches Thrift.

Verified live on both backends (America/New_York + Asia/Kolkata, including a
DST spring-forward literal, and nested/null shapes): kernel == Thrift
byte-for-byte for both types. Add an untagged parity case (TimeZone "UTC" vs
"") so a future "don't shift NTZ" change — which looks correct in isolation
but would diverge from Thrift, which shifts NTZ too — fails default CI, plus a
live e2e pinning the round-trip.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
VARIANT and GEOMETRY need no special rendering on the kernel path: verified
live on both backends, both arrive over Arrow as plain STRING columns — a
top-level VARIANT is its JSON text ({"a":1,"b":[2,3]}), a scalar VARIANT is
"42", and GEOMETRY is its WKT "POINT(1 2)". Nested inside a container the
variant/geometry element is a string leaf, rendered as a quoted, JSON-escaped
string (the variant's own JSON is escaped as text, NOT re-parsed) — identical
on both backends.

Add untagged parity cases: a top-level string equivalence (variant object /
scalar / geometry WKT) and nested string-leaf cases in an array, so the string
arm's handling of these types can't silently drift between backends. GEOGRAPHY
is intentionally excluded — not enabled on the benchmark warehouse and no
consumer has asked for it.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
A failed kernel query returned the raw *KernelError, so consumers doing
errors.As(err, &DBExecutionError) — the way they inspect Thrift failures —
didn't get SqlState()/QueryId()/IsRetryable() through the standard interface.
kernelOp.ExecutionError now digs the sqlstate out of the underlying
*KernelError and wraps the cause via NewExecutionErrorWithState, so kernel
query failures surface with the same DBExecutionError shape as Thrift.

Adds the neutral NewExecutionErrorWithState to the untagged internal/errors
package (Thrift's NewExecutionError needs a TGetOperationStatusResp the kernel
backend can't produce), unit-tested in the default CGO_ENABLED=0 build. Parity
is type + SQLSTATE, not byte-identical text — kernel messages are richer (they
carry the SQL error class + suggestions). Verified live: unknown table → 42P01,
unknown column → 42703, byte-identical sqlstate to Thrift.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…al caveat

Record what the kernel backend inherits unchanged above the backend seam — the
database/sql connection pool (each conn wraps one kernel session), per-connection
CREATE_SESSION / DELETE_SESSION telemetry (recorded unconditionally in
connector.go, backend-agnostic), and the telemetry exporter's circuit breaker —
and that result types render byte-for-byte with Thrift (scalars, exact DECIMAL,
TIMESTAMP / TIMESTAMP_NTZ, INTERVAL, nested + VARIANT as JSON, GEOMETRY as WKT).

Remove the now-stale "INTERVAL types are not yet handled by the kernel scanner"
caveat (intervals render now), and narrow the telemetry caveat to what is
actually missing: only EXECUTE_STATEMENT telemetry (gated on a per-statement
query id the kernel C ABI doesn't yet surface) — CREATE_SESSION / DELETE_SESSION
are unaffected. Add a live-verified connection-pool e2e (40 concurrent queries
over pool cap 8) backing the inherited-pool claim.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
The kernel backend rejected bound parameters at execute time. Now it binds them:
the driver's backend.Param{Name, Type, Value} maps 1:1 onto the kernel's
kernel_statement_bind_parameter (K1) — value already stringified, Type the
Databricks SQL type name, empty Name → positional, nil Value → SQL NULL ("VOID").
bindParams runs after set_sql (which clears any prior binds), using the existing
newCStr/newCStrOrNull helpers and the call() FFI-safety wrapper; a bind failure
closes the statement and surfaces via toStatementError.

Removes the fail-loud reject in Execute (and its now-unused errors import). The
old TestExecuteRejectsParams is repurposed as TestExecuteHandleLessOpContract
(the non-nil handle-less Operation contract, now driven by a nil-session failure
since params no longer reject). Live parity: 10 cases (positional/named, each
scalar type, NULL, multi-param, predicate) produce byte-identical output on the
kernel and Thrift backends.

Requires a kernel build carrying kernel_statement_bind_parameter; the KERNEL_REV
pin is bumped to the K1 merge SHA when it lands.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
kernelOp.StatementID() returned "", so the kernel backend emitted no
EXECUTE_STATEMENT telemetry and QueryIdCallback fired with an empty id
(connection.go gates both on a non-empty statement id). Wire StatementID() to
the server query id via kernel_executed_statement_query_id (K1), captured at
execute time into a cached field — the same lifetime discipline as affectedRows,
since the C accessor returns a pointer borrowed from the exec handle and the op
is closed (nulling exec) before StatementID() is read on some paths. C.GoString
deep-copies out of the borrowed string.

Live e2e: a registered QueryIdCallback fires with a non-empty server id after a
kernel query. Updates doc.go — bound parameters (c6) and EXECUTE_STATEMENT
telemetry are now supported; the remaining kernel-backend limitation is
batch-boundary (not mid-fetch) read cancellation.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Bumps the kernel pin from the #163 canceller rev to the tip of the PuPr
statement-surface branch (databricks-sql-kernel#165), which adds
kernel_statement_bind_parameter and kernel_executed_statement_query_id — the two
C-ABI symbols the bound-parameter (c6) and EXECUTE_STATEMENT-telemetry (c7)
commits link against. Re-pin to the squash-merge SHA once #165 lands.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…ction, bind-mapping test

Addresses the review pass on this PR (comments left on #399). Fixes the
actionable set; a follow-up Isaac re-review then flagged that one of the
requested changes (surfacing the kernel Retryable flag) was itself unsafe, so
that one is intentionally NOT made — see below.

- Interval negation overflow (High): formatDayTimeInterval / formatYearMonthInterval
  negated the full magnitude up front (v = -v). At math.MinInt64 (day-time µs) /
  math.MinInt32 (year-month) that wraps back negative, so every component came out
  negative AND a '-' was prepended — doubly-negated garbage — and both are
  representable Spark interval bounds. Now derive each component from the signed
  value and take its magnitude when formatting (abs64); widen year-month to int64
  before negating. Adds MinInt64 (µs + ns) and MinInt32 regression cases.

- Cancelled-execute skipped session-fatal eviction (Med): when execErr raced a ctx
  cancel, execute returned ctx.Err() without calling evictIfSessionFatal, leaving a
  dead conn marked valid in the pool. Hoist the evict above the ctx-cancelled
  branch so it fires on both paths (drained watcher first, so no race). Also wrap
  BOTH the ctx error and the kernel error with two %w verbs so
  errors.Is(context.DeadlineExceeded) still matches AND the *KernelError
  (sqlstate/queryId) stays reachable via errors.As instead of being dropped.

- Bind mapping had no executing coverage (Med): the live Param-binding proof
  (TestKernelParamsVsThrift) needs a warehouse and only runs in the credentialed
  nightly job, so the positional/named + SQL-NULL/empty-string decision shipped
  untested at PR time. Extract that pure decision into an untagged
  paramBindArg (bindparams.go), consumed by the cgo bindParams, and unit-test it
  under CGO_ENABLED=0 (TestParamBindArg). Rename a comment's dead
  TestKernelE2EParams reference to the real tests.

- Stale StatementID() comments (Low): both said StatementID() is "" on this
  backend, but this PR made it return the real server id on the success path.
  Scope the empty-id claim to the execute-error path.

NOT changed (Isaac re-review, MAJOR): surfacing the kernel's Retryable flag on
kernelOp.ExecutionError. That is exclusively the post-submission path, where a
network/unavailable failure may have already committed a non-idempotent
INSERT/UPDATE/MERGE — reporting IsRetryable()==true would invite an app to
double-write, and it diverges from the Thrift path (always non-retryable here).
This mirrors toStatementError refusing driver.ErrBadConn for the same reason.
sqlState/queryId extraction is unchanged; TestExecutionErrorNeverRetryable pins
that a Retryable KernelError still reports IsRetryable()==false. The connect-phase
path (toConnError), where the retryable signal IS safe, is unaffected.

Verified: default (CGO_ENABLED=0) + kernel-tagged suites pass, gofmt clean, Isaac
review clean (0 final comments).

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…rnel

Follow-up to the review round: the unsupported-authenticator default case in
resolveKernelAuth still returned a plain errors.New, while every other
unsupported kernel option wraps ErrNotSupportedByKernel and doc.go advertises
that errors.Is(err, ErrNotSupportedByKernel) detects any unsupported kernel
feature. So this PR shipped a documented contract its own code broke for
token-provider / external / federated auth. Wrap it with %w to honor the
contract (same fix #403 makes one commit up the stack — matching its wording so
the two converge cleanly on rebase). The empty-PAT case stays unwrapped: a
missing token is misconfiguration to fix, not a feature the kernel can't honor.

Tighten the "non-PAT/non-OAuth authenticator rejected" test to assert
errors.Is(err, ErrNotSupportedByKernel) instead of only err != nil, so the
contract is pinned rather than documented as an exception.

Verified: default-build suite passes, go vet + gofmt clean.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…e skip

Expose the two kernel-only TLS knobs whose C-ABI setters already exist on
kernel main (no kernel change needed), via an experimental-option idiom:

  - WithKernelTrustedCerts(pem) -> kernel_session_config_set_tls_trusted_certs,
    adding a PEM CA bundle on top of the system roots. Required because the
    kernel's rustls stack ignores SSL_CERT_FILE, so a custom CA (corporate
    re-signing proxy / on-prem CA) must be handed over explicitly.
  - WithKernelSkipHostnameVerify() -> set_tls_skip_hostname_verification,
    skipping only the hostname check while keeping chain validation
    (finer-grained than the blanket WithSkipTLSHostVerify).

The knobs live on a non-exported config.KernelExperimentalConfig off
config.Config (not UserConfig), so they stay off the stable DSN surface
(mirroring Node's InternalConnectionOptions / Python's underscore kwargs).
The Thrift path rejects a non-nil block loudly at connect rather than
silently ignoring it, so a caller who forgets WithUseKernel learns the
option had no effect. OpenSession forwards each to the kernel C ABI via a
byte-buffer helper (cBytes); a reflective guard
(TestKernelExperimentalFieldsClassified) keeps a new field from slipping
either path unclassified.

mTLS client cert/key (needs an absent kernel C-ABI setter, K5) and the
CloudFetch on/off toggle (K3) are deliberately out of scope here — they are
tracked separately (PECOBLR-3652 / PECOBLR-3653).

Closes PECOBLR-3651.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…ift-reject test

Follow-up on the four review findings for the richer-TLS kernel options:

- Add ErrRequiresKernelBackend sentinel (mirror of ErrNotSupportedByKernel) and
  wrap the Thrift-path rejection with %w, so callers detect it via errors.Is
  instead of message text. Documented in doc.go.
- WithKernelTrustedCerts copies the PEM defensively (matching DeepCopy) so a
  caller mutating the slice between NewConnector and Connect can't change the
  trust store.
- Add a MITM WARNING to WithKernelSkipHostnameVerify, consistent with
  WithSkipTLSHostVerify.
- Add TestWithKernelOptionsRejectedOnThriftPath: end-to-end Connect() reject on
  the Thrift path, asserting on the sentinel.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
…erage

Non-blocking follow-ups:
- doc.go: mention ErrRequiresKernelBackend in the main Errors section, symmetric
  with ErrNotSupportedByKernel.
- Add TestWithKernelTrustedCertsCopiesPEM: mutating the caller's slice after the
  option must not change stored config (option-set counterpart to the DeepCopy
  aliasing test).
- TestConfig_DeepCopy: set KernelExperimental in the all-values case so
  Config.DeepCopy's wiring of that field is exercised, and assert it's copied not
  aliased.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
@mani-mathur-arch mani-mathur-arch added the integration-test-kernel Preview the full passthrough kernel integration suite (live warehouse) label Jul 15, 2026
@github-actions

Copy link
Copy Markdown

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

@mani-mathur-arch mani-mathur-arch changed the title feat(kernel): consolidated DAIS gap-closure + PuPr feature set feat(kernel): consolidated DAIS gap-closure + PuPr feature set + nightly workflows for e2e tests Jul 15, 2026
@mani-mathur-arch
mani-mathur-arch marked this pull request as ready for review July 15, 2026 19:09
@@ -56,7 +61,14 @@ type kernelRows struct {
// newKernelRows fetches the schema up front (for Columns()) and returns the row
// iterator; batches are pulled lazily on Next.
func newKernelRows(ctx context.Context, op *kernelOp, stream *C.kernel_result_stream_t, cb *dbsqlrows.TelemetryCallbacks) (driver.Rows, error) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — Failed result-schema fetch leaves EXECUTE_STATEMENT dangling; comment claims otherwise

When op.Results() errors (schema fetch/import fails after a successful server execute), QueryContext returns return rows, err (connection.go:378-383) with no AfterExecute/CompleteStatement call. The completing EXECUTE_STATEMENT telemetry lives only in closeCallback, which is armed on the Rows — but on this path no Rows is armed (callbacks are deliberately left unset until construction succeeds), so the callback never fires.

Result: EXECUTE_STATEMENT is BeforeExecute'd + FinalizeLatency'd but never completed — a dangling, never-finalized statement metric for a query that executed server-side but whose result-schema fetch failed. This contradicts the newKernelRows comment: "the construction error itself is surfaced to and recorded by the conn execute path."

Caveat: this asymmetry is shared with the Thrift path (same op.Results()return rows, err shape), and the trigger (schema fetch/import failing after a committed execute) is a narrow edge case.

Suggested fix

on the op.Results() error path in QueryContext, finalize the metric (AfterExecute + CompleteStatement(err)) the same way the execute-error path at connection.go:282-291 does; and correct the newKernelRows comment to match actual behavior.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a7228f6 . On the op.Results() error path in QueryContext, we now finalize the metric instead of returning early, the completing telemetry previously lived only in closeCallback, which never fires when no Rows is armed. We use the detached telemetryCtx, not the caller's ctx, because a Results-after-execute failure is often caused by ctx cancellation and the export threads that ctx to the metric's HTTP request, the raw ctx would abort the very metric we're adding. Returns nil rows so the discarded Rows can't later double-fire. The newKernelRows comment is now accurate as written (the error is surfaced and recorded).

@vikrantpuppala

Copy link
Copy Markdown
Collaborator

Code Review Squad — Review

Score: 92/100 — LOW RISK

Consolidated kernel-backend feature PR (39 files, 3003 lines), reviewed by 9 parallel reviewers. Code is exceptionally well-documented; boundary cases are explicitly handled and unit-tested. No correctness or security defects that block merge. Posting the one Medium finding below; the remaining Low findings are edge-case/observability nits (several documented as deliberate, tracked deferrals) and are not posted.


Comment thread kernel_config.go
// (the native Thrift path hardcodes both), so there is nothing to forward —
// but the kernel.Auth fields + setAuth wiring model the full set_auth_u2m
// surface for if/when such an option is added (see kernel.Auth docs).
return kernel.Auth{Mode: kernel.AuthU2M, ClientID: a.U2MClientID()}, nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] OAuth U2M requests different scopes on kernel than on Thrift — silent, backend-specific auth failure

The M2M path guards against scope divergence (M2MScopesSupported rejects a custom set), but the U2M branch forwards only ClientID, leaving kernel.Auth.Scopes nil → the kernel applies its own default scopes. The Thrift path unconditionally requests offline_access + sql (or, on Azure, <tenant>/user_impersonation) via oauth.GetScopes inside GetConfig (auth/oauth/u2m/u2m.go:15). On a workspace whose OAuth app grants only sql, kernel U2M fails (or over-requests) while Thrift U2M succeeds — contradicting the doc.go claim that kernel U2M uses "the same client id the Thrift path would." U2MCredentialsProvider exposes no U2MScopes() and the authenticator doesn't store its scopes, so the gap can't be closed without an interface change — yet nothing rejects the mismatch the way M2M does.

Suggested fix

extend U2MCredentialsProvider with a U2MScopes() accessor and forward the resolved scopes into kernel.Auth.Scopes, or reject U2M-over-kernel with an explicit error until scope parity exists (mirroring the M2M custom-scope rejection).

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed as a doc fix in a7228f6. The scope difference is real (kernel requests all-apis+offline_access, Thrift sql+offline_access), but neither backend exposes a U2M-scopes option, so it's a fixed-vs-fixed default difference, not a dropped caller choice. Both use the same built-in databricks-sql-connector public client, which all-apis targets so the "Thrift succeeds / kernel fails" outcome depends on a server-side client registration that isn't reproducible from a custom sql-only app. Rather than forward sql (which would swap the kernel onto an untested scope set), I've corrected the overclaim: the U2MClientID comment and doc.go now state the client id is shared but the default scopes differ. If a U2M-scopes option is ever added, kernel.Auth.Scopes is already wired through set_auth_u2m to close it. Could take the complete fix with tests as a follow up.

Comment thread kernel_config.go
// an opaque Unauthenticated.
func resolveKernelAuth(cfg *config.Config) (kernel.Auth, error) {
switch a := cfg.Authenticator.(type) {
case kernel.M2MCredentialsProvider:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Real OAuth authenticators bind to the kernel provider interfaces structurally with no compile-time assertion — a method rename silently disables M2M/U2M

authClient.M2MCredentials/M2MScopes and u2mAuthenticator.U2MClientID satisfy kernel.M2MCredentialsProvider / U2MCredentialsProvider only structurally — there is no var _ kernel.M2MCredentialsProvider = (*authClient)(nil) assertion anywhere, and the unit tests bind fake*Auth types, never the real ones. The one live test (TestKernelE2EM2M) self-skips in CI. Renaming/reshaping a real method compiles clean and go test ./... stays green, but at runtime the authenticator no longer matches its case, falls to default, and every OAuth-over-kernel connect is rejected as unsupported.

Suggested fix

add compile-time assertions (var _ kernel.M2MCredentialsProvider = (*authClient)(nil) and the U2M equivalent) in a package importing both, or drive resolveKernelAuth with the real m2m/u2m constructors in a unit test rather than fakes.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a7228f6. Added TestResolveKernelAuthRealAuthenticators, which drives resolveKernelAuth with the real m2m/u2m constructors (not the fakes) so a rename of M2MCredentials/M2MScopes/U2MClientID now fails the build/test instead of only breaking at runtime. The test-in-dbsql form covers both. Also verified live: M2M-over-kernel authenticates end-to-end against a real warehouse.
The tests were skipped as I did not know if we should add the m2m creds in the repo as no other tests were using them so that is the only test that is done locally and not exercised in ci or the nightly runs.

if !kdebug {
return
}
initLoggingOnce.Do(func() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Kernel Rust log subscriber now installed unconditionally at WARN, to raw stderr, bypassing logger.SetLogOutput

Previously the subscriber installed only when DBSQL_KERNEL_DEBUG was set (if !kdebug { return }, now removed); initLoggingOnce.Do now fires on the first OpenSession in every configuration, installing a process-wide, first-call-wins Rust subscriber at the mapped driver level (WARN default). Consequences: (1) kernel WARN/ERROR now prints to stderr for all users, bypassing a configured SetLogOutput sink; (2) a host process using its own in-process Rust tracing stack silently loses/conflicts (failed install only logged at Warn); (3) the level is sampled once — a later SetLogLevel can't re-quiet it.

Suggested fix

consider mapping an unset driver level to OFF unless explicitly opted in, and prominently document the stderr split-sink limitation in the runbook.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not changing this here. the substantive part is fixed one PR up the stack in #411. #411 swaps kernel_init_logging (stderr) for kernel_set_log_callback, routing kernel logs through the driver's logger.Logger and honoring SetLogOutput which resolves the "bypasses the configured sink" consequence. The residual items (process-global first-call-wins, level sampled once) are kernel-ABI properties, not fixable Go-side, and are documented in both PRs. Installing at WARN by default is intentional: WARN/ERROR should surface the problem was the destination, which #411 fixes. Re-gating here would just conflict with #411's rewrite of these lines.

Comment thread kernel_backend.go Outdated
// WithKernelSkipHostnameVerify), if any. These have no Thrift-path equivalent
// (the connector rejects them on that path) and are forwarded verbatim to the
// kernel C ABI in OpenSession.
if ke := cfg.KernelExperimental; ke != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] KernelExperimental TLS forwarding in newKernelBackend has no test

Nothing asserts that newKernelBackend actually copies cfg.KernelExperimental.{TLSTrustedCertsPEM,TLSSkipHostnameVerify} into kernel.Config. Tests cover option→config, the cgo setters with a hand-built Config, and Thrift rejection — but deleting these two forwarding lines would silently pass all unit tests, dropping a caller's WithKernelTrustedCerts / WithKernelSkipHostnameVerify. No e2e exercises them either.

Suggested fix

add a test asserting a Config built by newKernelBackend from a cfg with KernelExperimental set carries both TLS fields.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a7228f6. Extracted the config assembly into a pure, untagged buildKernelConfig seam so the KernelExperimental TLS forwarding is asserted under CGO_ENABLED=0 (TestBuildKernelConfig) deleting the forwarding lines now fails a test rather than silently passing.

// kernel (sea) leg is previewed on demand via the kernel labels but
// is not a required gate — it becomes one once the SEA backend ships
// in a released driver and its recordings are captured.
go_mode: 'thrift',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Kernel↔Thrift parity tests don't run on the required merge gate

The merge-queue required gate hardcodes go_mode: 'thrift'; kernel runs only via the opt-in integration-test-kernel label or the nightly cron (TestKernelThriftParity, TestKernelParamsVsThrift). The byte-for-byte parity guarantee doc.go advertises rests on live tests excluded from the required merge path, so a Go-side kernel divergence (interval formatter, param mapping) can merge and is caught only on the next nightly.

Suggested fix

consider adding a kernel replay/parity leg to the required gate once committed recordings exist.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged and intentional for now as the kernel replay recordings do not exist yet as the features are still being implemented. the nighty and the integration tests are used for testing along with local testing

# Guard: fail loudly if a credential is missing rather than letting the tests
# silently t.Skip() into a misleading green. The token may be provided as either
# _TOKEN or _TOKEN_PERSONAL (the tests accept either), so require at least one.
- name: Verify warehouse credentials are present

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] Warehouse-credential guard duplicated byte-for-byte across both nightly-e2e jobs

The "Verify warehouse credentials are present" step (env block + guard + ::error:: text) is copy-pasted into both thrift-e2e and kernel-e2e (also at lines ~204-214). Adding/renaming a required secret requires editing both; a one-sided edit drifts silently. The repo already uses composite actions (./.github/actions/setup-jfrog).

Suggested fix

extract the guard into .github/actions/verify-warehouse-creds/action.yml and reference it from both jobs.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving it for a small follow-up rather than this PR as it does not break anything

@msrathore-db

Copy link
Copy Markdown
Contributor

Code Review Squad — Review

Score: 75/100 — MODERATE RISK

Large PR (39 files, ~3,200 lines) landing the full DAIS gap-closure feature set on the opt-in SEA-via-kernel backend. The kernel↔Thrift abstraction boundary is clean, pure decision logic is well-isolated and unit-tested under CGO_ENABLED=0, cgo lifetimes/frees check out, and the hot path stays allocation-free. Remaining risk is kernel↔Thrift behavioral parity — one confirmed auth-scope divergence plus several parity/observability gaps that hermetic tests and the required merge gate don't cover. No security, correctness, or performance blockers; all 10 prior-review findings remain resolved.


Could not post inline comments for: F2, F6 — see body below.

Medium Findings

F2 · internal/backend/kernel/backend.go:L104

[MEDIUM] U2M session-open blocks in cgo up to ~120s, ignoring the connect-context deadline — can wedge the sql.DB pool

OpenSession checks ctx.Err() only at entry; once inside kernel_session_open → the kernel-owned browser/PKCE flow, the goroutine parks in cgo until login or the kernel's ~120s callback timeout, and context cancellation has no effect (documented at doc.go:225-228). Under sql.DB, a process misconfigured with authType=oauthU2M can pile up connection attempts each blocked ~120s in cgo, with no log line naming the interactive-auth stall.

  • Suggested fix: add a Go-side watchdog that logs at Warn when the connect deadline fires, or reject U2M when the connect ctx deadline is shorter than the kernel callback timeout so the failure names the cause instead of hanging.

Low Findings

Low findings (1) — click to expand

F6 · internal/backend/kernel/operation.go:L51

[LOW] Query text not checked for interior NUL — silently truncated on kernel while Thrift sends it whole

bindParams got an explicit interior-NUL guard (checkParamValue/errParamNUL, bindparams.go:67) precisely because the kernel C ABI is NUL-terminated and would truncate silently vs. Thrift's length-prefixed wire form. The identical reasoning applies to newCStr(req.Query) (SQL text), which has no such check — a NUL-containing statement is silently truncated on kernel, executing a different, shorter statement than Thrift.

  • Suggested fix: apply the same interior-NUL check to req.Query before set_sql, failing loudly rather than diverging from Thrift.

…query NUL, U2M scope docs

- connection.go: finalize EXECUTE_STATEMENT on the op.Results() error path
  (AfterExecute/CompleteStatement via the detached telemetryCtx, return nil
  rows) so a query that executed server-side but failed schema fetch/import no
  longer leaves a dangling, never-completed metric.
- Extract a pure untagged buildKernelConfig seam (move kernel.Config to an
  untagged config.go) and add TestBuildKernelConfig so the experimental
  KernelExperimental TLS forwarding is covered under CGO_ENABLED=0.
- Add TestResolveKernelAuthRealAuthenticators driving resolveKernelAuth with the
  real m2m/u2m constructors, so renaming M2MCredentials/M2MScopes/U2MClientID
  fails a test instead of silently disabling OAuth-over-kernel at runtime.
- Guard SQL statement text for an interior NUL (checkQueryText/errQueryNUL)
  before set_sql, mirroring the existing bound-param guard.
- doc.go / U2MClientID: note that kernel and Thrift share the U2M client id but
  request different default scopes (neither exposes a scopes option).

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
@github-actions github-actions Bot removed the integration-test-kernel Preview the full passthrough kernel integration suite (live warehouse) label Jul 17, 2026
@github-actions

Copy link
Copy Markdown

Integration test approval reset.

New commits were pushed to this PR. Label(s) integration-test-kernel 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: a7228f6

Bring the PR #399 branch up to date with main (picks up #408,
.gitleaksignore Arrow IPC base64 allowlist). No conflicts.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
@mani-mathur-arch mani-mathur-arch added the integration-test-kernel Preview the full passthrough kernel integration suite (live warehouse) label Jul 17, 2026
@github-actions

Copy link
Copy Markdown

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

@mani-mathur-arch
mani-mathur-arch merged commit cb8a8a0 into main Jul 17, 2026
16 checks passed
@mani-mathur-arch
mani-mathur-arch deleted the mani/sea-kernel-consolidated branch July 17, 2026 09:36
mani-mathur-arch added a commit that referenced this pull request Jul 18, 2026
Register a cgo-exported trampoline (kernelLogTrampoline) as the kernel's log sink
(kernel_set_log_callback) so kernel-internal tracing events flow into the driver's
logger — the unified Go+kernel stream, honouring logger.SetLogOutput — instead of
only reaching stderr via kernel_init_logging. This completes the one-knob logging
story: #399 unified the LEVEL across the Go binding lines and the kernel's Rust
lines (PECOBLR-3650) against the stderr sink; this replaces that sink with the
callback so the Rust lines land in the same writer as everything else.

The reverse-call machinery: a runtime/cgo.Handle round-trips the *logSink through
the C void* ctx (cgo pointer rules), and a defer recover() firewall converts any
panic into a dropped line (a panic across the cgo boundary would abort the
process) — the same shape a kernel->host token-provider callback would need. The
driver maps its own log level (resolveKernelLogArg, unchanged) and passes it as
the callback's level argument, so the kernel's Rust lines follow
DATABRICKS_LOG_LEVEL, not just RUST_LOG; DBSQL_KERNEL_DEBUG still defers to
RUST_LOG (NULL level).

The forwarding sink logs through a SNAPSHOT of the driver logger taken at install
and pinned to TraceLevel, for two reasons: (1) no double gating — the kernel
already filtered events against the level we passed it, so re-gating through the
live logger.Logger (at the driver level) would re-drop exactly the events the
DBSQL_KERNEL_DEBUG override was meant to surface; (2) no data race — the kernel
drain thread reads only its immutable snapshot rather than the mutable global
logger a concurrent SetLogLevel/SetLogOutput would rewrite. A non-Success install
is surfaced at Warn (visible at the default level), not the Debug-gated klog.

The two C-ABI logging routes share the one process-global subscriber and are
mutually exclusive; we install the callback, and a non-Success install (host
already installed a subscriber) is logged, never fatal to connect.

Level mapping and sink routing are pure Go (logforward.go, untagged) so they test
under CGO_ENABLED=0 — including the per-level mapping assertions and the
not-re-gated-by-driver-level property. The reverse-call round-trip, the recover
firewall (a panicking sink must not crash the process), the wrong-type/nil-ctx
guards, and delivery after a panic are exercised by tagged tests via a C invoke
seam.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch added a commit that referenced this pull request Jul 18, 2026
Register a cgo-exported trampoline (kernelLogTrampoline) as the kernel's log sink
(kernel_set_log_callback) so kernel-internal tracing events flow into the driver's
logger — the unified Go+kernel stream, honouring logger.SetLogOutput — instead of
only reaching stderr via kernel_init_logging. This completes the one-knob logging
story: #399 unified the LEVEL across the Go binding lines and the kernel's Rust
lines (PECOBLR-3650) against the stderr sink; this replaces that sink with the
callback so the Rust lines land in the same writer as everything else.

The reverse-call machinery: a runtime/cgo.Handle round-trips the *logSink through
the C void* ctx (cgo pointer rules), and a defer recover() firewall converts any
panic into a dropped line (a panic across the cgo boundary would abort the
process) — the same shape a kernel->host token-provider callback would need. The
driver maps its own log level (resolveKernelLogArg, unchanged) and passes it as
the callback's level argument, so the kernel's Rust lines follow
DATABRICKS_LOG_LEVEL, not just RUST_LOG; DBSQL_KERNEL_DEBUG still defers to
RUST_LOG (NULL level).

The forwarding sink logs through a SNAPSHOT of the driver logger taken at install
and pinned to TraceLevel, for two reasons: (1) no double gating — the kernel
already filtered events against the level we passed it, so re-gating through the
live logger.Logger (at the driver level) would re-drop exactly the events the
DBSQL_KERNEL_DEBUG override was meant to surface; (2) no data race — the kernel
drain thread reads only its immutable snapshot rather than the mutable global
logger a concurrent SetLogLevel/SetLogOutput would rewrite. A non-Success install
is surfaced at Warn (visible at the default level), not the Debug-gated klog.

The two C-ABI logging routes share the one process-global subscriber and are
mutually exclusive; we install the callback, and a non-Success install (host
already installed a subscriber) is logged, never fatal to connect.

Level mapping and sink routing are pure Go (logforward.go, untagged) so they test
under CGO_ENABLED=0 — including the per-level mapping assertions and the
not-re-gated-by-driver-level property. The reverse-call round-trip, the recover
firewall (a panicking sink must not crash the process), the wrong-type/nil-ctx
guards, and delivery after a panic are exercised by tagged tests via a C invoke
seam.

Co-authored-by: Isaac
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
mani-mathur-arch added a commit that referenced this pull request Jul 18, 2026
…d error drop

Emit connection-configuration telemetry on the kernel path (PECOBLR-3627)
and stop silently dropping connection-scoped errors (PECOBLR-3628).

Neither backend populated DriverConnectionParameters before: the struct
existed but was never filled, and recordConnection was dead code. This is
a net-new emission on top of the #399 per-statement telemetry (which
already emits EXECUTE_STATEMENT / CLOSE_STATEMENT + chunk details and
per-statement errors — not duplicated here).

- telemetry: telemetryMetric gains connParams; createTelemetryRequest
  serializes it (and mirrors AuthMech onto the top-level AuthType) only
  for a "connection" metric — a statement/operation/error metric leaves
  it nil and serializes byte-identically to before (pinned by a test).
  Interceptor.RecordConnectionConfig replaces the dead recordConnection.
- aggregator: a non-terminal error with no statement to attach to (a
  connection-scoped error, empty statementID) was silently dropped. It
  now flushes immediately instead — the connection-scoped error-log path
  (3628). Both backends benefit.
- driver: the connector emits RecordConnectionConfig at connect, KERNEL
  PATH ONLY (gated on the backend type, not just WithUseKernel), so the
  default (Thrift) path's telemetry is byte-identical. The payload is
  built by untagged kernelConnectionTelemetry(cfg): mode=SEA (the closed
  DatabricksClientType enum has no "kernel" member — emitting it NULLs the
  field on ingestion), auth_mech ∈ {PAT, OAUTH} + auth_flow ∈
  {CLIENT_CREDENTIALS, BROWSER_BASED_AUTHENTICATION} (the closed enums, not
  the driver's own auth names), proxy usage, arrow, query tags, metric-view
  — mirroring Python's TelemetryHelper enum mapping so all drivers land the
  same values.

Tests: untagged builder test; serializer + "statement metric unchanged" +
full-loop (RecordConnectionConfig lands; connection-scoped error not
dropped) tests. Both build paths + full suite green; go vet clean. Isaac
Review clean (0 actionable findings).

Co-authored-by: Isaac <isaac@databricks.com>
Signed-off-by: Mani Kaustubh Mathur <mani.mathur@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-test-kernel Preview the full passthrough kernel integration suite (live warehouse)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants