Skip to content
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.

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

precision is not considered when cast value to decimal #3223

Closed
liukun4515 opened this issue Nov 29, 2022 · 6 comments · Fixed by #3242
Closed

precision is not considered when cast value to decimal #3223

liukun4515 opened this issue Nov 29, 2022 · 6 comments · Fixed by #3242
Labels
arrow Changes to the arrow crate enhancement Any new improvement worthy of a entry in the changelog

Comments

@liukun4515
Copy link
Contributor

Describe the bug

If the CastOptions {safe: true} and cast INT32(120) to decimal(3,1), the result should be None, but
we get the result of decimal(Some(1200),3,1)

To Reproduce

Expected behavior

Additional context

@liukun4515 liukun4515 added the bug label Nov 29, 2022
@liukun4515
Copy link
Contributor Author

Do you have any thoughts about this? cc @tustvold @viirya

@tustvold
Copy link
Contributor

I thought we had agreed to only care about overflow of the underlying type, and leave it to users to validate decimals if they care about precision. Provided we aren't overflowing and losing data, I'm not sure why we would validate precision at all?

@liukun4515
Copy link
Contributor Author

I thought we had agreed to only care about overflow of the underlying type, and leave it to users to validate decimals if
they care about precision. Provided we aren't overflowing and losing data, I'm not sure why we would validate precision at all?

Now arrow-rs has the API of pub fn validate_decimal_precision(&self, precision: u8) -> Result<(), ArrowError> to check the precision for the decimal array.
I think we need a method to do the above requirements, if the value is overflow the precision, and it should be converted to None.
Maybe we should provide the api to change the null_bit_map in the DecimalArray, If user want to validate precision to get None or Error.

@tustvold

@tustvold
Copy link
Contributor

Adding a null_if_overflowed_precision method makes sense to me

@viirya
Copy link
Member

viirya commented Nov 29, 2022

Currently we did as @tustvold mentioned. We validate decimals by validate_decimal_precision after casting and put nulls for overflowing slots in an array.

I think it is trivial to do by users. Okay for me if you want to add something like null_if_overflowed_precision to do that.

@liukun4515
Copy link
Contributor Author

@viirya @tustvold thanks for your advice.
In the user case, some cases want to get the error when the data is overflow for the precision, and some cases don't want to get the error instead of the None for the overflowed slot in the array.

@liukun4515 liukun4515 added enhancement Any new improvement worthy of a entry in the changelog and removed bug labels Nov 30, 2022
@alamb alamb added the arrow Changes to the arrow crate label Dec 1, 2022
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 enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants