[
https://issues.apache.org/jira/browse/AVRO-1847?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15333984#comment-15333984
]
Sean Busbey edited comment on AVRO-1847 at 6/16/16 3:21 PM:
------------------------------------------------------------
{code}
+ /**
+ * Convert the underlying representation of a logical type (such as a
+ * ByteBuffer) to a higher level object (such as a BigDecimal).
+ * @param datum The object to be converted
+ * @param schema The schema of datum. Cannot be null if datum is not null.
+ * @param type The {@link org.apache.avro.LogicalType} of datum. Cannot
+ * be null if datum is not null.
+ * @param conversion The tool used to finish the conversion. Should be a
+ * child of {@link org.apache.avro.Conversion}. Cannot
+ * be null if datum is not null.
+ * @return The result object, which is a meaningful object to the logical
type
+ */
+ public static Object convertToLogicalType(Object datum, Schema schema,
LogicalType type,
+ Conversion<?> conversion) {
{code}
The line about conversion being a child of org.apache.avro.Conversion should be
left out; it’s already implied by the type of the argument.
the return description should state that the result of passing datum in as null
is a null result.
the javadoc needs to say what exception we throw and what it means.
{code}
+
+ if (schema == null || type == null || conversion == null) {
+ throw new AvroRuntimeException("Parameters cannot be null! Parameter
values:" +
+ Arrays.deepToString(new Object[]{datum, schema, type, conversion}));
+ }
+
{code}
What’s the advantage of using {{AvroRuntimeException}} here instead of
{{IllegalArgumentException}}? Giving the wrong kind of parameters usually
results in IAE.
{code}
+
+ /**
+ * Convert a high level object for a logical type (such as a BigDecimal)
+ * to the its underlying representation object (such as a ByteBuffer)
+ * @param datum The object to be converted.
+ * @param schema The schema of datum. Cannot be null if datum is not null.
+ * @param type The {@link org.apache.avro.LogicalType} of datum. Cannot
+ * be null if datum is not null.
+ * @param conversion The tool used to finish the conversion. Should be a
+ * child of {@link org.apache.avro.Conversion}. Cannot
+ * be null if datum is not null.
+ * @return The result object, which is a meaningful object to the logical
type
+ */
+ public static <T> Object convertToRawType(Object datum, Schema schema,
LogicalType type,
+ Conversion<T> conversion) {
+ if (datum == null) {
+ return null;
+ }
+
+ if (schema == null || type == null || conversion == null) {
+ throw new AvroRuntimeException("Parameters cannot be null! Parameter
values:" +
+ Arrays.deepToString(new Object[]{datum, schema, type, conversion}));
+ }
+
{code}
Same feedback as on the {{convertToLogicalType}} method.
{code}
diff --git
a/lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumWriter.java
b/lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumWriter.java
index e66726e..cf06d63 100644
---
a/lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumWriter.java
+++
b/lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumWriter.java
@@ -24,8 +24,10 @@ import java.util.Iterator;
import java.util.Map;
import java.util.Collection;
+import org.apache.avro.AvroRuntimeException;
import org.apache.avro.AvroTypeException;
import org.apache.avro.Conversion;
+import org.apache.avro.Conversions;
import org.apache.avro.LogicalType;
import org.apache.avro.Schema;
import org.apache.avro.Schema.Field;
@@ -76,25 +78,25 @@ public class GenericDatumWriter<D> implements
DatumWriter<D> {
protected <T> Object convert(Schema schema, LogicalType logicalType,
Conversion<T> conversion, Object datum) {
- if (conversion == null) {
- return datum;
- }
- Class<T> fromClass = conversion.getConvertedType();
- switch (schema.getType()) {
- case RECORD: return conversion.toRecord(fromClass.cast(datum), schema,
logicalType);
- case ENUM: return conversion.toEnumSymbol(fromClass.cast(datum),
schema, logicalType);
- case ARRAY: return conversion.toArray(fromClass.cast(datum), schema,
logicalType);
- case MAP: return conversion.toMap(fromClass.cast(datum), schema,
logicalType);
- case FIXED: return conversion.toFixed(fromClass.cast(datum), schema,
logicalType);
- case STRING: return conversion.toCharSequence(fromClass.cast(datum),
schema, logicalType);
- case BYTES: return conversion.toBytes(fromClass.cast(datum), schema,
logicalType);
- case INT: return conversion.toInt(fromClass.cast(datum), schema,
logicalType);
- case LONG: return conversion.toLong(fromClass.cast(datum), schema,
logicalType);
- case FLOAT: return conversion.toFloat(fromClass.cast(datum), schema,
logicalType);
- case DOUBLE: return conversion.toDouble(fromClass.cast(datum), schema,
logicalType);
- case BOOLEAN: return conversion.toBoolean(fromClass.cast(datum), schema,
logicalType);
+ try {
+ if (conversion == null) {
+ return datum;
+ } else {
+ return Conversions.convertToRawType(datum, schema, logicalType,
conversion);
+ }
+ } catch (AvroRuntimeException e) {
+ Throwable cause = e.getCause();
+ if (cause != null && cause.getClass() == ClassCastException.class) {
+ // This is to keep backwards compatibility. The convert function here
used to
+ // throw CCE. When prompted to Conversions, it throws
AvroRuntimeException. So,
+ // if the cause is a CCE, rethrow it in case any child class checks
it. This
+ // behaviour can be changed later in future versions to make it
consistent with
+ // reading path, which throws AvroRuntimeException
+ throw (ClassCastException)cause;
+ } else {
+ throw e;
+ }
}
- return datum;
}
{code}
This is still going to change the behavior of the protected convert method when
arguments are wrong. I’m fine with that so long as we add javadocs to the
method and call it out in the release note for this lira.
[~rdblue] you fine with this too? or would you prefer that this version go into
the 1.9 branch and I munge things as needed to keep the same failure path in
the 1.8 branch?
{code}
diff --git
a/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java
b/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java
index 6bf7bd5..54e955c 100644
---
a/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java
+++
b/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java
…
@@ -106,6 +108,7 @@ public class SpecificCompiler {
private boolean createSetters = true;
private boolean createAllArgsConstructor = true;
private String outputCharacterEncoding;
+ private boolean enableDecimalLogicalType = false;
/*
* Used in the record.vm template.
@@ -208,6 +211,14 @@ public class SpecificCompiler {
this.createSetters = createSetters;
}
+ /**
+ * Set to false to use meaningful Java classes for logical types. For
example,
+ * use {@link java.math.BigDecimal} instead of {@link java.nio.ByteBuffer}
for logical type "decimal"
+ */
+ public void setEnableDecimalLogicalType(boolean enableDecimalLogicalType) {
+ this.enableDecimalLogicalType = enableDecimalLogicalType;
+ }
{code}
should be “set to true to use meaningful…” I think? Also should say “for the
decimal logical type” rather than for logical types generally.
{quote}
I think there should also be a test for this to make sure logical types without
conversions don't break the specific compiler.
{quote}
I see that there’s a check of the correct java type in
{{TestSpecificCompiler}}. [~rdblue] is this what you had in mind with this
request, or were you looking for end-to-end via something like the tests in
TestSpecificLogicalTypes?
was (Author: busbey):
{code}
+ /**
+ * Convert the underlying representation of a logical type (such as a
+ * ByteBuffer) to a higher level object (such as a BigDecimal).
+ * @param datum The object to be converted
+ * @param schema The schema of datum. Cannot be null if datum is not null.
+ * @param type The {@link org.apache.avro.LogicalType} of datum. Cannot
+ * be null if datum is not null.
+ * @param conversion The tool used to finish the conversion. Should be a
+ * child of {@link org.apache.avro.Conversion}. Cannot
+ * be null if datum is not null.
+ * @return The result object, which is a meaningful object to the logical
type
+ */
+ public static Object convertToLogicalType(Object datum, Schema schema,
LogicalType type,
+ Conversion<?> conversion) {
{code}
The line about conversion being a child of org.apache.avro.Conversion should be
left out; it’s already implied by the type of the argument.
the return description should state that the result of passing datum in as null
is a null result.
the javadoc needs to say what exception we throw and what it means.
{code}
+
+ if (schema == null || type == null || conversion == null) {
+ throw new AvroRuntimeException("Parameters cannot be null! Parameter
values:" +
+ Arrays.deepToString(new Object[]{datum, schema, type, conversion}));
+ }
+
{code}
What’s the advantage of using {{AvroRuntimeException}} here instead of
{{IllegalArgumentException}}? Giving the wrong kind of parameters usually
results in IAE.
{code}
+
+ /**
+ * Convert a high level object for a logical type (such as a BigDecimal)
+ * to the its underlying representation object (such as a ByteBuffer)
+ * @param datum The object to be converted.
+ * @param schema The schema of datum. Cannot be null if datum is not null.
+ * @param type The {@link org.apache.avro.LogicalType} of datum. Cannot
+ * be null if datum is not null.
+ * @param conversion The tool used to finish the conversion. Should be a
+ * child of {@link org.apache.avro.Conversion}. Cannot
+ * be null if datum is not null.
+ * @return The result object, which is a meaningful object to the logical
type
+ */
+ public static <T> Object convertToRawType(Object datum, Schema schema,
LogicalType type,
+ Conversion<T> conversion) {
+ if (datum == null) {
+ return null;
+ }
+
+ if (schema == null || type == null || conversion == null) {
+ throw new AvroRuntimeException("Parameters cannot be null! Parameter
values:" +
+ Arrays.deepToString(new Object[]{datum, schema, type, conversion}));
+ }
+
{code}
Same feedback as on the {{convertToLogicalType}} method.
{code}
diff --git
a/lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumWriter.java
b/lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumWriter.java
index e66726e..cf06d63 100644
---
a/lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumWriter.java
+++
b/lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumWriter.java
@@ -24,8 +24,10 @@ import java.util.Iterator;
import java.util.Map;
import java.util.Collection;
+import org.apache.avro.AvroRuntimeException;
import org.apache.avro.AvroTypeException;
import org.apache.avro.Conversion;
+import org.apache.avro.Conversions;
import org.apache.avro.LogicalType;
import org.apache.avro.Schema;
import org.apache.avro.Schema.Field;
@@ -76,25 +78,25 @@ public class GenericDatumWriter<D> implements
DatumWriter<D> {
protected <T> Object convert(Schema schema, LogicalType logicalType,
Conversion<T> conversion, Object datum) {
- if (conversion == null) {
- return datum;
- }
- Class<T> fromClass = conversion.getConvertedType();
- switch (schema.getType()) {
- case RECORD: return conversion.toRecord(fromClass.cast(datum), schema,
logicalType);
- case ENUM: return conversion.toEnumSymbol(fromClass.cast(datum),
schema, logicalType);
- case ARRAY: return conversion.toArray(fromClass.cast(datum), schema,
logicalType);
- case MAP: return conversion.toMap(fromClass.cast(datum), schema,
logicalType);
- case FIXED: return conversion.toFixed(fromClass.cast(datum), schema,
logicalType);
- case STRING: return conversion.toCharSequence(fromClass.cast(datum),
schema, logicalType);
- case BYTES: return conversion.toBytes(fromClass.cast(datum), schema,
logicalType);
- case INT: return conversion.toInt(fromClass.cast(datum), schema,
logicalType);
- case LONG: return conversion.toLong(fromClass.cast(datum), schema,
logicalType);
- case FLOAT: return conversion.toFloat(fromClass.cast(datum), schema,
logicalType);
- case DOUBLE: return conversion.toDouble(fromClass.cast(datum), schema,
logicalType);
- case BOOLEAN: return conversion.toBoolean(fromClass.cast(datum), schema,
logicalType);
+ try {
+ if (conversion == null) {
+ return datum;
+ } else {
+ return Conversions.convertToRawType(datum, schema, logicalType,
conversion);
+ }
+ } catch (AvroRuntimeException e) {
+ Throwable cause = e.getCause();
+ if (cause != null && cause.getClass() == ClassCastException.class) {
+ // This is to keep backwards compatibility. The convert function here
used to
+ // throw CCE. When prompted to Conversions, it throws
AvroRuntimeException. So,
+ // if the cause is a CCE, rethrow it in case any child class checks
it. This
+ // behaviour can be changed later in future versions to make it
consistent with
+ // reading path, which throws AvroRuntimeException
+ throw (ClassCastException)cause;
+ } else {
+ throw e;
+ }
}
- return datum;
}
{code}
This is still going to change the behavior of the protected convert method when
arguments are wrong. I’m fine with that so long as we add javadocs to the
method and call it out in the release note for this lira.
@rdblue you fine with this too? or would you prefer that this version go into
the 1.9 branch and I munge things as needed to keep the same failure path in
the 1.8 branch?
{code}
diff --git
a/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java
b/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java
index 6bf7bd5..54e955c 100644
---
a/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java
+++
b/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java
…
@@ -106,6 +108,7 @@ public class SpecificCompiler {
private boolean createSetters = true;
private boolean createAllArgsConstructor = true;
private String outputCharacterEncoding;
+ private boolean enableDecimalLogicalType = false;
/*
* Used in the record.vm template.
@@ -208,6 +211,14 @@ public class SpecificCompiler {
this.createSetters = createSetters;
}
+ /**
+ * Set to false to use meaningful Java classes for logical types. For
example,
+ * use {@link java.math.BigDecimal} instead of {@link java.nio.ByteBuffer}
for logical type "decimal"
+ */
+ public void setEnableDecimalLogicalType(boolean enableDecimalLogicalType) {
+ this.enableDecimalLogicalType = enableDecimalLogicalType;
+ }
{code}
should be “set to true to use meaningful…” I think? Also should say “for the
decimal logical type” rather than for logical types generally.
{quote}
I think there should also be a test for this to make sure logical types without
conversions don't break the specific compiler.
{quote}
I see that there’s a check of the correct java type in
{{TestSpecificCompiler}}. @rdblue is this what you had in mind with this
request, or were you looking for end-to-end via something like the tests in
TestSpecificLogicalTypes?
> IDL compiler uses ByteBuffer for decimal type even if logical type is
> supported
> --------------------------------------------------------------------------------
>
> Key: AVRO-1847
> URL: https://issues.apache.org/jira/browse/AVRO-1847
> Project: Avro
> Issue Type: Bug
> Components: java
> Affects Versions: 1.8.0
> Reporter: Yibing Shi
> Assignee: Yibing Shi
> Attachments: AVRO-1847.1.patch, AVRO-1847.2.patch, AVRO-1847.3.patch,
> AVRO-1847.4.patch, AVRO-1847.5.patch, AVRO-1847.6.patch
>
>
> Version 1.8.0 has added the support of logical types. A conversion class
> (Conversions.DecimalConversion) has also been added for decimal type.
> However, the IDL compiler still uses ByteBuffer for decimal types, which is
> not the same behaviour as data, time or timestamp type (added in AVRO-1684).
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
