jhump opened a new pull request, #3266: URL: https://github.com/apache/avro/pull/3266
This adds new methods to `CustomAttributes` to allow setting non-string values. These other methods work with JSON-encoded strings. Unlike #3064 and #3069, this change attempts to be backwards compatible. However, from reading more comments in pull requests, it looks like the "fix" I added (to escape the keys and values in custom attributes when printing to JSON) may actually be a compatibility issue since it seems that users were _expected_ to have to escape string values if they contained any characters that would be escaped in JSON (including quotes). That seems like a really terrible API, and it also meant that the values would not round-trip correctly: reading a data file would not create custom attributes with these strings properly escaped, so later writing out data with the same schema would generate an invalid schema JSON document. In any event, this uses strings as the values even though it would be ideal if we could pass some sort of structured data as the value type. The ideal types (`json::Entity` and its accompanying `json::Object` and `json::Array` types) are defined in `json/JsonDom.hh`. But that header files is not part of the Avro include files distribution, which means we cannot `#include` it from `CustomAttributes.hh`, so it's a no-go. From a little history spelunking, I see that they indeed used to use a structured form which was simplified to strings in #1821, purely because these JSON header files aren't available to users in the Avro distribution. Alternatives that I considered for using JSON-encoded strings: * Include the JSON types in the Avro distribution. I think this would be as simple as moving the header files out of `lang/c++/impl` and into `lang/c++/include/avro`. But then we expand the public API of too much. This approach was already tried and rejected in #1820. * Use a wrapper like `std::any` as the value type. This _can_ be `#include`d in `CustomAttributes.hh` but eliminates type safety in the signature. The only concrete accepted value would likely be `json::Entity` -- though we could make it more sophisticated and also allow the various value types sans wrapper: `std::string`, `bool`, `int64_t`, `double`, `json::Array` (aka `std::vector<json::Entity>`), and `json::Object` (aka `std::map<std::string, json::Entity>`). But this isn't really usable by external/user code, at least not for any composite values, since they aren't able to include the JSON headers and then produce valid values of `json::Entity`. * Convert from `json::Entity` to some other structured type that is defined in `CustomAttributes.hh`. This could possibly be a concrete `std::variant` that allows the various options (and could use `std::monostate` to represent JSON null values). This introduces non-trivial conversion code. From a performance perspective, it could likely be better than converting to/from strings, but it's a non-trivial amount of new-new code to maintain, which didn't feel right. ## 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: * Updates existing unit test cases to use the new method and use non-string custom attributes. ## Documentation * Does this pull request introduce a new feature? yes * If yes, how is the feature documented? not documented -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@avro.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
