8700: fix: liberal parsing of zero scale decimals #4
Conversation
WalkthroughThe parse_decimal function now correctly handles zero-scale decimals by discarding fractional digits from the parsed result through division by 10^fractionals when scale is zero. Updated and new test cases validate this behavior for Decimal128 and Decimal256 types. Changes
Sequence DiagramsequenceDiagram
participant Parser as parse_decimal()
participant Accum as accumulated result
participant Scale as scale parameter
rect rgb(240, 248, 255)
Note over Parser: Parse decimal string
Parser->>Parser: Accumulate digits (including fractional)
end
alt is_e_notation
Parser->>Parser: Handle exponential notation
else not is_e_notation AND scale == 0 AND fractionals > 0
rect rgb(255, 240, 245)
Note over Parser: Zero-scale fractional handling
Parser->>Accum: Divide by 10^fractionals
Parser->>Parser: Clear fractionals counter
Parser->>Parser: Update digits tracking
end
else default
Parser->>Parser: Keep fractional precision
end
Parser->>Parser: Return parsed decimal value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The change modifies core parsing logic in a single file combined with multiple test additions. Review requires understanding the decimal parsing flow, verifying the fractional digit ejection logic is correct, and confirming test cases adequately cover the new zero-scale behavior across different decimal types. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
PR Review: Fix Liberal Parsing of Zero Scale DecimalsSummaryThis PR adds support for liberal parsing of decimal values with scale=0, allowing fractional digits to be present in the input string (e.g., "123.45") and automatically truncating them to produce the integer value (123). Code Quality & Best Practices ✅Strengths:
Suggestions:
Potential Bugs & Issues
|
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| if scale == 0 && fractionals > 0 { | ||
| // The input string contained some fractional digits after the decimal point despite | ||
| // the scale being zero. Eject all the fractional digits from the number. | ||
| result = result.div_wrapping(base.pow_wrapping(fractionals as _)); |
There was a problem hiding this comment.
With scale == 0, this builds the entire fractional part into result and only then divides it out. For inputs with many fractional digits this can overflow/wrap the intermediate value (and counters), yielding an incorrect final value even though the post-division digits check passes.
🤖 React with 👍 or 👎 to let us know if the comment was useful.
There was a problem hiding this comment.
value:delightful; category:bug; feedback:The AI assistant is correct that the wrapping would lead to problems if there are more than 10 fractional digits if base is 32bit integer or more than 20 fractional digits if the base is 64bit integer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
arrow-cast/src/parse.rs (1)
2765-2780: Expand test coverage for zero-scale decimals.The tests validate the zero-scale handling logic, but there are two coverage gaps:
- Missing Decimal256Type tests: Earlier tests in this function consistently validate both
Decimal128TypeandDecimal256Type, butzero_scale_testsonly testsDecimal128Type.- Missing negative number tests: No validation for negative inputs like
"-1.23"or"-123.45"with scale 0.Consider enhancing the test coverage:
let zero_scale_tests = [ ("1.0", 1), ("1.2", 1), ("1.00", 1), ("1.23", 1), ("1.000", 1), ("1.123", 1), ("123.0", 123), ("123.4", 123), ("123.00", 123), ("123.45", 123), + ("-1.0", -1), + ("-1.23", -1), + ("-123.45", -123), ]; for (s, i) in zero_scale_tests { let result_128 = parse_decimal::<Decimal128Type>(s, 3, 0).unwrap(); assert_eq!(i, result_128); + let result_256 = parse_decimal::<Decimal256Type>(s, 3, 0).unwrap(); + assert_eq!(i256::from_i128(i), result_256); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
arrow-cast/src/parse.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Test
- GitHub Check: Build wasm32
- GitHub Check: Test
- GitHub Check: Check Compilation
- GitHub Check: PySpark Integration Test (stable)
- GitHub Check: Verify MSRV (Minimum Supported Rust Version)
- GitHub Check: Test on Windows
- GitHub Check: Clippy
- GitHub Check: MIRI
- GitHub Check: Test
- GitHub Check: Test on Mac
- GitHub Check: Archery test With other arrows
- GitHub Check: Rustdocs are clean
- GitHub Check: codex
- GitHub Check: claude-review
| if scale == 0 && fractionals > 0 { | ||
| // The input string contained some fractional digits after the decimal point despite | ||
| // the scale being zero. Eject all the fractional digits from the number. | ||
| result = result.div_wrapping(base.pow_wrapping(fractionals as _)); | ||
| digits -= fractionals as u8; | ||
| fractionals = 0; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Core logic looks correct for zero-scale decimal handling.
The implementation correctly discards fractional digits by dividing the result by 10^fractionals when scale is zero. The integer division properly truncates toward zero for both positive and negative values.
Please verify that the new behavior works correctly with negative numbers (e.g., "-1.23" with scale 0 should parse to -1). Consider adding test cases for negative numbers in the zero_scale_tests section to ensure comprehensive coverage.
🏁 Script executed:
#!/bin/bash
# Verify zero-scale decimal parsing with negative numbers
cat << 'EOF' | python
from decimal import Decimal
test_cases = [
("-1.0", -1),
("-1.2", -1),
("-1.9", -1),
("-123.45", -123),
("-0.99", 0),
]
print("Expected behavior for negative zero-scale decimals:")
for s, expected in test_cases:
d = Decimal(s)
actual = int(d)
status = "✓" if actual == expected else "✗"
print(f"{status} '{s}' -> {actual} (expected {expected})")
EOFLength of output: 202
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the actual test cases in zero_scale_tests section
cat arrow-cast/src/parse.rs | sed -n '2765,2800p'
Length of output: 1236
🌐 Web query:
Rust div_wrapping integer division truncation toward zero
💡 Result:
Short answer: Rust's integer division (/) truncates toward zero. wrapping_div just computes self / rhs; for unsigned types that's identical to normal division, and for signed types it only "wraps" in the single overflow case MIN / -1 (it returns MIN). [1][2][3]
Examples:
- 7i32 / 2 == 3 (truncates toward zero). [1]
- For unsigned: 100u32.wrapping_div(10) == 10 (same as /). [3]
- For signed edge case: (-128i8).wrapping_div(-1) == -128 (wrap/return MIN instead of overflowing). [2]
Sources:
- Div behavior (rounds toward zero). [1]
- i32.wrapping_div documentation (MIN / -1 behavior). [2]
- u32.wrapping_div documentation (wrapping == normal division for unsigned). [3]
Add test cases for negative numbers and Decimal256Type to zero_scale_tests.
The core logic correctly truncates toward zero for negative numbers (confirmed: Rust's div_wrapping truncates toward zero, matching the expected behavior). However, the test coverage at lines 2765-2780 is incomplete:
- Missing negative test cases (e.g.,
"-1.2"should parse to-1,"-123.45"to-123) - Only tests
Decimal128Type, but the implementation supports bothDecimal128TypeandDecimal256Type
Expand the test cases to include:
- Negative numbers:
("-1.0", -1),("-1.2", -1),("-123.45", -123),("-0.99", 0) - Both decimal types
🤖 Prompt for AI Agents
In arrow-cast/src/parse.rs around lines 966 and update tests at lines
~2765-2780: add missing negative-number test cases to zero_scale_tests and run
them for both Decimal128Type and Decimal256Type; specifically add cases for
("-1.0", -1), ("-1.2", -1), ("-123.45", -123), and ("-0.99", 0) and ensure the
test harness asserts parsing/truncation toward zero for each decimal type so
both Decimal128Type and Decimal256Type are covered.
|
8700: To review by AI
Summary by CodeRabbit
Bug Fixes
Tests