Conversation
This turned out to be way more annoying than anticipated, but the C code for (de)serialization of numeric was incomprehensible (and buggy, e.g. it didn't handle NaN properly), so I adjusted code from postgresql-binary package to serialize on Haskell side instead.
| , numVarWeight :: !CShort | ||
| , numVarSign :: !CShort | ||
| , numVarDscale :: !CShort | ||
| , numVarDigits :: !(Ptr CShort) -- elements in network byte order |
There was a problem hiding this comment.
Are you sure about the network byte order? Isn't that what we invoke htons and ntohs for in pqt_{get,put}_numeric for?
There was a problem hiding this comment.
They are invoked for all fields except the array:
hpqtypes/libpqtypes/src/numerics.c
Line 200 in fd0dab5
hpqtypes/libpqtypes/src/numerics.c
Line 221 in fd0dab5
It's a bit messy, but avoids having to allocate a new buffer for digits on the C side (which is always annoying) in pqt_get_numeric.
| mkInteger acc ptr = \case | ||
| 0 -> pure acc | ||
| n -> do | ||
| v <- ntohs <$> peek ptr |
There was a problem hiding this comment.
Related to my above comment, aren't we swapping the byte order a second time here?
There was a problem hiding this comment.
As I wrote above, no, because the order is not swapped for the array in C.
| 0 -> pure acc | ||
| n -> do | ||
| v <- ntohs <$> peek ptr | ||
| mkInteger (acc * 10000 + fromIntegral v) (ptr `plusPtr` 2) (n - 1) |
There was a problem hiding this comment.
Shouldn't we check that each short is < 10^4 ? (And >= 0)
Every NumericDigit represents values 0 <= NBASE -1 where NBASE = 10^4.
There was a problem hiding this comment.
Also, on a tangential note, why do we use (platform dependent) short when the postgres backend uses int16 for NumericDigit?
There was a problem hiding this comment.
Shouldn't we check that each short is < 10^4 ? (And >= 0)
Sounds good, I added a check.
Also, on a tangential note, why do we use (platform dependent) short when the postgres backend uses int16 for NumericDigit?
Because libpqtypes is not of the highest quality and uses short/int all over the place :P short is 2 bytes on all platforms that matter though.
There was a problem hiding this comment.
Fair enough. That's also how we get away with using a constant 2 for the pointer increment I guess.
bgohla
left a comment
bgohla
left a comment
There was a problem hiding this comment.
Thanks. I'm confused about the byte order conversion though.
| @@ -1,5 +1,6 @@ | |||
| # hpqtypes-1.14.1.0 | |||
| # hpqtypes-1.14.1.0 (????-??-??) | |||
There was a problem hiding this comment.
Will the date be set in a separate commit?
| type PQDest Word64 = CULLong | ||
| toSQL n _ = putAsPtr (fromIntegral n) | ||
|
|
||
| instance ToSQL Integer where |
There was a problem hiding this comment.
Actually, there is a major caveat, that I should have pointed out, that should be documented here: There is a limit to how many decimal digits Postgres allows in a Numeric value, namely
up to 131072 digits before the decimal point; up to 16383 digits after the decimal point
according to https://www.postgresql.org/docs/18/datatype-numeric.html#DATATYPE-NUMERIC
2 participants
This turned out to be way more annoying than anticipated, but the C code for (de)serialization of numeric was incomprehensible (and buggy, e.g. it didn't handle NaN properly), so I adjusted code from postgresql-binary package to serialize on Haskell side instead.