Conversation
lovasoa
left a comment
lovasoa
left a comment
There was a problem hiding this comment.
Thanks so much for this comprehensive PR! These are valuable additions to the MSSQL driver!
To help with maintainability and review process, could you please:
- Split into separate PRs - Each concern (dependency updates, SSL options docs, Money type support) would be easier to review and merge independently
- Eliminate code duplication - The Money parsing logic in
bigdecimal.rsanddecimal.rscould be extracted into a shared helper function - Add comprehensive tests - This is super important! Please ensure every new line of code has test coverage. You can look at existing tests for other mssql types.
The implementation looks solid technically - just want to make sure we have solid test coverage before merging. Thanks again for tackling these important improvements!
| DataType::MoneyN | DataType::Money | DataType::SmallMoney => { | ||
| // Money is stored as an 8-byte integer representing the amount in ten-thousandths of a currency unit | ||
| let bytes = value.as_bytes()?; | ||
| // println!("bytes: {:?}", bytes); |
sqlx-core/src/mssql/types/decimal.rs
Outdated
| DataType::MoneyN | DataType::Money | DataType::SmallMoney => { | ||
| // Money is stored as an 8-byte integer representing the amount in ten-thousandths of a currency unit | ||
| let bytes = value.as_bytes()?; | ||
| // println!("bytes: {:?}", bytes); | ||
| if bytes.len() != 8 && bytes.len() != 4 { | ||
| return Err( | ||
| err_protocol!("expected 8/4 bytes for Money, got {}", bytes.len()).into(), | ||
| ); | ||
| } | ||
| let amount: i64 = if bytes.len() == 8 { | ||
| let amount_h = i32::from_le_bytes(bytes[0..4].try_into()?) as i64; | ||
| let amount_l = u32::from_le_bytes(bytes[4..8].try_into()?) as i64; | ||
| (amount_h << 32) | amount_l | ||
| } else { | ||
| i32::from_le_bytes(bytes.try_into()?) as i64 | ||
| }; | ||
| Ok(Decimal::new(amount, 4)) |
There was a problem hiding this comment.
can we avoid duplicating the logic ?
Cargo.lock
Outdated
| # This file is automatically @generated by Cargo. | ||
| # It is not intended for manual editing. | ||
| version = 4 | ||
| version = 3 |
|
If the money type actually fits in either an i32 or an i64, we should probably implement Decode for these types, and then just code i64::decode from the decimal and bigdecimal implementations |
In MSSQL, the MONEY and SMALLMONEY types are specialized variants of DECIMAL with fixed decimal precision: |
|
Hi ! I'm okay to merge that without adding decoding to i64 or i32. But you still need to address my comments above before we can integrate your contribution:
|
|
close this PR and create a new one. |
2 participants
Refactor sqlx-core/src/mssql/types/bigdecimal.rs and sqlx-core/src/mssql/types/decimal.rs, Add Money type support to mssql