3545: AVRO-4200: [C++] Encode and decode std::optional #20
3545: AVRO-4200: [C++] Encode and decode std::optional #20martin-augment wants to merge 1 commit intomainfrom
Conversation
WalkthroughThis pull request adds support for encoding and decoding ✨ 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 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| */ | ||
| static void decode(Decoder &d, std::optional<T> &s) { | ||
| size_t n = d.decodeUnionIndex(); | ||
| if (n >= 2) { throw avro::Exception("Union index too big"); } |
There was a problem hiding this comment.
This header throws avro::Exception here, but Exception.hh isn’t included in this file; consider including it to avoid relying on transitive includes and ensure Specific.hh compiles standalone.
🤖 Was this useful? React with 👍 or 👎
There was a problem hiding this comment.
value:useful; category:bug; feedback:The Augment AI reviewer is correct that the import is needed to be able to build Specific.hh by itself. Currently it compiles only because some other import imports Exception.hh transitively
| case 1: { | ||
| T t; | ||
| avro::decode(d, t); | ||
| s.emplace(t); |
There was a problem hiding this comment.
When decoding into std::optional<T>, this uses emplace(t) which requires a copy; consider constructing from a moved temporary to support move-only T and avoid unnecessary copies.
🤖 Was this useful? React with 👍 or 👎
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback:The Augment AI reviewer is correct that there is a implicit copy of memory here. It could be improved to use a std::move
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| case 1: { | ||
| T t; | ||
| avro::decode(d, t); | ||
| s.emplace(t); |
There was a problem hiding this comment.
Bug: Move Semantics for Optional Values
The decoded value t is being copied into the optional via s.emplace(t) instead of moved. This creates an unnecessary copy for types with expensive copy constructors. The array codec at line 296 uses std::move(t) in the same pattern, so this should use s.emplace(std::move(t)) or s = std::move(t) for consistency and efficiency.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback:The Bugbot AI reviewer is correct that there is a implicit copy of memory here. It could be improved to use a std::move
Code Review for PR #20: AVRO-4200 - C++ std::optional Encoder and DecoderThank you for this contribution! This is a useful addition to the Avro C++ library. I've reviewed the code and have some feedback organized by category: ✅ Strengths
🔍 Issues & ImprovementsCritical: Inconsistent Formatting (Tabs vs Spaces)The documentation comments in the new code use tabs for indentation, while all existing codec_traits use spaces: Lines 175-176 and 187-188 in Specific.hh: /**
* Encodes a given value. // <-- Tab character used here
*/Expected (matching existing style): /**
* Encodes a given value. // <-- Space character used here
*/This breaks consistency with the entire file. Please update these 4 comment lines to use spaces instead of tabs. Bug: Incorrect Union Schema AssumptionThe implementation hardcodes the assumption that the union schema is Current code (lines 177-184): if (b) {
e.encodeUnionIndex(1); // Assumes value is always at index 1
avro::encode(e, b.value());
} else {
e.encodeUnionIndex(0); // Assumes null is always at index 0
e.encodeNull();
}Issues:
Possible solutions:
At minimum, please add documentation clarifying this assumption. Minor: Error Message Could Be More DescriptiveLine 192: if (n >= 2) { throw avro::Exception("Union index too big"); }Suggestion: if (n >= 2) {
throw avro::Exception("Union index too big for optional (expected 0 or 1, got " + std::to_string(n) + ")");
}This helps with debugging when the wrong schema is used. Code Style: Unnecessary BracesLines 194-202: switch (n) {
case 0: {
d.decodeNull();
s = std::nullopt;
} break;
case 1: {
T t;
avro::decode(d, t);
s.emplace(t);
} break;
}The braces around each case are only necessary for Consider removing braces from 🧪 Test CoverageThe tests are good but could be enhanced:
🔒 Security ConsiderationsNo security concerns identified. The code properly:
⚡ Performance Considerations
📝 DocumentationMissing from this PR:
✅ SummaryThis is a solid contribution that adds useful functionality. The main issues to address are: Must fix:
Should fix: Nice to have: Once the formatting is fixed and the schema assumption is documented, this should be good to merge! Review generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lang/c++/include/avro/Specific.hh (2)
175-176: Fix comment indentation to use spaces instead of tabs.The indentation in these comment lines uses tabs, which is inconsistent with the rest of the file that uses spaces.
Apply this diff to fix the formatting:
/** - * Encodes a given value. - */ + * Encodes a given value. + */ static void encode(Encoder &e, const std::optional<T> &b) {/** - * Decodes into a given value. - */ + * Decodes into a given value. + */ static void decode(Decoder &d, std::optional<T> &s) {Also applies to: 188-189
190-203: Consider optimizing to avoid potential copy/move.The decode logic is correct, but Line 201 creates a temporary
T t, decodes into it, then emplaces it into the optional, which may involve a copy or move operation.Consider this optimization to decode directly into the emplaced value:
case 1: { - T t; - avro::decode(d, t); - s.emplace(t); + s.emplace(); + avro::decode(d, s.value()); } break;This avoids the intermediate temporary and any associated copy/move overhead.
lang/c++/test/SpecificTests.cc (1)
131-135: Consider checkinghas_value()before callingvalue().The test logic is correct, but calling
value()without first verifyinghas_value()could throw an exception if decoding fails. Consider making the test more defensive.Apply this diff for a more robust check:
void testNonEmptyOptional() { optional<int64_t> n = -109; optional<int64_t> b = encodeAndDecode(n); - BOOST_CHECK_EQUAL(b.value(), n.value()); + BOOST_REQUIRE(b.has_value()); + BOOST_CHECK_EQUAL(b.value(), n.value()); }Or more simply, use direct optional comparison:
void testNonEmptyOptional() { optional<int64_t> n = -109; optional<int64_t> b = encodeAndDecode(n); - BOOST_CHECK_EQUAL(b.value(), n.value()); + BOOST_CHECK(b == n); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lang/c++/include/avro/Specific.hh(2 hunks)lang/c++/test/SpecificTests.cc(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lang/c++/include/avro/Specific.hh (1)
lang/c++/include/avro/Exception.hh (3)
Exception(31-38)Exception(33-33)Exception(36-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: codex
- GitHub Check: claude-review
- GitHub Check: Cursor Bugbot
🔇 Additional comments (5)
lang/c++/include/avro/Specific.hh (2)
25-25: LGTM!The
<optional>include is correctly added and properly ordered.
177-184: LGTM!The encoding logic correctly handles both cases: present values are encoded with union index 1 followed by the value, and absent values are encoded with union index 0 followed by null.
lang/c++/test/SpecificTests.cc (3)
27-27: LGTM!The
using std::optional;declaration is correctly placed and consistent with the file's style.
137-141: LGTM!The test correctly verifies that an empty optional is properly encoded and decoded, using an appropriate check with
has_value().
207-208: LGTM!The test cases are correctly registered in the test suite, following the existing pattern.
value:good-to-have; category:documentation; feedback:The Claude AI reviewer is correct that the formatting of the file is inconsistent. It does not cause any other issues. |
value:good-to-have; category:bug; feedback:The Claude AI reviewer is correct that the implementation assumes that the Null is the first item in the schema. This is the recommended way but it is not mandatory in the Avro specification. |
2 participants
3545: To review by AI