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/09/06 08:47:42 UTC
[GitHub] [arrow] jorgecarleitao opened a new pull request #8118: ARROW-9922: [Rust] Add StructArray::TryFrom and deprecate StructBuilder (+40%)
jorgecarleitao opened a new pull request #8118:
URL: https://github.com/apache/arrow/pull/8118
(This PR removes a lot of code ^_^)
(This PR is built on top of #8117)
The core problem that this PR addresses is the construction of a `StructArray`, whose spec can be found [here](https://arrow.apache.org/docs/format/Columnar.html#struct-layout).
The current API to build a `StructArray` of 4 entries of fixed type is (part of a test):
```rust
let string_builder = StringBuilder::new(4);
let int_builder = Int32Builder::new(4);
let mut fields = Vec::new();
let mut field_builders = Vec::new();
fields.push(Field::new("f1", DataType::Utf8, false));
field_builders.push(Box::new(string_builder) as Box<ArrayBuilder>);
fields.push(Field::new("f2", DataType::Int32, false));
field_builders.push(Box::new(int_builder) as Box<ArrayBuilder>);
let mut builder = StructBuilder::new(fields, field_builders);
assert_eq!(2, builder.num_fields());
let string_builder = builder
.field_builder::<StringBuilder>(0)
.expect("builder at field 0 should be string builder");
string_builder.append_value("joe").unwrap();
string_builder.append_null().unwrap();
string_builder.append_null().unwrap();
string_builder.append_value("mark").unwrap();
let int_builder = builder
.field_builder::<Int32Builder>(1)
.expect("builder at field 1 should be int builder");
int_builder.append_value(1).unwrap();
int_builder.append_value(2).unwrap();
int_builder.append_null().unwrap();
int_builder.append_value(4).unwrap();
builder.append(true).unwrap();
builder.append(true).unwrap();
builder.append_null().unwrap();
builder.append(true).unwrap();
let arr = builder.finish();
```
This PR's proposal for the same array:
```rust
let strings: ArrayRef = Arc::new(StringArray::from(vec![
Some("joe"),
None,
None,
Some("mark"),
]));
let ints: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(2), None, Some(4)]));
let arr = StructArray::try_from(vec![("f1", strings.clone()), ("f2", ints.clone())]).unwrap();
```
Note that:
* There is no `Field`, only name: the attributes (type and nullability) are obtained from the `ArrayData`'s itself, and thus there a guarantee that the field's attributes are aligned with the Data.
* The implementation is dynamically typed: the type is obtained from `Array::data_type`, instead of having to match Field's datatype to each field' builders
* `Option` is used to specify whether the quantity is null or not
The construction uses an OR on the entry's null bitmaps to decide whether the struct null bitmap is null at a given index. I.e. the third index of the example in [the spec](https://arrow.apache.org/docs/format/Columnar.html#struct-layout) is obtained by checking if all fields are null at that index.
There is an edge case, that this constructor is unable to build (and the user needs to use the other `From`): a struct with a `0` at position X and all field's bitmap at position X to be `1`:
```
# array of 1 entry:
bitmap struct = [0]
bitmap field1 = [1]
bitmap field2 = [1]
```
this is because, in this `TryFrom`, the bitmap of the struct is computed from a bitwise `or` of the field's entries.
IMO this is a non-issue because a `null` in the struct already implies an `unspecified` value on every field and thus that field's value is already assumed to be undefined. However, this is important to mention as a round-trip with this case will fail: in the example above, `bitmap struct` will have a `1`.
Finally, this has a performance improvement of 40%.
<details>
<summary>Benchmark results</summary>
```
git checkout HEAD^ && cargo bench --bench array_from_vec -- struct_array_from_vec && git checkout no_builder1 && cargo bench --bench array_from_vec -- struct_array_from_vec
```
```
struct_array_from_vec 128
time: [8.6816 us 8.8225 us 9.0221 us]
change: [-39.407% -38.457% -37.537%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
2 (2.00%) high mild
3 (3.00%) high severe
struct_array_from_vec 256
time: [11.218 us 11.448 us 11.707 us]
change: [-44.091% -42.210% -40.540%] (p = 0.00 < 0.05)
Performance has improved.
struct_array_from_vec 512
time: [16.402 us 17.445 us 18.638 us]
change: [-49.129% -46.775% -44.109%] (p = 0.00 < 0.05)
Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
2 (2.00%) high mild
9 (9.00%) high severe
struct_array_from_vec 1024
time: [25.527 us 26.019 us 26.648 us]
change: [-46.410% -42.028% -37.256%] (p = 0.00 < 0.05)
Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
5 (5.00%) high mild
5 (5.00%) high severe
```
</details>
## Final note:
The general direction that I am heading with this is to minimize the usage of builders. My issue with builders is that they are statically typed and perform incremental changes, but almost all our operations are dynamically typed and in bulk: batch read, batch write, etc. As such, it is often faster (and much simpler from UX's perspective) to create a `Vec<Option<_>>` and use it to create an Arrow Array.
FYI @nevi-me @andygrove @alamb
----------------------------------------------------------------
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 #8118: ARROW-9922: [Rust] Add StructArray::TryFrom and deprecate StructBuilder (+40%)
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8118:
URL: https://github.com/apache/arrow/pull/8118#issuecomment-687912467
> but I would guess that they are not widely used for code that has high performance requirements.
That is interesting. So, in terms of performance, is it fair to rank then as:
1. Buffers / ArrayData
2. `From`
3. Builder*
----------------------------------------------------------------
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 edited a comment on pull request #8118: ARROW-9922: [Rust] Add StructArray::TryFrom and deprecate StructBuilder (+40%)
Posted by GitBox <gi...@apache.org>.
jorgecarleitao edited a comment on pull request #8118:
URL: https://github.com/apache/arrow/pull/8118#issuecomment-687891730
> Maybe I am misunderstanding, but I think there may be a flaw with this approach and we're not comparing apples with apples when looking at the benchmarks.
>
> The original code is dynamically building a struct using the builder. The new code starts with a `vec!` where everything is known at compile time. In theory, the builders should be more efficient than building a `Vec` and then converting it.
I though that `criterion::black_box()` would block the compiler from optimizing the code on it, so that the benchmark would not be tainted by compiler optimizations. I use these in both the Builder and `From`.
Regardless, the reason I used this approach was because I looked through the code on where we use Builders, and I found two main inputs:
* a vector:
* constructed from reading batches of rows (e.g. `StringRecord` in CSV, `&[Value]` in json)
* constructed in memory from some external source (e.g. `MemoryScan`)
* an Arrow Array, in most in-memory calculations (e.g. `RecordBatch` and `ArrayRef`, in `compute` and DataFusion)
In all cases, we use the builders to append rows row-by-row:
* see [here](https://github.com/apache/arrow/blob/master/rust/arrow/src/csv/reader.rs#L432) for CSV
* see [here](https://github.com/apache/arrow/blob/master/rust/arrow/src/json/reader.rs#L491) for JSON
* in parquet [we do not use Array builders](https://github.com/apache/arrow/blob/master/rust/parquet/src/arrow/array_reader.rs#L27)
* see [here](https://github.com/apache/arrow/blob/master/rust/arrow/src/compute/kernels/cast.rs#L207) for an example in compute
Based on this analysis, I though that:
* this benchmark was a good representation of our use-cases
* we can use `[Try]From` to build our results instead of a builder. The `from` is essentially `builder.append_many().finish()`, with a significantly simpler API
----------------------------------------------------------------
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 #8118: ARROW-9922: [Rust] Add StructArray::TryFrom (+40%)
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8118:
URL: https://github.com/apache/arrow/pull/8118#issuecomment-695335519
@nevi-me and @andygrove , I reverted the change wrt to the builder, so that this is an additive PR.
@andygrove, wrt to the dynamically building the array, note that a StructArray is almost only composed by child data: the struct itself is a null bitmap and some pointers. Therefore, the cost of building a Struct will always be driven by the allocation of those buffers.
With that said, you are right that during the creation of the fields, the benchmark clones the arrays, while a builder will build them on the fly and thus reduce memory footprint.
IMO that issue is separated from the creation of the struct itself (but related to the build of its childs): it is how we efficiently build non-struct arrays without first allocating vectors, that the builders aimed at solving. I am outlying some of this on #8211, which allows to build primitive Arrays from an iterator without exposing a unsafe API to users and would avoid the double allocation that you refer 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] andygrove commented on a change in pull request #8118: ARROW-9922: [Rust] Add StructArray::TryFrom (+40%)
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8118:
URL: https://github.com/apache/arrow/pull/8118#discussion_r492065611
##########
File path: rust/arrow/src/array/array.rs
##########
@@ -834,7 +840,7 @@ impl From<Vec<Option<bool>>> for BooleanArray {
fn from(data: Vec<Option<bool>>) -> Self {
let data_len = data.len();
let num_byte = bit_util::ceil(data_len, 8);
- let mut null_buf = MutableBuffer::new(num_byte).with_bitset(num_byte, false);
+ let mut null_buf = make_null_buffer(data.len());
let mut val_buf = MutableBuffer::new(num_byte).with_bitset(num_byte, false);
Review comment:
Shouldn't `val_buf` also be initialized by calling `make_null_buffer`?
----------------------------------------------------------------
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 #8118: ARROW-9922: [Rust] Add StructArray::TryFrom and deprecate StructBuilder (+40%)
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8118:
URL: https://github.com/apache/arrow/pull/8118#issuecomment-687907669
> Aside from that, I can't see how we can remove the StructBuilder though. The builder has semantics that I don't think we can cover by creating a struct from individual ArrayRefs representing fields. For example, how would we append a null struct versus appending a struct with null fields?
I will roll back the builder and associated tests.
Just for my understanding, since I do not know the background: is it fair to say that the primary purpose of builders is to enable users to create Arrow Arrays without having to fiddle with `Buffers`, `ArrayData`? I.e. they are like helpers.
----------------------------------------------------------------
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 #8118: ARROW-9922: [Rust] Add StructArray::TryFrom and deprecate StructBuilder (+40%)
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #8118:
URL: https://github.com/apache/arrow/pull/8118#discussion_r484088318
##########
File path: rust/arrow/src/array/builder.rs
##########
@@ -1872,294 +1871,6 @@ impl FixedSizeBinaryBuilder {
}
}
Review comment:
I did not want to add new code without removing other code, but I am fine either way.
----------------------------------------------------------------
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] andygrove commented on pull request #8118: ARROW-9922: [Rust] Add StructArray::TryFrom and deprecate StructBuilder (+40%)
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8118:
URL: https://github.com/apache/arrow/pull/8118#issuecomment-687909038
Yes, that's correct. These are the main high-level APIs for users to create
Arrow structures. The convenience methods that convert from Vec are great
for examples and tests but I would guess that they are not widely used for
code that has high performance requirements.
On Sun, Sep 6, 2020 at 3:34 PM Jorge Leitao <no...@github.com>
wrote:
> Aside from that, I can't see how we can remove the StructBuilder though.
> The builder has semantics that I don't think we can cover by creating a
> struct from individual ArrayRefs representing fields. For example, how
> would we append a null struct versus appending a struct with null fields?
>
> I will roll back the builder and associated tests.
>
> Just for my understanding, since I do not know the background: is it fair
> to say that the primary purpose of builders is to enable users to create
> Arrow Arrays without having to fiddle with Buffers, ArrayData? I.e. they
> are like helpers.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/arrow/pull/8118#issuecomment-687907669>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAHEBRD56FPE45RQ4ILMOQ3SEP56DANCNFSM4Q4DPXMQ>
> .
>
----------------------------------------------------------------
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 #8118: ARROW-9922: [Rust] Add StructArray::TryFrom and deprecate StructBuilder (+40%)
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #8118:
URL: https://github.com/apache/arrow/pull/8118#discussion_r484066509
##########
File path: rust/arrow/src/array/builder.rs
##########
@@ -1872,294 +1871,6 @@ impl FixedSizeBinaryBuilder {
}
}
Review comment:
Wouldn't it be safer to deprecate the code (not sure what That's deprecation annotation is), then remove it 2 releases from now?
We don't yet have a Rust-specific deprecation policy, so I'm still fine if other opinions are to remove the builder immediately.
----------------------------------------------------------------
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] andygrove commented on pull request #8118: ARROW-9922: [Rust] Add StructArray::TryFrom and deprecate StructBuilder (+40%)
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8118:
URL: https://github.com/apache/arrow/pull/8118#issuecomment-687844129
Maybe I am misunderstanding, but I think there may be a flaw with this approach and we're not comparing apples with apples when looking at the benchmarks.
The original code is dynamically building a struct using the builder. The new code starts with a `vec!` where everything is known at compile time. In theory, the builders should be more efficient than building a `Vec` and then converting 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] andygrove commented on a change in pull request #8118: ARROW-9922: [Rust] Add StructArray::TryFrom (+40%)
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8118:
URL: https://github.com/apache/arrow/pull/8118#discussion_r492065611
##########
File path: rust/arrow/src/array/array.rs
##########
@@ -834,7 +840,7 @@ impl From<Vec<Option<bool>>> for BooleanArray {
fn from(data: Vec<Option<bool>>) -> Self {
let data_len = data.len();
let num_byte = bit_util::ceil(data_len, 8);
- let mut null_buf = MutableBuffer::new(num_byte).with_bitset(num_byte, false);
+ let mut null_buf = make_null_buffer(data.len());
let mut val_buf = MutableBuffer::new(num_byte).with_bitset(num_byte, false);
Review comment:
Shouldn't `val_buf` also be initialized by calling `make_null_buffer`?
----------------------------------------------------------------
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 #8118: ARROW-9922: [Rust] Add StructArray::TryFrom and deprecate StructBuilder (+40%)
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8118:
URL: https://github.com/apache/arrow/pull/8118#issuecomment-687732718
https://issues.apache.org/jira/browse/ARROW-9922
----------------------------------------------------------------
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] andygrove commented on pull request #8118: ARROW-9922: [Rust] Add StructArray::TryFrom and deprecate StructBuilder (+40%)
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8118:
URL: https://github.com/apache/arrow/pull/8118#issuecomment-687895865
Sorry, I should have been clearer with my comments. I was referring to the code samples in the PR description. The first example is using the builders to push data into the contiguous buffers that will eventually become the arrays. The second example is building an intermediate data structure (the Vec) and then copying from the Vec into buffers. so there is 2x the memory usage and additional memory allocations and copies. Maybe I'm reading too much into these examples though.
Aside from that, I can't see how we can remove the StructBuilder though. The builder has semantics that I don't think we can cover by creating a struct from individual ArrayRefs representing fields. For example, how would we append a null struct versus appending a struct with null fields?
----------------------------------------------------------------
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 #8118: ARROW-9922: [Rust] Add StructArray::TryFrom and deprecate StructBuilder (+40%)
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #8118:
URL: https://github.com/apache/arrow/pull/8118#issuecomment-687891730
> Maybe I am misunderstanding, but I think there may be a flaw with this approach and we're not comparing apples with apples when looking at the benchmarks.
>
> The original code is dynamically building a struct using the builder. The new code starts with a `vec!` where everything is known at compile time. In theory, the builders should be more efficient than building a `Vec` and then converting it.
I though that `criterion::black_box()` would block the compiler from optimizing the code on it, so that the benchmark would not be tainted by compiler optimizations. I use these in both the Builder and `From`.
Regardless, the reason I used this approach was because I looked through the code on where we use Builders, and I found two main inputs:
* a vector:
* constructed from reading batches of rows (e.g. `StringRecord` in CSV, `&[Value]` in json)
* constructed in memory from some external source (e.g. `MemoryScan`)
* an Arrow Array, in most in-memory calculations (e.g. `RecordBatch` and `ArrayRef`, in `compute` and DataFusion)
In all cases, we use the builders to append rows row-by-row:
* see [here](https://github.com/apache/arrow/blob/master/rust/arrow/src/csv/reader.rs#L432) for CSV
* see [here](https://github.com/apache/arrow/blob/master/rust/arrow/src/json/reader.rs#L491) for JSON
* in parquet [we do not use Array builders](https://github.com/apache/arrow/blob/master/rust/parquet/src/arrow/array_reader.rs#L27)
* see [here](https://github.com/apache/arrow/blob/master/rust/arrow/src/compute/kernels/cast.rs#L207) for an example in compute
Based on this analysis, I though that:
* this benchmark was a good representation of our use-cases
* we can use `[Try]From` to build our results instead of a builder. The `from` is essentially `builder.append_data().finish()`, with a significantly simpler API
----------------------------------------------------------------
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] andygrove closed pull request #8118: ARROW-9922: [Rust] Add StructArray::TryFrom (+40%)
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #8118:
URL: https://github.com/apache/arrow/pull/8118
----------------------------------------------------------------
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] andygrove closed pull request #8118: ARROW-9922: [Rust] Add StructArray::TryFrom (+40%)
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #8118:
URL: https://github.com/apache/arrow/pull/8118
----------------------------------------------------------------
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