AVRO-4026: [c++] Allow non-string custom attributes in schemas

[jmd-z]

Custom attributes are encoded as JSON to avoid ambiguity with simple string values.

What is the purpose of the change

Fixes exception from C++ library when compiling schemas with non-string custom attributes.

Verifying this change

This change added tests and can be verified as follows:

• Added test that compiles a schema with a non-string custom attribute

Documentation

• Does this pull request introduce a new feature? yes
• If yes, how is the feature documented? not documented

[pascalginter]

LGTM
I would be happy to see this PR revived and eventually merged

[pascalginter]

Nit: format the json over more lines for better readability

[martin-g]

@Gerrit0 @mkmkme Do you want to review too ?

[mkmkme]

LGTM!

[martin-g]

Looking at these changes I think this change may break someone's application.
Do we want to support a backward compatible encoding via some property/setting/... ?

[jmd-z]

This particular case isn't much of a problem. The current behavior of the C++ library is to throw an exception if you attempt to read a schema with non-string custom attributes, so this is unlikely in the wild. This test creates the schema programmatically bypassing the reading issue. The compatibility issue with this PR is existing code that interrogates the value of a custom attribute that is a string type. With this PR, the value is JSON encoded text, so the returned value will contain a quoted string instead of the raw string in that case. There is another PR 3604 that avoids this compatibility issue, but at the cost of creating ambiguity. The API was recently changed, remaining CustomFields to CustomAttributes, signaling a breaking change. Perhaps renaming the function CustomAttributes::getAttributes to CustomAttributes::getAttributesJSON would do the same here.

…

On Thu, Sep 26, 2024, 7:13 AM Martin Grigorov ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lang/c++/test/unittest.cc <#3069 (comment)>: > @@ -457,11 +457,11 @@ struct TestSchema { std::string expectedJsonWithCustomAttribute = "{\"type\": \"record\", \"name\": \"Test\",\"fields\": " "[{\"name\": \"f1\", \"type\": \"long\", " - "\"arrayField\": \"[1]\", " - "\"booleanField\": \"true\", " - "\"mapField\": \"{\\\"key1\\\":\\\"value1\\\", \\\"key2\\\":\\\"value2\\\"}\", " - "\"nullField\": \"null\", " - "\"numberField\": \"1.23\", " + "\"arrayField\": [1], " Looking at these changes I think this change may break someone's application. Do we want to support a backward compatible encoding via some property/setting/... ? — Reply to this email directly, view it on GitHub <#3069 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BKKFEFOTN2DUMQW6AUKEGT3ZYPT7DAVCNFSM6AAAAABMDDFJFGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMZQHA4DSNRZGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[martin-g]

Perhaps renaming the function CustomAttributes::getAttributes to CustomAttributes::getAttributesJSON would do the same here.

Perhaps this would be the best!
In my experience (Java and Rust) the best is to keep the old behavior as deprecated and introduce a new one as the recommended. In a later version, e.g. 1.13.0, the deprecated method could be removed.

[pascalginter]

It seems that everyone involved in this discussion agrees with @jmd-z's proposal, and the PR could be merged once the change is implemented. @jmd--z, do you have time to make the small adjustment?