Skip to content

test: Fix insert rowcount #242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 17, 2023
Merged

test: Fix insert rowcount #242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c6498f1
test: Fix insert rowcount
ptiurin Mar 15, 2023
f1f0819
no longer special case for insert
ptiurin Mar 15, 2023
63c39ab
return the content length
ptiurin Mar 16, 2023
08ae986
space
ptiurin Mar 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/firebolt/async_db/cursor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,10 +303,12 @@ def _row_set_from_response(
# Skip parsing floats to properly parse them later
query_data = response.json(parse_float=str)
rowcount = int(query_data["rows"])
descriptions = [
descriptions: Optional[List[Column]] = [
Column(d["name"], parse_type(d["type"]), None, None, None, None, None)
for d in query_data["meta"]
]
if not descriptions:
Copy link
Collaborator

@stepansergeevitch stepansergeevitch Mar 17, 2023

Choose a reason for hiding this comment

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

Why can't we just have it empty?

Copy link
Collaborator

@stepansergeevitch stepansergeevitch Mar 17, 2023

Choose a reason for hiding this comment

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

Ok, pep 249 says it should be None

Copy link
Contributor Author

@ptiurin ptiurin Mar 17, 2023

Choose a reason for hiding this comment

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

I feel like it's a bit counter-intuitive to receive empty array in description. For example,
A user does an INSERT query, checks cursor.rowcount is correct then checks for some reason cursor.description and it's []. The name itself implies singular as well.
I don't feel too strongly about it, but this is also how we had it up until now.

descriptions = None
statistics = Statistics(**query_data["statistics"])
# Parse data during fetch
rows = query_data["data"]
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/dbapi/async/test_queries_async.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,9 +388,9 @@ async def test_multi_statement_query(connection: Connection) -> None:
"SELECT * FROM test_tb_async_multi_statement;"
"SELECT * FROM test_tb_async_multi_statement WHERE i <= 1"
)
== -1
== 1 # Insert always returns 1 row with num of rows modified
), "Invalid row count returned for insert"
assert c.rowcount == -1, "Invalid row count"
assert c.rowcount == 1, "Invalid row count"
assert c.description is None, "Invalid description"

assert await c.nextset()
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/dbapi/sync/test_queries.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,9 +333,9 @@ def test_multi_statement_query(connection: Connection) -> None:
"SELECT * FROM test_tb_multi_statement;"
"SELECT * FROM test_tb_multi_statement WHERE i <= 1"
)
== -1
== 1 # Insert always returns 1 row with num of rows modified
), "Invalid row count returned for insert"
assert c.rowcount == -1, "Invalid row count"
assert c.rowcount == 1, "Invalid row count"
assert c.description is None, "Invalid description"

assert c.nextset()
Expand Down