nifi-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [nifi] tpalfy commented on a change in pull request #4223: NIFI-7369 Adding big decimal support for record handling in order to avoid missing precision when reading in records
Date Wed, 27 May 2020 03:23:08 GMT

tpalfy commented on a change in pull request #4223:
URL: https://github.com/apache/nifi/pull/4223#discussion_r430518456



##########
File path: nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java
##########
@@ -1232,6 +1242,10 @@ public static boolean isBigIntTypeCompatible(final Object value) {
         return isNumberTypeCompatible(value, DataTypeUtils::isIntegral);
     }
 
+    public static boolean isBigDecimalTypeCompatible(final Object value) {
+        return isNumberTypeCompatible(value, DataTypeUtils::isFloatingPoint);

Review comment:
       This could be an unarmed landmine.
   The `DataTypeUtils::isFloatingPoint` has a `Float.parse()` with the comment above it: `//
Just to ensure that the exponents are in range, etc.`
   It ensures _no_ such thing in fact (if the number is out of range the parse still succeeds
and returns infinitiy), but this might get fixed later ("arming" the landmine), after which
for `BigDecimal` it might start to cause issues.

##########
File path: nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java
##########
@@ -34,6 +34,7 @@
 import java.io.InputStream;
 import java.io.Reader;
 import java.lang.reflect.Array;
+import java.math.BigDecimal;

Review comment:
       I think `public static Optional<DataType> getWiderType(final DataType thisDataType,
final DataType otherDataType)` should be updated as well.
   
   Could add along a new test in `TestFieldTypeInference`:
   ```java
       @Test
       public void test() {
           // GIVEN
           List<DataType> dataTypes = Arrays.asList(
               RecordFieldType.DECIMAL.getDecimalDataType(10, 1),
               RecordFieldType.DECIMAL.getDecimalDataType(10, 3),
               RecordFieldType.DECIMAL.getDecimalDataType(7, 3),
               RecordFieldType.DECIMAL.getDecimalDataType(7, 5),
               RecordFieldType.DECIMAL.getDecimalDataType(7, 7),
               RecordFieldType.FLOAT.getDataType(),
               RecordFieldType.DOUBLE.getDataType()
           );
   
           DataType expected = RecordFieldType.DECIMAL.getDecimalDataType(10, 7);
   
           // WHEN
           // THEN
           runWithAllPermutations(this::testToDataTypeShouldReturnSingleType, dataTypes, expected);
       }
   ```




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