Skip to content

Conversation

@mdesmet
Copy link
Contributor

@mdesmet mdesmet commented Jan 18, 2023

Description

Resolves #319

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

* Provide row count on dbapi's Cursor for DML queries

Copy link
Member

@hovaesco hovaesco left a comment

Choose a reason for hiding this comment

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

LGTM % one comment

the number of rows is only knowns when all rows have been retrieved.
"""

if self._query is not None and self._query.update_count is not None:
Copy link
Member

Choose a reason for hiding this comment

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

The comment above can be updated to reflect the change.

self.update_type = update_type
self.rows = rows
self.columns = columns
@dataclass
Copy link
Member

Choose a reason for hiding this comment

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

Very nice. ❤️


if self._query is not None and self._query.update_count is not None:
return self._query.update_count
return -1
Copy link
Member

Choose a reason for hiding this comment

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

This is not obvious so maybe a link to https://peps.python.org/pep-0249/#rowcount here would be useful i.e. the -1 is what the DB-API requires.

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % outdated comment

@mdesmet mdesmet force-pushed the feat/affected_rows branch from 10edf59 to d0b3ba1 Compare January 19, 2023 08:55
@mdesmet mdesmet self-assigned this Jan 19, 2023
@mdesmet mdesmet force-pushed the feat/affected_rows branch from d0b3ba1 to f8f9aa8 Compare January 19, 2023 15:38
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Lgtm. The comment doesn't need to be addressed in this PR, it's just a question.

assert cur.description[0][6] is None


class _TestTable:
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a utils module under test directory? I see some other such classes and methods which we be useful across tests.

@hashhar hashhar merged commit bfa754a into trinodb:master Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Support row count for DML queries

4 participants