Copilot commented on code in PR #6577:
URL:
https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2792273945
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java:
##########
@@ -224,77 +224,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 ('?') question mark
+ if (containsQuestionMarkReference(value)) {
+ Object existing = context.getValue("?");
+ if (Objects.equals(existing, left)) {
+ right = value.evaluate(context);
+ } else {
+ context.enterFrame();
+ try {
+ context.setValue("?", left);
+ right = value.evaluate(context);
+ } finally {
+ context.exitFrame();
+ }
+ }
+ } else {
+ right = value.evaluate(context);
+ }
+ return right;
+ }
Review Comment:
This PR introduces new behavior to bind the special `"?"` variable while
evaluating unary tests (via `evaluateRightValue()`), but there are no tests
exercising unary-test expressions that reference `?` (e.g. `<= ? + 1`, `in [ ?
.. ? + 2 ]`). Since this module already has unary-test evaluation tests, please
add coverage to prevent regressions and to prove the fix for issue #2228.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java:
##########
@@ -224,77 +224,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 ('?') question mark
+ if (containsQuestionMarkReference(value)) {
Review Comment:
`evaluateRightValue()` calls `containsQuestionMarkReference(value)` on every
unary-test evaluation. That method recursively traverses the entire AST, so for
decision tables (many rows/cells) this adds avoidable overhead. Consider
computing this once per `UnaryTestNode` instance (e.g., a cached boolean/lazy
init) and reusing it in `evaluateRightValue()`.
```suggestion
/**
* Cached result of checking whether this unary test's value expression
* contains a reference to the special "?" variable.
* <p>
* Uses lazy initialization to avoid paying the cost of traversing the
* expression AST more than once per {@link UnaryTestNode} instance.
*/
private Boolean cachedContainsQuestionMarkReference;
/**
* Returns true if this node's value expression contains a reference to
* the special "?" variable. The result is computed at most once per
* {@link UnaryTestNode} instance and then cached.
*
* @return true if the value expression contains a "?" reference
*/
private boolean hasQuestionMarkReference() {
Boolean cached = cachedContainsQuestionMarkReference;
if (cached == null) {
cached = containsQuestionMarkReference(value);
cachedContainsQuestionMarkReference = cached;
}
return cached;
}
private Object evaluateRightValue(EvaluationContext context, Object
left) {
Object right;
// set the value if the expression contains ('?') question mark
if (hasQuestionMarkReference()) {
```
--
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]
