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

[C++] AVRO-4058: Allow custom attributes in arrays #3168

Merged
merged 3 commits into from
Sep 24, 2024
Merged

[C++] AVRO-4058: Allow custom attributes in arrays #3168

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
858253b
Allow custom attributes on array values
Sep 20, 2024
ce5c47a
Merge branch 'main' into allow-custom-attributes
Sep 24, 2024
235dde9
Fix merge error of undoing conversion of custom attribute to string
Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lang/c++/impl/Compiler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ static const std::unordered_set<std::string> &getKnownFields() {
// return known fields
static const std::unordered_set<std::string> kKnownFields =
{"name", "type", "aliases", "default", "doc", "size", "logicalType",
"values", "precision", "scale", "namespace"};
"values", "precision", "scale", "namespace", "items"};
return kKnownFields;
}

Expand Down Expand Up @@ -424,6 +424,9 @@ static NodePtr makeArrayNode(const Entity &e, const Object &m,
if (containsField(m, "doc")) {
node->setDoc(getDocField(e, m));
}
CustomAttributes customAttributes;
getCustomAttributes(m, customAttributes);
node->addCustomAttributesForField(customAttributes);
return node;
}

Expand Down
3 changes: 3 additions & 0 deletions lang/c++/impl/NodeImpl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,9 @@ void NodeArray::printJson(std::ostream &os, size_t depth) const {
os << indent(depth + 1) << "\"items\": ";
leafAttributes_.get()->printJson(os, depth + 1);
os << '\n';
for (size_t i = 0; i != customAttributes_.size(); i++){
printCustomAttributes(customAttributes_.get(i), depth + 1, os);
}
os << indent(depth) << '}';
}

Expand Down
4 changes: 2 additions & 2 deletions lang/c++/include/avro/NodeImpl.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ using NodeImplSymbolic = NodeImpl<HasName, NoLeaves, NoLeafNames, NoAttributes,

using NodeImplRecord = NodeImpl<HasName, MultiLeaves, LeafNames, MultiAttributes, NoSize>;
using NodeImplEnum = NodeImpl<HasName, NoLeaves, LeafNames, NoAttributes, NoSize>;
using NodeImplArray = NodeImpl<NoName, SingleLeaf, NoLeafNames, NoAttributes, NoSize>;
using NodeImplArray = NodeImpl<NoName, SingleLeaf, NoLeafNames, MultiAttributes, NoSize>;
using NodeImplMap = NodeImpl<NoName, MultiLeaves, NoLeafNames, NoAttributes, NoSize>;
using NodeImplUnion = NodeImpl<NoName, MultiLeaves, NoLeafNames, NoAttributes, NoSize>;
using NodeImplFixed = NodeImpl<HasName, NoLeaves, NoLeafNames, NoAttributes, HasSize>;
Expand Down Expand Up @@ -363,7 +363,7 @@ class AVRO_DECL NodeArray : public NodeImplArray {
public:
NodeArray() : NodeImplArray(AVRO_ARRAY) {}

explicit NodeArray(const SingleLeaf &items) : NodeImplArray(AVRO_ARRAY, NoName(), items, NoLeafNames(), NoAttributes(), NoSize()) {}
explicit NodeArray(const SingleLeaf &items) : NodeImplArray(AVRO_ARRAY, NoName(), items, NoLeafNames(), {}, NoSize()) {}

SchemaResolution resolve(const Node &reader) const override;

Expand Down
7 changes: 4 additions & 3 deletions lang/c++/test/SchemaTests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"})"
Copy link
Member

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" ?!?

Copy link
Contributor Author

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)

};

const char *malformedLogicalTypes[] = {
Expand Down Expand Up @@ -448,11 +449,11 @@ static void testRoundTrip(const char *schema) {
compiledSchema.toJson(os);
std::string result = removeWhitespaceFromSchema(os.str());
std::string trimmedSchema = removeWhitespaceFromSchema(schema);
BOOST_CHECK(result == trimmedSchema);
BOOST_CHECK_EQUAL(result, trimmedSchema);
// Verify that the compact schema from toJson has the same content as the
// schema.
std::string result2 = compiledSchema.toJson(false);
BOOST_CHECK(result2 == trimmedSchema);
BOOST_CHECK_EQUAL(result2, trimmedSchema);
}

static void testCompactSchemas() {
Expand Down
Loading