Updated output formats to output BigDecimal fields as numeric values instead of a byte representation. #1640
Conversation
|
|
||
| // Wrap BigDecimal fields in a wrapper which handles the conversion to String. | ||
| if (value instanceof BigDecimal) { | ||
| value = new BigDecimalWrapper((BigDecimal) value); |
There was a problem hiding this comment.
This wrapper is used because the original JsonWriter class uses toString() to generate the representation of the Number instance.
Becase of internal private methods, I can't simply implement this logic directly, so I'm using the wrapper class defined below as a workaround.
| * the License. | ||
| */ | ||
|
|
||
| package io.cdap.cdap.format.io; |
There was a problem hiding this comment.
I wrote this in this package so I had access to the underlaying JsonWriter instance.
fernst
force-pushed
the
format-bigdecimals-as-number-text-json-output
branch
2 times, most recently
from
c30d43c
to
929b962
Compare
| * Custom Datum Writer for Structured Records, this class writes BigDecimal values as numbers instead of byte arrays in | ||
| * the JSON output. | ||
| */ | ||
| public class BigDecimalAwareJsonStructuredRecordDatumWriter extends JsonStructuredRecordDatumWriter { |
There was a problem hiding this comment.
In which cases do we need the original represenatation? Also, I am wondering why is it a byte array given io.cdap.cdap.format.io.JsonStructuredRecordDatumWriter#encode(io.cdap.cdap.common.io.Encoder, io.cdap.cdap.api.data.schema.Schema, java.lang.Object) handles DECIMAL?
There was a problem hiding this comment.
because we pass the value directly from the StructuredRecord. So decimals are simply a byte array internally.
There was a problem hiding this comment.
I mean, why this logic does not go into JsonStructuredRecordDatumWriter itself?
|
|
||
| if (value != null && logicalType == Schema.LogicalType.DECIMAL) { | ||
| BigDecimal bdValue = fromObject(value, schema); | ||
| getJsonWriter(encoder).value(bdValue); |
There was a problem hiding this comment.
It looks like a hack. Why can't we do it without it?
There was a problem hiding this comment.
Because the code as implemented in the platform doesn't have logic to handle BigDecimals directly. Only primitive CDAP types are handled.
There was a problem hiding this comment.
io.cdap.cdap.format.io.JsonStructuredRecordDatumWriter#encode(io.cdap.cdap.common.io.Encoder, io.cdap.cdap.api.data.schema.Schema, java.lang.Object) has Decimal support. If it does not work, we should fix it over there.
There was a problem hiding this comment.
since it's cdap-formats it's shipped in plugin, so we can have a new plugin released with a cdap-formats fix without new CDAP release
There was a problem hiding this comment.
This class only handles Decimals if the logicalTypeAsString flag is set (which we only do for preview). It also writes the value as a String which would wrap the output in quotes
.../src/test/java/io/cdap/plugin/format/BigDecimalAwareStructuredRecordStringConverterTest.java
Show resolved
Hide resolved
| private static final StructuredRecord RECORD = StructuredRecord.builder(SCHEMA) | ||
| .set("i", 1) | ||
| .set("f", 256.1f) | ||
| .setDecimal("bd", BD) |
There was a problem hiding this comment.
Please also test for `.setDecimal(.., null)
...mmon/src/main/java/io/cdap/plugin/format/BigDecimalAwareStructuredRecordStringConverter.java
Show resolved
Hide resolved
| if (fieldSchema.getLogicalType() != null && fieldSchema.getLogicalType() == Schema.LogicalType.DECIMAL) { | ||
| BigDecimal decimalValue = record.getDecimal(fieldName); | ||
|
|
||
| // Throw exception if the field is expected tu be decimal, but it could not be processed as such. |
| Schema.LogicalType logicalType = nonNullableSchema.getLogicalType(); | ||
|
|
||
| if (value != null && logicalType == Schema.LogicalType.DECIMAL) { | ||
| BigDecimal bdValue = fromObject(value, schema); |
There was a problem hiding this comment.
BTW: Can we have a BigDecimalWrapper right here? This would remove the need for a special JsonWriter
There was a problem hiding this comment.
We still to call the writer directly, as the superclass doesn't expose methods to write number instances.
This is the reason I have to use the writer directly.
There was a problem hiding this comment.
I understand why do you use it directly (and that another question above, we should just expose it). My question is if you do wrapping right here, you would not need BigDecimalAwareJsonWriter and use just vanilla JsonWriter, no?
There was a problem hiding this comment.
The problem is the JsonWriter's value(Number number) method uses the Number class toString() method internally to generate the representation of the value that it needs to output.
The problem with this is that BigQuery's toString() method can represent values in scientific notation. In order to avoid this, I create that wrapper which delegates to BigQuery's toPlainString() method.
There was a problem hiding this comment.
Yep. If you pass this Wrapper to vanilla JsonWriter.value(Number) it would do the job, no? You don't need to create wrapper inside JsonWriter
fernst
force-pushed
the
format-bigdecimals-as-number-text-json-output
branch
from
929b962
to
efa31f4
Compare
…instead of a byte representation.
fernst
force-pushed
the
format-bigdecimals-as-number-text-json-output
branch
from
efa31f4
to
14a4992
Compare
No description provided.