exceptionfactory commented on a change in pull request #4781:
URL: https://github.com/apache/nifi/pull/4781#discussion_r564073721
##########
File path:
nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java
##########
@@ -1085,6 +1087,28 @@ private static Object toEnum(Object value, EnumDataType
dataType, String fieldNa
throw new IllegalTypeConversionException("Cannot convert value [" +
value + "] of type " + value.getClass() + " to Date for field " + fieldName);
}
+ /**
+ * Converts a java.sql.Date object with 00:00:00 in local time zone
(typically coming from a java.sql.ResultSet)
+ * to UTC normalized form (storing epoch corresponding to UTC 00:00:00 on
the given day).
+ *
+ * @param date java.sql.Date with local time zone 00:00:00
+ * @return java.sql.Date with UTC 00:00:00
+ */
+ public static Date convertDateToUTC(Date date) {
+ return new
Date(date.toLocalDate().atStartOfDay(ZoneId.of("UTC")).toInstant().toEpochMilli());
Review comment:
Can the `ZoneId.of("UTC")` reference can be replaced with
`ZoneOffset.UTC`?
##########
File path:
nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/ResultSetRecordSet.java
##########
@@ -26,12 +27,12 @@
import java.math.BigDecimal;
import java.math.BigInteger;
import java.sql.Array;
+import java.sql.Date;
Review comment:
Changing `java.util.Date` to `java.sql.Date` changes the value of
`DATE_CLASS_NAME`, does that have any other implications that should be
considered?
##########
File path:
nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java
##########
@@ -1085,6 +1087,28 @@ private static Object toEnum(Object value, EnumDataType
dataType, String fieldNa
throw new IllegalTypeConversionException("Cannot convert value [" +
value + "] of type " + value.getClass() + " to Date for field " + fieldName);
}
+ /**
+ * Converts a java.sql.Date object with 00:00:00 in local time zone
(typically coming from a java.sql.ResultSet)
+ * to UTC normalized form (storing epoch corresponding to UTC 00:00:00 on
the given day).
+ *
+ * @param date java.sql.Date with local time zone 00:00:00
+ * @return java.sql.Date with UTC 00:00:00
+ */
+ public static Date convertDateToUTC(Date date) {
+ return new
Date(date.toLocalDate().atStartOfDay(ZoneId.of("UTC")).toInstant().toEpochMilli());
+ }
+
+ /**
+ * Converts a java.sql.Date object with 00:00:00 in UTC
+ * to local time zone normalized form (storing epoch corresponding to
00:00:00 in local time zone on the given day).
+ *
+ * @param date java.sql.Date with UTC 00:00:00
+ * @return java.sql.Date with local time zone 00:00:00
+ */
+ public static Date convertDateToLocalTZ(Date date) {
+ return
Date.valueOf(Instant.ofEpochMilli(date.getTime()).atZone(ZoneId.of("UTC")).toLocalDate());
Review comment:
The method naming and implementation are a little confusing on initial
read. The `atZone()` method returns a `ZonedDateTime` and `toLocalDate()`
return the `java.time.LocalDate` portion, but does that actually convert the
date to the system default time zone? It may be helpful to break this into
multiple lines as it is a bit hard to follow.
##########
File path:
nifi-nar-bundles/nifi-extension-utils/nifi-database-utils/src/test/java/org/apache/nifi/util/db/TestJdbcCommon.java
##########
@@ -679,11 +680,12 @@ public void
testConvertToAvroStreamForDateTimeAsLogicalType() throws SQLExceptio
testConvertToAvroStreamForDateTime(options,
(record, date) -> {
+ java.sql.Date expected =
DataTypeUtils.convertDateToUTC(date);
final int daysSinceEpoch = (int) record.get("date");
final long millisSinceEpoch =
TimeUnit.MILLISECONDS.convert(daysSinceEpoch, TimeUnit.DAYS);
- java.sql.Date actual =
java.sql.Date.valueOf(Instant.ofEpochMilli(millisSinceEpoch).atZone(ZoneOffset.UTC).toLocalDate());
- LOGGER.debug("comparing dates, expecting '{}', actual
'{}'", date, actual);
- assertEquals(date, actual);
+ java.sql.Date actual = new java.sql.Date(millisSinceEpoch);
+ LOGGER.debug("comparing dates, expecting '{}', actual
'{}'", expected, actual);
+ assertEquals(expected, actual);
Review comment:
Instead of performing multiple conversions for the test, could this
comparison be simplified by comparing `daysSinceEpoch` to the actual number of
expected days? That would make the assert much clearer.
----------------------------------------------------------------
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]
