exceptionfactory commented on a change in pull request #5388:
URL: https://github.com/apache/nifi/pull/5388#discussion_r710100686
##########
File path:
nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/ResultSetRecordSet.java
##########
@@ -298,7 +272,67 @@ private DataType getDataType(final int sqlType, final
ResultSet rs, final int co
}
}
- private static DataType getArrayBaseType(final Array array) throws
SQLException {
+ private DataType determineDataTypeToReturn(DataType dataType, boolean
useLogicalTypes) {
+ RecordFieldType fieldType = dataType.getFieldType();
+ if (!useLogicalTypes
+ && (fieldType == RecordFieldType.DECIMAL
+ || fieldType == RecordFieldType.DATE
+ || fieldType == RecordFieldType.TIME
+ || fieldType == RecordFieldType.TIMESTAMP)) {
+ return RecordFieldType.STRING.getDataType();
+ } else {
+ return dataType;
+ }
+ }
+
+ private DataType getArrayDataType(ResultSet rs, int columnIndex, boolean
useLogicalTypes) throws SQLException {
+ // The JDBC API does not allow us to know what the base type of an
array is through the metadata.
+ // As a result, we have to obtain the actual Array for this record.
Once we have this, we can determine
+ // the base type. However, if the base type is, itself, an array, we
will simply return a base type of
+ // String because otherwise, we need the ResultSet for the array
itself, and many JDBC Drivers do not
+ // support calling Array.getResultSet() and will throw an Exception if
that is not supported.
+ try {
+ final Array array = rs.getArray(columnIndex);
+
+ if (array == null) {
+ return
RecordFieldType.ARRAY.getArrayDataType(RecordFieldType.STRING.getDataType());
+ }
+ final DataType baseType = getArrayBaseType(array, useLogicalTypes);
+ return RecordFieldType.ARRAY.getArrayDataType(baseType);
+ } catch (SQLFeatureNotSupportedException sfnse) {
+ return
RecordFieldType.ARRAY.getArrayDataType(RecordFieldType.STRING.getDataType());
+ }
+ }
+
+ private DataType getDecimalDataType(ResultSet rs, int columnIndex) throws
SQLException {
Review comment:
Recommend `final`:
```suggestion
private DataType getDecimalDataType(final ResultSet rs, final int
columnIndex) throws SQLException {
```
##########
File path:
nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/ResultSetRecordSet.java
##########
@@ -298,7 +272,67 @@ private DataType getDataType(final int sqlType, final
ResultSet rs, final int co
}
}
- private static DataType getArrayBaseType(final Array array) throws
SQLException {
+ private DataType determineDataTypeToReturn(DataType dataType, boolean
useLogicalTypes) {
+ RecordFieldType fieldType = dataType.getFieldType();
+ if (!useLogicalTypes
+ && (fieldType == RecordFieldType.DECIMAL
+ || fieldType == RecordFieldType.DATE
+ || fieldType == RecordFieldType.TIME
+ || fieldType == RecordFieldType.TIMESTAMP)) {
+ return RecordFieldType.STRING.getDataType();
+ } else {
+ return dataType;
+ }
+ }
+
+ private DataType getArrayDataType(ResultSet rs, int columnIndex, boolean
useLogicalTypes) throws SQLException {
Review comment:
Recommend adding `final` to these parameters:
```suggestion
private DataType getArrayDataType(final ResultSet rs, final int
columnIndex, final boolean useLogicalTypes) throws SQLException {
```
##########
File path:
nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/ResultSetRecordSetTest.java
##########
@@ -272,6 +278,76 @@ public void
testCreateSchemaArrayThrowsNotSupportedException() throws SQLExcepti
assertEquals(RecordFieldType.ARRAY.getArrayDataType(RecordFieldType.STRING.getDataType()),
resultSchema.getField(0).getDataType());
}
+ @Test
+ public void testCreateSchemaWithLogicalTypes() throws SQLException {
+ testCreateSchemaLogicalTypes(true);
+ }
+
+ @Test
+ public void testCreateSchemaNoLogicalTypes() throws SQLException {
+ testCreateSchemaLogicalTypes(false);
+ }
+
+ private void testCreateSchemaLogicalTypes(boolean useLogicalTypes) throws
SQLException {
+ // GIVEN
+ Object[][] columns = new Object[][] {
Review comment:
This structure seems difficult to maintain as it requires precise index
references in other methods. It seems like it would be better and easier to
use one or or Maps with more precise types. Another option might be a List of
a custom objects declared in this test class.
##########
File path:
nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/ResultSetRecordSetTest.java
##########
@@ -272,6 +278,76 @@ public void
testCreateSchemaArrayThrowsNotSupportedException() throws SQLExcepti
assertEquals(RecordFieldType.ARRAY.getArrayDataType(RecordFieldType.STRING.getDataType()),
resultSchema.getField(0).getDataType());
}
+ @Test
+ public void testCreateSchemaWithLogicalTypes() throws SQLException {
+ testCreateSchemaLogicalTypes(true);
+ }
+
+ @Test
+ public void testCreateSchemaNoLogicalTypes() throws SQLException {
+ testCreateSchemaLogicalTypes(false);
+ }
+
+ private void testCreateSchemaLogicalTypes(boolean useLogicalTypes) throws
SQLException {
+ // GIVEN
+ Object[][] columns = new Object[][] {
+ {1, COLUMN_NAME_DATE, Types.DATE,
RecordFieldType.DATE.getDataType()},
+ {2, "time", Types.TIME, RecordFieldType.TIME.getDataType()},
+ {3, "time_with_timezone", Types.TIME_WITH_TIMEZONE,
RecordFieldType.TIME.getDataType()},
+ {4, "timestamp", Types.TIMESTAMP,
RecordFieldType.TIMESTAMP.getDataType()},
+ {5, "timestamp_with_timezone", Types.TIMESTAMP_WITH_TIMEZONE,
RecordFieldType.TIMESTAMP.getDataType()},
+ {6, COLUMN_NAME_BIG_DECIMAL_1,
Types.DECIMAL,RecordFieldType.DECIMAL.getDecimalDataType(7, 3)},
+ {7, COLUMN_NAME_BIG_DECIMAL_2, Types.NUMERIC,
RecordFieldType.DECIMAL.getDecimalDataType(4, 0)},
+ {8, COLUMN_NAME_BIG_DECIMAL_3, Types.JAVA_OBJECT,
RecordFieldType.DECIMAL.getDecimalDataType(501, 1)},
+ {9, COLUMN_NAME_BIG_DECIMAL_4, Types.DECIMAL,
RecordFieldType.DECIMAL.getDecimalDataType(10, 3)},
+ {10, COLUMN_NAME_BIG_DECIMAL_5, Types.DECIMAL,
RecordFieldType.DECIMAL.getDecimalDataType(3, 10)}
+ };
+ final RecordSchema recordSchema = givenRecordSchema(columns);
+
+ ResultSetMetaData resultSetMetaData =
Mockito.mock(ResultSetMetaData.class);
+ ResultSet resultSet = Mockito.mock(ResultSet.class);
+
+ RecordSchema expectedSchema = useLogicalTypes ?
givenRecordSchema(columns) : givenRecordSchemaWithOnlyStringType(columns);
+
+ // WHEN
+ setUpMocks(columns, resultSetMetaData, resultSet);
+
+ ResultSetRecordSet testSubject = new ResultSetRecordSet(resultSet,
recordSchema, 10,0, useLogicalTypes);
+ RecordSchema actualSchema = testSubject.getSchema();
+
+ // THEN
+ thenAllColumnDataTypesAreCorrect(columns, expectedSchema,
actualSchema);
+ }
+
+ private void setUpMocks(Object[][] columns, ResultSetMetaData
resultSetMetaData, ResultSet resultSet) throws SQLException {
+ when(resultSet.getMetaData()).thenReturn(resultSetMetaData);
+ when(resultSetMetaData.getColumnCount()).thenReturn(columns.length);
+
+ int indexOfBigDecimal = -1;
+ int index = 0;
+ for (final Object[] column : columns) {
+ when(resultSetMetaData.getColumnLabel((Integer)
column[0])).thenReturn((String) column[1]);
+ when(resultSetMetaData.getColumnName((Integer)
column[0])).thenReturn((String) column[1]);
+ when(resultSetMetaData.getColumnType((Integer)
column[0])).thenReturn((Integer) column[2]);
Review comment:
Restructuring the mocked columns would help avoid this type casting.
##########
File path:
nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/ResultSetRecordSetTest.java
##########
@@ -157,10 +142,31 @@ public void testCreateSchemaWhenOtherType() throws
SQLException {
assertEquals(RecordFieldType.DECIMAL.getDecimalDataType(30, 10),
resultSchema.getField(0).getDataType());
}
+ @Test
+ public void testCreateSchemaWhenOtherTypeAndNoLogicalTypes() throws
SQLException {
+ // given
+ final List<RecordField> fields = new ArrayList<>();
+ fields.add(new RecordField("decimal",
RecordFieldType.DECIMAL.getDecimalDataType(30, 10)));
+ fields.add(new RecordField("date",
RecordFieldType.DATE.getDataType()));
+ fields.add(new RecordField("time",
RecordFieldType.TIME.getDataType()));
+ fields.add(new RecordField("timestamp",
RecordFieldType.TIMESTAMP.getDataType()));
+ final RecordSchema recordSchema = new SimpleRecordSchema(fields);
+ final ResultSet resultSet = givenResultSetForOther(fields);
+
+ // when
+ final ResultSetRecordSet testSubject = new
ResultSetRecordSet(resultSet, recordSchema, 10, 0, false);
+ final RecordSchema resultSchema = testSubject.getSchema();
+
+ // then
+ assertEquals(RecordFieldType.STRING.getDataType(),
resultSchema.getField(0).getDataType());
Review comment:
This checks the type of the first field, but what about the other
fields? Should all fields be of type `STRING`? This should also be checked.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
