SQLiteEngine not handling large Integers properly.

[vertyco]

Issue

Large integers (For example, a Discord guild ID of 625757527765811240) are not inserted properly when using the SQLiteEngine.
The resulting int that actually gets inserted is 625757527765811200 for some reason.

When using aiosqlite manually I'm able to insert the row normally.

How to reproduce

Simply run this code:

import asyncio
from pathlib import Path

import aiosqlite
from piccolo.columns import BigInt, BigSerial, Integer, Serial
from piccolo.engine.sqlite import SQLiteEngine
from piccolo.table import Table

root = Path(__file__).parent
db_path = root / "db.sqlite"

if db_path.exists():
    db_path.unlink()

DB = SQLiteEngine(path=str(db_path))


class SomeTable(Table, db=DB):
    id: Serial
    # Discord Guild ID
    num1 = BigInt()
    num2 = Integer()
    num3 = BigSerial()


async def main():
    # Create the table
    await SomeTable.create_table()
    guild_id = 625757527765811240
    
    # Insert a row
    guild = SomeTable(num1=guild_id, num2=guild_id, num3=guild_id)
    await guild.save()
    res = await SomeTable.select().first()
    print("PICCOLO")
    print("num1", res["num1"])
    print("num2", res["num2"])
    print("num3", res["num3"])
    await SomeTable.delete(force=True)

    print("---")
    print("aiosqlite")
    # Now insert a row manually using aiosqlite
    async with aiosqlite.connect(db_path) as conn:
        await conn.execute(
            "INSERT INTO some_table (num1, num2, num3) VALUES (?, ?, ?)",
            (guild_id, guild_id, guild_id),
        )
        await conn.commit()
        async with conn.execute("SELECT * FROM some_table") as cursor:
            res = await cursor.fetchone()
            print("num1", res[1])
            print("num2", res[2])
            print("num3", res[3])


if __name__ == "__main__":
    asyncio.run(main())

It should output the following:

PICCOLO
num1 625757527765811200
num2 625757527765811200
num3 625757527765811200
---
aiosqlite
num1 625757527765811240
num2 625757527765811240
num3 625757527765811240

I know BigInt is just an alias for Integer for the sqlite engine but included it just in case, I'm not sure why this happens.
The issue occurs on both Win10 and Ubuntu if that matters, but only tested this script on windows.

Also, been using the postgres engine for a while now. Thanks for making an amazing ORM <3

[sinisaos]

@vertyco When precision is important, you should use Numeric (or Decimal, which is an alias for Numeric) column. In that case, both Piccolo and aiosqlite have same precision.

from piccolo.columns import Serial, Numeric, Decimal

class SomeTable(Table, db=DB):
    id: Serial
    # Discord Guild ID
    num1 = Numeric()
    num2 = Decimal()
    num3 = Numeric()

Result is:

PICCOLO
num1 625757527765811240
num2 625757527765811240
num3 625757527765811240
---
aiosqlite
num1 625757527765811240
num2 625757527765811240
num3 625757527765811240

[vertyco]

@sinisaos ahh sweet, thanks for the quick solution! However, is this still a bug? wouldn't we want piccolo to reflect raw aiosqlite in terms of consistency?

Also noticed that the Decimal column is not shown in the documentation for the stable branch, not a big deal but just something to point out.

[sinisaos]

@sinisaos ahh sweet, thanks for the quick solution! However, is this still a bug? wouldn't we want piccolo to reflect raw aiosqlite in terms of consistency?

@vertyco No problem. I don't think it's a bug. Sqlite only has an integer type for storing numbers. Piccolo is a query builder and ORM with a wide range of column types to store numbers based on precision etc. I think it is up to us to use the appropriate column in the table definition based on our needs. In this case Numeric or Decimal.

Also noticed that the Decimal column is not shown in the documentation for the stable branch, not a big deal but just something to point out.

There is a hint in the Numeric column docs section that mentions the Decimal column as an alias for the Numeric column.

[vertyco]

I dont think I am quite following the logic behind that. We want to store a large integer but have to use a column type that returns a decimal, yet SQLite's integer column itself is sufficient for storing those when used directly without Piccolo.

Sorry if i'm just misunderstanding something, I certainly would like to if that's the case. It just seems like there is such a clear inconsistency between the ORM and what the column type should be capable of storing to me.

