You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/07/11 04:34:31 UTC
[GitHub] [nifi] exceptionfactory opened a new pull request #5210: NIFI-8768 Added toLocalDate() for convertType() handling of DATE fields
exceptionfactory opened a new pull request #5210:
URL: https://github.com/apache/nifi/pull/5210
#### Description of PR
NIFI-8768 Adds `DataTypeUtils.toLocalDate()` and updates `DataTypeUtils.convertType()` to use a new `convertTypeToDate()` method for converting input values to `java.sql.Date` when processing fields of `RecordFieldType.DATE`. The purpose of these changes is to handle year-month-day values using local system default time zone settings instead of performing implicit conversion in some scenarios. These changes address latent issues when converting strings to `java.sql.Date` objects on systems with a time zone other than Universal Coordinated Time.
Processors such as `PutKudu`, `PutDatabaseRecord`, and `PutElasticsearchHttpRecord` leverage Record Readers to convert information from original formats to records that NiFi uses for persisting to the destination system. For example, when configured with a `JsonTreeReader`, these processors can take a year-month-day string such as `2000-01-01` and store it using the logical `DATE` type of the destination system. When running on systems with a time zone other than UTC, these processors could result in storing a logical year-month-day of `1999-12-31` when receiving a record with a string field value of `2000-01-01`. Pull Request #4781 for NIFI-8023 addressed this problem for `PutDatabaseRecord` using a workaround conversion method `DataTypeUtils.convertDateToLocalTZ()`. The issue was still present in `PutKudu` due to the behavior of Record Readers such as `JsonTreeReader` leveraging the String to `java.sql.Date` conversion methods in `DataTypeUtils`.
The combination of an implicit default Time Zone of GMT in `DataTypeUtils.getDateFormat()` and the absence of a declared time zone in year-month-day strings such as `2000-01-01` results in unexpected date changes when running in time zones other than UTC. This Pull Request addresses the problem by handling `RecordFieldType.DATE` and `java.sql.Types.DATE` according to the system default time zone, as implied by the use of `java.time.LocalDate` for parsing and conversion to `java.sql.Date`. This approach removes the need for the custom `DataTypeUtils.convertDateToLocalTZ()` method in `PutDatabaseRecord`. This change applies only to `DATE` fields does not involve `TIME` or `TIMESTAMP` fields.
Both `java.time.LocalDate` and `java.sql.Date` represent a year-month-day combination without reference to a time zone, thus converting to UTC can change the day by shifting the number of hours forward or backward from the original year-month-day in the local system time zone. This Pull Request includes several additional unit test methods for both `DataTypeUtils` and referencing processors to describe and confirm expected behavior. Introduction of `DataTypeUtils.toLocalDate()` and `DataTypeUtils.convertTypeToDate()` provide methods to handle existing classes that expect `java.sql.Date` while also supporting classes that can leverage the newer `java.time.LocalDate`.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [X] 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.
- [X] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [X] 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:
- [X] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [X] Have you written or updated unit tests to verify your changes?
- [X] Have you verified that the full build is successful on JDK 8?
- [X] 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](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] 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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] gresockj commented on a change in pull request #5210: NIFI-8768 Added toLocalDate() for convertType() handling of DATE fields
Posted by GitBox <gi...@apache.org>.
gresockj commented on a change in pull request #5210:
URL: https://github.com/apache/nifi/pull/5210#discussion_r668903660
##########
File path: nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/TestDataTypeUtils.java
##########
@@ -923,23 +939,73 @@ public void testConvertDateToUTC() {
assertEquals(0, zdt.getNano());
}
+ /**
+ * Convert String to java.sql.Date using implicit default DateFormat with GMT Time Zone
+ *
+ * Running this method on a system with a time zone other than GMT should return the same year-month-day
+ */
@Test
- public void testConvertDateToLocalTZ() {
- int year = 2021;
- int month = 1;
- int dayOfMonth = 25;
+ public void testConvertTypeStringToDateDefaultTimeZoneFormat() {
Review comment:
Good to know, that makes me feel better.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] gresockj commented on a change in pull request #5210: NIFI-8768 Added toLocalDate() for convertType() handling of DATE fields
Posted by GitBox <gi...@apache.org>.
gresockj commented on a change in pull request #5210:
URL: https://github.com/apache/nifi/pull/5210#discussion_r668885621
##########
File path: nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/TestDataTypeUtils.java
##########
@@ -923,23 +939,73 @@ public void testConvertDateToUTC() {
assertEquals(0, zdt.getNano());
}
+ /**
+ * Convert String to java.sql.Date using implicit default DateFormat with GMT Time Zone
+ *
+ * Running this method on a system with a time zone other than GMT should return the same year-month-day
+ */
@Test
- public void testConvertDateToLocalTZ() {
- int year = 2021;
- int month = 1;
- int dayOfMonth = 25;
+ public void testConvertTypeStringToDateDefaultTimeZoneFormat() {
Review comment:
How do you feel about setting the the user.timezone system property in some of these tests to show it working regardless of the user's time zone? You'd have to reset the property after the test, of course. The only thing I'd be concerned about is a multi-threaded build, but I wanted to see what you thought.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mattyb149 commented on pull request #5210: NIFI-8768 Added toLocalDate() for convertType() handling of DATE fields
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on pull request #5210:
URL: https://github.com/apache/nifi/pull/5210#issuecomment-879374018
+1 LGTM, thanks @gresockj for the review! I tried PutDatabaseRecord and PutElasticsearchRecord and verified the date/time values were as expected. Thanks for the fix! Merging to main
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mattyb149 commented on pull request #5210: NIFI-8768 Added toLocalDate() for convertType() handling of DATE fields
Posted by GitBox <gi...@apache.org>.
mattyb149 commented on pull request #5210:
URL: https://github.com/apache/nifi/pull/5210#issuecomment-879274193
Reviewing...
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] mattyb149 closed pull request #5210: NIFI-8768 Added toLocalDate() for convertType() handling of DATE fields
Posted by GitBox <gi...@apache.org>.
mattyb149 closed pull request #5210:
URL: https://github.com/apache/nifi/pull/5210
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5210: NIFI-8768 Added toLocalDate() for convertType() handling of DATE fields
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5210:
URL: https://github.com/apache/nifi/pull/5210#discussion_r668902144
##########
File path: nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/TestDataTypeUtils.java
##########
@@ -923,23 +939,73 @@ public void testConvertDateToUTC() {
assertEquals(0, zdt.getNano());
}
+ /**
+ * Convert String to java.sql.Date using implicit default DateFormat with GMT Time Zone
+ *
+ * Running this method on a system with a time zone other than GMT should return the same year-month-day
+ */
@Test
- public void testConvertDateToLocalTZ() {
- int year = 2021;
- int month = 1;
- int dayOfMonth = 25;
+ public void testConvertTypeStringToDateDefaultTimeZoneFormat() {
Review comment:
I thought about that approach, and there is one existing unit test method that works as you described. However, since the GitHub automated builds run with three different time zone settings, that seems to be a better way to cover a variety of system configurations.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org