[
https://issues.apache.org/jira/browse/ARROW-16696?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
L. C. Hsieh updated ARROW-16696:
--------------------------------
Description:
This is one issue found when I'm trying to enable generate_decimal128_case
integration case for Rust at C++ repo:
https://github.com/apache/arrow/pull/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:
https://github.com/apache/arrow/blob/c715bebbd89089f385c9996560866da23ea1ddda/cpp/src/arrow/array/validate.cc#L672.
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 (https://github.com/apache/arrow-rs/pull/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.
was:
This is one issue found when I'm trying to enable generate_decimal128_case
integration case for Rust at C++ repo:
https://github.com/apache/arrow/pull/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 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:
https://github.com/apache/arrow/blob/c715bebbd89089f385c9996560866da23ea1ddda/cpp/src/arrow/array/validate.cc#L672.
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 (https://github.com/apache/arrow-rs/pull/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.
> [C++] Is Precision check necessary when appending values at decimal builder
> ---------------------------------------------------------------------------
>
> Key: ARROW-16696
> URL: https://issues.apache.org/jira/browse/ARROW-16696
> Project: Apache Arrow
> Issue Type: Improvement
> Reporter: L. C. Hsieh
> Priority: Major
>
> This is one issue found when I'm trying to enable generate_decimal128_case
> integration case for Rust at C++ repo:
> https://github.com/apache/arrow/pull/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:
> https://github.com/apache/arrow/blob/c715bebbd89089f385c9996560866da23ea1ddda/cpp/src/arrow/array/validate.cc#L672.
> 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 (https://github.com/apache/arrow-rs/pull/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.
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
