exceptionfactory commented on code in PR #9196: URL: https://github.com/apache/nifi/pull/9196#discussion_r1758831115
########## nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/FractionalSecondsUtils.java: ########## @@ -0,0 +1,57 @@ +/* + * 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. + */ +package org.apache.nifi.serialization.record.util; + +import java.time.Instant; + +public class FractionalSecondsUtils { + private static final long YEAR_TEN_THOUSAND = 253_402_300_800_000L; + + private FractionalSecondsUtils() { + } + + public static Instant tryParseAsNumber(final String value) { Review Comment: Recommend renaming this method to `toInstant()` as well, following the pattern of the other methods. ########## nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/field/ObjectTimestampFieldConverter.java: ########## @@ -39,7 +57,80 @@ class ObjectTimestampFieldConverter implements FieldConverter<Object, Timestamp> */ @Override public Timestamp convertField(final Object field, final Optional<String> pattern, final String name) { Review Comment: The multiple `return` statements in this method make it much more difficult to follow. Refactoring to have a single return would be helpful. ########## nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/field/ObjectTimestampFieldConverter.java: ########## @@ -39,7 +57,80 @@ class ObjectTimestampFieldConverter implements FieldConverter<Object, Timestamp> */ @Override public Timestamp convertField(final Object field, final Optional<String> pattern, final String name) { - final LocalDateTime localDateTime = CONVERTER.convertField(field, pattern, name); - return localDateTime == null ? null : Timestamp.valueOf(localDateTime); + Instant instant = null; + switch (field) { + case null -> { + return null; + } + case Timestamp timestamp -> { + return timestamp; + } + case ZonedDateTime zonedDateTime -> { + instant = zonedDateTime.toInstant(); + } + case Time time -> { + // Convert to an Instant object preserving millisecond precision + final long epochMilli = time.getTime(); + instant = Instant.ofEpochMilli(epochMilli); + } + case Date date -> { + final long epochMilli = date.getTime(); + instant = Instant.ofEpochMilli(epochMilli); + } + case Number number -> { + switch (field) { + case Double d -> instant = FractionalSecondsUtils.toInstant(d); + case Float f -> instant = FractionalSecondsUtils.toInstant(f.doubleValue()); + default -> instant = FractionalSecondsUtils.toInstant(number.longValue()); + } + } + case String string -> { + final String stringTrimmed = string.trim(); + if (stringTrimmed.isEmpty()) { + return null; + } + + if (pattern.isPresent()) { + final String patternString = pattern.get(); + final DateTimeFormatter formatter = DateTimeFormatterRegistry.getDateTimeFormatter(patternString); + try { + // NOTE: In order to calculate any possible timezone offsets, the string must be parsed as a ZoneDateTime. + // It is not possible to always parse as a ZoneDateTime as it will fail if the pattern has + // no timezone information. Hence, a regular expression is used to determine whether it is necessary + // to parse with ZoneDateTime or not. + final Matcher matcher = TIMEZONE_PATTERN.matcher(patternString); + final ZonedDateTime zonedDateTime; + + if (matcher.find()) { + zonedDateTime = ZonedDateTime.parse(stringTrimmed, formatter); + } else { + final LocalDateTime localDateTime = LocalDateTime.parse(stringTrimmed, formatter); + zonedDateTime = ZonedDateTime.of(localDateTime, ZoneId.systemDefault()); Review Comment: This intermediate conversion to `ZonedDateTime` and then `Instant` is not necessary when the destination type is `Timestamp`. It seems like the general approach should be refactored to set a Timestamp value instead of using `Instant` as the intermediate type. ########## nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/field/ObjectTimestampFieldConverter.java: ########## @@ -39,7 +57,80 @@ class ObjectTimestampFieldConverter implements FieldConverter<Object, Timestamp> */ @Override public Timestamp convertField(final Object field, final Optional<String> pattern, final String name) { - final LocalDateTime localDateTime = CONVERTER.convertField(field, pattern, name); - return localDateTime == null ? null : Timestamp.valueOf(localDateTime); + Instant instant = null; + switch (field) { + case null -> { + return null; + } + case Timestamp timestamp -> { + return timestamp; + } + case ZonedDateTime zonedDateTime -> { + instant = zonedDateTime.toInstant(); + } + case Time time -> { + // Convert to an Instant object preserving millisecond precision + final long epochMilli = time.getTime(); + instant = Instant.ofEpochMilli(epochMilli); + } + case Date date -> { + final long epochMilli = date.getTime(); + instant = Instant.ofEpochMilli(epochMilli); + } + case Number number -> { + switch (field) { + case Double d -> instant = FractionalSecondsUtils.toInstant(d); + case Float f -> instant = FractionalSecondsUtils.toInstant(f.doubleValue()); + default -> instant = FractionalSecondsUtils.toInstant(number.longValue()); + } + } + case String string -> { + final String stringTrimmed = string.trim(); + if (stringTrimmed.isEmpty()) { + return null; + } + + if (pattern.isPresent()) { + final String patternString = pattern.get(); + final DateTimeFormatter formatter = DateTimeFormatterRegistry.getDateTimeFormatter(patternString); + try { + // NOTE: In order to calculate any possible timezone offsets, the string must be parsed as a ZoneDateTime. + // It is not possible to always parse as a ZoneDateTime as it will fail if the pattern has + // no timezone information. Hence, a regular expression is used to determine whether it is necessary + // to parse with ZoneDateTime or not. + final Matcher matcher = TIMEZONE_PATTERN.matcher(patternString); + final ZonedDateTime zonedDateTime; + + if (matcher.find()) { + zonedDateTime = ZonedDateTime.parse(stringTrimmed, formatter); + } else { + final LocalDateTime localDateTime = LocalDateTime.parse(stringTrimmed, formatter); + zonedDateTime = ZonedDateTime.of(localDateTime, ZoneId.systemDefault()); + } + instant = zonedDateTime.toInstant(); + } catch (final DateTimeParseException e) { + return tryParseAsNumber(stringTrimmed, name); + } Review Comment: This logic is complex enough it looks like it warrants a separate method. ########## nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/FractionalSecondsUtils.java: ########## @@ -0,0 +1,57 @@ +/* + * 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. + */ +package org.apache.nifi.serialization.record.util; + +import java.time.Instant; + +public class FractionalSecondsUtils { + private static final long YEAR_TEN_THOUSAND = 253_402_300_800_000L; + + private FractionalSecondsUtils() { + } + + public static Instant tryParseAsNumber(final String value) { + // If decimal, treat as a double and convert to seconds and nanoseconds. + if (value.contains(".")) { + final double number = Double.parseDouble(value); + return toInstant(number); + } + + // attempt to parse as a long value + final long number = Long.parseLong(value); Review Comment: Promoting this parsing to a public method moves the exception handling requirements to the caller, which could be unexpected for referencing classes. I recommend catching the NumberFormatException in this class and throwing the FieldConversionException. ########## nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/field/ObjectTimestampFieldConverterTest.java: ########## @@ -44,70 +50,108 @@ public class ObjectTimestampFieldConverterTest { private static final String DATE_TIME_NANOSECONDS = "2000-01-01 12:00:00.123456789"; @Test - public void testConvertFieldNull() { + void testConvertFieldNull() { final Timestamp timestamp = CONVERTER.convertField(null, DEFAULT_PATTERN, FIELD_NAME); assertNull(timestamp); } @Test - public void testConvertFieldTimestamp() { + void testConvertFieldTimestamp() { final Timestamp field = new Timestamp(System.currentTimeMillis()); final Timestamp timestamp = CONVERTER.convertField(field, DEFAULT_PATTERN, FIELD_NAME); assertEquals(field, timestamp); } @Test - public void testConvertFieldDate() { + void testConvertFieldDate() { final Date field = new Date(); final Timestamp timestamp = CONVERTER.convertField(field, DEFAULT_PATTERN, FIELD_NAME); assertEquals(field.getTime(), timestamp.getTime()); } @Test - public void testConvertFieldLong() { + void testConvertFieldLong() { final long field = System.currentTimeMillis(); final Timestamp timestamp = CONVERTER.convertField(field, DEFAULT_PATTERN, FIELD_NAME); assertEquals(field, timestamp.getTime()); } @Test - public void testConvertFieldStringEmpty() { + void testConvertFieldStringEmpty() { final Timestamp timestamp = CONVERTER.convertField(EMPTY, DEFAULT_PATTERN, FIELD_NAME); assertNull(timestamp); } @Test - public void testConvertFieldStringFormatNull() { + void testConvertFieldStringFormatNull() { final long currentTime = System.currentTimeMillis(); final String field = Long.toString(currentTime); final Timestamp timestamp = CONVERTER.convertField(field, Optional.empty(), FIELD_NAME); assertEquals(currentTime, timestamp.getTime()); } @Test - public void testConvertFieldStringFormatNullNumberFormatException() { + void testConvertFieldStringFormatNullNumberFormatException() { final String field = String.class.getSimpleName(); final FieldConversionException exception = assertThrows(FieldConversionException.class, () -> CONVERTER.convertField(field, Optional.empty(), FIELD_NAME)); assertTrue(exception.getMessage().contains(field)); } @Test - public void testConvertFieldStringFormatDefault() { + void testConvertFieldStringFormatDefault() { final Timestamp timestamp = CONVERTER.convertField(DATE_TIME_DEFAULT, DEFAULT_PATTERN, FIELD_NAME); final Timestamp expected = Timestamp.valueOf(DATE_TIME_DEFAULT); assertEquals(expected, timestamp); } @Test - public void testConvertFieldStringFormatCustomNanoseconds() { + void testConvertFieldStringFormatCustomNanoseconds() { final Timestamp timestamp = CONVERTER.convertField(DATE_TIME_NANOSECONDS, DATE_TIME_NANOSECONDS_PATTERN, FIELD_NAME); final Timestamp expected = Timestamp.valueOf(DATE_TIME_NANOSECONDS); assertEquals(expected, timestamp); } @Test - public void testConvertFieldStringFormatCustomFormatterException() { + void testConvertFieldStringFormatCustomFormatterException() { final FieldConversionException exception = assertThrows(FieldConversionException.class, () -> CONVERTER.convertField(DATE_TIME_DEFAULT, DATE_TIME_NANOSECONDS_PATTERN, FIELD_NAME)); assertTrue(exception.getMessage().contains(DATE_TIME_DEFAULT)); } + + @Test + void testConvertFieldStringFormatWithTimeZone() { + final String originalTimestampHour = "12"; + //NOTE: Antarctica/Casey is the timezone offset chosen in timestamp below + final String originalTimestamp = "2000-01-01 " + originalTimestampHour + ":00:00+0800"; + final Optional<String> timezonePattern = Optional.of("yyyy-MM-dd HH:mm:ssZ"); + final Timestamp actual = CONVERTER.convertField(originalTimestamp, timezonePattern, FIELD_NAME); + final String actualString = actual.toString(); + + //Validate timezone is taken into account as the hour in the timestamp should be different per timezone offset + final String partialTimestampWithOriginalHour = " %s:".formatted(originalTimestampHour); + assertFalse(actualString.contains(partialTimestampWithOriginalHour)); + } + + @ParameterizedTest + @MethodSource("getPatterns") + void testTimeZonePattern(String pattern, boolean expected) { Review Comment: ```suggestion void testTimeZonePattern(String pattern, boolean matchExpected) { ``` ########## nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/field/ObjectTimestampFieldConverterTest.java: ########## @@ -44,70 +50,108 @@ public class ObjectTimestampFieldConverterTest { private static final String DATE_TIME_NANOSECONDS = "2000-01-01 12:00:00.123456789"; @Test - public void testConvertFieldNull() { + void testConvertFieldNull() { final Timestamp timestamp = CONVERTER.convertField(null, DEFAULT_PATTERN, FIELD_NAME); assertNull(timestamp); } @Test - public void testConvertFieldTimestamp() { + void testConvertFieldTimestamp() { final Timestamp field = new Timestamp(System.currentTimeMillis()); final Timestamp timestamp = CONVERTER.convertField(field, DEFAULT_PATTERN, FIELD_NAME); assertEquals(field, timestamp); } @Test - public void testConvertFieldDate() { + void testConvertFieldDate() { final Date field = new Date(); final Timestamp timestamp = CONVERTER.convertField(field, DEFAULT_PATTERN, FIELD_NAME); assertEquals(field.getTime(), timestamp.getTime()); } @Test - public void testConvertFieldLong() { + void testConvertFieldLong() { final long field = System.currentTimeMillis(); final Timestamp timestamp = CONVERTER.convertField(field, DEFAULT_PATTERN, FIELD_NAME); assertEquals(field, timestamp.getTime()); } @Test - public void testConvertFieldStringEmpty() { + void testConvertFieldStringEmpty() { final Timestamp timestamp = CONVERTER.convertField(EMPTY, DEFAULT_PATTERN, FIELD_NAME); assertNull(timestamp); } @Test - public void testConvertFieldStringFormatNull() { + void testConvertFieldStringFormatNull() { final long currentTime = System.currentTimeMillis(); final String field = Long.toString(currentTime); final Timestamp timestamp = CONVERTER.convertField(field, Optional.empty(), FIELD_NAME); assertEquals(currentTime, timestamp.getTime()); } @Test - public void testConvertFieldStringFormatNullNumberFormatException() { + void testConvertFieldStringFormatNullNumberFormatException() { final String field = String.class.getSimpleName(); final FieldConversionException exception = assertThrows(FieldConversionException.class, () -> CONVERTER.convertField(field, Optional.empty(), FIELD_NAME)); assertTrue(exception.getMessage().contains(field)); } @Test - public void testConvertFieldStringFormatDefault() { + void testConvertFieldStringFormatDefault() { final Timestamp timestamp = CONVERTER.convertField(DATE_TIME_DEFAULT, DEFAULT_PATTERN, FIELD_NAME); final Timestamp expected = Timestamp.valueOf(DATE_TIME_DEFAULT); assertEquals(expected, timestamp); } @Test - public void testConvertFieldStringFormatCustomNanoseconds() { + void testConvertFieldStringFormatCustomNanoseconds() { final Timestamp timestamp = CONVERTER.convertField(DATE_TIME_NANOSECONDS, DATE_TIME_NANOSECONDS_PATTERN, FIELD_NAME); final Timestamp expected = Timestamp.valueOf(DATE_TIME_NANOSECONDS); assertEquals(expected, timestamp); } @Test - public void testConvertFieldStringFormatCustomFormatterException() { + void testConvertFieldStringFormatCustomFormatterException() { final FieldConversionException exception = assertThrows(FieldConversionException.class, () -> CONVERTER.convertField(DATE_TIME_DEFAULT, DATE_TIME_NANOSECONDS_PATTERN, FIELD_NAME)); assertTrue(exception.getMessage().contains(DATE_TIME_DEFAULT)); } + + @Test + void testConvertFieldStringFormatWithTimeZone() { + final String originalTimestampHour = "12"; + //NOTE: Antarctica/Casey is the timezone offset chosen in timestamp below + final String originalTimestamp = "2000-01-01 " + originalTimestampHour + ":00:00+0800"; + final Optional<String> timezonePattern = Optional.of("yyyy-MM-dd HH:mm:ssZ"); + final Timestamp actual = CONVERTER.convertField(originalTimestamp, timezonePattern, FIELD_NAME); + final String actualString = actual.toString(); + + //Validate timezone is taken into account as the hour in the timestamp should be different per timezone offset + final String partialTimestampWithOriginalHour = " %s:".formatted(originalTimestampHour); + assertFalse(actualString.contains(partialTimestampWithOriginalHour)); Review Comment: Asserting false does not provide a high level of validation for this behavior. Instead, it would be better to declare the expected value and assert equals. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
