Return real row counts from cursor.rowcount for CHANGED_ROWS statements - #578
Open
colin-k-rogers wants to merge 3 commits into
Open
Conversation
DuckDBPyConnection::GetRowcount() has unconditionally returned -1 since PR #8911 in duckdb/duckdb added the DB-API rowcount attribute as a stub. For statements whose StatementReturnType is CHANGED_ROWS (INSERT/UPDATE/ DELETE/CREATE TABLE AS/MERGE), the result already contains a single-row, single-column value with the affected-row count - the same value the C API's duckdb_rows_changed() reads. Plumb that value through DuckDBPyResult, DuckDBPyRelation, and DuckDBPyConnection so rowcount reports it for real, while staying -1 for SELECT and other statements without a known count. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…erialize Two issues from review: - rowcount was computed lazily on first access by peeking at the live QueryResult, but DuckDBPyRelation::FetchAll/FetchDF/etc. null out `result` once fully consumed, so `.rowcount` read after `.fetchall()` (a common pattern for DML statements) incorrectly returned -1. Fixed by computing the CHANGED_ROWS value eagerly at DuckDBPyResult construction time and caching it on DuckDBPyRelation itself, which outlives the Fetch*() calls that discard the underlying result. - Materialize() was being called while holding the GIL, which could block unrelated Python threads for the duration of a long-running statement. Now released around it, matching the pattern already used by Fetchone() and friends. Also adds regression tests for rowcount after fetchall/fetchone/ fetchmany/fetchdf/fetchnumpy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… tests
ExecuteOrThrow() only runs for relations backed by a lazy Relation
object (rel != nullptr), and RunQuery() never builds one of those for
a CHANGED_ROWS statement - it only does so for SELECT_STATEMENT, and
falls back to nullptr (duckdb.sql("INSERT ...") returns None) for
everything else. So the row_changes assignment there could never
observe anything but the default -1; remove it rather than carry
untestable dead code.
Also adds two tests: one pinning executemany()'s current rowcount
behavior (reflects only the last parameter set's statement, not the
total across all of them - a known limitation, not fixed here), and
one confirming rowcount survives to_arrow_table() the same way it
already does for fetchall/fetchone/fetchmany/fetchdf/fetchnumpy.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DuckDBPyConnection.rowcounthas unconditionally returned-1since #8911 in duckdb/duckdb (2023) added the DB-APIrowcountattribute as a stub. This implements it for real.For statements whose
StatementReturnTypeisCHANGED_ROWS(INSERT/UPDATE/DELETE/CREATE TABLE AS/MERGE), DuckDB already computes a single-row/single-column result containing the affected-row count — this is the same value the C API'sduckdb_rows_changed()reads. This PR plumbs that value through tocursor.rowcount:DuckDBPyResult::ComputeRowChanges()reads the count eagerly at construction time (materializing a streaming result first if necessary — cheap, since CHANGED_ROWS results are always exactly one already-computed row), and releases the GIL aroundMaterialize()to match the existingFetchone()pattern.DuckDBPyRelationitself (not justDuckDBPyResult), since severalFetch*()methods (FetchAll,FetchDF,FetchNumpy,to_arrow_table, ...) null out the underlyingDuckDBPyResultonce fully consumed. Caching one level up means.rowcountcorrectly survives being read after a fetch, not just immediately afterexecute().DuckDBPyConnection::GetRowcount()just reads that cached value.SELECT(or anything without a known count), it correctly stays-1, matching the DB-API 2.0 spec (rowcountis-1when unknown) and mirroring e.g.sqlite3's own behavior for SELECTs.Related:
Known limitation (not fixed here)
executemany()reports the last statement's count rather than a sum across all parameter sets, since it only keeps the final iteration'sQueryResult. Pinned with a test (test_rowcount_executemany_reflects_last_statement_only) documenting current behavior rather than silently leaving it untested. Happy to follow up separately if a summed total is wanted.Test plan
tests/fast/api/test_dbapi10.py::TestCursorRowcountcovering INSERT/UPDATE/DELETE/CREATE TABLE AS, SELECT staying-1, rowcount survivingfetchall/fetchone/fetchmany/fetchdf/fetchnumpy/to_arrow_table, and theexecutemanylimitation above.tests/fast/api/suite locally: 374 passed, 0 failures attributable to this change (2 pre-existing failures unrelated to this change: a timezone/tzinfo conversion test and a query-progress test, neither touching the files this PR modifies).clang-formatandruffclean on the modified files.🤖 Generated with Claude Code