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

Disable value validation for Decimal256 case #2232

Merged
merged 1 commit into from
Jul 30, 2022

Conversation

viirya
Copy link
Member

@viirya viirya commented Jul 29, 2022

Which issue does this PR close?

Closes #2233.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@viirya
Copy link
Member Author

viirya commented Jul 29, 2022

cc @alamb

@codecov-commenter
Copy link

Codecov Report

Merging #2232 (738b405) into master (82e0512) will decrease coverage by 0.31%.
The diff coverage is 61.13%.

❗ Current head 738b405 differs from pull request most recent head fd59a76. Consider uploading reports for the commit fd59a76 to get more accurate results

@@            Coverage Diff             @@
##           master    #2232      +/-   ##
==========================================
- Coverage   82.62%   82.31%   -0.32%     
==========================================
  Files         239      240       +1     
  Lines       62308    62446     +138     
==========================================
- Hits        51482    51400      -82     
- Misses      10826    11046     +220     
Impacted Files Coverage Δ
arrow-flight/examples/flight_sql_server.rs 0.00% <0.00%> (ø)
arrow-flight/src/sql/server.rs 0.00% <0.00%> (ø)
arrow/src/datatypes/schema.rs 70.41% <0.00%> (-3.05%) ⬇️
integration-testing/src/lib.rs 0.00% <0.00%> (ø)
object_store/src/aws.rs 0.00% <0.00%> (ø)
object_store/src/azure.rs 0.00% <0.00%> (ø)
object_store/src/gcp.rs 0.00% <0.00%> (ø)
...et/src/arrow/array_reader/byte_array_dictionary.rs 86.97% <0.00%> (+0.30%) ⬆️
parquet/src/arrow/array_reader/null_array.rs 81.48% <0.00%> (+2.91%) ⬆️
parquet/src/encodings/rle.rs 92.42% <0.00%> (-0.37%) ⬇️
... and 27 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

@@ -618,6 +618,10 @@ fn array_from_json(
}
DataType::Decimal256(precision, scale) => {
let mut b = Decimal256Builder::new(json_col.count, *precision, *scale);
// C++ interop tests involve incompatible decimal values
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@viirya
Hi java or c++ version, Does the decimal builder or the decimal array has the validation for the input value?
If there is no validation, how to make sure all of the element of the decimal array are valid?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like Decimal128 case #1766, C++ doesn't perform validation in Decimal builders. It only does validation when doing a full validating ArrayData.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @viirya

I wonder if we should file a follow on ticket to track the incompatible value in the C++ integration test? Something doesn't seem right there -- if the integration test is using incompatible values perhaps someone should fix the C++ test

(btw we can see the integration test failing on arrow mater here: https://github.com/apache/arrow-rs/runs/7588317417?check_suite_focus=true and it passes with your PR 🎉 )

@alamb alamb merged commit ca43719 into apache:master Jul 30, 2022
@alamb alamb added the arrow Changes to the arrow crate label Jul 30, 2022
@ursabot
Copy link

ursabot commented Jul 30, 2022

Benchmark runs are scheduled for baseline = d727618 and contender = ca43719. ca43719 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@viirya
Copy link
Member Author

viirya commented Jul 30, 2022

Thanks @alamb! Yea, let me create a ticket for the C++ integration test.

Sorry, after rechecking it, I made a mistake when filling maximum and minimum decimal values for precision > 38. So for generate_decimal256_case it is a false alarm. Sorry for confusion.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable value validation for Decimal256 case
5 participants