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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
