You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kie.apache.org by "yesamer (via GitHub)" <gi...@apache.org> on 2025/07/02 09:51:05 UTC
[PR] `range()` function shouldn't allow descendant ranges endpoints [incubator-kie-drools]
yesamer opened a new pull request, #6389:
URL: https://github.com/apache/incubator-kie-drools/pull/6389
Closes https://github.com/apache/incubator-kie-issues/issues/2017
According to the specs:
> Ranges
> FEEL supports a compact syntax for a range of values, useful in decision table test cells and elsewhere. Ranges can
> be syntactically represented either:
> a) as a comparison operator and a single endpoint (grammar rule 7.a.)
> b) or a pair of endpoints and endpoint inclusivity flags that indicate whether one or both endpoints are
> included in the range (grammar rule 7.b.); on this case, endpoints must be of equivalent types (see section
> 10.3.2.9.1for the definition of type equivalence) and **the endpoints must be ordered such that range start
> <= range end**
Ranges endpoints should be ascendant (i.e range start <= range end)
--
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] [kie-issues#2017] `range()` function shouldn't allow descendant range endpoints [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6389:
URL: https://github.com/apache/incubator-kie-drools/pull/6389#discussion_r2179875425
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/runtime/functions/RangeFunctionTest.java:
##########
@@ -356,6 +356,26 @@ void nodesEndpointsAscendant_True() {
assertThat(rangeFunction.nodesValueRangeAreAscending(left, right))
.withFailMessage("P2DT20H14M - P2DT20H15M")
.isTrue();
+ left = new NullNode("null");
Review Comment:
@gitgabrio It's the opposite, if there is at least one null endpoint, that method returns true. That method's scope is to return false when the two endpoints defines a descendant range, and with at least one null node that is NOT the case.
--
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] [kie-issues#2017] `range()` function shouldn't allow descendant range endpoints [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #6389:
URL: https://github.com/apache/incubator-kie-drools/pull/6389#discussion_r2179720640
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/RangeFunction.java:
##########
@@ -181,6 +186,18 @@ protected boolean nodesReturnsSameType(Object leftObject, Object rightObject) {
}
}
+ /**
+ * @param leftObject
+ * @param rightObject
+ * @return It checks if the leftObject is lower or equals to rightObject, false otherwise. If one of the endpoints is null,
+ * the endpoint range is considered to be in ascending order.
+ */
+ @SuppressWarnings("unchecked")
+ protected boolean nodesValueRangeAreAscending(Object leftObject, Object rightObject) {
Review Comment:
Nope, IINW. `NullNode` are concrete instances, not `null`. And I'm not sure/remember of what would be the behavior when one, or both, of the objects are `NullNode` instances
--
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] [kie-issues#2017] `range()` function shouldn't allow descendant range endpoints [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6389:
URL: https://github.com/apache/incubator-kie-drools/pull/6389#discussion_r2179680385
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/RangeFunction.java:
##########
@@ -181,6 +186,18 @@ protected boolean nodesReturnsSameType(Object leftObject, Object rightObject) {
}
}
+ /**
+ * @param leftObject
+ * @param rightObject
+ * @return It checks if the leftObject is lower or equals to rightObject, false otherwise. If one of the endpoints is null,
+ * the endpoint range is considered to be in ascending order.
+ */
+ @SuppressWarnings("unchecked")
+ protected boolean nodesValueRangeAreAscending(Object leftObject, Object rightObject) {
Review Comment:
@gitgabrio The null check in place doesn't cover that case?
--
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] [kie-issues#2017] `range()` function shouldn't allow descendant range endpoints [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #6389:
URL: https://github.com/apache/incubator-kie-drools/pull/6389#discussion_r2179859138
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/runtime/functions/RangeFunctionTest.java:
##########
@@ -356,6 +356,26 @@ void nodesEndpointsAscendant_True() {
assertThat(rangeFunction.nodesValueRangeAreAscending(left, right))
.withFailMessage("P2DT20H14M - P2DT20H15M")
.isTrue();
+ left = new NullNode("null");
Review Comment:
@yesamer @baldimir
here's the issue I wanted to highlight:
IIRC, NullNode are semantically valid value for one - or both - of the range ends.
But, looking this code, it seems that whenever there is a `NullNode`, the range would be invalid, since in either case `nodesValueRangeAreAscending` would fail.
Am I misreading/misunderstanding something 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] [kie-issues#2017] `range()` function shouldn't allow descendant range endpoints [incubator-kie-drools]
Posted by "yesamer (via GitHub)" <gi...@apache.org>.
yesamer commented on code in PR #6389:
URL: https://github.com/apache/incubator-kie-drools/pull/6389#discussion_r2179680385
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/RangeFunction.java:
##########
@@ -181,6 +186,18 @@ protected boolean nodesReturnsSameType(Object leftObject, Object rightObject) {
}
}
+ /**
+ * @param leftObject
+ * @param rightObject
+ * @return It checks if the leftObject is lower or equals to rightObject, false otherwise. If one of the endpoints is null,
+ * the endpoint range is considered to be in ascending order.
+ */
+ @SuppressWarnings("unchecked")
+ protected boolean nodesValueRangeAreAscending(Object leftObject, Object rightObject) {
Review Comment:
@yesamer The null check in place doesn't cover that case?
--
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] [kie-issues#2017] `range()` function shouldn't allow descendant range endpoints [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio merged PR #6389:
URL: https://github.com/apache/incubator-kie-drools/pull/6389
--
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] [kie-issues#2017] `range()` function shouldn't allow descendant range endpoints [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #6389:
URL: https://github.com/apache/incubator-kie-drools/pull/6389#discussion_r2179674653
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/runtime/functions/RangeFunction.java:
##########
@@ -181,6 +186,18 @@ protected boolean nodesReturnsSameType(Object leftObject, Object rightObject) {
}
}
+ /**
+ * @param leftObject
+ * @param rightObject
+ * @return It checks if the leftObject is lower or equals to rightObject, false otherwise. If one of the endpoints is null,
+ * the endpoint range is considered to be in ascending order.
+ */
+ @SuppressWarnings("unchecked")
+ protected boolean nodesValueRangeAreAscending(Object leftObject, Object rightObject) {
Review Comment:
Thanks @yesamer
Could you please check/consider also `NullNode` and what should be the behavior in that case ?
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/runtime/functions/RangeFunctionTest.java:
##########
@@ -329,6 +329,59 @@ void nodesReturnsSameType_False() {
.isFalse();
}
+ @Test
Review Comment:
Please look and consider also `NullNode` in tests, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org
Re: [PR] [kie-issues#2017] `range()` function shouldn't allow descendant range endpoints [incubator-kie-drools]
Posted by "gitgabrio (via GitHub)" <gi...@apache.org>.
gitgabrio commented on code in PR #6389:
URL: https://github.com/apache/incubator-kie-drools/pull/6389#discussion_r2179859138
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/runtime/functions/RangeFunctionTest.java:
##########
@@ -356,6 +356,26 @@ void nodesEndpointsAscendant_True() {
assertThat(rangeFunction.nodesValueRangeAreAscending(left, right))
.withFailMessage("P2DT20H14M - P2DT20H15M")
.isTrue();
+ left = new NullNode("null");
Review Comment:
@yesamer @baldimir
here's the issue I wanted to highlight:
IIRC, UndefinedValueComparable are semantically valid value for one - or both - of the range ends.
Am I misreading/misunderstanding something 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