You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tisonkun (via GitHub)" <gi...@apache.org> on 2024/10/30 13:39:56 UTC
[PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
tisonkun opened a new pull request, #6652:
URL: https://github.com/apache/arrow-rs/pull/6652
Similar to `impl Write for GenericStringBuilder`.
We found this helps in reducing one allocation when append bytes in many calls (compared with gather those bytes into a Vec first).
Not sure if there is some trait suitable for this method but just add it first.
cc @alamb @waynexia
The doc tests cover the basic usage.
--
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
Re: [PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6652:
URL: https://github.com/apache/arrow-rs/pull/6652#discussion_r1822912381
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -387,6 +387,10 @@ impl MutableBuffer {
/// ```
#[inline]
pub fn extend_from_slice<T: ArrowNativeType>(&mut self, items: &[T]) {
+ if items.is_empty() {
Review Comment:
Given the potential for unintentional performance impact of this, I would prefer to not make this change without benchmarks showing it to be beneficial
--
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
Re: [PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #6652:
URL: https://github.com/apache/arrow-rs/pull/6652
--
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
Re: [PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
Posted by "andylokandy (via GitHub)" <gi...@apache.org>.
andylokandy commented on PR #6652:
URL: https://github.com/apache/arrow-rs/pull/6652#issuecomment-2447602847
I'll suggest `builder.commit_row();`
--
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
Re: [PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #6652:
URL: https://github.com/apache/arrow-rs/pull/6652#discussion_r1822805638
##########
arrow-array/src/builder/generic_bytes_builder.rs:
##########
@@ -287,10 +286,9 @@ impl<T: ByteArrayType, V: AsRef<T::Native>> Extend<Option<V>> for GenericByteBui
/// assert_eq!(array.value(0), "foobarbaz");
/// assert_eq!(array.value(1), "v2");
/// ```
-///
pub type GenericStringBuilder<O> = GenericByteBuilder<GenericStringType<O>>;
-impl<O: OffsetSizeTrait> Write for GenericStringBuilder<O> {
+impl<O: OffsetSizeTrait> core::fmt::Write for GenericStringBuilder<O> {
Review Comment:
```suggestion
impl<O: OffsetSizeTrait> std::fmt::Write for GenericStringBuilder<O> {
```
--
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
Re: [PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
Posted by "andylokandy (via GitHub)" <gi...@apache.org>.
andylokandy commented on PR #6652:
URL: https://github.com/apache/arrow-rs/pull/6652#issuecomment-2447573349
I'm the member of [databend](https://github.com/databendlabs/databend), which heavily adopt arrow-rs and I'm sure it is a very common pattern that constructing an item in a StringArrayBuilder/BinaryArrayBuilder by several parts of `&str` or `&[u8]`.
For example, to add a &[0x20] padding before each item of BinaryArray to generate a new BinaryArray, we can do the following:
```rust
for x in array.iter() {
write!(&mut builder, b" ").unwrap();
write!(&mut builder, x).unwrap();
builder.append_value("");
}
```
Moreover, `StringBuilder` has already supported [this pattern](https://docs.rs/arrow/latest/arrow/array/builder/type.GenericStringBuilder.html#:~:text=This%20builder%20also%20implements%20std%3A%3Afmt%3A%3AWrite%20with%20any%20written%20data%20included%20in%20the%20next%20appended%20value.%20This%20allows%20using%20std%3A%3Afmt%3A%3ADisplay%20with%20standard%20Rust%20idioms%20like%20write!%20and%20writeln!%20to%20write%20data%20directly%20to%20the%20builder%20without%20intermediate%20allocations.). We use it in databend smoothly and we hope BinaryBuilder can have the same ability.
--
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
Re: [PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #6652:
URL: https://github.com/apache/arrow-rs/pull/6652#issuecomment-2447234111
Try impl `io::Write` interface. No sure if a good fit.
--
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
Re: [PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
Posted by "andylokandy (via GitHub)" <gi...@apache.org>.
andylokandy commented on code in PR #6652:
URL: https://github.com/apache/arrow-rs/pull/6652#discussion_r1822763778
##########
arrow-array/src/builder/generic_bytes_builder.rs:
##########
@@ -318,13 +316,50 @@ impl<O: OffsetSizeTrait> Write for GenericStringBuilder<O> {
/// assert_eq!(array.value(0), b"foo");
/// assert_eq!(array.value(1), b"\x00\x01\x02");
/// ```
+///
+/// # Example incrementally writing bytes with `write_bytes`
+///
+/// ```
+/// # use std::io::Write;
+/// # use arrow_array::builder::GenericBinaryBuilder;
+/// let mut builder = GenericBinaryBuilder::<i32>::new();
+///
+/// // Write data in multiple `write_bytes` calls
+/// write!(builder, "foo").unwrap();
+/// write!(builder, "bar").unwrap();
+/// // The next call to append_value finishes the current string
+/// // including all previously written strings.
+/// builder.append_value("baz");
+///
+/// // Write second value with a single write call
+/// write!(builder, "v2").unwrap();
+/// // finish the value by calling append_value with an empty string
+/// builder.append_value("");
+///
+/// let array = builder.finish();
+/// assert_eq!(array.value(0), "foobarbaz".as_bytes());
+/// assert_eq!(array.value(1), "v2".as_bytes());
+/// ```
pub type GenericBinaryBuilder<O> = GenericByteBuilder<GenericBinaryType<O>>;
+impl<O: OffsetSizeTrait> std::io::Write for GenericBinaryBuilder<O> {
+ fn write(&mut self, bs: &[u8]) -> std::io::Result<usize> {
+ self.value_builder.append_slice(bs);
+ Ok(bs.len())
+ }
+
+ fn flush(&mut self) -> std::io::Result<()> {
+ Ok(())
+ }
+}
+
#[cfg(test)]
mod tests {
use super::*;
use crate::array::Array;
use crate::GenericStringArray;
+ use std::fmt::Write as FmtWrite;
+ use std::io::Write as IoWrite;
Review Comment:
```suggestion
use std::fmt::Write as _;
use std::io::Write as _;
```
--
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
Re: [PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #6652:
URL: https://github.com/apache/arrow-rs/pull/6652#discussion_r1822667313
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -387,6 +387,10 @@ impl MutableBuffer {
/// ```
#[inline]
pub fn extend_from_slice<T: ArrowNativeType>(&mut self, items: &[T]) {
+ if items.is_empty() {
Review Comment:
Also add this improvement since we'd at least call `builder.append_value("");` to finish the write at the method added above.
Not sure if this "improvement" always valid but it seems highly 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
Re: [PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #6652:
URL: https://github.com/apache/arrow-rs/pull/6652#discussion_r1823011285
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -387,6 +387,10 @@ impl MutableBuffer {
/// ```
#[inline]
pub fn extend_from_slice<T: ArrowNativeType>(&mut self, items: &[T]) {
+ if items.is_empty() {
Review Comment:
Ok. Reverted
--
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
Re: [PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
Posted by "waynexia (via GitHub)" <gi...@apache.org>.
waynexia commented on PR #6652:
URL: https://github.com/apache/arrow-rs/pull/6652#issuecomment-2447475717
From the API level: this looks kind of misleading to me. The proposed `Write::write` or `write_bytes` makes `BinaryBuilder` closer to an opaque byte container, shadowing the concept of `item` and `offset`. I.e., array built from this API would contain only one element. Plus we don't have an API to "cut" the bytes buffer, but need to call `builder.append_value("");` which looks a bit hack.
How about considering something like `Vec::from_raw_parts`, which accepts different buffers obviously and hence avoids the potential misunderstand?
--
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
Re: [PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #6652:
URL: https://github.com/apache/arrow-rs/pull/6652#issuecomment-2447417491
@andylokandy Thanks for your review! Addressed.
--
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
Re: [PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
Posted by "waynexia (via GitHub)" <gi...@apache.org>.
waynexia commented on PR #6652:
URL: https://github.com/apache/arrow-rs/pull/6652#issuecomment-2447588725
> I'm the member of [databend](https://github.com/databendlabs/databend), which heavily adopts arrow-rs and I'm sure it is a very common pattern to construct an item in a StringArrayBuilder/BinaryArrayBuilder by several parts of `&str` or `&[u8]`.
>
> For example, to add a space padding before each item of BinaryArray and generate a new BinaryArray, we can do the following:
>
> ```rust
> for x in array.iter() {
> write!(&mut builder, b" ").unwrap();
> write!(&mut builder, x).unwrap();
> builder.append_value("");
> }
> ```
>
> Moreover, `StringBuilder` has already supported [this pattern](https://docs.rs/arrow/latest/arrow/array/builder/type.GenericStringBuilder.html#:~:text=This%20builder%20also%20implements%20std%3A%3Afmt%3A%3AWrite%20with%20any%20written%20data%20included%20in%20the%20next%20appended%20value.%20This%20allows%20using%20std%3A%3Afmt%3A%3ADisplay%20with%20standard%20Rust%20idioms%20like%20write!%20and%20writeln!%20to%20write%20data%20directly%20to%20the%20builder%20without%20intermediate%20allocations.). We use it in databend smoothly and we hope BinaryBuilder can have the same ability.
Thanks for your insight, this case makes sense to me 🙌 BTW, do you think it's necessary to encapsulate this pattern `builder.append_value("");` into a dedicated method?
--
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
Re: [PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
Posted by "andylokandy (via GitHub)" <gi...@apache.org>.
andylokandy commented on code in PR #6652:
URL: https://github.com/apache/arrow-rs/pull/6652#discussion_r1822761994
##########
arrow-array/src/builder/generic_bytes_builder.rs:
##########
@@ -287,10 +286,9 @@ impl<T: ByteArrayType, V: AsRef<T::Native>> Extend<Option<V>> for GenericByteBui
/// assert_eq!(array.value(0), "foobarbaz");
/// assert_eq!(array.value(1), "v2");
/// ```
-///
pub type GenericStringBuilder<O> = GenericByteBuilder<GenericStringType<O>>;
-impl<O: OffsetSizeTrait> Write for GenericStringBuilder<O> {
Review Comment:
Maybe use std::fmt::Write for symmetry
--
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
Re: [PR] feat: add write_bytes for GenericBinaryBuilder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6652:
URL: https://github.com/apache/arrow-rs/pull/6652#issuecomment-2450716264
Thank you @tisonkun and @tustvold
--
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