[GitHub] [arrow] jorgecarleitao commented on a change in pull request #8640: ARROW-4193: [Rust] Add support for decimal data type

2020-11-16 Thread GitBox


jorgecarleitao commented on a change in pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#discussion_r524923532



##
File path: rust/arrow/src/array/builder.rs
##
@@ -1882,6 +1975,67 @@ impl FixedSizeBinaryBuilder {
 }
 }
 
+impl DecimalBuilder {
+/// Creates a new `BinaryBuilder`, `capacity` is the number of bytes in 
the values
+/// array
+pub fn new(capacity: usize, precision: usize, scale: usize) -> Self {
+let values_builder = UInt8Builder::new(capacity);
+let byte_width = DecimalArray::calc_fixed_byte_size(precision);
+Self {
+builder: FixedSizeListBuilder::new(values_builder, byte_width),
+precision,
+scale,
+}
+}
+
+/// Appends a byte slice into the builder.
+///
+/// Automatically calls the `append` method to delimit the slice appended 
in as a
+/// distinct array element.
+pub fn append_value( self, value: i128) -> Result<()> {
+let value_as_bytes = Self::from_i128_to_fixed_size_bytes(
+value,
+self.builder.value_length() as usize,
+)?;
+if self.builder.value_length() != value_as_bytes.len() as i32 {
+return Err(ArrowError::InvalidArgumentError(
+"Byte slice does not have the same length as DecimalBuilder 
value lengths".to_string()
+));
+}
+self.builder
+.values()
+.append_slice(value_as_bytes.as_slice())?;
+self.builder.append(true)
+}
+
+fn from_i128_to_fixed_size_bytes(v: i128, size: usize) -> Result> {
+if size > 16 {
+return Err(ArrowError::InvalidArgumentError(
+"DecimalBuilder only supports values up to 16 
bytes.".to_string(),
+));
+}
+let res = v.to_be_bytes();
+let start_byte = 16 - size;
+Ok(res[start_byte..16].to_vec())
+}
+
+/// Append a null value to the array.
+pub fn append_null( self) -> Result<()> {
+let length: usize = self.builder.value_length() as usize;
+self.builder.values().append_slice(![0u8; length][..])?;

Review comment:
   you are absolutely right. xD





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:
us...@infra.apache.org




[GitHub] [arrow] jorgecarleitao commented on a change in pull request #8640: ARROW-4193: [Rust] Add support for decimal data type

2020-11-16 Thread GitBox


jorgecarleitao commented on a change in pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#discussion_r524410551



##
File path: rust/arrow/src/array/builder.rs
##
@@ -1882,6 +1975,67 @@ impl FixedSizeBinaryBuilder {
 }
 }
 
+impl DecimalBuilder {
+/// Creates a new `BinaryBuilder`, `capacity` is the number of bytes in 
the values
+/// array
+pub fn new(capacity: usize, precision: usize, scale: usize) -> Self {
+let values_builder = UInt8Builder::new(capacity);
+let byte_width = DecimalArray::calc_fixed_byte_size(precision);
+Self {
+builder: FixedSizeListBuilder::new(values_builder, byte_width),
+precision,
+scale,
+}
+}
+
+/// Appends a byte slice into the builder.
+///
+/// Automatically calls the `append` method to delimit the slice appended 
in as a
+/// distinct array element.
+pub fn append_value( self, value: i128) -> Result<()> {
+let value_as_bytes = Self::from_i128_to_fixed_size_bytes(
+value,
+self.builder.value_length() as usize,
+)?;
+if self.builder.value_length() != value_as_bytes.len() as i32 {
+return Err(ArrowError::InvalidArgumentError(
+"Byte slice does not have the same length as DecimalBuilder 
value lengths".to_string()
+));
+}
+self.builder
+.values()
+.append_slice(value_as_bytes.as_slice())?;
+self.builder.append(true)
+}
+
+fn from_i128_to_fixed_size_bytes(v: i128, size: usize) -> Result> {
+if size > 16 {
+return Err(ArrowError::InvalidArgumentError(
+"DecimalBuilder only supports values up to 16 
bytes.".to_string(),
+));
+}
+let res = v.to_be_bytes();
+let start_byte = 16 - size;
+Ok(res[start_byte..16].to_vec())
+}
+
+/// Append a null value to the array.
+pub fn append_null( self) -> Result<()> {
+let length: usize = self.builder.value_length() as usize;
+self.builder.values().append_slice(![0u8; length][..])?;

Review comment:
   ```suggestion
   self.builder.values().append_slice(&[0u8; length])?;
   ```

##
File path: rust/arrow/src/array/equal/mod.rs
##
@@ -604,6 +613,76 @@ mod tests {
 test_equal(_slice, _slice, true);
 }
 
+fn create_decimal_array(data: &[Option]) -> ArrayDataRef {
+let mut builder = DecimalBuilder::new(20, 23, 6);
+
+for d in data {
+if let Some(v) = d {
+builder.append_value(*v).unwrap();
+} else {
+builder.append_null().unwrap();
+}
+}
+builder.finish().data()
+}
+
+#[test]
+fn test_decimal_equal() {
+let a = create_decimal_array(&[Some(8_887_000_000), 
Some(-8_887_000_000)]);
+let b = create_decimal_array(&[Some(8_887_000_000), 
Some(-8_887_000_000)]);
+test_equal(a.as_ref(), b.as_ref(), true);
+
+let b = create_decimal_array(&[Some(15_887_000_000), 
Some(-8_887_000_000)]);
+test_equal(a.as_ref(), b.as_ref(), false);
+}
+
+// Test the case where null_count > 0
+#[test]
+fn test_decimal_null() {
+let a = create_decimal_array(&[Some(8_887_000_000), None, 
Some(-8_887_000_000)]);
+let b = create_decimal_array(&[Some(8_887_000_000), None, 
Some(-8_887_000_000)]);
+test_equal(a.as_ref(), b.as_ref(), true);
+
+let b = create_decimal_array(&[Some(8_887_000_000), 
Some(-8_887_000_000), None]);
+test_equal(a.as_ref(), b.as_ref(), false);
+
+let b = create_decimal_array(&[Some(15_887_000_000), None, 
Some(-8_887_000_000)]);
+test_equal(a.as_ref(), b.as_ref(), false);
+}
+
+#[test]
+fn test_decimal_offsets() {
+// Test the case where offset != 0
+let a = create_decimal_array(&[
+Some(8_887_000_000),
+None,
+None,
+Some(-8_887_000_000),
+None,
+None,
+]);
+let b = create_decimal_array(&[
+Some(8_887_000_000),
+None,
+None,
+Some(15_887_000_000),
+None,
+None,
+]);
+
+let a_slice = a.slice(0, 3);
+let b_slice = b.slice(0, 3);
+test_equal(_slice, _slice, true);
+
+let a_slice = a.slice(0, 5);
+let b_slice = b.slice(0, 5);
+test_equal(_slice, _slice, false);
+
+let a_slice = a.slice(4, 1);
+let b_slice = b.slice(4, 1);
+test_equal(_slice, _slice, true);

Review comment:
   Could we add a case with a non-zero start slice whose test is not equal 
(non-zero slices are the tricky ones due to the offset calculation.





This is an automated message from the Apache Git Service.
To