AVRO-4200: [C++] Encode and decode std::optional #3545
AVRO-4200: [C++] Encode and decode std::optional #3545philippeVerney wants to merge 2 commits intoapache:mainfrom
Conversation
lang/c++/include/avro/Specific.hh
Outdated
| case 1: { | ||
| T t; | ||
| avro::decode(d, t); | ||
| s.emplace(t); |
There was a problem hiding this comment.
std::move(t) here could avoid a copy. Compiler Explorer
There was a problem hiding this comment.
100% agree. I change that.
There was a problem hiding this comment.
I actually even preferred in place construction. Hoping it is fine with you.
| #include "AvroTraits.hh" | ||
| #include "Config.hh" | ||
| #include "Decoder.hh" | ||
| #include "Encoder.hh" |
There was a problem hiding this comment.
| #include "Encoder.hh" | |
| #include "Exception.hh" |
for the new usage at https://github.com/apache/avro/pull/3545/files#diff-6a9c42629e99021e3a2ea7e61e2c364940c9e04228a8b382ea47032f04d44c74R192
| /** | ||
| * Encodes a given value. | ||
| */ | ||
| static void encode(Encoder &e, const std::optional<T> &b) { |
There was a problem hiding this comment.
This makes an assumption that the schema is ["null", T].
This is true most of the time but it is not mandatory. Without having the schema in scope I see no way how to improve it.
There was a problem hiding this comment.
I add this remark in a comment not to lost it. Thanks.
Could you give me another schema example of an AVRO optional please.
I have some "optional" as ["null", U, V, T, ...] but I map them to std::variant for now (other PR to come maybe)
There was a problem hiding this comment.
AVRO optional
There is no such thing as Avro optional. The type is Union. It can have from 2 to N variants.
"null" is not a mandatory variant. It could be at any index in the variants, but usually it is the first one. Without a Schema it is a wild guess.
There was a problem hiding this comment.
Got it now, thanks for the explanation.
I don't see anything else to improve about your remark now that I added the comment in the documentation of the code_traits.
However, I now perfectly share your concern.
There was a problem hiding this comment.
I like the idea of standard encoding for optional<T>. Users are expected to use it only if the schema is [null, T]. We can even make the code generator use optional<T> whenever the schema is [null, T]. This will break compatibility, of course. So there should be a switch for the generator to make this optional (pun intended).
Since this PR does not break compatibility and makes its intent clear, I think, we should merge it.
lang/c++/include/avro/Specific.hh
Outdated
| */ | ||
| 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.
| if (n >= 2) { throw avro::Exception("Union index too big"); } | |
| if (n >= 2) { throw avro::Exception("Union index too big for optional (expected 0 or 1, got " + std::to_string(n) + ")"); } |
Add missing header. Improve error message. Document expected corresponding schema. Prefere optional.reset() over optional = std::nullopt
4 participants
What is the purpose of the change
This pull request allows (C++ 17) std::optional structure to be encoded and decoded.
Verifying this change
This change added tests and can be verified as follows:
Documentation