Skip to content

AVRO-4200: [C++] Encode and decode std::optional #3545

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.

Sign up for GitHub

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

Open
philippeVerney wants to merge 2 commits into
base: main
Choose a base branch
from
+54 −0
Open

AVRO-4200: [C++] Encode and decode std::optional #3545

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf25928
C++ std::optional Specific Encoder and Decoder
philippeVerney Nov 12, 2025
e702210
In place construction for optional
philippeVerney Nov 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions lang/c++/include/avro/Specific.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "array"
#include <algorithm>
#include <map>
#include <optional>
#include <string>
#include <vector>

Expand Down Expand Up @@ -165,6 +166,44 @@ struct codec_traits<double> {
}
};

/**
* codec_traits for Avro optional.
*/
template<typename T>
struct codec_traits<std::optional<T>> {
/**
* Encodes a given value.
*/
static void encode(Encoder &e, const std::optional<T> &b) {
Copy link
Member

@martin-g martin-g Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@philippeVerney philippeVerney Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@martin-g martin-g Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@philippeVerney philippeVerney Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@thiru-mg thiru-mg Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (b) {
e.encodeUnionIndex(1);
avro::encode(e, b.value());
} else {
e.encodeUnionIndex(0);
e.encodeNull();
}
}

/**
* Decodes into a given value.
*/
static void decode(Decoder &d, std::optional<T> &s) {
size_t n = d.decodeUnionIndex();
if (n >= 2) { throw avro::Exception("Union index too big"); }
Copy link
Member

@martin-g martin-g Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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) + ")"); }

switch (n) {
case 0: {
d.decodeNull();
s = std::nullopt;
} break;
case 1: {
T t;
avro::decode(d, t);
s.emplace(t);
Copy link
Contributor

@KalleOlaviNiemitalo KalleOlaviNiemitalo Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::move(t) here could avoid a copy. Compiler Explorer

Copy link
Author

@philippeVerney philippeVerney Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% agree. I change that.

Copy link
Author

@philippeVerney philippeVerney Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually even preferred in place construction. Hoping it is fine with you.

} break;
}
}
};

/**
* codec_traits for Avro string.
*/
Expand Down
15 changes: 15 additions & 0 deletions lang/c++/test/SpecificTests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

using std::array;
using std::map;
using std::optional;
using std::string;
using std::unique_ptr;
using std::vector;
Expand Down Expand Up @@ -127,6 +128,18 @@ void testDouble() {
BOOST_CHECK_CLOSE(b, n, 0.00000001);
}

void testNonEmptyOptional() {
optional<int64_t> n = -109;
optional<int64_t> b = encodeAndDecode(n);
BOOST_CHECK_EQUAL(b.value(), n.value());
}

void testEmptyOptional() {
optional<int64_t> n;
optional<int64_t> b = encodeAndDecode(n);
BOOST_CHECK(!b.has_value());
}

void testString() {
string n = "abc";
string b = encodeAndDecode(n);
Expand Down Expand Up @@ -191,6 +204,8 @@ init_unit_test_suite(int /*argc*/, char * /*argv*/[]) {
ts->add(BOOST_TEST_CASE(avro::specific::testLong));
ts->add(BOOST_TEST_CASE(avro::specific::testFloat));
ts->add(BOOST_TEST_CASE(avro::specific::testDouble));
ts->add(BOOST_TEST_CASE(avro::specific::testNonEmptyOptional));
ts->add(BOOST_TEST_CASE(avro::specific::testEmptyOptional));
ts->add(BOOST_TEST_CASE(avro::specific::testString));
ts->add(BOOST_TEST_CASE(avro::specific::testBytes));
ts->add(BOOST_TEST_CASE(avro::specific::testFixed));
Expand Down