martin-g commented on PR #356: URL: https://github.com/apache/avro/pull/356#issuecomment-2069131946
Here is my personal opinion as a non-Java user: The PR introduces a new schema type (Param) only in the Java SDK without trying to explain this new type in the Avro specification in a language agnostic way. Some Avro SDKs/languages do not support generics at all, e.g. C, Perl, JavaScript, ... and this will make this new type an exception! But let's ignore that and continue with the review: - The PR does not even give an example of the new schema! The only snippet I see is at https://github.com/apache/avro/pull/356#issuecomment-433292283 but IMO it is not very clear. IMO new tests should be added that cover parsing of this new schema type, encoding (binary and JSON) - Any attempt to implement the same in a second SDK just to prove that the implementation is good and interop-able will make it easier to approve such new addition - Looking at `{"type":"param","values":{"type":"record","name":"PairIntegerString6f72de2f3285b50e","namespace":"org.apache.avro","fields":[{"name":"key","type":"int"},{"name":"value","type":"string"}],"java-class":"org.apache.avro.TestParamTypes$Pair"}}` I have the feeling that there is no need of a new Schema type at all. It seems like RecordSchema is almost capable to cover the need. My 2c! -- 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: issues-unsubscr...@avro.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
