docs: fix two incorrect Kernel claims in CONNECTION_PARAMETERS.md - #930
docs: fix two incorrect Kernel claims in CONNECTION_PARAMETERS.md#930eric-wang-1990 wants to merge 3 commits into
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Pull request overview
Updates the connection-parameter reference to correct two Kernel/Thrift behavioral claims so the documentation matches actual backend behavior (doc-only change).
Changes:
- Clarifies that
username/passwordremoval errors are Thrift-only, while Kernel ignores them. - Updates the TLS section to note a Kernel exception for
_tls_client_cert_key_password. - Fixes the
_tls_client_cert_key_passwordtable row to mark Kernel as unsupported and describe the workaround.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| | `_tls_client_cert_file` | `str` | ✅ | ✅ | `None` | Client certificate for mutual TLS. | | ||
| | `_tls_client_cert_key_file` | `str` | ✅ | ✅ | `None` | Private key for the client certificate. | | ||
| | `_tls_client_cert_key_password` | `str` | ✅ | ✅ | `None` | Password for an encrypted client-key file. | | ||
| | `_tls_client_cert_key_password` | `str` | ✅ | ❌ | `None` | Password for an encrypted client-key file. **Kernel rejects this** with `NotSupportedError` — the kernel has no surface for an encrypted client key today; pass an unencrypted PEM key, or use the Thrift backend. | |
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
Two rows disagreed with the code: - `_tls_client_cert_key_password`: documented as honored on Kernel (✅), but the kernel path raises `NotSupportedError` when it is set (`_kernel_tls_kwargs`, src/databricks/sql/backend/kernel/client.py) — the kernel has no surface for an encrypted client key. Marked ❌ and noted the workaround (unencrypted key, or Thrift). Also caveated the section callout, which had claimed all TLS options are honored on both backends. - `username` / `password`: the "raises `ValueError`" note is Thrift-only. That error is raised inside `get_python_sql_connector_auth_provider` (src/databricks/sql/auth/auth.py), which the kernel path never calls, so on Kernel the params are silently ignored rather than rejected. Doc-only change. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
_disable_pandas is a Python-side Arrow->row conversion toggle, not a wire option. KernelResultSet inherits the shared _convert_arrow_table, which reads connection.disable_pandas, so the flag is honored on the kernel path too (the kernel returns Arrow that flows through the same conversion). Mark the row Kernel-supported and drop it from the "ignored on Kernel" summary list. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
… live Both `_use_arrow_native_decimals` and `_use_arrow_native_timestamps` look like twin knobs, but behave oppositely when off — confirmed against a live warehouse: - `_use_arrow_native_decimals=False`: no value-level effect. The wire encoding becomes an Arrow string, but the connector unconditionally re-casts it back to `decimal128` (`convert_decimals_in_arrow_table`), so fetches always yield `Decimal`. - `_use_arrow_native_timestamps=False`: genuinely returns Python `str` (Arrow `string`) — there is no re-cast on the Arrow path — while `cursor.description` still reports `'timestamp'`. The `timestampAsArrow=False` flag also wins over the always-sent `timestampAsString=false` conf. Doc-only change. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
6e9a401 to
0decb62
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — doc-only change and all claims verify against the code. I cross-checked each edited row: the kernel NotSupportedError for _tls_client_cert_key_password (client.py:1084), the Thrift-only ValueError for username/password (auth.py:109), the _disable_pandas honoring on kernel via inherited _convert_arrow_table, and the unconditional decimal re-cast in convert_decimals_in_arrow_table — all accurate. One low note: the diff changes four rows but the description only mentions two.
| | `max_download_threads` | `int` | ✅ | ❌ | `10` | Worker threads for cloud-fetch downloads. Not forwarded to the kernel. | | ||
| | `enable_query_result_lz4_compression` | `bool` | ✅ | ❌ | `True` | LZ4-compress result payloads. Not forwarded; the kernel handles compression internally. | | ||
| | `_disable_pandas` | `bool` | ✅ | ❌ | `False` | Skip the pandas-based Arrow deserialization path. Not forwarded to the kernel. | | ||
| | `_disable_pandas` | `bool` | ✅ | ✅ | `False` | Skip the pandas-based Arrow→row deserialization and materialize rows directly with PyArrow. This is a **Python-side** result-conversion toggle, not a wire option: the kernel returns results as Arrow (`RecordBatch`es) and the connector runs the *same* `_convert_arrow_table` for both backends, so the flag is honored on the kernel path too. Affects only row fetches (`fetchone`/`fetchmany`/`fetchall`); the `fetch*_arrow` methods return the Arrow table unchanged regardless of this flag. | |
There was a problem hiding this comment.
🔵 Low — Scope note: the PR title/description say "fix two incorrect Kernel claims," but the diff also rewrites the _disable_pandas row (❌→✅ on Kernel) and substantially expands the _use_arrow_native_decimals / _use_arrow_native_timestamps rows with new behavioral claims (unconditional decimal re-cast, timestamp string surfacing + cursor.description mismatch). I verified all of these against the code and they are correct — _disable_pandas is honored on the kernel path via the inherited _convert_arrow_table (result_set.py:97, reading connection.disable_pandas), and convert_decimals_in_arrow_table (utils.py:734) does re-cast to decimal128 regardless of the decimalAsArrow flag. The only issue is that these extra changes aren't mentioned in the PR summary, so a reviewer skimming the description could miss that four rows changed, not two. Consider updating the description to cover the results-rendering edits.
Summary
Two rows in
CONNECTION_PARAMETERS.mddisagreed with the code. Both are on the Kernel path. Doc-only change.1.
_tls_client_cert_key_password— was marked Kernel ✅, actually rejectedThe doc claimed this TLS option is honored on both backends. On the kernel path,
_kernel_tls_kwargs(src/databricks/sql/backend/kernel/client.py) raisesNotSupportedErrorwhen the param is set — the kernel has no surface for an encrypted client key today.2.
username/password— the "raisesValueError" note is Thrift-onlyThe
ValueErroris raised insideget_python_sql_connector_auth_provider(src/databricks/sql/auth/auth.py), which the kernel path never calls (session.pybuilds only a minimal PAT provider foruse_kernel=True). So on Kernel these params are silently ignored, not rejected. Updated the note to distinguish the two backends.Verification
Cross-checked every row in the doc (connection identity, auth, HTTP/retries, TLS, results/type rendering, session/transactions, telemetry) against the code. These two were the only discrepancies — all other defaults, per-backend ✅/❌/⚠️ flags, retry-knob forwarding, and telemetry behavior match.
This pull request and its description were written by Isaac.