GeezerVison is showing github.com — fix(kernel): honor the client CloudFetch default by aakash-saravanan-db · Pull Request #574 · databricks/databricks-sql-nodejs
Conversation
With useKernel: true, the connector ignores the client's useCloudFetch=false setting, so large results still require direct access to cloud storage. This PR forwards the connection default when opening the native session, allowing complete results to be fetched through the SQL connection.
The connector sets the Rust kernel's existing cloudfetch_enabled option. The kernel handles option precedence and result delivery. This change covers the connection default; per-statement overrides remain unsupported in kernel mode.
Dependency: Requires databricks-sql-kernel #436, which fixes large-result delivery with CloudFetch disabled. KERNEL_REV pins that implementation for source builds. Before merging, publish native Node packages containing the fix and update the platform dependencies in package.json and package-lock.json to require them. The release version has not yet been assigned.
Validation: All 1,533 unit tests, lint, and build pass. With a locally built native binding, disabled delivery returns all 250,000 rows and 32 MB of payload with the expected hash through a proxy that blocks storage access. The enabled control attempts storage access. Query errors, reuse, and close also pass.
Performance follow-up: Five measured runs per mode, after warmup, give medians of 4.240 s for Thrift and 4.735 s for kernel (+11.7%). The 10% performance criterion remains unmet. Kernel issue #440 tracks repeated result fetching in the asynchronous path separately.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verdict: 1 Nit
Looks good. The change is small and contained: the caller's configuration is copied rather than mutated, the key is removed regardless of case before being set, and queryTags still merges correctly after it, all backed by parameterized unit tests. I couldn't confirm from this repo that the kernel actually acts on the can_cloud_download session-conf key (nothing in native/ mentions it), so that rests on the PR's proxy-based validation.
Other findings
- ⚪ Nit — CHANGELOG: the new entry is followed by a blank line, so it sits apart from the rest of the
Unreleased list, and unlike its neighbours it has no PR reference (e.g. (#574)).
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No issues identified by the review bot.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No issues identified by the review bot.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
![peco-review-bot[bot]](/api/image?u=https%3A%2F%2Favatars.githubusercontent.com%2Fin%2F3801177%3Fs%3D60%26v%3D4)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verdict: 1 Medium · 1 Nit
The code change is small and looks correct, with 1 medium doc issue and 1 low test-gating issue. openSession() adds cloudfetch_enabled: 'false' without mutating the caller's configuration, and the unit tests cover it. Precedence over user-supplied can_cloud_download / CLOUDFETCH_ENABLED is up to the kernel (databricks-sql-kernel#436), which I couldn't verify here.
Other findings
- ⚪ Nit — Every other kernel e2e suite (
select-one, execution, auth-*, results, logging, …) has a before gate that calls this.skip() when the kernel credentials or the native binding are missing. This suite has no gate. On machines without the kernel binding or provisioned credentials it fails instead of skipping. It also uses the generic ../utils/config host rather than the pecotesting env vars the sibling kernel suites use. Add the same skip gate the other kernel suites use.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No issues identified by the review bot.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No issues identified by the review bot.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This branch was previously deployed
1 inactive
deployment
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|