feat: implement Connection.commit, DataError on empty data fetch (#76)

* add connection.commit method

* raise DataError on empty result fetch

* fix type annotations

* improve integration tests

Author: stepansergeevitch

src/firebolt/async_db/connection.py

@@ -191,6 +191,11 @@ def _remove_cursor(self, cursor: Cursor) -> None:
         except ValueError:
             pass
 
+    def commit(self) -> None:
+        """Does nothing since Firebolt doesn't have transactions"""
+        if self.closed:
+            raise ConnectionClosedError("Unable to commit: connection closed")
+
 
 class Connection(BaseConnection):
     cleandoc(

src/firebolt/async_db/cursor.py

@@ -290,9 +290,11 @@ def _get_next_range(self, size: int) -> Tuple[int, int]:
             and update _idx to point to the end of this range
             """
         )
+
         if self._rows is None:
             # No elements to take
-            return (0, 0)
+            raise DataError("no rows to fetch")
+
         left = self._idx
         right = min(self._idx + size, len(self._rows))
         self._idx = right
@@ -328,8 +330,8 @@ def fetchmany(self, size: Optional[int] = None) -> List[List[ColType]]:
     @check_query_executed
     def fetchall(self) -> List[List[ColType]]:
         """Fetch all remaining rows of a query result."""
+        left, right = self._get_next_range(self.rowcount)
         assert self._rows is not None
-        left, right = self._get_next_range(len(self._rows))
         rows = self._rows[left:right]
         return [self._parse_row(row) for row in rows]
 

tests/integration/dbapi/async/test_queries_async.py

@@ -1,9 +1,9 @@
 from datetime import date, datetime
 from typing import Any, List
 
-from pytest import mark
+from pytest import mark, raises
 
-from firebolt.async_db import Connection, Cursor
+from firebolt.async_db import Connection, Cursor, DataError
 from firebolt.async_db._types import ColType, Column
 
 
@@ -137,7 +137,14 @@ async def test_empty_query(c: Cursor, query: str) -> None:
         assert await c.execute(query) == -1, "Invalid row count returned"
         assert c.rowcount == -1, "Invalid rowcount value"
         assert c.description is None, "Invalid description"
-        assert await c.fetchone() is None, "Invalid data returned"
+        with raises(DataError):
+            await c.fetchone()
+
+        with raises(DataError):
+            await c.fetchmany()
+
+        with raises(DataError):
+            await c.fetchall()
 
     with connection.cursor() as c:
         await c.execute("DROP TABLE IF EXISTS test_tb")

tests/integration/dbapi/sync/test_queries.py

@@ -1,9 +1,11 @@
 from datetime import date, datetime
 from typing import Any, List
 
+from pytest import raises
+
 from firebolt.async_db._types import ColType
 from firebolt.async_db.cursor import Column
-from firebolt.db import Connection, Cursor
+from firebolt.db import Connection, Cursor, DataError
 
 
 def assert_deep_eq(got: Any, expected: Any, msg: str) -> bool:
@@ -128,7 +130,14 @@ def test_empty_query(c: Cursor, query: str) -> None:
         assert c.execute(query) == -1, "Invalid row count returned"
         assert c.rowcount == -1, "Invalid rowcount value"
         assert c.description is None, "Invalid description"
-        assert c.fetchone() is None, "Invalid data returned"
+        with raises(DataError):
+            c.fetchone()
+
+        with raises(DataError):
+            c.fetchmany()
+
+        with raises(DataError):
+            c.fetchall()
 
     with connection.cursor() as c:
         c.execute("DROP TABLE IF EXISTS test_tb")

tests/unit/async_db/test_connection.py

@@ -190,3 +190,13 @@ async def test_connect_engine_name(
         api_endpoint=settings.server,
     ) as connection:
         assert await connection.cursor().execute("select*") == len(python_query_data)
+
+
+@mark.asyncio
+async def test_connection_commit(connection: Connection):
+    # nothing happens
+    connection.commit()
+
+    await connection.aclose()
+    with raises(ConnectionClosedError):
+        connection.commit()

tests/unit/async_db/test_cursor.py

@@ -10,6 +10,7 @@
 from firebolt.async_db.cursor import ColType, CursorState
 from firebolt.common.exception import (
     CursorClosedError,
+    DataError,
     EngineNotRunningError,
     FireboltDatabaseError,
     OperationalError,
@@ -175,7 +176,6 @@ async def test_cursor_execute(
         assert await query() == -1, "Invalid row count for insert query"
         assert cursor.rowcount == -1, "Invalid rowcount value for insert query"
         assert cursor.description is None, "Invalid description for insert query"
-        assert await cursor.fetchone() is None, "Non-empty fetchone for insert query"
 
 
 @mark.asyncio
@@ -264,6 +264,7 @@ async def test_cursor_fetchone(
     auth_callback: Callable,
     auth_url: str,
     query_callback: Callable,
+    insert_query_callback: Callable,
     query_url: str,
     cursor: Cursor,
 ):
@@ -284,13 +285,19 @@ async def test_cursor_fetchone(
         await cursor.fetchone() is None
     ), "fetchone should return None when no rows left to fetch"
 
+    httpx_mock.add_callback(insert_query_callback, url=query_url)
+    await cursor.execute("sql")
+    with raises(DataError):
+        await cursor.fetchone()
+
 
 @mark.asyncio
 async def test_cursor_fetchmany(
     httpx_mock: HTTPXMock,
     auth_callback: Callable,
     auth_url: str,
     query_callback: Callable,
+    insert_query_callback: Callable,
     query_url: str,
     cursor: Cursor,
 ):
@@ -342,13 +349,19 @@ async def test_cursor_fetchmany(
         len(await cursor.fetchmany()) == 0
     ), "fetchmany should return empty result set when no rows left to fetch"
 
+    httpx_mock.add_callback(insert_query_callback, url=query_url)
+    await cursor.execute("sql")
+    with raises(DataError):
+        await cursor.fetchmany()
+
 
 @mark.asyncio
 async def test_cursor_fetchall(
     httpx_mock: HTTPXMock,
     auth_callback: Callable,
     auth_url: str,
     query_callback: Callable,
+    insert_query_callback: Callable,
     query_url: str,
     cursor: Cursor,
 ):
@@ -371,6 +384,11 @@ async def test_cursor_fetchall(
         len(await cursor.fetchall()) == 0
     ), "fetchmany should return empty result set when no rows left to fetch"
 
+    httpx_mock.add_callback(insert_query_callback, url=query_url)
+    await cursor.execute("sql")
+    with raises(DataError):
+        await cursor.fetchall()
+
 
 # This tests a temporary functionality, needs to be removed when the
 # functionality is removed

tests/unit/db/test_connection.py

@@ -168,3 +168,12 @@ def test_connection_unclosed_warnings():
     assert "Unclosed" in str(
         winfo.list[0].message
     ), "Invalid unclosed connection warning"
+
+
+def test_connection_commit(connection: Connection):
+    # nothing happens
+    connection.commit()
+
+    connection.close()
+    with raises(ConnectionClosedError):
+        connection.commit()

tests/unit/db/test_cursor.py

@@ -8,6 +8,7 @@
 from firebolt.async_db.cursor import ColType, Column, CursorState
 from firebolt.common.exception import (
     CursorClosedError,
+    DataError,
     OperationalError,
     QueryNotRunError,
 )
@@ -160,7 +161,6 @@ def test_cursor_execute(
         assert query() == -1, "Invalid row count for insert query"
         assert cursor.rowcount == -1, "Invalid rowcount value for insert query"
         assert cursor.description is None, "Invalid description for insert query"
-        assert cursor.fetchone() is None, "Non-empty fetchone for insert query"
 
 
 def test_cursor_execute_error(
@@ -215,6 +215,7 @@ def test_cursor_fetchone(
     auth_callback: Callable,
     auth_url: str,
     query_callback: Callable,
+    insert_query_callback: Callable,
     query_url: str,
     cursor: Cursor,
 ):
@@ -235,12 +236,18 @@ def test_cursor_fetchone(
         cursor.fetchone() is None
     ), "fetchone should return None when no rows left to fetch"
 
+    httpx_mock.add_callback(insert_query_callback, url=query_url)
+    cursor.execute("sql")
+    with raises(DataError):
+        cursor.fetchone()
+
 
 def test_cursor_fetchmany(
     httpx_mock: HTTPXMock,
     auth_callback: Callable,
     auth_url: str,
     query_callback: Callable,
+    insert_query_callback: Callable,
     query_url: str,
     cursor: Cursor,
 ):
@@ -292,12 +299,18 @@ def test_cursor_fetchmany(
         len(cursor.fetchmany()) == 0
     ), "fetchmany should return empty result set when no rows left to fetch"
 
+    httpx_mock.add_callback(insert_query_callback, url=query_url)
+    cursor.execute("sql")
+    with raises(DataError):
+        cursor.fetchmany()
+
 
 def test_cursor_fetchall(
     httpx_mock: HTTPXMock,
     auth_callback: Callable,
     auth_url: str,
     query_callback: Callable,
+    insert_query_callback: Callable,
     query_url: str,
     cursor: Cursor,
 ):
@@ -320,6 +333,11 @@ def test_cursor_fetchall(
         len(cursor.fetchall()) == 0
     ), "fetchmany should return empty result set when no rows left to fetch"
 
+    httpx_mock.add_callback(insert_query_callback, url=query_url)
+    cursor.execute("sql")
+    with raises(DataError):
+        cursor.fetchall()
+
 
 # This tests a temporary functionality, needs to be removed when the
 # functionality is removed