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/10/09 15:23:05 UTC
[GitHub] [arrow-rs] alamb opened a new pull request #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
alamb opened a new pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822
# Which issue does this PR close?
Part of https://github.com/apache/arrow-rs/issues/817
# Rationale for this change
This PR is a step towards making arrow-rs Rust `safe` and resolving open RUSTSEC issues.
* `ArrayData::new()` is fundamentally `unsafe` (in the Rust sense) as it relies on the user to pass in valid data or else allows undefined behavior. The API is easy to misuse and should be marked as `unsafe` to reflect this. See #817 for more background.
Builds on @jhorstmann 's work in https://github.com/apache/arrow-rs/pull/813
# What changes are included in this PR?
- [ ] Removes `ArrayData::new()`
- [ ] Creates `unsafe ArrayData::new_unchecked()` and `ArrayData::try_new()`
- [ ] Makes `ArrayDataBuilder::build()` fallible
- [ ] Adds `unsafe ArrayDataBuilder::build_unchecked()`
Note:
* This PR contains only the proposed API changes. It does not contain any validation. I plan to add the extra validation as a follow on PR (I have it partly implemented in https://github.com/apache/arrow-rs/pull/810)
** Splitting the changes into several PRs I think will help with reviews
** I would like to ensure the API changes are included it arrow-rs 6.0 (planning to make a release candidate in the next week or so). We can then add additional validation in 6.1, 6.2, etc as they will be non breaking API changes.
* Other than the API changes noted above, the PR is entirely mechanical to use the new API
# Are there any user-facing changes?
Yes -- the APIs for creating ArrayData are different. This should not affect any users who create Arrays directly, only those using the lower level APIs.
--
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 #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-939316127
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/822?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 [#822](https://codecov.io/gh/apache/arrow-rs/pull/822?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9718f64) into [master](https://codecov.io/gh/apache/arrow-rs/commit/cc9e285f26cf04e8283397b2fca57053de81c1dd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cc9e285) will **increase** coverage by `0.02%`.
> The diff coverage is `90.06%`.
> :exclamation: Current head 9718f64 differs from pull request most recent head 92e4789. Consider uploading reports for the commit 92e4789 to get more accurate results
[](https://codecov.io/gh/apache/arrow-rs/pull/822?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 #822 +/- ##
==========================================
+ Coverage 82.54% 82.56% +0.02%
==========================================
Files 168 168
Lines 47910 47988 +78
==========================================
+ Hits 39545 39622 +77
- Misses 8365 8366 +1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/822?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/array\_map.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2FycmF5L2FycmF5X21hcC5ycw==) | `81.50% <ø> (ø)` | |
| [arrow/src/array/equal/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2FycmF5L2VxdWFsL21vZC5ycw==) | `93.45% <ø> (ø)` | |
| [arrow/src/array/equal/utils.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2FycmF5L2VxdWFsL3V0aWxzLnJz) | `74.00% <ø> (ø)` | |
| [arrow/src/compute/kernels/limit.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9saW1pdC5ycw==) | `100.00% <ø> (ø)` | |
| [arrow/src/json/writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2pzb24vd3JpdGVyLnJz) | `91.92% <ø> (ø)` | |
| [arrow/src/record\_batch.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL3JlY29yZF9iYXRjaC5ycw==) | `92.65% <ø> (ø)` | |
| [arrow/src/util/integration\_util.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL3V0aWwvaW50ZWdyYXRpb25fdXRpbC5ycw==) | `70.10% <ø> (ø)` | |
| [integration-testing/src/lib.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-aW50ZWdyYXRpb24tdGVzdGluZy9zcmMvbGliLnJz) | `0.00% <0.00%> (ø)` | |
| [parquet/src/arrow/arrow\_writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-cGFycXVldC9zcmMvYXJyb3cvYXJyb3dfd3JpdGVyLnJz) | `98.06% <ø> (ø)` | |
| [parquet/src/arrow/levels.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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) | `83.56% <ø> (ø)` | |
| ... and [31 more](https://codecov.io/gh/apache/arrow-rs/pull/822/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/822?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/822?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 [cc9e285...92e4789](https://codecov.io/gh/apache/arrow-rs/pull/822?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 #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-939326028
FYI @jhorstmann @jorgecarleitao @Dandandan @nevi-me
--
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] houqp commented on a change in pull request #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#discussion_r725692357
##########
File path: arrow/src/compute/kernels/arithmetic.rs
##########
@@ -182,15 +186,17 @@ where
// `values` is an iterator with a known size.
let buffer = unsafe { Buffer::from_trusted_len_iter(values) };
- let data = ArrayData::new(
- T::DATA_TYPE,
- left.len(),
- None,
- null_bit_buffer,
- 0,
- vec![buffer],
- vec![],
- );
+ let data = unsafe {
Review comment:
```suggestion
// Safety: generic guarantees array data type matches with buffer value iterator datatype
let data = unsafe {
```
##########
File path: arrow/src/array/array_primitive.rs
##########
@@ -124,31 +124,35 @@ impl<T: ArrowPrimitiveType> PrimitiveArray<T> {
/// Creates a PrimitiveArray based on an iterator of values without nulls
pub fn from_iter_values<I: IntoIterator<Item = T::Native>>(iter: I) -> Self {
let val_buf: Buffer = iter.into_iter().collect();
- let data = ArrayData::new(
- T::DATA_TYPE,
- val_buf.len() / mem::size_of::<<T as ArrowPrimitiveType>::Native>(),
- None,
- None,
- 0,
- vec![val_buf],
- vec![],
- );
+ let data = unsafe {
Review comment:
```suggestion
// Safety: Buffer from iterator trait is sound, generic guarantees array data type matches iterator item type
let data = unsafe {
```
##########
File path: arrow/src/array/array.rs
##########
@@ -414,15 +416,17 @@ pub fn new_null_array(data_type: &DataType, length: usize) -> ArrayRef {
new_null_sized_array::<IntervalDayTimeType>(data_type, length)
}
},
- DataType::FixedSizeBinary(value_len) => make_array(ArrayData::new(
- data_type.clone(),
- length,
- Some(length),
- Some(MutableBuffer::new_null(length).into()),
- 0,
- vec![Buffer::from(vec![0u8; *value_len as usize * length])],
- vec![],
- )),
+ DataType::FixedSizeBinary(value_len) => make_array(unsafe {
Review comment:
```suggestion
// Safety: `value_len` from `DataType::FixedSizeBinary` is used as data unit length value buffer length calculation
DataType::FixedSizeBinary(value_len) => make_array(unsafe {
```
##########
File path: arrow/src/array/array_boolean.rs
##########
@@ -212,15 +213,17 @@ impl<Ptr: Borrow<Option<bool>>> FromIterator<Ptr> for BooleanArray {
}
});
- let data = ArrayData::new(
- DataType::Boolean,
- data_len,
- None,
- Some(null_buf.into()),
- 0,
- vec![val_buf.into()],
- vec![],
- );
+ let data = unsafe {
Review comment:
```suggestion
// Safety: value buffer size is derived directly from iterator size hint above
let data = unsafe {
```
--
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 #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-939457515
> Changes look good to me as well and I agree with @Dandandan that we should add safety notes.
@Dandandan and @houqp --
First I want to emphasize that this PR *does not change the safety of the arrow-rs implementation* -- the code is as safe/unsafe before this PR as it is after this PR.
I agree that all `unsafe` annotations in the arrow codebase should be evaluated for soundness eventually.
However, I propose not requiring such annotations for this PR because:
1. My goal with this PR is not to make arrow-rs totally safe, it is to set the API up so that we can incrementally make it safer going forward (among other things resolving the RUSTSEC security advisories against arrow-rs)
3. All code newly marked as `unsafe` was previously reviewed in other PRs
1. Manually verifying all locations both requires codebase expertise which is quite rare, and is subject to human fallibility -- I would rather have a compiler or runtime code check rather than rely on additional human review.
2. Existing MIRI test coverage, while not perfect, does offer a measure of protection against undefined behavior.
2. I think there is some sense of urgency to get this change in so it can be included in arrow 6.0,
Thus, I propose a multi-pronged approach:
1. When adding runtime validation checks to `ArrayData::try_new()` -- also add a CI check (or maybe always in debug mode) a check that does `ArrayData` validation even in `ArrayData::new_unchecked`
2. Perhaps crowd source (somehow) the existing tickets for adding `safety` annotations (see [list](https://github.com/apache/arrow-rs/issues?q=is%3Aissue+is%3Aopen+label%3Asecurity+%22Document+use+of%22) and when someone works on those, the unsafe uses of `ArrayData::new_unchecked` can be included.
3. Re-evaluate if it is time to bring in arrow2 ideas to the main arrow-rs repo.
I will admit that part of my reason for not wanting to try and annotate all uses of `unsafe` is that doing so is a large / thankless a task, and I suspect the prospect of doing so might have contributed to @jorgecarleitao's choice to focus his time working on arrow2 instead). I think my own time is more productively spent adding runtime verification and/or helping integrate arrow2 (or its approach or whatever) rather than manually evaluating the existing arrow-rs code base.
--
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 #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#discussion_r725614308
##########
File path: arrow/benches/array_from_vec.rs
##########
@@ -31,9 +31,11 @@ fn array_from_vec(n: usize) {
for i in 0..n {
v.push((i & 0xffff) as u8);
}
- let arr_data = ArrayDataBuilder::new(DataType::Int32)
- .add_buffer(Buffer::from(v))
- .build();
+ let arr_data = unsafe {
Review comment:
It might be helpful to note that this is (very old) benchmark crate seemingly last modified by @andygrove and @sunchao https://github.com/apache/arrow-rs/blame/master/arrow/benches/array_from_vec.rs#L29-L38
To be honest it does not look safe to me. I will try and rewrite 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.
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 #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#discussion_r725500478
##########
File path: arrow/src/array/data.rs
##########
@@ -264,6 +278,53 @@ impl ArrayData {
}
}
+ /// Create a new ArrayData, validating that the provided buffers
+ /// form a valid Arrow array of the specified data type.
+ ///
+ /// If `null_count` is not specified, the number of nulls in
+ /// null_bit_buffer is calculated
+ ///
+ /// Note: This is a low level API and most users of the arrow
+ /// crate should create arrays using the methods in the `array`
+ /// module.
+ pub fn try_new(
+ data_type: DataType,
+ len: usize,
+ null_count: Option<usize>,
+ null_bit_buffer: Option<Buffer>,
+ offset: usize,
+ buffers: Vec<Buffer>,
+ child_data: Vec<ArrayData>,
+ ) -> Result<Self> {
+ // Safetly justification: `validate` is (will be) called below
+ let new_self = unsafe {
+ Self::new_unchecked(
+ data_type,
+ len,
+ null_count,
+ null_bit_buffer,
+ offset,
+ buffers,
+ child_data,
+ )
+ };
+
+ new_self.validate()?;
+ Ok(new_self)
+ }
+
+ /// Validates that buffers in this ArrayData are sufficiently
+ /// sized, to store `len` + `offset` total elements of
+ /// `data_type`.
+ ///
+ /// This check is "cheap" in the sense that it does not validate the
+ /// contents of the buffers (e.g. that string offsets for UTF8 arrays
+ /// are within the length of the buffer).
+ pub fn validate(&self) -> Result<()> {
+ // will be filled in a subsequent PR
Review comment:
https://github.com/apache/arrow-rs/pull/810 has an initial set of checks
##########
File path: arrow/src/array/data.rs
##########
@@ -264,6 +278,53 @@ impl ArrayData {
}
}
+ /// Create a new ArrayData, validating that the provided buffers
Review comment:
The API changes in `data.rs` are the core changes in this PR -- everything else is a mechanical changes to use the new APIs.
--
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 #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822
--
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] houqp commented on pull request #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-939549216
Tried adding some safety notes, I have to agree that the task is quite intense :D I also suggest we focus on filling gaps in arrow2 instead of retrofitting arrow-rs, it's a huge undertaking that @jorgecarleitao has already tried and decided that the effort is not worth 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.
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 #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822
--
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] Dandandan commented on pull request #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-941132449
> @Dandandan -- are you OK with merging this PR given the rationale listed in #822 (comment) (you have marked the PR as changes requested)
Hehe 😃 I was just looking at it. Yeah merging as is would be great.
--
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] Dandandan commented on pull request #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-941130631
--
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] Dandandan commented on pull request #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-941130631
@alamb
I agree with your points. The PR already improves on the current state (`::new` leads to UB when used improperly and should be marked unsafe).
--
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 #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-939316127
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/822?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 [#822](https://codecov.io/gh/apache/arrow-rs/pull/822?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (49ef9d7) into [master](https://codecov.io/gh/apache/arrow-rs/commit/cc9e285f26cf04e8283397b2fca57053de81c1dd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cc9e285) will **increase** coverage by `0.02%`.
> The diff coverage is `90.06%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/822?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 #822 +/- ##
==========================================
+ Coverage 82.54% 82.56% +0.02%
==========================================
Files 168 168
Lines 47910 47988 +78
==========================================
+ Hits 39545 39621 +76
- Misses 8365 8367 +2
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/822?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/array\_map.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2FycmF5L2FycmF5X21hcC5ycw==) | `81.50% <ø> (ø)` | |
| [arrow/src/array/equal/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2FycmF5L2VxdWFsL21vZC5ycw==) | `93.45% <ø> (ø)` | |
| [arrow/src/array/equal/utils.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2FycmF5L2VxdWFsL3V0aWxzLnJz) | `74.00% <ø> (ø)` | |
| [arrow/src/compute/kernels/limit.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9saW1pdC5ycw==) | `100.00% <ø> (ø)` | |
| [arrow/src/json/writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2pzb24vd3JpdGVyLnJz) | `91.92% <ø> (ø)` | |
| [arrow/src/record\_batch.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL3JlY29yZF9iYXRjaC5ycw==) | `92.65% <ø> (ø)` | |
| [arrow/src/util/integration\_util.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL3V0aWwvaW50ZWdyYXRpb25fdXRpbC5ycw==) | `70.10% <ø> (ø)` | |
| [integration-testing/src/lib.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-aW50ZWdyYXRpb24tdGVzdGluZy9zcmMvbGliLnJz) | `0.00% <0.00%> (ø)` | |
| [parquet/src/arrow/arrow\_writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-cGFycXVldC9zcmMvYXJyb3cvYXJyb3dfd3JpdGVyLnJz) | `98.06% <ø> (ø)` | |
| [parquet/src/arrow/levels.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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) | `83.56% <ø> (ø)` | |
| ... and [30 more](https://codecov.io/gh/apache/arrow-rs/pull/822/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/822?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/822?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 [cc9e285...49ef9d7](https://codecov.io/gh/apache/arrow-rs/pull/822?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 #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-939315014
I have run out of time for today -- I have a few more places in the code to update to get a clean CI build. Will request review when they are green
--
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] jhorstmann commented on pull request #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-943224146
Thanks @alamb! I was away from a computer for a few days, this looks good!
--
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] Dandandan commented on a change in pull request #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#discussion_r725513714
##########
File path: arrow/benches/array_from_vec.rs
##########
@@ -31,9 +31,11 @@ fn array_from_vec(n: usize) {
for i in 0..n {
v.push((i & 0xffff) as u8);
}
- let arr_data = ArrayDataBuilder::new(DataType::Int32)
- .add_buffer(Buffer::from(v))
- .build();
+ let arr_data = unsafe {
Review comment:
We should add a safety note here?
##########
File path: arrow/src/array/array.rs
##########
@@ -414,15 +416,17 @@ pub fn new_null_array(data_type: &DataType, length: usize) -> ArrayRef {
new_null_sized_array::<IntervalDayTimeType>(data_type, length)
}
},
- DataType::FixedSizeBinary(value_len) => make_array(ArrayData::new(
- data_type.clone(),
- length,
- Some(length),
- Some(MutableBuffer::new_null(length).into()),
- 0,
- vec![Buffer::from(vec![0u8; *value_len as usize * length])],
- vec![],
- )),
+ DataType::FixedSizeBinary(value_len) => make_array(unsafe {
Review comment:
Here too.
--
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 removed a comment on pull request #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
alamb removed a comment on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-939315014
I have run out of time for today -- I have a few more places in the code to update to get a clean CI build. Will request review when they are green
--
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 #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-939370985
> I think we should have some notes around the usages of unsafe: why is it sound in this place?
It is a good idea @Dandandan -- I will attempt to do so. To be honest I am not sure why all the references are legitimate uses of `unsafe`.
I don't think this PR has made the code any more or less safe / unsafe than it was before. However, it is now clearer where assumptions are made (they are all annotated with `unsafe`). I sort of imagine over time people can chip away at removing the `unsafe` possible
--
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] houqp commented on pull request #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-939541187
I agree with you @alamb :+1:
--
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 #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-942511505
Given the feedback I plan to rebase this and merge it in
--
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 #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-939316127
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/822?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 [#822](https://codecov.io/gh/apache/arrow-rs/pull/822?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5449be7) into [master](https://codecov.io/gh/apache/arrow-rs/commit/cc9e285f26cf04e8283397b2fca57053de81c1dd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cc9e285) will **increase** coverage by `0.02%`.
> The diff coverage is `90.06%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/822?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 #822 +/- ##
==========================================
+ Coverage 82.54% 82.56% +0.02%
==========================================
Files 168 168
Lines 47910 47988 +78
==========================================
+ Hits 39545 39622 +77
- Misses 8365 8366 +1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/822?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/array\_map.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2FycmF5L2FycmF5X21hcC5ycw==) | `81.50% <ø> (ø)` | |
| [arrow/src/array/equal/mod.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2FycmF5L2VxdWFsL21vZC5ycw==) | `93.45% <ø> (ø)` | |
| [arrow/src/array/equal/utils.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2FycmF5L2VxdWFsL3V0aWxzLnJz) | `74.00% <ø> (ø)` | |
| [arrow/src/compute/kernels/limit.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9saW1pdC5ycw==) | `100.00% <ø> (ø)` | |
| [arrow/src/json/writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL2pzb24vd3JpdGVyLnJz) | `91.92% <ø> (ø)` | |
| [arrow/src/record\_batch.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL3JlY29yZF9iYXRjaC5ycw==) | `92.65% <ø> (ø)` | |
| [arrow/src/util/integration\_util.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-YXJyb3cvc3JjL3V0aWwvaW50ZWdyYXRpb25fdXRpbC5ycw==) | `70.10% <ø> (ø)` | |
| [integration-testing/src/lib.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-aW50ZWdyYXRpb24tdGVzdGluZy9zcmMvbGliLnJz) | `0.00% <0.00%> (ø)` | |
| [parquet/src/arrow/arrow\_writer.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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-cGFycXVldC9zcmMvYXJyb3cvYXJyb3dfd3JpdGVyLnJz) | `98.06% <ø> (ø)` | |
| [parquet/src/arrow/levels.rs](https://codecov.io/gh/apache/arrow-rs/pull/822/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) | `83.56% <ø> (ø)` | |
| ... and [31 more](https://codecov.io/gh/apache/arrow-rs/pull/822/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/822?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/822?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 [cc9e285...5449be7](https://codecov.io/gh/apache/arrow-rs/pull/822?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 #822: Replace `ArrayData::new()` with `ArrayData::try_new()` and `unsafe ArrayData::new_unchecked`
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #822:
URL: https://github.com/apache/arrow-rs/pull/822#issuecomment-942511505
Given the feedback I plan to rebase this and merge it in
--
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