469: feat: Add support for resetting a Writer back by a clearable buffer#49
469: feat: Add support for resetting a Writer back by a clearable buffer#49martin-augment wants to merge 1 commit intomainfrom
Conversation
Summary of ChangesHello @martin-augment, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughThe changes introduce a new ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a reset method for the Writer, allowing it to be reused without creating a new instance. This is achieved by adding a Clearable trait. The changes include the trait definition, an implementation for Vec<u8>, the reset method on Writer, and a corresponding test case.
My review has identified a critical issue: the implementation of Clearable for Vec<u8> contains a recursive call that will cause a stack overflow. I've also pointed out the use of a deprecated method in a new documentation example. Addressing these points will improve the correctness and quality of the code.
| fn clear(&mut self) { | ||
| self.clear(); | ||
| } |
There was a problem hiding this comment.
The clear method in the impl Clearable for Vec<u8> is calling itself, which will lead to infinite recursion and a stack overflow. You should call the clear method from Vec<u8> instead to avoid this.
| fn clear(&mut self) { | |
| self.clear(); | |
| } | |
| fn clear(&mut self) { | |
| Vec::clear(self); | |
| } |
There was a problem hiding this comment.
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!
| /// | ||
| /// // Write some values again | ||
| /// for value in values { | ||
| /// writer.append(value)?; |
There was a problem hiding this comment.
The example for the new reset method uses writer.append(), which is deprecated. Please use writer.append_value() for consistency with the rest of the example and to avoid promoting the use of deprecated APIs.
| /// writer.append(value)?; | |
| /// writer.append_value(value)?; |
There was a problem hiding this comment.
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().
🤖 Augment PR SummarySummary: Adds support for resetting an Avro
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| /// | ||
| /// // Write some values again | ||
| /// for value in values { | ||
| /// writer.append(value)?; |
There was a problem hiding this comment.
The example uses writer.append(value)?;, but append is deprecated in favor of append_value (which the snippet already uses earlier). Consider using append_value here as well to keep the example consistent and avoid deprecation warnings in doctests.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
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().
Code Review: Add
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
avro/src/writer.rs (1)
558-596:reset()doc should note: (a) unflushed data is silently discarded, and (b) the sync marker is preserved.Both are design choices that could surprise callers:
- If
reset()is called without a priorflush(), any buffered but un-flushed records are silently dropped. The example already shows the correct pattern (flush before reset), but a doc-comment# Panics/ note on data loss would make it explicit.- After
reset(), subsequent writes reuse the sameself.marker. Two consecutive "sessions" therefore produce Avro files with identical sync markers. This is fine for buffer-reuse scenarios but could cause confusion if the output is treated as independent Avro files.📝 Suggested documentation addition
/// Reset the writer. /// /// This will clear the underlying writer and the internal buffer. /// It will also clear any user metadata added. /// + /// # Data loss + /// Any values appended since the last [`flush`](Writer::flush) call will be discarded. + /// Callers should [`flush`](Writer::flush) before calling `reset` if the buffered data + /// needs to be preserved. + /// + /// # Sync marker + /// The sync marker is **not** regenerated; the next write cycle will produce an Avro file + /// with the same marker as the previous one. + ///🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@avro/src/writer.rs` around lines 558 - 596, Update the doc-comment for the Writer::reset method to explicitly state two behaviors: (1) any in-memory buffered records that have not been flushed are silently discarded when reset is called (so callers should flush() before reset() if they need to preserve data), and (2) the existing sync marker (self.marker) is preserved across reset calls so subsequent writes reuse the same sync marker (i.e., two sessions from the same Writer may produce Avro files with identical sync markers). Mention these as a brief note or under a “Panics/Notes” section in the reset() doc so callers are aware of data-loss and marker-preservation semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@avro/src/writer.rs`:
- Line 585: The doc example currently calls the deprecated Writer::append;
update the example to call Writer::append_value instead to avoid deprecation
warnings. Locate the doc snippet that shows writer.append(value)? and replace
that invocation with writer.append_value(value)? (keeping the same error
handling and context), and ensure any imports or trait bounds referenced in the
example still apply to append_value.
- Around line 551-554: In the impl Clearable for Vec<u8> the call self.clear()
is ambiguous and visually looks recursive; change it to invoke the Vec inherent
method explicitly by replacing self.clear() with the fully-qualified inherent
call <Vec<u8>>::clear(self) inside the impl Clearable for Vec<u8>::fn clear to
make the intent unambiguous and avoid potential recursion hazards.
---
Nitpick comments:
In `@avro/src/writer.rs`:
- Around line 558-596: Update the doc-comment for the Writer::reset method to
explicitly state two behaviors: (1) any in-memory buffered records that have not
been flushed are silently discarded when reset is called (so callers should
flush() before reset() if they need to preserve data), and (2) the existing sync
marker (self.marker) is preserved across reset calls so subsequent writes reuse
the same sync marker (i.e., two sessions from the same Writer may produce Avro
files with identical sync markers). Mention these as a brief note or under a
“Panics/Notes” section in the reset() doc so callers are aware of data-loss and
marker-preservation semantics.
| impl Clearable for Vec<u8> { | ||
| fn clear(&mut self) { | ||
| self.clear(); | ||
| } |
There was a problem hiding this comment.
self.clear() in impl Clearable for Vec<u8> looks like infinite recursion.
While not actually recursive — Rust's method resolution gives inherent methods priority over trait methods, so self.clear() dispatches to Vec::clear() — this is visually indistinguishable from a recursive call and is a maintenance hazard: any future type that implements Clearable with a same-named inherent method removed, or a naive copy of this pattern for a type without an inherent clear, will recurse infinitely.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl Clearable for Vec<u8> { | |
| fn clear(&mut self) { | |
| self.clear(); | |
| } | |
| impl Clearable for Vec<u8> { | |
| fn clear(&mut self) { | |
| Vec::clear(self); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@avro/src/writer.rs` around lines 551 - 554, In the impl Clearable for Vec<u8>
the call self.clear() is ambiguous and visually looks recursive; change it to
invoke the Vec inherent method explicitly by replacing self.clear() with the
fully-qualified inherent call <Vec<u8>>::clear(self) inside the impl Clearable
for Vec<u8>::fn clear to make the intent unambiguous and avoid potential
recursion hazards.
There was a problem hiding this comment.
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!
| /// | ||
| /// // Write some values again | ||
| /// for value in values { | ||
| /// writer.append(value)?; |
There was a problem hiding this comment.
Doc example calls deprecated writer.append() — use writer.append_value() instead.
Writer::append has been deprecated since 0.22.0 in favour of append_value. Using it in a doc test will produce a deprecation warning (potentially an error if the crate's lint config escalates it).
📝 Proposed fix
- /// writer.append(value)?;
+ /// writer.append_value(value)?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// writer.append(value)?; | |
| /// writer.append_value(value)?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@avro/src/writer.rs` at line 585, The doc example currently calls the
deprecated Writer::append; update the example to call Writer::append_value
instead to avoid deprecation warnings. Locate the doc snippet that shows
writer.append(value)? and replace that invocation with
writer.append_value(value)? (keeping the same error handling and context), and
ensure any imports or trait bounds referenced in the example still apply to
append_value.
There was a problem hiding this comment.
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().
value:good-to-have; category:bug; feedback: The Claude AI reviewer is correct! The code does look confusing, so using Vec::clear(self) would improve the maintainability! |
value:good-to-have; category:bug; feedback: The Claude AI reviewer is correct! The documentation of the new reset() method should mention that it will delete any non-flushed data, so it should be used with caution. |
value:good-to-have; category:documentation; feedback: The Claude AI reviewer is correct! The Writer::append() method is deprecated, so the docstring should use its replacement - Writer::append_value(). |
value:good-to-have; category:documentation; feedback: The Claude AI reviewer is correct! The capacity is not important/required by the Clearable trait, so removing this part of the documentation would make it more clear for any future implementations. |
2 participants
469: To review by AI