[GitHub] [avro] clesaec commented on a diff in pull request #639: AVRO-2539: fix nullable types for avro to thrift

["clesaec (via GitHub)" <gi...@apache.org>]

clesaec commented on code in PR #639:
URL: https://github.com/apache/avro/pull/639#discussion_r1273516605


##########
lang/java/thrift/src/main/java/org/apache/avro/thrift/ThriftData.java:
##########
@@ -230,40 +247,57 @@ private Schema getSchema(FieldValueMetaData f) {
       return Schema.create(Schema.Type.DOUBLE);
     case TType.ENUM:
       EnumMetaData enumMeta = (EnumMetaData) f;
-      return nullable(getSchema(enumMeta.enumClass));
+      return nullable(getSchema(enumMeta.enumClass), hasNotNullDefaultValue);
     case TType.LIST:
       ListMetaData listMeta = (ListMetaData) f;
-      return nullable(Schema.createArray(getSchema(listMeta.elemMetaData)));
+      return nullable(Schema.createArray(getSchema(listMeta.elemMetaData)), hasNotNullDefaultValue);
     case TType.MAP:
       MapMetaData mapMeta = (MapMetaData) f;
       if (mapMeta.keyMetaData.type != TType.STRING)
         throw new AvroRuntimeException("Map keys must be strings: " + f);
       Schema map = Schema.createMap(getSchema(mapMeta.valueMetaData));
       GenericData.setStringType(map, GenericData.StringType.String);
-      return nullable(map);
+      return nullable(map, hasNotNullDefaultValue);
     case TType.SET:
       SetMetaData setMeta = (SetMetaData) f;
       Schema set = Schema.createArray(getSchema(setMeta.elemMetaData));
       set.addProp(THRIFT_PROP, "set");
-      return nullable(set);
+      return nullable(set, hasNotNullDefaultValue);
     case TType.STRING:
       if (f.isBinary())
         return nullable(Schema.create(Schema.Type.BYTES));
       Schema string = Schema.create(Schema.Type.STRING);
       GenericData.setStringType(string, GenericData.StringType.String);
-      return nullable(string);
+      return nullable(string, hasNotNullDefaultValue);
     case TType.STRUCT:
       StructMetaData structMeta = (StructMetaData) f;
       Schema record = getSchema(structMeta.structClass);
-      return nullable(record);
+      return nullable(record, hasNotNullDefaultValue);
     case TType.VOID:
       return NULL;
     default:
       throw new RuntimeException("Unexpected type in field: " + f);
     }
   }
 
+  @SuppressWarnings("unchecked")
+  private Object getOptionalDefaultValue(TBase instance, TFieldIdEnum field) {
+    if (instance.isSet(field)) {
+      return null;
+    } else {
+      return NULL_DEFAULT_VALUE;
+    }
+  }
+
   private Schema nullable(Schema schema) {
+    return nullable(schema, false);
+  }
+
+  private Schema nullable(Schema schema, boolean hasNotNullDefaultValue) {

Review Comment:
   Would be simpler to me with inverting boolean, as 
   `private Schema nullable(Schema schema, boolean defaultValueIsNull) {`
   Then caller would be 
   `s = nullable(s, defaultValue == NULL_DEFAULT_VALUE);`
   (otherwise, call this function with flase make double negative sentence, which is positive, double possitive is easier)



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