You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "hwse (via GitHub)" <gi...@apache.org> on 2024/08/12 15:54:11 UTC
[PR] AVRO-4033: [C++] Filter out redundant union classes generated by avrogencpp [avro]
hwse opened a new pull request, #3088:
URL: https://github.com/apache/avro/pull/3088
[AVRO-4033](https://issues.apache.org/jira/browse/AVRO-4033)
## What is the purpose of the change
Currently avrogencpp generates a class for each union type that is encountered in the schema. Even if there is a class that represents the exact same union, a new type will be generated.
Example Schema:
```json
{
"type": "record",
"doc": "Top level Doc.",
"name": "RootRecord",
"fields": [
{
"name": "nullable_string_1",
"type": [
"null",
"string"
]
},
{
"name": "nullable_string_2",
"type": [
"null",
"string"
]
},
{
"name": "nullable_string_3",
"type": [
"null",
"string"
]
}
]
}
```
The generated RootRecord will look like this:
```c++
struct RootRecord {
typedef _union_test_json_Union__0__ nullable_string_1_t;
typedef _union_test_json_Union__1__ nullable_string_2_t;
typedef _union_test_json_Union__2__ nullable_string_3_t;
nullable_string_1_t nullable_string_1;
nullable_string_2_t nullable_string_2;
nullable_string_3_t nullable_string_3;
RootRecord() :
nullable_string_1(nullable_string_1_t()),
nullable_string_2(nullable_string_2_t()),
nullable_string_3(nullable_string_3_t())
{ }
}
```
Each field uses a unique union class, but their code is identical. With the suggested changes, only one union class will be generated.
This can help to reduce the header size for schemas that contain many unions.
## Verifying this change
This change added tests and can be verified as follows:
- added unit test to check that c++ types are the same for some union types
- added a unit test with encode/decode for the schema used in test above
## Documentation
- Does this pull request introduce a new feature? only a small improvement in the generated code
- If yes, how is the feature documented? added release note in JIRA ticket
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4033: [C++] Filter out redundant union classes generated by avrogencpp [avro]
Posted by "hwse (via GitHub)" <gi...@apache.org>.
hwse commented on code in PR #3088:
URL: https://github.com/apache/avro/pull/3088#discussion_r1724929886
##########
lang/c++/impl/avrogencpp.cc:
##########
@@ -915,3 +924,34 @@ int main(int argc, char **argv) {
return 1;
}
}
+
+UnionCodeTracker::UnionCodeTracker(const std::string &schemaFile) : schemaFile_(schemaFile) {
+}
+
+std::optional<std::string> UnionCodeTracker::getExistingUnionName(const std::vector<std::string> &unionBranches) const {
+ if (const auto it = unionBranchNameMapping_.find(unionBranches); it != unionBranchNameMapping_.end()) {
+ return it->second;
+ }
+ return std::nullopt;
+}
+
+std::string UnionCodeTracker::generateNewUnionName(const std::vector<std::string> &unionBranches) {
+ string s = schemaFile_;
+ string::size_type n = s.find_last_of("/\\");
+ if (n != string::npos) {
+ s = s.substr(n);
+ }
+ makeCanonical(s, false);
+
+ std::string result = s + "_Union__" + boost::lexical_cast<string>(unionNumber_++) + "__";
+ unionBranchNameMapping_.emplace(unionBranches, result);
+ return result;
+}
+
+bool UnionCodeTracker::unionTraitsAlreadyGenerated(const std::string &unionClassName) const {
+ return generatedUnionTraits_.find(unionClassName) != generatedUnionTraits_.end();
+}
+
+void UnionCodeTracker::setTraitsGenerated(const std::string &unionName) {
Review Comment:
Good Point, i renamed it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4033: [C++] Filter out redundant union classes generated by avrogencpp [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #3088:
URL: https://github.com/apache/avro/pull/3088#issuecomment-2304011287
Thank you, @hwse !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4033: [C++] Filter out redundant union classes generated by avrogencpp [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g merged PR #3088:
URL: https://github.com/apache/avro/pull/3088
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4033: [C++] Filter out redundant union classes generated by avrogencpp [avro]
Posted by "hwse (via GitHub)" <gi...@apache.org>.
hwse commented on PR #3088:
URL: https://github.com/apache/avro/pull/3088#issuecomment-2301571376
Merged main into this branch to fix conflicts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4033: [C++] Filter out redundant union classes generated by avrogencpp [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #3088:
URL: https://github.com/apache/avro/pull/3088#discussion_r1724874921
##########
lang/c++/impl/avrogencpp.cc:
##########
@@ -915,3 +924,34 @@ int main(int argc, char **argv) {
return 1;
}
}
+
+UnionCodeTracker::UnionCodeTracker(const std::string &schemaFile) : schemaFile_(schemaFile) {
+}
+
+std::optional<std::string> UnionCodeTracker::getExistingUnionName(const std::vector<std::string> &unionBranches) const {
+ if (const auto it = unionBranchNameMapping_.find(unionBranches); it != unionBranchNameMapping_.end()) {
+ return it->second;
+ }
+ return std::nullopt;
+}
+
+std::string UnionCodeTracker::generateNewUnionName(const std::vector<std::string> &unionBranches) {
+ string s = schemaFile_;
+ string::size_type n = s.find_last_of("/\\");
+ if (n != string::npos) {
+ s = s.substr(n);
+ }
+ makeCanonical(s, false);
+
+ std::string result = s + "_Union__" + boost::lexical_cast<string>(unionNumber_++) + "__";
+ unionBranchNameMapping_.emplace(unionBranches, result);
+ return result;
+}
+
+bool UnionCodeTracker::unionTraitsAlreadyGenerated(const std::string &unionClassName) const {
+ return generatedUnionTraits_.find(unionClassName) != generatedUnionTraits_.end();
+}
+
+void UnionCodeTracker::setTraitsGenerated(const std::string &unionName) {
Review Comment:
For consistency I'd suggest to use the same name for the parameter `unionName` -> `unionClassName`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org