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 2025/04/04 05:29:51 UTC
[PR] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
ChinchuAjith opened a new pull request, #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299
Fixes https://github.com/apache/incubator-kie-issues/issues/1911
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2065685852
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -39,17 +42,15 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n) {
}
public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @ParameterName( "scale" ) BigDecimal scale) {
- if ( n == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
- }
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
+ try {
+ int coercedN = coerceIntegerNumber(n).orElseThrow(() -> new NoSuchElementException("n"));
Review Comment:
Hi @ChinchuAjith
it seems that `coercedN` is not used at all ... what's missing here ? π€
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/FloorFunction.java:
##########
@@ -38,18 +41,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n) {
return invoke(n, BigDecimal.ZERO);
}
- public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @ParameterName( "scale" ) BigDecimal scale) {
- if ( n == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
- }
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
+ public FEELFnResult<BigDecimal> invoke(@ParameterName("n") BigDecimal n, @ParameterName("scale") BigDecimal scale) {
+ try {
+ int coercedN = coerceIntegerNumber(n).orElseThrow(() -> new NoSuchElementException("n"));
Review Comment:
Hi @ChinchuAjith
it seems that coercedN is not used at all ... what's missing here ? π€
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DecimalFunction.java:
##########
@@ -34,18 +37,17 @@ private DecimalFunction() {
super( "decimal" );
}
- public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @ParameterName( "scale" ) BigDecimal scale) {
- if ( n == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
- }
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
+ public FEELFnResult<BigDecimal> invoke(@ParameterName("n") BigDecimal n, @ParameterName("scale") BigDecimal scale) {
+ try {
+ int coercedN = coerceIntegerNumber(n).orElseThrow(() -> new NoSuchElementException("n"));
Review Comment:
Hi @ChinchuAjith
it seems that coercedN is not used at all ... what's missing here ? π€
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/RoundUpFunction.java:
##########
@@ -38,17 +41,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n) {
return invoke(n, BigDecimal.ZERO);
}
- public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @ParameterName( "scale" ) BigDecimal scale) {
- if ( n == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
- }
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
- }
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ public FEELFnResult<BigDecimal> invoke(@ParameterName("n") BigDecimal n, @ParameterName("scale") BigDecimal scale) {
+ try {
+ int coercedN = coerceIntegerNumber(n).orElseThrow(() -> new NoSuchElementException("n"));
Review Comment:
Hi @ChinchuAjith
it seems that coercedN is not used at all ... what's missing here ? π€
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/RoundHalfDownFunction.java:
##########
@@ -38,17 +41,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n) {
return invoke(n, BigDecimal.ZERO);
}
- public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @ParameterName( "scale" ) BigDecimal scale) {
- if ( n == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
- }
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
- }
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ public FEELFnResult<BigDecimal> invoke(@ParameterName("n") BigDecimal n, @ParameterName("scale") BigDecimal scale) {
+ try {
+ int coercedN = coerceIntegerNumber(n).orElseThrow(() -> new NoSuchElementException("n"));
Review Comment:
Hi @ChinchuAjith
it seems that coercedN is not used at all ... what's missing here ? π€
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/RoundDownFunction.java:
##########
@@ -38,17 +41,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n) {
return invoke(n, BigDecimal.ZERO);
}
- public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @ParameterName( "scale" ) BigDecimal scale) {
- if ( n == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
- }
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
- }
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ public FEELFnResult<BigDecimal> invoke(@ParameterName("n") BigDecimal n, @ParameterName("scale") BigDecimal scale) {
+ try {
+ int coercedN = coerceIntegerNumber(n).orElseThrow(() -> new NoSuchElementException("n"));
Review Comment:
Hi @ChinchuAjith
it seems that coercedN is not used at all ... what's missing here ? π€
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/RoundHalfUpFunction.java:
##########
@@ -38,17 +41,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n) {
return invoke(n, BigDecimal.ZERO);
}
- public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @ParameterName( "scale" ) BigDecimal scale) {
- if ( n == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
- }
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
- }
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ public FEELFnResult<BigDecimal> invoke(@ParameterName("n") BigDecimal n, @ParameterName("scale") BigDecimal scale) {
+ try {
+ int coercedN = coerceIntegerNumber(n).orElseThrow(() -> new NoSuchElementException("n"));
Review Comment:
Hi @ChinchuAjith
it seems that coercedN is not used at all ... what's missing here ? π€
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2048304319
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DateAndTimeFunction.java:
##########
@@ -122,33 +126,33 @@ public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" ) Number yea
public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" ) Number year, @ParameterName( "month" ) Number month, @ParameterName( "day" ) Number day,
@ParameterName( "hour" ) Number hour, @ParameterName( "minute" ) Number minute, @ParameterName( "second" ) Number second,
@ParameterName( "hour offset" ) Number hourOffset ) {
- if ( year == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "year", "cannot be null"));
- }
- if ( month == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "month", "cannot be null"));
- }
- if ( day == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "day", "cannot be null"));
- }
- if ( hour == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "hour", "cannot be null"));
- }
- if ( minute == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "minute", "cannot be null"));
- }
- if ( second == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "second", "cannot be null"));
- }
-
try {
+ Optional<Integer> coercedYear = coerceIntegerNumber(year);
Review Comment:
Code modified.
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2066057301
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -39,17 +42,15 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n) {
}
public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @ParameterName( "scale" ) BigDecimal scale) {
- if ( n == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
- }
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
+ try {
+ int coercedN = coerceIntegerNumber(n).orElseThrow(() -> new NoSuchElementException("n"));
Review Comment:
Reviewed the spec and found no reference to coercion for variable n, so I removed it. The null check has been retained.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DecimalFunction.java:
##########
@@ -34,18 +37,17 @@ private DecimalFunction() {
super( "decimal" );
}
- public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @ParameterName( "scale" ) BigDecimal scale) {
- if ( n == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
- }
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
+ public FEELFnResult<BigDecimal> invoke(@ParameterName("n") BigDecimal n, @ParameterName("scale") BigDecimal scale) {
+ try {
+ int coercedN = coerceIntegerNumber(n).orElseThrow(() -> new NoSuchElementException("n"));
Review Comment:
Reviewed the spec and found no reference to coercion for variable n, so I removed it. The null check has been retained.
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2048300986
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +36,53 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new BigDecimal("10000000000.5"));
}
+ @Test
Review Comment:
Method removed. no needed
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2039513888
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DateAndTimeFunction.java:
##########
@@ -122,33 +126,33 @@ public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" ) Number yea
public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" ) Number year, @ParameterName( "month" ) Number month, @ParameterName( "day" ) Number day,
@ParameterName( "hour" ) Number hour, @ParameterName( "minute" ) Number minute, @ParameterName( "second" ) Number second,
@ParameterName( "hour offset" ) Number hourOffset ) {
- if ( year == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "year", "cannot be null"));
- }
- if ( month == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "month", "cannot be null"));
- }
- if ( day == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "day", "cannot be null"));
- }
- if ( hour == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "hour", "cannot be null"));
- }
- if ( minute == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "minute", "cannot be null"));
- }
- if ( second == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "second", "cannot be null"));
- }
-
try {
+ Optional<Integer> coercedYear = coerceIntegerNumber(year);
Review Comment:
Hi @ChinchuAjith
Code could be rewritten with a more idiomatic usage of Optional
```java
try {
int coercedYear = coerceIntegerNumber(year).orElseThrow();
int coercedMonth = coerceIntegerNumber(month).orElseThrow();
int coercedDay = coerceIntegerNumber(day).orElseThrow();
int coercedHour = coerceIntegerNumber(hour).orElseThrow();
int coercedMinute = coerceIntegerNumber(minute).orElseThrow();
int coercedSecond = coerceIntegerNumber(second).orElseThrow();
if (hourOffset != null) {
Optional<Integer> coercedHourOffset = coerceIntegerNumber(hourOffset);
return coercedHourOffset.<FEELFnResult<TemporalAccessor>>map(integer -> FEELFnResult.ofResult(
OffsetDateTime.of(
coercedYear, coercedMonth, coercedDay,
coercedHour, coercedMinute, coercedSecond,
0, ZoneOffset.ofHours(integer)))).orElseGet(() -> FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "hour offset", "could not be coerced to Integer")));
} else {
return FEELFnResult.ofResult(
LocalDateTime.of(
coercedYear, coercedMonth, coercedDay,
coercedHour, coercedMinute, coercedSecond)
);
}
} catch (NoSuchElementException e) { // thrown by Optional.orElseThrow()
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "coercion", "One or more input values could not be coerced to Integer: either null or not a valid Number."));
} catch (DateTimeException e) {
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "input parameters date-parsing exception", e));
}
```
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/util/NumberEvalHelper.java:
##########
@@ -66,12 +67,47 @@ public static BigDecimal getBigDecimalOrNull(Object value) {
return null;
}
+ /**
+ * The method handles various numeric types and converts the given object to an Integer
+ * Returns null If the conversion is not possible.
+ *
+ * @param value : The object to be converted.
+ * @return : An Integer representation of the value, or null if conversion is not possible.
Review Comment:
Thanks @ChinchuAjith
IINW, this method is used only once, and inside this very class.
Could you please:
1. remove public accessor
2. move it to bottom of class (readability)
3. refactor it to return an Optional<Integer> (let's reduce the proliferation of `null`s π )
4. unit test it
5. update javadoc
Thanks!
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/util/NumberEvalHelper.java:
##########
@@ -66,12 +67,47 @@ public static BigDecimal getBigDecimalOrNull(Object value) {
return null;
}
+ /**
+ * The method handles various numeric types and converts the given object to an Integer
+ * Returns null If the conversion is not possible.
+ *
+ * @param value : The object to be converted.
+ * @return : An Integer representation of the value, or null if conversion is not possible.
+ */
+ public static Integer getIntegerOrNull(Object value) {
+ if ( value instanceof BigDecimal ) {
+ return ((BigDecimal) value).intValue();
+ }
+ if ( value instanceof BigInteger ) {
+ return ((BigInteger) value).intValue();
+ }
+ if ( value instanceof Number ) {
+ return ((Number) value).intValue();
+ }
+ return null;
+ }
+
public static Object coerceNumber(Object value) {
if ( value instanceof Number && !(value instanceof BigDecimal) ) {
return getBigDecimalOrNull( value );
- } else {
+ } else {
return value;
}
}
+ /**
+ * Converts the given object to an integer if it is an instance of Number.
+ * else it is returned unchanged.
+ *
+ * @param value : the object to be converted
+ * @return : an integer representation of the number if applicable, otherwise the original value
+ */
+ public static Optional<Integer> coerceIntegerNumber(Object value) {
Review Comment:
Could you please update the javadoc ?
Moreover, since the called method will return an Optional<Integer> by itself, no need to wrap it inside another Optional
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +36,53 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new BigDecimal("10000000000.5"));
}
+ @Test
Review Comment:
Could you please add some unit test for NumberEvalHelper.getIntegerOrNull() ?
Thanks!
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DateFunction.java:
##########
@@ -71,31 +75,28 @@ public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "from" ) String val
}
}
- public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" ) Number year, @ParameterName( "month" ) Number month, @ParameterName( "day" ) Number day) {
- if ( year == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "year", "cannot be null"));
- }
- if ( month == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "month", "cannot be null"));
- }
- if ( day == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "day", "cannot be null"));
- }
-
+ public FEELFnResult<TemporalAccessor> invoke(@ParameterName("year") Number year, @ParameterName("month") Number month, @ParameterName("day") Number day) {
try {
- return FEELFnResult.ofResult( LocalDate.of( year.intValue(), month.intValue(), day.intValue() ) );
+ Optional<Integer> coercedYear = coerceIntegerNumber(year);
Review Comment:
See previous comment
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +36,53 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new BigDecimal("10000000000.5"));
}
+ @Test
+ void coerceIntegerNumber_withBigDecimal() {
+ //Verifies that BigDecimal values are truncated (not rounded) when coerced to integers.
+ Optional<Integer> result = NumberEvalHelper.coerceIntegerNumber(new BigDecimal("99.99"));
Review Comment:
Could you please also test, in the same method, the other "extreme" value, i.e. "99.00001": it also should return 99
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +36,53 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new BigDecimal("10000000000.5"));
}
+ @Test
+ void coerceIntegerNumber_withBigDecimal() {
+ //Verifies that BigDecimal values are truncated (not rounded) when coerced to integers.
+ Optional<Integer> result = NumberEvalHelper.coerceIntegerNumber(new BigDecimal("99.99"));
+ assertThat(result).isPresent();
+ assertThat(result.get()).isEqualTo(99);
+ }
+
+ @Test
+ void coerceIntegerNumber_withBigInteger() {
+ Optional<Integer> result = NumberEvalHelper.coerceIntegerNumber(new BigInteger("1000"));
+ assertThat(result).isPresent();
+ assertThat(result.get()).isEqualTo(1000);
+ }
+
+ @Test
+ void coerceIntegerNumber_withDouble() {
Review Comment:
Could you please also test, in the same method, "extreme" values, e.g. 42.009 and 42.99999: both should return 42
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DecimalFunction.java:
##########
@@ -38,14 +40,15 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @Para
if ( n == null ) {
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
}
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
+ Optional<Integer> scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
+ if(scaleObj.isEmpty()) {
+ return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be a non-null Number value."));
}
+ int scaleInt = scaleObj.get();
// Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ if (scaleInt < -6111 || scaleInt > 6176) {
Review Comment:
See comment on CeilingFunction
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -45,11 +47,14 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @Para
if ( scale == null ) {
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
}
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ Optional<Integer> scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
Review Comment:
Hi @ChinchuAjith
Optional could be used in more idiomatic way to express the same behavior:
```java
Optional<Integer> scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
AtomicReference<FEELFnResult<BigDecimal>> toReturn = new AtomicReference<>();
scaleObj.ifPresentOrElse(scaleInt -> {
if (scaleInt < -6111 || scaleInt > 6176) {
toReturn.set(FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 and 6176.")));
} else {
toReturn.set(FEELFnResult.ofResult(n.setScale(scaleInt, RoundingMode.CEILING)));
}
},
() -> toReturn.set(FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "should be a Number type"))));
return toReturn.get();
```
Your choice, anyway
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2057703592
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -45,11 +48,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @Para
if ( scale == null ) {
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
}
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
- }
-
- return FEELFnResult.ofResult( n.setScale( scale.intValue(), RoundingMode.CEILING ) );
+ Optional<Integer> scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
+ AtomicReference<FEELFnResult<BigDecimal>> toReturn = new AtomicReference<>();
Review Comment:
@ChinchuAjith Thank you, I guess the performances are not impacted as you said (but the memory footprint is undoubtedly increased any time you create an AtomicReference).
In any case, despite your solution being correct from a functional point of view, I still believe that removing the `Optional` could result in a considerable simplification. From my point of view, the case you're managing is an INVALID case, not an OPTIONAL case. The difference is subtle, let me explain it:
Currently, you are:
- Coercing a `number` returning an `Optional`. If the given parameter is valid, the Optional holds the number. If the given parameter is INVALID, an empty optional is returned.
- The callers of the `NumberEvalHelper.coerceIntegerNumber` function will take the number, if present in the Optional, or throw an exception, strengthen my above statement that we are managing an INVALID case
- The exception is then caught and managed.
My proposal is NOT returning null in case of invalid parameters, but immediately throwing an exception in that case, simplifying the code because the second point of the above list is no longer necessary.
To be clear:
```
public static Integer coerceIntegerNumber(Object value) {
if ( value instanceof BigDecimal ) {
return ((BigDecimal) value).intValue());
}
if ( value instanceof BigInteger ) {
return ((BigInteger) value).intValue();
}
if ( value instanceof Number ) {
return ((Number) value).intValue();
}
throw new IllegalArgumentException("The provided value: " + value + " is not a number");
}
```
Ping me in private in case, I'm available to talk!
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2063608967
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/TimeFunction.java:
##########
@@ -181,4 +181,11 @@ protected FEELFnResult<TemporalAccessor> manageDateTimeException(DateTimeExcepti
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "from", e));
}
+ private FEELFnResult<TemporalAccessor> checkParamIfNull(String paramName, Number value) {
+ if (NumberEvalHelper.coerceIntegerNumber(value) == null) {
Review Comment:
The checkParamIfNull method incorrectly checks for null on an Optional, which is never null. Instead, use isEmpty() to verify whether the Optional contains a value.
```suggestion
if (NumberEvalHelper.coerceIntegerNumber(value).isEmpty()) {
```
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2048296302
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -45,11 +46,14 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @Para
if ( scale == null ) {
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
}
+ Object scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
// Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ if (scaleObj instanceof Integer) {
+ int scaleInt = (Integer) scaleObj;
+ if (scaleInt < -6111 || scaleInt > 6176) {
+ return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ }
Review Comment:
Modified this. Please check
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2048300349
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +36,53 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new BigDecimal("10000000000.5"));
}
+ @Test
+ void coerceIntegerNumber_withBigDecimal() {
+ //Verifies that BigDecimal values are truncated (not rounded) when coerced to integers.
+ Optional<Integer> result = NumberEvalHelper.coerceIntegerNumber(new BigDecimal("99.99"));
+ assertThat(result).isPresent();
+ assertThat(result.get()).isEqualTo(99);
+ }
+
+ @Test
+ void coerceIntegerNumber_withBigInteger() {
+ Optional<Integer> result = NumberEvalHelper.coerceIntegerNumber(new BigInteger("1000"));
+ assertThat(result).isPresent();
+ assertThat(result.get()).isEqualTo(1000);
+ }
+
+ @Test
+ void coerceIntegerNumber_withDouble() {
Review Comment:
test cases added
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +36,53 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new BigDecimal("10000000000.5"));
}
+ @Test
+ void coerceIntegerNumber_withBigDecimal() {
+ //Verifies that BigDecimal values are truncated (not rounded) when coerced to integers.
+ Optional<Integer> result = NumberEvalHelper.coerceIntegerNumber(new BigDecimal("99.99"));
Review Comment:
test cases 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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2048310749
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DateFunction.java:
##########
@@ -71,31 +75,28 @@ public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "from" ) String val
}
}
- public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" ) Number year, @ParameterName( "month" ) Number month, @ParameterName( "day" ) Number day) {
- if ( year == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "year", "cannot be null"));
- }
- if ( month == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "month", "cannot be null"));
- }
- if ( day == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "day", "cannot be null"));
- }
-
+ public FEELFnResult<TemporalAccessor> invoke(@ParameterName("year") Number year, @ParameterName("month") Number month, @ParameterName("day") Number day) {
try {
- return FEELFnResult.ofResult( LocalDate.of( year.intValue(), month.intValue(), day.intValue() ) );
+ Optional<Integer> coercedYear = coerceIntegerNumber(year);
Review Comment:
Method refactored as suggested
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2038979571
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DateFunction.java:
##########
@@ -72,16 +73,10 @@ public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "from" ) String val
}
public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" ) Number year, @ParameterName( "month" ) Number month, @ParameterName( "day" ) Number day) {
- if ( year == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "year", "cannot be null"));
- }
- if ( month == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "month", "cannot be null"));
- }
- if ( day == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "day", "cannot be null"));
- }
-
+ FEELFnResult<TemporalAccessor> result;
+ if ((result = checkNullParam("year", year)) != null) return result;
Review Comment:
Done
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2048300066
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/util/NumberEvalHelper.java:
##########
@@ -66,12 +67,47 @@ public static BigDecimal getBigDecimalOrNull(Object value) {
return null;
}
+ /**
+ * The method handles various numeric types and converts the given object to an Integer
+ * Returns null If the conversion is not possible.
+ *
+ * @param value : The object to be converted.
+ * @return : An Integer representation of the value, or null if conversion is not possible.
+ */
+ public static Integer getIntegerOrNull(Object value) {
+ if ( value instanceof BigDecimal ) {
+ return ((BigDecimal) value).intValue();
+ }
+ if ( value instanceof BigInteger ) {
+ return ((BigInteger) value).intValue();
+ }
+ if ( value instanceof Number ) {
+ return ((Number) value).intValue();
+ }
+ return null;
+ }
+
public static Object coerceNumber(Object value) {
if ( value instanceof Number && !(value instanceof BigDecimal) ) {
return getBigDecimalOrNull( value );
- } else {
+ } else {
return value;
}
}
+ /**
+ * Converts the given object to an integer if it is an instance of Number.
+ * else it is returned unchanged.
+ *
+ * @param value : the object to be converted
+ * @return : an integer representation of the number if applicable, otherwise the original value
+ */
+ public static Optional<Integer> coerceIntegerNumber(Object value) {
Review Comment:
Modified the method and javadoc
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2055808560
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DateAndTimeFunction.java:
##########
@@ -119,71 +124,57 @@ public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" ) Number yea
return invoke( year, month, day, hour, minute, second, (Number) null );
}
- public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" ) Number year, @ParameterName( "month" ) Number month, @ParameterName( "day" ) Number day,
- @ParameterName( "hour" ) Number hour, @ParameterName( "minute" ) Number minute, @ParameterName( "second" ) Number second,
- @ParameterName( "hour offset" ) Number hourOffset ) {
- if ( year == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "year", "cannot be null"));
- }
- if ( month == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "month", "cannot be null"));
- }
- if ( day == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "day", "cannot be null"));
- }
- if ( hour == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "hour", "cannot be null"));
- }
- if ( minute == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "minute", "cannot be null"));
- }
- if ( second == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "second", "cannot be null"));
- }
-
+ public FEELFnResult<TemporalAccessor> invoke(@ParameterName("year") Number year, @ParameterName("month") Number month, @ParameterName("day") Number day,
+ @ParameterName("hour") Number hour, @ParameterName("minute") Number minute, @ParameterName("second") Number second,
+ @ParameterName("hour offset") Number hourOffset) {
try {
- if( hourOffset != null ) {
- return FEELFnResult.ofResult( OffsetDateTime.of( year.intValue(), month.intValue(), day.intValue(),
- hour.intValue(), minute.intValue(), second.intValue(),
- 0, ZoneOffset.ofHours( hourOffset.intValue() ) ) );
+ int coercedYear = coerceIntegerNumber(year).orElseThrow();
+ int coercedMonth = coerceIntegerNumber(month).orElseThrow();
+ int coercedDay = coerceIntegerNumber(day).orElseThrow();
+ int coercedHour = coerceIntegerNumber(hour).orElseThrow();
+ int coercedMinute = coerceIntegerNumber(minute).orElseThrow();
+ int coercedSecond = coerceIntegerNumber(second).orElseThrow();
Review Comment:
~~@ChinchuAjith `orElseThrow()` with an empty parameter will throw an NPE. Is that the expected behaviour?~~
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2057611093
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -45,11 +48,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @Para
if ( scale == null ) {
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
}
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
- }
-
- return FEELFnResult.ofResult( n.setScale( scale.intValue(), RoundingMode.CEILING ) );
+ Optional<Integer> scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
+ AtomicReference<FEELFnResult<BigDecimal>> toReturn = new AtomicReference<>();
Review Comment:
Not really a performance issue in most cases. AtomicReference is very lightweight. Unless you're creating tons of them in a tight loop or on a critical hot path, performance impact is negligible. I believe AtomicReference is used here because the result needs to be assigned within a lambda expression, which doesnβt allow direct modification of local variables. Also, returning Optional<Integer> is generally preferred over null as it makes the absence of a value explicit and helps avoid potential NullPointerExceptions. Correct me if i am wrong. @gitgabrio ,Curious to hear your take on this! π
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2038843439
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -6,9 +6,7 @@
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
+ * http://www.apache.org/licenses/LICENSE-2.0
Review Comment:
Fixed and committed in all classes impacted by prior modifications.
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2039442276
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +35,46 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new BigDecimal("10000000000.5"));
}
+ @Test
+ void coerceIntegerNumber_withBigDecimal() {
Review Comment:
Done
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2066057522
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/FloorFunction.java:
##########
@@ -38,18 +41,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n) {
return invoke(n, BigDecimal.ZERO);
}
- public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @ParameterName( "scale" ) BigDecimal scale) {
- if ( n == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
- }
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
+ public FEELFnResult<BigDecimal> invoke(@ParameterName("n") BigDecimal n, @ParameterName("scale") BigDecimal scale) {
+ try {
+ int coercedN = coerceIntegerNumber(n).orElseThrow(() -> new NoSuchElementException("n"));
Review Comment:
Reviewed the spec and found no reference to coercion for variable n, so I removed it. The null check has been retained.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/RoundDownFunction.java:
##########
@@ -38,17 +41,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n) {
return invoke(n, BigDecimal.ZERO);
}
- public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @ParameterName( "scale" ) BigDecimal scale) {
- if ( n == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
- }
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
- }
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ public FEELFnResult<BigDecimal> invoke(@ParameterName("n") BigDecimal n, @ParameterName("scale") BigDecimal scale) {
+ try {
+ int coercedN = coerceIntegerNumber(n).orElseThrow(() -> new NoSuchElementException("n"));
Review Comment:
Reviewed the spec and found no reference to coercion for variable n, so I removed it. The null check has been retained.
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2057703592
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -45,11 +48,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @Para
if ( scale == null ) {
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
}
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
- }
-
- return FEELFnResult.ofResult( n.setScale( scale.intValue(), RoundingMode.CEILING ) );
+ Optional<Integer> scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
+ AtomicReference<FEELFnResult<BigDecimal>> toReturn = new AtomicReference<>();
Review Comment:
@ChinchuAjith Thank you, I guess the performances are not impacted as you said (but the memory footprint is undoubtedly increased any time you create an AtomicReference).
In any case, despite your solution being correct from a functional point of view, I still believe that removing the `Optional` could result in a considerable simplification. From my point of view, the case you're managing is an INVALID case, not an OPTIONAL case. The difference is subtle, let me explain it:
Currently, you are:
- Coercing a `number` returning an `Optional`. If the given parameter is valid, the Optional holds the number. If the given parameter is INVALID, an empty optional is returned.
- The callers of the `NumberEvalHelper.coerceIntegerNumber` function will take the number, if present in the Optional, or throw an exception, strengthen my above statement that we are managing an INVALID case
- The exception is then caught and managed.
My proposal is NOT returning null in case of invalid parameters, but immediately throwing an exception in that case, simplifying the code because the second point of the above list is no longer necessary.
To be clear:
```
public static Integer coerceIntegerNumber(Object value) {
if ( value instanceof BigDecimal ) {
return ((BigDecimal) value).intValue();
}
if ( value instanceof BigInteger ) {
return ((BigInteger) value).intValue();
}
if ( value instanceof Number ) {
return ((Number) value).intValue();
}
throw new IllegalArgumentException("The provided value: " + value + " is not a number");
}
```
Ping me in private in case, I'm available to talk!
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2058856668
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -45,11 +48,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @Para
if ( scale == null ) {
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
}
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
- }
-
- return FEELFnResult.ofResult( n.setScale( scale.intValue(), RoundingMode.CEILING ) );
+ Optional<Integer> scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
+ AtomicReference<FEELFnResult<BigDecimal>> toReturn = new AtomicReference<>();
Review Comment:
@yesamer @ChinchuAjith Since some of the current code has been influenced by my suggestion, let's discuss of this all together next Monday, ok ?
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2048298882
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/util/NumberEvalHelper.java:
##########
@@ -66,12 +67,47 @@ public static BigDecimal getBigDecimalOrNull(Object value) {
return null;
}
+ /**
+ * The method handles various numeric types and converts the given object to an Integer
+ * Returns null If the conversion is not possible.
+ *
+ * @param value : The object to be converted.
+ * @return : An Integer representation of the value, or null if conversion is not possible.
Review Comment:
Updated. Removed this method as the logic can be handled inside the main method itself. Please check
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2048303984
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -45,11 +47,14 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @Para
if ( scale == null ) {
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
}
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ Optional<Integer> scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
Review Comment:
I initially thought of this approach, but felt the other one was simpler and more readable. That said, your comment helped clear up the confusion. I've updated it as suggestedβthis version looks better.
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2048314971
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DecimalFunction.java:
##########
@@ -38,14 +40,15 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @Para
if ( n == null ) {
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
}
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
+ Optional<Integer> scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
+ if(scaleObj.isEmpty()) {
+ return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be a non-null Number value."));
}
+ int scaleInt = scaleObj.get();
// Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ if (scaleInt < -6111 || scaleInt > 6176) {
Review Comment:
Method modified as suggested
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2066056435
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/RoundUpFunction.java:
##########
@@ -38,17 +41,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n) {
return invoke(n, BigDecimal.ZERO);
}
- public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @ParameterName( "scale" ) BigDecimal scale) {
- if ( n == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
- }
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
- }
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ public FEELFnResult<BigDecimal> invoke(@ParameterName("n") BigDecimal n, @ParameterName("scale") BigDecimal scale) {
+ try {
+ int coercedN = coerceIntegerNumber(n).orElseThrow(() -> new NoSuchElementException("n"));
Review Comment:
Reviewed the spec and found no reference to coercion for variable n, so I removed it. The null check has been retained.
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2066056895
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/RoundHalfUpFunction.java:
##########
@@ -38,17 +41,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n) {
return invoke(n, BigDecimal.ZERO);
}
- public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @ParameterName( "scale" ) BigDecimal scale) {
- if ( n == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
- }
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
- }
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ public FEELFnResult<BigDecimal> invoke(@ParameterName("n") BigDecimal n, @ParameterName("scale") BigDecimal scale) {
+ try {
+ int coercedN = coerceIntegerNumber(n).orElseThrow(() -> new NoSuchElementException("n"));
Review Comment:
Reviewed the spec and found no reference to coercion for variable n, so I removed it. The null check has been retained.
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2063929126
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/TimeFunction.java:
##########
@@ -181,4 +181,11 @@ protected FEELFnResult<TemporalAccessor> manageDateTimeException(DateTimeExcepti
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "from", e));
}
+ private FEELFnResult<TemporalAccessor> checkParamIfNull(String paramName, Number value) {
+ if (NumberEvalHelper.coerceIntegerNumber(value) == null) {
Review Comment:
This is an unused method. So removed
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2055822233
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DateAndTimeFunction.java:
##########
@@ -119,71 +124,57 @@ public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" ) Number yea
return invoke( year, month, day, hour, minute, second, (Number) null );
}
- public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" ) Number year, @ParameterName( "month" ) Number month, @ParameterName( "day" ) Number day,
- @ParameterName( "hour" ) Number hour, @ParameterName( "minute" ) Number minute, @ParameterName( "second" ) Number second,
- @ParameterName( "hour offset" ) Number hourOffset ) {
- if ( year == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "year", "cannot be null"));
- }
- if ( month == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "month", "cannot be null"));
- }
- if ( day == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "day", "cannot be null"));
- }
- if ( hour == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "hour", "cannot be null"));
- }
- if ( minute == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "minute", "cannot be null"));
- }
- if ( second == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "second", "cannot be null"));
- }
-
+ public FEELFnResult<TemporalAccessor> invoke(@ParameterName("year") Number year, @ParameterName("month") Number month, @ParameterName("day") Number day,
+ @ParameterName("hour") Number hour, @ParameterName("minute") Number minute, @ParameterName("second") Number second,
+ @ParameterName("hour offset") Number hourOffset) {
try {
- if( hourOffset != null ) {
- return FEELFnResult.ofResult( OffsetDateTime.of( year.intValue(), month.intValue(), day.intValue(),
- hour.intValue(), minute.intValue(), second.intValue(),
- 0, ZoneOffset.ofHours( hourOffset.intValue() ) ) );
+ int coercedYear = coerceIntegerNumber(year).orElseThrow();
+ int coercedMonth = coerceIntegerNumber(month).orElseThrow();
+ int coercedDay = coerceIntegerNumber(day).orElseThrow();
+ int coercedHour = coerceIntegerNumber(hour).orElseThrow();
+ int coercedMinute = coerceIntegerNumber(minute).orElseThrow();
+ int coercedSecond = coerceIntegerNumber(second).orElseThrow();
Review Comment:
@ChinchuAjith Apologies, my bad. Please ignore my previous comment.
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2066058453
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/RoundHalfDownFunction.java:
##########
@@ -38,17 +41,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n) {
return invoke(n, BigDecimal.ZERO);
}
- public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @ParameterName( "scale" ) BigDecimal scale) {
- if ( n == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "n", "cannot be null"));
- }
- if ( scale == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
- }
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ public FEELFnResult<BigDecimal> invoke(@ParameterName("n") BigDecimal n, @ParameterName("scale") BigDecimal scale) {
+ try {
+ int coercedN = coerceIntegerNumber(n).orElseThrow(() -> new NoSuchElementException("n"));
Review Comment:
Reviewed the spec and found no reference to coercion for variable n, so I removed it. The null check has been retained.
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "bncriju (via GitHub)" <gi...@apache.org>.
bncriju commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2038930033
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +35,46 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new BigDecimal("10000000000.5"));
}
+ @Test
+ void coerceIntegerNumber_withBigDecimal() {
Review Comment:
Add a note that it truncates instead of rounding, to clarify behavior
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +35,46 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new BigDecimal("10000000000.5"));
}
+ @Test
+ void coerceIntegerNumber_withBigDecimal() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(new BigDecimal("99.99"));
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo(99);
+ }
+
+ @Test
+ void coerceIntegerNumber_withBigInteger() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(new BigInteger("1000"));
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo(1000);
+ }
+
+ @Test
+ void coerceIntegerNumber_withDouble() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(042.50);
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo(42);
+ }
+
+ @Test
+ void coerceIntegerNumber_withString() {
+ Object result = NumberEvalHelper.coerceIntegerNumber("123");
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo("123");
+ }
+
+ @Test
+ void coerceIntegerNumber_withInteger() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(42);
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo(42);
+ }
+
+ @Test
+ void coerceIntegerNumber_withLong() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(423L);
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo(423);
+ }
Review Comment:
Add a test for null input. Add boundary tests for the scale validation logic (e.g., exactly -6111, 6176, just outside the bounds).
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -45,11 +46,14 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @Para
if ( scale == null ) {
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
}
+ Object scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
// Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ if (scaleObj instanceof Integer) {
+ int scaleInt = (Integer) scaleObj;
+ if (scaleInt < -6111 || scaleInt > 6176) {
+ return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
+ }
Review Comment:
else case could be written here? (i.e., if scaleObj is not an Integer). currently, the flow silently skips validation.
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +35,46 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new BigDecimal("10000000000.5"));
}
+ @Test
+ void coerceIntegerNumber_withBigDecimal() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(new BigDecimal("99.99"));
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo(99);
+ }
+
+ @Test
+ void coerceIntegerNumber_withBigInteger() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(new BigInteger("1000"));
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo(1000);
+ }
+
+ @Test
+ void coerceIntegerNumber_withDouble() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(042.50);
Review Comment:
042.50 is invalid syntax in Java (leading 0 implies octal, and .50 isn't valid in octal). it could be corrected to 42.50
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DateFunction.java:
##########
@@ -72,16 +73,10 @@ public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "from" ) String val
}
public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" ) Number year, @ParameterName( "month" ) Number month, @ParameterName( "day" ) Number day) {
- if ( year == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "year", "cannot be null"));
- }
- if ( month == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "month", "cannot be null"));
- }
- if ( day == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "day", "cannot be null"));
- }
-
+ FEELFnResult<TemporalAccessor> result;
+ if ((result = checkNullParam("year", year)) != null) return result;
Review Comment:
checkNullParam could be renamed to checkParamIfNull
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2038968477
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +35,46 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new BigDecimal("10000000000.5"));
}
+ @Test
+ void coerceIntegerNumber_withBigDecimal() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(new BigDecimal("99.99"));
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo(99);
+ }
+
+ @Test
+ void coerceIntegerNumber_withBigInteger() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(new BigInteger("1000"));
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo(1000);
+ }
+
+ @Test
+ void coerceIntegerNumber_withDouble() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(042.50);
Review Comment:
Done
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2057703592
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -45,11 +48,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @Para
if ( scale == null ) {
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
}
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
- }
-
- return FEELFnResult.ofResult( n.setScale( scale.intValue(), RoundingMode.CEILING ) );
+ Optional<Integer> scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
+ AtomicReference<FEELFnResult<BigDecimal>> toReturn = new AtomicReference<>();
Review Comment:
@ChinchuAjith Thank you, I guess the performances are not impacted as you said (but the memory footprint is undoubtedly increased any time you create an AtomicReference).
In any case, despite your solution being correct from a functional point of view, I still believe that removing the `Optional` could result in a considerable simplification. From my point of view, the case you're managing is an INVALID case, not an OPTIONAL case. The difference is subtle, let me explain it:
Currently, you are:
- Coercing a `number` returning an `Optional`. If the given parameter is valid, the Optional holds the number. If the given parameter is INVALID, an empty optional is returned.
- The callers of the `NumberEvalHelper.coerceIntegerNumber` function will take the number, if present in the Optional, or throw an exception, strengthen my above statement that we are managing an INVALID case
- The exception is then caught and managed.
My proposal is NOT returning null in case of invalid parameters, but immediately throwing an exception in that case, simplifying the code because the second point of the above list is no longer necessary.
To be clear:
```
public static Integer coerceIntegerNumber(Object value) {
if ( value instanceof BigDecimal ) {
return ((BigDecimal) value).intValue();
}
if ( value instanceof BigInteger ) {
return ((BigInteger) value).intValue();
}
if ( value instanceof Number ) {
return ((Number) value).intValue();
}
throw new IllegalArgumentException("The provided value: " + value + " is not a number");
}
```
Ping me in private in case, I'm available to talk!
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "baldimir (via GitHub)" <gi...@apache.org>.
baldimir merged PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "ChinchuAjith (via GitHub)" <gi...@apache.org>.
ChinchuAjith commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2038967198
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/NumberEvalHelperTest.java:
##########
@@ -34,4 +35,46 @@ void getBigDecimalOrNull() {
assertThat(NumberEvalHelper.getBigDecimalOrNull(10000000000.5D)).isEqualTo(new BigDecimal("10000000000.5"));
}
+ @Test
+ void coerceIntegerNumber_withBigDecimal() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(new BigDecimal("99.99"));
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo(99);
+ }
+
+ @Test
+ void coerceIntegerNumber_withBigInteger() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(new BigInteger("1000"));
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo(1000);
+ }
+
+ @Test
+ void coerceIntegerNumber_withDouble() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(042.50);
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo(42);
+ }
+
+ @Test
+ void coerceIntegerNumber_withString() {
+ Object result = NumberEvalHelper.coerceIntegerNumber("123");
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo("123");
+ }
+
+ @Test
+ void coerceIntegerNumber_withInteger() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(42);
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo(42);
+ }
+
+ @Test
+ void coerceIntegerNumber_withLong() {
+ Object result = NumberEvalHelper.coerceIntegerNumber(423L);
+ assertThat(result).isNotNull();
+ assertThat(result).isEqualTo(423);
+ }
Review Comment:
Added new test case with null input
--
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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "kie-ci3 (via GitHub)" <gi...@apache.org>.
kie-ci3 commented on PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#issuecomment-2797084495
**PR job** `#6` 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/6299 --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-6299/6/display/redirect
**Test results:**
- PASSED: 23398
- FAILED: 16
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-6299/6/testReport/org.kie.dmn.feel.documentation/ADocFEELExamplesTest/test/">org.kie.dmn.feel.documentation.ADocFEELExamplesTest.test</a></summary>
sublist( [4,5,6], 1, 2 ) = [4,5]
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime/FEELFunctionsTest/instanceTest(String,_Object,_Severity,_FEEL_TARGET,_Boolean,_FEELDialect)[177]/">org.kie.dmn.feel.runtime.FEELFunctionsTest.instanceTest(String, Object, Severity, FEEL_TARGET, Boolean, FEELDialect)[177]</a></summary>
[Evaluating: 'sublist( [1, 2, 3, 4, 5 ], 3, 2 )'] <br/>expected: [3, 4]<br/> but was: null
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime/FEELFunctionsTest/instanceTest(String,_Object,_Severity,_FEEL_TARGET,_Boolean,_FEELDialect)[178]/">org.kie.dmn.feel.runtime.FEELFunctionsTest.instanceTest(String, Object, Severity, FEEL_TARGET, Boolean, FEELDialect)[178]</a></summary>
[Evaluating: 'sublist( [1, 2, 3, 4, 5 ], 3, 2 )'] <br/>expected: [3, 4]<br/> but was: null
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime/FEELFunctionsTest/instanceTest(String,_Object,_Severity,_FEEL_TARGET,_Boolean,_FEELDialect)[179]/">org.kie.dmn.feel.runtime.FEELFunctionsTest.instanceTest(String, Object, Severity, FEEL_TARGET, Boolean, FEELDialect)[179]</a></summary>
[Evaluating: 'sublist( [1, 2, 3, 4, 5 ], -2, 1 )'] <br/>expected: [4]<br/> but was: null
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime/FEELFunctionsTest/instanceTest(String,_Object,_Severity,_FEEL_TARGET,_Boolean,_FEELDialect)[180]/">org.kie.dmn.feel.runtime.FEELFunctionsTest.instanceTest(String, Object, Severity, FEEL_TARGET, Boolean, FEELDialect)[180]</a></summary>
[Evaluating: 'sublist( [1, 2, 3, 4, 5 ], -2, 1 )'] <br/>expected: [4]<br/> but was: null
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime/FEELFunctionsTest/instanceTest(String,_Object,_Severity,_FEEL_TARGET,_Boolean,_FEELDialect)[187]/">org.kie.dmn.feel.runtime.FEELFunctionsTest.instanceTest(String, Object, Severity, FEEL_TARGET, Boolean, FEELDialect)[187]</a></summary>
[Evaluating: 'sublist( [1, 2, 3, 4, 5 ], -5, 3 )'] <br/>expected: [1, 2, 3]<br/> but was: null
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime/FEELFunctionsTest/instanceTest(String,_Object,_Severity,_FEEL_TARGET,_Boolean,_FEELDialect)[188]/">org.kie.dmn.feel.runtime.FEELFunctionsTest.instanceTest(String, Object, Severity, FEEL_TARGET, Boolean, FEELDialect)[188]</a></summary>
[Evaluating: 'sublist( [1, 2, 3, 4, 5 ], -5, 3 )'] <br/>expected: [1, 2, 3]<br/> but was: null
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime/FEELFunctionsTest/instanceTest(String,_Object,_Severity,_FEEL_TARGET,_Boolean,_FEELDialect)[189]/">org.kie.dmn.feel.runtime.FEELFunctionsTest.instanceTest(String, Object, Severity, FEEL_TARGET, Boolean, FEELDialect)[189]</a></summary>
[Evaluating: 'sublist( [1, 2, 3, 4, 5 ], 1, 3 )'] <br/>expected: [1, 2, 3]<br/> but was: null
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime/FEELFunctionsTest/instanceTest(String,_Object,_Severity,_FEEL_TARGET,_Boolean,_FEELDialect)[190]/">org.kie.dmn.feel.runtime.FEELFunctionsTest.instanceTest(String, Object, Severity, FEEL_TARGET, Boolean, FEELDialect)[190]</a></summary>
[Evaluating: 'sublist( [1, 2, 3, 4, 5 ], 1, 3 )'] <br/>expected: [1, 2, 3]<br/> but was: null
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime/FEELFunctionsTest/instanceTest(String,_Object,_Severity,_FEEL_TARGET,_Boolean,_FEELDialect)[191]/">org.kie.dmn.feel.runtime.FEELFunctionsTest.instanceTest(String, Object, Severity, FEEL_TARGET, Boolean, FEELDialect)[191]</a></summary>
[Evaluating: 'sublist( 1, 1, 1 )'] <br/>expected: [1]<br/> but was: null
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime/FEELFunctionsTest/instanceTest(String,_Object,_Severity,_FEEL_TARGET,_Boolean,_FEELDialect)[192]/">org.kie.dmn.feel.runtime.FEELFunctionsTest.instanceTest(String, Object, Severity, FEEL_TARGET, Boolean, FEELDialect)[192]</a></summary>
[Evaluating: 'sublist( 1, 1, 1 )'] <br/>expected: [1]<br/> but was: null
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime/FEELFunctionsTest/instanceTest(String,_Object,_Severity,_FEEL_TARGET,_Boolean,_FEELDialect)[193]/">org.kie.dmn.feel.runtime.FEELFunctionsTest.instanceTest(String, Object, Severity, FEEL_TARGET, Boolean, FEELDialect)[193]</a></summary>
[Evaluating: 'sublist( 1, 1 )'] <br/>expected: [1]<br/> but was: null
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime/FEELFunctionsTest/instanceTest(String,_Object,_Severity,_FEEL_TARGET,_Boolean,_FEELDialect)[194]/">org.kie.dmn.feel.runtime.FEELFunctionsTest.instanceTest(String, Object, Severity, FEEL_TARGET, Boolean, FEELDialect)[194]</a></summary>
[Evaluating: 'sublist( 1, 1 )'] <br/>expected: [1]<br/> but was: null
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime.functions/RangeFunctionTest/evaluateWithInvalidFunctionInvocationNode/">org.kie.dmn.feel.runtime.functions.RangeFunctionTest.evaluateWithInvalidFunctionInvocationNode</a></summary>
Expected 'retrieved.isLeft()' from, [sublist( [1, 2, 3, 4, 5 ], 3, 2 )..sublist( [1, 2, 3, 4, 5 ], 3, 2 )]
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime.functions/SublistFunctionTest/invokeStartPositive/">org.kie.dmn.feel.runtime.functions.SublistFunctionTest.invokeStartPositive</a></summary>
<br/>Expecting value to be true but was false
</details>
<details>
<summary><a href="https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6299/6/testReport/org.kie.dmn.feel.runtime.functions/SublistFunctionTest/invokeStartNegative/">org.kie.dmn.feel.runtime.functions.SublistFunctionTest.invokeStartNegative</a></summary>
<br/>Expecting value to be true but was false
</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] [DMN 1.6] Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2037104791
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -6,9 +6,7 @@
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
+ * http://www.apache.org/licenses/LICENSE-2.0
Review Comment:
Please use the same format applied on other classes
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2055808560
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/DateAndTimeFunction.java:
##########
@@ -119,71 +124,57 @@ public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" ) Number yea
return invoke( year, month, day, hour, minute, second, (Number) null );
}
- public FEELFnResult<TemporalAccessor> invoke(@ParameterName( "year" ) Number year, @ParameterName( "month" ) Number month, @ParameterName( "day" ) Number day,
- @ParameterName( "hour" ) Number hour, @ParameterName( "minute" ) Number minute, @ParameterName( "second" ) Number second,
- @ParameterName( "hour offset" ) Number hourOffset ) {
- if ( year == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "year", "cannot be null"));
- }
- if ( month == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "month", "cannot be null"));
- }
- if ( day == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "day", "cannot be null"));
- }
- if ( hour == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "hour", "cannot be null"));
- }
- if ( minute == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "minute", "cannot be null"));
- }
- if ( second == null ) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "second", "cannot be null"));
- }
-
+ public FEELFnResult<TemporalAccessor> invoke(@ParameterName("year") Number year, @ParameterName("month") Number month, @ParameterName("day") Number day,
+ @ParameterName("hour") Number hour, @ParameterName("minute") Number minute, @ParameterName("second") Number second,
+ @ParameterName("hour offset") Number hourOffset) {
try {
- if( hourOffset != null ) {
- return FEELFnResult.ofResult( OffsetDateTime.of( year.intValue(), month.intValue(), day.intValue(),
- hour.intValue(), minute.intValue(), second.intValue(),
- 0, ZoneOffset.ofHours( hourOffset.intValue() ) ) );
+ int coercedYear = coerceIntegerNumber(year).orElseThrow();
+ int coercedMonth = coerceIntegerNumber(month).orElseThrow();
+ int coercedDay = coerceIntegerNumber(day).orElseThrow();
+ int coercedHour = coerceIntegerNumber(hour).orElseThrow();
+ int coercedMinute = coerceIntegerNumber(minute).orElseThrow();
+ int coercedSecond = coerceIntegerNumber(second).orElseThrow();
Review Comment:
@ChinchuAjith `orElseThrow()` with an empty parameter will throw an NPE. Is that the expected behaviour?
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2055845301
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -45,11 +48,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @Para
if ( scale == null ) {
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
}
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
- }
-
- return FEELFnResult.ofResult( n.setScale( scale.intValue(), RoundingMode.CEILING ) );
+ Optional<Integer> scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
+ AtomicReference<FEELFnResult<BigDecimal>> toReturn = new AtomicReference<>();
Review Comment:
@ChinchuAjith @gitgabrio @baldimir I'm not an expert, but using an AtomicReference here just to handle a callback result isn't too much in terms of performance?
--
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] [incubator-kie-issues#1911] DMN 1.6: Add a type conversion from decimal to integer with Test cases [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6299:
URL: https://github.com/apache/incubator-kie-drools/pull/6299#discussion_r2055855670
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/CeilingFunction.java:
##########
@@ -45,11 +48,16 @@ public FEELFnResult<BigDecimal> invoke(@ParameterName( "n" ) BigDecimal n, @Para
if ( scale == null ) {
return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "cannot be null"));
}
- // Based on Table 76: Semantics of numeric functions, the scale is in range β6111 .. 6176
- if (scale.compareTo(BigDecimal.valueOf(-6111)) < 0 || scale.compareTo(BigDecimal.valueOf(6176)) > 0) {
- return FEELFnResult.ofError(new InvalidParametersEvent(Severity.ERROR, "scale", "must be in range between -6111 to 6176."));
- }
-
- return FEELFnResult.ofResult( n.setScale( scale.intValue(), RoundingMode.CEILING ) );
+ Optional<Integer> scaleObj = NumberEvalHelper.coerceIntegerNumber(scale);
+ AtomicReference<FEELFnResult<BigDecimal>> toReturn = new AtomicReference<>();
Review Comment:
@ChinchuAjith @gitgabrio @baldimir Personally, I would avoid returning the `Optional` from the `NumberEvalHelper.coerceIntegerNumber`. My suggestion is to always return an `Integer`. If an invalid (not a number) parameter is passed, we can just throw `IllegalArgumentException`. This would simplify the code in the `*Function` classes and remove the need for the AtomicReference. WDYT?
--
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