Copilot commented on code in PR #6577:
URL:
https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2768373432
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java:
##########
@@ -224,77 +223,122 @@ static Boolean areElementsEqual(Object left, Object
right) {
() -> Boolean.FALSE)
);
}
+ private Object evaluateRightValue(EvaluationContext context, Object left) {
+ Object right;
+ // set the value if the expression contains '?'
+ if (containsQuestionMarkReference(value)) {
+ Object existing = context.getMapping("?");
+ if(existing != null && existing.equals(left)) {
+ right = value.evaluate(context);
+ } else {
+ context.enterFrame();
+ try {
+ context.setValue("?", left);
+ right = value.evaluate(context);
+ } finally {
+ context.exitFrame();
+ }
+ }
Review Comment:
The optimization check using `getMapping("?")` on line 230 will never work
as intended because `setMapping("?", left)` is never called after setting the
value. The mapping is retrieved but never updated, which means `existing` will
always be null on the first call and the optimization to skip frame creation
will never trigger. Consider calling `context.setMapping("?", left)` after line
236 to properly track the current value of "?" and enable the optimization.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/impl/EvaluationContextImpl.java:
##########
@@ -50,6 +50,8 @@ public class EvaluationContextImpl implements
EvaluationContext {
private DMNRuntime dmnRuntime;
private boolean performRuntimeTypeCheck = false;
+ private final Map<String, Object> variableMapping = new HashMap<>();
Review Comment:
The `variableMapping` HashMap is not thread-safe. If multiple threads share
the same EvaluationContextImpl instance and call setMapping/getMapping
concurrently, this could lead to race conditions. Consider using
ConcurrentHashMap instead of HashMap, or document that EvaluationContextImpl
instances are not thread-safe and should not be shared across threads.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/impl/EvaluationContextImpl.java:
##########
@@ -262,4 +264,15 @@ public DMNVersion getDMNVersion() {
public boolean isLenient() {
return isLenient;
}
+
+ @Override
+ public void setMapping(String variable, Object value) {
+ variableMapping.put(variable, value);
+ }
Review Comment:
Missing blank line between the two methods. The setMapping method on line
269-271 and getMapping method on line 272-275 should have a blank line
separator for consistency with the rest of the class formatting (see lines
243-251, 253-256, etc.).
```suggestion
}
```
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java:
##########
@@ -224,77 +223,122 @@ static Boolean areElementsEqual(Object left, Object
right) {
() -> Boolean.FALSE)
);
}
+ private Object evaluateRightValue(EvaluationContext context, Object left) {
+ Object right;
+ // set the value if the expression contains '?'
+ if (containsQuestionMarkReference(value)) {
+ Object existing = context.getMapping("?");
+ if(existing != null && existing.equals(left)) {
Review Comment:
Missing space after 'if' keyword. The code style in Java typically requires
a space between 'if' and the opening parenthesis for readability.
```suggestion
if (existing != null && existing.equals(left)) {
```
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java:
##########
@@ -224,77 +223,122 @@ static Boolean areElementsEqual(Object left, Object
right) {
() -> Boolean.FALSE)
);
}
+ private Object evaluateRightValue(EvaluationContext context, Object left) {
+ Object right;
+ // set the value if the expression contains '?'
+ if (containsQuestionMarkReference(value)) {
+ Object existing = context.getMapping("?");
+ if(existing != null && existing.equals(left)) {
Review Comment:
The null check on line 231 `existing != null && existing.equals(left)`
doesn't handle the case where `left` is null correctly. If `left` is null and
`existing` is null, the condition fails and a new frame is created
unnecessarily. This check should use a null-safe equality comparison, similar
to `Objects.equals(existing, left)`, to properly handle null values on both
sides.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/impl/EvaluationContextImpl.java:
##########
@@ -262,4 +264,15 @@ public DMNVersion getDMNVersion() {
public boolean isLenient() {
return isLenient;
}
+
+ @Override
+ public void setMapping(String variable, Object value) {
+ variableMapping.put(variable, value);
+ }
+ @Override
+ public Object getMapping(String variable) {
+ return variableMapping.get(variable);
+ }
+
+
Review Comment:
There are two consecutive blank lines (276-277) before the closing brace.
The rest of the file uses a single blank line before the closing brace for
consistency.
```suggestion
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
