| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Minor Missing unit test for the new decimal precision/scale extraction (tests/unit/test_kernel_type_mapping.py) _async_statements[guid] = stmt overwrite leaks on duplicate statement_id (client.py:305) cancel_command does not release the parent Statement (client.py:333-346) intervals_as_string=True is hard-coded with no opt-out (client.py:188) Stale-wheel errors for intervals_as_string / bind_named_param surface as generic OperationalError("Unexpected error from databricks_sql_kernel: ...") (client.py:172-192, type_mapping.py:235) |
Sorry, something went wrong.
|
Thanks for the review. Summary of what's addressed in the new push (ecd0d37b): Item 1 — fixedMissing decimal precision/scale unit test ✅
The existing test_description_from_schema_preserves_field_names_and_order stays — its d[2:] == (None,)*5 assertion is still true for the non-decimal schema it uses; the new tests cover the decimal case the existing one didn't reach. pytest tests/unit/test_kernel_type_mapping.py: 48 / 48 pass (was 43; +5 new). Items 2, 3, 4, 5 — accepted as documentedPer your own punt signals, leaving these as-is for follow-up:
Signed off the amended commit. |
Sorry, something went wrong.
…as_string + precision/scale + named params + utils.exc fix Five small comparator-parity fixes surfaced by the same audit run. All are independently small but ship together because they share the kernel-backend client surface and the audit baseline. ### 1. Async Statement retention `KernelDatabricksClient.execute_command` closed the parent `Statement` in `finally` regardless of `async_op`. The kernel's `Statement.close()` invalidates child handles (see databricks-sql-kernel `src/statement/validity.rs`), so the async handle died before the user could poll it — breaking `execute_async` → `is_query_pending` → `get_async_execution_result`. Fix: when `async_op=True`, retain the parent Statement in a new `_async_statements` dict alongside `_async_handles`, and close it from `close_command`, `close_session`, and `get_execution_result` after the executed handle is done. Comparator outcome: STATEMENT_ASYNC suite 3/3 match (was 0/3). ### 2. `intervals_as_string` wire-through pyarrow's Python bindings cannot decode Arrow's `month_interval` type (id 21 — `KeyError` from `.as_py`, `to_pylist`, `cast(string)`, `to_pandas`). Every kernel-backend `SELECT *` over a table with an `INTERVAL YEAR TO MONTH` column raised `ArrowNotImplementedError` — 32 / 88 audit diffs. Fix: pass `intervals_as_string=True` to the kernel `Session(...)` constructor unconditionally. The kernel post-processor stringifies `Interval` / `Duration` columns server-side to `Utf8` (kernel PR #64). Comparator outcome: bucket A (ArrowNotImplementedError) 32 → 0. ### 3. Decimal precision/scale extraction `description_from_arrow_schema` hard-coded `None` for PEP 249 description-tuple slots 4 and 5. For DECIMAL columns the Arrow schema carries `precision` / `scale` on `Decimal128Type`, but they were silently dropped — 88 cell diffs (44 precision + 44 scale). Fix: factor out `_precision_scale_for_arrow_type(arrow_type)` and call it from the description builder. Today it only handles decimals; future extensions slot in here. Comparator outcome: 88 precision+scale diffs → 0. ### 4. Named-parameter binding `bind_tspark_params` raised `NotSupportedError` for any `TSparkParameter` with `name` set. The canonical SEA proto marks `StatementParameter.name` as `openapi_required=true` (named is the spec-required public form; `ordinal` is `PUBLIC_UNDOCUMENTED`). Kernel PR #65 added a `Statement.bind_named_param` PyO3 API. Fix: route named bindings via the new API. Positional ordinals self-increment so a named entry in the middle of the list doesn't claim a positional slot. Comparator outcome: PREPARED_STATEMENT_NAMED 1/1 match (was 0/1). Full thrift-vs-kernel run: 97/132 match (was 96/132). ### 5. `utils.ParamEscaper` `exc.ProgrammingError` import fix `ParamEscaper.escape_args` and `escape_item` both raised `exc.ProgrammingError(...)`, but `exc` was never imported. On any unsupported parameter shape the user saw `NameError: name 'exc' is not defined` instead of a clean PEP 249 `ProgrammingError`. Surfaced via the same audit harness running INLINE_PARAMS: both backends raised `NameError`, which the comparator's class+message match treated as parity — a false-positive that hid both the driver bug and the underlying caller-side type mismatch. Fix: import `ProgrammingError` directly from `databricks.sql.exc` (matching the pattern used in `session.py`, `client.py`, `result_set.py`, etc.) and replace the two `exc.ProgrammingError(...)` sites with bare `ProgrammingError(...)`. ## Dependencies Items #2, #3, and #4 require the matching databricks-sql-kernel changes: PR #64 (`intervals_as_string` + empty-result schema fix) and PR #65 (named-param binding). For local testing the comparator harness uses `KERNEL_FREEZE=1` against a kernel checkout of the feature branches. ## Headline comparator results (full thrift-vs-kernel run) | | Before | After | |------------------------------------|--------|-------| | match / diff / skipped | 60 / 88 / 0 | **97 / 34 / 1** | | Bucket A (ArrowNotImplementedError)| 32 | 0 | | `decimal_column` precision/scale | 88 | 0 | | Bucket B1 (named params) | 1 | 0 | | Suites fully clean | 12 / 30 | **17 / 30** | Remaining 34 diffs cluster into documented causes (complex types in `fetchall_arrow`, timestamp tz on Arrow path, VOID surface, METADATA pattern semantics) — tracked in `~/docs/python-kernel/comparator-diff-tasklist.md`. ## Test plan - [x] Manual repro of async path: `cur.execute_async("SELECT 1")` → `is_query_pending` → `get_async_execution_result` → `fetchall` succeeds on `use_kernel=True` - [x] Manual repro of interval path: `cur.execute("SELECT ym_interval_column FROM ...")` returns string-shaped rows matching the Thrift surface - [x] Manual repro of decimal path: `cur.description` for a `DECIMAL(10,2)` column now reports `precision=10, scale=2` - [x] Live e2e: `test_parameterized_query_named_params` / `test_parameterized_query_named_param_with_null` (added) - [x] Unit tests: `bind_tspark_params` named/positional/mixed routing (`tests/unit/test_kernel_type_mapping.py`) - [x] Manual repro of utils fix: `ParamEscaper().escape_args(object())` raises `ProgrammingError`, not `NameError` - [x] Full thrift-vs-kernel comparator: 97 / 34 / 1 of 132 - [x] Existing connector unit suite: 752 passed Co-authored-by: Isaac Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
| Back | FazBrowse Home | New Git URL |
Summary
Five comparator-parity fixes bundled because they share the same audit baseline and the same kernel-backend client surface.
1. Async Statement retention
KernelDatabricksClient.execute_command closed the parent Statement in finally regardless of async_op. The kernel's Statement.close() invalidates child handles (see databricks-sql-kernel/src/statement/validity.rs), so the async handle was being killed before the user could poll it — breaking the entire async surface (execute_async → is_query_pending → get_async_execution_result).
Fix: when async_op=True, retain the parent Statement in a new _async_statements dict alongside _async_handles, and close it from close_command, close_session, and get_execution_result after the executed handle is done.
Comparator outcome: STATEMENT_ASYNC suite 3/3 match (was 0/3).
2. intervals_as_string wire-through (companion to kernel PR #64)
pyarrow's Python bindings cannot decode Arrow's month_interval type at all (id 21 — raises KeyError from .as_py, to_pylist, cast(string), to_pandas). Every kernel-backend SELECT * over any table with an INTERVAL YEAR TO MONTH column threw ArrowNotImplementedError — 32 / 88 audit diffs.
Fix: pass intervals_as_string=True to the kernel Session(...) constructor unconditionally. The kernel post-processor then stringifies Interval / Duration columns server-side to Utf8 (see kernel PR #64), so pyarrow never sees the unreadable type.
Comparator outcome: bucket A (ArrowNotImplementedError) drops from 32 → 0 diffs.
3. Decimal precision/scale extraction
description_from_arrow_schema hard-coded None for slots 4 and 5 of the PEP 249 description tuple. For DECIMAL columns the Arrow schema carries precision / scale on Decimal128Type, but we were silently dropping them. The kernel backend returned:
while Thrift returned:
Any consumer that reads PEP 249 slots 4/5 (SQLAlchemy, pandas-read-sql, etc.) can't distinguish DECIMAL(10,2) from DECIMAL(38,18) on the kernel backend. 88 comparator diffs (44 precision + 44 scale).
Fix: factor out _precision_scale_for_arrow_type(arrow_type) and call it from the description builder. Today it only handles decimals; future extensions (e.g. fractional-second precision from Time64Type) slot in here without touching the description builder.
Comparator outcome: 88 precision+scale diffs → 0.
4. Named-parameter binding (companion to kernel PR #65)
bind_tspark_params previously raised NotSupportedError for any TSparkParameter with name set. The canonical SEA proto marks StatementParameter.name as openapi_required=true — named binding is the spec-required public form, and ordinal is PUBLIC_UNDOCUMENTED. Kernel PR #65 added a Statement.bind_named_param PyO3 API mirroring bind_param.
Fix: route named bindings via the new kernel API. Positional ordinals self-increment so a named entry in the middle of the list doesn't claim a positional slot.
Comparator outcome: PREPARED_STATEMENT_NAMED 1/1 match (was 0/1). Bucket B1 in comparator-diff-tasklist.md drops to 0 diffs.
5. utils.ParamEscaper exc.ProgrammingError import fix (was PR #802)
ParamEscaper.escape_args and ParamEscaper.escape_item both raised exc.ProgrammingError(...) (lines 551 and 609 of utils.py), but exc was never imported. On any unsupported parameter shape the user saw NameError: name 'exc' is not defined instead of a clean PEP 249 ProgrammingError.
Surfaced via the python-comparator audit harness running the INLINE_PARAMS connection config: both backends raised NameError, which the comparator's class+message match treated as parity — a false-positive that hid both the real driver bug and the underlying caller-side type mismatch.
Fix: import ProgrammingError directly from databricks.sql.exc (matching the pattern used in session.py, client.py, result_set.py, etc.) and replace the two exc.ProgrammingError(...) sites with bare ProgrammingError(...).
Dependencies
Items #2, #3, and #4 require the matching databricks-sql-kernel changes:
For local testing the comparator harness uses KERNEL_FREEZE=1 against a kernel checkout of the feature branches.
Headline comparator results (full thrift-vs-kernel run)
Remaining 34 diffs cluster into documented causes (complex types in fetchall_arrow, timestamp tz on Arrow path, VOID surface, METADATA pattern semantics, empty-result nullability) — tracked in ~/docs/python-kernel/comparator-diff-tasklist.md.
Test plan
This pull request and its description were written by Isaac.