[GitHub] [avro] cutting commented on a change in pull request #842: AVRO-2723: Refactor ReflectData to allow derived classes to customize default values for fields

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

cutting commented on a change in pull request #842:
URL: https://github.com/apache/avro/pull/842#discussion_r413383635



##########
File path: lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectData.java
##########
@@ -500,6 +582,37 @@ static boolean isNonStringMapSchema(Schema s) {
     return false;
   }
 
+  /**
+   * Get default value for a schema field. Derived classes can override this
+   * method to provide values based on object instantiation
+   *
+   * @param type        Type
+   * @param field       Field
+   * @param fieldSchema Schema of the field
+   * @return The default value
+   */
+  protected Object createSchemaDefaultValue(Type type, Field field, Schema fieldSchema) {
+    Object defaultValue;
+    if (defaultGenerated) {
+      defaultValue = getOrCreateDefaultValue(type, field);
+      if (defaultValue != null) {
+        return defaultValue;

Review comment:
       Should we instead return deepCopy(fieldSchema, defaultValue)?

##########
File path: lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectData.java
##########
@@ -110,14 +112,96 @@ public static ReflectData get() {
   }
 
   /**
-   * Cause a class to be treated as though it had an {@link Stringable}
-   ** annotation.
+   * Cause a class to be treated as though it had an {@link Stringable} *
+   * annotation.
    */
   public ReflectData addStringable(Class c) {
     stringableClasses.add(c);
     return this;
   }
 
+  /**
+   * If this flag is set to true, default values for fields will be assigned
+   * dynamically using Java reflections. Let's call this feature `default
+   * reflection`. Initially this feature is disabled
+   */
+  private boolean defaultGenerated = false;
+
+  /**
+   * Enable or disable `default reflection`
+   *
+   * @param enabled set to `true` to enable the feature. This feature is disabled
+   *                by default
+   * @return The current instance
+   */
+  public ReflectData setDefaultsGenerated(boolean enabled) {
+    this.defaultGenerated = enabled;
+    return this;
+  }
+
+  private final Map<String, Object> defaultValues = new WeakHashMap<>();

Review comment:
       This needs to be <Class,Object> so that defaults are GC'd when a class is.  Also, Class should probably be used instead of String for classes in other code below.

##########
File path: lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectData.java
##########
@@ -110,14 +112,96 @@ public static ReflectData get() {
   }
 
   /**
-   * Cause a class to be treated as though it had an {@link Stringable}
-   ** annotation.
+   * Cause a class to be treated as though it had an {@link Stringable} *
+   * annotation.
    */
   public ReflectData addStringable(Class c) {
     stringableClasses.add(c);
     return this;
   }
 
+  /**
+   * If this flag is set to true, default values for fields will be assigned
+   * dynamically using Java reflections. Let's call this feature `default

Review comment:
       We might say a bit more about how this works, that, when enabled, defaults are the field values of an instance created with a no-arg constructor.




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