You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "ashley-taylor (via GitHub)" <gi...@apache.org> on 2025/07/30 10:53:01 UTC
[PR] AVRO-3520: [java] expose read schema in custom encoding [avro]
ashley-taylor opened a new pull request, #3445:
URL: https://github.com/apache/avro/pull/3445
## What is the purpose of the change
Allows custom encodings to support file changes.
Used https://github.com/apache/avro/pull/1692 as inspiration
## Verifying this change
*(Please pick one of the following options)*
This change added tests and can be verified as follows:
Test custom encodings with several different versions, confirming that the encoder is linked to the schema
## Documentation
- Does this pull request introduce a new feature? (yes)
- If yes, how is the feature documented? JavaDocs
--
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-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3520: [java] expose read schema in custom encoding [avro]
Posted by "ashley-taylor (via GitHub)" <gi...@apache.org>.
ashley-taylor commented on code in PR #3445:
URL: https://github.com/apache/avro/pull/3445#discussion_r2508476733
##########
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:
Is only called by `#getAccessorsFor(Schema schema` which caches this
--
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
Re: [PR] AVRO-3520: [java] expose read schema in custom encoding [avro]
Posted by "ashley-taylor (via GitHub)" <gi...@apache.org>.
ashley-taylor commented on code in PR #3445:
URL: https://github.com/apache/avro/pull/3445#discussion_r2508477546
##########
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:
updated comment
--
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
Re: [PR] AVRO-3520: [java] expose read schema in custom encoding [avro]
Posted by "ashley-taylor (via GitHub)" <gi...@apache.org>.
ashley-taylor commented on PR #3445:
URL: https://github.com/apache/avro/pull/3445#issuecomment-3185700244
@opwvhk @martin-g anything I can do to help this one get reviewed? Thanks
--
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
Re: [PR] AVRO-3520: [java] expose read schema in custom encoding [avro]
Posted by "itstheceo (via GitHub)" <gi...@apache.org>.
itstheceo commented on PR #3445:
URL: https://github.com/apache/avro/pull/3445#issuecomment-3226336817
@KalleOlaviNiemitalo @opwvhk could you please have a look? Much appreciated
--
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
Re: [PR] AVRO-3520: [java] expose read schema in custom encoding [avro]
Posted by "ashley-taylor (via GitHub)" <gi...@apache.org>.
ashley-taylor commented on PR #3445:
URL: https://github.com/apache/avro/pull/3445#issuecomment-3144330462
@opwvhk fixed prettier thanks
--
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
Re: [PR] AVRO-3520: [java] expose read schema in custom encoding [avro]
Posted by "ashley-taylor (via GitHub)" <gi...@apache.org>.
ashley-taylor commented on code in PR #3445:
URL: https://github.com/apache/avro/pull/3445#discussion_r2508477264
##########
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:
Currently, it is only called right after creation, so the libraries use is thread safe.
We could make it so that instead of a zero-argument constructor, it looks for a single-argument constructor and passes the schema in that way. It might be a little less discoverable as a feature, but safer.
They would need two constructors, as the zero-argument constructor will be needed to identify the schema
Alternativly perform an identity comparison after calling this method. Would mean default method would perform a reflection creation which I don't paticually like.
--
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
Re: [PR] AVRO-3520: [java] expose read schema in custom encoding [avro]
Posted by "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
Re: [PR] AVRO-3520: [java] expose read schema in custom encoding [avro]
Posted by "ashley-taylor (via GitHub)" <gi...@apache.org>.
ashley-taylor commented on PR #3445:
URL: https://github.com/apache/avro/pull/3445#issuecomment-3135881062
@opwvhk If you could review would be awesome thanks
--
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
Re: [PR] AVRO-3520: [java] expose read schema in custom encoding [avro]
Posted by "ashley-taylor (via GitHub)" <gi...@apache.org>.
ashley-taylor commented on PR #3445:
URL: https://github.com/apache/avro/pull/3445#issuecomment-3509111031
@martin-g thank you for the review. Have addressed or replied to the feedback. If you would like to see the multiple constructor approach happy to make that change.
--
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
Re: [PR] AVRO-3520: [java] expose read schema in custom encoding [avro]
Posted by "ashley-taylor (via GitHub)" <gi...@apache.org>.
ashley-taylor commented on PR #3445:
URL: https://github.com/apache/avro/pull/3445#issuecomment-3316655546
@opwvhk, following up to see if you have any time to review this PR. Thanks
--
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