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:
[email protected]
