You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "exceptionfactory (via GitHub)" <gi...@apache.org> on 2023/05/09 15:01:41 UTC
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7169: NIFI-8161 NiFi EL: migration from SimpleDateFormat to DateTimeFormatter: rebased to 2.0
exceptionfactory commented on code in PR #7169:
URL: https://github.com/apache/nifi/pull/7169#discussion_r1188740275
##########
nifi-commons/nifi-expression-language/src/main/java/org/apache/nifi/attribute/expression/language/evaluation/functions/FormatEvaluator.java:
##########
@@ -47,23 +71,27 @@ public QueryResult<String> evaluate(final EvaluationContext evaluationContext) {
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);
}
Review Comment:
Recommend reversing the conditional blocks to avoid the negative null check:
```suggestion
if (preparedFormatter == null) {
final QueryResult<String> formatResult = format.evaluate(evaluationContext);
final String format = formatResult.getValue();
if (format == null) {
return null;
}
dtf = DateTimeFormatter.ofPattern(format, Locale.US);
} else {
dtf = preparedFormatter;
}
```
##########
nifi-commons/nifi-expression-language/src/main/java/org/apache/nifi/attribute/expression/language/evaluation/functions/FormatEvaluator.java:
##########
@@ -47,23 +71,27 @@ public QueryResult<String> evaluate(final EvaluationContext evaluationContext) {
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 || !preparedFormatterHasRequestedTimeZone) && 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 would be helpful to declare the result of `subject.toInstant().atZone(ZoneId.systemDefault())` and then pass it to avoid the nested calls.
##########
nifi-commons/nifi-expression-language/src/main/java/org/apache/nifi/attribute/expression/language/evaluation/functions/FormatEvaluator.java:
##########
@@ -47,23 +71,27 @@ public QueryResult<String> evaluate(final EvaluationContext evaluationContext) {
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 || !preparedFormatterHasRequestedTimeZone) && 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) {
Review Comment:
```suggestion
if (tz != null) {
```
##########
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 preparedFormatterHasRequestedTimeZone;
+
public StringToDateEvaluator(final Evaluator<String> subject, final Evaluator<String> format, final Evaluator<String> timeZone) {
this.subject = subject;
this.format = format;
+ // if the search string is a literal, we don't need to prepare formatter each time; we can just
+ // prepare it once. Otherwise, it must be prepared for each time.
+ if (format instanceof StringLiteralEvaluator) {
+ DateTimeFormatter dtf = FormatUtils.prepareLenientCaseInsensitiveDateTimeFormatter(format.evaluate(new StandardEvaluationContext(Collections.emptyMap())).getValue());
Review Comment:
See above note on nested calls and separate variable declaration.
##########
nifi-commons/nifi-expression-language/src/main/java/org/apache/nifi/attribute/expression/language/evaluation/functions/FormatEvaluator.java:
##########
@@ -17,26 +17,50 @@
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.Evaluator;
import org.apache.nifi.attribute.expression.language.evaluation.QueryResult;
import org.apache.nifi.attribute.expression.language.evaluation.StringEvaluator;
import org.apache.nifi.attribute.expression.language.evaluation.StringQueryResult;
+import org.apache.nifi.attribute.expression.language.evaluation.literals.StringLiteralEvaluator;
-import java.text.SimpleDateFormat;
+import java.time.ZoneId;
+import java.time.format.DateTimeFormatter;
+import java.util.Collections;
import java.util.Date;
import java.util.Locale;
-import java.util.TimeZone;
public class FormatEvaluator extends StringEvaluator {
private final DateEvaluator subject;
private final Evaluator<String> format;
private final Evaluator<String> timeZone;
+ private final DateTimeFormatter preparedFormatter;
+ private final boolean preparedFormatterHasRequestedTimeZone;
+
public FormatEvaluator(final DateEvaluator subject, final Evaluator<String> format, final Evaluator<String> timeZone) {
this.subject = subject;
this.format = format;
+ // if the search string is a literal, we don't need to prepare formatter each time; we can just
+ // prepare it once. Otherwise, it must be prepared for each time.
+ if (format instanceof StringLiteralEvaluator) {
+ DateTimeFormatter dtf = DateTimeFormatter.ofPattern(format.evaluate(new StandardEvaluationContext(Collections.emptyMap())).getValue(), Locale.US);
Review Comment:
The nested of format evaluation is difficult to follow, it would be helpful to declare the pattern string as a separate variable before passing it to `DateTimeFormatter.ofPattern()`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org