-
Notifications
You must be signed in to change notification settings - Fork 0
469: feat: Add support for resetting a Writer back by a clearable buffer #49
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -542,6 +542,60 @@ impl<'a, W: Write> Writer<'a, W> { | |||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// A buffer that can be cleared. | ||||||||||||||||||||
| pub trait Clearable { | ||||||||||||||||||||
| /// Clear the buffer, keeping the capacity. | ||||||||||||||||||||
| fn clear(&mut self); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| impl Clearable for Vec<u8> { | ||||||||||||||||||||
| fn clear(&mut self) { | ||||||||||||||||||||
| self.clear(); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+551
to
+554
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
While not actually recursive — Rust's method resolution gives inherent methods priority over trait methods, so Use the fully-qualified inherent call to make the intent unambiguous: 🐛 Proposed fix impl Clearable for Vec<u8> {
fn clear(&mut self) {
- self.clear();
+ Vec::clear(self);
}
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! The code does look confusing, so using Vec::clear(self) would improve the maintainability! |
||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| impl<'a, W: Clearable + Write> Writer<'a, W> { | ||||||||||||||||||||
| /// Reset the writer. | ||||||||||||||||||||
| /// | ||||||||||||||||||||
| /// This will clear the underlying writer and the internal buffer. | ||||||||||||||||||||
| /// It will also clear any user metadata added. | ||||||||||||||||||||
| /// | ||||||||||||||||||||
| /// # Example | ||||||||||||||||||||
| /// ``` | ||||||||||||||||||||
| /// # use apache_avro::{Writer, Schema, Error}; | ||||||||||||||||||||
| /// # let schema = Schema::Boolean; | ||||||||||||||||||||
| /// # let values = [true, false]; | ||||||||||||||||||||
| /// # fn send(_: &Vec<u8>) {} | ||||||||||||||||||||
| /// let mut writer = Writer::new(&schema, Vec::new())?; | ||||||||||||||||||||
| /// | ||||||||||||||||||||
| /// // Write some values | ||||||||||||||||||||
| /// for value in values { | ||||||||||||||||||||
| /// writer.append_value(value)?; | ||||||||||||||||||||
| /// } | ||||||||||||||||||||
| /// | ||||||||||||||||||||
| /// // Flush the buffer and only then do something with buffer | ||||||||||||||||||||
| /// writer.flush()?; | ||||||||||||||||||||
| /// send(writer.get_ref()); | ||||||||||||||||||||
| /// | ||||||||||||||||||||
| /// // Reset the writer | ||||||||||||||||||||
| /// writer.reset(); | ||||||||||||||||||||
| /// | ||||||||||||||||||||
| /// // Write some values again | ||||||||||||||||||||
| /// for value in values { | ||||||||||||||||||||
| /// writer.append(value)?; | ||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:documentation; feedback: The Gemini AI reviewer is correct! The Writer::append() method is deprecated, so the docstring should use its replacement - Writer::append_value(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The example uses Severity: low 🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:documentation; feedback: The Augment AI reviewer is correct! The Writer::append() method is deprecated, so the docstring should use its replacement - Writer::append_value(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doc example calls deprecated
📝 Proposed fix- /// writer.append(value)?;
+ /// writer.append_value(value)?;📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. value:good-to-have; category:documentation; feedback: The CodeRabbit AI reviewer is correct! The Writer::append() method is deprecated, so the docstring should use its replacement - Writer::append_value(). |
||||||||||||||||||||
| /// } | ||||||||||||||||||||
| /// | ||||||||||||||||||||
| /// # Ok::<(), Error>(()) | ||||||||||||||||||||
| /// ``` | ||||||||||||||||||||
| pub fn reset(&mut self) { | ||||||||||||||||||||
| self.buffer.clear(); | ||||||||||||||||||||
| self.writer.clear(); | ||||||||||||||||||||
| self.has_header = false; | ||||||||||||||||||||
| self.num_values = 0; | ||||||||||||||||||||
| self.user_metadata.clear(); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| impl<W: Write> Drop for Writer<'_, W> { | ||||||||||||||||||||
| /// Drop the writer, will try to flush ignoring any errors. | ||||||||||||||||||||
| fn drop(&mut self) { | ||||||||||||||||||||
|
|
@@ -1858,4 +1912,31 @@ mod tests { | |||||||||||||||||||
|
|
||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| #[test] | ||||||||||||||||||||
| fn avro_rs_469_reset_writer() -> TestResult { | ||||||||||||||||||||
| let schema = Schema::Boolean; | ||||||||||||||||||||
| let values = [true, false, true, false]; | ||||||||||||||||||||
| let mut writer = Writer::new(&schema, Vec::new())?; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| for value in values { | ||||||||||||||||||||
| writer.append_value(value)?; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| writer.flush()?; | ||||||||||||||||||||
| let first_buffer = writer.get_ref().clone(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| writer.reset(); | ||||||||||||||||||||
| assert_eq!(writer.get_ref().len(), 0); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| for value in values { | ||||||||||||||||||||
| writer.append_value(value)?; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| writer.flush()?; | ||||||||||||||||||||
| let second_buffer = writer.get_ref().clone(); | ||||||||||||||||||||
| assert_eq!(first_buffer, second_buffer); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| 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.
The
clearmethod in theimpl Clearable for Vec<u8>is calling itself, which will lead to infinite recursion and a stack overflow. You should call theclearmethod fromVec<u8>instead to avoid this.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.
value:annoying; category:bug; feedback: The Gemini AI reviewer is not correct! The code works fine because the Rust compiler prefers inherent methods over trait methods. The code does look confusing, so using
Vec::clear(self)would improve the maintainablity!