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