Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/nightly-e2e.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand All @@ -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 }}
Expand Down
64 changes: 14 additions & 50 deletions CONNECTION_PARAMETERS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Low — This PR rewrites the user-facing session-conf docs to claim the kernel now forwards WithSessionParams unchanged with no allowlist (here, plus the removed "Kernel session-conf allowlist" section and the WithSessionParams row flipped ⚠️ → ✅). But the PR description states the shipped bindings dependency is unchanged and the no-allowlist behavior only exists in the source-built pinned KERNEL_REV (167651e), pending the 1.1.0 bindings.

CONNECTION_PARAMETERS.md / README are consumed by users of the released driver, who link the older published bindings that (per the text being deleted) still allowlist-filter and drop non-allowlisted keys with a warning. Until 1.1.0 ships, a consumer reading these docs will expect an arbitrary conf (e.g. a non-allowlisted key) to be forwarded, when their linked kernel still drops it. Consider gating this doc change on the bindings bump, or noting that the unfiltered-forwarding behavior applies from kernel 1.1.0 onward.

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.

kernel change is already in 1.0.0 binding https://github.com/databricks/databricks-sql-kernel/pull/303. This is a doc update

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks — you may well be right that the change rode along in the published v1.0.0 binding. My concern was specifically that the docs describe unfiltered forwarding for users linking the released bindings, and I can't confirm from this repo alone that databricks-sql-kernel-bindings v1.0.0 (the pin in go.mod) actually contains the no-allowlist behavior from kernel PR #303 — that module's contents live outside this checkout. If a maintainer can confirm the shipped v1.0.0 archive is built from a kernel rev that includes #303 (rather than the behavior only being present in the source-built KERNEL_REV), this is fully resolved and the docs are accurate as written. Leaving the thread open only so that binding-version confirmation isn't lost.

preserves their names and values unchanged.

## Endpoint & routing

Expand Down Expand Up @@ -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 `<isv-name+product-name>`. |
| *(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
Expand Down
2 changes: 1 addition & 1 deletion KERNEL_REV
Original file line number Diff line number Diff line change
@@ -1 +1 @@
dd810d6d0a179886b923c6e22dc785ddca16ebef
167651ecc67143ef258ad70fd2682ced00beaa99
18 changes: 7 additions & 11 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -101,20 +101,16 @@ 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
# that this repo requires in go.mod; a kernel opt-in `go get` build downloads only
# 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/<platform> module at a
# local bindings checkout via a go.work whose lib/<platform>/ 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)" \
Expand All @@ -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) ./...
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions internal/backend/kernel/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading