Skip to content

Conversation

@liamzwbao
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

Code cleanup and optimization

What changes are included in this PR?

Addressed the post-comments in #8552 and refactor/optimize the method rescale_decimal

Are these changes tested?

Covered by existing tests

Are there any user-facing changes?

No

@github-actions github-actions bot added arrow Changes to the arrow crate parquet-variant parquet-variant* crates labels Oct 19, 2025
@liamzwbao liamzwbao force-pushed the issue-8477-decimal-followup branch from 0603e21 to 594cf52 Compare October 19, 2025 20:13
@liamzwbao liamzwbao marked this pull request as ready for review October 19, 2025 20:15
@liamzwbao
Copy link
Contributor Author

Hi @alamb @scovich, this is a followup PR to address the comments and refactor the code. PTAL, thanks!

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks like a nice improvement to me.

@scovich let me know whey you think this is ready to go and I'll merge it in

Copy link
Contributor

@scovich scovich left a comment

Choose a reason for hiding this comment

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

Nice! Couple small nits.

Comment on lines 220 to 224
let scaled = if is_infallible_cast {
Some(O::Native::from_decimal(value).unwrap().mul_wrapping(mul))
} else {
O::Native::from_decimal(value).and_then(|x| x.mul_checked(mul).ok())
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let scaled = if is_infallible_cast {
Some(O::Native::from_decimal(value).unwrap().mul_wrapping(mul))
} else {
O::Native::from_decimal(value).and_then(|x| x.mul_checked(mul).ok())
};
let value = O::Native::from_decimal(value);
let scaled = if is_infallible_cast {
Some(value.unwrap().mul_wrapping(mul))
} else {
value.and_then(|x| x.mul_checked(mul).ok())
};

(O::Native::from_decimal(adjusted), is_infallible_cast)
};

scaled.filter(|v| is_infallible_cast || O::is_valid_decimal_precision(*v, output_precision))
Copy link
Contributor

Choose a reason for hiding this comment

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

aside: I believe compiler "jump threading" optimizations should bypass the filter call when is_infallible_cast=true. Not sure how to verify that, tho.

Alternatively, we can drastically increase the probability of successful jump threading by defining mut scale and then:

Suggested change
scaled.filter(|v| is_infallible_cast || O::is_valid_decimal_precision(*v, output_precision))
if !is_infallible_cast {
scaled = scaled.filter(|v| O::is_valid_decimal_precision(*v, output_precision));
}
scaled

(if we pursue that, we may want to change the logic to is_fallible_cast to avoid error-prone double negative logic)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make it simple, how about this

    if is_infallible_cast {
        scaled
    } else {
        scaled.filter(|v| O::is_valid_decimal_precision(*v, output_precision))
    }

@alamb alamb merged commit 89846a8 into apache:main Oct 22, 2025
29 checks passed
@alamb
Copy link
Contributor

alamb commented Oct 22, 2025

Thanks @liamzwbao and @scovich

@liamzwbao liamzwbao deleted the issue-8477-decimal-followup branch October 22, 2025 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrow Changes to the arrow crate parquet-variant parquet-variant* crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants