avro-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [avro] RyanSkraba commented on a change in pull request #973: AVRO-2952: AvroDatatype
Date Tue, 02 Mar 2021 17:41:16 GMT

RyanSkraba commented on a change in pull request #973:
URL: https://github.com/apache/avro/pull/973#discussion_r585662437



##########
File path: lang/java/avro/src/main/java/org/apache/avro/logicaltypes/AvroByte.java
##########
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.logicaltypes;
+
+import org.apache.avro.AvroTypeException;
+import org.apache.avro.LogicalType;
+import org.apache.avro.Schema;
+import org.apache.avro.Schema.Type;
+
+/**
+ * Based on an INT but is supposed to hold data from -127 to +128 only. A single

Review comment:
       A "Byte" logical type, if it were to exist, would probably be better off as a `FIXED`
!

##########
File path: lang/java/avro/src/main/java/org/apache/avro/logicaltypes/AvroBoolean.java
##########
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.logicaltypes;
+
+import org.apache.avro.AvroTypeException;
+import org.apache.avro.Schema;
+import org.apache.avro.Schema.Type;
+
+/**
+ * Wrapper for the Avro BOOLEAN type.
+ */
+public class AvroBoolean implements AvroPrimitive {
+  public static final String NAME = "BOOLEAN";
+  private static final AvroBoolean ELEMENT = new AvroBoolean();
+  private static final Schema SCHEMA = Schema.create(Type.BOOLEAN);
+
+  private AvroBoolean() {
+    super();
+  }
+
+  public static AvroBoolean create() {
+    return ELEMENT;
+  }
+
+  @Override
+  public String toString() {
+    return NAME;
+  }
+
+  @Override
+  public Boolean convertToRawType(Object value) {
+    if (value == null) {
+      return null;
+    } else if (value instanceof Boolean) {
+      return (Boolean) value;
+    } else if (value instanceof String) {
+      if ("TRUE".equalsIgnoreCase((String) value)) {

Review comment:
       Throughout the PR, these conversion rules are a bit surprising.  Are they aligned to
a standard?
   
   It kind of doesn't matter which rules we pick, some developer will think it's unexpected,
and this should be well-documented.

##########
File path: lang/java/avro/src/main/java/org/apache/avro/LogicalTypes.java
##########
@@ -222,6 +267,19 @@ private Decimal(Schema schema) {
       }
     }
 
+    protected Decimal(String text) {

Review comment:
       For initialising from a string like DECIMAL(XX,YY) ? 

##########
File path: lang/java/avro/src/main/java/org/apache/avro/logicaltypes/AvroBytes.java
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.logicaltypes;
+
+import java.nio.ByteBuffer;
+
+import org.apache.avro.AvroTypeException;
+import org.apache.avro.Schema;
+import org.apache.avro.Schema.Type;
+
+/**
+ * Is the Avro Type.BYTES datatype, a binary store of any length. A BLOB column.
+ *
+ */
+public class AvroBytes implements AvroPrimitive {
+  public static final String NAME = "BYTES";
+  private static final AvroBytes ELEMENT = new AvroBytes();
+  private static final Schema SCHEMA = Schema.create(Type.BYTES);
+
+  private AvroBytes() {
+    super();
+  }
+
+  public static AvroBytes create() {
+    return ELEMENT;
+  }
+
+  @Override
+  public String toString() {
+    return NAME;
+  }
+
+  @Override
+  public ByteBuffer convertToRawType(Object value) {
+    if (value == null) {
+      return null;
+    } else if (value instanceof ByteBuffer) {
+      return (ByteBuffer) value;
+    } else if (value instanceof byte[]) {
+      return ByteBuffer.wrap((byte[]) value);
+    } else if (value instanceof Number) {
+      byte[] b = new byte[1];

Review comment:
       To me, this is also a surprising conversion choice.

##########
File path: lang/java/avro/src/main/java/org/apache/avro/logicaltypes/AvroDatatype.java
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.logicaltypes;
+
+import org.apache.avro.Schema;
+import org.apache.avro.Schema.Type;
+
+/**
+ * This interface defines what information an Avro data type should provide and
+ * defines default conversion routines.
+ *
+ */
+public interface AvroDatatype {
+
+  /**
+   * Converts an input value into the object used by the backing data type. For
+   * example the data type is a timestamp-millis, which is backed by a Long. This
+   * method will convert any suitable input, e.g. Long, Instant,... into a long
+   * value required by the writer.
+   * 
+   * @param value is the provided input
+   * @return an object in the correct backing data type
+   */
+  Object convertToRawType(Object value);

Review comment:
       Should the output of this conversion be a generic?  `<T>`  It might be an opportunity
to give a boost to type safety.

##########
File path: lang/java/avro/src/main/java/org/apache/avro/logicaltypes/AvroDate.java
##########
@@ -0,0 +1,115 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.logicaltypes;
+
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.ZoneId;
+import java.time.ZonedDateTime;
+import java.util.Date;
+
+import org.apache.avro.AvroTypeException;
+import org.apache.avro.LogicalTypes;
+import org.apache.avro.Schema;
+import org.apache.avro.Schema.Type;
+import org.apache.avro.data.TimeConversions.DateConversion;
+
+/**
+ * Based on a Avro Type.INT holds the date portion without time.
+ *
+ */
+public class AvroDate extends LogicalTypes.Date implements AvroPrimitive {
+  public static final String NAME = "DATE";
+  public static final String TYPENAME = LogicalTypes.DATE;
+  private static final Schema SCHEMA;
+  private static final AvroDate ELEMENT = new AvroDate();
+  private static final DateConversion CONVERTER = new DateConversion();
+
+  static {
+    SCHEMA = ELEMENT.addToSchema(Schema.create(Type.INT));
+  }
+
+  private AvroDate() {
+    super();
+  }
+
+  public static AvroDate create() {
+    return ELEMENT;
+  }
+
+  @Override
+  public String toString() {
+    return NAME;
+  }
+
+  @Override
+  public Integer convertToRawType(Object value) {
+    if (value == null) {
+      return null;
+    } else if (value instanceof Integer) {
+      return (Integer) value;
+    } else if (value instanceof LocalDate) {
+      return CONVERTER.toInt((LocalDate) value, null, this);
+    } else if (value instanceof Number) {
+      return convertToRawType(((Number) value).intValue());
+    } else if (value instanceof CharSequence) {
+      return convertToRawType(LocalDate.parse((CharSequence) value));

Review comment:
       This is almost certainly going to be unexpected to somebody (even if ISO_LOCAL_DATE
is the least surprising of the formats)!

##########
File path: lang/java/avro/src/main/java/org/apache/avro/logicaltypes/AvroShort.java
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.logicaltypes;
+
+import org.apache.avro.AvroTypeException;
+import org.apache.avro.LogicalType;
+import org.apache.avro.Schema;
+import org.apache.avro.Schema.Type;
+
+/**
+ * Based on an Avro Type.INT holds 2-byte signed numbers.
+ *
+ */
+public class AvroShort extends LogicalType implements AvroPrimitive {
+  public static final String NAME = "SHORT";
+  public static final String TYPENAME = NAME;

Review comment:
       To this point, all of the logical types shipping with Avro come in lower-case-kebab
style.

##########
File path: lang/java/avro/src/main/java/org/apache/avro/Schema.java
##########
@@ -565,6 +633,17 @@ public Field(String name, Schema schema) {
       this(name, schema, (String) null, (JsonNode) null, true, Order.ASCENDING);
     }
 
+    /**
+     * This is the data type of the field, e.g. if the field is defined as
+     * union[NULL, Long] the returned data type is AvroLong because the field should
+     * be set with Long values.
+     * 
+     * @return the AvroDataType used to convert values for this field
+     */
+    public AvroDatatype getDataType() {

Review comment:
       Minor nit-pick -- some inconsistent capital letters!

##########
File path: lang/java/avro/src/main/java/org/apache/avro/Schema.java
##########
@@ -191,6 +206,57 @@ void setLogicalType(LogicalType logicalType) {
     this.logicalType = logicalType;
   }
 
+  /**
+   * For the Avro-defined primitive and complex data types this method returns
+   * matching AvroDatatype objects. The Avro-provided logical data types
+   * themselves implement the AvroDatatype interface already.
+   * 
+   * @return An object with more metadata about the data type and methods to
+   *         convert values
+   */
+  public AvroDatatype getDataType() {
+    if (logicalType != null && logicalType instanceof AvroDatatype) {
+      return (AvroDatatype) logicalType;

Review comment:
       So you can create new customized LogicalTypes and use them as long as they implement
AvroDatatype.

##########
File path: lang/java/avro/src/main/java/org/apache/avro/logicaltypes/AvroType.java
##########
@@ -0,0 +1,188 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.logicaltypes;
+
+import java.util.List;
+
+import org.apache.avro.Schema;
+import org.apache.avro.Schema.Type;
+import org.apache.commons.text.StringEscapeUtils;
+
+/**
+ * ENUM with the list of all Avro data types
+ *
+ */
+public enum AvroType {
+  /**
+   * A 8bit signed integer
+   */
+  AVROBYTE,
+  /**
+   * ASCII text of large size, comparison and sorting is binary
+   */
+  AVROCLOB,
+  /**
+   * Unicode text of large size, comparison and sorting is binary
+   */
+  AVRONCLOB,
+  /**
+   * Unicode text up t n chars long, comparison and sorting is binary
+   */
+  AVRONVARCHAR,
+  /**
+   * A 16bit signed integer
+   */
+  AVROSHORT,
+  /**
+   * A Spatial data type in WKT representation
+   */
+  AVROSTGEOMETRY,
+  /**
+   * A Spatial data type in WKT representation
+   */
+  AVROSTPOINT,
+  /**
+   * A string as URI
+   */
+  AVROURI,
+  /**
+   * An ASCII string of n chars length, comparison and sorting is binary
+   */
+  AVROVARCHAR,
+  /**
+   * A date without time information
+   */
+  AVRODATE,
+  /**
+   * A numeric value with precision and scale
+   */
+  AVRODECIMAL,
+  /**
+   * A time information down to milliseconds
+   */
+  AVROTIMEMILLIS,
+  /**
+   * A time information down to microseconds
+   */
+  AVROTIMEMICROS,
+  /**
+   * A timestamp down to milliseconds in UTC
+   */
+  AVROTIMESTAMPMILLIS,
+  /**
+   * A timestamp down to microseconds in UTC
+   */
+  AVROTIMESTAMPMICROS,
+  /**
+   * A timestamp down to milliseconds without time zone info
+   */
+  AVROLOCALTIMESTAMPMILLIS,
+  /**
+   * A timestamp down to microseconds without time zone info
+   */
+  AVROLOCALTIMESTAMPMICROS,
+  /**
+   * Boolean
+   */
+  AVROBOOLEAN,
+  /**
+   * A 32bit signed integer value
+   */
+  AVROINT,
+  /**
+   * A 64bit signed integer value
+   */
+  AVROLONG,
+  /**
+   * A 32bit floating point number
+   */
+  AVROFLOAT,
+  /**
+   * A 64bit floating point number
+   */
+  AVRODOUBLE,
+  /**
+   * Binary data of any length
+   */
+  AVROBYTES,
+  /**
+   * A unbounded unicode text - prefer using nvarchar or nclob instead to indicate
+   * its normal length, comparison and sorting is binary
+   */
+  AVROSTRING,
+  /**
+   * A binary object with an upper size limit
+   */
+  AVROFIXED,
+  /**
+   * A unicode string with a list of allowed values - one of enum(), comparison
+   * and sorting is binary
+   */
+  AVROENUM,
+  /**
+   * A unicode string array with a list of allowed values - many of map(),
+   * comparison and sorting is binary
+   */
+  AVROMAP,
+  /**
+   * An ASCII string formatted as UUID, comparison and sorting is binary
+   */
+  AVROUUID,
+  /**
+   * An array of elements
+   */
+  AVROARRAY,
+  /**
+   * A Record of its own
+   */
+  AVRORECORD;
+
+  /**
+   * In case this schema is a union of null and something else, it returns the
+   * _something else_
+   * 
+   * @param schema of the input
+   * @return schema without the union of null, in case it is just that. Does
+   *         return an union in all other cases.
+   */
+  public static Schema getBaseSchema(Schema schema) {
+    if (schema == null) {
+      return null;
+    } else if (schema.getType() == Type.UNION) {
+      List<Schema> types = schema.getTypes();
+      if (types.size() == 2 && types.get(0).getType() == Type.NULL) {

Review comment:
       NULL appearing first in a union is just a convention.  It can be the second branch
in a UNION

##########
File path: lang/java/avro/src/main/java/org/apache/avro/logicaltypes/AvroDecimal.java
##########
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.logicaltypes;
+
+import java.math.BigDecimal;
+import java.math.RoundingMode;
+import java.nio.ByteBuffer;
+
+import org.apache.avro.AvroTypeException;
+import org.apache.avro.Conversions.DecimalConversion;
+import org.apache.avro.LogicalTypes;
+import org.apache.avro.LogicalTypes.Decimal;
+import org.apache.avro.Schema;
+import org.apache.avro.Schema.Type;
+import org.apache.avro.generic.GenericFixed;
+
+public class AvroDecimal extends Decimal implements AvroPrimitive {
+  private static final DecimalConversion DECIMAL_CONVERTER = new DecimalConversion();
+  public static final String NAME = "DECIMAL";
+  public static final String TYPENAME = LogicalTypes.DECIMAL;
+
+  public AvroDecimal(String text) {
+    super(text);
+  }
+
+  public AvroDecimal(int precision, int scale) {
+    super(precision, scale);
+  }
+
+  public static AvroDecimal create(int precision, int scale) {
+    return new AvroDecimal(precision, scale);
+  }
+
+  public AvroDecimal(Schema schema) {
+    super(schema);
+  }
+
+  @Override
+  public String toString() {
+    return NAME + "(" + getPrecision() + "," + getScale() + ")";
+  }
+
+  @Override
+  public Schema getRecommendedSchema() {
+    return addToSchema(Schema.create(Type.BYTES));

Review comment:
       This can also be a FIXED -- is there any reason to recommend one over the other?  Could
we have two AvroDecimal?

##########
File path: lang/java/avro/src/main/java/org/apache/avro/logicaltypes/AvroDatatype.java
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.logicaltypes;
+
+import org.apache.avro.Schema;
+import org.apache.avro.Schema.Type;
+
+/**
+ * This interface defines what information an Avro data type should provide and
+ * defines default conversion routines.
+ *
+ */
+public interface AvroDatatype {
+
+  /**
+   * Converts an input value into the object used by the backing data type. For
+   * example the data type is a timestamp-millis, which is backed by a Long. This
+   * method will convert any suitable input, e.g. Long, Instant,... into a long
+   * value required by the writer.
+   * 
+   * @param value is the provided input
+   * @return an object in the correct backing data type
+   */
+  Object convertToRawType(Object value);
+
+  /**
+   * @return the Avro schema type used for storing this logical type
+   */
+  Type getBackingType();
+
+  /**
+   * @return a default schema for this data type for primitives, null otherwise
+   */
+  Schema getRecommendedSchema();

Review comment:
       Where is this used?  Why is it recommended?  (I noticed that the decimal logical type
recommends bytes, but it can be bytes or fixed).
   
   I've never seen a logical type on an aggregate type (like a UNION or a RECORD), but I think
it's meant to be possible.

##########
File path: lang/java/avro/src/main/java/org/apache/avro/logicaltypes/AvroEnum.java
##########
@@ -0,0 +1,115 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.logicaltypes;
+
+import org.apache.avro.AvroTypeException;
+import org.apache.avro.Schema;
+import org.apache.avro.Schema.Type;
+import org.apache.avro.generic.GenericData.EnumSymbol;
+import org.apache.avro.generic.GenericEnumSymbol;
+
+/**
+ * Wrapper around the Avro Type.ENUM data type
+ *
+ */
+public class AvroEnum implements AvroPrimitive {
+  public static final String NAME = "ENUM";
+  private final Schema schema;
+
+  public AvroEnum(Schema schema) {
+    super();
+    this.schema = schema;
+  }
+
+  public static AvroEnum create(Schema schema) {
+    return new AvroEnum(schema);
+  }
+
+  @Override
+  public boolean equals(Object o) {
+    if (this == o)
+      return true;
+    if (o == null || getClass() != o.getClass())
+      return false;
+    if (schema != null) {
+      if (((AvroEnum) o).getRecommendedSchema() != null) {
+        return schema.equals(((AvroEnum) o).getRecommendedSchema());
+      }
+    } else if (((AvroEnum) o).getRecommendedSchema() == null) {
+      return true;
+    }
+    return false;
+  }
+
+  @Override
+  public int hashCode() {
+    if (schema != null) {
+      return schema.hashCode();

Review comment:
       I'm not sure I understand under what circumstances this schema can be null.

##########
File path: lang/java/avro/src/main/java/org/apache/avro/logicaltypes/AvroBoolean.java
##########
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.logicaltypes;
+
+import org.apache.avro.AvroTypeException;
+import org.apache.avro.Schema;
+import org.apache.avro.Schema.Type;
+
+/**
+ * Wrapper for the Avro BOOLEAN type.
+ */
+public class AvroBoolean implements AvroPrimitive {
+  public static final String NAME = "BOOLEAN";
+  private static final AvroBoolean ELEMENT = new AvroBoolean();
+  private static final Schema SCHEMA = Schema.create(Type.BOOLEAN);
+
+  private AvroBoolean() {
+    super();
+  }
+
+  public static AvroBoolean create() {
+    return ELEMENT;
+  }
+
+  @Override
+  public String toString() {
+    return NAME;
+  }
+
+  @Override
+  public Boolean convertToRawType(Object value) {
+    if (value == null) {
+      return null;
+    } else if (value instanceof Boolean) {
+      return (Boolean) value;
+    } else if (value instanceof String) {
+      if ("TRUE".equalsIgnoreCase((String) value)) {
+        return Boolean.TRUE;
+      } else if ("FALSE".equalsIgnoreCase((String) value)) {
+        return Boolean.FALSE;
+      }
+    } else if (value instanceof Number) {
+      int v = ((Number) value).intValue();
+      if (v == 1) {
+        return Boolean.TRUE;
+      } else if (v == 0) {
+        return Boolean.FALSE;
+      }
+    }
+    throw new AvroTypeException(
+        "Cannot convert a value of type \"" + value.getClass().getSimpleName() + "\" into
a Boolean");

Review comment:
       This could say `Cannot convert a value of type "String" into a Boolean`
   
   For AvroByte, for example, specific nonconvertible values are printed.

##########
File path: lang/java/avro/src/main/java/org/apache/avro/logicaltypes/AvroDatatype.java
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.logicaltypes;
+
+import org.apache.avro.Schema;
+import org.apache.avro.Schema.Type;
+
+/**
+ * This interface defines what information an Avro data type should provide and
+ * defines default conversion routines.
+ *
+ */
+public interface AvroDatatype {

Review comment:
       For consistency: `AvroDataType`?  If we could come up with a more explicit name, that
would be better!  I mean, this is specifically responsible for doing to logical type conversions
and providing more information about the logical type.  If `Conversion` didn't already exist,
it would be good candidate for a name.

##########
File path: lang/java/avro/src/main/java/org/apache/avro/logicaltypes/AvroDatatype.java
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.avro.logicaltypes;
+
+import org.apache.avro.Schema;
+import org.apache.avro.Schema.Type;
+
+/**
+ * This interface defines what information an Avro data type should provide and
+ * defines default conversion routines.
+ *
+ */
+public interface AvroDatatype {
+
+  /**
+   * Converts an input value into the object used by the backing data type. For
+   * example the data type is a timestamp-millis, which is backed by a Long. This
+   * method will convert any suitable input, e.g. Long, Instant,... into a long
+   * value required by the writer.
+   * 
+   * @param value is the provided input
+   * @return an object in the correct backing data type
+   */
+  Object convertToRawType(Object value);
+
+  /**
+   * @return the Avro schema type used for storing this logical type
+   */
+  Type getBackingType();

Review comment:
       There's a question of consistent terminology here -- sometimes it's **"raw"** type
and sometimes it's **"backing"** type.
   
   I have typically used **"underlying"** type (or "primitive" type if I can be sure that
the type underneath is a primitive).  I like either of your terms better, but we should try
to be consistent thorught the code.

##########
File path: lang/java/avro/src/main/java/org/apache/avro/Schema.java
##########
@@ -565,6 +633,17 @@ public Field(String name, Schema schema) {
       this(name, schema, (String) null, (JsonNode) null, true, Order.ASCENDING);
     }
 
+    /**
+     * This is the data type of the field, e.g. if the field is defined as
+     * union[NULL, Long] the returned data type is AvroLong because the field should
+     * be set with Long values.

Review comment:
       This is also true for any UNION schema -- shouldn't `getField(0).getDataType()` here
always be equivalent to `getField(0).schema().getDataType()`?




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



Mime
View raw message