crystal-lang / crystal-sqlite3

SQLite3 bindings for Crystal
MIT License
139 stars 30 forks source link

add arg support for Int16, Int8, UInt16, UInt8 #98

Closed baseballlover723 closed 1 month ago

baseballlover723 commented 1 month ago

I was trying to use Jennifer with sqlite with an Int16 in my model, and I got an error saying that it was supported. Upon looking into it, it seems that sqlite only stores regular Int32s (and it works if I switch the type to Int32). However, given that Int8, Int16, UInt8, and UInt16 can all fit in a Int32. I don't see why we can't just convert them to Int32's. There is already a similar thing happening for Float32 being converted to Float64, and this is essentially the same thing but for ints.

I didn't see any tests for any of the other binds, so I didn't add any here.

bcardiff commented 1 month ago

ResultSet should be able to work with the same types as Statement. Would you add the overloads?

I don't have concern on signed ints being added. But adding unsigned ones goes against the idea of using them mostly for bit/protocol implementation rather than domain logic. I'm not against adding them, but they feel odd. Why don't put them in crystal-db directly to enforce them on every driver?

baseballlover723 commented 1 month ago

ResultSet should be able to work with the same types as Statement. Would you add the overloads?

I have added the overloads

I don't have concern on signed ints being added. But adding unsigned ones goes against the idea of using them mostly for bit/protocol implementation rather than domain logic. I'm not against adding them, but they feel odd. Why don't put them in crystal-db directly to enforce them on every driver?

I'd prefer to have it here, at least for now (though crystal-db may be the better spot for it), because I'm really sure that this conversion can be made that generally. Other databases with multiple int types may want to handle things differently imo and I don't want to drive something that I don't have much experience in, nor much time to dive any deeper into this. And to me, this seems to be sqlite specific, as sqlite only has 1 integer type (and other dbs with more integer types may want to do other things). And I would expect that if sqlite can work with a larger int type, then it should be able to work with smaller unsigned int types that can be fully encapsulated in the larger int type.

bcardiff commented 1 month ago

Hmm. for some reason I can't push to your branch. I will open another PR with the specs.

Note: UInt64 is not supported because sqlite integer types are always signed up to 8 bytes. So trying to store a UInt64 will not fit.