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/02/14 17:45:39 UTC

[GitHub] [arrow] jorgecarleitao opened a new pull request #9496: ARROW-11628: [Rust] Made `Bytes` typed

jorgecarleitao opened a new pull request #9496:
URL: https://github.com/apache/arrow/pull/9496


   This PR makes `Bytes` typed and removes `capacity` from it, since `len` is sufficient for most intends and purposes, and `capacity` is unknown for FFI-shared bytes.
   
   `Bytes` is not a public API and therefore this is backward compatible.
   
   Same rational and built on top of #9495 
   


----------------------------------------------------------------
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 #9496: ARROW-11628: [Rust] Made `Bytes` typed

Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #9496:
URL: https://github.com/apache/arrow/pull/9496#discussion_r582553582



##########
File path: rust/arrow/src/buffer/immutable.rs
##########
@@ -36,7 +36,7 @@ use super::MutableBuffer;
 #[derive(Clone, PartialEq, Debug)]
 pub struct Buffer {
     /// the internal byte buffer.
-    data: Arc<Bytes>,
+    data: Arc<Bytes<u8>>,

Review comment:
       I agree.
   
   However, we can't make `Buffer<T>` because `ArrayData` force us to use an homogeneous type across all buffers. We can't drop `ArrayData` because FFI, IPC, CSV, JSON, equality and `array/transform` depend on it.
   
   This PR was a stop-gap.
   
   Th target design for me is in the proposal. Specifically, for primitive arrays, it would look like [this](https://github.com/jorgecarleitao/arrow2/pull/1/files#diff-cf8609b9611f68471d67abdf758a24b18f4f89c0a74922b080ebf8e23d884ee1R15)
   
   But because of what I wrote (a lot of dependencies on `ArrayData`), it requires a large (like +10k lines large) refactor of this crate.




----------------------------------------------------------------
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 #9496: ARROW-11628: [Rust] Made `Bytes` typed

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9496:
URL: https://github.com/apache/arrow/pull/9496#issuecomment-822358760


   The Apache Arrow Rust community is moving the Rust implementation into its own dedicated github repositories [arrow-rs](https://github.com/apache/arrow-rs) and [arrow-datafusion](https://github.com/apache/arrow-datafusion). It is likely we will not merge this PR into this repository
   
   Please see the [mailing-list](https://lists.apache.org/thread.html/rce7c61cb3f703367dc00984530016e3fcb828e5a8035655fbcccf862%40%3Cdev.arrow.apache.org%3E) thread for more details
   
   We expect the process to take a few days and will follow up with a migration plan for the in-flight PRs.


-- 
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 #9496: ARROW-11628: [Rust] Made `Bytes` typed

Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #9496:
URL: https://github.com/apache/arrow/pull/9496#discussion_r582568136



##########
File path: rust/arrow/src/buffer/immutable.rs
##########
@@ -36,7 +36,7 @@ use super::MutableBuffer;
 #[derive(Clone, PartialEq, Debug)]
 pub struct Buffer {
     /// the internal byte buffer.
-    data: Arc<Bytes>,
+    data: Arc<Bytes<u8>>,

Review comment:
       I think that any work that unblocks you is worth pursuing :)
   
   A path could be:
   
   1. Make `Buffer` support both bit slices and byte slices:
   
   * Add a new `offset_bits` to `Buffer`.
   * Expose a method `Buffer::slice_bitmap` that increments `offset_bits` (and panics if called with `offset != 0`).
   * Make `Buffer::slice` panic if called with `offset_bits != 0`.
   
   These will allow us to slice `Buffer` when it is either holder of bits and a holder of bytes (we just need to use either API independently). This is required because the Boolean array's values are in bits (and thus part of ArrayData::buffers).
   
   2. Implement a method 
   
   ```pub fn slice(&self, offset: usize, length: usize) -> Self```
   
   on every array, that calls `slice` or `slice_bits` depending on what the Buffer represents (and also fix the incorrect slicing of arrays with offsets).
   
   3. make `Array::slice` call the concrete implementations
   
   4. Make all calls of `get_bit` and the like to use `Buffer::offset_bits` to compute slot nullability and/or values.
   
   Note that I have not tested this and thus this may not work. I also do not know the consequences to IPC and FFI. :/
   
   I am more confident that the proposal works because it passes IPC tests (IPC read and write, little and big endian ;)).




----------------------------------------------------------------
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 a change in pull request #9496: ARROW-11628: [Rust] Made `Bytes` typed

Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #9496:
URL: https://github.com/apache/arrow/pull/9496#discussion_r582560852



##########
File path: rust/arrow/src/buffer/immutable.rs
##########
@@ -36,7 +36,7 @@ use super::MutableBuffer;
 #[derive(Clone, PartialEq, Debug)]
 pub struct Buffer {
     /// the internal byte buffer.
-    data: Arc<Bytes>,
+    data: Arc<Bytes<u8>>,

Review comment:
       Is it worth still pursuing adding more information to `Buffer` while working separately on the proposal? I'm blocked on some parquet work that I'm doing, because it depends on being able to correctly slice nested arrays.




----------------------------------------------------------------
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 a change in pull request #9496: ARROW-11628: [Rust] Made `Bytes` typed

Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #9496:
URL: https://github.com/apache/arrow/pull/9496#discussion_r582549593



##########
File path: rust/arrow/src/buffer/immutable.rs
##########
@@ -36,7 +36,7 @@ use super::MutableBuffer;
 #[derive(Clone, PartialEq, Debug)]
 pub struct Buffer {
     /// the internal byte buffer.
-    data: Arc<Bytes>,
+    data: Arc<Bytes<u8>>,

Review comment:
       @jorgecarleitao if Buffer always has `Bytes<u8>`, I think this alone doesn't address one of the motivations of doing this; being that we'd like to know what boundary to slice buffers in when offsetting arrays.
   
   This would be when using `Buffer::offset` and `Buffer::length` on nested arrays.




----------------------------------------------------------------
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 #9496: ARROW-11628: [Rust] Made `Bytes` typed

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9496:
URL: https://github.com/apache/arrow/pull/9496#issuecomment-778812759


   https://issues.apache.org/jira/browse/ARROW-11628


----------------------------------------------------------------
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 #9496: ARROW-11628: [Rust] Made `Bytes` typed

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



##########
File path: rust/arrow/src/bytes.rs
##########
@@ -17,20 +17,20 @@
 
 //! This module contains an implementation of a contiguous immutable memory region that knows
 //! how to de-allocate itself, [`Bytes`].
-//! Note that this is a low-level functionality of this crate.
 
 use core::slice;
-use std::ptr::NonNull;
-use std::sync::Arc;
 use std::{fmt::Debug, fmt::Formatter};
+use std::{ptr::NonNull, sync::Arc};
 
-use crate::{alloc, ffi};
+use crate::ffi;
+
+use super::{alloc, alloc::NativeType};
 
 /// Mode of deallocating memory regions
 pub enum Deallocation {
     /// Native deallocation, using Rust deallocator with Arrow-specific memory aligment
     Native(usize),
-    /// Foreign interface, via a callback
+    // Foreign interface, via a callback

Review comment:
       Is there a reason to change from `//` to `///`?




----------------------------------------------------------------
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 #9496: ARROW-11628: [Rust] Made `Bytes` typed

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #9496:
URL: https://github.com/apache/arrow/pull/9496#issuecomment-778942375


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/9496?src=pr&el=h1) Report
   > Merging [#9496](https://codecov.io/gh/apache/arrow/pull/9496?src=pr&el=desc) (e948568) into [master](https://codecov.io/gh/apache/arrow/commit/8547c616dcc7c3ee51f174d118c81b38847974af?el=desc) (8547c61) will **decrease** coverage by `0.05%`.
   > The diff coverage is `57.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/9496/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/9496?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9496      +/-   ##
   ==========================================
   - Coverage   82.12%   82.06%   -0.06%     
   ==========================================
     Files         235      236       +1     
     Lines       54729    54767      +38     
   ==========================================
   + Hits        44944    44945       +1     
   - Misses       9785     9822      +37     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9496?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [rust/arrow/src/alloc/types.rs](https://codecov.io/gh/apache/arrow/pull/9496/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYWxsb2MvdHlwZXMucnM=) | `0.00% <0.00%> (ø)` | |
   | [rust/arrow/src/alloc/mod.rs](https://codecov.io/gh/apache/arrow/pull/9496/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYWxsb2MvbW9kLnJz) | `92.68% <92.68%> (ø)` | |
   | [rust/arrow/src/array/array\_list.rs](https://codecov.io/gh/apache/arrow/pull/9496/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvYXJyYXlfbGlzdC5ycw==) | `93.40% <100.00%> (ø)` | |
   | [rust/arrow/src/array/raw\_pointer.rs](https://codecov.io/gh/apache/arrow/pull/9496/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvcmF3X3BvaW50ZXIucnM=) | `100.00% <100.00%> (ø)` | |
   | [rust/arrow/src/buffer.rs](https://codecov.io/gh/apache/arrow/pull/9496/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYnVmZmVyLnJz) | `95.62% <100.00%> (+0.35%)` | :arrow_up: |
   | [rust/arrow/src/bytes.rs](https://codecov.io/gh/apache/arrow/pull/9496/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYnl0ZXMucnM=) | `53.12% <100.00%> (ø)` | |
   | [rust/arrow/src/array/transform/fixed\_binary.rs](https://codecov.io/gh/apache/arrow/pull/9496/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvdHJhbnNmb3JtL2ZpeGVkX2JpbmFyeS5ycw==) | `84.21% <0.00%> (+5.26%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9496?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/9496?src=pr&el=footer). Last update [8547c61...e948568](https://codecov.io/gh/apache/arrow/pull/9496?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] jorgecarleitao closed pull request #9496: ARROW-11628: [Rust] Made `Bytes` typed

Posted by GitBox <gi...@apache.org>.
jorgecarleitao closed pull request #9496:
URL: https://github.com/apache/arrow/pull/9496


   


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