-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[C++] AVRO-4058: Allow custom attributes in arrays #3168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -355,7 +355,8 @@ const char *roundTripSchemas[] = { | |||
{"name":"f1","type":"long","extra_field":"1"}, | |||
{"name":"f2","type":"int","extra_field1":"21","extra_field2":"22"} | |||
] | |||
})" | |||
})", | |||
R"({"type":"array","items":"long","extra":"1"})" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an error:
unknown location(0): fatal error: in "Avro C__ unit tests for schemas/avro__schema__testBasic_35": avro::Exception: Invalid type. Expected "string" actual long
/home/runner/work/avro/avro/lang/c++/test/SchemaTests.cc(428): last checkpoint: {"type": "array", "items": "long", "extra attribute": 1}
Where does attribute
come from in "extra attribute"
?!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is not in the added test case but in an existing test case (line 146) in testBasic checking that custom attributes do not impact the ability to compile schemas.
The specified attribute in the test was given as a integer, which so far was no issue as it was ignored but now it is attempted to be parsed. This attempt failed as only strings are supported in custom attributes. For now I changed the attribute to a string, the original test case can be restored once non-string attributes are supported.
(This change of 1 to "1" was present in the original PR, I accidentally undid it when merging the latest main)
Thank you, @pascalginter ! |
AVRO-4058
What is the purpose of the change
Other languages (i.e. Java) allow adding custom attributes in the schema of an array while the C++ implementation does not. The sparse spec states to allow all custom attributes ("Attributes not defined in this document are permitted as metadata, but must not affect the format of serialized data") and other projects using avro (i.e. Iceberg) depend on custom attributes in arrays.
This PR allows adding custom attributes to arrays.
Verifying this change
This change added tests and can be verified as follows:
Run SchemaTests, I added a single additional test case in testRoundTrip
Documentation