You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@avro.apache.org by "Hagen Weiße (Jira)" <ji...@apache.org> on 2024/04/30 14:11:00 UTC
[jira] [Updated] (AVRO-3984) Improve generated code for unions (avrogencpp)
[ https://issues.apache.org/jira/browse/AVRO-3984?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Hagen Weiße updated AVRO-3984:
------------------------------
Description:
There are some issues in the code generated by avrogencpp for union types. The issues i would like to address in this ticket are listed below
I will try to provide a merge request in the near future.
h2. Performance
h3. Setter
The generated setter for a field takes a const reference and performs a copy for each set:
{code:java}
void set_BranchA(const BranchA& v); {code}
This might lead to redundant copies if BranchA contains a large string or vector.
The solution would be to provide an additional setter with move semantics:
{code:java}
void set_BranchA(BranchA&& v); {code}
h2. Getter
The generated setter for a field returns a value:
{code:java}
BranchA get_BranchA() const;
{code}
This leads to a copy of BranchA, which might be a issue if BranchA is large or contains a large string/vector.
It would propably be better to provide a reference to the stored value like this:
{code:java}
const BranchA& get_BranchA() const;
BranchA& get_BranchA();
{code}
h2. std::any
Generated unions store the value in a private std::any. This could probably be replaced with a std::variant.
h1. API issues
h2. accessing union branches
There are currently two ways to read a union branch:
First using a get_[BRANCH] method. This will either return the value from the branch or throw an exception. This version is okay, if you know that this branch is set.
Alternative is to use the idx() function. This will return a size_t with the index that is used.
!https://ticket.rsint.net/images/icons/emoticons/warning.png|width=16,height=16! The issue here is that you need to manually implement a mapping between branch index -> union branch.
A solution may look like this:
{code:java}
struct _Example_no_alias_avsc_Union__209__ {
private:
size_t idx_;
std::any value_;
public:
enum class Index: size_t {
BranchA = 0,
BranchB = 1,
BranchC = 2,
};
size_t idx() const { return idx_; }
Index idx_enum() const { return static_cast<Index>(idx_); }
bool is_BranchA() const;
const AttributeGroup& get_BranchA() const;
void set_BranchA(const AttributeGroup& v);
{code}
Added stuff:
* a enum with all union branches
* a additional idx_enum() function that returns this Index enum
* a is_[BRANCH] method, to check if a branch is set
was:
There are some issues in the code generated by avrogencpp for union types. The issues i would like to address in this ticket are listed below
h2. Performance
h3. Setter
The generated setter for a field takes a const reference and performs a copy for each set:
{code:java}
void set_BranchA(const BranchA& v); {code}
This might lead to redundant copies if BranchA contains a large string or vector.
The solution would be to provide an additional setter with move semantics:
{code:java}
void set_BranchA(BranchA&& v); {code}
h2. Getter
The generated setter for a field returns a value:
{code:java}
BranchA get_BranchA() const;
{code}
This leads to a copy of BranchA, which might be a issue if BranchA is large or contains a large string/vector.
It would propably be better to provide a reference to the stored value like this:
{code:java}
const BranchA& get_BranchA() const;
BranchA& get_BranchA();
{code}
h2. std::any
Generated unions store the value in a private std::any. This could propably be replaced with a std::variant.
h1. API issues
h2. accessing union branches
There are currently two ways to read a union branch:
First using a get_[BRANCH] method. This will either return the value from the branch or throw an exception. This version is okay, if you know that this branch is set.
Alternative is to use the idx() function. This will return a size_t with the index that is used. !https://ticket.rsint.net/images/icons/emoticons/warning.png|width=16,height=16! The issue here is that you need to manually implement a mapping between branch index -> union branch. We already do this, since avro::GenericDatum has the same issue. But it would be nice to get away from this "manual labor".
A solution may look like this:
{code:java}
struct _Example_no_alias_avsc_Union__209__ {
private:
size_t idx_;
std::any value_;
public:
enum class Index: size_t {
BranchA = 0,
BranchB = 1,
BranchC = 2,
};
size_t idx() const { return idx_; }
Index idx_enum() const { return static_cast<Index>(idx_); }
bool is_BranchA() const;
const AttributeGroup& get_BranchA() const;
void set_BranchA(const AttributeGroup& v);
{code}
Added stuff:
* a enum with all union branches
* a additional idx_enum() function that returns this Index enum
* a is_[BRANCH] method, to check if a branch is set
> Improve generated code for unions (avrogencpp)
> ----------------------------------------------
>
> Key: AVRO-3984
> URL: https://issues.apache.org/jira/browse/AVRO-3984
> Project: Apache Avro
> Issue Type: Improvement
> Components: c++
> Reporter: Hagen Weiße
> Priority: Major
>
> There are some issues in the code generated by avrogencpp for union types. The issues i would like to address in this ticket are listed below
> I will try to provide a merge request in the near future.
> h2. Performance
> h3. Setter
> The generated setter for a field takes a const reference and performs a copy for each set:
> {code:java}
> void set_BranchA(const BranchA& v); {code}
> This might lead to redundant copies if BranchA contains a large string or vector.
> The solution would be to provide an additional setter with move semantics:
> {code:java}
> void set_BranchA(BranchA&& v); {code}
> h2. Getter
> The generated setter for a field returns a value:
> {code:java}
> BranchA get_BranchA() const;
> {code}
> This leads to a copy of BranchA, which might be a issue if BranchA is large or contains a large string/vector.
> It would propably be better to provide a reference to the stored value like this:
> {code:java}
> const BranchA& get_BranchA() const;
> BranchA& get_BranchA();
> {code}
> h2. std::any
> Generated unions store the value in a private std::any. This could probably be replaced with a std::variant.
> h1. API issues
> h2. accessing union branches
> There are currently two ways to read a union branch:
> First using a get_[BRANCH] method. This will either return the value from the branch or throw an exception. This version is okay, if you know that this branch is set.
> Alternative is to use the idx() function. This will return a size_t with the index that is used.
> !https://ticket.rsint.net/images/icons/emoticons/warning.png|width=16,height=16! The issue here is that you need to manually implement a mapping between branch index -> union branch.
> A solution may look like this:
> {code:java}
> struct _Example_no_alias_avsc_Union__209__ {
> private:
> size_t idx_;
> std::any value_;
> public:
> enum class Index: size_t {
> BranchA = 0,
> BranchB = 1,
> BranchC = 2,
> };
> size_t idx() const { return idx_; }
> Index idx_enum() const { return static_cast<Index>(idx_); }
> bool is_BranchA() const;
> const AttributeGroup& get_BranchA() const;
> void set_BranchA(const AttributeGroup& v);
> {code}
> Added stuff:
> * a enum with all union branches
> * a additional idx_enum() function that returns this Index enum
> * a is_[BRANCH] method, to check if a branch is set
--
This message was sent by Atlassian Jira
(v8.20.10#820010)