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/02/25 18:13:00 UTC
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4773: NIFI-8161 NiFi EL: migration from SimpleDateFormat to DateTimeFormatter
exceptionfactory commented on a change in pull request #4773:
URL: https://github.com/apache/nifi/pull/4773#discussion_r583004827
##########
File path: pom.xml
##########
@@ -361,6 +362,17 @@
<version>${jetty.version}</version>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>org.openjdk.jmh</groupId>
+ <artifactId>jmh-core</artifactId>
Review comment:
The `FormatEvaluatorBenchmark` is interesting, but additional dependencies need to be added to `NOTICE` files. Although it is a useful point of reference, it would simplify the PR to remove these dependencies and the associated benchmark. Perhaps providing the benchmark as link in the PR description or associated Jira issue would be a better way to go.
##########
File path: nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/FormatUtils.java
##########
@@ -425,4 +434,32 @@ public static String formatNanos(final long nanos, final boolean includeTotalNan
return sb.toString();
}
+
+ public static DateTimeFormatter prepareLenientCaseInsensitiveDateTimeFormatter(String pattern) {
+ return new DateTimeFormatterBuilder()
+ .parseLenient()
+ .parseCaseInsensitive()
+ .appendPattern(pattern)
+ .toFormatter(Locale.US);
Review comment:
NiFi has users across the world, so hard-coding formatting to particular localities should be avoided.
##########
File path: nifi-commons/nifi-utils/src/test/java/org/apache/nifi/processor/TestFormatUtils.java
##########
@@ -103,4 +111,41 @@ public void testFormatDataSize() {
assertEquals(String.format("181%s9 TB", decimalFormatSymbols.getDecimalSeparator()), FormatUtils.formatDataSize(200_000_000_000_000d));
}
+
+ @Test
+ public void testParseInstant() throws Exception {
+ TimeZone current = TimeZone.getDefault();
+ TimeZone.setDefault(TimeZone.getTimeZone("Europe/Kiev"));
+ try {
+ checkSameResultsWithSimpleDateFormat("yyyy-MM-dd HH:mm:ss", "2020-01-01 02:00:00", null, "2020-01-01T00:00:00");
+ checkSameResultsWithSimpleDateFormat("yyyy-MM-dd HH:mm:ss", "2020-01-01 01:00:00", "Europe/Warsaw", "2020-01-01T00:00:00");
+ checkSameResultsWithSimpleDateFormat("yyyy-MM-dd HH:mm:ss Z", "2020-01-01 01:00:00 +0100", null, "2020-01-01T00:00:00");
+ checkSameResultsWithSimpleDateFormat("yyyy-MM-dd", "2020-01-01", null, "2019-12-31T22:00:00");
+ checkSameResultsWithSimpleDateFormat("HH:mm:ss", "03:00:00", null, "1970-01-01T00:00:00");
+ checkSameResultsWithSimpleDateFormat("yyyy-MMM-dd", "2020-may-01", null, "2020-04-30T21:00:00");
+ } finally {
+ TimeZone.setDefault(current);
+ }
+ }
+
+ private void checkSameResultsWithSimpleDateFormat(String pattern, String parsedDateTime, String zone, String expectedUtcDateTime) throws Exception {
+ LocalDateTime expectedDateTime = LocalDateTime.parse(expectedUtcDateTime);
+
+ // reference implementation
+ SimpleDateFormat sdf = new SimpleDateFormat(pattern, Locale.US);
+ if (zone != null) {
+ sdf.setTimeZone(TimeZone.getTimeZone(zone));
+ }
+ Instant simpleDateFormatResult = sdf.parse(parsedDateTime).toInstant();
+ assertEquals(expectedDateTime, simpleDateFormatResult.atZone(ZoneOffset.UTC).toLocalDateTime());
Review comment:
The use of `ZoneOffset.UTC` effectively changes the value of `simpleDateFormatResult`, so recommend avoiding format conversion when perform assert checks to ensure that the test is comparing expected return values.
##########
File path: nifi-commons/nifi-expression-language/src/test/java/org/apache/nifi/attribute/expression/language/TestQuery.java
##########
@@ -846,7 +848,7 @@ public void testEscapeQuotes() {
final String query = "startDateTime=\"${date:toNumber():toDate():format(\"" + format + "\")}\"";
final String result = Query.evaluateExpressions(query, attributes, null);
- final String expectedTime = new SimpleDateFormat(format, Locale.US).format(timestamp);
+ final String expectedTime = DateTimeFormatter.ofPattern(format, Locale.US).format(Instant.ofEpochMilli(timestamp).atZone(ZoneId.systemDefault()));
Review comment:
When comparing expected output, it is best to use a specific `ZoneId` as the test results could be different at build time depending on the timezone of the system running the build.
##########
File path: nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/FormatUtils.java
##########
@@ -425,4 +434,32 @@ public static String formatNanos(final long nanos, final boolean includeTotalNan
return sb.toString();
}
+
+ public static DateTimeFormatter prepareLenientCaseInsensitiveDateTimeFormatter(String pattern) {
+ return new DateTimeFormatterBuilder()
+ .parseLenient()
+ .parseCaseInsensitive()
+ .appendPattern(pattern)
+ .toFormatter(Locale.US);
+ }
+
+ /**
+ * Parse text to Instant - support different formats like: zoned date time, date time, date, time (similar to those supported in SimpleDateFormat)
+ * @param formatter configured formatter
+ * @param text text which will be parsed
+ * @return parsed Instant
+ */
+ public static Instant parseInstant(DateTimeFormatter formatter, String text) {
Review comment:
Since this is a public method, recommend checking the `text` input for `null` before parsing.
##########
File path: nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/FormatUtils.java
##########
@@ -425,4 +434,32 @@ public static String formatNanos(final long nanos, final boolean includeTotalNan
return sb.toString();
}
+
+ public static DateTimeFormatter prepareLenientCaseInsensitiveDateTimeFormatter(String pattern) {
+ return new DateTimeFormatterBuilder()
+ .parseLenient()
+ .parseCaseInsensitive()
+ .appendPattern(pattern)
+ .toFormatter(Locale.US);
+ }
+
+ /**
+ * Parse text to Instant - support different formats like: zoned date time, date time, date, time (similar to those supported in SimpleDateFormat)
+ * @param formatter configured formatter
+ * @param text text which will be parsed
+ * @return parsed Instant
+ */
+ public static Instant parseInstant(DateTimeFormatter formatter, String text) {
+ TemporalAccessor parsed = formatter.parseBest(text, Instant::from, LocalDateTime::from, LocalDate::from, LocalTime::from);
+ if (parsed instanceof Instant) {
+ return (Instant) parsed;
+ } else if (parsed instanceof LocalDateTime) {
+ return ((LocalDateTime) parsed).atZone(ZoneId.systemDefault()).toInstant();
Review comment:
Using `ZoneId.systemDefault()` can introduce unexpected behavior at runtime depending on the system configuration. Recommend introducing unit tests to evaluate the behavior on different time zones.
##########
File path: nifi-commons/nifi-expression-language/src/main/java/org/apache/nifi/attribute/expression/language/evaluation/functions/FormatEvaluator.java
##########
@@ -47,23 +71,27 @@ public FormatEvaluator(final DateEvaluator subject, final Evaluator<String> form
return new StringQueryResult(null);
}
- final QueryResult<String> formatResult = format.evaluate(evaluationContext);
- final String format = formatResult.getValue();
- if (format == null) {
- return null;
+ DateTimeFormatter dtf;
+ if (preparedFormatter != null) {
+ dtf = preparedFormatter;
+ } else {
+ final QueryResult<String> formatResult = format.evaluate(evaluationContext);
+ final String format = formatResult.getValue();
+ if (format == null) {
+ return null;
+ }
+ dtf = DateTimeFormatter.ofPattern(format, Locale.US);
}
- final SimpleDateFormat sdf = new SimpleDateFormat(format, Locale.US);
-
- if(timeZone != null) {
+ if ((preparedFormatter == null || !preparedFormatterHasCorrectZone) && timeZone != null) {
final QueryResult<String> tzResult = timeZone.evaluate(evaluationContext);
final String tz = tzResult.getValue();
- if(tz != null && TimeZone.getTimeZone(tz) != null) {
- sdf.setTimeZone(TimeZone.getTimeZone(tz));
+ if(tz != null) {
+ dtf = dtf.withZone(ZoneId.of(tz));
}
}
- return new StringQueryResult(sdf.format(subjectValue));
+ return new StringQueryResult(dtf.format(subjectValue.toInstant().atZone(ZoneId.systemDefault())));
Review comment:
Converting the result using `ZoneId.systemDefault()` can result in different behavior based on the running system configuration, is this the best approach? At minimum it is worth documenting the behavior. Given that `SimpleDateFormat` returns a `Date` object that depends on the system time zone internally, this approach might make sense, but it is worth testing and documenting.
##########
File path: nifi-commons/nifi-expression-language/src/test/java/org/apache/nifi/attribute/expression/language/evaluation/functions/FormatEvaluatorBenchmark.java
##########
@@ -0,0 +1,66 @@
+/*
+ * 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.attribute.expression.language.evaluation.functions;
+
+import org.apache.nifi.attribute.expression.language.EvaluationContext;
+import org.apache.nifi.attribute.expression.language.StandardEvaluationContext;
+import org.apache.nifi.attribute.expression.language.evaluation.literals.StringLiteralEvaluator;
+import org.apache.nifi.attribute.expression.language.evaluation.literals.WholeNumberLiteralEvaluator;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.OutputTimeUnit;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.Threads;
+import org.openjdk.jmh.runner.Runner;
+import org.openjdk.jmh.runner.RunnerException;
+import org.openjdk.jmh.runner.options.Options;
+import org.openjdk.jmh.runner.options.OptionsBuilder;
+
+import java.util.Collections;
+import java.util.concurrent.TimeUnit;
+
+@State(Scope.Benchmark)
+@Threads(16)
+@BenchmarkMode(Mode.AverageTime)
+@OutputTimeUnit(TimeUnit.MICROSECONDS)
+public class FormatEvaluatorBenchmark {
Review comment:
This is useful for comparison, but removing it would simplify the PR and avoid introducing new dependencies.
##########
File path: nifi-commons/nifi-utils/src/test/java/org/apache/nifi/processor/TestFormatUtils.java
##########
@@ -103,4 +111,41 @@ public void testFormatDataSize() {
assertEquals(String.format("181%s9 TB", decimalFormatSymbols.getDecimalSeparator()), FormatUtils.formatDataSize(200_000_000_000_000d));
}
+
+ @Test
+ public void testParseInstant() throws Exception {
+ TimeZone current = TimeZone.getDefault();
+ TimeZone.setDefault(TimeZone.getTimeZone("Europe/Kiev"));
Review comment:
This is a helpful check, recommend creating additional unit test methods to exercise several different time zones both greater than and less than GMT.
##########
File path: nifi-commons/nifi-utils/src/test/java/org/apache/nifi/processor/TestFormatUtils.java
##########
@@ -103,4 +111,41 @@ public void testFormatDataSize() {
assertEquals(String.format("181%s9 TB", decimalFormatSymbols.getDecimalSeparator()), FormatUtils.formatDataSize(200_000_000_000_000d));
}
+
+ @Test
+ public void testParseInstant() throws Exception {
+ TimeZone current = TimeZone.getDefault();
+ TimeZone.setDefault(TimeZone.getTimeZone("Europe/Kiev"));
+ try {
+ checkSameResultsWithSimpleDateFormat("yyyy-MM-dd HH:mm:ss", "2020-01-01 02:00:00", null, "2020-01-01T00:00:00");
+ checkSameResultsWithSimpleDateFormat("yyyy-MM-dd HH:mm:ss", "2020-01-01 01:00:00", "Europe/Warsaw", "2020-01-01T00:00:00");
+ checkSameResultsWithSimpleDateFormat("yyyy-MM-dd HH:mm:ss Z", "2020-01-01 01:00:00 +0100", null, "2020-01-01T00:00:00");
+ checkSameResultsWithSimpleDateFormat("yyyy-MM-dd", "2020-01-01", null, "2019-12-31T22:00:00");
+ checkSameResultsWithSimpleDateFormat("HH:mm:ss", "03:00:00", null, "1970-01-01T00:00:00");
+ checkSameResultsWithSimpleDateFormat("yyyy-MMM-dd", "2020-may-01", null, "2020-04-30T21:00:00");
+ } finally {
+ TimeZone.setDefault(current);
+ }
+ }
+
+ private void checkSameResultsWithSimpleDateFormat(String pattern, String parsedDateTime, String zone, String expectedUtcDateTime) throws Exception {
+ LocalDateTime expectedDateTime = LocalDateTime.parse(expectedUtcDateTime);
+
+ // reference implementation
+ SimpleDateFormat sdf = new SimpleDateFormat(pattern, Locale.US);
Review comment:
As mentioned elsewhere, references to specific locales, unless for the purpose of testing that particular instance, should be avoided.
----------------------------------------------------------------
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