Copilot commented on code in PR #6770:
URL:
https://github.com/apache/incubator-kie-drools/pull/6770#discussion_r3482662004
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/dialectHandlers/BFEELDialectHandler.java:
##########
@@ -232,79 +230,55 @@ public Map<CheckedPredicate, BiFunction<Object, Object,
Object>> getGteOperation
: null;
if (greater == null && equal == null) {
- return Boolean.FALSE; // BFEEL default
+ return Boolean.FALSE; // BFEEL default for
incompatible types
}
if (Boolean.TRUE.equals(greater) ||
Boolean.TRUE.equals(equal)) {
return Boolean.TRUE;
}
return Boolean.FALSE;
});
Review Comment:
EqExecutor.instance().evaluate(left, right, ctx) is invoked twice to compute
`equal`, and the subsequent `greater == null` check is effectively dead in
BFEEL because this dialect’s compare() uses FALSE fallbacks and never returns
null for non-null operands. Caching the EqExecutor result and simplifying the
return reduces work and clarifies the intended fallback-to-false behavior for
incompatible types.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/dialectHandlers/BFEELDialectHandler.java:
##########
@@ -315,50 +289,33 @@ public Map<CheckedPredicate, BiFunction<Object, Object,
Object>> getLteOperation
: null;
if (less == null && equal == null) {
- return Boolean.FALSE; // BFEEL default
+ return Boolean.FALSE; // BFEEL default for
incompatible types
}
if (Boolean.TRUE.equals(less) ||
Boolean.TRUE.equals(equal)) {
return Boolean.TRUE;
}
return Boolean.FALSE;
});
Review Comment:
Same as in getGteOperations(): EqExecutor.instance().evaluate(left, right,
ctx) is executed twice to compute `equal`, and the `less == null` branch is not
reachable in BFEEL because compare() uses FALSE fallbacks. Cache the EqExecutor
result and simplify the final boolean expression.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/dialectHandlers/BFEELDialectHandler.java:
##########
@@ -315,50 +289,33 @@ public Map<CheckedPredicate, BiFunction<Object, Object,
Object>> getLteOperation
: null;
if (less == null && equal == null) {
- return Boolean.FALSE; // BFEEL default
+ return Boolean.FALSE; // BFEEL default for
incompatible types
}
if (Boolean.TRUE.equals(less) ||
Boolean.TRUE.equals(equal)) {
return Boolean.TRUE;
}
return Boolean.FALSE;
});
-
- map.putAll(getCommonLteOperations(ctx));
return map;
}
@Override
public Map<CheckedPredicate, BiFunction<Object, Object, Object>>
getLtOperations(EvaluationContext ctx) {
Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
- // Non-Boolean coerces to false, so (false,false) --> false
+ // BFEEL: null returns FALSE (not null like standard FEEL)
map.put(
- new CheckedPredicate((left, right) -> {
- Boolean leftBool = (left instanceof Boolean) ? (Boolean)
left : Boolean.FALSE;
- Object rightValue = evalRight(right, ctx);
- Boolean rightBool = (rightValue instanceof Boolean) ?
(Boolean) rightValue : Boolean.FALSE;
- return Boolean.FALSE.equals(leftBool) &&
Boolean.FALSE.equals(rightBool);
- }, false),
+ new CheckedPredicate((left, right) -> left == null || right ==
null, false),
(left, right) -> Boolean.FALSE);
- // General non-Boolean coercion to false
- map.put(
- new CheckedPredicate((left, right) -> (!(left instanceof
Boolean) || !(right instanceof Boolean)), false),
- (left, right) -> {
- Boolean leftBool = (left instanceof Boolean) ? (Boolean)
left : Boolean.FALSE;
- Boolean rightBool = (right instanceof Boolean) ? (Boolean)
right : Boolean.FALSE;
- return leftBool || rightBool;
- });
-
- // Numeric/comparable < logic
+ // BFEEL: All other comparisons (numeric, dates, strings, Booleans,
etc.)
map.put(
new CheckedPredicate((left, right) -> true, false),
(left, right) -> {
Boolean less = compare(left, right,
(l, r) -> l.compareTo(r) < 0);
return Objects.requireNonNullElse(less, Boolean.FALSE);
});
Review Comment:
Similarly, compare(left, right, …) in BFEEL uses FALSE fallbacks, so `less`
should not be null and Objects.requireNonNullElse is redundant.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/dialectHandlers/BFEELDialectHandler.java:
##########
@@ -232,79 +230,55 @@ public Map<CheckedPredicate, BiFunction<Object, Object,
Object>> getGteOperation
: null;
if (greater == null && equal == null) {
- return Boolean.FALSE; // BFEEL default
+ return Boolean.FALSE; // BFEEL default for
incompatible types
}
if (Boolean.TRUE.equals(greater) ||
Boolean.TRUE.equals(equal)) {
return Boolean.TRUE;
}
return Boolean.FALSE;
});
-
- // Fall back to common >= operations for all other cases
- map.putAll(getCommonGteOperations(ctx));
return map;
}
@Override
public Map<CheckedPredicate, BiFunction<Object, Object, Object>>
getGtOperations(EvaluationContext ctx) {
Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
- // Any non-Boolean coerces to false, so (false,false) --> false
+ // BFEEL: null returns FALSE (not null like standard FEEL)
map.put(
- new CheckedPredicate((left, right) -> {
- Boolean leftBool = (left instanceof Boolean) ? (Boolean)
left : Boolean.FALSE;
- Object rightValue = evalRight(right, ctx);
- Boolean rightBool = (rightValue instanceof Boolean) ?
(Boolean) rightValue : Boolean.FALSE;
- return Boolean.FALSE.equals(leftBool) &&
Boolean.FALSE.equals(rightBool);
- }, false),
+ new CheckedPredicate((left, right) -> left == null || right ==
null, false),
(left, right) -> Boolean.FALSE);
- // non-Boolean coercion to false
- map.put(
- new CheckedPredicate((left, right) -> (!(left instanceof
Boolean) || !(right instanceof Boolean)), false),
- (left, right) -> {
- Boolean leftBool = (left instanceof Boolean) ? (Boolean)
left : Boolean.FALSE;
- Boolean rightBool = (right instanceof Boolean) ? (Boolean)
right : Boolean.FALSE;
- return leftBool || rightBool;
- });
-
- // numeric/comparable > logic
+ // BFEEL: All other comparisons (numeric, dates, strings, Booleans,
etc.)
map.put(
new CheckedPredicate((left, right) -> true, false),
(left, right) -> {
Boolean greater = compare(left, right,
(l, r) -> l.compareTo(r) > 0);
return Objects.requireNonNullElse(greater, Boolean.FALSE);
});
Review Comment:
In BFEELDialectHandler, compare(left, right, …) already uses FALSE fallbacks
(including for incompatible types), so `greater` should never be null here. The
Objects.requireNonNullElse wrapper is redundant and can be removed to make the
intent clearer.
--
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]
