You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "ChinchuAjith (via GitHub)" <gi...@apache.org> on 2026/02/02 10:55:02 UTC
[PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
ChinchuAjith opened a new pull request, #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577
Fixes https://github.com/apache/incubator-kie-issues/issues/2228
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Decision Engine: Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2797890575
##########
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) {
Review Comment:
unit test added
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Decision Engine: Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#issuecomment-3897570414
@gitgabrio Can you please review?
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Decision Engine: Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2815781479
##########
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)
);
}
+ Object evaluateRightValue(EvaluationContext context, Object left) {
+ Object right;
+ // set the value if the expression contains ('?') question mark
+ if (containsQuestionMarkReference(value)) {
+ Object existing = context.getValue("?");
Review Comment:
I've the impression this is a flaw in the overall logic
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Decision Engine: Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer merged PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2783123964
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java:
##########
@@ -224,77 +224,123 @@ 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.getMapping("?");
+ if (Objects.equals(existing, left)) {
+ right = value.evaluate(context);
+ } else {
+ context.enterFrame();
+ try {
+ context.setMapping("?", left);
Review Comment:
Yes @gitgabrio . Thanks for pointing this out. I’ve removed the newly added set/getMapping frame and using the existing getValue/setValue frames instead.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Decision Engine: Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2792285945
##########
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:
Checking...
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Decision Engine: Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2792504452
##########
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:
@ChinchuAjith Ok, that is the same suggestion it previously shared, right? That wasn't applied because not thread-safe, right?
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Decision Engine: Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2815778242
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/lang/ast/UnaryTestNodeTest.java:
##########
@@ -93,4 +101,104 @@ private static Stream<Arguments> provideElementsForEqualityTest() {
Arguments.of(null, 42, false, "Left null, right non-null")
);
}
+
+ /**
+ * Tests for evaluateRightValue method - without question mark
+ */
+
+ @Test
+ void testEvaluateRightValue_SimpleValue_WithoutQuestionMark() {
+ NumberNode valueNode = new NumberNode(BigDecimal.valueOf(42), "42");
+ UnaryTestNode unaryTestNode = new UnaryTestNode(UnaryTestNode.UnaryOperator.EQ, valueNode);
+
+ EvaluationContext context = new EvaluationContextImpl(ClassLoaderUtil.findDefaultClassLoader(), new FEELEventListenersManager(), FEELDialect.FEEL);
+ Object left = BigDecimal.valueOf(100);
+ Object result = unaryTestNode.evaluateRightValue(context, left);
+ assertThat(result).isEqualTo(BigDecimal.valueOf(42));
+ }
+
+ @Test
+ void testEvaluateRightValue_WithQuestionMark_NotInContext() {
+ NameRefNode questionMarkNode = new NameRefNode(BuiltInType.UNKNOWN, "?");
+ UnaryTestNode unaryTestNode = new UnaryTestNode(UnaryTestNode.UnaryOperator.EQ, questionMarkNode);
+
+ EvaluationContext context = new EvaluationContextImpl(ClassLoaderUtil.findDefaultClassLoader(), new FEELEventListenersManager(), FEELDialect.FEEL);
+ Object left = BigDecimal.valueOf(123);
+ Object result = unaryTestNode.evaluateRightValue(context, left);
+ assertThat(result).isEqualTo(BigDecimal.valueOf(123));
+ }
+
+ @Test
+ void testEvaluateRightValue_WithQuestionMark_AlreadyInContext_SameValue() {
Review Comment:
I've the impression this test demonstrate a mistake in the overall logic
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2753893890
##########
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:
@gitgabrio @ChinchuAjith Please consider this comment carefully.
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2769213071
##########
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:
added map instead of cache
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2791994511
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/EvaluationContext.java:
##########
@@ -74,4 +74,5 @@ default void setPerformRuntimeTypeCheck(boolean performRuntimeTypeCheck) {
default void enterFrame(int size) {
// Default implementation does nothing
}
+
Review Comment:
@ChinchuAjith I guess you can remove this new added line, so the EvaluationContext.java will no longer listened here.
##########
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) {
Review Comment:
@ChinchuAjith Can you please add UnitTests to verify those new introduced methods? Thanks!
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2769280394
##########
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:
removed extra blank line
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Decision Engine: Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2792504452
##########
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:
@ChinchuAjith Ok, that is the same suggestion it previously shared, right?
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Decision Engine: Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2769269552
##########
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:
changed to ConcurrentHashMap implementation
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Decision Engine: Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2815778242
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/lang/ast/UnaryTestNodeTest.java:
##########
@@ -93,4 +101,104 @@ private static Stream<Arguments> provideElementsForEqualityTest() {
Arguments.of(null, 42, false, "Left null, right non-null")
);
}
+
+ /**
+ * Tests for evaluateRightValue method - without question mark
+ */
+
+ @Test
+ void testEvaluateRightValue_SimpleValue_WithoutQuestionMark() {
+ NumberNode valueNode = new NumberNode(BigDecimal.valueOf(42), "42");
+ UnaryTestNode unaryTestNode = new UnaryTestNode(UnaryTestNode.UnaryOperator.EQ, valueNode);
+
+ EvaluationContext context = new EvaluationContextImpl(ClassLoaderUtil.findDefaultClassLoader(), new FEELEventListenersManager(), FEELDialect.FEEL);
+ Object left = BigDecimal.valueOf(100);
+ Object result = unaryTestNode.evaluateRightValue(context, left);
+ assertThat(result).isEqualTo(BigDecimal.valueOf(42));
+ }
+
+ @Test
+ void testEvaluateRightValue_WithQuestionMark_NotInContext() {
+ NameRefNode questionMarkNode = new NameRefNode(BuiltInType.UNKNOWN, "?");
+ UnaryTestNode unaryTestNode = new UnaryTestNode(UnaryTestNode.UnaryOperator.EQ, questionMarkNode);
+
+ EvaluationContext context = new EvaluationContextImpl(ClassLoaderUtil.findDefaultClassLoader(), new FEELEventListenersManager(), FEELDialect.FEEL);
+ Object left = BigDecimal.valueOf(123);
+ Object result = unaryTestNode.evaluateRightValue(context, left);
+ assertThat(result).isEqualTo(BigDecimal.valueOf(123));
+ }
+
+ @Test
+ void testEvaluateRightValue_WithQuestionMark_AlreadyInContext_SameValue() {
Review Comment:
I've the impression this test demonstrate a mistake in the overall logic
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2782075013
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/EvaluationContext.java:
##########
@@ -74,4 +74,12 @@ default void setPerformRuntimeTypeCheck(boolean performRuntimeTypeCheck) {
default void enterFrame(int size) {
// Default implementation does nothing
}
+
Review Comment:
HI @ChinchuAjith
TBH, I'm a bit doubtful of that 🤔
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Decision Engine: Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2815781479
##########
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)
);
}
+ Object evaluateRightValue(EvaluationContext context, Object left) {
+ Object right;
+ // set the value if the expression contains ('?') question mark
+ if (containsQuestionMarkReference(value)) {
+ Object existing = context.getValue("?");
Review Comment:
I've the impression this is a flaw in the overall logic
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2769259667
##########
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:
changed to (Objects.equals(existing, left) which is null safe
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2769273301
##########
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:
blank line added
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2769236119
##########
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:
changed to setMapping
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2769285857
##########
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:
space added after if
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "kie-ci3 (via GitHub)" <gi...@apache.org>.
kie-ci3 commented on PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#issuecomment-3835243326
**PR job** `#2` was: **UNSTABLE**
Possible explanation: This should be test failures
<details>
<summary><b>Reproducer</b></summary>
build-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-drools -u https://github.com/apache/incubator-kie-drools/pull/6577 --skipParallelCheckout
NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution
</details>
Please look here: https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6577/2/display/redirect
**Test results:**
- PASSED: 24089
- FAILED: 1
Those are the test failures:
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6577/2/testReport/org.kie.dmn.core/DMNDecisionTableRuntimeTest/decisionTablesQuestionMarkOnly(boolean)[1]/">org.kie.dmn.core.DMNDecisionTableRuntimeTest.decisionTablesQuestionMarkOnly(boolean)[1]</a></summary>
<br/>expected: FAILED<br/> but was: SUCCEEDED
</details>
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] Handling Question mark Operator in decision table unary test expressions [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #6577:
URL: https://github.com/apache/incubator-kie-drools/pull/6577#discussion_r2782094660
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java:
##########
@@ -224,77 +224,123 @@ 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.getMapping("?");
+ if (Objects.equals(existing, left)) {
+ right = value.evaluate(context);
+ } else {
+ context.enterFrame();
+ try {
+ context.setMapping("?", left);
Review Comment:
HI @ChinchuAjith , related to previous comment, I'm doubtufl about that:
1. this is not a real mapping, i.e. if the key is always "?", we'll always have at most one entry - my idea was to have a revers mapping, i.e. for each referred element (e.g. for each inputText) verify if there is a mapping "?" - the key should be - somehow, the identifier of the element - mind you I'm not sure it could be done!
2. considering the above, the "setMapping" and "setValue" ends up having exactly the same behavior, so they are redundant
3. I'm, not sure about all the possible implications of creating a new frame here - it could be a nice solution, but then, maybe that context.setValue could be enough (no need for the mapping at all)
Am I clear ? Does this make sense ?
--
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: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org