Uh oh!
There was an error while loading. Please reload this page.
docs: add connection parameter reference (Thrift vs SEA/Kernel) - #457
docs: add connection parameter reference (Thrift vs SEA/Kernel)#457eric-wang-1990 wants to merge 5 commits into
Conversation
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Node.js integration tests triggered. View workflow run. |
Integration test approval reset. New commits were pushed to this PR. The A maintainer must re-review the changes and re-add the label to trigger tests again. Latest commit: adcb617 |
Thanks for your contribution! To satisfy the DCO policy in our contributing guide every commit message must include a sign-off message. One or more of your commits is missing this message. You can reword previous commit messages with an interactive rebase ( |
Documents every public ConnectionOptions / OpenSessionRequest / per-statement parameter and whether it is honored on the Thrift backend, the SEA/Kernel backend (useKernel), or both — making protocol support gaps explicit. Mirrors the databricks-sql-kernel connection-parameters reference format. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
Relocate docs/connection-parameters.md → CONNECTION_PARAMETERS.md at the repo root and add a README "Configuration" section linking to it, mirroring the existing Telemetry section's summary+link pattern. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Nit
Looks good — docs-only change and the load-bearing Thrift claims verify against source (getConnectionOptions exposes no public TLS fields, HttpConnection.createHttpsAgent hardcodes rejectUnauthorized: false, enableMetricViewMetadata auto-injects the conf in openSession, and the OAuth flow/oauthClientId behavior matches createAuthProvider). Only a minor nit: the PR description calls the file docs/connection-parameters.md but it lands as CONNECTION_PARAMETERS.md at repo root.
Other findings
- ⚪ Nit — The PR description repeatedly refers to the file as
docs/connection-parameters.md, but the diff addsCONNECTION_PARAMETERS.mdat the repository root (and README links toCONNECTION_PARAMETERS.md). No code impact — just a description/filename mismatch worth reconciling so future readers aren't sent to a nonexistentdocs/path. Consider either moving the file underdocs/(to match the description and the databricks-sql-kernel layout it models) or updating the description.
adcb617 to
789c465CompareAligns markdown table columns to satisfy the repo prettier check (prettier . --check). Whitespace-only; no content changes. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Docs-only PR adding a Thrift-vs-Kernel connection-parameter reference. I verified the doc's claims against lib/DBSQLClient.ts, HttpConnection.ts, and the contracts files — nearly everything is accurate, but one row is wrong: oauthScopes is marked ✅ honored on Thrift, yet createAuthProvider never passes it into DatabricksOAuth, so Thrift silently ignores it. One medium finding inline.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Docs-only PR adding a Thrift-vs-Kernel connection-parameter reference. Overall accurate and well-sourced — I verified the socketTimeout:0, SOCKS/proxy, noProxy→bypassHosts, retry ms→sec, maxConnections, mTLS both-or-neither, and customHeaders claims against the code and they hold. Two rows misdescribe the implementation: enableMetricViewMetadata injection is backend-agnostic (in DBSQLClient.openSession, not Thrift-only), and the kernel OAuth U2M / oauthClientId-no-secret behavior contradicts KernelAuth.ts (which mirrors Thrift and forwards the id rather than throwing). Nit: the PR description references docs/connection-parameters.md but the file is added at repo root as CONNECTION_PARAMETERS.md (the README link correctly points at the root file, so no broken link).
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Addresses: - #3663057241 at CONNECTION_PARAMETERS.md:60 - #3663100673 at CONNECTION_PARAMETERS.md:108 - #3663100689 at CONNECTION_PARAMETERS.md:62 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Docs-only PR adding CONNECTION_PARAMETERS.md plus comment corrections in KernelAuth.ts / KernelBackend.ts. I spot-checked the doc's high-risk claims against source (oauthScopes Thrift-ignore, oauthClientSecret-based U2M/M2M flow selection, enableMetricViewMetadata double-injection, the TLS rejectUnauthorized:false caveat, kernel sessionConf folding) — all accurate, and the comment edits correctly match the actual oauthClientSecret === undefined ? U2M : M2M logic. One row is wrong: noProxy is marked ✅ for Thrift but the Thrift path never consumes it (inline). Otherwise looks good.
Uh oh!
There was an error while loading. Please reload this page.
Addresses: - #3663168408 at CONNECTION_PARAMETERS.md:77 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
What
Adds
docs/connection-parameters.md— a single reference listing every public connection / session / per-statement parameter the Node.js connector accepts, and whether each is honored on the Thrift backend (default), the SEA / Kernel backend (useKernel: true), or both.The goal is to make protocol support gaps explicit: a parameter honored on one backend but ignored or rejected on the other is called out in a Gap column.
Format
Modeled on the databricks-sql-kernel connection-parameters reference. Grouped by concern: connection identity · authentication · HTTP/proxy/retries · TLS/SSL · results & type rendering · session defaults · telemetry · per-statement options. Each row has a ✅ / ❌ /⚠️ / — verdict per backend plus a Gap note.
Sources of truth
lib/contracts/IDBSQLClient.tslib/contracts/InternalConnectionOptions.tslib/DBSQLClient.ts,lib/thrift-backend/ThriftBackend.ts,lib/connection/connections/HttpConnection.tslib/kernel/KernelAuth.ts,lib/kernel/KernelBackend.ts,lib/kernel/KernelSessionBackend.tsNotable gaps documented
Thrift-only (missing/ignored on Kernel):
enableMetricViewMetadataauto-injection; auth typescustom/token-provider/external-token/static-token+ token federation; Azure-direct OAuth; custom OAuthpersistence; custom U2MoauthClientId; SOCKS proxies; per-statementuseCloudFetch/useLZ4Compression/stagingAllowedLocalPath.Kernel-only (no Thrift public equivalent):
maxConnections; all TLS controls (checkServerCertificate,checkServerCertificateHostname,customCaCert,clientCertPem,clientKeyPem).TLS caveat surfaced: the Thrift
connect()path exposes no public TLS options and hardcodesrejectUnauthorized: false(HttpConnection.ts); the internalca/cert/keyfields are never populated from a public option. TLS customization is therefore kernel-only in practice.All kernel-path behavior reflects the M0 stub (
useKernelis internal/unstable) and is noted as subject to change.Docs-only change — no code touched.
This pull request and its description were written by Isaac.