exceptionfactory commented on code in PR #8332: URL: https://github.com/apache/nifi/pull/8332#discussion_r1483154296
########## nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/field/TestObjectLocalDateTimeFieldConverter.java: ########## @@ -0,0 +1,87 @@ +/* + * 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.field; + +import org.junit.jupiter.api.Test; + +import java.time.Instant; +import java.time.LocalDateTime; +import java.time.ZoneId; +import java.util.Optional; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +public class TestObjectLocalDateTimeFieldConverter { + private final long MILLIS_TIMESTAMP_LONG = 1707238288351L; + private final long MICROS_TIMESTAMP_LONG = 1707238288351567L; + private final String MICROS_TIMESTAMP_STRING = Long.toString(MICROS_TIMESTAMP_LONG); + private final double MICROS_TIMESTAMP_DOUBLE = ((double) MICROS_TIMESTAMP_LONG) / 1000000D; + private final long NANOS_AFTER_SECOND = 351567000L; + private final Instant INSTANT_MILLIS_PRECISION = Instant.ofEpochMilli(MILLIS_TIMESTAMP_LONG); + // Create an instant to represent the same time as the microsecond precision timestamp. We add nanoseconds after second but then have to subtract the milliseconds after the second that are already + // present in the MILLIS_TIMESTAMP_LONG value. + private final Instant INSTANT_MICROS_PRECISION = Instant.ofEpochMilli(MILLIS_TIMESTAMP_LONG).plusNanos(NANOS_AFTER_SECOND).minusMillis(MILLIS_TIMESTAMP_LONG % 1000); + private final LocalDateTime LOCAL_DATE_TIME_MILLIS_PRECISION = LocalDateTime.ofInstant(INSTANT_MILLIS_PRECISION, ZoneId.systemDefault()); + private final LocalDateTime LOCAL_DATE_TIME_MICROS_PRECISION = LocalDateTime.ofInstant(INSTANT_MICROS_PRECISION, ZoneId.systemDefault()); + + private final ObjectLocalDateTimeFieldConverter converter = new ObjectLocalDateTimeFieldConverter(); + + + @Test + public void testConvertTimestampMillis() { + final LocalDateTime result = converter.convertField(MILLIS_TIMESTAMP_LONG, Optional.empty(), "test"); Review Comment: Just a minor bit, but recommend declaring a static `FIELD_NAME` value and reusing across all test methods. ########## nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/field/ObjectLocalDateTimeFieldConverter.java: ########## @@ -67,22 +70,59 @@ public LocalDateTime convertField(final Object field, final Optional<String> pat try { return LocalDateTime.parse(string, formatter); } catch (final DateTimeParseException e) { - throw new FieldConversionException(LocalDateTime.class, field, name, e); + return tryParseAsNumber(string, name); } } else { - try { - final long number = Long.parseLong(string); - final Instant instant = Instant.ofEpochMilli(number); - return ofInstant(instant); - } catch (final NumberFormatException e) { - throw new FieldConversionException(LocalDateTime.class, field, name, e); - } + return tryParseAsNumber(string, name); } } throw new FieldConversionException(LocalDateTime.class, field, name); } + private LocalDateTime tryParseAsNumber(final String value, final String fieldName) { + // If decimal, treat as a double and convert to seconds and nanoseconds. + if (value.contains(".")) { + final double number = Double.parseDouble(value); Review Comment: The `Double.parseDouble()` method could throw a `NumberFormatException`, similar to below with `Long.parseLong()`, so it would be helpful to have the same try-catch that throws a `FieldConversionException` for troubleshooting. For example, if the string contains two period characters, instead of one, which would be a good test case. ########## nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/field/ObjectLocalDateTimeFieldConverter.java: ########## @@ -67,22 +70,59 @@ public LocalDateTime convertField(final Object field, final Optional<String> pat try { return LocalDateTime.parse(string, formatter); } catch (final DateTimeParseException e) { - throw new FieldConversionException(LocalDateTime.class, field, name, e); + return tryParseAsNumber(string, name); } } else { - try { - final long number = Long.parseLong(string); - final Instant instant = Instant.ofEpochMilli(number); - return ofInstant(instant); - } catch (final NumberFormatException e) { - throw new FieldConversionException(LocalDateTime.class, field, name, e); - } + return tryParseAsNumber(string, name); } } throw new FieldConversionException(LocalDateTime.class, field, name); } + private LocalDateTime tryParseAsNumber(final String value, final String fieldName) { + // If decimal, treat as a double and convert to seconds and nanoseconds. + if (value.contains(".")) { + final double number = Double.parseDouble(value); + return toLocalDateTime(number); + } + + // attempt to parse as a long value + try { + final long number = Long.parseLong(value); + return toLocalDateTime(number); + } catch (final NumberFormatException e) { + throw new FieldConversionException(LocalDateTime.class, value, fieldName, e); + } + } + + private LocalDateTime toLocalDateTime(final double secondsSinceEpoch) { + // Determine the number of micros past the second by subtracting the number of seconds from the decimal value and multiplying by 1 million. + final long micros = (long) (1_000_000 * (secondsSinceEpoch - (long) secondsSinceEpoch)); + // Convert micros to nanos. Note that we perform this as a separate operation, rather than multiplying by 1_000,000,000 in order to avoid + // issues that occur with rounding at high precision. + final long nanos = micros * 1000L; + + return toLocalDateTime((long) secondsSinceEpoch, nanos); Review Comment: Given the multiple casts to `long` for `secondsSinceEpoch`, it seems like it would help readability to declare that separately and reuse the value. ########## nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/field/ObjectLocalDateTimeFieldConverter.java: ########## @@ -67,22 +70,59 @@ public LocalDateTime convertField(final Object field, final Optional<String> pat try { return LocalDateTime.parse(string, formatter); } catch (final DateTimeParseException e) { - throw new FieldConversionException(LocalDateTime.class, field, name, e); + return tryParseAsNumber(string, name); } } else { - try { - final long number = Long.parseLong(string); - final Instant instant = Instant.ofEpochMilli(number); - return ofInstant(instant); - } catch (final NumberFormatException e) { - throw new FieldConversionException(LocalDateTime.class, field, name, e); - } + return tryParseAsNumber(string, name); } } throw new FieldConversionException(LocalDateTime.class, field, name); } + private LocalDateTime tryParseAsNumber(final String value, final String fieldName) { + // If decimal, treat as a double and convert to seconds and nanoseconds. + if (value.contains(".")) { + final double number = Double.parseDouble(value); + return toLocalDateTime(number); + } + + // attempt to parse as a long value + try { + final long number = Long.parseLong(value); + return toLocalDateTime(number); + } catch (final NumberFormatException e) { + throw new FieldConversionException(LocalDateTime.class, value, fieldName, e); + } + } + + private LocalDateTime toLocalDateTime(final double secondsSinceEpoch) { + // Determine the number of micros past the second by subtracting the number of seconds from the decimal value and multiplying by 1 million. + final long micros = (long) (1_000_000 * (secondsSinceEpoch - (long) secondsSinceEpoch)); + // Convert micros to nanos. Note that we perform this as a separate operation, rather than multiplying by 1_000,000,000 in order to avoid + // issues that occur with rounding at high precision. + final long nanos = micros * 1000L; + + return toLocalDateTime((long) secondsSinceEpoch, nanos); + } + + private LocalDateTime toLocalDateTime(final long epochSeconds, final long nanosPastSecond) { + final Instant instant = Instant.ofEpochSecond(epochSeconds).plusNanos(nanosPastSecond); + return ofInstant(instant); + } + + private LocalDateTime toLocalDateTime(final long value) { + final Instant instant = Instant.ofEpochMilli(value); + final LocalDateTime localDateTime = ofInstant(instant); + if (localDateTime.getYear() > 10_000) { Review Comment: Instead of comparing the year value after the conversion, what about checking that the initial `long value` is greater than a certain number equivalent to 10K years in the future? That would avoid multiple calls to `ofInstant()` in the case of handling microseconds. -- 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]
