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

Remove validate_decimal_precision check in DecimalBuilder.append_value #1766

Closed
viirya opened this issue May 31, 2022 · 0 comments · Fixed by #1767
Closed

Remove validate_decimal_precision check in DecimalBuilder.append_value #1766

viirya opened this issue May 31, 2022 · 0 comments · Fixed by #1767
Labels
development-process Related to development process of arrow-rs enhancement Any new improvement worthy of a entry in the changelog

Comments

@viirya
Copy link
Member

viirya commented May 31, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

This is one issue found by enabling generate_decimal128_case integration case at C++ repo (apache/arrow#13219).

This crate validates precision of decimal by validate_decimal_precision in few places, e.g. DecimalBuilder's append_value. However, in decimal golden file 0.14.1_decimal.gold.json, there are some values failing this check. C++ doesn't perform similar check as I did a search (e.g. Decimal128Builder, DecimalFromString), and I can confirm that test case can be passed if I remove validate_decimal_precision check in append_value.

I'm wondering if we should remove the check to pass the test case? Or leaving generate_decimal128_case as skipped test case at the integration test?

Actually in C++, similar check is done when doing a full validating ArrayData: https://github.com/apache/arrow/blob/c715bebbd89089f385c9996560866da23ea1ddda/cpp/src/arrow/array/validate.cc#L672. We may move the check to validating ArrayData too.

Describe the solution you'd like

Remove validate_decimal_precision check in DecimalBuilder's append_value to pass the integration test case generate_decimal128_case.

Describe alternatives you've considered

Keep generate_decimal128_case as skipped test case

Additional context
Add any other context or screenshots about the feature request here.

@viirya viirya added the enhancement Any new improvement worthy of a entry in the changelog label May 31, 2022
@viirya viirya changed the title Remove Remove validate_decimal_precision check in DecimalBuilder.append_value May 31, 2022
@alamb alamb added the development-process Related to development process of arrow-rs label Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development-process Related to development process of arrow-rs enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
2 participants