NIFI-8023: Convert java.sql.Date between UTC/local time zone normaliz… #4781

turcsanyip · 2021-01-25T21:44:50Z

…ed forms before/after database operations

https://issues.apache.org/jira/browse/NIFI-8023?focusedCommentId=17271710&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17271710

Thank you for submitting a contribution to Apache NiFi.

Please provide a short description of the PR here:

Description of PR

Enables X functionality; fixes bug NIFI-YYYY.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
Have you written or updated unit tests to verify your changes?
Have you verified that the full build is successful on JDK 8?
Have you verified that the full build is successful on JDK 11?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.

…ed forms before/after database operations

exceptionfactory

Thanks for the PR @turcsanyip. This looks like a more straightforward solution than previous pull requests. See individual comments for a couple questions and recommendations on streamlining the UTC references.

exceptionfactory · 2021-01-25T22:00:48Z

...mmons/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;


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?

It was intentional because it seemed to be a bug. ResultSet.getMetaData().getColumnClassName() would never return java.util.Date but the SQL date/time types.
However, now I see that this code section is related to QueryRecord and it may use java.util.Date. I'll revert it back.
Thanks for the heads up that made me double check it in more detail.

Thanks for responding regarding the column class name handling. I will take another look once you push the next set of updates.

exceptionfactory · 2021-01-25T22:01:53Z

...mmons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java

+     * @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());


Can the ZoneId.of("UTC") reference can be replaced with ZoneOffset.UTC?

atStartOfDay() has ZoneId parameter only.
In my understanding ZoneOffset is DST-agnostic, so the ZoneId is the right choice in any case.

Reading the documentation, ZoneOffset extends ZoneId so would ZoneOffset.UTC function the same as ZoneId.of("UTC")?

You are right. I did not notice that ZoneOffset extends ZoneId. Just focused on that ZoneId references an offset and the DST info.
As there is no DST in UTC, it is a simple offset so ZoneOffset.UTC will be fine. Thanks!

exceptionfactory · 2021-01-25T22:08:47Z

...xtension-utils/nifi-database-utils/src/test/java/org/apache/nifi/util/db/TestJdbcCommon.java

-                    assertEquals(date, actual);
+                    java.sql.Date actual = new java.sql.Date(millisSinceEpoch);
+                    LOGGER.debug("comparing dates, expecting '{}', actual '{}'", expected, actual);
+                    assertEquals(expected, actual);


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.

These tests (historically) do not assert the raw value (int / long) from the Avro record but reconstruct the Date/Time/Timestamp objects and compare them with the original objects (which were the inputs of the test cases).
I believe the reason was to avoid to calculate the days/millis/etc. from the test input data which would be a similarly complicated method.

As the test inputs are literals, it is not needed to calculate the expected value from code, but it can be determined once and hard coded as literals too. What do you think?
I mean:

int expectedDaysSinceEpoch = 17444; assertDate.accept(record, expectedDaysSinceEpoch);

The code snippet you suggested looks much clearer and avoids the conversion, thanks!

I could not go on that way because testConvertToAvroStreamForDateTime() is used in two places and the other really expects a Date object.
However, I found a simpler way to convert the Date to the expected number of days, and then to assert the raw value from Avro against that.

exceptionfactory · 2021-01-25T22:17:20Z

...mmons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java

+     * @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());


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.

Indeed... what do you think about this?

public static Date convertDateToUTC(Date dateLocalTZ) { ZonedDateTime zdtLocalTZ = ZonedDateTime.ofInstant(Instant.ofEpochMilli(dateLocalTZ.getTime()), ZoneId.systemDefault()); ZonedDateTime zdtUTC = zdtLocalTZ.withZoneSameLocal(ZoneId.of("UTC")); Date dateUTC = new Date(zdtUTC.toInstant().toEpochMilli()); return dateUTC; }

No LocaDate and I believe withZoneSameLocal() in the middle of the method really grasps what is going on here.
The same can be applied to convertDateToLocalTZ() similarly.

Thanks for the updated suggestion, breaking things out read much more easily, and the withZoneSameLocal() method makes more sense now. Based on that snippet, I would suggest just returning the final Date object instead of declaring and returning.

turcsanyip · 2021-01-26T14:41:58Z

@exceptionfactory Thanks for the review, implemented your suggestions.

exceptionfactory · 2021-01-26T16:04:38Z

@turcsanyip Thanks for making the changes. I confirmed that several of the tests run in different time zones as designed. I noticed that the changes for PutDatabaseRecord and ResultSetRecordSet do not seem to be covered by unit tests. Should there be additional unit test updates to account for those changes?

exceptionfactory · 2021-01-26T19:46:17Z

@turcsanyip Thanks for adding the tests, I confirmed the changes function as designed and cover the lines changed in PutDatabaseRecord and ResultSetRecordSet. +1

…ed forms before/after database operations This closes apache#4781 Signed-off-by: David Handermann <exceptionfactory@gmail.com>

NIFI-8023: Convert java.sql.Date between UTC/local time zone normaliz…

faaaf98

…ed forms before/after database operations

exceptionfactory reviewed Jan 25, 2021

View reviewed changes

exceptionfactory mentioned this pull request Jan 25, 2021

NIFI-8023 Added toLocalDate() and updated toDate() in DataTypeUtils #4734

Closed

13 tasks

adenes mentioned this pull request Jan 26, 2021

NIFI-8023 Test failure after NIFI-7996 commit if system default TZ is… #4677

Closed

13 tasks

NIFI-8023: Review changes.

50b3b7c

NIFI-8023: Added more tests.

9a1d4aa

asfgit closed this in 67d0600 Jan 26, 2021

exceptionfactory mentioned this pull request Jul 11, 2021

NIFI-8768 Added toLocalDate() for convertType() handling of DATE fields #5210

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-8023: Convert java.sql.Date between UTC/local time zone normaliz… #4781

NIFI-8023: Convert java.sql.Date between UTC/local time zone normaliz… #4781

turcsanyip commented Jan 25, 2021 •

edited

exceptionfactory left a comment

exceptionfactory Jan 25, 2021

turcsanyip Jan 25, 2021

exceptionfactory Jan 26, 2021

exceptionfactory Jan 25, 2021

turcsanyip Jan 25, 2021

exceptionfactory Jan 26, 2021

turcsanyip Jan 26, 2021

exceptionfactory Jan 25, 2021

turcsanyip Jan 25, 2021 •

edited

exceptionfactory Jan 26, 2021

turcsanyip Jan 26, 2021

exceptionfactory Jan 25, 2021

turcsanyip Jan 26, 2021 •

edited

exceptionfactory Jan 26, 2021

turcsanyip commented Jan 26, 2021 •

edited

exceptionfactory commented Jan 26, 2021

exceptionfactory commented Jan 26, 2021

NIFI-8023: Convert java.sql.Date between UTC/local time zone normaliz… #4781

NIFI-8023: Convert java.sql.Date between UTC/local time zone normaliz… #4781

Conversation

turcsanyip commented Jan 25, 2021 • edited

Description of PR

For all changes:

For code changes:

For documentation related changes:

Note:

exceptionfactory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turcsanyip Jan 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turcsanyip Jan 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turcsanyip commented Jan 26, 2021 • edited

exceptionfactory commented Jan 26, 2021

exceptionfactory commented Jan 26, 2021

turcsanyip commented Jan 25, 2021 •

edited

turcsanyip Jan 25, 2021 •

edited

turcsanyip Jan 26, 2021 •

edited

turcsanyip commented Jan 26, 2021 •

edited