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 2021/05/09 16:52:05 UTC

[GitHub] [arrow-rs] mgill25 opened a new pull request #273: Added Decimal support to pretty-print display utility (#230)

mgill25 opened a new pull request #273:
URL: https://github.com/apache/arrow-rs/pull/273


   # Which issue does this PR close?
   
   Closes #230 .
   
   # What changes are included in this PR?
   
   Added support for `Decimal` column datatype during conversion of value to String. Removes the previous error.
   
   
   @alamb I made a tiny change and added a couple of tests (copied similar test data from other tests I found) within the same file just to ensure things are working correctly.
   
   I wasn't sure what the definition of "pretty" in the pretty-printing context here would be, so I made no assumptions and just propagated whatever was being printed by default. Still just in the learning mode :) If more improvements are needed, please do let me know!


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-rs] alamb commented on a change in pull request #273: Added Decimal support to pretty-print display utility (#230)

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #273:
URL: https://github.com/apache/arrow-rs/pull/273#discussion_r632724899



##########
File path: arrow/src/util/display.rs
##########
@@ -217,6 +231,9 @@ pub fn array_value_to_string(column: &array::ArrayRef, row: usize) -> Result<Str
         DataType::Float16 => make_string!(array::Float32Array, column, row),
         DataType::Float32 => make_string!(array::Float32Array, column, row),
         DataType::Float64 => make_string!(array::Float64Array, column, row),
