Skip to content

8700: fix: liberal parsing of zero scale decimals #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

8700: fix: liberal parsing of zero scale decimals #4

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions arrow-cast/src/parse.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,6 +963,14 @@ pub fn parse_decimal<T: DecimalType>(
}

if !is_e_notation {
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 _));
Copy link

@augmentcode augmentcode bot Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

@martin-augment martin-augment Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

digits -= fractionals as u8;
fractionals = 0;
}
Comment on lines +966 to +972
Copy link

@coderabbitai coderabbitai bot Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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})")
EOF

Length 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 both Decimal128Type and Decimal256Type

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.


if fractionals < scale {
let exp = scale - fractionals;
if exp as u8 + digits > precision {
Expand Down Expand Up @@ -2636,6 +2644,7 @@ mod tests {
("12345678908765.123456", 3),
("123456789087651234.56e-4", 3),
("1234560000000", 0),
("12345678900.0", 0),
("1.23456e12", 0),
];
for (s, scale) in overflow_parse_tests {
Expand Down Expand Up @@ -2752,6 +2761,23 @@ mod tests {
let result = parse_decimal::<Decimal256Type>(s, 76, scale);
assert_eq!(i, result.unwrap());
}

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),
];
for (s, i) in zero_scale_tests {
let result_128 = parse_decimal::<Decimal128Type>(s, 3, 0).unwrap();
assert_eq!(i, result_128);
}
}

#[test]
Expand Down
Loading