exceptionfactory commented on a change in pull request #4773:
URL: https://github.com/apache/nifi/pull/4773#discussion_r590458077
##########
File path:
nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/FormatUtils.java
##########
@@ -425,4 +434,36 @@ 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) {
+ if (text == null) {
+ throw new IllegalArgumentException("Text cannot be null");
+ }
+
+ 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();
+ } else if (parsed instanceof LocalDate) {
+ return ((LocalDate) parsed).atTime(0,
0).atZone(ZoneId.systemDefault()).toInstant();
+ } else {
+ return ((LocalTime) parsed).atDate(LocalDate.of(1970, 1,
1)).atZone(ZoneId.systemDefault()).toInstant();
Review comment:
Is it worth having a static value of `LocalDate.of(1970, 1, 1)` instead
of creating a new `LocalDate` object for each method invocation?
##########
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:
Given that the existing SimpleDateFormat is subject to the same issue,
this is probably acceptable. Is it possible to refactor the three calls to
`atZone(ZoneId.systemDefault()).toInstant()` to a shared method? That would
reduce some duplication and also make the approach a little clearer.
##########
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:
Thanks for pointing out the existing implementation references to
`Locale.US`, that is worth noting. In that case, this seems fine for now,
perhaps worth revisiting in the future.
##########
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:
Thanks for adding the unit test and referencing the documentation, that
clarifies the implementation.
##########
File path:
nifi-commons/nifi-expression-language/src/main/java/org/apache/nifi/attribute/expression/language/evaluation/functions/StringToDateEvaluator.java
##########
@@ -17,27 +17,51 @@
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.DateEvaluator;
import
org.apache.nifi.attribute.expression.language.evaluation.DateQueryResult;
import org.apache.nifi.attribute.expression.language.evaluation.Evaluator;
import org.apache.nifi.attribute.expression.language.evaluation.QueryResult;
+import
org.apache.nifi.attribute.expression.language.evaluation.literals.StringLiteralEvaluator;
import
org.apache.nifi.attribute.expression.language.exception.IllegalAttributeException;
+import org.apache.nifi.util.FormatUtils;
-import java.text.ParseException;
-import java.text.SimpleDateFormat;
+import java.time.ZoneId;
+import java.time.format.DateTimeFormatter;
+import java.time.format.DateTimeParseException;
+import java.util.Collections;
import java.util.Date;
-import java.util.Locale;
-import java.util.TimeZone;
public class StringToDateEvaluator extends DateEvaluator {
private final Evaluator<String> subject;
private final Evaluator<String> format;
private final Evaluator<String> timeZone;
+ private final DateTimeFormatter preparedFormatter;
+ private final boolean preparedFormatterHasCorrectZone;
Review comment:
Perhaps renaming this variable would help clarify the implementation.
Using `Correct` implies that there might be some kind of error condition,
whereas the purpose is to ensure that the formatter has the timezone specified
by the expression language parameter. Some options might be
`preparedFormatterHasRequestedTimeZone` or `timeZoneConfigured`.
##########
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:
This is acceptable given the existing implementation.
----------------------------------------------------------------
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]
