turcsanyip commented on a change in pull request #5318:
URL: https://github.com/apache/nifi/pull/5318#discussion_r722388428



##########
File path: 
nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/ResultSetRecordSet.java
##########
@@ -131,10 +131,17 @@ protected Record createRecord(final ResultSet rs) throws 
SQLException {
 
         for (final RecordField field : schema.getFields()) {
             final String fieldName = field.getFieldName();
-
-            final Object value;
+            RecordFieldType fieldType = field.getDataType().getFieldType();
+            Object value;
             if (rsColumnNames.contains(fieldName)) {
-                value = normalizeValue(rs.getObject(fieldName));
+                switch (fieldType) {
+                    case TIMESTAMP:
+                        value = rs.getTimestamp(fieldName);
+                        break;
+                    default:
+                        value = rs.getObject(fieldName);
+                }
+                value = normalizeValue(value);

Review comment:
       In my opinion, using final variables and avoiding reassignment is a good 
practice that should be followed where possible. `normalizeValue()` is already 
a method so it could simply be called twice in the two branches instead of 
reassigning the variable. Something like this:
   ```
   if (fieldType  == TIMESTAMP) {
       value = normalizeValue(rs.getTimestamp(fieldName));
   } else {
       value = normalizeValue(rs.getObject(fieldName));
   }
   ```

##########
File path: 
nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/ResultSetRecordSet.java
##########
@@ -131,10 +131,17 @@ protected Record createRecord(final ResultSet rs) throws 
SQLException {
 
         for (final RecordField field : schema.getFields()) {
             final String fieldName = field.getFieldName();
-
-            final Object value;
+            RecordFieldType fieldType = field.getDataType().getFieldType();
+            Object value;
             if (rsColumnNames.contains(fieldName)) {
-                value = normalizeValue(rs.getObject(fieldName));
+                switch (fieldType) {
+                    case TIMESTAMP:
+                        value = rs.getTimestamp(fieldName);
+                        break;
+                    default:
+                        value = rs.getObject(fieldName);

Review comment:
       I think `if-else` would be a better option than the single-case `switch` 
(IntelliJ also marks it as a possible improvement).




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


Reply via email to