Copilot commented on code in PR #6577:
URL:
https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2753839366
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java:
##########
@@ -224,77 +223,117 @@ 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)) {
+ context.enterFrame();
+ try {
+ context.setValue("?", left);
+ right = value.evaluate(context);
+ } finally {
+ context.exitFrame();
+ }
+ } else {
+ right = value.evaluate(context);
+ }
+ return right;
+ }
+
+ /**
+ * Checks if the given node is a plain '?'
+ */
+ private boolean isPlainQuestionMark(BaseNode node) {
+ return node instanceof NameRefNode && "?".equals(((NameRefNode)
node).getText());
+ }
+
+ /**
+ * Recursively checks if a BaseNode or its children contain a reference to
'?'
+ */
+ private boolean containsQuestionMarkReference(BaseNode node) {
+ if (isPlainQuestionMark(node)) {
+ return true;
+ }
+ if (node.getChildrenNode() != null) {
+ for (ASTNode child : node.getChildrenNode()) {
+ if (child instanceof BaseNode &&
containsQuestionMarkReference((BaseNode) child)) {
+ return true;
+ }
+ }
Review Comment:
The loop iterates over children nodes but doesn't check if individual child
elements are null before casting to BaseNode. While this may not be a problem
in practice, other parts of the codebase (e.g., UnaryTestListNode.evaluate() at
line 96) explicitly check for null elements in similar scenarios. Adding a null
check here would make the code more defensive and consistent with patterns
elsewhere in the codebase.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java:
##########
@@ -224,77 +223,117 @@ 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)) {
Review Comment:
The method `containsQuestionMarkReference()` is called every time
`evaluateRightValue()` is invoked, which recursively traverses the entire AST
tree of the value node. This could have a performance impact in scenarios where
unary tests are evaluated frequently (e.g., decision tables with many rows).
Consider caching the result of `containsQuestionMarkReference()` in a field
that is computed once during construction or lazily on first use. This would
avoid repeated AST traversals.
```suggestion
private Boolean cachedContainsQuestionMarkInValue;
private boolean containsQuestionMarkInValue() {
if (cachedContainsQuestionMarkInValue == null) {
cachedContainsQuestionMarkInValue =
containsQuestionMarkReference(value);
}
return cachedContainsQuestionMarkInValue;
}
private Object evaluateRightValue(EvaluationContext context, Object
left) {
Object right;
// set the value if the expression contains '?'
if (containsQuestionMarkInValue()) {
```
--
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]
