fix(backend/kernel): comparator parity — async retention + intervals_as_string + precision/scale + named params + utils.exc fix#803
Conversation
291e18e to
312164a
Compare
312164a to
81f065f
Compare
81f065f to
932ec74
Compare
932ec74 to
73760c7
Compare
|
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) |
73760c7 to
ecd0d37
Compare
|
Thanks for the review. Summary of what's addressed in the new push ( Item 1 — fixedMissing decimal precision/scale unit test ✅
The existing
Items 2, 3, 4, 5 — accepted as documentedPer your own punt signals, leaving these as-is for follow-up:
Signed off the amended commit. |
…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>
ecd0d37 to
7adb049
Compare
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_commandclosed the parentStatementinfinallyregardless ofasync_op. The kernel'sStatement.close()invalidates child handles (seedatabricks-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_statementsdict alongside_async_handles, and close it fromclose_command,close_session, andget_execution_resultafter the executed handle is done.Comparator outcome: STATEMENT_ASYNC suite 3/3 match (was 0/3).
2.
intervals_as_stringwire-through (companion to kernel PR #64)pyarrow's Python bindings cannot decode Arrow's
month_intervaltype at all (id 21 — raisesKeyErrorfrom.as_py,to_pylist,cast(string),to_pandas). Every kernel-backendSELECT *over any table with anINTERVAL YEAR TO MONTHcolumn threwArrowNotImplementedError— 32 / 88 audit diffs.Fix: pass
intervals_as_string=Trueto the kernelSession(...)constructor unconditionally. The kernel post-processor then stringifiesInterval/Durationcolumns server-side toUtf8(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_schemahard-codedNonefor slots 4 and 5 of the PEP 249 description tuple. For DECIMAL columns the Arrow schema carriesprecision/scaleonDecimal128Type, 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)fromDECIMAL(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 fromTime64Type) 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_paramspreviously raisedNotSupportedErrorfor anyTSparkParameterwithnameset. The canonical SEA proto marksStatementParameter.nameasopenapi_required=true— named binding is the spec-required public form, andordinalisPUBLIC_UNDOCUMENTED. Kernel PR #65 added aStatement.bind_named_paramPyO3 API mirroringbind_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.mddrops to 0 diffs.5.
utils.ParamEscaperexc.ProgrammingErrorimport fix (was PR #802)ParamEscaper.escape_argsandParamEscaper.escape_itemboth raisedexc.ProgrammingError(...)(lines 551 and 609 ofutils.py), butexcwas never imported. On any unsupported parameter shape the user sawNameError: name 'exc' is not definedinstead of a clean PEP 249ProgrammingError.Surfaced via the python-comparator audit harness running the
INLINE_PARAMSconnection config: both backends raisedNameError, 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
ProgrammingErrordirectly fromdatabricks.sql.exc(matching the pattern used insession.py,client.py,result_set.py, etc.) and replace the twoexc.ProgrammingError(...)sites with bareProgrammingError(...).Dependencies
Items #2, #3, and #4 require the matching
databricks-sql-kernelchanges:intervals_as_stringpost-processor + empty-result schema fix + composite type parser + TIMESTAMP tz + metadata reshape fixesStatementParameter.name+bind_named_paramAPI (to be opened)For local testing the comparator harness uses
KERNEL_FREEZE=1against a kernel checkout of the feature branches.Headline comparator results (full thrift-vs-kernel run)
decimal_columnprecision/scaleRemaining 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
cur.execute_async("SELECT 1"); while cur.is_query_pending(): time.sleep(0.2); cur.get_async_execution_result(); cur.fetchall()succeeds onuse_kernel=Truecur.execute("SELECT ym_interval_column FROM ...")returns string-shaped rows matching the Thrift surfacecur.descriptionfor aDECIMAL(10,2)column now reportsprecision=10, scale=2on both backendstest_parameterized_query_named_params/test_parameterized_query_named_param_with_null(added) pass against pecotesting warehousebind_tspark_paramsnamed/positional/mixed routing (tests/unit/test_kernel_type_mapping.py)ParamEscaper().escape_args(object())raisesProgrammingError, notNameErrorThis pull request and its description were written by Isaac.