Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
78e18c7
feat(volume): enable native file transfers through ExecContext
aakash-saravanan-db Nov 11, 2026
04a0591
fix(volume): align missing-context errors and document native transfers
aakash-saravanan-db Nov 11, 2026
39733aa
test(volume): cover execution mode and staging context
aakash-saravanan-db Nov 11, 2026
cec0d85
docs(volume): keep usage in connection parameter documentation
aakash-saravanan-db Nov 11, 2026
484a2f6
docs(volume): remove obsolete unsupported-transfer example
aakash-saravanan-db Nov 11, 2026
2cbe208
test(volume): retain the live transfer lifecycle and error cases
aakash-saravanan-db Nov 11, 2026
28f724d
fix(kernel): align native pin and explain transfer opt-in
aakash-saravanan-db Nov 11, 2026
6cda8ed
test(volume): assert local transfer errors and clarify backend contract
aakash-saravanan-db Nov 11, 2026
6b45c4c
test(kernel): verify raw file transfer instruction contents
aakash-saravanan-db Nov 11, 2026
4c00ab8
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
e540d7a
test(kernel): name the staging context guard precisely
aakash-saravanan-db Nov 12, 2026
8479abd
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
b93ca56
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
2c4a296
test(kernel): rely on live raw-result contract coverage
aakash-saravanan-db Nov 12, 2026
05c00b7
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
e5f6e8f
test: exercise transfer routing with a valid operation
aakash-saravanan-db Nov 12, 2026
0beb472
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
b47ceda
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
092917d
docs: explain native transfer fallback behavior
aakash-saravanan-db Nov 12, 2026
c965c7f
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
a4ef5cf
docs: include native volume transfers in package support
aakash-saravanan-db Nov 12, 2026
c65fde1
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
a0c5358
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
55fb979
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
aa8695c
ci: verify transfers link against published native modules
aakash-saravanan-db Nov 12, 2026
e02b741
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
235ad3e
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
684d44c
test(staging): bound volume fixture cleanup
aakash-saravanan-db Nov 12, 2026
76d50d4
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
1c97719
test(volume): assert structured local transfer error categories
aakash-saravanan-db Nov 12, 2026
16e43e9
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
43ecee4
test(kernel): cover transfer cancellation and raw execution
aakash-saravanan-db Nov 12, 2026
3f3d591
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
f95ffe0
test(kernel): tolerate repeated storage request notifications
aakash-saravanan-db Nov 12, 2026
fdae998
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
94b7221
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
9980687
test(kernel): synchronize transfer fixtures and check raw contexts
aakash-saravanan-db Nov 12, 2026
5345fb0
docs: clarify native QueryContext transfer behavior
aakash-saravanan-db Nov 12, 2026
5bcd046
docs: explain completion for every ExecContext
aakash-saravanan-db Nov 12, 2026
55686d0
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
3856db2
Recognize commented volume commands before checking staging context
aakash-saravanan-db Nov 12, 2026
60ca0f2
build(kernel): pin reviewed file transfer source
aakash-saravanan-db Nov 12, 2026
c041335
Keep volume transfer documentation focused on usage
aakash-saravanan-db Nov 12, 2026
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
25 changes: 25 additions & 0 deletions .github/workflows/go.yml
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,31 @@ jobs:
- name: Build
run: make linux

build-published-kernel:
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
name: Build (published kernel)
timeout-minutes: 15
runs-on:
group: databricks-protected-runner-group
labels: linux-ubuntu-latest
steps:
- name: Check out code into the Go module directory
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0

- name: Setup JFrog
uses: ./.github/actions/setup-jfrog

- name: Set up Go Toolchain
uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0
with:
go-version: '1.27.3'
cache: false

- name: Link against published native modules
run: go test -mod=readonly -tags databricks_kernel -run '^$' ./...
env:
CGO_ENABLED: 1
GOWORK: off

build-and-test-kernel:
name: Test (kernel backend)
# Exercises the opt-in SEA-via-kernel backend: builds the Rust kernel static
Expand Down
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Release History

## Unreleased
- Support SQL PUT, GET, and REMOVE through kernel-backed `ExecContext` with a staging context. Kernel-backed `QueryContext` now returns raw transfer instructions, matching Thrift, instead of `ErrNotSupportedByKernel`; it does not transfer files. Callers should review fallback handling that depends on that error and use `ExecContext` to complete transfers.

