-
Notifications
You must be signed in to change notification settings - Fork 850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement std::fmt::Write for StringBuilder (#3638) #3659
Conversation
write!(builder, "foo").unwrap(); | ||
builder.append_value(""); | ||
writeln!(builder, "bar").unwrap(); | ||
builder.append_value(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite neat 👍
@@ -235,6 +236,13 @@ impl<T: ByteArrayType, V: AsRef<T::Native>> Extend<Option<V>> for GenericByteBui | |||
/// Array builder for [`GenericStringArray`][crate::GenericStringArray] | |||
pub type GenericStringBuilder<O> = GenericByteBuilder<GenericStringType<O>>; | |||
|
|||
impl<O: OffsetSizeTrait> Write for GenericStringBuilder<O> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impl<O: OffsetSizeTrait> Write for GenericStringBuilder<O> { | |
/// Implement `Write` to support things like `write!(builder, ...`)) | |
/// | |
/// Note each individual call to `write_str` will result in a new row in the builder | |
impl<O: OffsetSizeTrait> Write for GenericStringBuilder<O> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be weird if someone puts a BufWriter
in front of the builder, but with some comments it should be ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each individual call won't result in a new row, you need to explicitly append an empty string to delimit the record
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent thing to document I think
This is pretty neat |
Benchmark runs are scheduled for baseline = fb11792 and contender = 04500e7. 04500e7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Part of #3638
Rationale for this change
I want to use this in the cast kernels in #3647 to avoid needing to allocate strings per value, but split out into a separate PR to highlight, as the interface is perhaps a little strange.
What changes are included in this PR?
Are there any user-facing changes?