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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
