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/01/01 08:21:09 UTC
[GitHub] [arrow] jorgecarleitao opened a new pull request #9066: ARROW-11097: [Rust] Minor simplification of some tests.
jorgecarleitao opened a new pull request #9066:
URL: https://github.com/apache/arrow/pull/9066
----------------------------------------------------------------
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 a change in pull request #9066: ARROW-11097: [Rust] Minor simplification of some tests.
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9066:
URL: https://github.com/apache/arrow/pull/9066#discussion_r551005680
##########
File path: rust/arrow/src/array/array_struct.rs
##########
@@ -303,27 +302,21 @@ mod tests {
#[test]
fn test_struct_array_from() {
- let boolean_data = ArrayData::builder(DataType::Boolean)
- .len(4)
- .add_buffer(Buffer::from([12_u8]))
- .build();
- let int_data = ArrayData::builder(DataType::Int32)
- .len(4)
- .add_buffer(Buffer::from([42, 28, 19, 31].to_byte_slice()))
- .build();
+ let boolean = Arc::new(BooleanArray::from(vec![false, false, true, true]));
Review comment:
this change certainly looks much better to me 👍
##########
File path: rust/arrow/src/array/array_struct.rs
##########
@@ -270,27 +270,26 @@ mod tests {
use std::sync::Arc;
- use crate::datatypes::{DataType, Field};
use crate::{
array::BooleanArray, array::Float32Array, array::Float64Array, array::Int32Array,
array::StringArray, bitmap::Bitmap,
};
+ use crate::{
+ array::Int64Array,
+ datatypes::{DataType, Field},
+ };
use crate::{buffer::Buffer, datatypes::ToByteSlice};
#[test]
fn test_struct_array_builder() {
- let boolean_data = ArrayData::builder(DataType::Boolean)
- .len(4)
- .add_buffer(Buffer::from([false, false, true, true].to_byte_slice()))
Review comment:
it might be worth filing a ticket about the issue in `ToByteSlice` - maybe others will be interesting in working / able to work on it
----------------------------------------------------------------
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] codecov-io commented on pull request #9066: ARROW-11097: [Rust] Minor simplification of some tests.
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #9066:
URL: https://github.com/apache/arrow/pull/9066#issuecomment-753286013
# [Codecov](https://codecov.io/gh/apache/arrow/pull/9066?src=pr&el=h1) Report
> Merging [#9066](https://codecov.io/gh/apache/arrow/pull/9066?src=pr&el=desc) (9480d83) into [master](https://codecov.io/gh/apache/arrow/commit/51672b28e97f19f70de0f0a8800c40ee9bb939d3?el=desc) (51672b2) will **decrease** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/arrow/pull/9066?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9066 +/- ##
==========================================
- Coverage 82.61% 82.61% -0.01%
==========================================
Files 202 202
Lines 50048 50034 -14
==========================================
- Hits 41347 41335 -12
+ Misses 8701 8699 -2
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9066?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [rust/arrow/src/array/array\_struct.rs](https://codecov.io/gh/apache/arrow/pull/9066/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvYXJyYXlfc3RydWN0LnJz) | `88.43% <100.00%> (-0.18%)` | :arrow_down: |
| [rust/arrow/src/record\_batch.rs](https://codecov.io/gh/apache/arrow/pull/9066/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvcmVjb3JkX2JhdGNoLnJz) | `86.90% <100.00%> (-1.40%)` | :arrow_down: |
| [rust/arrow/src/array/equal/mod.rs](https://codecov.io/gh/apache/arrow/pull/9066/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvZXF1YWwvbW9kLnJz) | `92.32% <0.00%> (+0.37%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9066?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9066?src=pr&el=footer). Last update [51672b2...9480d83](https://codecov.io/gh/apache/arrow/pull/9066?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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 closed pull request #9066: ARROW-11097: [Rust] Minor simplification of some tests.
Posted by GitBox <gi...@apache.org>.
alamb closed pull request #9066:
URL: https://github.com/apache/arrow/pull/9066
----------------------------------------------------------------
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 #9066: ARROW-11097: [Rust] Minor simplification of some tests.
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #9066:
URL: https://github.com/apache/arrow/pull/9066#discussion_r550745492
##########
File path: rust/arrow/src/array/array_struct.rs
##########
@@ -270,27 +270,26 @@ mod tests {
use std::sync::Arc;
- use crate::datatypes::{DataType, Field};
use crate::{
array::BooleanArray, array::Float32Array, array::Float64Array, array::Int32Array,
array::StringArray, bitmap::Bitmap,
};
+ use crate::{
+ array::Int64Array,
+ datatypes::{DataType, Field},
+ };
use crate::{buffer::Buffer, datatypes::ToByteSlice};
#[test]
fn test_struct_array_builder() {
- let boolean_data = ArrayData::builder(DataType::Boolean)
- .len(4)
- .add_buffer(Buffer::from([false, false, true, true].to_byte_slice()))
Review comment:
This is the line that triggered this PR. As per this code shows, `to_byte_slice` can be applied to a `bool` or slice of bools, which promptly converts it to `u8`. However, `bool` is not represented as `u8` in the Arrow specification. Consequently, this `ArrayData` is incorrect, as it is built from 4 bytes, but represents 4 slots of booleans (line above), 4 bits or 1 byte (not 4).
This indicates a problem in the `ToByteSlice`, which should not allow `bool` to be used there. I have concrete ideas to solve 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] github-actions[bot] commented on pull request #9066: ARROW-11097: [Rust] Minor simplification of some tests.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9066:
URL: https://github.com/apache/arrow/pull/9066#issuecomment-753288142
https://issues.apache.org/jira/browse/ARROW-11097
----------------------------------------------------------------
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 #9066: ARROW-11097: [Rust] Minor simplification of some tests.
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #9066:
URL: https://github.com/apache/arrow/pull/9066#discussion_r550745492
##########
File path: rust/arrow/src/array/array_struct.rs
##########
@@ -270,27 +270,26 @@ mod tests {
use std::sync::Arc;
- use crate::datatypes::{DataType, Field};
use crate::{
array::BooleanArray, array::Float32Array, array::Float64Array, array::Int32Array,
array::StringArray, bitmap::Bitmap,
};
+ use crate::{
+ array::Int64Array,
+ datatypes::{DataType, Field},
+ };
use crate::{buffer::Buffer, datatypes::ToByteSlice};
#[test]
fn test_struct_array_builder() {
- let boolean_data = ArrayData::builder(DataType::Boolean)
- .len(4)
- .add_buffer(Buffer::from([false, false, true, true].to_byte_slice()))
Review comment:
This is the line that triggered this PR. As per this code shows, `to_byte_slice` can be applied to a `bool`, which promptly converts it to `u8`. However, `bool` is not represented as `u8` in the Arrow specification, but as a single bit. This indicates a problem in the `ToByteSlice`, which should not allow `bool` to be used there. I have concrete ideas to solve 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] jorgecarleitao commented on a change in pull request #9066: ARROW-11097: [Rust] Minor simplification of some tests.
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #9066:
URL: https://github.com/apache/arrow/pull/9066#discussion_r550745492
##########
File path: rust/arrow/src/array/array_struct.rs
##########
@@ -270,27 +270,26 @@ mod tests {
use std::sync::Arc;
- use crate::datatypes::{DataType, Field};
use crate::{
array::BooleanArray, array::Float32Array, array::Float64Array, array::Int32Array,
array::StringArray, bitmap::Bitmap,
};
+ use crate::{
+ array::Int64Array,
+ datatypes::{DataType, Field},
+ };
use crate::{buffer::Buffer, datatypes::ToByteSlice};
#[test]
fn test_struct_array_builder() {
- let boolean_data = ArrayData::builder(DataType::Boolean)
- .len(4)
- .add_buffer(Buffer::from([false, false, true, true].to_byte_slice()))
Review comment:
This is the line that triggered this PR. As per this code shows, `to_byte_slice` can be applied to a `bool` or slice of bools, which promptly converts it to `u8`. However, `bool` is not represented as `u8` in the Arrow specification. Consequently, this `ArrayData` is incorrect, as it is built from 4 bytes, but represents 4 slots of booleans (line above), 4 bits or 1 byte (not 4).
This indicates a problem in the `ToByteSlice`, which should not allow `bool` to be used there. I have concrete ideas to solve this, but is a larger PR and I am trying to split it in smaller chunks.
----------------------------------------------------------------
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