NIFI-5572 #3343
NIFI-5572 #3343
Conversation
|
@gideonkorir can you rebase and force push to your branch? |
gideonkorir
force-pushed
the
NIFI-5572
branch
from
7b47c47
to
95081ce
Compare
|
@MikeThomsen I've just done that again after fixing the style issues |
|
@gideonkorir there is a conflict and ideally the commits would be squashed to allow for reviews to result in easier to follow changes. |
|
@joewitt I've fixed the conflicts, I assumed it's a squash merge according to contributor guide. |
gideonkorir
force-pushed
the
NIFI-5572
branch
2 times, most recently
from
78029c0
to
bed5f88
Compare
DecimalDataType hashcode fix AvroTypeUtil conversions include decimal now :) Read/Write tests are passing - Added missing overrides to DecimalDataType - Add scale to DecimalDataType - Fix broken Avro tests - Fix broken ORC tests - Fix broken Csv tests - Fix broken Xml tests tests are passing fix checkstyle issues added missing license header look for getAsDouble and handle decimal conversions fixed bug with TestPutKudu.java decimal support for query record
…util to fix breaking tests
|
Realized I was naive in my initial PR, I've gone through all the references to
|
| @@ -265,6 +270,19 @@ public DataType getDataType(final String format) { | |||
| return new DataType(this, format); | |||
| } | |||
|
|
|||
| /** | |||
| * Returns a Data Type that represents a "Decimal" with given precision | |||
| * @param precision number of digits to the right of point (.) | |||
There was a problem hiding this comment.
The scale is actually the number of digits to the right of the point. The precision is the number of digits in the number.
There was a problem hiding this comment.
You right don't know what I was thinking when I wrote the comment, will fix that. thanks
| @@ -172,6 +180,8 @@ public static Object convertType(final Object value, final DataType dataType, fi | |||
|
|
|||
| return convertType(value, chosenDataType, fieldName, charset); | |||
| } | |||
| case DECIMAL: | |||
| return toDecimal(value, fieldName).setScale(((DecimalDataType)dataType).getScale(), RoundingMode.HALF_UP); | |||
There was a problem hiding this comment.
Why not pass the scale (and ideally precision) to the toDecimal method and set the scale and rounding mode (and check precision) within that method?
| if(value instanceof Number){ | ||
| return new BigDecimal(((Number)value).doubleValue()); | ||
| } | ||
|
|
There was a problem hiding this comment.
I would also check for precision here and throw an IllegalTypeConversionException if the required precision is less than the precision of the converted BigDecimal.
There was a problem hiding this comment.
Good feedback, will add that
| @@ -11,6 +11,7 @@ | |||
| "time" : "17:00:00", | |||
| "timestamp" : "2017-01-01 17:00:00", | |||
| "char" : "c", | |||
| "decimal" : "45.76", | |||
There was a problem hiding this comment.
I would rather have this as a number to be honest. There is a specific case in the WriteJsonResult class.
|
We're marking this PR as stale due to lack of updates in the past few months. If after another couple of weeks the stale label has not been removed this PR will be closed. This stale marker and eventual auto close does not indicate a judgement of the PR just lack of reviewer bandwidth and helps us keep the PR queue more manageable. If you would like this PR re-opened you can do so and a committer can remove the stale tag. Or you can open a new PR. Try to help review other PRs to increase PR review bandwidth which in turn helps yours. |
Add Decimal support