Re: [PR] AVRO-3520: [java] expose read schema in custom encoding [avro]

["martin-g (via GitHub)" <gi...@apache.org>]

martin-g commented on code in PR #3445:
URL: https://github.com/apache/avro/pull/3445#discussion_r2503487027


##########
lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectData.java:
##########
@@ -1055,7 +1068,8 @@ private CustomEncodingWrapper populateEncoderCache(Schema schema) {
     var enc = ReflectionUtil.getAvroEncode(getClass(schema));
     if (enc != null) {
       try {
-        return new CustomEncodingWrapper(enc.using().getDeclaredConstructor().newInstance());
+        var customEncodingClass = enc.using().getDeclaredConstructor().newInstance();

Review Comment:
   `...Class` suggests that this is a java.lang.Class, but actually it is an instance of CustomEncoding.class.



##########
lang/java/avro/src/main/java/org/apache/avro/reflect/CustomEncoding.java:
##########
@@ -48,4 +48,16 @@ protected Schema getSchema() {
     return schema;
   }
 
+  /**
+   * Receives the schema, giving the concrete encoder implementation an
+   * opportunity to detect schema changes and behave accordingly. Useful for
+   * maintaining backwards compatibility.
+   *
+   * @param schema the schema detected during read.

Review Comment:
   `#withSchema()` is used also at https://github.com/apache/avro/pull/3445/files#diff-87216da701d0b2e3135497e3a9ffc4065a56d31142729d9867e0ca2e7dd2b2d1R1072 where the schema is `write`. The javadoc seems not correct by saying `read`



##########
lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectData.java:
##########
@@ -397,10 +389,12 @@ private FieldAccessor[] getAccessorsFor(Schema schema) {
     }
 
     private FieldAccessor[] createAccessorsFor(Schema schema) {
+
+      var byNameSchema = buildByName(clazz, schema);

Review Comment:
   I am not sure how often `#createAccessorsFor(Schema)` is called but `buildByName(clazz, schema)` seems to be wasteful. It uses reflection to extract the field names -> accessor map and then drops it. Next time this method is used again it does it again.



##########
lang/java/avro/src/main/java/org/apache/avro/reflect/CustomEncoding.java:
##########
@@ -48,4 +48,16 @@ protected Schema getSchema() {
     return schema;
   }
 
+  /**
+   * Receives the schema, giving the concrete encoder implementation an
+   * opportunity to detect schema changes and behave accordingly. Useful for
+   * maintaining backwards compatibility.
+   *
+   * @param schema the schema detected during read.
+   * @return custom encoding to be used.
+   */
+  public CustomEncoding<T> withSchema(Schema schema) {

Review Comment:
   This API bothers me a bit.
   Does it need to be thread-safe ? Because the overrides of this method could do something like `this.schema = schema`.
   IMO this method should make sure somehow that it always returns a new instance of CustomEncoding that uses the provided schema.



-- 
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-unsubscribe@avro.apache.org

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