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/01/23 12:25:42 UTC
[GitHub] [arrow-rs] alamb opened a new pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
alamb opened a new pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223
# Which issue does this PR close?
Closes https://github.com/apache/arrow-rs/issues/1009
Closes https://github.com/apache/arrow-rs/issues/1083
# Rationale for this change
It is somewhat awkward to create `DecimalArray` as well as iterate through their values.
This leads to (repeated) code like `create_decimal_array` https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/apache/arrow-rs%24+create_decimal_array&patternType=literal
```rust
fn create_decimal_array(
array: &[Option<i128>],
precision: usize,
scale: usize,
) -> DecimalArray {
let mut decimal_builder = DecimalBuilder::new(array.len(), precision, scale);
for value in array {
match value {
None => {
decimal_builder.append_null().unwrap();
}
Some(v) => {
decimal_builder.append_value(*v).unwrap();
}
}
}
decimal_builder.finish()
}
```
(there is similar repetition in datafusion)
# What changes are included in this PR?
1. Add `DecimalArray::from` and `DecimalArray::from_iter_values` (mirroring `PrimitiveArray`)
2. Add `DecimalArray::into_iter()` and `DecimalArray::iter()` for iterating through values
3. Add `DecimalArray::with_precision_and_scale()` for changing the relevant precision and scale
2. Add documentation
3. Refactor some existing code to show how the new APIs can be used
# Are there any user-facing changes?
1. Nicer APIs for creating and working with `DecimalArray`s
--
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 change in pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#discussion_r790269438
##########
File path: arrow/src/array/array_binary.rs
##########
@@ -816,13 +827,80 @@ impl DecimalArray {
let array_data = unsafe { builder.build_unchecked() };
Self::from(array_data)
}
+
+ /// Creates a [DecimalArray] with default precision and scale,
+ /// based on an iterator of `i128` values without nulls
+ pub fn from_iter_values<I: IntoIterator<Item = i128>>(iter: I) -> Self {
+ let val_buf: Buffer = iter.into_iter().collect();
+ let data = unsafe {
+ ArrayData::new_unchecked(
+ Self::default_type(),
+ val_buf.len() / std::mem::size_of::<i128>(),
+ None,
+ None,
+ 0,
+ vec![val_buf],
+ vec![],
+ )
+ };
+ DecimalArray::from(data)
+ }
+
+ /// Return the precision (total digits) that can be stored by this array
pub fn precision(&self) -> usize {
self.precision
}
+ /// Return the scale (digits after the decimal) that can be stored by this array
pub fn scale(&self) -> usize {
self.scale
}
+
+ /// Returns a DecimalArray with the same data as self, with the
+ /// specified precision.
+ ///
+ /// panic's if:
+ /// 1. the precision is larger than [`DECIMAL_MAX_PRECISION`]
+ /// 2. scale is larger than [`DECIMAL_MAX_SCALE`];
+ /// 3. scale is > precision
+ pub fn with_precision_and_scale(mut self, precision: usize, scale: usize) -> Self {
Review comment:
I also thought about an API that returns `Result` rather than `panic` - any thoughts from reviewers?
##########
File path: arrow/src/array/array_binary.rs
##########
@@ -690,19 +693,27 @@ impl Array for FixedSizeBinaryArray {
}
}
-/// A type of `DecimalArray` whose elements are binaries.
+/// `DecimalArray` stores fixed width decimal numbers,
+/// with a fixed precision and scale.
///
/// # Examples
///
/// ```
/// use arrow::array::{Array, DecimalArray, DecimalBuilder};
/// use arrow::datatypes::DataType;
-/// let mut builder = DecimalBuilder::new(30, 23, 6);
///
-/// builder.append_value(8_887_000_000).unwrap();
-/// builder.append_null().unwrap();
-/// builder.append_value(-8_887_000_000).unwrap();
-/// let decimal_array: DecimalArray = builder.finish();
+/// // Create a DecimalArray with the default precision and scale
Review comment:
this is an example of how the new API workrs -- I think it is easier to understand and use
##########
File path: arrow/src/array/array_binary.rs
##########
@@ -1342,6 +1511,48 @@ mod tests {
assert_eq!("9.9", arr.value_as_string(0));
assert_eq!("-9.9", arr.value_as_string(1));
}
+ #[test]
+ fn test_decimal_from_iter_values() {
+ let array = DecimalArray::from_iter_values(vec![-100, 0, 101].into_iter());
+ assert_eq!(array.len(), 3);
+ assert_eq!(array.data_type(), &DataType::Decimal(38, 10));
+ assert_eq!(-100, array.value(0));
+ assert!(!array.is_null(0));
+ assert_eq!(0, array.value(1));
+ assert!(!array.is_null(1));
+ assert_eq!(101, array.value(2));
+ assert!(!array.is_null(2));
+ }
+
+ #[test]
+ fn test_decimal_from_iter() {
+ let array: DecimalArray = vec![Some(-100), None, Some(101)].into_iter().collect();
+ assert_eq!(array.len(), 3);
+ assert_eq!(array.data_type(), &DataType::Decimal(38, 10));
+ assert_eq!(-100, array.value(0));
+ assert!(!array.is_null(0));
+ assert!(array.is_null(1));
+ assert_eq!(101, array.value(2));
+ assert!(!array.is_null(2));
+ }
+
+ #[test]
+ fn test_decimal_iter() {
Review comment:
Also included a basic `iter()` and `into_iter()` functions -- while there is room for performance improvement I think the API is solid
--
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 #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1025135391
This PR is now ready for (re) review @liukun4515
cc @sweb as you contributed the initial `DecimalArray` implementation (thanks again!)
You can see examples of how the API is used in https://github.com/apache/arrow-rs/pull/1247 and https://github.com/apache/arrow-rs/pull/1249
--
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] liukun4515 commented on a change in pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#discussion_r790371475
##########
File path: arrow/src/array/array_binary.rs
##########
@@ -816,13 +827,80 @@ impl DecimalArray {
let array_data = unsafe { builder.build_unchecked() };
Self::from(array_data)
}
+
+ /// Creates a [DecimalArray] with default precision and scale,
+ /// based on an iterator of `i128` values without nulls
+ pub fn from_iter_values<I: IntoIterator<Item = i128>>(iter: I) -> Self {
+ let val_buf: Buffer = iter.into_iter().collect();
+ let data = unsafe {
+ ArrayData::new_unchecked(
+ Self::default_type(),
+ val_buf.len() / std::mem::size_of::<i128>(),
+ None,
+ None,
+ 0,
+ vec![val_buf],
+ vec![],
+ )
+ };
+ DecimalArray::from(data)
+ }
+
+ /// Return the precision (total digits) that can be stored by this array
pub fn precision(&self) -> usize {
self.precision
}
+ /// Return the scale (digits after the decimal) that can be stored by this array
pub fn scale(&self) -> usize {
self.scale
}
+
+ /// Returns a DecimalArray with the same data as self, with the
+ /// specified precision.
+ ///
+ /// panic's if:
+ /// 1. the precision is larger than [`DECIMAL_MAX_PRECISION`]
+ /// 2. scale is larger than [`DECIMAL_MAX_SCALE`];
+ /// 3. scale is > precision
+ pub fn with_precision_and_scale(mut self, precision: usize, scale: usize) -> Self {
+ assert!(
+ precision <= DECIMAL_MAX_PRECISION,
+ "precision {} is greater than max {}",
+ precision,
+ DECIMAL_MAX_PRECISION
+ );
+ assert!(
+ scale <= DECIMAL_MAX_SCALE,
+ "scale {} is greater than max {}",
+ scale,
+ DECIMAL_MAX_SCALE
+ );
+ assert!(
+ scale <= precision,
+ "scale {} is greater than precision {}",
+ scale,
+ precision
+ );
+
+ assert_eq!(
+ self.data.data_type(),
+ &DataType::Decimal(self.precision, self.scale)
+ );
+
+ // safety: self.data is valid DataType::Decimal
+ let new_data_type = DataType::Decimal(precision, scale);
+ self.precision = precision;
+ self.scale = scale;
+ self.data = self.data.with_data_type(new_data_type);
+ self
+ }
+
+ /// The default precision and scale used when not specified
+ pub fn default_type() -> DataType {
+ // Keep maximum precision
+ DataType::Decimal(38, 10)
Review comment:
we can replace 38 to `DECIMAL_MAX_PRECISION`, and 10 to `OTHER_DEFAULT_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 pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1029164621
FYI @liukun4515 this is ready for review. I know you away for a few days so no worries.
I plan to wait for a review prior to merging this PR; If it gets reviewed by tomorrow's cutoff for arrow 9.0.0 I'll include it there, otherwise this can wait for 2 weeks and perhaps be included in arrow 10.0.0
--
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 edited a comment on pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1019476786
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1223?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 [#1223](https://codecov.io/gh/apache/arrow-rs/pull/1223?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cd83d94) into [master](https://codecov.io/gh/apache/arrow-rs/commit/c7e36ea7aa53b65a3fd11fabdb0ef560962f953b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c7e36ea) will **increase** coverage by `0.01%`.
> The diff coverage is `91.78%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1223 +/- ##
==========================================
+ Coverage 83.02% 83.04% +0.01%
==========================================
Files 180 180
Lines 52289 52418 +129
==========================================
+ Hits 43413 43529 +116
- Misses 8876 8889 +13
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1223?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/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2FycmF5L21vZC5ycw==) | `100.00% <ø> (ø)` | |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2RhdGF0eXBlcy9kYXRhdHlwZS5ycw==) | `66.40% <66.66%> (+0.01%)` | :arrow_up: |
| [arrow/src/array/array\_binary.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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.53% <93.16%> (-0.12%)` | :arrow_down: |
| [arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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.73% <100.00%> (-0.04%)` | :arrow_down: |
| [arrow/src/array/data.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2FycmF5L2RhdGEucnM=) | `83.13% <100.00%> (+0.10%)` | :arrow_up: |
| [arrow/src/compute/kernels/cast.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jYXN0LnJz) | `95.21% <100.00%> (+<0.01%)` | :arrow_up: |
| [arrow/src/csv/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2Nzdi9yZWFkZXIucnM=) | `88.12% <100.00%> (ø)` | |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `93.52% <0.00%> (-0.20%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1223?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/1223?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 [c7e36ea...cd83d94](https://codecov.io/gh/apache/arrow-rs/pull/1223?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 #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1033017520
Thanks again for the help @liukun4515
--
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] liukun4515 commented on pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1031014348
> FYI @liukun4515 this is ready for review. I know you away for a few days so no worries.
>
> I plan to wait for a review prior to merging this PR; If it gets reviewed by tomorrow's cutoff for arrow 9.0.0 I'll include it there, otherwise this can wait for 2 weeks and perhaps be included in arrow 10.0.0
I will review it later today.
--
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 #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223
--
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 edited a comment on pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1019476786
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1223?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 [#1223](https://codecov.io/gh/apache/arrow-rs/pull/1223?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e4a1d65) into [master](https://codecov.io/gh/apache/arrow-rs/commit/aac1844ce707e3744595472b0357c101a91b5749?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (aac1844) will **increase** coverage by `0.02%`.
> The diff coverage is `91.78%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1223 +/- ##
==========================================
+ Coverage 82.96% 82.98% +0.02%
==========================================
Files 178 178
Lines 51547 51676 +129
==========================================
+ Hits 42765 42883 +118
- Misses 8782 8793 +11
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1223?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/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2FycmF5L21vZC5ycw==) | `100.00% <ø> (ø)` | |
| [arrow/src/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2RhdGF0eXBlcy9kYXRhdHlwZS5ycw==) | `66.40% <66.66%> (-0.41%)` | :arrow_down: |
| [arrow/src/array/array\_binary.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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.39% <93.16%> (-0.10%)` | :arrow_down: |
| [arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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.73% <100.00%> (-0.04%)` | :arrow_down: |
| [arrow/src/array/data.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2FycmF5L2RhdGEucnM=) | `83.13% <100.00%> (+0.10%)` | :arrow_up: |
| [arrow/src/compute/kernels/cast.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9jYXN0LnJz) | `95.21% <100.00%> (+<0.01%)` | :arrow_up: |
| [arrow/src/csv/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2Nzdi9yZWFkZXIucnM=) | `88.12% <100.00%> (ø)` | |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `93.52% <0.00%> (-0.20%)` | :arrow_down: |
| ... and [2 more](https://codecov.io/gh/apache/arrow-rs/pull/1223/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/1223?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/1223?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 [aac1844...e4a1d65](https://codecov.io/gh/apache/arrow-rs/pull/1223?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 a change in pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#discussion_r795069276
##########
File path: arrow/src/array/array_binary.rs
##########
@@ -816,13 +827,80 @@ impl DecimalArray {
let array_data = unsafe { builder.build_unchecked() };
Self::from(array_data)
}
+
+ /// Creates a [DecimalArray] with default precision and scale,
+ /// based on an iterator of `i128` values without nulls
+ pub fn from_iter_values<I: IntoIterator<Item = i128>>(iter: I) -> Self {
+ let val_buf: Buffer = iter.into_iter().collect();
+ let data = unsafe {
+ ArrayData::new_unchecked(
+ Self::default_type(),
+ val_buf.len() / std::mem::size_of::<i128>(),
+ None,
+ None,
+ 0,
+ vec![val_buf],
+ vec![],
+ )
+ };
+ DecimalArray::from(data)
+ }
+
+ /// Return the precision (total digits) that can be stored by this array
pub fn precision(&self) -> usize {
self.precision
}
+ /// Return the scale (digits after the decimal) that can be stored by this array
pub fn scale(&self) -> usize {
self.scale
}
+
+ /// Returns a DecimalArray with the same data as self, with the
+ /// specified precision.
+ ///
+ /// panic's if:
+ /// 1. the precision is larger than [`DECIMAL_MAX_PRECISION`]
+ /// 2. scale is larger than [`DECIMAL_MAX_SCALE`];
+ /// 3. scale is > precision
+ pub fn with_precision_and_scale(mut self, precision: usize, scale: usize) -> Self {
Review comment:
89f9cee442
--
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] liukun4515 commented on a change in pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#discussion_r801614526
##########
File path: arrow/src/array/builder.rs
##########
@@ -1153,87 +1153,6 @@ pub struct FixedSizeBinaryBuilder {
builder: FixedSizeListBuilder<UInt8Builder>,
}
-pub const MAX_DECIMAL_FOR_EACH_PRECISION: [i128; 38] = [
Review comment:
This change is an API breaks.
--
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 #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1032622663
No problem -- thanks for the review @liukun4515
--
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 edited a comment on pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1019476786
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1223?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 [#1223](https://codecov.io/gh/apache/arrow-rs/pull/1223?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (89f9cee) into [master](https://codecov.io/gh/apache/arrow-rs/commit/aac1844ce707e3744595472b0357c101a91b5749?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (aac1844) will **increase** coverage by `0.02%`.
> The diff coverage is `93.22%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1223 +/- ##
==========================================
+ Coverage 82.96% 82.98% +0.02%
==========================================
Files 178 178
Lines 51547 51661 +114
==========================================
+ Hits 42765 42871 +106
- Misses 8782 8790 +8
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1223?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/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2RhdGF0eXBlcy9kYXRhdHlwZS5ycw==) | `66.38% <ø> (-0.43%)` | :arrow_down: |
| [arrow/src/array/array\_binary.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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.33% <92.79%> (-0.15%)` | :arrow_down: |
| [arrow/src/array/data.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2FycmF5L2RhdGEucnM=) | `83.13% <100.00%> (+0.10%)` | :arrow_up: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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=) | `84.64% <0.00%> (ø)` | |
| [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2RhdGF0eXBlcy9maWVsZC5ycw==) | `54.10% <0.00%> (+0.30%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1223?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/1223?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 [aac1844...89f9cee](https://codecov.io/gh/apache/arrow-rs/pull/1223?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] liukun4515 edited a comment on pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
liukun4515 edited a comment on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1019649335
Do you want to refactor some codes like https://github.com/apache/arrow-rs/blob/66e029ebe41bcaa1754f88034deee500a7803fc8/arrow/src/compute/kernels/cast.rs#L2075 in this pull request?
If you have the plan to refactor this code, I will review this later.
LGTM, expect the https://github.com/apache/arrow-rs/pull/1223#discussion_r790370190
--
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] liukun4515 commented on pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1019649335
Do you want to refactor some codes like https://github.com/apache/arrow-rs/blob/66e029ebe41bcaa1754f88034deee500a7803fc8/arrow/src/compute/kernels/cast.rs#L2075 in this pull request?
--
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 #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1020127246
> Do you want to refactor some codes like ... in this pull request?
I was planning to do that in a follow on PR to keep this PR small and focused on the API changes.
So I will plan to make the changes you suggest in this PR, and then prepare a new one that refactors the rest of the code.
Thank you for the review.
--
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] liukun4515 commented on a change in pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#discussion_r801621567
##########
File path: arrow/src/array/array_binary.rs
##########
@@ -1360,6 +1588,57 @@ mod tests {
assert_eq!("0.000", arr.value_as_string(6));
}
+ #[test]
+ fn test_decimal_array_with_precision_and_scale() {
+ let arr = DecimalArray::from_iter_values([12345, 456, 7890, -123223423432432])
+ .with_precision_and_scale(20, 2)
+ .unwrap();
+
+ assert_eq!(arr.data_type(), &DataType::Decimal(20, 2));
+ assert_eq!(arr.precision(), 20);
+ assert_eq!(arr.scale(), 2);
+
+ let actual: Vec<_> = (0..arr.len()).map(|i| arr.value_as_string(i)).collect();
+ let expected = vec!["123.45", "4.56", "78.90", "-1232234234324.32"];
+
+ assert_eq!(actual, expected);
+ }
+
+ #[test]
Review comment:
good tests.
--
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 change in pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#discussion_r802018536
##########
File path: arrow/src/array/builder.rs
##########
@@ -1153,87 +1153,6 @@ pub struct FixedSizeBinaryBuilder {
builder: FixedSizeListBuilder<UInt8Builder>,
}
-pub const MAX_DECIMAL_FOR_EACH_PRECISION: [i128; 38] = [
Review comment:
Added 'api-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] liukun4515 commented on a change in pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#discussion_r801615203
##########
File path: arrow/src/compute/kernels/cast.rs
##########
@@ -2297,7 +2298,7 @@ mod tests {
let array = Arc::new(array) as ArrayRef;
let casted_array = cast(&array, &DataType::Decimal(3, 1));
assert!(casted_array.is_err());
- assert_eq!("Invalid argument error: The value of 1000 i128 is not compatible with Decimal(3,1)", casted_array.unwrap_err().to_string());
+ assert_eq!("Invalid argument error: 1000 is too large to store in a Decimal of precision 3. Max is 999", casted_array.unwrap_err().to_string());
Review comment:
Nice error message.
--
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 #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1024947863
🤔 while working to port some of the code over, it turns out that DecimalBuilder also does value validation on each value (so it is certain that the decimal values are within correct range. I need to think about this 🤔
--
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 change in pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#discussion_r795177390
##########
File path: arrow/src/csv/reader.rs
##########
@@ -900,15 +899,8 @@ fn parse_decimal_with_parameter(s: &str, precision: usize, scale: usize) -> Resu
if negative {
result = result.neg();
}
- if result > MAX_DECIMAL_FOR_EACH_PRECISION[precision - 1]
- || result < MIN_DECIMAL_FOR_EACH_PRECISION[precision - 1]
- {
- return Err(ArrowError::ParseError(format!(
- "parse decimal overflow, the precision {}, the scale {}, the value {}",
- precision, scale, s
- )));
- }
- Ok(result)
+ validate_decimal_precision(result, precision)
Review comment:
I consolidated the bounds checking (so that I could also use it in `with_precision_and_scale`)
--
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 #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1022646757
Update: I have not forgotten about this PR but I have been busy with other tasks this week. If I don't get to it later this week I'll work on it over the weekend
--
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 #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1019476786
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1223?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 [#1223](https://codecov.io/gh/apache/arrow-rs/pull/1223?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (41f2cbc) into [master](https://codecov.io/gh/apache/arrow-rs/commit/66e029ebe41bcaa1754f88034deee500a7803fc8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (66e029e) will **increase** coverage by `0.02%`.
> The diff coverage is `92.98%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/1223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #1223 +/- ##
==========================================
+ Coverage 82.70% 82.72% +0.02%
==========================================
Files 175 175
Lines 51712 51826 +114
==========================================
+ Hits 42770 42875 +105
- Misses 8942 8951 +9
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/1223?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/datatypes/datatype.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2RhdGF0eXBlcy9kYXRhdHlwZS5ycw==) | `66.80% <ø> (ø)` | |
| [arrow/src/array/array\_binary.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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.33% <92.52%> (-0.15%)` | :arrow_down: |
| [arrow/src/array/data.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2FycmF5L2RhdGEucnM=) | `82.39% <100.00%> (+0.11%)` | :arrow_up: |
| [arrow/src/datatypes/field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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-YXJyb3cvc3JjL2RhdGF0eXBlcy9maWVsZC5ycw==) | `53.79% <0.00%> (-0.31%)` | :arrow_down: |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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.98% <0.00%> (-0.23%)` | :arrow_down: |
| [arrow/src/array/transform/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/1223/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=) | `85.56% <0.00%> (+0.13%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/1223?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/1223?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 [66e029e...41f2cbc](https://codecov.io/gh/apache/arrow-rs/pull/1223?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] liukun4515 commented on a change in pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#discussion_r790373634
##########
File path: arrow/src/datatypes/datatype.rs
##########
@@ -189,6 +194,12 @@ impl fmt::Display for DataType {
}
}
+/// The maximum precision for [DataType::Decimal] values
+pub const DECIMAL_MAX_PRECISION: usize = 38;
Review comment:
In the datafusion, we use
https://github.com/apache/arrow-datafusion/blob/e92225d6f4660363eec3b1e767a188fccebb7ed9/datafusion/src/scalar.rs#L39 .
Maybe we can replace the const in the datafusion.
If we have the plan to support the decimal256, we may need to use some labels to identify diff decimal type.
--
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 change in pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#discussion_r795177240
##########
File path: arrow/src/array/builder.rs
##########
@@ -1153,87 +1153,6 @@ pub struct FixedSizeBinaryBuilder {
builder: FixedSizeListBuilder<UInt8Builder>,
}
-pub const MAX_DECIMAL_FOR_EACH_PRECISION: [i128; 38] = [
Review comment:
moved to `datatype`
--
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] liukun4515 commented on a change in pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#discussion_r790373634
##########
File path: arrow/src/datatypes/datatype.rs
##########
@@ -189,6 +194,12 @@ impl fmt::Display for DataType {
}
}
+/// The maximum precision for [DataType::Decimal] values
+pub const DECIMAL_MAX_PRECISION: usize = 38;
Review comment:
In the datafusion, we use https://github.com/apache/arrow-datafusion/blob/e92225d6f4660363eec3b1e767a188fccebb7ed9/datafusion/src/scalar.rs#L39
--
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] liukun4515 commented on a change in pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#discussion_r790370190
##########
File path: arrow/src/array/array_binary.rs
##########
@@ -816,13 +827,80 @@ impl DecimalArray {
let array_data = unsafe { builder.build_unchecked() };
Self::from(array_data)
}
+
+ /// Creates a [DecimalArray] with default precision and scale,
+ /// based on an iterator of `i128` values without nulls
+ pub fn from_iter_values<I: IntoIterator<Item = i128>>(iter: I) -> Self {
+ let val_buf: Buffer = iter.into_iter().collect();
+ let data = unsafe {
+ ArrayData::new_unchecked(
+ Self::default_type(),
+ val_buf.len() / std::mem::size_of::<i128>(),
+ None,
+ None,
+ 0,
+ vec![val_buf],
+ vec![],
+ )
+ };
+ DecimalArray::from(data)
+ }
+
+ /// Return the precision (total digits) that can be stored by this array
pub fn precision(&self) -> usize {
self.precision
}
+ /// Return the scale (digits after the decimal) that can be stored by this array
pub fn scale(&self) -> usize {
self.scale
}
+
+ /// Returns a DecimalArray with the same data as self, with the
+ /// specified precision.
+ ///
+ /// panic's if:
+ /// 1. the precision is larger than [`DECIMAL_MAX_PRECISION`]
+ /// 2. scale is larger than [`DECIMAL_MAX_SCALE`];
+ /// 3. scale is > precision
+ pub fn with_precision_and_scale(mut self, precision: usize, scale: usize) -> Self {
Review comment:
I think `Result` is better.
--
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 #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#issuecomment-1024978546
back to draft while I think about this a bit
--
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 change in pull request #1223: `DecimalArray` API ergonomics: add iter(), create from iter(), change precision / scale
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1223:
URL: https://github.com/apache/arrow-rs/pull/1223#discussion_r795166839
##########
File path: arrow/src/datatypes/datatype.rs
##########
@@ -189,6 +194,12 @@ impl fmt::Display for DataType {
}
}
+/// The maximum precision for [DataType::Decimal] values
+pub const DECIMAL_MAX_PRECISION: usize = 38;
Review comment:
I agree that we should remove the duplication in DataFusion -- when this PR is merged, I can file a ticket in DataFusion to upgrade to the new DecimalAPI and remove the duplication
--
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