From 0f33f31eaf2b0af8b8e5a73acc4cc9dfe7ae7d49 Mon Sep 17 00:00:00 2001 From: Vu Anh Phung Date: Thu, 3 Sep 2026 17:12:50 +0000 Subject: [PATCH 1/3] chore(kernel): bump pinned revision Signed-off-by: Vu Anh Phung --- KERNEL_REV | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/KERNEL_REV b/KERNEL_REV index f751c496..110c974a 100644 --- a/KERNEL_REV +++ b/KERNEL_REV @@ -1 +1 @@ -dd810d6d0a179886b923c6e22dc785ddca16ebef +167651ecc67143ef258ad70fd2682ced00beaa99 From 82f49a0644061ce00252e69b19f60894789cb703 Mon Sep 17 00:00:00 2001 From: Vu Anh Phung Date: Thu, 3 Sep 2026 17:34:05 +0000 Subject: [PATCH 2/3] build(kernel): link development tests to pinned source Signed-off-by: Vu Anh Phung --- .github/workflows/nightly-e2e.yml | 2 + Makefile | 18 ++--- README.md | 4 +- .../kernel/include/databricks_kernel.h | 79 ++++++++++++++++--- 4 files changed, 80 insertions(+), 23 deletions(-) diff --git a/.github/workflows/nightly-e2e.yml b/.github/workflows/nightly-e2e.yml index 80da8740..569d9e86 100644 --- a/.github/workflows/nightly-e2e.yml +++ b/.github/workflows/nightly-e2e.yml @@ -236,6 +236,7 @@ jobs: # secrets + drop the -skip to cover it. - name: Run kernel E2E env: + CGO_LDFLAGS: -L${{ github.workspace }}/internal/backend/kernel/lib/linux_amd64 -l:libdatabricks_sql_kernel.a DATABRICKS_PECOTESTING_SERVER_HOSTNAME: ${{ secrets.DATABRICKS_PECOTESTING_SERVER_HOSTNAME }} DATABRICKS_PECOTESTING_HTTP_PATH2: ${{ secrets.DATABRICKS_PECOTESTING_HTTP_PATH2 }} DATABRICKS_PECOTESTING_TOKEN: ${{ secrets.DATABRICKS_PECOTESTING_TOKEN }} @@ -255,6 +256,7 @@ jobs: - name: Run kernel E2E against Reyden (non-blocking) continue-on-error: true env: + CGO_LDFLAGS: -L${{ github.workspace }}/internal/backend/kernel/lib/linux_amd64 -l:libdatabricks_sql_kernel.a DATABRICKS_PECOTESTING_SERVER_HOSTNAME: ${{ secrets.DATABRICKS_PECOTESTING_SERVER_HOSTNAME }} DATABRICKS_PECOTESTING_HTTP_PATH2: ${{ secrets.DATABRICKS_PECOTESTING_HTTP_PATH2 }} DATABRICKS_PECOTESTING_TOKEN: ${{ secrets.DATABRICKS_PECOTESTING_TOKEN }} diff --git a/Makefile b/Makefile index f6df151d..9b7327b9 100644 --- a/Makefile +++ b/Makefile @@ -101,6 +101,7 @@ KERNEL_LIB_DIR = internal/backend/kernel/lib/$(KERNEL_GOOS)_$(KERNEL_GOARCH) KERNEL_INC_DIR = internal/backend/kernel/include KERNEL_GO = CGO_ENABLED=1 go KERNEL_TAGS = -tags databricks_kernel +KERNEL_LOCAL_LDFLAGS = -L$(abspath $(KERNEL_LIB_DIR)) -l:libdatabricks_sql_kernel.a # SHIPPED PATH (consumers): the kernel archives are NOT built here. They come from # the external per-platform modules in github.com/databricks/databricks-sql-kernel-bindings @@ -108,13 +109,8 @@ KERNEL_TAGS = -tags databricks_kernel # the target platform's archive (no Rust, no build step). The targets below are # for LOCAL DEVELOPMENT against a kernel source checkout only. # -# Local-dev flow: `make kernel-lib` builds the host-platform archive from the -# pinned KERNEL_REV into $(KERNEL_LIB_DIR) (a .gitignore'd scratch dir). To have a -# `-tags databricks_kernel` build actually LINK that freshly built archive instead -# of the published bindings module, point the matching lib/ module at a -# local bindings checkout via a go.work whose lib// holds the built .a -#. TODO(dev-loop): wire this go.work step into the targets -# so `make test-kernel` links the local build end-to-end without manual setup. +# Local-dev flow: `make test-kernel` builds the host-platform archive from the +# pinned KERNEL_REV and puts it before the published archive on the linker line. .PHONY: kernel-lib kernel-lib: ## Build the pinned kernel static lib + header locally (source build, dev only). KERNEL_REPO="$(KERNEL_REPO)" KERNEL_REV="$(KERNEL_REV)" KERNEL_SRC="$(KERNEL_SRC)" \ @@ -125,9 +121,9 @@ kernel-lib: ## Build the pinned kernel static lib + header locally (source buil ./build/kernel-lib.sh .PHONY: build-kernel -build-kernel: ## Build the driver with the kernel backend linked (against the bindings modules). - $(KERNEL_GO) build $(KERNEL_TAGS) ./... +build-kernel: kernel-lib ## Build the driver against the source-built KERNEL_REV archive. + CGO_LDFLAGS='$(KERNEL_LOCAL_LDFLAGS) $(CGO_LDFLAGS)' $(KERNEL_GO) build $(KERNEL_TAGS) ./... .PHONY: test-kernel -test-kernel: ## Run the kernel-tagged unit tests (no warehouse needed; links the bindings modules). - $(KERNEL_GO) test $(KERNEL_TAGS) ./... +test-kernel: kernel-lib ## Run kernel-tagged tests against the source-built KERNEL_REV archive. + CGO_LDFLAGS='$(KERNEL_LOCAL_LDFLAGS) $(CGO_LDFLAGS)' $(KERNEL_GO) test $(KERNEL_TAGS) ./... diff --git a/README.md b/README.md index 871e4945..f4986001 100644 --- a/README.md +++ b/README.md @@ -141,7 +141,7 @@ clone `databricks-sql-kernel` at [`KERNEL_REV`](./KERNEL_REV): ```bash make kernel-lib # clone + cargo-build the pinned archive into the cgo link dir make build-kernel # == CGO_ENABLED=1 go build -tags databricks_kernel ./... -make test-kernel # kernel-tagged unit tests +make test-kernel # build KERNEL_REV and run kernel-tagged unit tests against it ``` Stage a prebuilt archive without Rust via @@ -427,7 +427,7 @@ We use `golangci-lint`. In VS Code: ```bash go test # default (Thrift) backend, pure Go -make test-kernel # kernel-tagged unit tests (links the prebuilt bindings; no Rust) +make test-kernel # kernel-tagged unit tests against the source-built KERNEL_REV ``` ## Issues diff --git a/internal/backend/kernel/include/databricks_kernel.h b/internal/backend/kernel/include/databricks_kernel.h index 001f4c17..755844d6 100644 --- a/internal/backend/kernel/include/databricks_kernel.h +++ b/internal/backend/kernel/include/databricks_kernel.h @@ -56,12 +56,11 @@ * - **A result stream borrows its executed handle.** Closing the executed * handle invalidates any stream obtained from it: subsequent stream * calls return `InvalidStatementHandle` (a defined error, not UB). - * - **Close is best-effort async.** `kernel_session_close` initiates the - * server-side delete before returning — a detached task runs it on the - * kernel's process-wide runtime — but does not wait for completion. A - * process that exits immediately after `kernel_session_close` may drop - * the detached task before it runs, leaving the server session to expire - * on its own idle timeout. + * - **Close is awaited.** `kernel_session_close` waits for the server-side + * delete to complete and flushes close-session telemetry before returning. + * If close fails, the kernel still schedules best-effort cleanup when the + * consumed session handle is dropped, but the status code reports the + * awaited close failure to the caller. * - **Close the session LAST.** Because close initiates the server-side * delete, any handle still open against that session — a statement / * executed handle, or a metadata result stream (which holds its own @@ -213,6 +212,16 @@ typedef struct kernel_result_stream_t kernel_result_stream_t; * cancels the LATER query. Use one per execute (new before execute, free after * it returns). */ typedef struct kernel_statement_canceller_t kernel_statement_canceller_t; +/* Detached canceller for a result stream, returned by + * kernel_result_stream_canceller_new. Firing it interrupts an in-flight + * kernel_result_stream_next_batch so it returns KernelStatusCode_Cancelled + * promptly instead of blocking until the (possibly slow / CloudFetch-link) + * batch fetch finishes. Unlike the statement canceller it holds ONLY a shared + * client-side cancellation token (no server RPC, no borrow of the stream's + * mutable state), so — see its doc — it is safe to obtain and to fire + * concurrently with a next_batch/close on the owning thread. Once fired, the + * stream is permanently aborted (every later next_batch returns Cancelled). */ +typedef struct kernel_result_stream_canceller_t kernel_result_stream_canceller_t; /* ─── Session config ────────────────────────────────────────────────── * @@ -317,7 +326,7 @@ kernel_session_config_set_oauth_scopes(KernelSessionConfig* config, const char* scopes); /* Add (or overwrite) one session-conf entry. Keys are normally server SET - * parameters forwarded on the SEA wire (allowlist-filtered). A small set of + * parameters forwarded unchanged on the SEA wire. A small set of * reserved keys are instead CLIENT-ONLY: they tune the kernel locally, are * consumed before session creation, and NEVER reach the wire. Client-only keys * (matched case-insensitively): @@ -469,6 +478,12 @@ KernelStatusCode kernel_session_config_set_telemetry_config(KernelSessionConfig* uint64_t retry_delay_ms, uint64_t close_flush_timeout_ms); +/* Configure telemetry circuit-breaker behavior. `threshold` and `timeout_ms` + * must be > 0 when circuit breaking is enabled. If this setter is not called, + * the kernel keeps its default circuit-breaker policy. */ +KernelStatusCode kernel_session_config_set_telemetry_circuit_breaker_config( + KernelSessionConfig* config, bool enabled, uint32_t threshold, uint64_t timeout_ms); + /* Configure driver/runtime/system identity supplied by the binding layer. * Every string argument is optional: pass NULL for values the binding does not * know. The kernel fills any missing fields it can derive before emitting @@ -577,9 +592,20 @@ KernelStatusCode kernel_session_is_open(const kernel_session_t* session, bool* o * alive); on failure returns the mapped status (a transport/availability * failure surfaces as Unavailable / NetworkError / Timeout — treat as * "connection dead" when backing SQL_ATTR_CONNECTION_DEAD; the full error - * is in kernel_get_last_error). Runs SQL, so it MUST be called from a - * native (non-async-runtime) thread. No ownership. */ -KernelStatusCode kernel_session_test(const kernel_session_t* session); + * is in kernel_get_last_error). + * + * `timeout_secs` bounds the WHOLE probe (every retry and HTTP round-trip + * included) to that many seconds; 0 means no per-call deadline (the probe + * runs on the session's own retry/timeout budget — the historical + * behaviour). This is the per-call deadline a host backs + * `ConnectionTestTimeout` with: with 0 the configured value has no effect. + * When the probe exceeds the deadline the call returns Timeout (typed error + * in kernel_get_last_error). The bound is scoped to this probe and cannot + * alter the timeout of later requests on the session. + * + * Runs SQL, so it MUST be called from a native (non-async-runtime) thread. + * No ownership. */ +KernelStatusCode kernel_session_test(const kernel_session_t* session, uint64_t timeout_secs); /* Construct a new mutable statement bound to this session. */ KernelStatusCode kernel_session_new_statement(kernel_session_t* session, @@ -809,6 +835,39 @@ KernelStatusCode kernel_result_stream_next_batch(kernel_result_stream_t* stream, struct ArrowSchema* out_schema); KernelStatusCode kernel_result_stream_close(kernel_result_stream_t* stream); +/* ─── Result-stream cancellation (detached canceller) ───────────────── + * + * Interrupt an in-flight `kernel_result_stream_next_batch` (odbc#355-B). A + * host that fetches on a background thread and wants a bounded teardown + * (SQLCancel / SQLCloseCursor / statement or connection close) fires this to + * make a parked `next_batch` return promptly instead of waiting for the + * batch's own (possibly slow, CloudFetch-link) request to finish. + * + * `kernel_result_stream_canceller_new` clones the stream's cancellation token + * into a detached handle. Unlike the statement canceller, this borrows none of + * the stream's mutable state and issues no server RPC, so it may be obtained + * WHILE a `next_batch` is already in flight on another thread, and + * `kernel_result_stream_canceller_cancel` may be called concurrently with a + * `next_batch`/`kernel_result_stream_close` on the owning thread. `_cancel` is + * non-blocking and idempotent; it returns Success once the signal is delivered + * (it is the interrupted `next_batch` that returns + * `KernelStatusCode_Cancelled`). After a cancel the stream is permanently + * aborted: every later `next_batch` returns `Cancelled`, so the host must not + * expect to resume it — close it. + * + * The canceller shares the token's lifetime independently of the stream, so + * its free order is unconstrained: firing it after `kernel_result_stream_close` + * is a harmless no-op. Free it with `kernel_result_stream_canceller_free` + * (drives no RPC — drops the box). Do not free it while another thread is + * inside `_cancel` on the same handle. + */ +KernelStatusCode kernel_result_stream_canceller_new(kernel_result_stream_t* stream, + kernel_result_stream_canceller_t** out); +KernelStatusCode kernel_result_stream_canceller_cancel( + kernel_result_stream_canceller_t* canceller); +KernelStatusCode kernel_result_stream_canceller_free( + kernel_result_stream_canceller_t* canceller); + /* ─── Metadata ──────────────────────────────────────────────────────── * * Each returns a self-contained result stream (released via From 37f8359f9cb774fe4268560f80cdaf00a3b46980 Mon Sep 17 00:00:00 2001 From: Vu Anh Phung Date: Thu, 3 Sep 2026 17:52:44 +0000 Subject: [PATCH 3/3] chore(kernel): align with pinned ABI contracts Signed-off-by: Vu Anh Phung --- CONNECTION_PARAMETERS.md | 64 ++++--------------- internal/backend/kernel/backend.go | 4 +- .../kernel/include/databricks_kernel.h | 6 ++ internal/config/config.go | 2 +- 4 files changed, 23 insertions(+), 53 deletions(-) diff --git a/CONNECTION_PARAMETERS.md b/CONNECTION_PARAMETERS.md index 571d05ed..614bcc75 100644 --- a/CONNECTION_PARAMETERS.md +++ b/CONNECTION_PARAMETERS.md @@ -15,7 +15,7 @@ sections of the README, laid out as one comparison matrix per concern. |:---:|---| | ✅ | Supported and honored. | | ❌ | Not supported — **rejected** at connect/execute (wraps `dbsqlerr.ErrNotSupportedByKernel` or `dbsqlerr.ErrRequiresKernelBackend`), never silently ignored. | -| ⚠️ | Accepted but not fully honored — either inert ("silently ignored") or only partially/conditionally honored (e.g. some session confs are honored while others are dropped/rejected on the kernel path). | +| ⚠️ | Accepted but not fully honored — either inert ("silently ignored") or only partially/conditionally honored. | | — | Not applicable. | **Backend selection.** Both backends are selected once per connection via @@ -25,11 +25,8 @@ sections of the README, laid out as one comparison matrix per concern. See [Building](./README.md#building). Any parameter not listed below (e.g. `ansi_mode`) is passed through as a -**session parameter**. On the **Thrift** path the session-conf map is forwarded freely. -On the **kernel** path conf keys are matched (case-insensitively) against an allowlist — -non-allowlisted keys are dropped with a warning, and a few are hard-rejected — so a conf -that takes effect on Thrift may silently be ignored on kernel. Broadening the kernel -allowlist is tracked in PECOBLR-4153. +**session parameter**. Both backends forward server-bound session confs; the kernel +preserves their names and values unchanged. ## Endpoint & routing @@ -90,60 +87,27 @@ Notes for the SEA/kernel backend: | `maxRows` | `WithMaxRows` | ✅ | ⚠️ | `100000` | Max rows per fetch. On the kernel path the kernel manages paging, so this is accepted but has no effect. | | `timeout` | `WithTimeout` | ✅ | ❌ | no timeout | Server-side query timeout, in seconds. On the kernel path use the `STATEMENT_TIMEOUT` session parameter instead. | | `userAgentEntry` | `WithUserAgentEntry` | ✅ | ✅ | | Identifies your application (partners/ISVs), format ``. | -| *(session param)* | `WithSessionParams` | ✅ | ⚠️ | | Arbitrary session confs (e.g. `ansi_mode`, `STATEMENT_TIMEOUT`, `QUERY_TAGS`). Allowlisted confs are honored on both; on kernel a non-allowlisted conf is dropped/rejected (see the note above; PECOBLR-4153). | +| *(session param)* | `WithSessionParams` | ✅ | ✅ | | Arbitrary server session confs (e.g. `ansi_mode`, `STATEMENT_TIMEOUT`, `QUERY_TAGS`) are forwarded unchanged. | | *(via session param)* | `WithQueryTags` | ✅ | ✅ | | Session-level query tags (serialized into `QUERY_TAGS`). | | `timezone` | `WithSessionParams(timezone=…)` | ✅ | ✅ | | Session time zone (e.g. `America/Los_Angeles`). | -| `enableMetricViewMetadata` | `WithEnableMetricViewMetadata` | ✅ | ⚠️ | `false` | Enables metric-view metadata (sets `spark.sql.thriftserver.metadata.metricview.enabled=true`). Both paths forward the **identical** conf; the kernel allowlists this key and sends it verbatim — it is **not** rejected driver- or kernel-side. Whether it takes effect on the SEA/kernel path depends on server-side SEA support (a `⚠️` pending confirmation against a live warehouse; PECOBLR-4142 / PECOBLR-4153). | +| `enableMetricViewMetadata` | `WithEnableMetricViewMetadata` | ✅ | ⚠️ | `false` | Enables metric-view metadata (sets `spark.sql.thriftserver.metadata.metricview.enabled=true`). Both paths forward the identical conf. Whether it takes effect on the SEA/kernel path depends on server-side SEA support (PECOBLR-4142). | -### Kernel session-conf allowlist +### Kernel session confs -On the **Thrift** path the `WithSessionParams` map is forwarded to the server freely. -On the **kernel** path each key is matched **case-insensitively** against the allowlist -below; a key not on it is **dropped with a warning** (never sent), so a conf that takes -effect on Thrift may silently do nothing on kernel. Broadening the allowlist is tracked in -PECOBLR-4153. - -> **Authoritative source.** This table is transcribed from the vendored kernel's -> allowlist (`build/kernel-src/src/config.rs`), which is not part of this repo checkout. -> Only `spark.sql.thriftserver.metadata.metricview.enabled` and the kernel max-chunks key -> have repo-side anchors (`internal/config/config.go`); the remaining keys and the -> uppercase-on-send / `spark.*`-verbatim rules have no CI guard here and may lag as the -> kernel evolves. When in doubt, treat the kernel allowlist as authoritative. - -**SET-style SQL parameters** — matched case-insensitively, sent **uppercased** (the server -echoes these uppercase, so `SET`-readback matches): - -| Key | Purpose | -|---|---| -| `ANSI_MODE` | Enable/disable ANSI SQL behavior. | -| `COLLATION` | Default collation. | -| `ENABLE_PHOTON` | Toggle the Photon engine. | -| `LEGACY_TIME_PARSER_POLICY` | Legacy datetime parsing behavior. | -| `MAX_FILE_PARTITION_BYTES` | Max bytes per file partition. | -| `QUERY_TAGS` | Query tags (comma-separated `key:value`). This is the key `WithQueryTags` writes. | -| `READ_ONLY_EXTERNAL_METASTORE` | Treat the external metastore as read-only. | -| `STATEMENT_TIMEOUT` | Server-side per-statement timeout (seconds). The real query-timeout knob on the kernel path, since `WithTimeout` is rejected there. | -| `TIMEZONE` | Session time zone (e.g. `UTC`). Also settable via the `timezone` DSN param / `WithSessionParams`. | -| `USE_CACHED_RESULT` | Toggle result caching. | - -**Dotted `spark.*` conf** — matched case-insensitively but sent **verbatim** (Spark conf -keys are case-sensitive and must not be uppercased): - -| Key | Purpose | -|---|---| -| `spark.sql.thriftserver.metadata.metricview.enabled` | Metric-view metadata; the conf `WithEnableMetricViewMetadata` sets. | +The kernel forwards server-bound `WithSessionParams` entries unchanged and lets the +server validate them. An unsupported key may therefore fail session creation or a later +statement instead of being dropped client-side. Notes: - Boolean-valued keys should use the exact strings `"true"` / `"false"` — the kernel does not pre-validate values and forwards them as-is. -- **`CAN_CLOUD_DOWNLOAD` is deliberately not allowlisted**: SEA has no such session conf - (it is accepted at CreateSession but rejected at the first statement with - `CONFIG_NOT_AVAILABLE`). Disable Cloud Fetch with `WithCloudFetch(false)` and bound its - memory with `WithKernelMaxChunksInMemory` instead of a raw conf. +- **`CAN_CLOUD_DOWNLOAD` is not a valid SEA session conf**: it may be accepted at + CreateSession and rejected by the first statement with `CONFIG_NOT_AVAILABLE`. + Disable Cloud Fetch with `WithCloudFetch(false)` instead of setting this raw conf. - The client-only keys (`cloudfetch_enabled`, `cloudfetch_max_chunks_in_memory`, - `complex_types_as_json`, `intervals_as_string`, …) are **not** in this allowlist: the - kernel reads them at session creation and strips them before the SEA wire. The driver + `complex_types_as_json`, `intervals_as_string`, …) are read by the kernel at session + creation and stripped before the SEA wire. The driver exposes the relevant ones as dedicated `WithKernel*` options rather than raw confs. ## Retry / backoff diff --git a/internal/backend/kernel/backend.go b/internal/backend/kernel/backend.go index 2a8ed818..f55939d1 100644 --- a/internal/backend/kernel/backend.go +++ b/internal/backend/kernel/backend.go @@ -750,8 +750,8 @@ func (k *KernelBackend) runNamespaceStmt(ctx context.Context, sql string) error return closeErr } -// CloseSession tears down the server-side session. Best-effort: the kernel's -// close is async (see the C header), so an error is logged, not hard-failed. +// CloseSession tears down the server-side session and returns the kernel's +// awaited close result. // // Deferred (tracked): this ignores ctx and blocks in the synchronous call() until // kernel_session_close returns, with no deadline — a stalled kernel-side close diff --git a/internal/backend/kernel/include/databricks_kernel.h b/internal/backend/kernel/include/databricks_kernel.h index 755844d6..6ea9bccb 100644 --- a/internal/backend/kernel/include/databricks_kernel.h +++ b/internal/backend/kernel/include/databricks_kernel.h @@ -352,6 +352,12 @@ KernelStatusCode kernel_session_config_set_custom_header(KernelSessionConfig* co KernelStatusCode kernel_session_config_set_request_timeout(KernelSessionConfig* config, uint64_t request_timeout_ms); +/* Configure the maximum number of idle HTTP connections retained per host. + * `max_connections` must be greater than zero. Omitting this setter keeps the + * kernel default (100). */ +KernelStatusCode kernel_session_config_set_max_connections(KernelSessionConfig* config, + size_t max_connections); + /* Initialize kernel logging, process-wide and ONCE (first non-OFF call wins; * later calls are no-ops). `level` is OFF/ERROR/WARN/INFO/DEBUG/TRACE * (NULL → RUST_LOG, default warn); `file_path` NULL → stderr. Not tied to diff --git a/internal/config/config.go b/internal/config/config.go index 37a337af..2d86f808 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -173,7 +173,7 @@ const MetricViewMetadataConfKey = "spark.sql.thriftserver.metadata.metricview.en // (WithKernelMaxChunksInMemory). Unlike MetricViewMetadataConfKey this is NOT a // server SET parameter and is NOT added by EffectiveSessionParams: the kernel // backend injects it into its own SessionConf only, and the kernel strips it -// before the SEA wire (it is absent from the kernel's server allowlist). It must +// before the SEA wire. It must // match the key the kernel's apply_client_result_overrides looks for. const KernelMaxChunksInMemoryConfKey = "cloudfetch_max_chunks_in_memory"