You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/05/31 02:19:42 UTC
[GitHub] [arrow-rs] viirya opened a new pull request, #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
viirya opened a new pull request, #1767:
URL: https://github.com/apache/arrow-rs/pull/1767
# Which issue does this PR close?
<!---
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #1766.
# Rationale for this change
<!---
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!---
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!---
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #1767: Optionally disable `validate_decimal_precision` check in `DecimalBuilder.append_value` for interop test
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#discussion_r888650741
##########
arrow/src/array/data.rs:
##########
@@ -999,6 +999,27 @@ impl ArrayData {
pub fn validate_dictionary_offset(&self) -> Result<()> {
match &self.data_type {
+ DataType::Decimal(p, _) => {
+ let values_buffer = &self.buffers[0];
+
+ for pos in 0..values_buffer.len() {
+ let raw_val = unsafe {
+ std::slice::from_raw_parts(
+ values_buffer.as_ptr().add(pos),
+ 16_usize,
+ )
+ };
+ let as_array = raw_val.try_into();
+ match as_array {
+ Ok(v) if raw_val.len() == 16 => {
Review Comment:
`data` is private. :disappointed:
I simplified it as:
```rust
for pos in 0..values_buffer.len() {
let raw_val = unsafe {
std::slice::from_raw_parts(
values_buffer.as_ptr().add(pos),
16_usize,
)
};
let value = i128::from_le_bytes(raw_val.try_into().unwrap());
validate_decimal_precision(value, *p)?;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#issuecomment-1145613279
> I think this would avoid having to add #[cfg(not(feature = "force_validate"))]
Oh, BTW, adding `#[cfg(not(feature = "force_validate"))]` to several tests is for the added check in ArrayData::validate, not due to removal of the check from DecimalBuilder.
These tests build ArrayData with incompatible decimal values. As this adds the check when full validation, these tests will fail during that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#issuecomment-1141848938
In C++, the similar check is done in validating ArrayData, not during appending values into the builder. I'm not sure which one is more correct under Arrow's context.
I'm trying to remove the check and (not committed yet) put the precision check into the validation of ArrayData (this is what C++ Arrow does).
In one way, as we don't do full validation when finishing the 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. I will open a ticket there and try to get some feedbacks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#issuecomment-1145043939
> I am a little worried about removing the validation from DecimalBuilder -- I think one of the philosophies we are shooting for is if you use safe APIs you will create valid Arrow arrays.
Hmm, seems at C++ implementation, the builder is unsafe API, in Rust parlance. This is based on the reply I got in the JIRA ticket. This seems the difference between Rust and C++ implementations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#issuecomment-1145145385
I wonder if this could be related to https://github.com/apache/arrow-rs/issues/1779 just filed by @tustvold 🤔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#issuecomment-1141812467
I'm not sure how I feel about this, the promise of the builders is they won't let you construct an ArrayData that would fail validation, and can therefore elide this validation. I think this change violates that?
Tbh this feels like a bug in the C++ implementation... Perhaps we should raise a ticket to at the very least get context?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1767: Optionally disable `validate_decimal_precision` check in `DecimalBuilder.append_value` for interop test
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#issuecomment-1146140192
Thanks @alamb @tustvold @nevi-me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#issuecomment-1145624525
> Generally it sounds good to me. But in the test, we are not using the builder directly. The builder is called by more higher API when reading JSON/Arrow files into Arrays. So seems we need more than adding an API like disable_value_validation to DecomalBuilder.
I'm wrong. Fortunately, for interop test, we directly invoke DecimalBuilder to read JSON file to Array. So adding `disable_value_validation` call there can fix the interop issue. Verified it locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #1767: Optionally disable `validate_decimal_precision` check in `DecimalBuilder.append_value` for interop test
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#discussion_r888822116
##########
arrow/src/array/data.rs:
##########
@@ -2707,4 +2723,36 @@ mod tests {
assert_eq!(array, &expected);
}
+
+ #[test]
+ #[cfg(not(feature = "force_validate"))]
+ fn test_decimal_full_validation() {
Review Comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#discussion_r885540606
##########
arrow/src/array/array_binary.rs:
##########
@@ -1497,38 +1497,6 @@ mod tests {
assert_eq!(16, decimal_array.value_length());
}
- #[test]
- fn test_decimal_append_error_value() {
Review Comment:
I recommend we change this test to show that it is ok to store these values in a decimal rather than removing it completely (aka change the test to validate there is no error).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#discussion_r885906397
##########
arrow/src/array/data.rs:
##########
@@ -999,6 +999,27 @@ impl ArrayData {
pub fn validate_dictionary_offset(&self) -> Result<()> {
match &self.data_type {
+ DataType::Decimal(p, _) => {
Review Comment:
C++ ArrayData full validation performs the precision check for decimal type. I think this is necessary to add even we don't remove `validate_decimal_precision` from `DecimalBuilder.append_value`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#discussion_r887825142
##########
arrow/src/array/data.rs:
##########
@@ -999,6 +999,27 @@ impl ArrayData {
pub fn validate_dictionary_offset(&self) -> Result<()> {
match &self.data_type {
+ DataType::Decimal(p, _) => {
+ let values_buffer = &self.buffers[0];
+
+ for pos in 0..values_buffer.len() {
+ let raw_val = unsafe {
+ std::slice::from_raw_parts(
+ values_buffer.as_ptr().add(pos),
+ 16_usize,
+ )
+ };
+ let as_array = raw_val.try_into();
+ match as_array {
+ Ok(v) if raw_val.len() == 16 => {
Review Comment:
For some reason this code seems overly complicated -- I wonder if you could call i128::from_le_bytes directly on a slice like
```rust
for pos in 0..values_buffer.len() {
let v = value_buffer.data[pos..pos+16];
let value = i128::from_le_bytes(v)
}
```
or something 🤔
##########
arrow/src/array/data.rs:
##########
@@ -999,6 +999,27 @@ impl ArrayData {
pub fn validate_dictionary_offset(&self) -> Result<()> {
match &self.data_type {
+ DataType::Decimal(p, _) => {
Review Comment:
I agree this code is necessary in `validate` 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#discussion_r885891338
##########
arrow/src/array/array_binary.rs:
##########
@@ -1497,38 +1497,6 @@ mod tests {
assert_eq!(16, decimal_array.value_length());
}
- #[test]
- fn test_decimal_append_error_value() {
Review Comment:
Changed the test. As I move the precision check to ArrayData full validation, I add another test for that too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb merged pull request #1767: Optionally disable `validate_decimal_precision` check in `DecimalBuilder.append_value` for interop test
Posted by GitBox <gi...@apache.org>.
alamb merged PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#issuecomment-1142403907
Thanks @alamb @nevi-me @tustvold. I've opened a ticket https://issues.apache.org/jira/browse/ARROW-16696 to hear some feedback there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#discussion_r885939424
##########
arrow/src/array/array_binary.rs:
##########
@@ -1486,7 +1486,7 @@ mod tests {
192, 219, 180, 17, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 64, 36, 75, 238, 253,
255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
];
- let array_data = ArrayData::builder(DataType::Decimal(23, 6))
+ let array_data = ArrayData::builder(DataType::Decimal(38, 6))
Review Comment:
Caught this invalid decimal value by decimal check in full validation. Increasing precision to pass it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter commented on pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#issuecomment-1141623446
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1767?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1767](https://codecov.io/gh/apache/arrow-rs/pull/1767?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e2ad586) into [master](https://codecov.io/gh/apache/arrow-rs/commit/5cf06bf5abacd4d3aacdb12daa4845eaf90d04cb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5cf06bf) will **increase** coverage by `0.15%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #1767 +/- ##
==========================================
+ Coverage 83.32% 83.47% +0.15%
==========================================
Files 196 196
Lines 55961 55925 -36
==========================================
+ Hits 46627 46683 +56
+ Misses 9334 9242 -92
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1767?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/array/array\_binary.rs](https://codecov.io/gh/apache/arrow-rs/pull/1767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X2JpbmFyeS5ycw==) | `93.05% <ø> (-0.26%)` | :arrow_down: |
| [arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/1767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIucnM=) | `86.79% <ø> (-0.01%)` | :arrow_down: |
| [parquet/src/util/cursor.rs](https://codecov.io/gh/apache/arrow-rs/pull/1767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvdXRpbC9jdXJzb3IucnM=) | `63.86% <0.00%> (-13.45%)` | :arrow_down: |
| [parquet/src/util/io.rs](https://codecov.io/gh/apache/arrow-rs/pull/1767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvdXRpbC9pby5ycw==) | `88.33% <0.00%> (-1.67%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `65.53% <0.00%> (-0.46%)` | :arrow_down: |
| [parquet/src/file/writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZmlsZS93cml0ZXIucnM=) | `92.85% <0.00%> (-0.45%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9tb2QucnM=) | `86.85% <0.00%> (-0.12%)` | :arrow_down: |
| [arrow/src/array/array\_struct.rs](https://codecov.io/gh/apache/arrow-rs/pull/1767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X3N0cnVjdC5ycw==) | `88.40% <0.00%> (-0.05%)` | :arrow_down: |
| [...arquet/src/arrow/array\_reader/dictionary\_buffer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyL2RpY3Rpb25hcnlfYnVmZmVyLnJz) | `94.44% <0.00%> (-0.04%)` | :arrow_down: |
| [parquet/src/arrow/array\_reader/offset\_buffer.rs](https://codecov.io/gh/apache/arrow-rs/pull/1767/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvYXJyYXlfcmVhZGVyL29mZnNldF9idWZmZXIucnM=) | `94.79% <0.00%> (-0.03%)` | :arrow_down: |
| ... and [19 more](https://codecov.io/gh/apache/arrow-rs/pull/1767/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1767?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1767?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5cf06bf...e2ad586](https://codecov.io/gh/apache/arrow-rs/pull/1767?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#issuecomment-1142035936
> In one way, as we don't do full validation when finishing the 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. I will open a ticket there and try to get some feedbacks.
I think this is the right thing to do -- thank you @viirya
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on a diff in pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#discussion_r886093014
##########
arrow/src/ipc/writer.rs:
##########
@@ -1130,6 +1131,7 @@ mod tests {
}
#[test]
+ #[cfg(not(feature = "force_validate"))]
Review Comment:
I think these tests use the same 0.14.1 decimal file too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#issuecomment-1145053554
> What would you think about making validation optional for the builder?
> I think this would avoid having to add #[cfg(not(feature = "force_validate"))] and it would allow the interop test to bypass value validation only when needed?
Generally it sounds good to me. But in the test, we are not using the builder directly. The builder is called by more higher API when reading JSON/Arrow files into Arrays. So seems we need more than adding an API like `disable_value_validation` to `DecomalBuilder`.
Let me try to play it around and see if an optional validation of the builder is possible to solve the interop test issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on pull request #1767: Remove `validate_decimal_precision` check in `DecimalBuilder.append_value`
Posted by GitBox <gi...@apache.org>.
viirya commented on PR #1767:
URL: https://github.com/apache/arrow-rs/pull/1767#issuecomment-1145194960
Hmm, I think it is not directly related. C++ also does this check but it does in full validation, not in the builder. Actually C++ Arrow avoids the full validation too in the interop to avoid similar issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org