[GitHub] [nifi] adenes commented on a change in pull request #4463: NIFI-7714: QueryCassandra loses precision when converting timestamps to JSON

[GitBox <gi...@apache.org>]

adenes commented on a change in pull request #4463:
URL: https://github.com/apache/nifi/pull/4463#discussion_r468642036



##########
File path: nifi-nar-bundles/nifi-cassandra-bundle/nifi-cassandra-processors/src/main/java/org/apache/nifi/processors/cassandra/QueryCassandra.java
##########
@@ -130,6 +132,23 @@
             .defaultValue(AVRO_FORMAT)
             .build();
 
+    public static final PropertyDescriptor DATE_FORMAT_PATTERN = new PropertyDescriptor.Builder()

Review comment:
       Thanks @mattyb149 for the review.
   I have updated the name/displayname according to your suggestion.
   I agree that the time/date handling should be improved, currently the CQL `timestamp` works correctly, but I'm not sure about the `date` and `time` types, to be honest. The aim of this PR was to add the ability to change the timestamp's formatting in JSON output as with the current formatting it discards the milliseconds, but I did not want to change anything else.
   I'll file a jira for this.

##########
File path: nifi-nar-bundles/nifi-cassandra-bundle/nifi-cassandra-processors/src/test/java/org/apache/nifi/processors/cassandra/QueryCassandraTest.java
##########
@@ -368,6 +380,42 @@ public void testConvertToJSONStream() throws Exception {
         assertEquals(2, numberOfRows);
     }
 
+    @Test
+    public void testDefaultDateFormatInConvertToJSONStream() throws Exception {
+        ResultSet rs = CassandraQueryTestUtil.createMockDateResultSet();
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+
+        DateFormat df = new SimpleDateFormat(QueryCassandra.DATE_FORMAT_PATTERN.getDefaultValue());
+        df.setTimeZone(TimeZone.getTimeZone("UTC"));
+
+        long numberOfRows = QueryCassandra.convertToJsonStream(Optional.of(testRunner.getProcessContext()), rs, baos,
+            StandardCharsets.UTF_8, 0, null);
+        assertEquals(1, numberOfRows);
+
+        Map<String, List<Map<String, String>>> map = new ObjectMapper().readValue(baos.toByteArray(), HashMap.class);
+        String date = map.get("results").get(0).get("date");
+        assertEquals(df.format(CassandraQueryTestUtil.TEST_DATE), date);
+    }
+
+    @Test
+    public void testCustomDateFormatInConvertToJSONStream() throws Exception {
+        MockProcessContext context = (MockProcessContext) testRunner.getProcessContext();
+        ResultSet rs = CassandraQueryTestUtil.createMockDateResultSet();
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+
+        final String customDateFormat = "yyyy-MM-dd HH:mm:ss.SSSZ";

Review comment:
       The current implementation formats the date with UTC timezone, it's hardcoded. I have updated the test though, to use a PST date/time as input and validate that it's properly formatted with UTC tz.

##########
File path: nifi-nar-bundles/nifi-cassandra-bundle/nifi-cassandra-processors/src/main/java/org/apache/nifi/processors/cassandra/QueryCassandra.java
##########
@@ -467,19 +494,30 @@ public static long convertToJsonStream(final ResultSet rs, final OutputStream ou
     }
 
     protected static String getJsonElement(Object value) {
+        return getJsonElement(Optional.empty(), value);
+    }
+
+    protected static String getJsonElement(final Optional<ProcessContext> context, Object value) {
         if (value instanceof Number) {
             return value.toString();
         } else if (value instanceof Date) {
-            SimpleDateFormat dateFormat = new SimpleDateFormat("yyyy-MM-dd HH:mm:ssZ");
-            dateFormat.setTimeZone(TimeZone.getTimeZone("UTC"));
-            return "\"" + dateFormat.format((Date) value) + "\"";
+            return "\"" + getFormattedDate(context, (Date) value) + "\"";
         } else if (value instanceof String) {
             return "\"" + StringEscapeUtils.escapeJson((String) value) + "\"";
         } else {
             return "\"" + value.toString() + "\"";
         }
     }
 
+    private static String getFormattedDate(final Optional<ProcessContext> context, Date value) {
+        final String dateFormatPattern = context
+                .map(_context -> _context.getProperty(DATE_FORMAT_PATTERN).getValue())
+                .orElse(DATE_FORMAT_PATTERN.getDefaultValue());
+        SimpleDateFormat dateFormat = new SimpleDateFormat(dateFormatPattern);
+        dateFormat.setTimeZone(TimeZone.getTimeZone("UTC"));

Review comment:
       The output timezone is hardcoded to UTC both in the current and in this updated implementation.
   I've added this to the newly added property's description.
   I'd take care of migrating to the `java.time` classes in a separate jira.




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