postgres-protocol: add pg numeric type #1231
Conversation
fredr
force-pushed
the
fredr/numeric-protocol
branch
from
e3dc77f to
ba79c93
Compare
|
Refactored the to/from string code a bit, its a bit hairy because of scientific notation, but should be a bit more readable now |
paolobarbolini
left a comment
paolobarbolini
left a comment
There was a problem hiding this comment.
I've left a couple of comments, including one on how you see this get used.
The final version should also have tests in tokio-postgres/tests/test/types/ and commits squashed.
|
|
||
| fn split_decimal(s: &[u8]) -> (&[u8], Option<&[u8]>) { | ||
| let mut s = s.splitn(2, |&b| b == b'.'); | ||
| let first = s.next().unwrap(); |
There was a problem hiding this comment.
Quickly scrolling through the code it's non-obvious that this panic is unreachable.
There was a problem hiding this comment.
split will always have something in the first "split" even if the predicate doesnt match or s is an empty slice, so the first next should always be Some here (unless I have missed something?), should I clarify that in a comment or "expect" or something?
| sign: NumericSign, | ||
| scale: u16, | ||
| weight: i16, | ||
| digits: Vec<i16>, |
There was a problem hiding this comment.
Do you see this get used through the FromStr and Display impls? digits does not get currently exposed (I'm curious, I like that we expose very little in the API). Would a future bigdecimal implementation too go through a formatted string?
There was a problem hiding this comment.
That is an oversight actually. The FromStr and Display can be used, but it is probably possible to implement directly between the types.
The reason I added the FromStr and Display is only because they have to be implemented here, where the type is defined (and ofc, it makes it easy to convert to most other types, since most other types have to/from string conversion).
Co-authored-by: Paolo Barbolini <[email protected]>
Co-authored-by: Paolo Barbolini <[email protected]>
Co-authored-by: Paolo Barbolini <[email protected]>
Co-authored-by: Paolo Barbolini <[email protected]>
|
Thanks for reviewing this! I'll wait with squashing until we are happy with the PR. Regarding tests in |
2 participants
A first stab at adding support for the Numeric type as well as a to/from implementation for string as that is kind of the only std type that can hold a numeric, and makes it a bit easier to verify correctness.
Would appreciate if you had the time to review this. Happy to fix any feedback. I'm thinking any ToSql and FromSql implementations can be added as followups to this one.
The protocol is quite strait forward, the string conversion is more finicky, but added a bunch of test to hopefully catch all corner cases. Let me know if you think there is any tests that should be added for those.
Partially solves #307 and #1223