+        DataType::Decimal(_, scale) => {
+            make_string_from_decimal!(array::DecimalArray, column, row, scale)

Review comment:
       I agree it can be a function (rather than a macro) but that can always be done as a follow on PR. I think this is a good step. 👍 

##########
File path: arrow/src/util/display.rs
##########
@@ -217,6 +231,9 @@ pub fn array_value_to_string(column: &array::ArrayRef, row: usize) -> Result<Str
         DataType::Float16 => make_string!(array::Float32Array, column, row),
         DataType::Float32 => make_string!(array::Float32Array, column, row),
         DataType::Float64 => make_string!(array::Float64Array, column, row),
+        DataType::Decimal(_, scale) => {
+            make_string_from_decimal!(array::DecimalArray, column, row, scale)

Review comment:
       I agree it can be a function (rather than a macro) but that can always be done as a follow on PR. I think this PR is a good step forward. 👍 




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-rs] mgill25 commented on a change in pull request #273: Added Decimal support to pretty-print display utility (#230)

Posted by GitBox <gi...@apache.org>.
mgill25 commented on a change in pull request #273:
URL: https://github.com/apache/arrow-rs/pull/273#discussion_r630614945



##########
File path: arrow/src/util/display.rs
##########
@@ -296,3 +297,48 @@ fn dict_array_value_to_string<K: ArrowPrimitiveType>(
 
     array_value_to_string(&dict_array.values(), dict_index)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::{
+        array::ArrayRef,
+        array::{ArrayData, DecimalArray, Int32Array},
+    };
+    use crate::{buffer::Buffer, datatypes::DataType};
+    use std::sync::Arc;
+
+    #[test]
+    fn test_int_display() -> Result<()> {
+        let array = Arc::new(Int32Array::from(vec![6, 3])) as ArrayRef;
+        let actual_one = array_value_to_string(&array, 0).unwrap();
+        let expected_one = String::from("6");
+
+        let actual_two = array_value_to_string(&array, 1).unwrap();
+        let expected_two = String::from("3");
+        assert_eq!(actual_one, expected_one);
+        assert_eq!(actual_two, expected_two);
+        Ok(())
+    }
+
+    #[test]
+    fn test_decimal_display() -> Result<()> {
+        let values: [u8; 32] = [

Review comment:
       Thanks a lot for your guidance. I was wondering exactly this - how to use `DecimalBuilder` and how to think in terms of real numbers instead of bytes during creation of test data. 😅 




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-rs] alamb commented on a change in pull request #273: Added Decimal support to pretty-print display utility (#230)

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #273:
URL: https://github.com/apache/arrow-rs/pull/273#discussion_r629255080



##########
File path: arrow/src/util/display.rs
##########
@@ -296,3 +297,48 @@ fn dict_array_value_to_string<K: ArrowPrimitiveType>(
 
     array_value_to_string(&dict_array.values(), dict_index)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::{
+        array::ArrayRef,
+        array::{ArrayData, DecimalArray, Int32Array},
+    };
+    use crate::{buffer::Buffer, datatypes::DataType};
+    use std::sync::Arc;
+
+    #[test]
+    fn test_int_display() -> Result<()> {
+        let array = Arc::new(Int32Array::from(vec![6, 3])) as ArrayRef;
+        let actual_one = array_value_to_string(&array, 0).unwrap();
+        let expected_one = String::from("6");

Review comment:
       ```suggestion
           let expected_one = "6";
   ```
   
   I don't think you need to create a `String` you can just compare with the `&str`

##########
File path: arrow/src/util/display.rs
##########
@@ -296,3 +297,48 @@ fn dict_array_value_to_string<K: ArrowPrimitiveType>(
 
     array_value_to_string(&dict_array.values(), dict_index)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::{
+        array::ArrayRef,
+        array::{ArrayData, DecimalArray, Int32Array},
+    };
+    use crate::{buffer::Buffer, datatypes::DataType};
+    use std::sync::Arc;
+
+    #[test]
+    fn test_int_display() -> Result<()> {
+        let array = Arc::new(Int32Array::from(vec![6, 3])) as ArrayRef;
+        let actual_one = array_value_to_string(&array, 0).unwrap();
+        let expected_one = String::from("6");
+
+        let actual_two = array_value_to_string(&array, 1).unwrap();
+        let expected_two = String::from("3");
+        assert_eq!(actual_one, expected_one);
+        assert_eq!(actual_two, expected_two);
+        Ok(())
+    }
+
+    #[test]
+    fn test_decimal_display() -> Result<()> {
+        let values: [u8; 32] = [

Review comment:
       This is a pretty low level way to make a `DecimalArray`
   
   What would you think of using the `DecimalBuilder` -- something like the following
   
   ```
           let capacity = 10;
           let precision = 10; // total number of digits
           let scale = 2; // 2 digits after the decimal point
           let mut builder = DecimalBuilder::new(capacity, precision, scale);
           builder.append_value(101).unwrap();
           builder.append_null().unwrap();
           builder.append_value(200).unwrap();
           builder.append_value(3040).unwrap();
   
           let dm = Arc::new(builder.finish()) as ArrayRef;
   ```
   
   
   Note in this case, I would expect the display values to be something like the following (always have 2 decimal places):
   ```
   1.01
   
   2.00
   30.40
   ```
   
   

##########
File path: arrow/src/util/display.rs
##########
@@ -296,3 +297,48 @@ fn dict_array_value_to_string<K: ArrowPrimitiveType>(
 
     array_value_to_string(&dict_array.values(), dict_index)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::{
+        array::ArrayRef,
+        array::{ArrayData, DecimalArray, Int32Array},
+    };
+    use crate::{buffer::Buffer, datatypes::DataType};
+    use std::sync::Arc;
+
+    #[test]

Review comment:
       There are a few more tests of `pretty-printing` which uses this display code in https://github.com/apache/arrow-rs/blob/master/arrow/src/util/pretty.rs#L229 
   
   You might find writing tests following that style (perhaps putting the tests in that module) easier

##########
File path: arrow/src/util/display.rs
##########
@@ -296,3 +297,48 @@ fn dict_array_value_to_string<K: ArrowPrimitiveType>(
 
     array_value_to_string(&dict_array.values(), dict_index)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::{
+        array::ArrayRef,
+        array::{ArrayData, DecimalArray, Int32Array},
+    };
+    use crate::{buffer::Buffer, datatypes::DataType};
+    use std::sync::Arc;
+
+    #[test]
+    fn test_int_display() -> Result<()> {
+        let array = Arc::new(Int32Array::from(vec![6, 3])) as ArrayRef;
+        let actual_one = array_value_to_string(&array, 0).unwrap();
+        let expected_one = String::from("6");
+
+        let actual_two = array_value_to_string(&array, 1).unwrap();
+        let expected_two = String::from("3");

Review comment:
       ```suggestion
           let expected_two = "3";
   ```




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-rs] codecov-commenter commented on pull request #273: Added Decimal support to pretty-print display utility (#230)

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #273:
URL: https://github.com/apache/arrow-rs/pull/273#issuecomment-836318490


   # [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/273?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 [#273](https://codecov.io/gh/apache/arrow-rs/pull/273?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a23138f) into [master](https://codecov.io/gh/apache/arrow-rs/commit/4dfbca6e5791be400d2fd3ae863655445327650e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4dfbca6) will **increase** coverage by `0.02%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow-rs/pull/273/graphs/tree.svg?width=650&height=150&src=pr&token=pq9V9qWZ1N&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/arrow-rs/pull/273?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     #273      +/-   ##
   ==========================================
   + Coverage   82.52%   82.54%   +0.02%     
   ==========================================
     Files         162      162              
     Lines       43672    43819     +147     
   ==========================================
   + Hits        36040    36172     +132     
   - Misses       7632     7647      +15     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/273?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/util/display.rs](https://codecov.io/gh/apache/arrow-rs/pull/273/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-YXJyb3cvc3JjL3V0aWwvZGlzcGxheS5ycw==) | `47.36% <100.00%> (+16.81%)` | :arrow_up: |
   | [arrow/src/array/transform/fixed\_binary.rs](https://codecov.io/gh/apache/arrow-rs/pull/273/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-YXJyb3cvc3JjL2FycmF5L3RyYW5zZm9ybS9maXhlZF9iaW5hcnkucnM=) | `78.94% <0.00%> (-5.27%)` | :arrow_down: |
   | [parquet/src/arrow/levels.rs](https://codecov.io/gh/apache/arrow-rs/pull/273/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-cGFycXVldC9zcmMvYXJyb3cvbGV2ZWxzLnJz) | `81.25% <0.00%> (-0.44%)` | :arrow_down: |
   | [parquet/src/column/writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/273/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-cGFycXVldC9zcmMvY29sdW1uL3dyaXRlci5ycw==) | `93.76% <0.00%> (-0.27%)` | :arrow_down: |
   | [arrow/src/compute/kernels/regexp.rs](https://codecov.io/gh/apache/arrow-rs/pull/273/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9yZWdleHAucnM=) | `97.56% <0.00%> (-0.03%)` | :arrow_down: |
   | [parquet/src/arrow/schema.rs](https://codecov.io/gh/apache/arrow-rs/pull/273/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-cGFycXVldC9zcmMvYXJyb3cvc2NoZW1hLnJz) | `88.93% <0.00%> (-0.02%)` | :arrow_down: |
   | [arrow/src/json/reader.rs](https://codecov.io/gh/apache/arrow-rs/pull/273/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-YXJyb3cvc3JjL2pzb24vcmVhZGVyLnJz) | `83.43% <0.00%> (-0.02%)` | :arrow_down: |
   | [arrow/src/array/data.rs](https://codecov.io/gh/apache/arrow-rs/pull/273/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=) | `72.07% <0.00%> (ø)` | |
   | [arrow/src/array/null.rs](https://codecov.io/gh/apache/arrow-rs/pull/273/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-YXJyb3cvc3JjL2FycmF5L251bGwucnM=) | `86.66% <0.00%> (ø)` | |
   | [arrow/src/array/array.rs](https://codecov.io/gh/apache/arrow-rs/pull/273/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-YXJyb3cvc3JjL2FycmF5L2FycmF5LnJz) | `76.07% <0.00%> (ø)` | |
   | ... and [4 more](https://codecov.io/gh/apache/arrow-rs/pull/273/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/273?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/273?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 [4dfbca6...a23138f](https://codecov.io/gh/apache/arrow-rs/pull/273?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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-rs] alamb merged pull request #273: Added Decimal support to pretty-print display utility (#230)

Posted by GitBox <gi...@apache.org>.
alamb merged pull request #273:
URL: https://github.com/apache/arrow-rs/pull/273


   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow-rs] paddyhoran commented on a change in pull request #273: Added Decimal support to pretty-print display utility (#230)

Posted by GitBox <gi...@apache.org>.
paddyhoran commented on a change in pull request #273:
URL: https://github.com/apache/arrow-rs/pull/273#discussion_r631790057



##########
File path: arrow/src/util/display.rs
##########
@@ -217,6 +231,9 @@ pub fn array_value_to_string(column: &array::ArrayRef, row: usize) -> Result<Str
         DataType::Float16 => make_string!(array::Float32Array, column, row),
         DataType::Float32 => make_string!(array::Float32Array, column, row),
         DataType::Float64 => make_string!(array::Float64Array, column, row),
+        DataType::Decimal(_, scale) => {
+            make_string_from_decimal!(array::DecimalArray, column, row, scale)

Review comment:
       Does this need to be a macro?  I think you can inline this as it's the only usage, right?




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org