[GitHub] [avro] wernerdaehn commented on pull request #973: AVRO-2952: AvroDatatype

[GitBox <gi...@apache.org>]

wernerdaehn commented on pull request #973:
URL: https://github.com/apache/avro/pull/973#issuecomment-744052343


   * **the methods public String `toString()` and `public String toString(StringBuilder, Value)` imply they do something similar, but actually don't:** Correct. But as both methods do not make sense by themselves, both help only in online debugging. I have no problem change the name, though, your choice.
   * **the methods `public int hashCode()` are not implemented consistently, and where they are, they all return 1 which leads to bad hashmapping in cases where that might ever be used**: There are two types of LogicalTypes: 1) Types like AvroInt, AvroLong etc that do not need any parameters. Hence they are Singletons. You cannot create a new one, you use the factory method to return the ONE. As there is just a single object, the hashCode() can return anything. Case 2) is where different instances are created because e.g. an AvroVarchar has a length. All these classes have a proper hashCode, e.g. LogicalTypeWithLength returns the hashCode of the length. So here I believe we are fine.
   * **`public boolean equals()` feels odd where it addresses Arrays and Maps**: I assume you meant Enum and Map. When should Enum.equals(Enum) return true? If both are Enums or if both are Enums and have the exact schema? Or if both are Enums and the schemas are forward compatible? I believe the first is the best option, as you will use these comparisons to convert values. And for values the possible values do not play a role. Hence I would suggest to stick with the current logic: all Enum datatypes are Enum datatypes.
   * **instances of override functions that only call their super version are redundant**: True. I thought it is a good idea for readability. To remind myself that there is nothing special. I have no problem changing that.
   * **it is unclear to me why some effectively static final fields are only set as static**: That I can explain, I am lacy on defining things as final. Agree, that is something I should change.
   * **`getRecommendedSchema` returns null in some cases (arrays, maps, records), but neither interface or javadoc specify that**: Deal, will update the Javadoc.
   * **getBackingType, getRecommendedSchema**: I took some inspiration from the current LogicalTypes, there the classes are rather self sufficient as well. And actually, it does make sense. When something does change and it should return a different object, a one-line change, you do not want to modify code at three different classes. Hence my feeling is it should rather stay as is. But maybe I misunderstood your point. Do you have an example?
   
   
   Before I make the changes, any counter arguments? Any requests from the Avro maintainers?
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org