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.

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

AVRO-4026: [C++] Allow non-string custom attributes #3344

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wgtmac
Copy link
Member

@wgtmac wgtmac commented Mar 18, 2025

What is the purpose of the change

This PR takes a different approach than #3308, #3069, #3064 and #3266 to fix the non-string custom attributes. It extends the CustomAttributes::addAttribute(const std::string &name, const std::string &value) with a third parameter bool addQuotes with default value set to true to support adding non-string values explicitly and keep backward compatibility.

Verifying this change

  • Make sure that all existing test cases are passed so backward compatibility is preserved.
  • Added new test case to verify that both json->schema->json and schema->json->schema round trips have preserved non-string attributes correctly.

Documentation

  • Does this pull request introduce a new feature? yes
  • If yes, how is the feature documented? added comment to explain the behavior

@github-actions github-actions bot added the C++ Pull Requests for C++ binding label Mar 18, 2025
@wgtmac
Copy link
Member Author

wgtmac commented Mar 18, 2025

This is my attempt to fix this issue after getting more familiar with this codebase. Please take a look, thanks! @thiru-mg @martin-g @Fokko @jhump

(This is the last blocker for me to integrate avro-cpp with iceberg-cpp)

//
// If `addQuotes` is true, the `value` will be wrapped in double quotes in the
// json serialization; otherwise, the `value` will be serialized as is.
void addAttribute(const std::string &name, const std::string &value, bool addQuotes = true);
Copy link
Member Author

@wgtmac wgtmac Mar 18, 2025

Choose a reason for hiding this comment

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

I can also mark it as deprecated and add two clearer functions like:

[[deprecated("Deprecated in 1.13.0 and will be removed in 1.14.0. Use addStringAttribute instead.")]]
void addAttribute(const std::string &name, const std::string &value, bool addQuotes = true);

void addStringAttribute(const std::string &name, const std::string &value) {
    addAttribute(name, value, /*addQuotes=*/true);
}

void addNonStringAttribute(const std::string &name, const std::string &value) {
    addAttribute(name, value, /*addQuotes=*/false);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ Pull Requests for C++ binding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant