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 in schemas. #3064

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jmd-z
Copy link

@jmd-z jmd-z commented Aug 5, 2024

What is the purpose of the change

Fix schema compilation errors when non-string custom attributes are present.

Verifying this change

This change added tests and can be verified as follows:

  • Added test case that contains non-string custom attributes

Documentation

  • Does this pull request introduce a new feature? no

@github-actions github-actions bot added the C++ Pull Requests for C++ binding label Aug 5, 2024
@thiru-mg
Copy link
Contributor

thiru-mg commented Aug 5, 2024

Looks good to me. There is a minor fidelity issue though: the JSON regenerated from the compiled schema loses the non-string type. That is, in the process of JSON -> Schema -> JSON, the input and output JSON may not be equivalent. However, Schema -> JSON -> Schema maintains fidelity.
We can fix this by changing the type of customAttributes, but that will not be strictly compatible for the existing users. To address this, we can have a more general customAttributes inside the Schema object and expose it as string->string map for compatibility (as well as the more general map for generality).
I'm fine with this fidelity issue for now.

@jmd-z
Copy link
Author

jmd-z commented Aug 5, 2024

Yes, it loses the distinction between a string value and a JSON fragment that would be up to the user to parse. I don't think a map of string->string would suffice as the fragment could be arbitrarily deep.
The other approach, which appears to have been the intent of AVRO-3601, would be to make the string returned by customAttributes always a JSON fragment. This would change the current behavior as string values would have to contain quotes that aren't there today.
Some of the existing tests are written this way, so instead of changing the current behavior I started with this approach to get some feedback.

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.

2 participants