feat(kernel): consolidated DAIS gap-closure + PuPr feature set #399
feat(kernel): consolidated DAIS gap-closure + PuPr feature set #399mani-mathur-arch wants to merge 10 commits into
Conversation
543c7a2 to
ba466d3
Compare
ba466d3 to
f35b387
Compare
…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>
f35b387 to
d0c999e
Compare
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>
d0c999e to
2caa354
Compare
| func formatDayTimeInterval(v int64, unit arrow.TimeUnit) string { | ||
| neg := v < 0 | ||
| if neg { | ||
| v = -v |
There was a problem hiding this comment.
This still overflows for the minimum negative interval value. For math.MinInt64, v = -v wraps and leaves v negative, so the formatter then computes negative components while also prepending - (the same issue exists for math.MinInt32 in formatYearMonthInterval). These bounds are valid Arrow/Spark interval values, and the current tests cover MaxInt64 plus small negatives but not the negative extreme. Can we avoid negating the full signed magnitude, or add explicit MinInt handling and regression cases?
| } | ||
| return kernel.Auth{Mode: kernel.AuthPAT, Token: token}, nil | ||
| default: | ||
| return kernel.Auth{}, errors.New("databricks: this authenticator is not supported by the kernel backend; " + |
There was a problem hiding this comment.
This unsupported-auth path no longer wraps ErrNotSupportedByKernel, even though validateKernelConfig documents that unsupported kernel options use that sentinel so callers can fall back with errors.Is. That means token-provider / external / static / federated auth failures behave differently from the other unsupported kernel options. Can this return a %w-wrapped ErrNotSupportedByKernel (or update the contract/tests if unsupported auth intentionally should not be programmatically detectable)?
|
|
||
| // TestKernelParamsVsThrift asserts parameterized queries produce byte-identical | ||
| // output on the kernel and Thrift backends — the bound-parameter acceptance gate. | ||
| func TestKernelParamsVsThrift(t *testing.T) { |
There was a problem hiding this comment.
This is the main test that proves kernel parameter binding actually works, but it self-skips without DATABRICKS_HOST / DATABRICKS_HTTP_PATH / DATABRICKS_TOKEN, and the kernel CI job intentionally runs without warehouse creds. As a result, regressions in named vs positional binding, NULL vs empty-string handling, type strings, or bind-before-execute ordering can pass required CI. Can we add some executing coverage for the bind mapping (for example a hermetic seam around the Param -> kernel bind arguments, or a required credentialed kernel parity run)?
|
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:
|
What
Extends the SEA-via-kernel backend for the Go driver with a set of authentication, session-setup, type-rendering, query-execution, and telemetry features. 24 files, +1265 / −161.
Features
Authentication
cfg.Authenticator. Adds akernel.Authvalue descriptor +resolveKernelAuth, wired through theset_auth_pat/_m2m/_u2mC-ABI setters.Session setup
USE CATALOG/USE SCHEMA, with identifier quoting (quoteIdent).config.EffectiveSessionParams()folds the server conf in a backend-neutral way.Type rendering
internal/arrowscan.Query execution & telemetry
kernel_statement_bind_parameter).DBExecutionErrorcarrying both the sqlstate and the server query id (empty-queryId → ctx fallback), soQueryId()is populated on the kernel error path.StatementID()accessor (kernel_executed_statement_query_id) and threaded into EXECUTE_STATEMENT telemetry.Notable behavior
executevia the kernel raw-param C ABI.doc.goupdated to describe OAuth / namespace / metric-view / params as supported and staging as unsupported; the stale INTERVAL caveat is dropped.Testing
CGO_ENABLED=0):go build,go vet,go test ./...— 24 packages ok, 0 fail.CGO_ENABLED=1 -tags databricks_kernel, linked against a locally-built kernel.aatKERNEL_REV):go build,go vet,go test ./...— 24 packages ok, 0 fail.gofmtclean across all 24 changed files.kernel_e2e_test.go) self-skip withoutDATABRICKS_*creds.Merge gate
KERNEL_REVat merge time. It currently points at the kernel dependency's PR-head SHA, which is GC-able once that kernel change merges; bump it to the resulting kernelmainSHA (bare token — the Makefile does$(shell cat KERNEL_REV)), re-sync the cgo drift assertions incgo.goif any signatures changed, and runmake test-kernelagainst the new rev.This pull request and its description were written by Isaac.