tpalfy commented on a change in pull request #4223:
URL: https://github.com/apache/nifi/pull/4223#discussion_r413232172
##########
File path:
nifi-nar-bundles/nifi-hive-bundle/nifi-hive3-processors/src/test/java/org/apache/nifi/processors/orc/PutORCTest.java
##########
@@ -460,7 +459,7 @@ private void verifyORCUsers(final Path orcUsers, final int
numExpectedUsers, BiF
RecordReader recordReader = reader.rows();
TypeInfo typeInfo =
-
TypeInfoUtils.getTypeInfoFromTypeString("struct<name:string,favorite_number:int,favorite_color:string,scale:double>");
+
TypeInfoUtils.getTypeInfoFromTypeString("struct<name:string,favorite_number:int,favorite_color:string,scale:decimal>");
Review comment:
Something's strange. This change doesn't seem to be necessary.
Even if left as `scale:double` all tests pass.
Note the assertion bellow expects `double`:
```java
assertEquals(10.0 * currUser, ((DoubleWritable)
x.get(3)).get(), Double.MIN_VALUE);
```
##########
File path:
nifi-nar-bundles/nifi-hive-bundle/nifi-hive3-processors/src/test/java/org/apache/nifi/processors/orc/PutORCTest.java
##########
@@ -285,7 +284,7 @@ public void testWriteORCWithAvroLogicalTypes() throws
IOException, Initializatio
final DateFormat noTimeOfDayDateFormat = new
SimpleDateFormat("yyyy-MM-dd");
noTimeOfDayDateFormat.setTimeZone(TimeZone.getTimeZone("gmt"));
assertEquals(noTimeOfDayDateFormat.format(dt),
((DateWritableV2) x.get(3)).get().toString());
- assertEquals(dec, ((DoubleWritable) x.get(4)).get(),
Double.MIN_VALUE);
+ assertEquals(dec.doubleValue(), ((HiveDecimalWritable)
x.get(4)).doubleValue(), Double.MIN_VALUE);
Review comment:
Can't we depend on the accuracy of `BigDecimal`?
```suggestion
assertEquals(dec, ((HiveDecimalWritable)
x.get(4)).getHiveDecimal().bigDecimalValue());
```
##########
File path:
nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java
##########
@@ -1266,6 +1279,46 @@ public static boolean isBooleanTypeCompatible(final
Object value) {
return false;
}
+ public static BigDecimal toBigDecimal(final Object value, final String
fieldName) {
+ if (value == null) {
+ return null;
+ }
+
+ if (value instanceof BigDecimal) {
+ return (BigDecimal) value;
+ }
+
+ if (value instanceof Number) {
+ final Number number = (Number) value;
+
+ if (number instanceof Byte
+ || number instanceof Short
+ || number instanceof Integer
+ || number instanceof Long) {
+ return BigDecimal.valueOf(number.longValue());
+ }
+
+ if (number instanceof BigInteger) {
+ return new BigDecimal((BigInteger) number);
+ }
+
+ if (number instanceof Float || number instanceof Double) {
+ return BigDecimal.valueOf(number.doubleValue());
Review comment:
We should probably use
```java
new BigDecimal(Float.toString((Float)number))
```
and
```java
new BigDecimal(Double.toString((Double)number))
```
According to the javadoc for this construcor:
> Note: For values other than float and double NaN and ±Infinity, this
constructor is compatible with the values returned by Float.toString(float) and
Double.toString(double). This is generally the preferred way to convert a float
or double into a BigDecimal, as it doesn't suffer from the unpredictability of
the BigDecimal(double) constructor.
##########
File path:
nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java
##########
@@ -1759,6 +1812,8 @@ public static int getSQLTypeValue(final DataType
dataType) {
return Types.DOUBLE;
case FLOAT:
return Types.FLOAT;
+ case BIGDECIMAL:
+ return Types.NUMERIC;
Review comment:
Wondering if it can be used for `DECIMAL` columns as well?
##########
File path:
nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/TestDataTypeUtils.java
##########
@@ -265,6 +268,92 @@ public void testBytesToBytes() {
assertEquals("Conversion from byte[] to String failed at char 0",
(Object) "Hello".getBytes(StandardCharsets.UTF_16)[0], ((Byte[]) b)[0]);
}
+ @Test
+ public void testConvertToBigDecimalWhenInputIsValid() {
+ // given
+ final BigDecimal expectedValue = BigDecimal.valueOf(12L);
+
+ // when & then
+ whenExpectingValidBigDecimalConversion(expectedValue,
BigDecimal.valueOf(12L));
+ whenExpectingValidBigDecimalConversion(expectedValue, (byte) 12);
+ whenExpectingValidBigDecimalConversion(expectedValue, (short) 12);
+ whenExpectingValidBigDecimalConversion(expectedValue, 12);
+ whenExpectingValidBigDecimalConversion(expectedValue, 12L);
+ whenExpectingValidBigDecimalConversion(expectedValue,
BigInteger.valueOf(12L));
+ whenExpectingValidBigDecimalConversion(expectedValue, 12F);
+ whenExpectingValidBigDecimalConversion(expectedValue, 12.000F);
+ whenExpectingValidBigDecimalConversion(expectedValue, 12D);
+ whenExpectingValidBigDecimalConversion(expectedValue, 12.000D);
+ whenExpectingValidBigDecimalConversion(expectedValue, "12");
+ whenExpectingValidBigDecimalConversion(expectedValue, "12.000");
+ }
+
+ @Test
+ public void testConvertToBigDecimalWhenNullInput() {
+ assertNull(DataTypeUtils.convertType(null,
RecordFieldType.BIGDECIMAL.getDataType(), null, StandardCharsets.UTF_8));
+ }
+
+ @Test(expected = IllegalTypeConversionException.class)
+ public void testConvertToBigDecimalWhenInputStringIsInvalid() {
+ DataTypeUtils.convertType("test",
RecordFieldType.BIGDECIMAL.getDataType(), null, StandardCharsets.UTF_8);
+ }
+
+ @Test(expected = IllegalTypeConversionException.class)
+ public void testConvertToBigDecimalWhenUnsupportedType() {
+ DataTypeUtils.convertType(new ArrayList<Double>(),
RecordFieldType.BIGDECIMAL.getDataType(), null, StandardCharsets.UTF_8);
+ }
+
+ @Test(expected = IllegalTypeConversionException.class)
+ public void testConvertToBigDecimalWhenUnsupportedNumberType() {
+ DataTypeUtils.convertType(new DoubleAdder(),
RecordFieldType.BIGDECIMAL.getDataType(), null, StandardCharsets.UTF_8);
+ }
+
+ @Test
+ public void testCompatibleDataTypeBigDecimal() {
+ // given
+ final DataType dataType = RecordFieldType.BIGDECIMAL.getDataType();
+
+ // when & then
+ assertTrue(DataTypeUtils.isCompatibleDataType(new
BigDecimal("1.2345678901234567890"), dataType));
+ assertTrue(DataTypeUtils.isCompatibleDataType(new
BigInteger("12345678901234567890"), dataType));
+ assertTrue(DataTypeUtils.isCompatibleDataType(1234567890123456789L,
dataType));
+ assertTrue(DataTypeUtils.isCompatibleDataType(1, dataType));
+ assertTrue(DataTypeUtils.isCompatibleDataType((byte) 1, dataType));
+ assertTrue(DataTypeUtils.isCompatibleDataType((short) 1, dataType));
+ assertTrue(DataTypeUtils.isCompatibleDataType("1.2345678901234567890",
dataType));
+ assertTrue(DataTypeUtils.isCompatibleDataType(3.1F, dataType));
+ assertTrue(DataTypeUtils.isCompatibleDataType(3.0D, dataType));
+ assertFalse(DataTypeUtils.isCompatibleDataType("1234567XYZ",
dataType));
+ assertFalse(DataTypeUtils.isCompatibleDataType(new Long[]{1L, 2L},
dataType));
+ }
+
+ @Test
+ public void testInferDataTypeWithBigDecimal() {
+ assertEquals(RecordFieldType.BIGDECIMAL.getDataType(),
DataTypeUtils.inferDataType(BigDecimal.valueOf(12L),
RecordFieldType.BIGDECIMAL.getDataType()));
Review comment:
The 2nd parameter for `DataTypeUtils.inferDataType` (default return
value) is probable better to left as `null`.
##########
File path:
nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/TestDataTypeUtils.java
##########
@@ -265,6 +268,92 @@ public void testBytesToBytes() {
assertEquals("Conversion from byte[] to String failed at char 0",
(Object) "Hello".getBytes(StandardCharsets.UTF_16)[0], ((Byte[]) b)[0]);
}
+ @Test
Review comment:
Can we please add a test like this?
```java
@Test
public void
testChooseDataTypeWhenBigDecimal_vs_FLOAT_DOUBLE_BIGDECIMAL_ThenShouldReturnBIGDECIMAL()
{
// GIVEN
List<DataType> dataTypes = Arrays.asList(
RecordFieldType.FLOAT.getDataType(),
RecordFieldType.DOUBLE.getDataType(),
RecordFieldType.BIGDECIMAL.getDataType()
);
Object value = new BigDecimal("1.2");
DataType expected = RecordFieldType.BIGDECIMAL.getDataType();
// WHEN
// THEN
testChooseDataTypeAlsoReverseTypes(value, dataTypes, expected);
}
```
##########
File path:
nifi-nar-bundles/nifi-standard-services/nifi-record-serialization-services-bundle/nifi-record-serialization-services/src/main/java/org/apache/nifi/json/WriteJsonResult.java
##########
@@ -376,6 +377,9 @@ private void writeValue(final JsonGenerator generator,
final Object value, final
case STRING:
generator.writeString(coercedValue.toString());
break;
+ case BIGDECIMAL:
+ generator.writeNumber((BigDecimal) coercedValue);
Review comment:
We might want
```java
generator.writeNumber(DataTypeUtils.toBigDecimal(coercedValue, fieldName));
```
just to follow conventions. I think `coercedValue` can only be `BigDecimal`
at this point but not sure if that is by design or because of the current
implementation.
##########
File path:
nifi-nar-bundles/nifi-solr-bundle/nifi-solr-processors/src/main/java/org/apache/nifi/processors/solr/SolrUtils.java
##########
@@ -463,6 +463,8 @@ private static void writeValue(final SolrInputDocument
inputDocument, final Obje
addFieldToSolrDocument(inputDocument,fieldName,(BigInteger)coercedValue,fieldsToIndex);
}
break;
+ case BIGDECIMAL:
+ addFieldToSolrDocument(inputDocument, fieldName, coercedValue,
fieldsToIndex);
Review comment:
I'd use `DataTypeUtils.toBigDecimal(coercedValue, fieldName)` to be safe
and follow conventions.
##########
File path:
nifi-nar-bundles/nifi-extension-utils/nifi-record-utils/nifi-standard-record-utils/src/test/java/org/apache/nifi/schema/validation/TestStandardSchemaValidator.java
##########
@@ -145,6 +148,9 @@ public void
testValidateCorrectSimpleTypesStrictValidation() throws ParseExcepti
valueMap.put("float_as_double", 8.0F);
+ valueMap.put("float_as_bigdecimal", 8.0F);
Review comment:
Minor: we might want to add `"byte/short/int/long/bigint_as_bigdecimal"`
as well.
Kind of redundant with the ones below but still better to have this stay a
simplified universal test.
##########
File path:
nifi-nar-bundles/nifi-solr-bundle/nifi-solr-processors/src/test/java/org/apache/nifi/processors/solr/SolrUtilsTest.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.processors.solr;
+
+import org.apache.nifi.serialization.SimpleRecordSchema;
+import org.apache.nifi.serialization.record.MapRecord;
+import org.apache.nifi.serialization.record.Record;
+import org.apache.nifi.serialization.record.RecordField;
+import org.apache.nifi.serialization.record.RecordFieldType;
+import org.apache.solr.common.SolrInputDocument;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import java.math.BigDecimal;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+@RunWith(MockitoJUnitRunner.class)
+public class SolrUtilsTest {
+
+ @Mock
+ private SolrInputDocument inputDocument;
+
+ @Test
+ public void test() throws Exception {
+ // given
+ final String value =
"12345678901234567890.123456789012345678901234567890";
+ final BigDecimal bigDecimalValue = new BigDecimal(value);
+ final List<RecordField> fields = Collections.singletonList(new
RecordField("test", RecordFieldType.BIGDECIMAL.getDataType()));
+
+ final Map<String, Object> values = new HashMap<>();
+ values.put("test", bigDecimalValue);
+
+ final Record record = new MapRecord(new SimpleRecordSchema(fields),
values);
+ final List<String> fieldsToIndex =
Collections.singletonList("parent_test");
+
+ // when
+ SolrUtils.writeRecord(record, inputDocument, fieldsToIndex, "parent");
+
+ // then
+ Mockito.verify(inputDocument,
Mockito.times(1)).addField(Mockito.eq("parent_test"),
Mockito.eq(bigDecimalValue));
Review comment:
Minor: could be simplified to
```java
Mockito.verify(inputDocument).addField("parent_test",
bigDecimalValue);
```
----------------------------------------------------------------
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:
[email protected]
