WIP - Decimal support #294
Conversation
|
Can one of the admins verify this patch? |
| @@ -67,10 +68,16 @@ | |||
| public static Object toAvro(JsonNode value, Schema schema) { | |||
| try { | |||
| ByteArrayOutputStream out = new ByteArrayOutputStream(); | |||
| value = JsonNodeConverters.transformJsonNode(value, schema, new JsonNodeConverter() { | |||
There was a problem hiding this comment.
i dont recall exactly how the JVM handles this (maybe it does optimally), but it might be more efficient to have your implementation of JsonNodeConverter defined somewhere rather than repeatedly created
| }; | ||
|
|
||
| static JsonNode decimalToText(JsonNode jsonNode, Schema schema) { | ||
| if (jsonNode instanceof DoubleNode) { |
There was a problem hiding this comment.
as a suggestion, I would make these methods more conditional in their names. For example, if the method was isDecimal (which encapsulated your instanceOf logic), then as a result of that condition being true, you'd do your conversion. False would lead to the function being an identity on the input jsonNode. The benefit of this is in AvroConverter, the code isn't read as "I am definitely turning this into text from a decimal", but more so "I will convert this into text iff it's a decimal". The same can be said for textToDecimal.
|
|
||
| static public String toEncodedString(BigDecimal bigDecimal, int scale, int precision) { | ||
| return new String(toByteArray(bigDecimal,scale), Charset.forName("ISO-8859-1")); | ||
| } |
There was a problem hiding this comment.
That's the only charset that I found that could encode the bytes the way I wanted for a String, so that my tests passed. Unsure why this one works and not others, but hoping for Confluent to let me know
| } else if (schema.getType() == Schema.Type.ARRAY) { | ||
| Schema subSchema = schema.getElementType(); | ||
| int i = 0; | ||
| for (Iterator<JsonNode> it = jsonNode.elements(); it.hasNext(); ) { |
There was a problem hiding this comment.
use instead
for (JsonNode elem: jsonNode.elements()) {
//
}
| ByteBuffer byteBuffer1 = (ByteBuffer) ((GenericData.Array) result).get(1); | ||
| assertEquals(new BigDecimal("555.55"), BigDecimalDecoder.fromBytes(byteBuffer1,2)); | ||
| } | ||
|
|
There was a problem hiding this comment.
can you add more test for different scales? You really only testing scale of 2
A big feature that was missing from the Rest proxy is the ability to send decimal fields. As it stands it requires the user to figure out a string to send, which is super risky and quite a complicated process.
Instead, my wish is for the rest proxy to take
{"foo": 123.45 }as an input, and do the conversion to avro correctly on its own. On read of decimal fields, it should do the opposite operation and return{"foo": 123.45 }I have added test cases that demonstrate all of this.
I look forward to feedback.
The one thing I'm not happy with is the use of
"ISO-8859-1". If you see a simpler solution please do let me know.The accuracy of this has been tested on a wide range of decimal numbers, and the deserialisation code from byte to BigDecimal has been directly sourced from the master version of avro at:
https://github.com/apache/avro/blob/33d495840c896b693b7f37b5ec786ac1acacd3b4/lang/java/avro/src/main/java/org/apache/avro/Conversions.java#L79
Needless to say, an update to the avro library would greatly simplify this PR