fix(NoTicket): better decimal parsing and testing (#429)

Author: ptiurin

src/firebolt/common/_types.py

@@ -341,12 +341,8 @@ def parse_value(
             raise DataError(f"Invalid bytea value {value}: str expected")
         return _parse_bytea(value)
     if isinstance(ctype, DECIMAL):
-        if not isinstance(value, (str, int, float)):
+        if not isinstance(value, (str, int)):
             raise DataError(f"Invalid decimal value {value}: str or int expected")
-        if isinstance(value, float):
-            # Decimal constructor doesn't support float
-            # so we need to convert it to string first
-            value = str(value)
         return Decimal(value)
     if isinstance(ctype, ARRAY):
         if not isinstance(value, list):

src/firebolt/common/row_set/streaming_common.py

@@ -94,7 +94,8 @@ def _next_json_lines_record_from_line(
             return None
 
         try:
-            record = json.loads(next_line)
+            # Skip parsing floats to properly parse them later
+            record = json.loads(next_line, parse_float=str)
         except json.JSONDecodeError as err:
             raise OperationalError(
                 f"Invalid JSON line response format: {next_line}"

src/firebolt/common/row_set/synchronous/in_memory.py

@@ -43,7 +43,8 @@ def append_response_stream(self, stream: Iterator[bytes]) -> None:
             self.append_empty_response()
         else:
             try:
-                query_data = json.loads(content)
+                # Skip parsing floats to properly parse them later
+                query_data = json.loads(content, parse_float=str)
 
                 if "errors" in query_data and len(query_data["errors"]) > 0:
                     raise FireboltStructuredError(query_data)

tests/integration/dbapi/async/V1/conftest.py

@@ -1,11 +1,7 @@
-from decimal import Decimal
-from typing import List
-
 from pytest import fixture
 
 from firebolt.async_db import Connection, connect
 from firebolt.client.auth.base import Auth
-from firebolt.common._types import ColType
 
 
 @fixture
@@ -78,14 +74,3 @@ async def connection_no_engine(
         api_endpoint=api_endpoint,
     ) as connection:
         yield connection
-
-
-@fixture
-def all_types_query_response_v1(
-    all_types_query_response: List[List[ColType]],
-) -> List[List[ColType]]:
-    """
-    V1 still returns decimals as floats, despite overflow. That's why it's not fully accurate.
-    """
-    all_types_query_response[0][18] = Decimal("1231232.1234599999152123928070068359375")
-    return all_types_query_response

tests/integration/dbapi/async/V1/test_queries_async.py

@@ -78,15 +78,15 @@ async def test_connect_engine_name(
     connection_engine_name: Connection,
     all_types_query: str,
     all_types_query_description: List[Column],
-    all_types_query_response_v1: List[ColType],
+    all_types_query_response: List[ColType],
     timezone_name: str,
 ) -> None:
     """Connecting with engine name is handled properly."""
     await test_select(
         connection_engine_name,
         all_types_query,
         all_types_query_description,
-        all_types_query_response_v1,
+        all_types_query_response,
         timezone_name,
     )
 
@@ -95,15 +95,15 @@ async def test_connect_no_engine(
     connection_no_engine: Connection,
     all_types_query: str,
     all_types_query_description: List[Column],
-    all_types_query_response_v1: List[ColType],
+    all_types_query_response: List[ColType],
     timezone_name: str,
 ) -> None:
     """Connecting with engine name is handled properly."""
     await test_select(
         connection_no_engine,
         all_types_query,
         all_types_query_description,
-        all_types_query_response_v1,
+        all_types_query_response,
         timezone_name,
     )
 
@@ -112,7 +112,7 @@ async def test_select(
     connection: Connection,
     all_types_query: str,
     all_types_query_description: List[Column],
-    all_types_query_response_v1: List[ColType],
+    all_types_query_response: List[ColType],
     timezone_name: str,
 ) -> None:
     """Select handles all data types properly."""
@@ -130,15 +130,15 @@ async def test_select(
         assert c.rowcount == 1, "Invalid rowcount value"
         data = await c.fetchall()
         assert len(data) == c.rowcount, "Invalid data length"
-        assert_deep_eq(data, all_types_query_response_v1, "Invalid data")
+        assert_deep_eq(data, all_types_query_response, "Invalid data")
         assert c.description == all_types_query_description, "Invalid description value"
         assert len(data[0]) == len(c.description), "Invalid description length"
         assert len(await c.fetchall()) == 0, "Redundant data returned by fetchall"
 
         # Different fetch types
         await c.execute(all_types_query)
         assert (
-            await c.fetchone() == all_types_query_response_v1[0]
+            await c.fetchone() == all_types_query_response[0]
         ), "Invalid fetchone data"
         assert await c.fetchone() is None, "Redundant data returned by fetchone"
 
@@ -147,7 +147,7 @@ async def test_select(
         data = await c.fetchmany()
         assert len(data) == 1, "Invalid data size returned by fetchmany"
         assert_deep_eq(
-            data, all_types_query_response_v1, "Invalid data returned by fetchmany"
+            data, all_types_query_response, "Invalid data returned by fetchmany"
         )
 
 
@@ -328,7 +328,7 @@ async def test_empty_query(c: Cursor, query: str, params: tuple) -> None:
             datetime(2022, 1, 1, 1, 1, 1),
             True,
             [1, 2, 3],
-            Decimal(123.456),
+            Decimal("123.456"),
         ]
 
         await test_empty_query(

tests/integration/dbapi/sync/V1/conftest.py

@@ -1,10 +1,6 @@
-from decimal import Decimal
-from typing import List
-
 from pytest import fixture
 
 from firebolt.client.auth.base import Auth
-from firebolt.common._types import ColType
 from firebolt.db import Connection, connect
 
 
@@ -97,14 +93,3 @@ def connection_system_engine(
     )
     yield connection
     connection.close()
-
-
-@fixture
-def all_types_query_response_v1(
-    all_types_query_response: List[List[ColType]],
-) -> List[List[ColType]]:
-    """
-    V1 still returns decimals as floats, despite overflow. That's why it's not fully accurate.
-    """
-    all_types_query_response[0][18] = Decimal("1231232.1234599999152123928070068359375")
-    return all_types_query_response

tests/integration/dbapi/sync/V1/test_queries.py

@@ -30,15 +30,15 @@ def test_connect_engine_name(
     connection_engine_name: Connection,
     all_types_query: str,
     all_types_query_description: List[Column],
-    all_types_query_response_v1: List[ColType],
+    all_types_query_response: List[ColType],
     timezone_name: str,
 ) -> None:
     """Connecting with engine name is handled properly."""
     test_select(
         connection_engine_name,
         all_types_query,
         all_types_query_description,
-        all_types_query_response_v1,
+        all_types_query_response,
         timezone_name,
     )
 
@@ -47,15 +47,15 @@ def test_connect_no_engine(
     connection_no_engine: Connection,
     all_types_query: str,
     all_types_query_description: List[Column],
-    all_types_query_response_v1: List[ColType],
+    all_types_query_response: List[ColType],
     timezone_name: str,
 ) -> None:
     """Connecting with engine name is handled properly."""
     test_select(
         connection_no_engine,
         all_types_query,
         all_types_query_description,
-        all_types_query_response_v1,
+        all_types_query_response,
         timezone_name,
     )
 
@@ -64,7 +64,7 @@ def test_select(
     connection: Connection,
     all_types_query: str,
     all_types_query_description: List[Column],
-    all_types_query_response_v1: List[ColType],
+    all_types_query_response: List[ColType],
     timezone_name: str,
 ) -> None:
     """Select handles all data types properly."""
@@ -82,22 +82,22 @@ def test_select(
         assert c.rowcount == 1, "Invalid rowcount value"
         data = c.fetchall()
         assert len(data) == c.rowcount, "Invalid data length"
-        assert_deep_eq(data, all_types_query_response_v1, "Invalid data")
+        assert_deep_eq(data, all_types_query_response, "Invalid data")
         assert c.description == all_types_query_description, "Invalid description value"
         assert len(data[0]) == len(c.description), "Invalid description length"
         assert len(c.fetchall()) == 0, "Redundant data returned by fetchall"
 
         # Different fetch types
         c.execute(all_types_query)
-        assert c.fetchone() == all_types_query_response_v1[0], "Invalid fetchone data"
+        assert c.fetchone() == all_types_query_response[0], "Invalid fetchone data"
         assert c.fetchone() is None, "Redundant data returned by fetchone"
 
         c.execute(all_types_query)
         assert len(c.fetchmany(0)) == 0, "Invalid data size returned by fetchmany"
         data = c.fetchmany()
         assert len(data) == 1, "Invalid data size returned by fetchmany"
         assert_deep_eq(
-            data, all_types_query_response_v1, "Invalid data returned by fetchmany"
+            data, all_types_query_response, "Invalid data returned by fetchmany"
         )
 
 
@@ -273,7 +273,7 @@ def test_empty_query(c: Cursor, query: str, params: tuple) -> None:
             datetime(2022, 1, 1, 1, 1, 1),
             True,
             [1, 2, 3],
-            Decimal(123.456),
+            Decimal("123.456"),
         ]
 
         test_empty_query(

tests/unit/common/row_set/asynchronous/test_in_memory.py

@@ -1,4 +1,5 @@
 import json
+from decimal import Decimal
 from unittest.mock import MagicMock, patch
 
 import pytest
@@ -173,6 +174,7 @@ def test_append_empty_response(self, in_memory_rowset):
 
     async def test_append_response(self, in_memory_rowset, mock_response):
         """Test appending a response with data."""
+
         # Create a proper aclose method
         async def mock_aclose():
             mock_response.is_closed = True
@@ -207,6 +209,7 @@ async def test_append_response_empty_content(
         self, in_memory_rowset, mock_empty_response
     ):
         """Test appending a response with empty content."""
+
         # Create a proper aclose method
         async def mock_aclose():
             mock_empty_response.is_closed = True
@@ -226,6 +229,7 @@ async def test_append_response_invalid_json(
         self, in_memory_rowset, mock_invalid_json_response
     ):
         """Test appending a response with invalid JSON."""
+
         # Create a proper aclose method
         async def mock_aclose():
             mock_invalid_json_response.is_closed = True
@@ -245,6 +249,7 @@ async def test_append_response_missing_meta(
         self, in_memory_rowset, mock_missing_meta_response
     ):
         """Test appending a response with missing meta field."""
+
         # Create a proper aclose method
         async def mock_aclose():
             mock_missing_meta_response.is_closed = True
@@ -264,6 +269,7 @@ async def test_append_response_missing_data(
         self, in_memory_rowset, mock_missing_data_response
     ):
         """Test appending a response with missing data field."""
+
         # Create a proper aclose method
         async def mock_aclose():
             mock_missing_data_response.is_closed = True
@@ -281,6 +287,7 @@ async def mock_aclose():
 
     async def test_nextset_no_more_sets(self, in_memory_rowset, mock_response):
         """Test nextset when there are no more result sets."""
+
         # Create a proper aclose method
         async def mock_aclose():
             pass
@@ -296,6 +303,7 @@ async def test_nextset_with_more_sets(self, in_memory_rowset, mock_response):
         The implementation seems to add rowsets correctly, but behaves differently
         than expected when accessing them via nextset.
         """
+
         # Create a proper aclose method
         async def mock_aclose():
             pass
@@ -322,6 +330,7 @@ async def mock_aclose():
 
     async def test_iteration(self, in_memory_rowset, mock_response):
         """Test row iteration."""
+
         # Create a proper aclose method
         async def mock_aclose():
             pass
@@ -347,6 +356,7 @@ async def test_iteration_after_nextset(self, in_memory_rowset, mock_response):
         This test is tricky because in the mock setup, the second row set
         is actually empty despite us adding the same mock response.
         """
+
         # Create a proper aclose method
         async def mock_aclose():
             pass
@@ -410,6 +420,7 @@ async def test_empty_rowset_iteration(self, in_memory_rowset):
 
     async def test_aclose(self, in_memory_rowset, mock_response):
         """Test aclose method."""
+
         # Create a proper aclose method
         async def mock_aclose():
             pass
@@ -423,3 +434,23 @@ async def mock_aclose():
 
             # Verify sync close was called
             mock_close.assert_called_once()
+
+    async def test_append_response_with_decimals(
+        self, in_memory_rowset: InMemoryAsyncRowSet, mock_decimal_bytes_stream: Response
+    ):
+
+        await in_memory_rowset.append_response(mock_decimal_bytes_stream)
+
+        # Verify basic properties
+        assert in_memory_rowset.row_count == 2
+        assert len(in_memory_rowset.columns) == 3
+
+        # Get the row values and check decimal values are equal
+        rows = [row async for row in in_memory_rowset]
+
+        # Verify the decimal value is correctly parsed
+        for row in rows:
+            assert isinstance(row[2], Decimal), "Expected Decimal type"
+            assert (
+                str(row[2]) == "1231232.123459999990457054844258706536"
+            ), "Decimal value mismatch"