test: add tests for more primitive types#5173
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
This is stacked on #4745. To review, just look at the last three commits, which are mostly in one file. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Convert integer to decimal by multiplying by 10^scale | ||
| DataType::Decimal128(precision, scale) => val.map(|v| { | ||
| let scaled_value = (v as i128) * 10_i128.pow(*scale as u32); | ||
| ScalarValue::Decimal128(Some(scaled_value), *precision, *scale) | ||
| }), | ||
| DataType::Decimal256(precision, scale) => val.and_then(|v| { | ||
| // Convert i64 to i256 and scale | ||
| // First compute the scale factor as i128, then build i256 | ||
| let scale_factor = 10_i128.pow(*scale as u32); | ||
| // Convert the integer value to i256 and multiply by scale factor | ||
| let v_i256 = i256::from(v); | ||
| // Convert i128 to i256 by converting to/from bytes | ||
| let scale_bytes = scale_factor.to_le_bytes(); | ||
| let mut i256_bytes = [0u8; 32]; | ||
| // Copy the i128 bytes into the lower 16 bytes of i256 | ||
| i256_bytes[..16].copy_from_slice(&scale_bytes); | ||
| // Sign extend if negative | ||
| if scale_factor < 0 { | ||
| i256_bytes[16..].fill(0xFF); |
There was a problem hiding this comment.
The new Decimal256 branch in safe_coerce_scalar builds its scale multiplier with let scale_factor = 10_i128.pow(*scale as u32) before converting to i256. Decimal256 columns allow scales up to 76, but any scale greater than 38 exceeds the range of i128, so this call will overflow (panic in debug, wrap in release) and produce an invalid scalar whenever a high-scale Decimal256 column is compared with an integer literal. The multiplier should be constructed directly as an i256 (or checked for overflow) instead of first materializing it as an i128.
Useful? React with 👍 / 👎.
wjones127
force-pushed
the
primitive-tests
branch
3 times, most recently
from
d35cafd to
d37f4c7
Compare
wjones127
force-pushed
the
primitive-tests
branch
from
d37f4c7 to
8e53491
Compare
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
looks good to me, sorry for the late review 🙏
Covers floats, strings, binary, decimals, dates, and timestamps. Closes lance-format#4933
3 participants
Covers floats, strings, binary, decimals, dates, and timestamps.
Closes #4933