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/19 20:15:11 UTC
[GitHub] [nifi] juldrixx opened a new pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
juldrixx opened a new pull request #5014:
URL: https://github.com/apache/nifi/pull/5014
<!--
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
#### Description of PR
Add the conversion of Java.sql.Timestamp, Java.sql.Time and Java.sql.Date type to String to be compatible with BigQuery.
Related Ticket : [NIFI-8442](https://issues.apache.org/jira/browse/NIFI-8442)
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] juldrixx commented on pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx commented on pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#issuecomment-882829047
Re-opening after a mistake here [#5231](https://github.com/apache/nifi/pull/5231)
--
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] juldrixx commented on pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx commented on pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#issuecomment-885078061
Thanks to both of you for your feedbacks.
--
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] pvillard31 commented on pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#issuecomment-885075043
This is now looking good, integration tests are green. Will wait for the checks to complete and merge. Thanks @juldrixx.
<img width="843" alt="Screenshot 2021-07-22 at 19 06 38" src="https://user-images.githubusercontent.com/11541012/126680241-60bcc771-4c8f-4f67-9610-335b5768c9b8.png">
--
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] juldrixx commented on a change in pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx commented on a change in pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#discussion_r672878946
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +195,16 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Timestamp) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timestampFormatter));
+ } else if (obj instanceof Time) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Time) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timeFormatter = DateTimeFormatter.ofPattern("HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timeFormatter) );
Review comment:
Hi, I have already tried this method but it is dependent on the timezone in which the code is running for my part instead of having 12:35:24, it shows me 14:35:24.
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +195,16 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Timestamp) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timestampFormatter));
Review comment:
Same reason as with Time, it is timezone dependent and comes out 2021-07-18 14:35:24.
--
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] juldrixx commented on a change in pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx commented on a change in pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#discussion_r673151339
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +195,16 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Timestamp) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timestampFormatter));
+ } else if (obj instanceof Time) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Time) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timeFormatter = DateTimeFormatter.ofPattern("HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timeFormatter) );
Review comment:
Done, thanks for the explanation and the advice.
--
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 #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#discussion_r672623248
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -18,10 +18,14 @@
package org.apache.nifi.processors.gcp.bigquery;
import java.io.InputStream;
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.sql.Date;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.time.Instant;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.util.*;
Review comment:
Asterisk imports should be replaced with explicit classes.
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +195,16 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Timestamp) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timestampFormatter));
+ } else if (obj instanceof Time) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Time) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timeFormatter = DateTimeFormatter.ofPattern("HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timeFormatter) );
Review comment:
The [Time type](https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#time_type) definition indicates that the value is "independent of a specific date and timezone", so converting the value with `ZoneOffset.UTC` does not seem to match the type definition. The [java.time.LocalTime.toString()](https://docs.oracle.com/javase/8/docs/api/java/time/LocalTime.html#toString) method supports string formatting based on the precision of the original value, so it looks like this could be simplified to using `java.sql.Time.toLocalTime().toString()`. That approach would also avoid the time zone conversion.
```suggestion
final LocalTime localTime = ((Time) obj).toLocalTime();
result.put(key, localTime.toString());
```
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +195,16 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Timestamp) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timestampFormatter));
Review comment:
Similar to the `java.sql.Date` and `java.sql.Time` fields, `java.sql.Timestamp` does not necessarily indicate a particular time zone. In this case, the [Datetime type](https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#datetime_type) seems to be a closer approximation of `java.sql.Timestamp`. Converting `java.sql.Timestamp` to `java.time.LocalDateTime` would allow using the `toString()` method for formatting while also avoiding the time zone conversion.
```suggestion
final LocalDateTime localDateTime = ((Timestamp) obj).toLocalDateTime();
result.put(key, localDateTime.toString());
```
--
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] juldrixx commented on a change in pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx commented on a change in pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#discussion_r672878946
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +195,16 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Timestamp) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timestampFormatter));
+ } else if (obj instanceof Time) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Time) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timeFormatter = DateTimeFormatter.ofPattern("HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timeFormatter) );
Review comment:
Hi, I have already tried this method but it is dependent on the timezone in which the code is running for my part instead of having 12:35:24, it shows me 14:35:24.
--
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] juldrixx commented on a change in pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx commented on a change in pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#discussion_r672880598
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +195,16 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Timestamp) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timestampFormatter));
Review comment:
Same reason as with Time, it is timezone dependent and comes out 2021-07-18 14:35:24.
--
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 #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#discussion_r672623248
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -18,10 +18,14 @@
package org.apache.nifi.processors.gcp.bigquery;
import java.io.InputStream;
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.sql.Date;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.time.Instant;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.util.*;
Review comment:
Asterisk imports should be replaced with explicit classes.
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +195,16 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Timestamp) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timestampFormatter));
+ } else if (obj instanceof Time) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Time) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timeFormatter = DateTimeFormatter.ofPattern("HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timeFormatter) );
Review comment:
The [Time type](https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#time_type) definition indicates that the value is "independent of a specific date and timezone", so converting the value with `ZoneOffset.UTC` does not seem to match the type definition. The [java.time.LocalTime.toString()](https://docs.oracle.com/javase/8/docs/api/java/time/LocalTime.html#toString) method supports string formatting based on the precision of the original value, so it looks like this could be simplified to using `java.sql.Time.toLocalTime().toString()`. That approach would also avoid the time zone conversion.
```suggestion
final LocalTime localTime = ((Time) obj).toLocalTime();
result.put(key, localTime.toString());
```
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +195,16 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Timestamp) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timestampFormatter));
Review comment:
Similar to the `java.sql.Date` and `java.sql.Time` fields, `java.sql.Timestamp` does not necessarily indicate a particular time zone. In this case, the [Datetime type](https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#datetime_type) seems to be a closer approximation of `java.sql.Timestamp`. Converting `java.sql.Timestamp` to `java.time.LocalDateTime` would allow using the `toString()` method for formatting while also avoiding the time zone conversion.
```suggestion
final LocalDateTime localDateTime = ((Timestamp) obj).toLocalDateTime();
result.put(key, localDateTime.toString());
```
--
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] juldrixx closed pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx closed pull request #5014:
URL: https://github.com/apache/nifi/pull/5014
--
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] juldrixx commented on pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx commented on pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#issuecomment-882829047
Re-opening after a mistake here [#5231](https://github.com/apache/nifi/pull/5231)
--
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] pvillard31 commented on pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#issuecomment-882379208
@juldrixx - can you rebase your pull request to make sure we only see your commits?
--
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 #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#discussion_r673102933
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +195,16 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Timestamp) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timestampFormatter));
+ } else if (obj instanceof Time) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Time) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timeFormatter = DateTimeFormatter.ofPattern("HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timeFormatter) );
Review comment:
Thanks for the reply, I understand the problem. The `JsonTreeReader` and other Record Reader implementations perform implicit conversion from the local system time zone to the GMT time zone when parsing a string to a Timestamp or Time object. This unfortunately impacts a number of processors, and as you have found in this class, it ends up requiring setting `ZoneOffset.UTC` when converting back from a Timestamp or Time to String.
I addressed a similar problem with Date field handling in #5210, so that's why `java.sql.Date.toString()` works in this class.
Addressing the larger issue with Time and Timestamp in Record Readers needs to be addressed elsewhere, so for that reason, maintaining the approach you have implemented seems reasonable for now.
With that background, I would recommend keeping the `DateTimeFormatter`, but declaring it as a static class variable. Unlike `java.text.SimpleDateFormat`, `java.time.DateTimeFormatter` is thread-safe, and can be reused, so making it a static class variable would avoid unnecessary object creation when handling each record.
It might also be helpful to include a short comment that the ZoneOffset.UTC time zone is necessary due to implicit time zone conversion in Record Readers. At least that will be a pointer to address it when evaluating the underlying issue.
--
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] pvillard31 commented on a change in pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on a change in pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#discussion_r671573920
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -18,10 +18,11 @@
package org.apache.nifi.processors.gcp.bigquery;
import java.io.InputStream;
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.sql.Date;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.text.SimpleDateFormat;
+import java.util.*;
Review comment:
```suggestion
import java.sql.Date;
import java.sql.Time;
import java.sql.Timestamp;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.TimeZone;
```
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +192,18 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ SimpleDateFormat format = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
+ format.setTimeZone(TimeZone.getTimeZone("UTC"));
+ result.put(key, format.format(((Timestamp) obj).getTime()));
+ } else if (obj instanceof Time) {
+ SimpleDateFormat format = new SimpleDateFormat("HH:mm:ss");
+ format.setTimeZone(TimeZone.getTimeZone("UTC"));
+ result.put(key, format.format(((Time) obj).getTime()));
+ } else if (obj instanceof Date) {
+ SimpleDateFormat format = new SimpleDateFormat("yyyy-MM-dd");
+ format.setTimeZone(TimeZone.getTimeZone("UTC"));
+ result.put(key, format.format(((Date) obj).getTime()));
Review comment:
When executing the integration test you added, I see the below data being added in BQ:
<img width="858" alt="Screenshot 2021-07-17 at 02 01 35" src="https://user-images.githubusercontent.com/11541012/126018935-4da2b128-6c1a-4538-a5a2-4dd1a8e6a490.png">
Which makes the test fail because:
````java
assertEquals(john.get("birth").getRecordValue().get(0).getStringValue(), "1995-06-01");
assertEquals(john.get("birth").getRecordValue().get(1).getStringValue(), "15:57:27");
assertEquals(john.get("birth").getRecordValue().get(2).getTimestampValue() / 1000, 802022247000L);
````
Something needs to be fixed.
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/test/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreamingIT.java
##########
@@ -21,12 +21,15 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
+import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.Iterator;
+import com.google.api.client.util.DateTime;
Review comment:
```suggestion
```
--
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] juldrixx removed a comment on pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx removed a comment on pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#issuecomment-882829047
Re-opening after a mistake here [#5231](https://github.com/apache/nifi/pull/5231)
--
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] asfgit closed pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #5014:
URL: https://github.com/apache/nifi/pull/5014
--
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] juldrixx commented on pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx commented on pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#issuecomment-882829047
Re-opening after a mistake here [#5231](https://github.com/apache/nifi/pull/5231)
--
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] juldrixx commented on pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx commented on pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#issuecomment-882049231
I added a new test and how the dates, times and timestamps were formatted. I tried my tests with different locations for the BigQuery dataset/table and changing the JVM timezone, and the tests pass.
--
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 #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#discussion_r672623248
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -18,10 +18,14 @@
package org.apache.nifi.processors.gcp.bigquery;
import java.io.InputStream;
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.sql.Date;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.time.Instant;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
+import java.util.*;
Review comment:
Asterisk imports should be replaced with explicit classes.
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +195,16 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Timestamp) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timestampFormatter));
+ } else if (obj instanceof Time) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Time) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timeFormatter = DateTimeFormatter.ofPattern("HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timeFormatter) );
Review comment:
The [Time type](https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#time_type) definition indicates that the value is "independent of a specific date and timezone", so converting the value with `ZoneOffset.UTC` does not seem to match the type definition. The [java.time.LocalTime.toString()](https://docs.oracle.com/javase/8/docs/api/java/time/LocalTime.html#toString) method supports string formatting based on the precision of the original value, so it looks like this could be simplified to using `java.sql.Time.toLocalTime().toString()`. That approach would also avoid the time zone conversion.
```suggestion
final LocalTime localTime = ((Time) obj).toLocalTime();
result.put(key, localTime.toString());
```
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +195,16 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Timestamp) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timestampFormatter));
Review comment:
Similar to the `java.sql.Date` and `java.sql.Time` fields, `java.sql.Timestamp` does not necessarily indicate a particular time zone. In this case, the [Datetime type](https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#datetime_type) seems to be a closer approximation of `java.sql.Timestamp`. Converting `java.sql.Timestamp` to `java.time.LocalDateTime` would allow using the `toString()` method for formatting while also avoiding the time zone conversion.
```suggestion
final LocalDateTime localDateTime = ((Timestamp) obj).toLocalDateTime();
result.put(key, localDateTime.toString());
```
--
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] juldrixx commented on pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx commented on pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#issuecomment-849640020
Any news ?
--
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:
users@infra.apache.org
[GitHub] [nifi] juldrixx commented on a change in pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx commented on a change in pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#discussion_r672878946
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +195,16 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Timestamp) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timestampFormatter));
+ } else if (obj instanceof Time) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Time) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timeFormatter = DateTimeFormatter.ofPattern("HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timeFormatter) );
Review comment:
Hi, I have already tried this method but it is dependent on the timezone in which the code is running for my part instead of having 12:35:24, it shows me 14:35:24.
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +195,16 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ LocalDateTime dateTime = LocalDateTime.ofInstant(Instant.ofEpochMilli(((Timestamp) obj).getTime()), ZoneOffset.UTC);
+ DateTimeFormatter timestampFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSSSSS");
+ result.put(key, dateTime.format(timestampFormatter));
Review comment:
Same reason as with Time, it is timezone dependent and comes out 2021-07-18 14:35:24.
--
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 #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#discussion_r671582027
##########
File path: nifi-nar-bundles/nifi-gcp-bundle/nifi-gcp-processors/src/main/java/org/apache/nifi/processors/gcp/bigquery/PutBigQueryStreaming.java
##########
@@ -191,6 +192,18 @@ public void onTrigger(ProcessContext context, ProcessSession session) throws Pro
lmapr.add(convertMapRecord(((MapRecord) mapr).toMap()));
}
result.put(key, lmapr);
+ } else if (obj instanceof Timestamp) {
+ SimpleDateFormat format = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
+ format.setTimeZone(TimeZone.getTimeZone("UTC"));
+ result.put(key, format.format(((Timestamp) obj).getTime()));
+ } else if (obj instanceof Time) {
+ SimpleDateFormat format = new SimpleDateFormat("HH:mm:ss");
+ format.setTimeZone(TimeZone.getTimeZone("UTC"));
+ result.put(key, format.format(((Time) obj).getTime()));
+ } else if (obj instanceof Date) {
+ SimpleDateFormat format = new SimpleDateFormat("yyyy-MM-dd");
+ format.setTimeZone(TimeZone.getTimeZone("UTC"));
Review comment:
Setting the Time Zone to `UTC` can lead to unexpected results based on the system default time zone.
--
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] juldrixx removed a comment on pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx removed a comment on pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#issuecomment-882829047
Re-opening after a mistake here [#5231](https://github.com/apache/nifi/pull/5231)
--
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] juldrixx removed a comment on pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx removed a comment on pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#issuecomment-882829047
Re-opening after a mistake here [#5231](https://github.com/apache/nifi/pull/5231)
--
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] pvillard31 commented on pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #5014:
URL: https://github.com/apache/nifi/pull/5014#issuecomment-882379208
@juldrixx - can you rebase your pull request to make sure we only see your commits?
--
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] juldrixx closed pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx closed pull request #5014:
URL: https://github.com/apache/nifi/pull/5014
--
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] juldrixx closed pull request #5014: NIFI-8442 Convert Timestamp, Date and Time to String
Posted by GitBox <gi...@apache.org>.
juldrixx closed pull request #5014:
URL: https://github.com/apache/nifi/pull/5014
--
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