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 2020/11/11 16:49:25 UTC
[GitHub] [arrow] sweb opened a new pull request #8640: WIP: ARROW-4193: [Rust] Add support for decimal data type
sweb opened a new pull request #8640:
URL: https://github.com/apache/arrow/pull/8640
This is very much work in progress.
The idea is to implement `DecimalArray` based on the existing `FixedSizeBinaryArray`. At the current state this is mostly C&P. The values are returned as `i128`. The underlying fixed size is calculated from the precision.
```
let values: [u8; 20] = [0, 0, 0, 0, 0, 2, 17, 180, 219, 192, 255, 255, 255, 255, 255, 253, 238, 75, 36, 64];
let array_data = ArrayData::builder(DataType::Decimal(23, 6))
.len(2)
.add_buffer(Buffer::from(&values[..]))
.build();
let fixed_size_binary_array = DecimalArray::from(array_data);
assert_eq!(
8_887_000_000,
fixed_size_binary_array.value(0)
);
assert_eq!(
-8_887_000_000,
fixed_size_binary_array.value(1)
);
assert_eq!(10, fixed_size_binary_array.value_length());
```
----------------------------------------------------------------
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] jorgecarleitao commented on a change in pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
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(&mut 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<Vec<u8>> {
+ 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(&mut self) -> Result<()> {
+ let length: usize = self.builder.value_length() as usize;
+ self.builder.values().append_slice(&vec![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(&a_slice, &b_slice, true);
}
+ fn create_decimal_array(data: &[Option<i128>]) -> 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(&a_slice, &b_slice, true);
+
+ let a_slice = a.slice(0, 5);
+ let b_slice = b.slice(0, 5);
+ test_equal(&a_slice, &b_slice, false);
+
+ let a_slice = a.slice(4, 1);
+ let b_slice = b.slice(4, 1);
+ test_equal(&a_slice, &b_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 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] jorgecarleitao commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-729035771
> @jorgecarleitao I didn't have a chance to get to this PR today in my allotted time budget, but it sounds like @nevi-me may be planning on doing so, so I am going to lower the priority. I'll try and read it more carefully shortly
I am sorry, I did not mean to push for reviewing to neither of you. It was just a comment to prepare @sweb that there is a part of the code that I can't review well and thus I am relying on some help ❤️
----------------------------------------------------------------
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] sweb commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-728805602
> Hi @sweb, I'm providing general comments, I'll look at this in detail over the days.
>
> I see that you're using `i128` as the backing type. One of the reasons that's prevented us from making progress on this, is not having a good BigDecimal impl in Rust to use.
> The biggest hurdle will be converting `123.45f64` to decimals, as we'll need to do that for integration testing.
>
> On the JIRA itself, the plan was to add the type, then also:
>
> * add support on the IPC reader and writer
>
> * enable decimal types in integration teting
>
>
> I'm fine with us doing the above as follow-ups, as they'd be quite involved.
Thank you very much for your first feedback!
I can try to create a second pull request for adding the IPC reader / writer after this one. I didn't think of it since I mainly use arrow as a processing layer after reading parquet files and never use the IPC parts.
I have some ideas concerning the conversion from f64 to decimal, but I will try to find a reference implementation in the Python sources - maybe we can borrow some ideas from there.
----------------------------------------------------------------
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] sweb commented on a change in pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
sweb commented on a change in pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#discussion_r524925542
##########
File path: rust/arrow/src/array/equal/mod.rs
##########
@@ -604,6 +613,76 @@ mod tests {
test_equal(&a_slice, &b_slice, true);
}
+ fn create_decimal_array(data: &[Option<i128>]) -> 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(&a_slice, &b_slice, true);
+
+ let a_slice = a.slice(0, 5);
+ let b_slice = b.slice(0, 5);
+ test_equal(&a_slice, &b_slice, false);
+
+ let a_slice = a.slice(4, 1);
+ let b_slice = b.slice(4, 1);
+ test_equal(&a_slice, &b_slice, true);
Review comment:
I think you found something. This fails:
```
let a_slice = a.slice(3, 3);
let b_slice = b.slice(3, 3);
test_equal(&a_slice, &b_slice, false);
```
I will try to fix it 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] sweb commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-731577989
@nevi-me I will gladly continue to work on supporting decimals. I will start with the IPC reader / writer.
----------------------------------------------------------------
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] jorgecarleitao closed pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
jorgecarleitao closed pull request #8640:
URL: https://github.com/apache/arrow/pull/8640
----------------------------------------------------------------
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] jorgecarleitao commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-731557499
I merged it. Wasn't it ready?
----------------------------------------------------------------
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] sweb commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-731556990
@jorgecarleitao may I ask why you closed this PR? :)
----------------------------------------------------------------
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] nevi-me commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-728739069
Hi @sweb, I'm providing general comments, I'll look at this in detail over the days.
I see that you're using `i128` as the backing type. One of the reasons that's prevented us from making progress on this, is not having a good BigDecimal impl in Rust to use.
The biggest hurdle will be converting `123.45f64` to decimals, as we'll need to do that for integration testing.
On the JIRA itself, the plan was to add the type, then also:
* add support on the IPC reader and writer
* enable decimal types in integration teting
I'm fine with us doing the above as follow-ups, as they'd be quite involved.
----------------------------------------------------------------
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] jorgecarleitao commented on a change in pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
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(&mut 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<Vec<u8>> {
+ 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(&mut self) -> Result<()> {
+ let length: usize = self.builder.value_length() as usize;
+ self.builder.values().append_slice(&vec![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:
users@infra.apache.org
[GitHub] [arrow] sweb commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-731565267
@jorgecarleitao In closing remarks, I just wanted to say that this was the most pleasant contribution experience I had so far on an open source project. Thank you very much for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-731568495
There's some follow-up to eventually support decimals in the integration testing (and Parquet reader and writer). Would you be interested in assisting us with that @sweb? You can open PRs on the Integration testing umbrella JIRA, or as individual/standalone ones. I'm fine with either appraoch. Thanks
----------------------------------------------------------------
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] sweb commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-727602232
@jorgecarleitao @nevi-me may I ask you for a review? From a functionality point of view I think this is more or less done, however I am very much at the beginning of writing stuff in Rust, so help is much appreciated!
----------------------------------------------------------------
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] sweb commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-731559147
Ah sorry, I missed the merge and thought it was just closed. Thank you for merging!
----------------------------------------------------------------
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] alamb commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-728398825
@jorgecarleitao I will put it on my queue for tomorrow. Hopefully the morning
----------------------------------------------------------------
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] sweb commented on a change in pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
sweb commented on a change in pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#discussion_r524922526
##########
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(&mut 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<Vec<u8>> {
+ 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(&mut self) -> Result<()> {
+ let length: usize = self.builder.value_length() as usize;
+ self.builder.values().append_slice(&vec![0u8; length][..])?;
Review comment:
Thank you very much for your review!
This part was shamefully copied from the fixed array size binary builder - I tried to change it to an array but the compiler rightfully complains that the value length is unknown at compile time. Is there another way to achieve this without an array, or am I missing something?
----------------------------------------------------------------
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] github-actions[bot] commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-727602423
https://issues.apache.org/jira/browse/ARROW-4193
----------------------------------------------------------------
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] sweb commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-729082645
@jorgecarleitao could you check the following strange thing I cannot explain to myself:
I added two additional asserts in `test_decimal_offsets` and `test_fixed_size_binary_offsets` (see latest commit, leading to 2 failing tests).
```
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(3, 3);
let b_slice = b.slice(3, 3);
test_equal(&a_slice, &b_slice, false);
```
From my current understanding, these slices should not be the same, since one is [-8887, None, None] and the other one is [15887, None, None]. However the test states that they are equal. Same for the fixed_binary test. Do you have an idea what I am missing? It is very likely that I do not really understand how the offsets work and that these checks should in fact evaluate to `true`.
Thanks!
----------------------------------------------------------------
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] github-actions[bot] commented on pull request #8640: WIP: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-725542282
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
----------------------------------------------------------------
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] nevi-me commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-731569254
I've created https://issues.apache.org/jira/browse/ARROW-10674
----------------------------------------------------------------
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] alamb commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-729030359
@jorgecarleitao I didn't have a chance to get to this PR today in my allotted time budget, but it sounds like @nevi-me may be planning on doing so, so I am going to lower the priority. I'll try and read it more carefully shortly
----------------------------------------------------------------
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] jorgecarleitao commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-729102741
@sweb , it is a bug in the equality itself. I have a PR for it, hang on a sec.
----------------------------------------------------------------
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] sweb commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
sweb commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-729113937
@jorgecarleitao Thanks for the quick feedback! I will remove the two asserts since this is already addressed in your PR.
----------------------------------------------------------------
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] jorgecarleitao commented on pull request #8640: ARROW-4193: [Rust] Add support for decimal data type
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8640:
URL: https://github.com/apache/arrow/pull/8640#issuecomment-729104773
PR for it: #8695
----------------------------------------------------------------
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