From f71734ee09fd0f2199d96e762d95b221be0609c6 Mon Sep 17 00:00:00 2001 From: Colin Rogers <111200756+colin-k-rogers@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:50:27 -0700 Subject: [PATCH 1/3] Return real row counts from cursor.rowcount for CHANGED_ROWS statements 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 --- .../pyconnection/pyconnection.hpp | 2 +- src/include/duckdb_python/pyrelation.hpp | 2 + src/include/duckdb_python/pyresult.hpp | 4 ++ src/pyconnection.cpp | 9 +++- src/pyrelation.cpp | 7 +++ src/pyresult.cpp | 30 +++++++++++ tests/fast/api/test_dbapi10.py | 52 +++++++++++++++++++ 7 files changed, 103 insertions(+), 3 deletions(-) diff --git a/src/include/duckdb_python/pyconnection/pyconnection.hpp b/src/include/duckdb_python/pyconnection/pyconnection.hpp index 638b0a4b..e5bcf7a3 100644 --- a/src/include/duckdb_python/pyconnection/pyconnection.hpp +++ b/src/include/duckdb_python/pyconnection/pyconnection.hpp @@ -321,7 +321,7 @@ struct DuckDBPyConnection : public std::enable_shared_from_this GetDescription(); - int GetRowcount(); + int64_t GetRowcount(); // these should be functions on the result but well Optional FetchOne(); diff --git a/src/include/duckdb_python/pyrelation.hpp b/src/include/duckdb_python/pyrelation.hpp index f71a6327..b86ba2b7 100644 --- a/src/include/duckdb_python/pyrelation.hpp +++ b/src/include/duckdb_python/pyrelation.hpp @@ -30,6 +30,8 @@ struct DuckDBPyRelation { nb::list Description(); + int64_t GetRowcount(); + void Close(); std::unique_ptr GetAttribute(const string &name); diff --git a/src/include/duckdb_python/pyresult.hpp b/src/include/duckdb_python/pyresult.hpp index 865f955f..eff850fc 100644 --- a/src/include/duckdb_python/pyresult.hpp +++ b/src/include/duckdb_python/pyresult.hpp @@ -59,6 +59,10 @@ struct DuckDBPyResult { ClientProperties GetClientProperties(); + //! Number of rows changed by the last CHANGED_ROWS-returning statement (INSERT/UPDATE/DELETE/...). + //! Returns -1 when not applicable/unknown, as permitted by the DB-API 2.0 spec for 'rowcount'. + int64_t GetRowcount(); + private: void FillNumpy(nb::dict &res, idx_t col_idx, NumpyResultConversion &conversion, const char *name); diff --git a/src/pyconnection.cpp b/src/pyconnection.cpp index ebdf25fa..489379cd 100644 --- a/src/pyconnection.cpp +++ b/src/pyconnection.cpp @@ -1920,8 +1920,13 @@ Optional DuckDBPyConnection::GetDescription() { return result.Description(); } -int DuckDBPyConnection::GetRowcount() { - return -1; +int64_t DuckDBPyConnection::GetRowcount() { + ConnectionLockGuard conn_lock(*this); + if (!con.HasResult()) { + return -1; + } + auto &result = con.GetResult(); + return result.GetRowcount(); } void DuckDBPyConnection::Close() { diff --git a/src/pyrelation.cpp b/src/pyrelation.cpp index 2d1ac47b..d17725dd 100644 --- a/src/pyrelation.cpp +++ b/src/pyrelation.cpp @@ -271,6 +271,13 @@ nb::list DuckDBPyRelation::Description() { return DuckDBPyResult::GetDescription(names, types); } +int64_t DuckDBPyRelation::GetRowcount() { + if (!result) { + return -1; + } + return result->GetRowcount(); +} + Relation &DuckDBPyRelation::GetRel() { if (!rel) { throw InternalException("DuckDBPyRelation - calling GetRel, but no rel was present"); diff --git a/src/pyresult.cpp b/src/pyresult.cpp index 7f0d0c9a..859c1f3b 100644 --- a/src/pyresult.cpp +++ b/src/pyresult.cpp @@ -68,6 +68,36 @@ const vector &DuckDBPyResult::GetTypes() { return result->types; } +int64_t DuckDBPyResult::GetRowcount() { + if (!result || result->HasError()) { + return -1; + } + if (result->properties.return_type != StatementReturnType::CHANGED_ROWS) { + // The row count of a SELECT (or a statement that returns nothing) is not known without fully + // consuming the result - report -1, as permitted by the DB-API 2.0 spec for 'rowcount'. + return -1; + } + if (result->type == QueryResultType::STREAM_RESULT) { + // CHANGED_ROWS statements always produce a single already-computed row, so materializing here + // does not trigger any additional query execution - it just changes the in-memory representation. + auto &stream_result = result->Cast(); + auto materialized = stream_result.Materialize(); + if (!materialized || materialized->HasError()) { + return -1; + } + result = std::move(materialized); + } + if (result->type != QueryResultType::MATERIALIZED_RESULT) { + // e.g. an Arrow result - can't peek at the value without disturbing it. + return -1; + } + auto &materialized_result = result->Cast(); + if (materialized_result.RowCount() != 1 || materialized_result.ColumnCount() != 1) { + return -1; + } + return materialized_result.GetValue(0, 0).GetValue(); +} + unique_ptr DuckDBPyResult::FetchChunk() { if (!result) { throw InternalException("FetchChunk called without a result object"); diff --git a/tests/fast/api/test_dbapi10.py b/tests/fast/api/test_dbapi10.py index 6d60b27c..829f3974 100644 --- a/tests/fast/api/test_dbapi10.py +++ b/tests/fast/api/test_dbapi10.py @@ -56,3 +56,55 @@ def test_none_description(self, duckdb_empty_cursor): class TestCursorRowcount: def test_rowcount(self, duckdb_cursor): assert duckdb_cursor.rowcount == -1 + + def test_rowcount_no_query_yet(self, duckdb_cursor): + assert duckdb_cursor.rowcount == -1 + + def test_rowcount_insert(self, duckdb_cursor): + duckdb_cursor.execute("CREATE TABLE t (i INTEGER)") + duckdb_cursor.execute("INSERT INTO t VALUES (1), (2), (3)") + assert duckdb_cursor.rowcount == 3 + + def test_rowcount_insert_select(self, duckdb_cursor): + duckdb_cursor.execute("CREATE TABLE t (i INTEGER)") + duckdb_cursor.execute("INSERT INTO t SELECT * FROM range(100)") + assert duckdb_cursor.rowcount == 100 + + def test_rowcount_update(self, duckdb_cursor): + duckdb_cursor.execute("CREATE TABLE t AS SELECT * FROM range(10) t(i)") + duckdb_cursor.execute("UPDATE t SET i = i + 1 WHERE i < 4") + assert duckdb_cursor.rowcount == 4 + + def test_rowcount_delete(self, duckdb_cursor): + duckdb_cursor.execute("CREATE TABLE t AS SELECT * FROM range(10) t(i)") + duckdb_cursor.execute("DELETE FROM t WHERE i < 3") + assert duckdb_cursor.rowcount == 3 + + def test_rowcount_create_table_as(self, duckdb_cursor): + duckdb_cursor.execute("CREATE TABLE t AS SELECT * FROM range(42) t(i)") + assert duckdb_cursor.rowcount == 42 + + def test_rowcount_select_is_unknown(self, duckdb_cursor): + # Matches DB-API 2.0: rowcount is -1 when it can't be determined without consuming the result. + duckdb_cursor.execute("SELECT * FROM range(10)") + assert duckdb_cursor.rowcount == -1 + duckdb_cursor.fetchall() + assert duckdb_cursor.rowcount == -1 + + def test_rowcount_reset_by_next_execute(self, duckdb_cursor): + duckdb_cursor.execute("CREATE TABLE t (i INTEGER)") + duckdb_cursor.execute("INSERT INTO t VALUES (1), (2)") + assert duckdb_cursor.rowcount == 2 + duckdb_cursor.execute("SELECT * FROM t") + assert duckdb_cursor.rowcount == -1 + + def test_rowcount_does_not_disturb_fetch(self, duckdb_cursor): + # Accessing rowcount must not consume the single-row result that fetchone() also reads. + duckdb_cursor.execute("CREATE TABLE t (i INTEGER)") + duckdb_cursor.execute("INSERT INTO t VALUES (1), (2), (3)") + assert duckdb_cursor.rowcount == 3 + assert duckdb_cursor.fetchone() == (3,) + + def test_rowcount_ddl_is_unknown(self, duckdb_cursor): + duckdb_cursor.execute("CREATE TABLE t (i INTEGER)") + assert duckdb_cursor.rowcount == -1 From bb6bd9854cbc22aaf79164a03c1e578b43a79040 Mon Sep 17 00:00:00 2001 From: Colin Rogers <111200756+colin-k-rogers@users.noreply.github.com> Date: Fri, 31 Jul 2026 20:08:13 -0700 Subject: [PATCH 2/3] Fix rowcount not surviving result consumption; release GIL during materialize 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 --- src/include/duckdb_python/pyrelation.hpp | 8 +++++- src/include/duckdb_python/pyresult.hpp | 12 ++++++++- src/pyrelation.cpp | 9 ++----- src/pyresult.cpp | 16 +++++++++--- tests/fast/api/test_dbapi10.py | 32 ++++++++++++++++++++++++ 5 files changed, 65 insertions(+), 12 deletions(-) diff --git a/src/include/duckdb_python/pyrelation.hpp b/src/include/duckdb_python/pyrelation.hpp index b86ba2b7..aebdf08b 100644 --- a/src/include/duckdb_python/pyrelation.hpp +++ b/src/include/duckdb_python/pyrelation.hpp @@ -30,7 +30,11 @@ struct DuckDBPyRelation { nb::list Description(); - int64_t GetRowcount(); + //! Cached at execution time (see ExecuteOrThrow / the DuckDBPyResult constructor overload) so that + //! it survives the Fetch*() methods below, several of which null out `result` once fully consumed. + int64_t GetRowcount() const { + return row_changes; + } void Close(); @@ -302,6 +306,8 @@ struct DuckDBPyRelation { vector names; std::shared_ptr result; std::string rendered_result; + //! Cached row-changed count - see GetRowcount(). + int64_t row_changes = -1; }; } // namespace duckdb diff --git a/src/include/duckdb_python/pyresult.hpp b/src/include/duckdb_python/pyresult.hpp index eff850fc..16a3d69a 100644 --- a/src/include/duckdb_python/pyresult.hpp +++ b/src/include/duckdb_python/pyresult.hpp @@ -61,7 +61,9 @@ struct DuckDBPyResult { //! Number of rows changed by the last CHANGED_ROWS-returning statement (INSERT/UPDATE/DELETE/...). //! Returns -1 when not applicable/unknown, as permitted by the DB-API 2.0 spec for 'rowcount'. - int64_t GetRowcount(); + int64_t GetRowcount() const { + return row_changes; + } private: void FillNumpy(nb::dict &res, idx_t col_idx, NumpyResultConversion &conversion, const char *name); @@ -86,6 +88,12 @@ struct DuckDBPyResult { duckdb::pyarrow::Table MaterializedResultToArrowTable(const ArrowSchema &arrow_schema, idx_t rows_per_batch); ArrowArrayStream FetchArrowArrayStream(idx_t rows_per_batch); + //! Computes the CHANGED_ROWS value (if any) up front, before any Fetch call has had a chance to + //! consume it, so that later fetch calls do not affect what GetRowcount() reports. Materializes a + //! streaming result if necessary; this is cheap since CHANGED_ROWS results are always exactly one + //! already-computed row. + int64_t ComputeRowChanges(); + private: idx_t chunk_offset = 0; @@ -96,6 +104,8 @@ struct DuckDBPyResult { // Holds the categorical type of Categorical/ENUM types unordered_map categories_type; bool result_closed = false; + //! Cached by ComputeRowChanges() at construction time - see GetRowcount(). + int64_t row_changes = -1; }; } // namespace duckdb diff --git a/src/pyrelation.cpp b/src/pyrelation.cpp index d17725dd..06cab6fb 100644 --- a/src/pyrelation.cpp +++ b/src/pyrelation.cpp @@ -71,6 +71,7 @@ DuckDBPyRelation::DuckDBPyRelation(std::shared_ptr result_p) this->executed = true; this->types = result->GetTypes(); this->names = result->GetNames(); + this->row_changes = result->GetRowcount(); } std::unique_ptr DuckDBPyRelation::ProjectFromExpression(const string &expression) { @@ -271,13 +272,6 @@ nb::list DuckDBPyRelation::Description() { return DuckDBPyResult::GetDescription(names, types); } -int64_t DuckDBPyRelation::GetRowcount() { - if (!result) { - return -1; - } - return result->GetRowcount(); -} - Relation &DuckDBPyRelation::GetRel() { if (!rel) { throw InternalException("DuckDBPyRelation - calling GetRel, but no rel was present"); @@ -841,6 +835,7 @@ void DuckDBPyRelation::ExecuteOrThrow(bool stream_result) { query_result->ThrowError(); } result = std::make_unique(std::move(query_result)); + row_changes = result->GetRowcount(); } PandasDataFrame DuckDBPyRelation::FetchDF(bool date_as_object) { diff --git a/src/pyresult.cpp b/src/pyresult.cpp index 859c1f3b..db3d6c0b 100644 --- a/src/pyresult.cpp +++ b/src/pyresult.cpp @@ -33,6 +33,9 @@ DuckDBPyResult::DuckDBPyResult(unique_ptr result_p) : result(std::m if (!result) { throw InternalException("PyResult created without a result object"); } + // Must happen before any Fetch call (from this object or a caller holding a reference to the + // same underlying QueryResult) has a chance to consume the single row we're reading here. + row_changes = ComputeRowChanges(); } DuckDBPyResult::~DuckDBPyResult() { @@ -68,7 +71,7 @@ const vector &DuckDBPyResult::GetTypes() { return result->types; } -int64_t DuckDBPyResult::GetRowcount() { +int64_t DuckDBPyResult::ComputeRowChanges() { if (!result || result->HasError()) { return -1; } @@ -80,8 +83,15 @@ int64_t DuckDBPyResult::GetRowcount() { if (result->type == QueryResultType::STREAM_RESULT) { // CHANGED_ROWS statements always produce a single already-computed row, so materializing here // does not trigger any additional query execution - it just changes the in-memory representation. - auto &stream_result = result->Cast(); - auto materialized = stream_result.Materialize(); + // Still release the GIL around it though: Materialize() drives the same native Fetch() machinery + // as any other result consumption, and other fetch paths (e.g. Fetchone()) release the GIL around it. + unique_ptr materialized; + { + D_ASSERT(duckdb::PyUtil::GilCheck()); + nb::gil_scoped_release release; + auto &stream_result = result->Cast(); + materialized = stream_result.Materialize(); + } if (!materialized || materialized->HasError()) { return -1; } diff --git a/tests/fast/api/test_dbapi10.py b/tests/fast/api/test_dbapi10.py index 829f3974..733e93ea 100644 --- a/tests/fast/api/test_dbapi10.py +++ b/tests/fast/api/test_dbapi10.py @@ -108,3 +108,35 @@ def test_rowcount_does_not_disturb_fetch(self, duckdb_cursor): def test_rowcount_ddl_is_unknown(self, duckdb_cursor): duckdb_cursor.execute("CREATE TABLE t (i INTEGER)") assert duckdb_cursor.rowcount == -1 + + def test_rowcount_after_fetchall(self, duckdb_cursor): + # Regression test: rowcount must survive full consumption of the result via fetchall(), + # not just a bare execute() with no fetch at all. + duckdb_cursor.execute("CREATE TABLE t (i INTEGER)") + duckdb_cursor.execute("INSERT INTO t VALUES (1), (2), (3), (4)") + assert duckdb_cursor.fetchall() == [(4,)] + assert duckdb_cursor.rowcount == 4 + + def test_rowcount_after_fetchone(self, duckdb_cursor): + duckdb_cursor.execute("CREATE TABLE t (i INTEGER)") + duckdb_cursor.execute("INSERT INTO t VALUES (1), (2), (3), (4)") + assert duckdb_cursor.fetchone() == (4,) + assert duckdb_cursor.rowcount == 4 + + def test_rowcount_after_fetchmany(self, duckdb_cursor): + duckdb_cursor.execute("CREATE TABLE t (i INTEGER)") + duckdb_cursor.execute("INSERT INTO t VALUES (1), (2), (3), (4)") + assert duckdb_cursor.fetchmany(1) == [(4,)] + assert duckdb_cursor.rowcount == 4 + + def test_rowcount_after_fetchdf(self, duckdb_cursor): + duckdb_cursor.execute("CREATE TABLE t (i INTEGER)") + duckdb_cursor.execute("INSERT INTO t VALUES (1), (2), (3), (4)") + duckdb_cursor.fetchdf() + assert duckdb_cursor.rowcount == 4 + + def test_rowcount_after_fetchnumpy(self, duckdb_cursor): + duckdb_cursor.execute("CREATE TABLE t (i INTEGER)") + duckdb_cursor.execute("INSERT INTO t VALUES (1), (2), (3), (4)") + duckdb_cursor.fetchnumpy() + assert duckdb_cursor.rowcount == 4 From 8a7798ab964d7708abc6a155a21ac3a2d89194a8 Mon Sep 17 00:00:00 2001 From: Colin Rogers <111200756+colin-k-rogers@users.noreply.github.com> Date: Tue, 4 Aug 2026 11:23:35 -0700 Subject: [PATCH 3/3] Remove dead rowcount caching in ExecuteOrThrow; add executemany/arrow 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 --- src/include/duckdb_python/pyrelation.hpp | 7 +++++-- src/pyrelation.cpp | 1 - tests/fast/api/test_dbapi10.py | 17 +++++++++++++++++ 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/include/duckdb_python/pyrelation.hpp b/src/include/duckdb_python/pyrelation.hpp index aebdf08b..b1611026 100644 --- a/src/include/duckdb_python/pyrelation.hpp +++ b/src/include/duckdb_python/pyrelation.hpp @@ -30,8 +30,11 @@ struct DuckDBPyRelation { nb::list Description(); - //! Cached at execution time (see ExecuteOrThrow / the DuckDBPyResult constructor overload) so that - //! it survives the Fetch*() methods below, several of which null out `result` once fully consumed. + //! Cached at construction time (see the DuckDBPyResult constructor overload) so that it survives + //! the Fetch*() methods below, several of which null out `result` once fully consumed. Note this + //! is only ever non-default for relations built directly from a DuckDBPyResult (the connection's + //! execute()/executemany() path) - relations built from a lazy Relation (rel != nullptr, executed + //! via ExecuteOrThrow) are always SELECT-shaped and so never produce a CHANGED_ROWS value anyway. int64_t GetRowcount() const { return row_changes; } diff --git a/src/pyrelation.cpp b/src/pyrelation.cpp index 06cab6fb..ff894dfb 100644 --- a/src/pyrelation.cpp +++ b/src/pyrelation.cpp @@ -835,7 +835,6 @@ void DuckDBPyRelation::ExecuteOrThrow(bool stream_result) { query_result->ThrowError(); } result = std::make_unique(std::move(query_result)); - row_changes = result->GetRowcount(); } PandasDataFrame DuckDBPyRelation::FetchDF(bool date_as_object) { diff --git a/tests/fast/api/test_dbapi10.py b/tests/fast/api/test_dbapi10.py index 733e93ea..a3907bc5 100644 --- a/tests/fast/api/test_dbapi10.py +++ b/tests/fast/api/test_dbapi10.py @@ -140,3 +140,20 @@ def test_rowcount_after_fetchnumpy(self, duckdb_cursor): duckdb_cursor.execute("INSERT INTO t VALUES (1), (2), (3), (4)") duckdb_cursor.fetchnumpy() assert duckdb_cursor.rowcount == 4 + + def test_rowcount_after_to_arrow_table(self, duckdb_cursor): + pytest.importorskip("pyarrow") + duckdb_cursor.execute("CREATE TABLE t (i INTEGER)") + duckdb_cursor.execute("INSERT INTO t VALUES (1), (2), (3), (4)") + duckdb_cursor.to_arrow_table() + assert duckdb_cursor.rowcount == 4 + + def test_rowcount_executemany_reflects_last_statement_only(self, duckdb_cursor): + # Documents current (limited) behavior: executemany() only keeps the QueryResult of the last + # parameter set it executes, so rowcount reflects that one statement rather than the total + # number of rows affected across all parameter sets. Not the DB-API-idiomatic answer, but + # pinned here so a future change to this behavior is a deliberate, visible decision. + duckdb_cursor.execute("CREATE TABLE t (i INTEGER)") + duckdb_cursor.executemany("INSERT INTO t VALUES (?)", [(1,), (2,), (3,)]) + assert duckdb_cursor.table("t").fetchall() == [(1,), (2,), (3,)] + assert duckdb_cursor.rowcount == 1