Skip to content

Support cursor.rowcount for DML on the Thrift backend (#784)#847

Open
vikrantpuppala wants to merge 3 commits into
mainfrom
fix/784-dml-rowcount-thrift
Open

Support cursor.rowcount for DML on the Thrift backend (#784)#847
vikrantpuppala wants to merge 3 commits into
mainfrom
fix/784-dml-rowcount-thrift

Conversation

@vikrantpuppala

Copy link
Copy Markdown
Contributor

Summary

cursor.rowcount was hardcoded to -1 and never updated for the Thrift backend (the default), so applications had no way to learn how many rows a DML statement affected. This was tracked in #784.

For DML (INSERT/UPDATE/DELETE/MERGE), the Databricks Thrift server reports the affected-row count in TGetOperationStatusResp.numModifiedRows, but the connector discarded it — _wait_until_command_done kept only operationState and threw the rest of the status response away.

This PR wires that value through to cursor.rowcount, bringing the Thrift path in line with the kernel backend (which already surfaces num_modified_rows, per the 4.3.0 changelog).

What changed

  • _wait_until_command_done now returns the terminal status response in addition to the state, so numModifiedRows is preserved (from either the direct-results operationStatus or the final poll response).
  • _handle_execute_response reads numModifiedRows and passes it into _results_message_to_execute_response.
  • ExecuteResponse and ResultSet carry a new optional num_modified_rows field; ThriftResultSet threads it through.
  • Cursor.execute sets self.rowcount from active_result_set.num_modified_rows when present.
  • rowcount resets to -1 before each statement (in _close_and_clear_active_result_set) so a prior DML's count never leaks into a subsequent SELECT.

Behavior

  • DML → rowcount = affected-row count.
  • SELECT and any statement the server doesn't report a count for → rowcount stays -1 (unchanged).
  • Kernel backend is unaffected: it already sets rowcount inside its own execute_command; the client.py read is None-guarded so the two coexist.
  • The DOUBLE TOperationHandle.modifiedRowCount field was intentionally not used — the reliable, server-populated signal for Databricks is the I64 numModifiedRows on the operation status.

Test plan

  • New unit tests in tests/unit/test_client.py:
    • test_rowcount_reports_num_modified_rows_for_dml — DML sets rowcount to the affected count.
    • test_rowcount_stays_default_for_selectSELECT leaves rowcount at -1.
    • test_rowcount_resets_between_statements — a DML count does not leak into a later SELECT on the same cursor.
  • New unit tests in tests/unit/test_thrift_backend.py:
    • test_handle_execute_response_reads_num_modified_rows_from_direct_resultsnumModifiedRows reaches ExecuteResponse.num_modified_rows.
    • test_handle_execute_response_num_modified_rows_none_for_selectNone stays None.
  • Updated test_handle_execute_response_can_handle_with_direct_results and test_handle_execute_response_sets_active_op_handle for the new _wait_until_command_done return shape / _results_message_to_execute_response signature.
  • Verified the new client tests fail without the fix (rowcount == -1) and pass with it.
  • Full unit suite: 872 passed, 4 skipped (plus realkernel set skipped — requires the real kernel wheel). black --check clean on all changed files.

Closes #784

@gopalldb

gopalldb commented Jul 8, 2026

Copy link
Copy Markdown

📄 src/databricks/sql/client.py

⚠️ [MAJOR] executemany rowcount reflects only the last parameter set, not the total across all statements
Line 1523 | bug | logical_claude_agent

▎ executemany reports only the last statement's rowcount instead of the aggregate. executemany (client.py:1522-1523) loops over seq_of_parameters calling execute() once per set. Each execute() resets self.rowcount to -1 via _close_and_clear_active_result_set() (client.py:1047) and then sets it from that single statement's num_modified_rows (client.py:1380-1382). Now that this PR wires rowcount for DML, executemany over N DML parameter sets leaves rowcount equal to only the final set's affected-row count — not the sum across all N. PEP 249 specifies rowcount after executemany should reflect the total rows affected across all operations (or -1 when undeterminable). Before this PR rowcount was always -1, so no caller could be misled; after it, callers summing DML impact through executemany silently get an undercount.

is_staging_operation=t_result_set_metadata_resp.isStagingOperation,
arrow_schema_bytes=schema_bytes,
result_format=t_result_set_metadata_resp.resultFormat,
num_modified_rows=num_modified_rows,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice fix

@vikrantpuppala

Copy link
Copy Markdown
Contributor Author

Good catch on the executemany aggregation — addressed in d21c0df.

executemany now accumulates the affected-row count across all parameter sets rather than reporting only the final one, per PEP 249. If no statement reports a count (all SELECT, or the server reports none), rowcount stays at its -1 default.

Covered by new unit tests (sum / mixed reported+unreported / all-unreported) and the live e2e test (TestPySQLCoreSuite::test_dml_rowcount) now asserts executemany of 3 INSERTs yields rowcount == 3 against a real warehouse.

cursor.rowcount was hardcoded to -1 and never updated for the Thrift
backend (the default). For DML (INSERT/UPDATE/DELETE/MERGE) the
Databricks Thrift server reports the affected-row count in
TGetOperationStatusResp.numModifiedRows, but the connector discarded it —
_wait_until_command_done kept only operationState.

Thread numModifiedRows through: _wait_until_command_done now returns the
terminal status response, _handle_execute_response reads numModifiedRows
from it, ExecuteResponse and ResultSet carry a num_modified_rows field,
and Cursor.execute sets self.rowcount from it. rowcount resets to -1
before each statement so a DML count never leaks into a later SELECT.

SELECT (and statements the server does not report a count for) leave
rowcount at its -1 default. This brings the Thrift path in line with the
kernel backend, which already surfaces num_modified_rows.

Closes #784

Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
End-to-end test in TestPySQLCoreSuite exercising INSERT/UPDATE/DELETE on
a real Thrift warehouse and asserting cursor.rowcount reports the exact
affected-row count (3/2/1), then that a following SELECT resets it to -1.
Verified live on dogfood; fails with the fix reverted (rowcount stays -1).

Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Address PR review: executemany looped execute() per parameter set and,
since each execute() resets rowcount, left rowcount equal to only the
final set's affected-row count. Per PEP 249, rowcount after executemany
should reflect the total across all operations. Accumulate the reported
per-statement counts; if no statement reports a count (all SELECT / the
server reports none), rowcount stays at its -1 default.

Adds unit tests for the sum, mixed reported/unreported, and all-unreported
cases, and extends the live e2e test to assert executemany aggregation.

Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] Support rowcount for DML operation

2 participants