[sinisaos]

@vertyco I'm sorry, but that's just my opinion. Probably @dantownsend (the author of Piccolo) has a better explanation (or solution) than I do.

[vertyco]

No worries! Just wanted to make sure i'm not missing something that went over my head.

[sinisaos]

Maybe the solution could be something like this

# in piccolo/columns/column_types.py
class BigInt(Integer):
   ...
   def _get_column_type(self, engine_type: str):
        if engine_type == "postgres":
            return "BIGINT"
        elif engine_type == "cockroach":
            return "BIGINT"
        elif engine_type == "sqlite":
            return "NUMERIC" # <- this is change
        raise Exception("Unrecognized engine type")
   ...

Than usage will be

from piccolo.columns import Serial, Numeric, Decimal, BigInt

class SomeTable(Table, db=DB):
    id: Serial
    # Discord Guild ID
    num1 = BigInt()
    num2 = Numeric()
    num3 = Decimal()

Result is:

PICCOLO
num1 625757527765811240
num2 625757527765811240
num3 625757527765811240
---
aiosqlite
num1 625757527765811240
num2 625757527765811240
num3 625757527765811240

With this small change the Piccolo unit test passed. Cheers

[vertyco]

While that may mask the issue on the surface, the underlying problem is the data type that it returns.
Take this table again for example.

class SomeTable(Table, db=DB):
    id: Serial
    # Discord Guild ID
    num1 = BigInt()
    num2 = Integer()
    num3 = Numeric()

Running the same code but printing out the type, we get the following:

PICCOLO
num1 625757527765811200 <class 'int'>
num2 625757527765811200 <class 'int'>
num3 625757527765811240 <class 'decimal.Decimal'>
---
aiosqlite
num1 625757527765811240 <class 'int'>
num2 625757527765811240 <class 'int'>
num3 625757527765811240 <class 'int'>

The problem is that it num3 returns decimal.Decimal, we do not want this and should be able to use the Integer column to store these large integers since that is what sqlite itself supports.

Hopefully @dantownsend can give some input on this

[sinisaos]

@vertyco Looks like you were right about it being a bug. I think the problem is with this converter.

piccolo/piccolo/engine/sqlite.py

Lines 174 to 178 in 7373a84

	def convert_int_out(value: str) -> int:
	"""
	Make sure Integer values are actually of type int.
	"""
	return int(float(value))

If we run the Python interpreter, we get this.

>>> int(float(625757527765811240))
625757527765811200 # wrong
>>> int(625757527765811240)              
625757527765811240 # correct

If we change return statement to return int(value), everything is fine and the unit tests pass.

class SomeTable(Table, db=DB):
    id: Serial
    # Discord Guild ID
    num1 = Integer()
    num2 = BigInt()
    num3 = BigSerial()

The result is

PICCOLO
num1 625757527765811240 <class 'int'>
num2 625757527765811240 <class 'int'>
num3 625757527765811240 <class 'int'>
---
aiosqlite
num1 625757527765811240
num2 625757527765811240
num3 625757527765811240

[vertyco]

Ahh nice! Figures its a python limitation with floats. I'll come up with a monkeypatch until this is able to be fixed, thanks for finding it!

Here is what i'm using as a workaround for now

# __init__.py 
import sqlite3
from piccolo.engine import sqlite

@sqlite.decode_to_string
def convert_int_out(value: str) -> int:
    return int(value)

sqlite.CONVERTERS["INTEGER"] = convert_int_out
sqlite3.register_converter("INTEGER", convert_int_out)

[sinisaos]

@vertyco I tried your workaround and it works :). I can do a PR for this small converter change if @dantownsend is okay with it.

[dantownsend]

Thanks both for looking into this.

I honestly can't remember why we do int(float(value)) instead of just int(value) here:

@decode_to_string
def convert_int_out(value: str) -> int:
    """
    Make sure Integer values are actually of type int.
    """
    return int(float(value))

It's as if we're expecting SQLite to return integers as 1.0 instead of just 1.

I'll have a look to see if this is ever the case, and if not, will change it to int(value).

[dantownsend]

I can't see any reason to have int(float(value)) - I've added a PR to fix it.