## v1.19.0 (2026-11-04)
- Upgrade the SQL kernel bindings to v1.4.0.
- Upgrade go-oidc to v3.21.0 (go-jose v4.1.6) and x/oauth2 to v0.36.0 to address go-jose/v3 security findings (databricks/databricks-sql-go#526).
Expand Down
9 changes: 9 additions & 0 deletions CONNECTION_PARAMETERS.md
Original file line number Diff line number Diff line change
Expand Up @@ -237,3 +237,12 @@ unset), kernel telemetry follows its server feature flag, and `telemetry_batch_s
The Go wrapper telemetry interceptor is skipped on the kernel path so it does not
duplicate kernel-owned telemetry for the same connection and statements. See
[`telemetry/DESIGN.md`](./telemetry/DESIGN.md).

## Volume file transfers

Both backends support SQL PUT, GET, and REMOVE through `ExecContext` with
`driverctx.NewContextWithStagingInfo(ctx, allowedLocalPaths)`. The context must
contain at least one allowed path, including for REMOVE. `QueryContext` returns
the transfer instructions for callers that handle file transfers themselves.

The credential-gated `TestKernelE2EVolumeFileLifecycle` uses `DATABRICKS_VOLUME_TEST_CATALOG` as a writable catalog and creates and removes an isolated schema and Volume. It covers ExecContext transfers and QueryContext raw instructions.
2 changes: 1 addition & 1 deletion KERNEL_REV
Original file line number Diff line number Diff line change
@@ -1 +1 @@
6f4b6d607a61bdde61dcda2cf82bc8e80fd32cb9
37c7a3ec1b54f4690d9325d65f2220e0bb64b83e
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
8 changes: 4 additions & 4 deletions connection.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func (c *conn) ExecContext(ctx context.Context, query string, args []driver.Name

// Capture execution start time for telemetry before running the query
executeStart := time.Now()
op, err := c.runQuery(ctx, query, args)
op, err := c.runQuery(ctx, query, args, true)
// A nil op means execution never reached the backend (parameter conversion
// failed): no server statement to close or measure. Return the already-wrapped
// error directly.
Expand Down Expand Up @@ -278,7 +278,7 @@ func (c *conn) QueryContext(ctx context.Context, query string, args []driver.Nam

// Capture execution start time for telemetry before running the query
executeStart := time.Now()
op, err := c.runQuery(ctx, query, args)
op, err := c.runQuery(ctx, query, args, false)
// A nil op means execution never reached the backend (parameter conversion
// failed): no statement id, no telemetry to emit. Log and return the
// already-wrapped error directly.
Expand Down Expand Up @@ -437,12 +437,12 @@ func (c *conn) QueryContext(ctx context.Context, query string, args []driver.Nam
// which case the wrapped error is returned directly. Once the backend is called,
// backend.Execute guarantees a non-nil Operation, so callers guard only for the
// pre-backend nil.
func (c *conn) runQuery(ctx context.Context, query string, args []driver.NamedValue) (backend.Operation, error) {
func (c *conn) runQuery(ctx context.Context, query string, args []driver.NamedValue, transferFiles bool) (backend.Operation, error) {
params, err := convertNamedValuesToParams(args)
if err != nil {
return nil, dbsqlerrint.NewExecutionError(ctx, dbsqlerr.ErrQueryExecution, err, nil)
}
return c.backend.Execute(ctx, backend.ExecRequest{Query: query, Params: params})
return c.backend.Execute(ctx, backend.ExecRequest{Query: query, Params: params, TransferFiles: transferFiles})
}

// enrichQueryId sets the operation's statement id as the context queryId. A
Expand Down
26 changes: 26 additions & 0 deletions connection_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/pkg/errors"

dbsqlerr "github.com/databricks/databricks-sql-go/errors"
"github.com/databricks/databricks-sql-go/internal/backend"
thriftbackend "github.com/databricks/databricks-sql-go/internal/backend/thrift"
"github.com/databricks/databricks-sql-go/internal/cli_service"
"github.com/databricks/databricks-sql-go/internal/client"
Expand Down Expand Up @@ -974,3 +975,28 @@ func TestConn_handleStagingRetry(t *testing.T) {
assert.Less(t, elapsed, 2*time.Second, "context cancel should abort PUT retry backoff promptly")
})
}

type transferCaptureBackend struct {
backend.Backend
transferFiles []bool
}

func (b *transferCaptureBackend) Execute(ctx context.Context, req backend.ExecRequest) (backend.Operation, error) {
b.transferFiles = append(b.transferFiles, req.TransferFiles)
return b.Backend.Execute(ctx, req)
}

func TestConnectionTransferMode(t *testing.T) {
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
testClient := &client.TestClient{
FnExecuteStatement: func(context.Context, *cli_service.TExecuteStatementReq) (*cli_service.TExecuteStatementResp, error) {
return &cli_service.TExecuteStatementResp{}, fmt.Errorf("capture request")
},
}
capture := &transferCaptureBackend{Backend: thriftbackend.NewForTest(testClient, getTestSession(), config.WithDefaults())}
connection := &conn{cfg: config.WithDefaults(), backend: capture}
_, execErr := connection.ExecContext(context.Background(), "REMOVE '/Volumes/c/s/v/file'", nil)
_, queryErr := connection.QueryContext(context.Background(), "GET '/Volumes/c/s/v/file' TO '/tmp/file'", nil)
assert.Error(t, execErr)
assert.Error(t, queryErr)
assert.Equal(t, []bool{true, false}, capture.transferFiles)
}
9 changes: 5 additions & 4 deletions doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -220,16 +220,17 @@ RUST_LOG. Filter on the target databricks::sql::kernel (note the colons):
Supported on the kernel backend: PAT and OAuth (M2M via WithClientCredentials, U2M
via the authType=oauthU2M DSN param); reading scalar, nested, and complex-typed
results (CloudFetch is transparent); bound query parameters (positional and named);
context cancellation during execute; the initial namespace (WithInitialNamespace,
context cancellation during execute; SQL PUT/GET/REMOVE on Unity Catalog Volumes
through ExecContext with a staging context (QueryContext returns raw instructions);
the initial namespace (WithInitialNamespace,
applied post-connect via USE CATALOG / USE SCHEMA); metric-view metadata
(WithEnableMetricViewMetadata); the retry / backoff policy (WithRetries:
RetryWaitMin / RetryWaitMax / RetryMax, including the disable form, forwarded to the
kernel's HTTP retry config); and the TLS, proxy, and session-conf (query tags,
statement timeout, time zone) options. Federated token providers use one token snapshot
during setup. Nothing is silently ignored: WithTimeout and token-provider / external /
static authenticators are rejected at connect; staging (PUT/GET/REMOVE on a
Unity Catalog volume) is rejected at execute. WithMaxRows is accepted but inert (the
kernel manages fetching below the C ABI).
static authenticators are rejected at connect. WithMaxRows is accepted but inert
(the kernel manages fetching below the C ABI).

Kernel HTTP requests inherit the driver's ClientTimeout (900s by default). If
that value is zero, the kernel uses its own 120s default; zero is neither
Expand Down
10 changes: 0 additions & 10 deletions examples/kernel/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,4 @@ func main() {
log.Fatalf("query: %v", err)
}
fmt.Printf("server version: %s\n", version)

// Options the kernel can't honor are rejected with an error wrapping
// ErrNotSupportedByKernel, never silently ignored. Staging (PUT/GET/REMOVE on a
// Unity Catalog volume) is one such feature, rejected at execute.
_, err = db.ExecContext(ctx, "PUT '/tmp/local.csv' INTO '/Volumes/main/default/vol/f.csv' OVERWRITE")
if errors.Is(err, dbsqlerr.ErrNotSupportedByKernel) {
fmt.Println("staging is not supported on the kernel backend (use Thrift for staging)")
} else if err != nil {
fmt.Printf("staging: %v\n", err)
}
}
3 changes: 3 additions & 0 deletions internal/backend/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ type ExecRequest struct {
// dbsql.Parameter/SqlType types; each backend maps these neutral params to its
// own wire type.
Params []Param
// TransferFiles requests native completion where the backend supports it.
// Other backends leave transfers to the connection layer.
TransferFiles bool
}

// Param is one bound query parameter in backend-neutral form: the name (empty
Expand Down
14 changes: 4 additions & 10 deletions internal/backend/kernel/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,9 @@ import (
"sync/atomic"
"time"

dbsqlerr "github.com/databricks/databricks-sql-go/errors"
"github.com/databricks/databricks-sql-go/driverctx"
"github.com/databricks/databricks-sql-go/internal/backend"
dbsqlerrint "github.com/databricks/databricks-sql-go/internal/errors"
)

// kernelSessionSeq is a process-wide monotonic counter for kernel session ids.
Expand Down Expand Up @@ -890,15 +891,8 @@ func (k *KernelBackend) SessionID() string { return k.sessionID }
// parameters (req.Params) are bound onto the statement in execute (see
// bindParams).
func (k *KernelBackend) Execute(ctx context.Context, req backend.ExecRequest) (backend.Operation, error) {
// Staging (Unity Catalog volume PUT/GET/REMOVE) needs a local file transfer the
// kernel path can't perform, and the kernel C ABI surfaces no IsStagingOperation
// signal to drive conn.execStagingOperation. Reject it here rather than let
// IsStaging return false and report success with no file moved (silent data loss).
// (Bound parameters, by contrast, ARE now supported — bound in execute via the
// kernel's raw-param C ABI.)
if isStagingStatement(req.Query) {
return &kernelOp{}, fmt.Errorf("databricks: staging operations (PUT/GET/REMOVE on a volume) are %w; "+
"use the default (Thrift) backend", dbsqlerr.ErrNotSupportedByKernel)
if req.TransferFiles && isStagingStatement(req.Query) && len(driverctx.StagingPathsFromContext(ctx)) == 0 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium — This turns kernel staging from rejected (wrapping ErrNotSupportedByKernel) into supported, but the user-facing docs still describe the old behavior. examples/kernel/main.go:91-99 still says staging is "rejected at execute" and branches on errors.Is(err, dbsqlerr.ErrNotSupportedByKernel). That branch can no longer match: the example passes no staging context, so it now gets the plain "staging ctx must be provided" error. Please update the example, and any README/CONNECTION_PARAMETERS rows that list staging as kernel-unsupported, in the same PR. That way callers learn they need driverctx.NewContextWithStagingInfo and stop relying on the sentinel.

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.

Updated the kernel example to pass driverctx.NewContextWithStagingInfo when optional upload paths are provided. Added the ExecContext and QueryContext contracts to CONNECTION_PARAMETERS.md and the feature to CHANGELOG.md.

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 for the update, but the checked-out head doesn't show it yet. examples/kernel/main.go:92-98 still has the "rejected at execute" comment, still branches on errors.Is(err, dbsqlerr.ErrNotSupportedByKernel), and still prints "staging is not supported on the kernel backend (use Thrift for staging)". I couldn't find a NewContextWithStagingInfo call anywhere in the example. A search of CHANGELOG.md also turned up no new kernel staging entry. Those changes may not have been pushed yet. Once a commit with the example and doc changes lands, this will be checked again.

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.

The stale unsupported-transfer example is removed. CONNECTION_PARAMETERS.md and CHANGELOG.md describe ExecContext transfers, required staging context, QueryContext raw instructions, and the native publication dependency. Tested changes are at 484a2f6.

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. The CHANGELOG.md entry (line 4) and the new "Volume file transfers" section in CONNECTION_PARAMETERS.md (around line 241) both look good. The example hasn't been updated yet, though: examples/kernel/main.go:92-98 in the checked-out head still has the "rejected at execute" comment, still branches on errors.Is(err, dbsqlerr.ErrNotSupportedByKernel), and still prints "use Thrift for staging". The head may not include 484a2f6 yet. If it does, that block still needs to be removed or switched to driverctx.NewContextWithStagingInfo. I'll check again on the next push.

Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
return &kernelOp{}, dbsqlerrint.NewDriverError(ctx, "staging ctx must be provided.", nil)
}
return k.execute(ctx, req)
}
5 changes: 5 additions & 0 deletions internal/backend/kernel/include/databricks_kernel.h
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,11 @@ KernelStatusCode kernel_session_rollback(kernel_session_t* session);
* so a reused statement handle starts each query with a fresh parameter set. */
KernelStatusCode kernel_statement_set_sql(kernel_statement_t* stmt, const char* sql);

/** Configure allowed local paths; null restores caller-managed transfers.
* retain_result keeps instructions after transfer. */
KernelStatusCode kernel_statement_set_staging_allowed_local_paths(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 High — The vendored header now declares kernel_statement_set_staging_allowed_local_paths, and operation.go calls it unconditionally on every ExecContext. But go.mod still pins the prebuilt databricks-sql-kernel-bindings/lib/* modules at v1.4.0, and those predate databricks-sql-kernel#439. A databricks_kernel-tagged build against the published archives will fail to link with an undefined symbol. The PR description already lists this as a merge gate. Flagging it inline so the PR can't merge until the bindings bump lands with it (and CHANGELOG notes the minimum bindings version).

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.

Agreed and still open: kernel #443 tracks publication plus the go.mod/go.sum updates in this PR before merge. Published native 1.4.0 lacks this symbol, so tagged builds using those archives cannot link. The validated local build replaces the archive with one built from the pinned kernel source; it does not validate the declared published dependency. No release version is assigned or invented.

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 for confirming. The finding stays open as a merge gate until the published bindings that include this symbol are released and go.mod/go.sum in this PR are bumped to them. When that happens, please also add a CHANGELOG note giving the minimum bindings version for databricks_kernel-tagged builds. Your local build against a source-built archive is useful, but you're right that it doesn't check the published dependency the PR declares, so the link failure is still there for consumers today.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium — The header now declares kernel_statement_set_staging_allowed_local_paths, and operation.go calls it. But go.mod still pins every databricks-sql-kernel-bindings/lib/* module at v1.4.0, and those archives were built before kernel#439. Any databricks_kernel-tagged build that uses the published modules will fail at link time with an undefined symbol. The PR description lists this as a merge gate. Flagging it here so the code can't merge while the KERNEL_REV/header bump and the go.mod bindings version disagree; CI with the tag should go red until the bindings are published and bumped.

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.

Confirmed: the published v1.4.0 archives do not export the setter. This PR is merge-gated on matching native publication and go.mod/go.sum updates in https://github.com/databricks/databricks-sql-kernel/issues/443; local validation uses the rebuilt archive, not v1.4.0. No release version has been assigned, so the dependency bump remains pending. For the summary nits: the actual JSON/CString/thread-lock/native-setter path measured 414.4 ns/call (five 10,000-call blocks after 10,000 warmups), which cannot explain the earlier 159 ms gap. None exposes raw instructions while [] denies local access, so setting it for every ExecContext prevents false-success transfers. Connection-level routing plus the live native positive/raw tests cover behavior; the previously rejected zero-session FFI/error-string test has not been restored. The changelog now explicitly describes the ErrNotSupportedByKernel fallback behavior change.

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 for confirming, and for linking databricks-sql-kernel#443. Since the published v1.4.0 archives still don't export kernel_statement_set_staging_allowed_local_paths, any databricks_kernel-tagged build that uses the go.mod-pinned modules will fail at link time until new bindings are published and go.mod/go.sum are bumped. Your local validation used a rebuilt archive, so it doesn't cover that path. I'm leaving this thread open as the merge gate. It can be resolved once the push that bumps the bindings lands. The benchmark and changelog notes are useful, but they don't change this finding.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 High — The Go code now calls kernel_statement_set_staging_allowed_local_paths (operation.go:93), and KERNEL_REV was bumped. But go.mod still pins every databricks-sql-kernel-bindings/lib/* module at v1.4.0, and those archives were built before databricks-sql-kernel#439 and don't export this symbol. If this merges as-is, any databricks_kernel-tagged build that uses the published bindings instead of a locally built archive will fail at link time with an undefined reference. The description already lists this as a merge gate. Recording it here so the PR can't merge until go.mod points at a bindings release that contains #439, and CI should run a tagged build against the published modules.

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.

Confirmed this is a blocking release dependency, and the failed review job correctly reflects it. Latest native release remains v1.4.0, no replacement version is assigned, and those archives lack the symbol. Local tagged builds use the rebuilt archive and do not validate the published modules. Matching publication, go.mod/go.sum updates in this PR, and a tagged build against the published modules remain mandatory under https://github.com/databricks/databricks-sql-kernel/issues/443. Do not merge before that gate is satisfied.

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 for confirming, and for linking databricks-sql-kernel#443 to track it. Nothing has changed in the code yet: go.mod still pins the bindings modules at v1.4.0, and those archives don't export kernel_statement_set_staging_allowed_local_paths. I'm leaving this thread open as the merge gate. It can be resolved once go.mod and go.sum point at a published bindings release that includes #439, and a databricks_kernel-tagged build against the published modules (not a locally rebuilt archive) passes in CI.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 High — The header now declares kernel_statement_set_staging_allowed_local_paths, and operation.go calls it on every ExecContext. But go.mod still pins every databricks-sql-kernel-bindings/lib/* module at v1.4.0, and the PR description says no native release with kernel#439 exists yet. A databricks_kernel build against the published archives will fail at link time with an undefined symbol. The local validation passed only because it used a locally built Rust archive. Please gate the merge on a bindings release and the matching go.mod bump in this PR, or fail CI (for example with a tagged build job against the module archives), so this can't be merged before the bindings exist.

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.

Agreed. This is a merge/release blocker tracked in https://github.com/databricks/databricks-sql-kernel/issues/443. Published v1.4.0 archives lack the setter; the local tagged build uses a rebuilt archive and does not validate those published modules. The matching native modules must be published and go.mod/go.sum updated in this PR before merge, followed by a tagged build and live lifecycle using the published artifacts. No replacement version has been assigned, and this PR remains unmerged.

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 for confirming, and for tracking this in databricks-sql-kernel#443. Since go.mod/go.sum still pin the v1.4.0 modules and those don't include the setter, I'm leaving this thread open as the merge blocker. It can be resolved once the PR bumps to the published bindings release that includes kernel#439, and a tagged build plus a live lifecycle run pass against those published archives rather than a locally rebuilt one.

Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium — operation.go now calls kernel_statement_set_staging_allowed_local_paths, which this header declares. But go.mod still pins the published v1.4.0 bindings, and per the PR description those don't export the symbol. So any databricks_kernel build that resolves the archive from the module (consumers, and the new Build (published kernel) job) fails at link time. The PR description already calls this a pre-merge gate. Flagging it inline so the PR isn't merged before the bindings bump and go.sum update land in this same PR. A tagged consumer build with a mismatched archive is a hard break, not a gradual degradation.

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.

Confirmed: this is the explicit kernel #443 merge/release gate. The published-module build must pass after an assigned native version is published and go.mod/go.sum are updated in this PR. Locally rebuilt-source validation is separate. No replacement version is available to pin yet, and this PR stays unmerged.

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 for confirming. Since the go.mod pin hasn't changed yet and the header still declares a symbol that the published v1.4.0 archive doesn't export, I'll leave this thread open as the merge gate. Once the new native version is published and go.mod/go.sum are bumped in this PR, the Build (published kernel) job passing will be the evidence that it's resolved.

kernel_statement_t* stmt, const char* paths_json, bool retain_result);

/* Typed positional parameter binding (1-based ordinal); the kernel infers the
* SEA wire type from the C type. These are the "typed params" the
* kernel_statement_bind_parameter contract refers to: within one statement they
Expand Down
40 changes: 16 additions & 24 deletions internal/backend/kernel/kernel_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -531,30 +531,22 @@ func TestExecuteHandleLessOpContract(t *testing.T) {
}
}

// TestExecuteRejectsStaging drives Execute with a staging statement (not just the
// isStagingStatement detector in isolation) to pin the detector→Execute wiring: a
// refactor that dropped or reordered the check would silently reopen the
// silent-no-op data-loss path. Mirrors TestExecuteRejectsParams.
func TestExecuteRejectsStaging(t *testing.T) {
k := &KernelBackend{}
op, err := k.Execute(context.Background(), backend.ExecRequest{
Query: "PUT '/tmp/f' INTO '/Volumes/main/s/e/f.csv'",
})
if err == nil {
t.Fatal("expected an error for a staging statement, got nil")
}
if !errors.Is(err, dbsqlerr.ErrNotSupportedByKernel) {
t.Errorf("staging rejection should wrap ErrNotSupportedByKernel, got %v", err)
}
if op == nil {
t.Fatal("Execute must return a non-nil Operation per the Backend contract")
}
closed, closeErr := op.Close(context.Background())
if closeErr != nil {
t.Errorf("Close error = %v, want nil", closeErr)
}
if closed {
t.Error("Close on a handle-less op must report closed=false (no CLOSE_STATEMENT)")
func TestExecuteRequiresStagingContext(t *testing.T) {
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved

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 — The new test only covers TransferFiles: true with no staging context. Two parts of the new contract have no unit test:

  • The TransferFiles: false path (QueryContext) must not be rejected when the staging context is missing. This is the behavior that replaced the old blanket rejection, and if someone drops the req.TransferFiles && guard, no unit test fails.
  • No unit test checks that a context holding allowed paths gets past the pre-check.

Only the credential-gated e2e test exercises either path. Please add a table entry such as {query, TransferFiles:false} and assert that the pre-check error does not appear. With a zero-value KernelBackend the call will still fail later, but the assertion can check the error message.

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.

The earlier zero-session test was removed after review comment 4544844987 identified its dependence on a native new_statement(NULL) error string. Restoring that pattern would reproduce the same test-quality issue. TestConnectionTransferMode now delegates through a real handle-less Operation and covers both routing flags. The durable TestKernelE2EVolumeFileLifecycle also verifies missing context, positive allowed paths, transferred bytes, and the actual raw GET instruction without side effects; those cases passed locally. The native option boundary is covered by C ABI and non-ignored ADBC tests. Kept this behavioral coverage without a test-only production seam.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I agree that tests shouldn't depend on the native new_statement(NULL) error string, but the assertion I suggested doesn't. It only checks that the error does not contain "staging ctx must be provided", so whatever the native layer returns later doesn't matter. TestConnectionTransferMode runs against a fake backend that just records req.TransferFiles (connection_test.go:985). That proves the flag is passed through, but it never reaches the guard at internal/backend/kernel/backend.go:894. That leaves the req.TransferFiles && condition and the allowed-paths case untested outside the credential-gated e2e test, which doesn't run in normal CI. It's low priority, so I'll leave the call to a maintainer, but the gap is still there.

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 — TestExecuteRequiresStagingContext only tests TransferFiles: true. The other half of the new guard is untested: QueryContext (TransferFiles: false) running a staging statement without a staging context must not hit the "staging ctx must be provided" error and must go on to execute. On a zero-value KernelBackend, new_statement fails anyway, so a test can just assert that the error does not contain the staging message. Without that test, a refactor that drops the req.TransferFiles && check would break the raw-instruction behavior of QueryContext, and only the credential-gated e2e test would catch it.

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.

An earlier review (#543 (comment)) explicitly requested removing the zero-session new_statement error-string test as brittle. It was removed accordingly. TestConnectionTransferMode checks Exec/Query flags through the real connection path; the durable native lifecycle tests positive staging context and consumes/asserts the raw GET instruction without downloading, and matched live Thrift/native raw GET/REMOVE probes pass. Keeping these behavioral checks instead of restoring the rejected null-session test or adding a production helper solely for branch coverage.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fair point about the earlier review. You're right not to bring back a brittle test that matches on new_statement error strings. But the gap I flagged is still there. TestConnectionTransferMode checks that Query sets TransferFiles: false. It doesn't check that Execute skips the staging-context guard when that flag is false, so a refactor that drops the req.TransferFiles && check would only show up in the credential-gated e2e test or a live probe. This is low-severity, so it's reasonable for maintainers to accept the trade-off. I'm leaving the thread open so that call stays visible rather than getting closed silently.

Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved

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 — TestExecuteRequiresStagingContext only covers TransferFiles: true. Nothing pins the other half of the guard: with TransferFiles: false (the QueryContext raw-instruction path) and no staging context, Execute must not reject a staging statement. The e2e test covers that case only behind credentials. An untagged unit test could cover it, for example by routing through an execute seam or asserting the error is not the staging-context error. Without one, a refactor that drops the req.TransferFiles && term would quietly break QueryContext's documented raw-instruction behaviour without failing CI.

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.

This is the previously resolved coverage trade-off: connection_test.go records the TransferFiles flag passed by QueryContext, and the real native API tests verify raw GET/REMOVE without side effects, ExecContext transfer behavior and missing-context rejection. The post-guard native execution combinations are not claimed as unit coverage. A nil-session error-string assertion or an extracted predicate would restate the guard without proving actual execution; I am retaining API coverage and the writable scheduled-fixture gate in kernel #443 rather than adding a test-only production seam.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's a fair trade-off to make, and I understand why you'd rather not add a test-only seam. One point on the alternative, though: a nil-session test that only checks the error is not staging ctx must be provided isn't just a restatement of the guard. It's the only untagged check that would fail if someone dropped the req.TransferFiles && term. The connection_test check confirms QueryContext passes TransferFiles: false, but not that Execute acts on that flag, and the native API tests only run with credentials. It's still Low and your call to accept. I'll leave the thread open so a human maintainer can sign off on relying on kernel #443 for this coverage.

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 — The new test only covers TransferFiles: true with no staging context. Nothing checks the other branch of the guard at backend.go:894: when TransferFiles is false (the QueryContext path), a staging statement must get past the pre-check so raw instructions come back. If a refactor dropped the req.TransferFiles && term, QueryContext would quietly break, and only the credential-gated e2e test would catch it. A CGO-free test could show that Execute with TransferFiles: false and a staging query does not return the staging ctx must be provided error. For example, use a nil-session backend and assert that the error, if any, is something else. That test should also cover a lowercase keyword such as get ..., since the table-driven test only uses uppercase queries.

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.

The connection test asserts the captured flags are exactly [true, false], and the live raw GET/REMOVE controls verify that QueryContext returns instructions without file side effects. These tests cover the public contract. A nil-session test that relies on a later failure being different from an error string would couple coverage to an invalid backend state; I am retaining the existing flag and real-API coverage.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agreed on one point: the connection test pins the flags that connection.go sends, so the QueryContext → TransferFiles: false wiring is covered. That test runs against a fake backend that only records req.TransferFiles, though, so it never reaches the guard at kernel/backend.go:894. The only TransferFiles reference in kernel tests is the true case at kernel_test.go:538. If the req.TransferFiles && term were dropped, the connection test would still pass, and only the credential-gated live GET/REMOVE tests would fail. I take the point that asserting on a later nil-session error is brittle. One way around it is to move the pre-check into a small pure helper, for example requiresStagingCtx(req, ctx) bool, and table-test it with both flag values and a lowercase keyword. That would cover the gap without relying on invalid backend state. It's low severity and your call, but the gap I flagged is still open.

Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
for _, query := range []string{"GET '/Volumes/main/s/v/f' TO '/tmp/f'", "PUT '/tmp/f' INTO '/Volumes/main/s/v/f'", "REMOVE '/Volumes/main/s/v/f'", "/* upload */ PUT '/tmp/f' INTO '/Volumes/main/s/v/f'"} {
t.Run(query, func(t *testing.T) {
k := &KernelBackend{}
op, err := k.Execute(context.Background(), backend.ExecRequest{Query: query, TransferFiles: true})
if err == nil || !strings.Contains(err.Error(), "staging ctx must be provided") {
t.Fatalf("expected missing staging context error, got %v", err)
}
if op == nil {
t.Fatal("expected non-nil operation")
}
closed, closeErr := op.Close(context.Background())
if closed || closeErr != nil {
t.Fatalf("Close = %v, %v", closed, closeErr)
}
})
}
}

Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
Expand Down
34 changes: 28 additions & 6 deletions internal/backend/kernel/operation.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import "C"
import (
"context"
"database/sql/driver"
"encoding/json"
"errors"
"fmt"
"sync"
Expand All @@ -35,8 +36,7 @@ import (
// client-timeout path, let an in-flight cancel finish in the background so
// it cannot extend the client deadline
//
// Binds any query parameters (bindParams) before executing; staging statements
// are rejected up front by Execute, so none reach here.
// Binds query parameters and configures local file access before executing.
func (k *KernelBackend) execute(ctx context.Context, req backend.ExecRequest) (backend.Operation, error) {
// Log the SQL length, not the text: query bodies can carry PII/secrets in
// WHERE/INSERT/SET, and this goes to stderr. Matches the driver's own
Expand Down Expand Up @@ -78,6 +78,30 @@ func (k *KernelBackend) execute(ctx context.Context, req backend.ExecRequest) (b
}
sql.free()

// Every ExecContext opts into completion, including transfers missed by the SQL recognizer.
// An empty list denies local transfers instead of reporting raw instructions as success.
if req.TransferFiles {
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium — The new kernel_statement_set_staging_allowed_local_paths branch has no unit coverage: there are no tests for the JSON encoding of nil vs. empty paths, the failure path (where stmt should be closed and evictIfSessionFatal called), or a check that QueryContext never sets the option. Only the credential-gated e2e test reaches this code, so a regression here (for example, sending null instead of [] when the context has no paths, which according to the header comment switches back to caller-managed transfers and silently returns instructions from ExecContext) would get through CI. The guard in Execute only catches statements that isStagingStatement recognizes. For anything it misses, the [] deny-list is the only thing preventing a silent no-op success. Consider pulling the path-encoding step into a pure-Go helper (as staging.go already does) and adding a test that asserts nil→[].

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.

The real native option boundary is covered by the kernel's C ABI option tests and non-ignored ADBC set/clear/invalid-value test. The Go connection test checks ExecContext forwards TransferFiles=true and QueryContext false; the credential-gated lifecycle checks transferred bytes, denied paths, and actual raw QueryContext rows. A separate matched Thrift/native lifecycle also passed with relative and parent-directory roots. Nil is explicitly normalized to [] before marshaling. The setter-error branch frees the CString, closes the statement, and invokes evictIfSessionFatal. A pure marshaling helper would not exercise setter failure or native resource cleanup, so I have not added a test-only production seam. The coordinated native publication and go.mod/go.sum update in kernel #443 remain an explicit pre-merge gate.

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 for the detailed breakdown. I agree the nil→[] normalization is in place and the setter-error branch cleans up correctly. My concern is narrower than that: none of the coverage you list runs in this repo's default CI. The C ABI and ADBC tests live in the kernel repo, the lifecycle tests need credentials, and the connection test only checks the TransferFiles flag. Nothing here would fail if someone later removed the paths == nil guard. A small helper like encodeStagingPaths(ctx) ([]byte, error), with a test that asserts nil gives [], wouldn't be a test-only seam. It would be the same encoding step Execute already does, and it would protect the one case that turns a denied transfer into a silent success. You're right that it wouldn't cover setter failure or native cleanup. That's fine; this one assertion is the part that matters most. If you still decide not to add it, I'd suggest saying so in the PR description, next to the kernel #443 gate, so reviewers can see that the gap is intentional.

Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium — This branch runs on every kernel ExecContext, including plain INSERT/UPDATE/DDL, not only staging statements. As a result, every databricks_kernel build now links kernel_statement_set_staging_allowed_local_paths unconditionally. go.mod still pins databricks-sql-kernel-bindings/lib/* at v1.4.0, and those archives don't contain this symbol. So any tagged build that uses the published bindings rather than a locally built archive won't link, which breaks every kernel user, not just volume users. The PR description lists this as a merge gate, but nothing in the code or build enforces it. Before merge, please either bump the bindings in go.mod in this PR, or add a check (CI or Makefile) that KERNEL_REV matches the pinned bindings.

There's a second-order cost: each non-staging Exec now pays an extra json.Marshal, a C string allocation, and an FFI call. Setting [] on every statement is a reasonable fail-closed default against isStagingStatement false negatives. Still, if the open 10% slowdown traces back here, consider applying it only when isStagingStatement matches, and relying on the kernel's default to deny transfers. Before changing it, confirm what the kernel default does when the option is left unset.

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.

Confirmed; the published-archive linkage gate remains unresolved in kernel #443. go.mod/go.sum must be updated in this PR before merge once a real release is assigned. The local archive replacement used for validation is explicitly recorded and does not validate published v1.4.0. On the separate setter cost, the complete JSON/CString/thread-lock/native call measured 414.4 ns median over five 10,000-call blocks after 10,000 warmups. The kernel default leaves raw instructions available; [] enables completion while denying local files. This is why ExecContext configures the option unconditionally.

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 for confirming, and for the setter numbers: about 414 ns per call is small, so I'm dropping the cost question. I'm keeping this open because the main issue is still there. The current snippet now guards the setter with if req.TransferFiles, but that doesn't help with linking. The cgo reference to kernel_statement_set_staging_allowed_local_paths is still compiled into every databricks_kernel build. With go.mod still pinned to bindings v1.4.0, builds that use the published archives will still fail to link. Also, your reply says ExecContext sets the option on every statement, but the code at line 82 now only sets it when req.TransferFiles is true. Please check that the comment and the behaviour say the same thing. Before merge, this needs either the go.mod/go.sum bump or a CI or Makefile check that KERNEL_REV matches the pinned bindings.

Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved

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 — The new TransferFiles branch in execute has no unit test. That branch JSON-encodes the staging paths, turns a nil list into [] so transfers are denied rather than returned as raw instructions, and on error closes the statement and evicts on session-fatal errors. TestExecuteRequiresStagingContext only covers the pre-check in Execute, and the live e2e test is the only thing that runs this path. A small seam would make it testable without a warehouse: for example, pull the path-encoding into a pure helper and test the nil → [] case. That case matters because [] is what stops a statement the staging detector misses from silently 'succeeding' with no file moved.

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.

The native lifecycle tests the actual JSON/C-string/C-ABI option boundary, successful bytes and denied paths; the kernel has non-ignored option set/clear/invalid-value tests. A pure encoding helper test would only retest json.Marshal and would not detect a missing setter or cleanup call. Earlier review explicitly requested removing the null-session FFI/error-string guard test, and it has not been restored. Keeping the behavioral coverage and the comment explaining that [] denies transfers while an unset option returns raw instructions; no production seam solely for encoding coverage.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's a fair point: a helper test on its own would mostly re-test json.Marshal, and the kernel-side option tests do cover what [] means. What they can't catch is the Go side, which is lines 84–86 turning a nil StagingPathsFromContext into []. If someone removes that guard, the driver sends null or skips the setter, and statements fall back to returning raw instructions. No test in this repo would fail. A grep turns up no Go test that reaches this branch (the only TransferFiles: true test hits the pre-check in backend.go), and the native lifecycle tests you mention aren't in this repo, so I couldn't check them. It's Low severity and your call whether the risk is worth a seam, so I'll leave this open for a human to decide rather than resolve it.

Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
paths := driverctx.StagingPathsFromContext(ctx)
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
if paths == nil {
paths = []string{}
}
encoded, err := json.Marshal(paths)
if err != nil {
C.kernel_statement_close(stmt)
return &kernelOp{}, fmt.Errorf("kernel: encode allowed local paths: %w", err)
}
value := newCStr(string(encoded))
err = call(func() C.KernelStatusCode {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium — This calls the new C symbol kernel_statement_set_staging_allowed_local_paths, but go.mod still pins the published databricks-sql-kernel-bindings/lib/* modules at v1.4.0, and those archives were built before databricks-sql-kernel#439. If this merges as is, any databricks_kernel build that links against the published modules fails with an undefined symbol. That covers every downstream consumer and the new build-published-kernel CI job. The description calls this a merge gate, but nothing in the code enforces it. Please bump the bindings version in go.mod and go.sum in this same PR before it lands, so the new CI job can actually catch the problem. Also make sure the new job is a required check.

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.

Confirmed by the new CI job: Build (published kernel) fails with the missing setter in v1.4.0. The source-build job and locally rebuilt archive are separate evidence. Kernel #443 remains the merge/release gate for publication and the go.mod/go.sum bump in this PR; no replacement version is assigned. Making the job a required branch check is a repository-policy action for maintainers, also recorded in the gate. This PR stays unmerged.

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 for confirming. The failing Build (published kernel) job shows the issue is real, and it's good that this PR stays unmerged until kernel #443 publishes the new bindings. Nothing in the code has changed yet: go.mod/go.sum still pin v1.4.0, and that version doesn't have kernel_statement_set_staging_allowed_local_paths. So I'll leave this thread open as the visible merge gate. It can be resolved once the bindings bump lands in this PR and the published-kernel job passes. Making that job a required check is up to the maintainers, as you said.

return C.kernel_statement_set_staging_allowed_local_paths(stmt, value.c, false)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 High — execute now calls C.kernel_statement_set_staging_allowed_local_paths on every ExecContext (whenever req.TransferFiles is true), not only on staging statements. That symbol comes from kernel#439, but go.mod still pins the native bindings at v1.4.0, which doesn't have it. So any databricks_kernel-tagged build that uses the published modules will fail to link. That breaks the whole kernel backend, not just volume transfers. The PR description calls this a merge gate, but nothing in the code enforces it. Please bump the databricks-sql-kernel-bindings/lib/* requires in go.mod to the release that contains kernel#439 as part of this PR (or in a blocking follow-up) before merging, so the KERNEL_REV/header pin and the linked archive can't get out of sync.

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.

Confirmed. Published v1.4.0 archives cannot link this new symbol. The local tagged build and live tests use an archive built from the pinned source; that validates implementation only. Kernel#443 tracks publishing the new native modules and updating every go.mod requirement/sum in this PR before merge. No version is assigned, so this remains an unresolved merge gate and the PR is not ready to merge.

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 for confirming, and for linking the tracking issue in kernel#443. The go.mod bump hasn't happened yet, so this finding is still open and works as the merge gate. The thread stays unresolved until the databricks-sql-kernel-bindings/lib/* requires and go.sum are updated to a published release that contains kernel#439. Once that's pushed, I'll re-check it.

Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium — kernel_statement_set_staging_allowed_local_paths gets called on every ExecContext, but the published kernel module this repo pins (v1.4.0, which go.mod/go.sum still reference) doesn't export that symbol. If this merges before the bindings are published and go.mod/go.sum are bumped, every -tags databricks_kernel build that links the published archive fails with an undefined symbol. That includes downstream users, not only CI. The PR description already lists this as a pre-merge gate. Please hold the merge (draft PR or a blocking label) until the dependency bump lands in this PR, so the header, KERNEL_REV and the module version change together.

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.

Confirmed on the current head: the published-native CI log fails to link kernel_statement_set_staging_allowed_local_paths against 1.4.0. This PR is explicitly blocked from merge until matching native modules are published, go.mod/go.sum are updated, and the published-native job passes. Source-build tests do not satisfy that gate. Kernel issue #443 tracks the coordinated publication and installed-package validation; no release version has been assigned.

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 for confirming, and for the link to kernel issue #443. The code still calls kernel_statement_set_staging_allowed_local_paths, and go.mod/go.sum still pin v1.4.0, so the finding is still outstanding. I'm leaving this thread open as a visible merge gate. Please also mark the PR as draft or add a blocking label so nobody merges it by accident. It can be resolved once the published native modules are out, go.mod/go.sum are bumped in this PR, and the published-native CI job passes.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Medium — Every kernel-backed ExecContext now calls kernel_statement_set_staging_allowed_local_paths, not only staging statements, because TransferFiles is always true for Exec. go.mod still pins the published bindings at v1.4.0, and that release doesn't export this symbol. Until the bindings are bumped, any consumer building with -tags databricks_kernel against the published modules gets a link failure, whether or not they use Volumes. The new build-published-kernel job will catch this, but only once it's a required check. Please treat the go.mod/go.sum bump as a hard merge blocker (e.g. keep the PR in draft) rather than relying on the description alone.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 High — kernel_statement_set_staging_allowed_local_paths is called on every ExecContext here, and declared in the vendored header. It is not in the databricks-sql-kernel-bindings v1.4.0 archives that go.mod still pins (go.mod:70-76). That means any -tags databricks_kernel build that uses the published modules will fail to link with an undefined symbol. The new Build (published kernel) job is there to catch exactly this, and it fails today. The description lists this as a pre-merge gate, but nothing in the code blocks a merge. Keep this PR unmergeable (draft/label) until the bindings are published and go.mod/go.sum are bumped in this same PR. Otherwise every kernel-tagged consumer breaks.

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.

Converted this PR to draft. Published 1.4.0 still lacks the setter, and Build (published kernel) remains a real failing gate. #443 tracks version assignment/publication, go.mod/go.sum updates and installed-package validation before this PR is made ready. Local source builds do not satisfy that gate.

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 for converting this to draft and opening #443 to track it. Draft status does what I asked for now, but the code at line 95 still calls a symbol that the pinned v1.4.0 bindings don't export. I'll leave this thread open as a visible blocker. It should close once go.mod/go.sum in this PR are bumped to a published bindings version that includes kernel_statement_set_staging_allowed_local_paths, and Build (published kernel) passes.

})
value.free()
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
if err != nil {
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Resolved
C.kernel_statement_close(stmt)
k.evictIfSessionFatal(err)
return &kernelOp{}, fmt.Errorf("kernel: allowed local paths: %w", toStatementError(err))
}
}

// Bind parameters. The driver hands us backend.Param{Name, Type, Value}: the
// value is already stringified, Type is the Databricks SQL type name, and a nil
// Value is SQL NULL (Type "VOID"). Each maps 1:1 onto
Expand Down Expand Up @@ -384,10 +408,8 @@ func (o *kernelOp) Results(ctx context.Context, callbacks *dbsqlrows.TelemetryCa
return newKernelRows(ctx, o, stream, callbacks)
}

// IsStaging reports whether this is a staging (PUT/GET/REMOVE) operation. Always
// false on the kernel path: staging statements are rejected up front in
// Execute (isStagingStatement), so no kernelOp is ever produced for one — there
// is nothing for conn.execStagingOperation to act on here.
// IsStaging is false because the kernel completes configured file transfers
// before returning the result.
func (o *kernelOp) IsStaging(ctx context.Context) (bool, error) { return false, nil }

// Close best-effort closes the executed statement and its statement handle. It is
Expand Down
Loading
Loading