Skip to content
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

Jump to bottom

AVRO-3601: C++ API header contains breaking include #1821

Merged
merged 2 commits into from
Aug 11, 2022
Merged

AVRO-3601: C++ API header contains breaking include #1821

merged 2 commits into from
Aug 11, 2022

Conversation

martin-g
Copy link
Member

Rename CustomFields to CustomAttributes.
Rework CustomAttributes to keep a map of string name with a string
value. The user application may parse the string to JSON if needed.
Add a new step to the CI workflow to prevent problems like this in the
future

Jira

Tests

  • My PR updates the relevant unit tests

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

@martin-g martin-g requested a review from thiru-mg August 11, 2022 12:07
@github-actions github-actions bot added build C++ Pull Requests for C++ binding labels Aug 11, 2022
@martin-g
Copy link
Member Author

CC @nileyadav @mruffing @chenrui333

@martin-g
AVRO-3601: C++ API header contains breaking include
ee2986e 
Rename CustomFields to CustomAttributes.
Rework CustomAttributes to keep a map of string name with a string
value. The user application may parse the string to JSON if needed.
Add a new step to the CI workflow to prevent problems like this in the
future

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
@martin-g martin-g force-pushed the avro-3601-c++-simplify-custom-attributes branch from a4821ef to ee2986e Compare August 11, 2022 12:24
@martin-g
AVRO-3601: Do not try to install
c9d0f6d 
Just build in Release mode

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
thiru-mg
thiru-mg approved these changes Aug 11, 2022
View reviewed changes
Copy link
Contributor

@thiru-mg thiru-mg left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for the prompt action.

@thiru-mg thiru-mg merged commit 864774b into master Aug 11, 2022
@thiru-mg thiru-mg deleted the avro-3601-c++-simplify-custom-attributes branch August 11, 2022 14:00
KalleOlaviNiemitalo
KalleOlaviNiemitalo reviewed Aug 11, 2022
View reviewed changes
lang/c++/impl/CustomAttributes.cc
std::map<std::string, std::string>::const_iterator iter =
attributes_.find(name);
if (iter == attributes_.end()) {
return NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Conversion from nullptr to std::string has undefined behavior. Is there a test case that covers this?

https://github.com/gcc-mirror/gcc/blob/04ce2400b35225302e0d6883bb0817378180f5d7/libstdc%2B%2B-v3/include/bits/basic_string.h#L634-L640

Copy link
Member Author

Choose a reason for hiding this comment

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

No, there is no such test.
Suggestions how to rework this ? Maybe return Option(al) ?! I have to see which version of C++ supports this.

Copy link
Member Author

Choose a reason for hiding this comment

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

std::optional is available since C++ 17. Avro uses 11

Copy link
Member Author

Choose a reason for hiding this comment

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

boost::optional is available!

Copy link
Member Author

Choose a reason for hiding this comment

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

#1826

KalleOlaviNiemitalo
KalleOlaviNiemitalo reviewed Aug 11, 2022
View reviewed changes
lang/c++/impl/Compiler.cc
{
// Don't add known fields on primitive type and fixed type into custom
// fields.
const std::unordered_set<std::string>& kKnownFields = getKnownFields();
for (const auto &entry : m) {
if (kKnownFields.find(entry.first) == kKnownFields.end()) {
customAttributes.addField(entry.first, entry.second);
customAttributes.addAttribute(entry.first, entry.second.stringValue());
Copy link
Contributor

Choose a reason for hiding this comment

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

Are custom attributes required to have string values? I thought the plan was to store a string in JSON syntax, i.e. including quotation marks if the value is a JSON string.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the C++ developers here have to step up! :-)
Thanks for the review, @KalleOlaviNiemitalo !

Copy link
Member Author

Choose a reason for hiding this comment

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

074eef4
Looks good ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I commented further in #1826 (comment).

@KalleOlaviNiemitalo KalleOlaviNiemitalo mentioned this pull request Aug 12, 2022
Merged
4 tasks
martin-g added a commit that referenced this pull request Aug 15, 2022
@martin-g
AVRO-3601: C++ API header contains breaking include (#1821)
32aa94d 
* AVRO-3601: C++ API header contains breaking include

Rename CustomFields to CustomAttributes.
Rework CustomAttributes to keep a map of string name with a string
value. The user application may parse the string to JSON if needed.
Add a new step to the CI workflow to prevent problems like this in the
future

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

* AVRO-3601: Do not try to install

Just build in Release mode

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
(cherry picked from commit 864774b)
@nileyadav
Copy link
Contributor

nileyadav commented Oct 11, 2022 via email

@martin-g
Copy link
Member Author

It will be part of 1.11.2 and 1.12.0 when they are released.
There are no discussions about specific dates for now.

@jhump jhump mentioned this pull request Dec 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KalleOlaviNiemitalo KalleOlaviNiemitalo KalleOlaviNiemitalo left review comments

@thiru-mg thiru-mg thiru-mg approved these changes

Assignees
No one assigned
Labels
build C++ Pull Requests for C++ binding
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@martin-g @nileyadav @thiru-mg @KalleOlaviNiemitalo