491: fix!: Remove unused fields in RecordField (order, position)#59
491: fix!: Remove unused fields in RecordField (order, position)#59martin-augment wants to merge 1 commit intomainfrom
RecordField (order, position)#59Conversation
Also changes the type of `aliases` from `Option<Vec<String>>` to `Vec<String>`.
WalkthroughThis pull request refactors field position and order handling in record schema structures. The ✨ 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 |
Summary of ChangesHello, 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 refactoring of 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
|
![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 is a nice refactoring that removes the unused order and position fields from RecordField. This simplification has been correctly propagated throughout the codebase, including parsing, serialization, derive macros, and tests. The change of RecordField.aliases from an Option<Vec<String>> to Vec<String> is also a welcome simplification. Additionally, this PR fixes an issue where the doc field of a RecordField was not being serialized. The changes are well-executed and improve the codebase. I've found one minor issue in the Debug implementation for RecordField which I've commented on.
| // As we are always skipping self.order, always show the .. | ||
| debug.finish_non_exhaustive() | ||
| if self.doc.is_none() | ||
| || !self.aliases.is_empty() |
There was a problem hiding this comment.
The condition !self.aliases.is_empty() seems incorrect for determining whether to use finish_non_exhaustive. finish_non_exhaustive should be used when a field is not displayed. The aliases field is not displayed when it's empty (self.aliases.is_empty()). Therefore, the condition should be self.aliases.is_empty() to correctly indicate that the debug output is incomplete.
| || !self.aliases.is_empty() | |
| || self.aliases.is_empty() |
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Gemini AI reviewer is correct! The check for the aliases should be inverted, otherwise the Debug formatting will look like complete instead of as incomplete.
PR Review: Remove unused fields in
|
| Issue | Severity |
|---|---|
Debug condition bug (!aliases.is_empty() instead of aliases.is_empty()) |
Bug |
RecordFieldOrder removal (breaking public API, Avro spec item) |
Breaking change |
aliases Option<Vec>→Vec (breaking public API) |
Breaking change |
position removal (breaking public API) |
Breaking change |
| Doc serialization fix | Good fix |
| Struct literal in derive macro (not using builder) | Minor concern |
The Debug condition bug should be fixed before merging. The breaking changes are acknowledged in the PR title (fix!) but should be documented in a CHANGELOG entry.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
avro/src/serde/derive.rs (1)
193-195:⚠️ Potential issue | 🟡 MinorUpdate stale docs for
#[serde(with = "...")]helper signature.Line 194 still documents the removed
usizeparameter. That now conflicts with the current trait API and examples.Suggested doc fix
-/// and `fn get_record_fields_in_ctxt(usize, &mut HashSet<Name>, &Namespace) -> Option<Vec<RecordField>>` in the module provided +/// and `fn get_record_fields_in_ctxt(&mut HashSet<Name>, &Namespace) -> Option<Vec<RecordField>>` in the module provided🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@avro/src/serde/derive.rs` around lines 193 - 195, The docs for the serde helper are out of date: update the example/signature that mentions `fn get_record_fields_in_ctxt(usize, &mut HashSet<Name>, &Namespace) -> Option<Vec<RecordField>>` to remove the removed `usize` parameter so it matches the current trait API; ensure both function signatures in the comments read `fn get_schema_in_ctxt(&mut HashSet<Name>, &Namespace) -> Schema` and `fn get_record_fields_in_ctxt(&mut HashSet<Name>, &Namespace) -> Option<Vec<RecordField>>`, and adjust any example text in the same comment block to reflect the new signature and parameter ordering so it no longer conflicts with `AvroSchemaComponent`.
🤖 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/schema/parser.rs`:
- Around line 572-574: The loop that inserts field aliases into the lookup map
(the for alias in &field.aliases { lookup.insert(alias.clone(), position); }
block) can silently overwrite existing entries; modify it to detect collisions
before inserting: check lookup.get(alias) or use lookup.entry(alias.clone()) and
if an entry exists, return an error (or otherwise report) that includes the
conflicting alias and both indices (existing and new position) so callers can
handle duplicate alias/name conflicts; ensure you reference the same symbols
(field.aliases, lookup.insert/lookup.entry, position) when implementing the
check and error path.
In `@avro/src/schema/record/field.rs`:
- Around line 71-75: The Debug exhaustiveness check incorrectly treats present
aliases as "omitted" because the condition uses !self.aliases.is_empty(); change
that to self.aliases.is_empty() inside the same if (the block that calls
finish_non_exhaustive()) so the check becomes: self.doc.is_none() ||
self.aliases.is_empty() || self.default.is_none() ||
self.custom_attributes.is_empty(), ensuring finish_non_exhaustive() is only
invoked when aliases are actually absent.
---
Outside diff comments:
In `@avro/src/serde/derive.rs`:
- Around line 193-195: The docs for the serde helper are out of date: update the
example/signature that mentions `fn get_record_fields_in_ctxt(usize, &mut
HashSet<Name>, &Namespace) -> Option<Vec<RecordField>>` to remove the removed
`usize` parameter so it matches the current trait API; ensure both function
signatures in the comments read `fn get_schema_in_ctxt(&mut HashSet<Name>,
&Namespace) -> Schema` and `fn get_record_fields_in_ctxt(&mut HashSet<Name>,
&Namespace) -> Option<Vec<RecordField>>`, and adjust any example text in the
same comment block to reflect the new signature and parameter ordering so it no
longer conflicts with `AvroSchemaComponent`.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
avro/src/encode.rsavro/src/schema/mod.rsavro/src/schema/parser.rsavro/src/schema/record/field.rsavro/src/schema/record/mod.rsavro/src/schema_compatibility.rsavro/src/schema_equality.rsavro/src/serde/derive.rsavro/src/serde/ser_schema/mod.rsavro/src/types.rsavro/src/writer.rsavro/tests/get_record_fields.rsavro/tests/schema.rsavro_derive/src/lib.rsavro_derive/tests/derive.rs
💤 Files with no reviewable changes (1)
- avro/tests/schema.rs
| for alias in &field.aliases { | ||
| lookup.insert(alias.clone(), position); | ||
| } |
There was a problem hiding this comment.
Prevent silent lookup overwrites for colliding aliases.
Line 573 inserts aliases without collision checks. If an alias matches an existing field name/alias, the previous index is silently replaced, which can corrupt field resolution.
💡 Proposed fix
- for alias in &field.aliases {
- lookup.insert(alias.clone(), position);
- }
+ for alias in &field.aliases {
+ if let Some(old_position) = lookup.insert(alias.clone(), position)
+ && old_position != position
+ {
+ return Err(Details::FieldNameDuplicate(alias.clone()).into());
+ }
+ }📝 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.
| for alias in &field.aliases { | |
| lookup.insert(alias.clone(), position); | |
| } | |
| for alias in &field.aliases { | |
| if let Some(old_position) = lookup.insert(alias.clone(), position) | |
| && old_position != position | |
| { | |
| return Err(Details::FieldNameDuplicate(alias.clone()).into()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@avro/src/schema/parser.rs` around lines 572 - 574, The loop that inserts
field aliases into the lookup map (the for alias in &field.aliases {
lookup.insert(alias.clone(), position); } block) can silently overwrite existing
entries; modify it to detect collisions before inserting: check
lookup.get(alias) or use lookup.entry(alias.clone()) and if an entry exists,
return an error (or otherwise report) that includes the conflicting alias and
both indices (existing and new position) so callers can handle duplicate
alias/name conflicts; ensure you reference the same symbols (field.aliases,
lookup.insert/lookup.entry, position) when implementing the check and error
path.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! The previous version of the code didn't check that the aliases are unique. It would be good the new version to add such check and report this kind of problem to the application developer.
| if self.doc.is_none() | ||
| || !self.aliases.is_empty() | ||
| || self.default.is_none() | ||
| || self.custom_attributes.is_empty() | ||
| { |
There was a problem hiding this comment.
Fix inverted alias condition in Debug exhaustiveness check.
Line 72 is inverted. It currently treats present aliases as “omitted,” which makes finish_non_exhaustive() behavior incorrect.
💡 Proposed fix
- if self.doc.is_none()
- || !self.aliases.is_empty()
+ if self.doc.is_none()
+ || self.aliases.is_empty()
|| self.default.is_none()
|| self.custom_attributes.is_empty()
{📝 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.
| if self.doc.is_none() | |
| || !self.aliases.is_empty() | |
| || self.default.is_none() | |
| || self.custom_attributes.is_empty() | |
| { | |
| if self.doc.is_none() | |
| || self.aliases.is_empty() | |
| || self.default.is_none() | |
| || self.custom_attributes.is_empty() | |
| { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@avro/src/schema/record/field.rs` around lines 71 - 75, The Debug
exhaustiveness check incorrectly treats present aliases as "omitted" because the
condition uses !self.aliases.is_empty(); change that to self.aliases.is_empty()
inside the same if (the block that calls finish_non_exhaustive()) so the check
becomes: self.doc.is_none() || self.aliases.is_empty() || self.default.is_none()
|| self.custom_attributes.is_empty(), ensuring finish_non_exhaustive() is only
invoked when aliases are actually absent.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The CodeRabbit AI reviewer is correct! The check for the aliases should be inverted, otherwise the Debug formatting will look like complete instead of as incomplete.
🤖 Augment PR SummarySummary: This PR makes a breaking cleanup to record-field handling by removing unused Changes:
Technical Notes: Because 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| }), | ||
| None => Err(Details::FieldName(String::from(key)).into()), | ||
| if let Some(position) = self.record_schema.lookup.get(key).copied() { | ||
| let field = &self.record_schema.fields[position]; |
There was a problem hiding this comment.
position comes from record_schema.lookup, but indexing with self.record_schema.fields[position] will panic if the lookup table is ever inconsistent/out-of-bounds (previously this path used .get() and returned a structured error). Consider keeping this path panic-free since RecordSchema can be constructed manually and lookup can also include aliases.
Severity: medium
Other Locations
avro/src/serde/ser_schema/mod.rs:399avro/src/serde/ser_schema/mod.rs:455
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Augment AI reviewer is correct! Using array indexing may lead to a panic if the lookup table becomes invalid for any reason. Using HashMap::get() would be safer since it will lead to returning an error instead of panicking.
value:useful; category:bug; feedback: The CodeRabbit AI reviewer is correct! The signature of the |
value:useful; category:bug; feedback: The Claude AI reviewer is correct! The check for the aliases should be inverted, otherwise the Debug formatting will look like complete instead of as incomplete. |
2 participants
491: To review by AI