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

Move DecimalArray to array_decimal.rs #1986

Merged
merged 1 commit into from
Jul 1, 2022

Conversation

HaoYang670
Copy link
Contributor

@HaoYang670 HaoYang670 commented Jul 1, 2022

Signed-off-by: remzi 13716567376yh@gmail.com

Which issue does this PR close?

Closes #1985.

Rationale for this change

  1. DecimalArray is a top level logical type: https://github.com/apache/arrow/blob/master/format/Schema.fbs#L405
  2. DecimalArray has no relationship with binary arrays: Personally, I don't think the decimal array is a logical type of FixedSizeBinaryArray because the value_length is a constant (128bits or 256bits) but not user-defined.
  3. If we add Decimal256Array support in the future, putting the code in array_decimal.rs could be more readable.

What changes are included in this PR?

Are there any user-facing changes?

Yes.

Signed-off-by: remzi <13716567376yh@gmail.com>
@github-actions github-actions bot added the arrow Changes to the arrow crate label Jul 1, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1986 (6595119) into master (4350b8c) will increase coverage by 0.00%.
The diff coverage is 93.67%.

@@           Coverage Diff           @@
##           master    #1986   +/-   ##
=======================================
  Coverage   83.57%   83.58%           
=======================================
  Files         221      222    +1     
  Lines       57495    57495           
=======================================
+ Hits        48054    48055    +1     
+ Misses       9441     9440    -1     
Impacted Files Coverage Δ
arrow/src/array/array_binary.rs 94.20% <ø> (+0.16%) ⬆️
arrow/src/array/mod.rs 100.00% <ø> (ø)
arrow/src/array/array_decimal.rs 93.67% <93.67%> (ø)
parquet_derive/src/parquet_field.rs 65.75% <0.00%> (-0.23%) ⬇️
parquet/src/encodings/encoding.rs 93.62% <0.00%> (+0.19%) ⬆️
arrow/src/datatypes/datatype.rs 65.79% <0.00%> (+0.37%) ⬆️

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 4350b8c...6595119. Read the comment docs.

@liukun4515 liukun4515 added the api-change Changes to the arrow API label Jul 1, 2022
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

I don't think this is an API change as the module hierarchy isn't public

@tustvold tustvold removed the api-change Changes to the arrow API label Jul 1, 2022
@tustvold tustvold merged commit 7ae97c9 into apache:master Jul 1, 2022
@HaoYang670 HaoYang670 deleted the move_decimal_array branch July 1, 2022 21:55
@HaoYang670
Copy link
Contributor Author

Thank you for your review @tustvold @liukun4515!

@liukun4515
Copy link
Contributor

LGTM! 👍

I don't think this is an API change as the module hierarchy isn't public

Sorry for this error label.
I did't review the mode file carefully for the decimal about pub use self::array_decimal::DecimalArray;

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.

Move DecimalArray to a new file
4 participants