Skip to content

fix: do not overflow in unwrap_cast for decimals with a negative scale - #24897

Closed
edubraqd wants to merge 1 commit into
apache:mainfrom
edubraqd:fix/unwrap-cast-negative-scale
Closed

edubraqd wants to merge 1 commit into
apache:mainfrom
edubraqd:fix/unwrap-cast-negative-scale

Conversation

@edubraqd

@edubraqd edubraqd commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

try_cast_numeric_literal computes 10_i128.pow(scale as u32) for both the literal's and the target's decimal scale. A negative scale wraps to a huge exponent, so simplifying arrow_cast(1, 'Decimal128(10, -2)') = 100 panicked with "attempt to multiply with overflow" in the unwrap_cast simplifier.

What changes are included in this PR?

A negative scale means the decimal holds multiples of 10^-scale, so the integer rescaling this path relies on does not apply. try_cast_numeric_literal now returns None when either side has a negative scale, so the simplifier leaves the cast in place, the same way it already does for other casts it cannot express.

Are these changes tested?

Yes. test_try_cast_to_type_negative_scale_decimal covers a negative-scale target for Decimal32/Decimal64/Decimal128 and a negative-scale literal; each used to panic.

Are there any user-facing changes?

No panic. Such comparisons are no longer simplified by unwrap_cast; they were never simplified correctly before, since the process panicked.

`try_cast_numeric_literal` computes `10_i128.pow(scale as u32)` for both the
literal's and the target's decimal scale. A negative scale wraps to a huge
exponent, so simplifying an expression such as

    arrow_cast(1, 'Decimal128(10, -2)') = 100

panicked with "attempt to multiply with overflow" in the `unwrap_cast`
simplifier.

A negative scale means the decimal holds multiples of `10^-scale`, so the
integer rescaling this path relies on does not apply. Return `None` for such
casts so the simplifier leaves them in place.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@kosiew

kosiew commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

@edubraqd

When I ran this in datafusion-cli (this PR):

SELECT arrow_cast(1, 'Decimal128(10, -2)') = 100;

I got

DataFusion CLI v55.0.0
+--------------------------------------------------------------+
| arrow_cast(Int64(1),Utf8("Decimal128(10, -2)")) = Int64(100) |
+--------------------------------------------------------------+
| false                                                        |
+--------------------------------------------------------------+

Shouldn't it be true?

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.63%. Comparing base (d02bd78) to head (62269ce).
⚠️ Report is 72 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/expr-common/src/casts.rs 96.96% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #24897   +/-   ##
=======================================
  Coverage   81.63%   81.63%           
=======================================
  Files        1123     1123           
  Lines      409546   409579   +33     
  Branches   409546   409579   +33     
=======================================
+ Hits       334319   334350   +31     
- Misses      55591    55596    +5     
+ Partials    19636    19633    -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alamb
alamb marked this pull request as draft September 8, 2026 19:46
@alamb

alamb commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look

@alamb

alamb commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Closing per #24893 (comment)

@alamb alamb closed this Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Comparing a decimal with a negative scale to an integer literal panics in unwrap_cast

4 participants