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/28 15:46:22 UTC
[GitHub] [nifi] arkadius commented on a change in pull request #4773: NIFI-8161 NiFi EL: migration from SimpleDateFormat to DateTimeFormatter
arkadius commented on a change in pull request #4773:
URL: https://github.com/apache/nifi/pull/4773#discussion_r584316601
##########
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:
You've right and I've checked that jmh is on GPL licence so I've removed this benchmark code at all from this PR.
##########
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:
Added
##########
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:
Removed
##########
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:
In fact it was implicitly done before my change because `SimpleDateFormat.format` uses system timezone in this case but I agree with you that using system time zone in tests should be avoided and in production code should be well documented and test - fixed.
##########
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:
It is tested in `TestFormatUtils.testParseInstant`.
##########
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:
In general it make sense but for some reasons someone hardcoded US locales in all places in nifi expression language. Take a look at `DateCastEvaluator`, `FormatEvaluator`, `StringToDateEvaluator` before my change. Everywhere was used `new SimpleDateFormat(formatValue, Locale.US)`. And it is quite good for most cases because quite often names of months, day of weeks and so on used in data are in English even if locales are different.
##########
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:
I'm testing here different time zone combination, there are explicitly passed Europe/Warsaw and system default (set to Europe/Kiev), what will give us testing with more combinations? Java time API handles in the same way time zones greater and less than GMT.
##########
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:
It is one approach that will give reasonable result when user wont provide time zone as an argument of `format` function and it worked this way before the change. Actually it is documented here: https://nifi.apache.org/docs/nifi-docs/html/expression-language-guide.html#format in "The number will be evaluated using the local time zone unless specified in the second optional argument". But you've right that it is not visible in tests. I've added `TestQuer.testFormatUsesLocalTimeZoneUnlessIsSpecified` test case.
##########
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:
You've right but I don't want to hardcode instant as a millis from 1970 - it would obfuscate test cases. I've changed the logic that now result isn't in UTC but expected value is converted to instant using UTC. It will be more readable?
----------------------------------------------------------------
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