Copilot commented on code in PR #6537:
URL:
https://github.com/apache/incubator-kie-drools/pull/6537#discussion_r2585125934
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/impl/RangeImpl.java:
##########
@@ -232,4 +239,11 @@ private String withUndefinedtoString() {
sb.append(" )");
return sb.toString();
}
+
+ private static Boolean checkIsAssignable(Comparable left, Object right,
BiPredicate<Comparable, Comparable> op) {
+ if (right != null &&
left.getClass().isAssignableFrom(right.getClass())) { // short path
+ return op.test(left, (Comparable) right);
+ }
+ return null; // shortcut not applicable
+ }
Review Comment:
Commented-out code and old implementation should be removed. This includes
both the old method calls and the `checkIsAssignable` helper method that
appears to be unused.
```suggestion
```
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/util/DateTimeEvalHelper.java:
##########
@@ -51,10 +45,10 @@ public static String toParsableString(TemporalAccessor
temporalAccessor) {
}
/**
- * DMNv1.2 10.3.2.3.6 date-time, valuedt(date and time), for use in this
{@link BooleanEvalHelper#compare(Object, Object, EvaluationContext,
BiPredicate)}
+ * DMNv1.2 10.3.2.3.6 date-time, valuedt(date and time), for use in this
{@link compare(Object, Object, EvaluationContext, BiPredicate)}
Review Comment:
Incorrect JavaDoc reference. The comment refers to `{@link
BooleanEvalHelper#compare(Object, Object, EvaluationContext, BiPredicate)}` but
this method signature doesn't exist anymore. The reference should be updated or
the link syntax should be corrected to match the new method signature.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/BetweenNode.java:
##########
@@ -107,18 +109,30 @@ public Object evaluate(EvaluationContext ctx) {
ctx.notifyEvt(astEvent(severity, Msg.createMessage(Msg.IS_NULL,
"end")));
problem = true;
}
- if (problem && ctx.getFEELDialect() != FEELDialect.BFEEL) return null;
+ if (problem && ctx.getFEELDialect() != FEELDialect.BFEEL)
+ return null;
+
+ /*
+ * Object gte = InfixExecutorUtils.or(BooleanEvalHelper.compare(o_val,
o_s, ctx.getFEELDialect(), (l, r) -> l.compareTo(r) > 0),
+ * BooleanEvalHelper.isEqual(o_val, o_s, ctx.getFEELDialect()),
+ * ctx);
+ */ // do not use Java || to avoid potential NPE due to FEEL 3vl.
+ //Object gte = GteExecutor.instance().evaluate(o_val, o_s, ctx);
+ DialectHandler handler = DialectHandlerFactory.getHandler(ctx);
+ Object gte = handler.executeGte(o_val, o_s, ctx);
- Object gte = InfixExecutorUtils.or(BooleanEvalHelper.compare(o_val,
o_s, ctx.getFEELDialect(), (l, r) -> l.compareTo(r) > 0),
- BooleanEvalHelper.isEqual(o_val,
o_s, ctx.getFEELDialect()),
- ctx); // do not use Java || to
avoid potential NPE due to FEEL 3vl.
if (gte == null) {
ctx.notifyEvt(astEvent(Severity.ERROR,
Msg.createMessage(Msg.X_TYPE_INCOMPATIBLE_WITH_Y_TYPE, "value", "start")));
}
- Object lte = InfixExecutorUtils.or(BooleanEvalHelper.compare(o_val,
o_e, ctx.getFEELDialect(), (l, r) -> l.compareTo(r) < 0),
- BooleanEvalHelper.isEqual(o_val, o_e, ctx.getFEELDialect()),
- ctx); // do not use Java || to avoid potential NPE due to FEEL
3vl.
+ /*
+ * Object lte = InfixExecutorUtils.or(BooleanEvalHelper.compare(o_val,
o_e, ctx.getFEELDialect(), (l, r) -> l.compareTo(r) < 0),
+ * BooleanEvalHelper.isEqual(o_val, o_e, ctx.getFEELDialect()),
+ * ctx); // do not use Java || to avoid potential NPE due to FEEL 3vl.
+ */
Review Comment:
Commented-out code blocks should be removed to improve code cleanliness.
This includes old implementations of comparison operations that have been
replaced.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/dialectHandlers/DefaultDialectHandler.java:
##########
@@ -27,23 +27,29 @@
import java.time.LocalTime;
import java.time.chrono.ChronoPeriod;
import java.time.temporal.Temporal;
+import java.time.temporal.TemporalAccessor;
import java.time.temporal.TemporalAmount;
-import java.util.LinkedHashMap;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Optional;
+import java.time.temporal.TemporalQueries;
+import java.util.*;
Review Comment:
[nitpick] Using wildcard imports (`import java.time.*;` and `import
java.util.*;`) is discouraged. Consider replacing with explicit imports to make
dependencies clearer.
```suggestion
import java.util.Map;
import java.util.LinkedHashMap;
```
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/util/BooleanEvalHelper.java:
##########
@@ -62,86 +50,149 @@ public static Boolean getBooleanOrNull(Object value) {
* @param op
* @return
*/
- public static Boolean compare(Object left, Object right, FEELDialect
feelDialect, BiPredicate<Comparable, Comparable> op) {
- if (left == null || right == null) {
- return getBooleanOrDialectDefault(null, feelDialect);
- }
- if (left instanceof ChronoPeriod && right instanceof ChronoPeriod) {
- // periods have special compare semantics in FEEL as it ignores
"days". Only months and years are compared
- Long l = ComparablePeriod.toTotalMonths((ChronoPeriod) left);
- Long r = ComparablePeriod.toTotalMonths((ChronoPeriod) right);
- return op.test(l, r);
- }
- if (left instanceof TemporalAccessor && right instanceof
TemporalAccessor) {
- // Handle specific cases when both time / datetime
- TemporalAccessor l = (TemporalAccessor) left;
- TemporalAccessor r = (TemporalAccessor) right;
- if (BuiltInTypeUtils.determineTypeFromInstance(left) ==
BuiltInType.TIME && BuiltInTypeUtils.determineTypeFromInstance(right) ==
BuiltInType.TIME) {
- return op.test(valuet(l), valuet(r));
- } else if (BuiltInTypeUtils.determineTypeFromInstance(left) ==
BuiltInType.DATE_TIME && BuiltInTypeUtils.determineTypeFromInstance(right) ==
BuiltInType.DATE_TIME) {
- return op.test(valuedt(l, r.query(TemporalQueries.zone())),
valuedt(r, l.query(TemporalQueries.zone())));
- }
- }
- if (left instanceof Number && right instanceof Number) {
- // Handle specific cases when both are Number, converting both to
BigDecimal
- BigDecimal l = getBigDecimalOrNull(left);
- BigDecimal r = getBigDecimalOrNull(right);
- return op.test(l, r);
- }
- // last fallback:
- if ((left instanceof String && right instanceof String) ||
- (left instanceof Boolean && right instanceof Boolean) ||
- (left instanceof Comparable &&
left.getClass().isAssignableFrom(right.getClass()))) {
- Comparable<?> l = (Comparable<?>) left;
- Comparable<?> r = (Comparable<?>) right;
- return op.test(l, r);
- }
- return getBooleanOrDialectDefault(null, feelDialect);
- }
+ /*
+ * public static Boolean compare(Object left, Object right,
BiPredicate<Comparable, Comparable> op, Supplier<Boolean> nullFallback,
+ * Supplier<Boolean> defaultFallback) {
+ * if (nullFallback == null || defaultFallback == null) {
+ * throw new IllegalArgumentException("Fallback suppliers must not be
null");
+ * }
+ * if (left == null || right == null) {
+ * return nullFallback.get();
+ * }
+ * if (left instanceof ChronoPeriod && right instanceof ChronoPeriod) {
+ * // periods have special compare semantics in FEEL as it ignores "days".
Only months and years are compared
+ * Long l = ComparablePeriod.toTotalMonths((ChronoPeriod) left);
+ * Long r = ComparablePeriod.toTotalMonths((ChronoPeriod) right);
+ * return op.test(l, r);
+ * }
+ * if (left instanceof TemporalAccessor && right instanceof
TemporalAccessor) {
+ * // Handle specific cases when both time / datetime
+ * TemporalAccessor l = (TemporalAccessor) left;
+ * TemporalAccessor r = (TemporalAccessor) right;
+ * if (BuiltInTypeUtils.determineTypeFromInstance(left) ==
BuiltInType.TIME && BuiltInTypeUtils.determineTypeFromInstance(right) ==
BuiltInType.TIME) {
+ * return op.test(valuet(l), valuet(r));
+ * } else if (BuiltInTypeUtils.determineTypeFromInstance(left) ==
BuiltInType.DATE_TIME && BuiltInTypeUtils.determineTypeFromInstance(right) ==
BuiltInType.DATE_TIME) {
+ * return op.test(valuedt(l, r.query(TemporalQueries.zone())), valuedt(r,
l.query(TemporalQueries.zone())));
+ * }
+ * }
+ * if (left instanceof Number && right instanceof Number) {
+ * // Handle specific cases when both are Number, converting both to
BigDecimal
+ * BigDecimal l = getBigDecimalOrNull(left);
+ * BigDecimal r = getBigDecimalOrNull(right);
+ * return op.test(l, r);
+ * }
+ * // last fallback:
+ * if ((left instanceof String && right instanceof String) ||
+ * (left instanceof Boolean && right instanceof Boolean) ||
+ * (left instanceof Comparable &&
left.getClass().isAssignableFrom(right.getClass()))) {
+ * Comparable<?> l = (Comparable<?>) left;
+ * Comparable<?> r = (Comparable<?>) right;
+ * return op.test(l, r);
+ * }
+ * return defaultFallback.get();
+ * }
+ */
Review Comment:
Large block of commented-out code should be removed rather than kept in the
codebase. If this code might be needed for reference, it should be documented
in a comment or retrievable from version control history.
```suggestion
```
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/BooleanEvalHelperTest.java:
##########
@@ -18,39 +18,32 @@
*/
package org.kie.dmn.feel.util;
-import java.math.BigDecimal;
-import java.math.BigInteger;
-import java.util.Date;
-
import org.junit.jupiter.api.Test;
import org.kie.dmn.feel.lang.FEELDialect;
-import org.mockito.MockedStatic;
-import org.mockito.Mockito;
import static org.assertj.core.api.Assertions.assertThat;
import static
org.kie.dmn.feel.util.BooleanEvalHelper.getBooleanOrDialectDefault;
-import static org.kie.dmn.feel.util.BooleanEvalHelper.getFalseOrDialectDefault;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.times;
class BooleanEvalHelperTest {
-
- @Test
- void numericValuesComparative() {
- assertThat(BooleanEvalHelper.compare(BigDecimal.valueOf(1),
BigDecimal.valueOf(2), FEELDialect.FEEL, (l, r) -> l.compareTo(r) <
0)).isTrue();
- assertThat(BooleanEvalHelper.compare(1.0, 2.0, FEELDialect.FEEL,(l, r)
-> l.compareTo(r) < 0)).isTrue();
- assertThat(BooleanEvalHelper.compare(1, 2, FEELDialect.FEEL, (l, r) ->
l.compareTo(r) > 0)).isFalse();
- assertThat(BooleanEvalHelper.compare(BigDecimal.valueOf(1), 2,
FEELDialect.FEEL,(l, r) -> l.compareTo(r) > 0)).isFalse();
- assertThat(BooleanEvalHelper.compare(1, BigDecimal.valueOf(2),
FEELDialect.FEEL,(l, r) -> l.compareTo(r) < 0)).isTrue();
- assertThat(BooleanEvalHelper.compare(BigDecimal.valueOf(1), 2.3,
FEELDialect.FEEL,(l, r) -> l.compareTo(r) == 0)).isFalse();
- assertThat(BooleanEvalHelper.compare(1.2, BigDecimal.valueOf(1.2),
FEELDialect.FEEL, (l, r) -> l.compareTo(r) == 0)).isTrue();
- assertThat(BooleanEvalHelper.compare(BigDecimal.valueOf(1), 0L,
FEELDialect.FEEL, (l, r) -> l.compareTo(r) > 0)).isTrue();
- assertThat(BooleanEvalHelper.compare(10L, BigDecimal.valueOf(2),
FEELDialect.FEEL, (l, r) -> l.compareTo(r) < 0)).isFalse();
- assertThat(BooleanEvalHelper.compare(BigInteger.valueOf(1),
BigInteger.valueOf(2), FEELDialect.FEEL, (l, r) -> l.compareTo(r) ==
0)).isFalse();
- assertThat(BooleanEvalHelper.compare(BigInteger.valueOf(1), 2,
FEELDialect.FEEL, (l, r) -> l.compareTo(r) < 0)).isTrue();
- assertThat(BooleanEvalHelper.compare(BigInteger.valueOf(1), 2.3,
FEELDialect.FEEL,(l, r) -> l.compareTo(r) == 0)).isFalse();
- }
+ //TODO Will move to FeelDialectTest clss
+ /*
+ * @Test
+ * void numericValuesComparative() {
+ * assertThat(BooleanEvalHelper.compare(BigDecimal.valueOf(1),
BigDecimal.valueOf(2), (l, r) -> l.compareTo(r) < 0, () -> null, () ->
null)).isTrue();
+ * assertThat(BooleanEvalHelper.compare(1.0, 2.0, (l, r) -> l.compareTo(r)
< 0, () -> null, () -> null)).isTrue();
+ * assertThat(BooleanEvalHelper.compare(1, 2, (l, r) -> l.compareTo(r) >
0, () -> null, () -> null)).isFalse();
+ * assertThat(BooleanEvalHelper.compare(BigDecimal.valueOf(1), 2, (l, r)
-> l.compareTo(r) > 0, () -> null, () -> null)).isFalse();
+ * assertThat(BooleanEvalHelper.compare(1, BigDecimal.valueOf(2), (l, r)
-> l.compareTo(r) < 0, () -> null, () -> null)).isTrue();
+ * assertThat(BooleanEvalHelper.compare(BigDecimal.valueOf(1), 2.3, (l, r)
-> l.compareTo(r) == 0, () -> null, () -> null)).isFalse();
+ * assertThat(BooleanEvalHelper.compare(1.2, BigDecimal.valueOf(1.2), (l,
r) -> l.compareTo(r) == 0, () -> null, () -> null)).isTrue();
+ * assertThat(BooleanEvalHelper.compare(BigDecimal.valueOf(1), 0L, (l, r)
-> l.compareTo(r) > 0, () -> null, () -> null)).isTrue();
+ * assertThat(BooleanEvalHelper.compare(10L, BigDecimal.valueOf(2), (l, r)
-> l.compareTo(r) < 0, () -> null, () -> null)).isFalse();
+ * assertThat(BooleanEvalHelper.compare(BigInteger.valueOf(1),
BigInteger.valueOf(2), (l, r) -> l.compareTo(r) == 0, () -> null, () ->
null)).isFalse();
+ * assertThat(BooleanEvalHelper.compare(BigInteger.valueOf(1), 2, (l, r)
-> l.compareTo(r) < 0, () -> null, () -> null)).isTrue();
+ * assertThat(BooleanEvalHelper.compare(BigInteger.valueOf(1), 2.3, (l, r)
-> l.compareTo(r) == 0, () -> null, () -> null)).isFalse();
+ * }
+ */
Review Comment:
TODO comment indicates this code will be moved but the commented test code
should be removed from this file. Either move it now or create a tracking issue
and remove the commented code.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/util/BooleanEvalHelper.java:
##########
@@ -62,86 +50,149 @@ public static Boolean getBooleanOrNull(Object value) {
* @param op
* @return
*/
- public static Boolean compare(Object left, Object right, FEELDialect
feelDialect, BiPredicate<Comparable, Comparable> op) {
- if (left == null || right == null) {
- return getBooleanOrDialectDefault(null, feelDialect);
- }
- if (left instanceof ChronoPeriod && right instanceof ChronoPeriod) {
- // periods have special compare semantics in FEEL as it ignores
"days". Only months and years are compared
- Long l = ComparablePeriod.toTotalMonths((ChronoPeriod) left);
- Long r = ComparablePeriod.toTotalMonths((ChronoPeriod) right);
- return op.test(l, r);
- }
- if (left instanceof TemporalAccessor && right instanceof
TemporalAccessor) {
- // Handle specific cases when both time / datetime
- TemporalAccessor l = (TemporalAccessor) left;
- TemporalAccessor r = (TemporalAccessor) right;
- if (BuiltInTypeUtils.determineTypeFromInstance(left) ==
BuiltInType.TIME && BuiltInTypeUtils.determineTypeFromInstance(right) ==
BuiltInType.TIME) {
- return op.test(valuet(l), valuet(r));
- } else if (BuiltInTypeUtils.determineTypeFromInstance(left) ==
BuiltInType.DATE_TIME && BuiltInTypeUtils.determineTypeFromInstance(right) ==
BuiltInType.DATE_TIME) {
- return op.test(valuedt(l, r.query(TemporalQueries.zone())),
valuedt(r, l.query(TemporalQueries.zone())));
- }
- }
- if (left instanceof Number && right instanceof Number) {
- // Handle specific cases when both are Number, converting both to
BigDecimal
- BigDecimal l = getBigDecimalOrNull(left);
- BigDecimal r = getBigDecimalOrNull(right);
- return op.test(l, r);
- }
- // last fallback:
- if ((left instanceof String && right instanceof String) ||
- (left instanceof Boolean && right instanceof Boolean) ||
- (left instanceof Comparable &&
left.getClass().isAssignableFrom(right.getClass()))) {
- Comparable<?> l = (Comparable<?>) left;
- Comparable<?> r = (Comparable<?>) right;
- return op.test(l, r);
- }
- return getBooleanOrDialectDefault(null, feelDialect);
- }
+ /*
+ * public static Boolean compare(Object left, Object right,
BiPredicate<Comparable, Comparable> op, Supplier<Boolean> nullFallback,
+ * Supplier<Boolean> defaultFallback) {
+ * if (nullFallback == null || defaultFallback == null) {
+ * throw new IllegalArgumentException("Fallback suppliers must not be
null");
+ * }
+ * if (left == null || right == null) {
+ * return nullFallback.get();
+ * }
+ * if (left instanceof ChronoPeriod && right instanceof ChronoPeriod) {
+ * // periods have special compare semantics in FEEL as it ignores "days".
Only months and years are compared
+ * Long l = ComparablePeriod.toTotalMonths((ChronoPeriod) left);
+ * Long r = ComparablePeriod.toTotalMonths((ChronoPeriod) right);
+ * return op.test(l, r);
+ * }
+ * if (left instanceof TemporalAccessor && right instanceof
TemporalAccessor) {
+ * // Handle specific cases when both time / datetime
+ * TemporalAccessor l = (TemporalAccessor) left;
+ * TemporalAccessor r = (TemporalAccessor) right;
+ * if (BuiltInTypeUtils.determineTypeFromInstance(left) ==
BuiltInType.TIME && BuiltInTypeUtils.determineTypeFromInstance(right) ==
BuiltInType.TIME) {
+ * return op.test(valuet(l), valuet(r));
+ * } else if (BuiltInTypeUtils.determineTypeFromInstance(left) ==
BuiltInType.DATE_TIME && BuiltInTypeUtils.determineTypeFromInstance(right) ==
BuiltInType.DATE_TIME) {
+ * return op.test(valuedt(l, r.query(TemporalQueries.zone())), valuedt(r,
l.query(TemporalQueries.zone())));
+ * }
+ * }
+ * if (left instanceof Number && right instanceof Number) {
+ * // Handle specific cases when both are Number, converting both to
BigDecimal
+ * BigDecimal l = getBigDecimalOrNull(left);
+ * BigDecimal r = getBigDecimalOrNull(right);
+ * return op.test(l, r);
+ * }
+ * // last fallback:
+ * if ((left instanceof String && right instanceof String) ||
+ * (left instanceof Boolean && right instanceof Boolean) ||
+ * (left instanceof Comparable &&
left.getClass().isAssignableFrom(right.getClass()))) {
+ * Comparable<?> l = (Comparable<?>) left;
+ * Comparable<?> r = (Comparable<?>) right;
+ * return op.test(l, r);
+ * }
+ * return defaultFallback.get();
+ * }
+ */
/**
- * Compares left and right for equality applying FEEL semantics to
specific data types
+ * Compares left and right operands using the given predicate and returns
TRUE/FALSE accordingly
*
* @param left
* @param right
+ * @param op
* @return
*/
- public static Boolean isEqual(Object left, Object right, FEELDialect
feelDialect) {
- if (left == null || right == null) {
- return left == right;
- }
-
- // spec defines that "a=[a]", i.e., singleton collections should be
treated as the single element
- // and vice-versa
- if (left instanceof Collection && !(right instanceof Collection) &&
((Collection) left).size() == 1) {
- left = ((Collection) left).toArray()[0];
- } else if (right instanceof Collection && !(left instanceof
Collection) && ((Collection) right).size() == 1) {
- right = ((Collection) right).toArray()[0];
- }
+ /*
+ * public static Boolean compare(Object left, Object right, FEELDialect
feelDialect, BiPredicate<Comparable, Comparable> op) {
+ * if (left == null || right == null) {
+ * return getBooleanOrDialectDefault(null, feelDialect);
+ * }
+ * if (left instanceof ChronoPeriod && right instanceof ChronoPeriod) {
+ * // periods have special compare semantics in FEEL as it ignores "days".
Only months and years are compared
+ * Long l = ComparablePeriod.toTotalMonths((ChronoPeriod) left);
+ * Long r = ComparablePeriod.toTotalMonths((ChronoPeriod) right);
+ * return op.test(l, r);
+ * }
+ * if (left instanceof TemporalAccessor && right instanceof
TemporalAccessor) {
+ * // Handle specific cases when both time / datetime
+ * TemporalAccessor l = (TemporalAccessor) left;
+ * TemporalAccessor r = (TemporalAccessor) right;
+ * if (BuiltInTypeUtils.determineTypeFromInstance(left) ==
BuiltInType.TIME && BuiltInTypeUtils.determineTypeFromInstance(right) ==
BuiltInType.TIME) {
+ * return op.test(valuet(l), valuet(r));
+ * } else if (BuiltInTypeUtils.determineTypeFromInstance(left) ==
BuiltInType.DATE_TIME && BuiltInTypeUtils.determineTypeFromInstance(right) ==
BuiltInType.DATE_TIME) {
+ * return op.test(valuedt(l, r.query(TemporalQueries.zone())), valuedt(r,
l.query(TemporalQueries.zone())));
+ * }
+ * }
+ * if (left instanceof Number && right instanceof Number) {
+ * // Handle specific cases when both are Number, converting both to
BigDecimal
+ * BigDecimal l = getBigDecimalOrNull(left);
+ * BigDecimal r = getBigDecimalOrNull(right);
+ * return op.test(l, r);
+ * }
+ * // last fallback:
+ * if ((left instanceof String && right instanceof String) ||
+ * (left instanceof Boolean && right instanceof Boolean) ||
+ * (left instanceof Comparable &&
left.getClass().isAssignableFrom(right.getClass()))) {
+ * Comparable<?> l = (Comparable<?>) left;
+ * Comparable<?> r = (Comparable<?>) right;
+ * return op.test(l, r);
+ * }
+ * return getBooleanOrDialectDefault(null, feelDialect);
+ * }
+ */
- if (left instanceof Range && right instanceof Range) {
- return isEqual((Range) left, (Range) right);
- } else if (left instanceof Iterable && right instanceof Iterable) {
- return isEqual((Iterable) left, (Iterable) right);
- } else if (left instanceof Map && right instanceof Map) {
- return isEqual((Map) left, (Map) right);
- } else if (left instanceof ChronoPeriod && right instanceof
ChronoPeriod) {
- // periods have special compare semantics in FEEL as it ignores
"days". Only months and years are compared
- Long l = ComparablePeriod.toTotalMonths((ChronoPeriod) left);
- Long r = ComparablePeriod.toTotalMonths((ChronoPeriod) right);
- return isEqual(l, r, feelDialect);
- } else if (left instanceof TemporalAccessor && right instanceof
TemporalAccessor) {
- // Handle specific cases when both time / datetime
- TemporalAccessor l = (TemporalAccessor) left;
- TemporalAccessor r = (TemporalAccessor) right;
- if (BuiltInTypeUtils.determineTypeFromInstance(left) ==
BuiltInType.TIME && BuiltInTypeUtils.determineTypeFromInstance(right) ==
BuiltInType.TIME) {
- return isEqual(DateTimeEvalHelper.valuet(l),
DateTimeEvalHelper.valuet(r), feelDialect);
- } else if (BuiltInTypeUtils.determineTypeFromInstance(left) ==
BuiltInType.DATE_TIME && BuiltInTypeUtils.determineTypeFromInstance(right) ==
BuiltInType.DATE_TIME) {
- return isEqual(DateTimeEvalHelper.valuedt(l,
r.query(TemporalQueries.zone())), DateTimeEvalHelper.valuedt(r,
l.query(TemporalQueries.zone())), feelDialect);
- } // fallback; continue:
- }
- return compare(left, right, feelDialect, (l, r) -> l.compareTo(r) ==
0);
- }
+ /**
+ * Compares left and right for equality applying FEEL semantics to
specific data types
+ *
+ * @param left
+ * @param right
+ * @return
+ */
+ /*
+ * public static Boolean isEqual(Object left, Object right,
Supplier<Boolean> nullFallback, Supplier<Boolean> defaultFallback) {
+ * if (nullFallback == null || defaultFallback == null) {
+ * throw new IllegalArgumentException("Fallback suppliers must not be
null");
+ * }
+ * if (left == null || right == null) {
+ * return nullFallback.get();
+ * }
+ *
+ * // spec defines that "a=[a]", i.e., singleton collections should be
treated as the single element
+ * // and vice-versa
+ * if (left instanceof Collection && !(right instanceof Collection) &&
((Collection) left).size() == 1) {
+ * left = ((Collection) left).toArray()[0];
+ * } else if (right instanceof Collection && !(left instanceof Collection)
&& ((Collection) right).size() == 1) {
+ * right = ((Collection) right).toArray()[0];
+ * }
+ *
+ * if (left instanceof Range && right instanceof Range) {
+ * return isEqual((Range) left, (Range) right);
+ * } else if (left instanceof Iterable && right instanceof Iterable) {
+ * return isEqual((Iterable) left, (Iterable) right);
+ * } else if (left instanceof Map && right instanceof Map) {
+ * return isEqual((Map) left, (Map) right);
+ * } else if (left instanceof ChronoPeriod && right instanceof
ChronoPeriod) {
+ * // periods have special compare semantics in FEEL as it ignores "days".
Only months and years are compared
+ * Long l = ComparablePeriod.toTotalMonths((ChronoPeriod) left);
+ * Long r = ComparablePeriod.toTotalMonths((ChronoPeriod) right);
+ * return isEqual(l, r, nullFallback, defaultFallback);
+ * } else if (left instanceof TemporalAccessor && right instanceof
TemporalAccessor) {
+ * // Handle specific cases when both time / datetime
+ * TemporalAccessor l = (TemporalAccessor) left;
+ * TemporalAccessor r = (TemporalAccessor) right;
+ * if (BuiltInTypeUtils.determineTypeFromInstance(left) ==
BuiltInType.TIME && BuiltInTypeUtils.determineTypeFromInstance(right) ==
BuiltInType.TIME) {
+ * return isEqual(DateTimeEvalHelper.valuet(l),
DateTimeEvalHelper.valuet(r), nullFallback, defaultFallback);
+ * } else if (BuiltInTypeUtils.determineTypeFromInstance(left) ==
BuiltInType.DATE_TIME && BuiltInTypeUtils.determineTypeFromInstance(right) ==
BuiltInType.DATE_TIME) {
+ * return isEqual(DateTimeEvalHelper.valuedt(l,
r.query(TemporalQueries.zone())), DateTimeEvalHelper.valuedt(r,
l.query(TemporalQueries.zone())), nullFallback, defaultFallback);
+ * } // fallback; continue:
+ * }
+ * //return compare(left, right, feelDialect, (l, r) -> l.compareTo(r) ==
0);
+ * // Fallback: Comparable equality
+ * return BooleanEvalHelper.compare(left, right,
+ * (l, r) -> l.compareTo(r) == 0,
+ * nullFallback,
+ * defaultFallback);
+ * }
+ */
Review Comment:
Large block of commented-out code should be removed. This includes two
variations of the `compare` and `isEqual` methods that are no longer used.
```suggestion
```
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/util/BooleanEvalHelper.java:
##########
@@ -186,18 +237,21 @@ public static Boolean
isEqualTimeInSemanticD(TemporalAccessor left, TemporalAcce
/**
* This method consider if the <code>value</code> object is a
<code>String</code>
* In that case, return the {@link String#equals(Object)} result
- * Otherwise, default to the {@link #isEqual(Object, Object, FEELDialect)}
+ * Otherwise, default to the isEqual method
Review Comment:
The comment at line 240 says "Otherwise, default to the isEqual method" but
the actual reference in the code is to `DefaultDialectHandler.isEqual`. The
comment should be more specific about which isEqual method is being called to
improve clarity.
##########
kie-dmn/kie-dmn-validation/src/main/java/org/kie/dmn/validation/dtanalysis/model/Interval.java:
##########
@@ -171,13 +167,14 @@ public boolean equals(Object obj) {
public boolean asRangeIncludes(Object param) {
// Defaulting FEELDialect to FEEL
- Boolean result = this.asRange.includes(FEELDialect.FEEL, param);
+ EvaluationContext ctx = null;
+ Boolean result = this.asRange.includes(ctx, param);
Review Comment:
Commented-out variable declaration `EvaluationContext ctx = null;` at line
170 should be removed. The variable is declared but never used, and the null
value is passed directly to the `includes` method at line 171.
```suggestion
Boolean result = this.asRange.includes(null, param);
```
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/dialectHandlers/BFEELDialectHandler.java:
##########
@@ -84,67 +85,245 @@ public Map<CheckedPredicate, BiFunction<Object, Object,
Object>> getAddOperation
public Map<CheckedPredicate, BiFunction<Object, Object, Object>>
getAndOperations(EvaluationContext ctx) {
Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
FEELDialect dialect = ctx.getFEELDialect();
+
// Special case: true AND otherwise → false
map.put(
new CheckedPredicate((left, right) -> {
- Boolean leftBool =
BooleanEvalHelper.getBooleanOrDialectDefault(left, dialect);
+ Boolean leftBool = (left instanceof Boolean) ? (Boolean)
left : Boolean.FALSE;
Object rightValue = evalRight(right, ctx);
- Boolean rightBool =
BooleanEvalHelper.getBooleanOrDialectDefault(rightValue, dialect);
+ Boolean rightBool = (rightValue instanceof Boolean) ?
(Boolean) rightValue : Boolean.FALSE;
return Boolean.TRUE.equals(leftBool) &&
Boolean.FALSE.equals(rightBool);
}, false),
(left, right) -> Boolean.FALSE);
- // Special case: otherwise AND true → false
+ // Special case: false AND true → false
map.put(
new CheckedPredicate((left, right) -> {
- Boolean leftBool =
BooleanEvalHelper.getBooleanOrDialectDefault(left, dialect);
+ Boolean leftBool = (left instanceof Boolean) ? (Boolean)
left : Boolean.FALSE;
Object rightValue = evalRight(right, ctx);
- Boolean rightBool =
BooleanEvalHelper.getBooleanOrDialectDefault(rightValue, dialect);
- return leftBool == null && Boolean.TRUE.equals(rightBool);
+ Boolean rightBool = (rightValue instanceof Boolean) ?
(Boolean) rightValue : Boolean.FALSE;
+ return Boolean.FALSE.equals(leftBool) &&
Boolean.TRUE.equals(rightBool);
}, false),
(left, right) -> Boolean.FALSE);
- // Special case: otherwise AND otherwise → false
+ // Special case: false AND false → false
map.put(
new CheckedPredicate((left, right) -> {
- Boolean leftBool =
BooleanEvalHelper.getBooleanOrDialectDefault(left, dialect);
+ Boolean leftBool = (left instanceof Boolean) ? (Boolean)
left : Boolean.FALSE;
Object rightValue = evalRight(right, ctx);
- Boolean rightBool =
BooleanEvalHelper.getBooleanOrDialectDefault(rightValue, dialect);
- return leftBool == null && Boolean.FALSE.equals(rightBool);
+ Boolean rightBool = (rightValue instanceof Boolean) ?
(Boolean) rightValue : Boolean.FALSE;
+ return Boolean.FALSE.equals(leftBool) &&
Boolean.FALSE.equals(rightBool);
}, false),
(left, right) -> Boolean.FALSE);
+
map.putAll(getCommonAndOperations(ctx));
return map;
}
@Override
public Map<CheckedPredicate, BiFunction<Object, Object, Object>>
getEqualOperations(EvaluationContext ctx) {
- return new LinkedHashMap<>(getCommonEqualOperations(ctx));
+ Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
+
+ // Shortcut: null = null → false
Review Comment:
Incorrect comment at line 127. The comment says "Shortcut: null = null →
false" but the code returns `Boolean.TRUE`. The comment should say "Shortcut:
null = null → true" to match the actual behavior.
```suggestion
// Shortcut: null = null → true
```
##########
kie-dmn/kie-dmn-validation/src/main/java/org/kie/dmn/validation/dtanalysis/model/Interval.java:
##########
@@ -18,16 +18,11 @@
*/
package org.kie.dmn.validation.dtanalysis.model;
-import java.util.ArrayDeque;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.Deque;
-import java.util.Iterator;
-import java.util.List;
+import java.util.*;
Review Comment:
[nitpick] Using wildcard imports (`import java.util.*;`) is discouraged as
it makes it unclear which specific classes are being used and can lead to
naming conflicts. Consider replacing with explicit imports.
```suggestion
import java.util.List;
import java.util.ArrayList;
import java.util.Objects;
```
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java:
##########
@@ -117,37 +117,59 @@ public UnaryTest evaluate(EvaluationContext ctx) {
}
public UnaryTest getUnaryTest() {
- switch ( operator ) {
- case LTE:
- return new UnaryTestImpl( createCompareUnaryTest( (l, r) ->
l.compareTo( r ) <= 0 ) , value.getText() );
- case LT:
- return new UnaryTestImpl( createCompareUnaryTest( (l, r) ->
l.compareTo( r ) < 0 ) , value.getText() );
- case GT:
- return new UnaryTestImpl( createCompareUnaryTest( (l, r) ->
l.compareTo( r ) > 0 ) , value.getText() );
- case GTE:
- return new UnaryTestImpl( createCompareUnaryTest( (l, r) ->
l.compareTo( r ) >= 0 ) , value.getText() );
- case EQ:
- return new UnaryTestImpl( createIsEqualUnaryTest( ) ,
value.getText() );
- case NE:
- return new UnaryTestImpl( createIsNotEqualUnaryTest( ) ,
value.getText() );
- case IN:
- return new UnaryTestImpl( createInUnaryTest() ,
value.getText() );
- case NOT:
- return new UnaryTestImpl( createNotUnaryTest() ,
value.getText() );
- case TEST:
- return new UnaryTestImpl( createBooleanUnaryTest(),
value.getText() );
- }
- return null;
- }
+ return new UnaryTestImpl((context, left) -> {
+ Object right = value.evaluate(context);
+ DialectHandler handler = DialectHandlerFactory.getHandler(context);
- private UnaryTest createCompareUnaryTest( BiPredicate<Comparable,
Comparable> op ) {
- return (context, left) -> {
- Object right = value.evaluate( context );
- // Defaulting FEELDialect to FEEL
- return BooleanEvalHelper.compare(left, right, FEELDialect.FEEL, op
);
- };
+ Object result;
+ switch (operator) {
+ case LTE:
+ result = handler.executeLte(left, right, context);
+ break;
+ case LT:
+ result = handler.executeLt(left, right, context);
+ break;
+ case GT:
+ result = handler.executeGt(left, right, context);
+ break;
+ case GTE:
+ result = handler.executeGte(left, right, context);
+ break;
+ case EQ:
+ return createIsEqualUnaryTest().apply(context, left);
+ case NE:
+ return createIsNotEqualUnaryTest().apply(context, left);
+ case IN:
+ return createInUnaryTest().apply(context, left);
+ case NOT:
+ return createNotUnaryTest().apply(context, left);
+ case TEST:
+ return createBooleanUnaryTest().apply(context, left);
+
+ default:
+ throw new UnsupportedOperationException("Unsupported
operator: " + operator);
+ }
+
+ return (result instanceof Boolean) ? (Boolean) result :
Boolean.FALSE;
+ }, value.getText());
}
+ /*
+ * private UnaryTest createCompareUnaryTest(InfixExecutor executor) {
+ * return (context, left) -> {
+ * Object right = value.evaluate(context);
+ *
+ * Object result = executor.evaluate(left, right, context);
+ * //return (result instanceof Boolean) ? (Boolean) result : null;
+ * if (result == null) {
+ * // treat null comparison as false
+ * return Boolean.FALSE;
+ * }
+ * return (result instanceof Boolean) ? (Boolean) result : Boolean.FALSE;
+ * };
+ * }
+ */
+
Review Comment:
Large commented-out code block should be removed. The old
`createCompareUnaryTest` method is no longer used after the refactoring.
```suggestion
```
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/util/BooleanEvalHelper.java:
##########
@@ -219,59 +273,50 @@ public static Boolean getBooleanOrDialectDefault(Object
rawReturn, FEELDialect f
return toReturn;
}
+ //TODO To be removed
/**
* Return <code>TRUE</code> if it is the original object or, depending on
the FEELDialect, a default value
*
- * @param rawReturn
- * @param feelDialect
+ * //* @param rawReturn
+ * //* @param feelDialect
+ *
* @return
*/
- public static Boolean getTrueOrDialectDefault(Object rawReturn,
FEELDialect feelDialect) {
- if (rawReturn instanceof Boolean bool && bool) {
- return bool;
- } else {
- return getBooleanOrDialectDefault(null, feelDialect);
- }
- }
-
- /**
- * Return <code>TRUE</code> if it is the original object or, depending on
the FEELDialect, a default value
- *
- * @param rawReturn
- * @param feelDialect
- * @return
+ /*
+ * public static Boolean getTrueOrDialectDefault(Object rawReturn,
FEELDialect feelDialect) {
+ * if (rawReturn instanceof Boolean bool && bool) {
+ * return bool;
+ * } else {
+ * return getBooleanOrDialectDefault(null, feelDialect);
+ * }
+ * }
*/
Review Comment:
TODO comment with commented-out code should either be implemented or
removed. The comment indicates this should be removed ("To be removed"), so it
should be deleted entirely.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/impl/RangeImpl.java:
##########
@@ -22,22 +22,20 @@
import java.time.LocalDate;
import java.util.function.BiPredicate;
-import org.kie.dmn.feel.lang.FEELDialect;
+import org.kie.dmn.feel.lang.EvaluationContext;
+import org.kie.dmn.feel.lang.ast.dialectHandlers.DialectHandler;
+import org.kie.dmn.feel.lang.ast.dialectHandlers.DialectHandlerFactory;
import org.kie.dmn.feel.runtime.Range;
-import org.kie.dmn.feel.util.BooleanEvalHelper;
-import static org.kie.dmn.feel.lang.ast.UnaryTestNode.UnaryOperator.GT;
-import static org.kie.dmn.feel.lang.ast.UnaryTestNode.UnaryOperator.GTE;
-import static org.kie.dmn.feel.lang.ast.UnaryTestNode.UnaryOperator.LT;
-import static org.kie.dmn.feel.lang.ast.UnaryTestNode.UnaryOperator.LTE;
+import static org.kie.dmn.feel.lang.ast.UnaryTestNode.UnaryOperator.*;
Review Comment:
[nitpick] Using wildcard static import (`import static
org.kie.dmn.feel.lang.ast.UnaryTestNode.UnaryOperator.*;`) makes it unclear
which specific operators are being used. Consider importing only the specific
operators needed.
```suggestion
import static
org.kie.dmn.feel.lang.ast.UnaryTestNode.UnaryOperator.GREATER_THAN;
import static
org.kie.dmn.feel.lang.ast.UnaryTestNode.UnaryOperator.GREATER_THAN_OR_EQUAL;
import static
org.kie.dmn.feel.lang.ast.UnaryTestNode.UnaryOperator.LESS_THAN;
import static
org.kie.dmn.feel.lang.ast.UnaryTestNode.UnaryOperator.LESS_THAN_OR_EQUAL;
```
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/BooleanEvalHelperTest.java:
##########
@@ -68,21 +61,24 @@ void getBooleanOrDialectDefaultBFEEL() {
assertThat(getBooleanOrDialectDefault(null,
FEELDialect.BFEEL)).isEqualTo(Boolean.FALSE);
}
- @Test
- void getFalseOrDialectDefaultFEEL() {
- assertThat(getFalseOrDialectDefault(false,
FEELDialect.FEEL)).isEqualTo(Boolean.FALSE);
- assertThat(getFalseOrDialectDefault(true, FEELDialect.FEEL)).isNull();
- assertThat(getFalseOrDialectDefault("true",
FEELDialect.FEEL)).isNull();
- assertThat(getFalseOrDialectDefault(null, FEELDialect.FEEL)).isNull();
- }
-
- @Test
- void getFalseOrDialectDefaultBFEEL() {
- assertThat(getFalseOrDialectDefault(false,
FEELDialect.BFEEL)).isEqualTo(Boolean.FALSE);
- assertThat(getFalseOrDialectDefault(true,
FEELDialect.BFEEL)).isEqualTo(Boolean.FALSE);
- assertThat(getFalseOrDialectDefault("true",
FEELDialect.BFEEL)).isEqualTo(Boolean.FALSE);
- assertThat(getFalseOrDialectDefault(null,
FEELDialect.BFEEL)).isEqualTo(Boolean.FALSE);
- }
+ //TODO to be removed
+ /*
+ * @Test
+ * void getFalseOrDialectDefaultFEEL() {
+ * assertThat(getFalseOrDialectDefault(false,
FEELDialect.FEEL)).isEqualTo(Boolean.FALSE);
+ * assertThat(getFalseOrDialectDefault(true, FEELDialect.FEEL)).isNull();
+ * assertThat(getFalseOrDialectDefault("true", FEELDialect.FEEL)).isNull();
+ * assertThat(getFalseOrDialectDefault(null, FEELDialect.FEEL)).isNull();
+ * }
+ *
+ * @Test
+ * void getFalseOrDialectDefaultBFEEL() {
+ * assertThat(getFalseOrDialectDefault(false,
FEELDialect.BFEEL)).isEqualTo(Boolean.FALSE);
+ * assertThat(getFalseOrDialectDefault(true,
FEELDialect.BFEEL)).isEqualTo(Boolean.FALSE);
+ * assertThat(getFalseOrDialectDefault("true",
FEELDialect.BFEEL)).isEqualTo(Boolean.FALSE);
+ * assertThat(getFalseOrDialectDefault(null,
FEELDialect.BFEEL)).isEqualTo(Boolean.FALSE);
+ * }
+ */
Review Comment:
TODO comment "to be removed" with large block of commented-out test code
should be addressed. Either remove this code entirely or implement the tests in
the appropriate location.
```suggestion
```
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java:
##########
@@ -158,22 +180,24 @@ private Boolean utEqualSemantic(Object left, Object
right) {
return ((Collection) right).contains(left);
} else {
// evaluate single entity
- return BooleanEvalHelper.isEqual(left, right, FEELDialect.FEEL);
+ //return DefaultDialectHandler.isEqual(left, right, () -> null, ()
-> null);
+ return DefaultDialectHandler.isEqual(left, right, () -> (left ==
null && right == null), () -> Boolean.FALSE);
+
}
Review Comment:
[nitpick] Inconsistent nullFallback behavior in utEqualSemantic. At line
184, the method calls `DefaultDialectHandler.isEqual` with `() -> (left == null
&& right == null)` as nullFallback. This means when both are null, the lambda
returns true, but when only one is null, it returns false. However, this is a
confusing way to express "null == null → true". A clearer approach would be to
handle the null case explicitly before calling isEqual, or use `() ->
Boolean.TRUE` for nullFallback with an explicit null check beforehand.
```suggestion
if (left == null && right == null) {
return Boolean.TRUE;
} else if (left == null || right == null) {
return Boolean.FALSE;
} else {
return DefaultDialectHandler.isEqual(left, right, () ->
Boolean.TRUE, () -> Boolean.FALSE);
}
```
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/Range.java:
##########
@@ -37,13 +35,14 @@ enum RangeBoundary {
RangeBoundary getHighBoundary();
- Boolean includes(FEELDialect feelDialect, Object param);
+ //Boolean includes(FEELDialect feelDialect, Object param);
Review Comment:
The commented-out import and old method signature should be removed to clean
up the interface definition.
```suggestion
```
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DecisionTableFunction.java:
##########
@@ -33,11 +33,7 @@
import org.kie.dmn.feel.lang.impl.FEELBuilder;
import org.kie.dmn.feel.runtime.Range;
import org.kie.dmn.feel.runtime.UnaryTest;
-import org.kie.dmn.feel.runtime.decisiontables.DTDecisionRule;
-import org.kie.dmn.feel.runtime.decisiontables.DTInputClause;
-import org.kie.dmn.feel.runtime.decisiontables.DTOutputClause;
-import org.kie.dmn.feel.runtime.decisiontables.DecisionTableImpl;
-import org.kie.dmn.feel.runtime.decisiontables.HitPolicy;
+import org.kie.dmn.feel.runtime.decisiontables.*;
Review Comment:
[nitpick] Using wildcard imports (`import java.time.*;` and `import
org.kie.dmn.feel.runtime.decisiontables.*;`) is discouraged. Consider replacing
with explicit imports.
```suggestion
import org.kie.dmn.feel.runtime.decisiontables.DecisionTable;
import org.kie.dmn.feel.runtime.decisiontables.DecisionTableImpl;
import org.kie.dmn.feel.runtime.decisiontables.HitPolicy;
import org.kie.dmn.feel.runtime.decisiontables.OutputClause;
import org.kie.dmn.feel.runtime.decisiontables.Rule;
import org.kie.dmn.feel.runtime.decisiontables.RuleAnnotationClause;
import org.kie.dmn.feel.runtime.decisiontables.RuleAnnotationClauseImpl;
import org.kie.dmn.feel.runtime.decisiontables.RuleAnnotationEntry;
import org.kie.dmn.feel.runtime.decisiontables.RuleAnnotationEntryImpl;
import org.kie.dmn.feel.runtime.decisiontables.InputClause;
import org.kie.dmn.feel.runtime.decisiontables.InputClauseImpl;
```
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/dialectHandlers/FEELDialectHandler.java:
##########
@@ -93,32 +93,155 @@ public Map<CheckedPredicate, BiFunction<Object, Object,
Object>> getAndOperation
@Override
public Map<CheckedPredicate, BiFunction<Object, Object, Object>>
getEqualOperations(EvaluationContext ctx) {
- return new LinkedHashMap<>(getCommonEqualOperations(ctx));
+ Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
+ map.put(
+ new CheckedPredicate((left, right) -> left == null && right ==
null, false),
+ (left, right) -> Boolean.TRUE);
+ map.put(
+ new CheckedPredicate((left, right) -> true, false),
+ (left, right) -> isEqual(left, right,
+ () -> Boolean.FALSE, () -> null));
+
+ return map;
}
@Override
public Map<CheckedPredicate, BiFunction<Object, Object, Object>>
getGteOperations(EvaluationContext ctx) {
- return new LinkedHashMap<>(getCommonGteOperations(ctx));
+ Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
+
+ // left is Boolean, right is null
+ map.put(
+ new CheckedPredicate((left, right) -> left instanceof Boolean
&& right == null, false),
+ (left, right) -> {
+ Boolean leftBool = (Boolean) left;
+ if (Boolean.FALSE.equals(leftBool)) {
+ return null; // false + null → null
+ }
+ return Boolean.TRUE; // true + null → true
+ });
+
+ // right is Boolean, left is null
+ map.put(
+ new CheckedPredicate((left, right) -> right instanceof Boolean
&& left == null, false),
+ (left, right) -> {
+ Boolean rightBool = (Boolean) right;
+ if (Boolean.FALSE.equals(rightBool)) {
+ return null; // null + false → null
+ }
+ return Boolean.TRUE; // null + true → true
+ });
+ // 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) {
- return new LinkedHashMap<>(getCommonGtOperations(ctx));
+ Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
+
+ // left is Boolean, right is null
+ map.put(
+ new CheckedPredicate((left, right) -> left instanceof Boolean
&& right == null, false),
+ (left, right) -> {
+ Boolean leftBool = (Boolean) left;
+ if (Boolean.FALSE.equals(leftBool)) {
+ return null; // false + null → null
+ }
+ return Boolean.TRUE; // true + null → true
+ });
+
+ // right is Boolean, left is null
+ map.put(
+ new CheckedPredicate((left, right) -> right instanceof Boolean
&& left == null, false),
+ (left, right) -> {
+ Boolean rightBool = (Boolean) right;
+ if (Boolean.FALSE.equals(rightBool)) {
+ return null; // null + false → null
+ }
+ return Boolean.TRUE; // null + true → true
+ });
+
+ // Fall back to common > operations
+ map.putAll(getCommonGtOperations(ctx));
+ return map;
}
@Override
public Map<CheckedPredicate, BiFunction<Object, Object, Object>>
getLteOperations(EvaluationContext ctx) {
- return new LinkedHashMap<>(getCommonLteOperations(ctx));
+ Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
+
+ // left is Boolean, right is null
+ map.put(
+ new CheckedPredicate((left, right) -> left instanceof Boolean
&& right == null, false),
+ (left, right) -> {
+ Boolean leftBool = (Boolean) left;
+ if (Boolean.FALSE.equals(leftBool)) {
+ return null; // false + null → null
+ }
+ return Boolean.TRUE; // true + null → true
+ });
+
+ // right is Boolean, left is null
+ map.put(
+ new CheckedPredicate((left, right) -> right instanceof Boolean
&& left == null, false),
+ (left, right) -> {
+ Boolean rightBool = (Boolean) right;
+ if (Boolean.FALSE.equals(rightBool)) {
+ return null; // null + false → null
+ }
+ return Boolean.TRUE; // null + true → true
+ });
+
+ // Fall back to common ≤ operations
+ map.putAll(getCommonLteOperations(ctx));
+ return map;
}
@Override
public Map<CheckedPredicate, BiFunction<Object, Object, Object>>
getLtOperations(EvaluationContext ctx) {
- return new LinkedHashMap<>(getCommonLtOperations(ctx));
+ Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
+
+ // left is Boolean, right is null
+ map.put(
+ new CheckedPredicate((left, right) -> left instanceof Boolean
&& right == null, false),
+ (left, right) -> {
+ Boolean leftBool = (Boolean) left;
+ if (Boolean.FALSE.equals(leftBool)) {
+ return null; // false + null → null
+ }
+ return Boolean.TRUE; // true + null → true
+ });
+
+ // right is Boolean, left is null
+ map.put(
+ new CheckedPredicate((left, right) -> right instanceof Boolean
&& left == null, false),
+ (left, right) -> {
+ Boolean rightBool = (Boolean) right;
+ if (Boolean.FALSE.equals(rightBool)) {
+ return null; // null + false → null
+ }
+ return Boolean.TRUE; // null + true → true
+ });
+
+ map.putAll(getCommonLtOperations(ctx));
+ return map;
Review Comment:
Misleading comments for Boolean comparison operations. The comments say
"false + null → null" and "true + null → true" (lines 118-120, 128-131, and
similar in other comparison operations) but these are >= (greater than or
equal) operations, not addition. The comments should use ">=", ">", "<=", or
"<" symbols instead of "+" to accurately describe the comparison being
performed.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/util/BooleanEvalHelper.java:
##########
@@ -186,18 +237,21 @@ public static Boolean
isEqualTimeInSemanticD(TemporalAccessor left, TemporalAcce
/**
* This method consider if the <code>value</code> object is a
<code>String</code>
* In that case, return the {@link String#equals(Object)} result
- * Otherwise, default to the {@link #isEqual(Object, Object, FEELDialect)}
+ * Otherwise, default to the isEqual method
*
* @param value
* @param itemFromList
* @return
*/
public static boolean isEqualsStringCompare(Object value, Object
itemFromList) {
+ if (value == null && itemFromList == null) {
+ return true; // both null → equal
+ }
if (value instanceof String) {
return value.equals(itemFromList);
} else {
// Defaulting FEELDialect to FEEL
- Boolean dmnEqual = isEqual(value, itemFromList, FEELDialect.FEEL);
+ Boolean dmnEqual = DefaultDialectHandler.isEqual(value,
itemFromList, () -> null, () -> null);
return dmnEqual != null && dmnEqual;
}
Review Comment:
Inconsistent null handling in equality check. At line 247-249, there's a
special case for `null == null → true`, but at line 254, the method calls
`DefaultDialectHandler.isEqual(value, itemFromList, () -> null, () -> null)`
which would return `null` for null inputs (based on the nullFallback supplier).
This creates an inconsistency: if both are null, line 248 returns true
directly, but if only one is null, the isEqual call returns null, which is then
checked against null at line 255. Consider making the null handling consistent
throughout the method.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/dialectHandlers/BFEELDialectHandler.java:
##########
@@ -84,67 +85,245 @@ public Map<CheckedPredicate, BiFunction<Object, Object,
Object>> getAddOperation
public Map<CheckedPredicate, BiFunction<Object, Object, Object>>
getAndOperations(EvaluationContext ctx) {
Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
FEELDialect dialect = ctx.getFEELDialect();
+
// Special case: true AND otherwise → false
map.put(
new CheckedPredicate((left, right) -> {
- Boolean leftBool =
BooleanEvalHelper.getBooleanOrDialectDefault(left, dialect);
+ Boolean leftBool = (left instanceof Boolean) ? (Boolean)
left : Boolean.FALSE;
Object rightValue = evalRight(right, ctx);
- Boolean rightBool =
BooleanEvalHelper.getBooleanOrDialectDefault(rightValue, dialect);
+ Boolean rightBool = (rightValue instanceof Boolean) ?
(Boolean) rightValue : Boolean.FALSE;
return Boolean.TRUE.equals(leftBool) &&
Boolean.FALSE.equals(rightBool);
}, false),
(left, right) -> Boolean.FALSE);
- // Special case: otherwise AND true → false
+ // Special case: false AND true → false
map.put(
new CheckedPredicate((left, right) -> {
- Boolean leftBool =
BooleanEvalHelper.getBooleanOrDialectDefault(left, dialect);
+ Boolean leftBool = (left instanceof Boolean) ? (Boolean)
left : Boolean.FALSE;
Object rightValue = evalRight(right, ctx);
- Boolean rightBool =
BooleanEvalHelper.getBooleanOrDialectDefault(rightValue, dialect);
- return leftBool == null && Boolean.TRUE.equals(rightBool);
+ Boolean rightBool = (rightValue instanceof Boolean) ?
(Boolean) rightValue : Boolean.FALSE;
+ return Boolean.FALSE.equals(leftBool) &&
Boolean.TRUE.equals(rightBool);
}, false),
(left, right) -> Boolean.FALSE);
- // Special case: otherwise AND otherwise → false
+ // Special case: false AND false → false
map.put(
new CheckedPredicate((left, right) -> {
- Boolean leftBool =
BooleanEvalHelper.getBooleanOrDialectDefault(left, dialect);
+ Boolean leftBool = (left instanceof Boolean) ? (Boolean)
left : Boolean.FALSE;
Object rightValue = evalRight(right, ctx);
- Boolean rightBool =
BooleanEvalHelper.getBooleanOrDialectDefault(rightValue, dialect);
- return leftBool == null && Boolean.FALSE.equals(rightBool);
+ Boolean rightBool = (rightValue instanceof Boolean) ?
(Boolean) rightValue : Boolean.FALSE;
+ return Boolean.FALSE.equals(leftBool) &&
Boolean.FALSE.equals(rightBool);
}, false),
(left, right) -> Boolean.FALSE);
+
map.putAll(getCommonAndOperations(ctx));
return map;
}
@Override
public Map<CheckedPredicate, BiFunction<Object, Object, Object>>
getEqualOperations(EvaluationContext ctx) {
- return new LinkedHashMap<>(getCommonEqualOperations(ctx));
+ Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
+
+ // Shortcut: null = null → false
+ map.put(
+ new CheckedPredicate((left, right) -> left == null && right ==
null, false),
+ (left, right) -> Boolean.TRUE);
+ map.put(
+ new CheckedPredicate((left, right) -> true, false),
+ (left, right) -> isEqual(left, right,
+ () -> Boolean.FALSE, () -> Boolean.FALSE));
+ return map;
}
@Override
public Map<CheckedPredicate, BiFunction<Object, Object, Object>>
getGteOperations(EvaluationContext ctx) {
- return new LinkedHashMap<>(getCommonGteOperations(ctx));
+ Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
+ // Any non-Boolean coerces to false, so (false,false) --> false
+ 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),
+ (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
+ map.put(
+ new CheckedPredicate((left, right) -> true, false),
+ (left, right) -> {
+ Boolean greater = compare(left, right, (l, r) ->
l.compareTo(r) > 0,
+ () -> Boolean.FALSE,
+ () -> Boolean.FALSE);
+ //Boolean equal = BooleanEvalHelper.isEqual(left, right,
ctx.getFEELDialect());
+ Boolean equal = (EqExecutor.instance().evaluate(left,
right, ctx) instanceof Boolean)
+ ? (Boolean) EqExecutor.instance().evaluate(left,
right, ctx)
+ : null;
+
+ if (greater == null && equal == null) {
+ return Boolean.FALSE; // BFEEL default
+ }
+ 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) {
- return new LinkedHashMap<>(getCommonGtOperations(ctx));
+ Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
+
+ // Any non-Boolean coerces to false, so (false,false) --> false
+ 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),
+ (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
+ map.put(
+ new CheckedPredicate((left, right) -> true, false),
+ (left, right) -> {
+ Boolean greater = compare(left, right,
+ (l, r) -> l.compareTo(r) > 0,
+ () -> Boolean.FALSE,
+ () -> Boolean.FALSE);
+ return Objects.requireNonNullElse(greater, Boolean.FALSE);
+ });
+
+ map.putAll(getCommonGtOperations(ctx));
+ return map;
}
@Override
public Map<CheckedPredicate, BiFunction<Object, Object, Object>>
getLteOperations(EvaluationContext ctx) {
- return new LinkedHashMap<>(getCommonLteOperations(ctx));
+ Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
+
+ // Any non-Boolean coerces to false, so (false,false) --> false
+ 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),
+ (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
+ map.put(
+ new CheckedPredicate((left, right) -> true, false),
+ (left, right) -> {
+ Boolean less = compare(left, right,
+ (l, r) -> l.compareTo(r) < 0,
+ () -> Boolean.FALSE,
+ () -> Boolean.FALSE);
+ Boolean equal = (EqExecutor.instance().evaluate(left,
right, ctx) instanceof Boolean)
+ ? (Boolean) EqExecutor.instance().evaluate(left,
right, ctx)
+ : null;
+
+ if (less == null && equal == null) {
+ return Boolean.FALSE; // BFEEL default
+ }
+ 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) {
- return new LinkedHashMap<>(getCommonLtOperations(ctx));
+ Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
+
+ // Non-Boolean coerces to false, so (false,false) --> false
+ 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),
+ (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
+ map.put(
+ new CheckedPredicate((left, right) -> true, false),
+ (left, right) -> {
+ Boolean less = compare(left, right,
+ (l, r) -> l.compareTo(r) < 0,
+ () -> Boolean.FALSE,
+ () -> Boolean.FALSE);
+ return Objects.requireNonNullElse(less, Boolean.FALSE);
+ });
+ map.putAll(getCommonLtOperations(ctx));
+ return map;
}
@Override
public Map<CheckedPredicate, BiFunction<Object, Object, Object>>
getNotEqualOperations(EvaluationContext ctx) {
- return new LinkedHashMap<>(getCommonNotEqualOperations(ctx));
+ Map<CheckedPredicate, BiFunction<Object, Object, Object>> map = new
LinkedHashMap<>();
+ // Shortcut: null != null → true
Review Comment:
Confusing comment at line 311. The comment says "Shortcut: null != null →
true" but the code returns `Boolean.FALSE` for this case. The comment should
say "Shortcut: null != null → false" to correctly describe the behavior.
```suggestion
// Shortcut: null != null → false
```
--
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]
