[GitHub] [avro] martin-g commented on a change in pull request #1320: Protobuf data fixes

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

martin-g commented on a change in pull request #1320:
URL: https://github.com/apache/avro/pull/1320#discussion_r699059403



##########
File path: lang/java/protobuf/src/main/java/org/apache/avro/protobuf/ProtobufData.java
##########
@@ -113,17 +114,17 @@ protected Object getField(Object record, String name, int pos, Object state) {
     }
   }
 
-  private final Map<Descriptor, FieldDescriptor[]> fieldCache = new ConcurrentHashMap<>();
+  private final Map<DescriptorProto, FieldDescriptor[]> fieldCache = new ConcurrentHashMap<>();
 
   @Override
   protected Object getRecordState(Object r, Schema s) {
     Descriptor d = ((MessageOrBuilder) r).getDescriptorForType();
-    FieldDescriptor[] fields = fieldCache.get(d);
+    FieldDescriptor[] fields = fieldCache.get(d.toProto());
     if (fields == null) { // cache miss
       fields = new FieldDescriptor[s.getFields().size()];
       for (Field f : s.getFields())
         fields[f.pos()] = d.findFieldByName(f.name());
-      fieldCache.put(d, fields); // update cache
+      fieldCache.put(d.toProto(), fields); // update cache

Review comment:
       Does it make sense to cache the result of `d.toProto()` and re-use it instead of calling it twice in the same method ?

##########
File path: lang/java/protobuf/src/main/java/org/apache/avro/protobuf/ProtobufData.java
##########
@@ -191,26 +192,27 @@ public Schema getSchema(Class c) {
     return schema;
   }
 
-  private static final ThreadLocal<Map<Descriptor, Schema>> SEEN = ThreadLocal.withInitial(IdentityHashMap::new);
+  private static final ThreadLocal<Map<DescriptorProto, Schema>> SEEN = ThreadLocal.withInitial(IdentityHashMap::new);
 
   public Schema getSchema(Descriptor descriptor) {
-    Map<Descriptor, Schema> seen = SEEN.get();
-    if (seen.containsKey(descriptor)) // stop recursion
-      return seen.get(descriptor);
+    Map<DescriptorProto, Schema> seen = SEEN.get();
+    DescriptorProto dp = descriptor.toProto();
+    if (seen.containsKey(dp)) // stop recursion
+      return seen.get(dp);

Review comment:
       Would it be better to do a `.get()` + check for `null` instead of two lookups (`containsKey()` + `get()`) ?

##########
File path: lang/java/protobuf/src/main/java/org/apache/avro/protobuf/ProtobufData.java
##########
@@ -161,11 +162,11 @@ protected boolean isBytes(Object datum) {
   @Override
   protected Schema getRecordSchema(Object record) {
     Descriptor descriptor = ((Message) record).getDescriptorForType();
-    Schema schema = schemaCache.get(descriptor);
+    Schema schema = schemaCache.get(descriptor.toProto());
 
     if (schema == null) {
       schema = getSchema(descriptor);
-      schemaCache.put(descriptor, schema);
+      schemaCache.put(descriptor.toProto(), schema);

Review comment:
       Same as above - cache the result of `descriptor.toProto()`




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