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

Doctests for DecimalArray. #414

Merged
merged 3 commits into from
Jun 14, 2021
Merged

Doctests for DecimalArray. #414

merged 3 commits into from
Jun 14, 2021

Conversation

novemberkilo
Copy link
Contributor

Which issue does this PR close?

re #301

What changes are included in this PR?

Doctests only

Are there any user-facing changes?

No

@novemberkilo
Copy link
Contributor Author

cc/ @alamb

/// assert_eq!(-8_887_000_000, decimal_array.value(1));
/// assert_eq!(16, decimal_array.value_length());
/// assert_eq!(23, decimal_array.precision());
/// assert_eq!(6, decimal_array.scale());
Copy link
Contributor

Choose a reason for hiding this comment

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

@alippai has an improvement for string formatting decimal values in #406

If he adds the method to DecimalArray in #406 (comment) it would be neat to also add it to the doc test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alamb I have followed your suggestion to replace the example with one that uses DecimalBuilder instead. If I am following correctly, I won't be able to really use any of #406 there will I?

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll be able to add assert_eq!("8887.000000", decimal_array.value_as_string(0)); when I finish the PR. Also I'd keep the assert_eq!(8_887_000_000, decimal_array.value(0)); example (to show the values are i128 values)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @alippai -- I will hold off updating this PR until #406 is in // @alamb

Copy link
Contributor

Choose a reason for hiding this comment

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

@novemberkilo now it's merged, thanks for your patience

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @alippai - I have added those doctests per your suggestion. // @alamb

/// # Examples
///
/// ```
/// use arrow::array::{ArrayData, Array, DecimalArray};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think using the DecimalBuilder to construct a DecimalArray may make for a better example:

For example, here: https://github.com/apache/arrow-rs/blob/master/arrow/src/array/builder.rs#L2856

@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2021

Codecov Report

Merging #414 (fda84cf) into master (e21f576) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head fda84cf differs from pull request most recent head 2233ab2. Consider uploading reports for the commit 2233ab2 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
- Coverage   82.64%   82.64%   -0.01%     
==========================================
  Files         164      164              
  Lines       45508    45508              
==========================================
- Hits        37609    37608       -1     
- Misses       7899     7900       +1     
Impacted Files Coverage Δ
arrow/src/array/array_binary.rs 90.13% <ø> (ø)
parquet/src/encodings/encoding.rs 94.85% <0.00%> (-0.20%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e21f576...2233ab2. Read the comment docs.

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.

Looks great -- thank you @novemberkilo

///
/// assert_eq!(&DataType::Decimal(23, 6), decimal_array.data_type());
/// assert_eq!(8_887_000_000, decimal_array.value(0));
/// assert_eq!("8887.000000", decimal_array.value_as_string(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@alamb alamb merged commit d41ca5f into apache:master Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants