[C++] Is Precision check necessary when appending values at decimal builder

[asfimport]

This is one issue found when I'm trying to enable generate_decimal128_case integration case for Rust at C++ repo: #13219.

Rust validates precision of decimal 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 in appending value at decimal builder as I did a search (e.g. Decimal128Builder, DecimalFromString), and I can confirm that test case can be passed if I remove the check in DecimalBuilder.append_value.

Actually in C++, similar check is done when doing a full validating ArrayData:

arrow/cpp/src/arrow/array/validate.cc

Line 672 in c715beb

Status ValidateDecimals(const DecimalType& type) {

.

I'm wondering if we should follow C++ at Rust implementation, i.e. removing the check from appending value at decimal builder, and moving the check to the full validation of ArrayData?

I opened a Rust PR (apache/arrow-rs#1767) for that.

Another opinion from the comments is, as we don't do full validation when finishing the decimal builder, it seems this check is necessary if it is the promise of the builder to always build a valid ArrayData. Then the check is needed and it seems C++ needs to add the check there.

Would like to hear some feedback here. Thanks.

Reporter: L. C. Hsieh

Related issues:

• [C++] Validate decimal data in ValidateFull (relates to)

_{Note: This issue was originally created as ARROW-16696. Please see the migration documentation for further details.}

[asfimport]

David Li / @lidavidm:
We skip the validation for this reason in the C++ tests now.

arrow/dev/archery/archery/integration/runner.py

Lines 154 to 160 in 5680d20

	if prefix in {'0.14.1', '0.17.1',
	'1.0.0-bigendian', '1.0.0-littleendian'}:
	# ARROW-13558: older versions generated decimal values that
	# were out of range for the given precision.
	quirks.add("no_decimal_validate")
	quirks.add("no_date64_validate")
	quirks.add("no_times_validate")

[asfimport]

David Li / @lidavidm:
I suppose in Rust parlance, the C++ builders are unsafe because they don't do the check. We could consider adding checked variants/adding an explicitly unchecked variant.

[asfimport]

L. C. Hsieh:
Thanks @lidavidm. Then it makes sense. I will keep this open for a while to get more comments if any.

[asfimport]

David Li / @lidavidm:
[~viirya] you may also find it useful to start a discussion on [email protected]