Copilot commented on code in PR #6557:
URL:
https://github.com/apache/incubator-kie-drools/pull/6557#discussion_r2711935826
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java:
##########
@@ -158,15 +159,68 @@ public UnaryTest getUnaryTest() {
* For a Unary Test an = (equal) semantic depends on the RIGHT value.
* If the RIGHT is NOT a list, then standard equals semantic applies
* If the RIGHT is a LIST, then the semantic is "right contains left"
+ * When both are Collections:
+ * - Verify that the two objects have the same size
+ * - Verify that the element at each position in the left object equals
the element at the same position in the right object.
*/
private Boolean utEqualSemantic(Object left, Object right) {
- if (right instanceof Collection) {
- return ((Collection) right).contains(left);
+ if (left instanceof Collection && right instanceof Collection) {
+ return areCollectionsEqual((Collection<?>) left, (Collection<?>)
right);
+ } else if (right instanceof Collection) {
+ return isElementInCollection((Collection<?>) right, left);
} else {
- // evaluate single entity
- return DefaultDialectHandler.isEqual(left, right, () -> (left ==
null && right == null), () -> Boolean.FALSE);
+ return areElementsEqual(left, right);
+ }
+ }
+
+ /**
+ * Checks if two collections are equal by comparing elements in order.
+ * Both collections must have the same size and each element at position i
in left
+ * must equal the element at position i in right.
+ *
+ * @param left the left collection
+ * @param right the right collection
+ * @return true if collections have same size and elements match in order,
false otherwise
+ */
+ static Boolean areCollectionsEqual(Collection<?> left, Collection<?>
right) {
+ if (left.size() != right.size()) {
+ return false;
+ }
+ Iterator<?> leftIterator = left.iterator();
+ Iterator<?> rightIterator = right.iterator();
+ while (leftIterator.hasNext() && rightIterator.hasNext()) {
+ if (!areElementsEqual(leftIterator.next(), rightIterator.next())) {
+ return false;
+ }
}
+ return true;
+ }
+
+ /**
+ * Checks if a collection contains a specific element.
+ *
+ * @param collection the collection to search in
+ * @param element the element to search for
+ * @return true if collection contains the element, false otherwise
+ */
+ static Boolean isElementInCollection(Collection<?> collection, Object
element) {
+ return collection.contains(element);
Review Comment:
The isElementInCollection method uses the standard Collection.contains
method, which relies on the equals implementation of the elements. This may not
correctly handle FEEL-specific types that require custom equality semantics,
such as Range objects or other types that need special comparison logic as
implemented in DefaultDialectHandler.isEqual. Consider iterating through the
collection and using areElementsEqual for each comparison to ensure consistent
FEEL equality semantics across all types.
```suggestion
* Checks if a collection contains a specific element using FEEL
equality semantics.
*
* @param collection the collection to search in
* @param element the element to search for
* @return true if collection contains the element, false otherwise
*/
static Boolean isElementInCollection(Collection<?> collection, Object
element) {
if (collection == null) {
return false;
}
for (Object item : collection) {
if (areElementsEqual(item, element)) {
return true;
}
}
return false;
```
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/lang/ast/UnaryTestNodeTest.java:
##########
@@ -0,0 +1,223 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * 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
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.kie.dmn.feel.lang.ast;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class UnaryTestNodeTest {
+
+ @Test
+ void testAreCollectionsEqualInOrder_withEqualCollections() {
+ List<Integer> left = Arrays.asList(1, 2, 3);
+ List<Integer> right = Arrays.asList(1, 2, 3);
+
+ Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+ assertThat(result).isTrue();
+ }
+
+ @Test
+ void testAreCollectionsEqualInOrder_withDifferentSizes() {
+ List<Integer> left = Arrays.asList(1, 2, 3);
+ List<Integer> right = Arrays.asList(1, 2, 3, 4);
+
+ Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+ assertThat(result).isFalse();
+ }
+
+ @Test
+ void testAreCollectionsEqualInOrder_withDifferentElements() {
+ List<Integer> left = Arrays.asList(1, 2, 3);
+ List<Integer> right = Arrays.asList(1, 5, 3);
+
+ Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+ assertThat(result).isFalse();
+ }
+
+ @Test
+ void testAreCollectionsEqualInOrder_withDifferentOrder() {
+ List<Integer> left = Arrays.asList(1, 2, 3);
+ List<Integer> right = Arrays.asList(3, 2, 1);
+
+ Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+ assertThat(result).isFalse();
+ }
+
+ @Test
+ void testAreCollectionsEqualInOrder_withEmptyCollections() {
+ List<Integer> left = Collections.emptyList();
+ List<Integer> right = Collections.emptyList();
+
+ Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+ assertThat(result).isTrue();
+ }
Review Comment:
The test method name "testAreCollectionsEqualInOrder_withEqualCollections"
includes "InOrder" but other related test methods like
"testAreCollectionsEqualInOrder_withDifferentSizes" and
"testAreCollectionsEqualInOrder_withDifferentElements" also include "InOrder".
However, the actual method being tested is named "areCollectionsEqual" without
any "InOrder" suffix. Consider either removing "InOrder" from all test method
names or renaming the method to "areCollectionsEqualInOrder" to maintain
consistency between method and test names.
##########
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/lang/ast/UnaryTestNodeTest.java:
##########
@@ -0,0 +1,223 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * 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
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.kie.dmn.feel.lang.ast;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class UnaryTestNodeTest {
+
+ @Test
+ void testAreCollectionsEqualInOrder_withEqualCollections() {
+ List<Integer> left = Arrays.asList(1, 2, 3);
+ List<Integer> right = Arrays.asList(1, 2, 3);
+
+ Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+ assertThat(result).isTrue();
+ }
+
+ @Test
+ void testAreCollectionsEqualInOrder_withDifferentSizes() {
+ List<Integer> left = Arrays.asList(1, 2, 3);
+ List<Integer> right = Arrays.asList(1, 2, 3, 4);
+
+ Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+ assertThat(result).isFalse();
+ }
+
+ @Test
+ void testAreCollectionsEqualInOrder_withDifferentElements() {
+ List<Integer> left = Arrays.asList(1, 2, 3);
+ List<Integer> right = Arrays.asList(1, 5, 3);
+
+ Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+ assertThat(result).isFalse();
+ }
+
+ @Test
+ void testAreCollectionsEqualInOrder_withDifferentOrder() {
+ List<Integer> left = Arrays.asList(1, 2, 3);
+ List<Integer> right = Arrays.asList(3, 2, 1);
+
+ Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+ assertThat(result).isFalse();
+ }
+
+ @Test
+ void testAreCollectionsEqualInOrder_withEmptyCollections() {
+ List<Integer> left = Collections.emptyList();
+ List<Integer> right = Collections.emptyList();
+
+ Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+ assertThat(result).isTrue();
+ }
+
+ @Test
+ void testAreCollectionsEqualInOrder_withNullElements() {
+ List<Integer> left = Arrays.asList(1, null, 3);
+ List<Integer> right = Arrays.asList(1, null, 3);
+
+ Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+ assertThat(result).isTrue();
+ }
+
+ @Test
+ void testAreCollectionsEqualInOrder_withMismatchedNulls() {
+ List<Integer> left = Arrays.asList(1, null, 3);
+ List<Integer> right = Arrays.asList(1, 2, 3);
+
+ Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+ assertThat(result).isFalse();
+ }
+
+ @Test
+ void testAreCollectionsEqualInOrder_withEqualStringCollections() {
+ List<String> left = Arrays.asList("a", "b", "c");
+ List<String> right = Arrays.asList("a", "b", "c");
+
+ Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+ assertThat(result).isTrue();
+ }
+
+ @Test
+ void testCollectionContainsListElement() {
+ List<Integer> element = Arrays.asList(1, 2, 3);
+ List<List<Integer>> collection = Arrays.asList(
+ Arrays.asList(1, 2, 3, 4), Arrays.asList(1, 2, 3));
+
+ Boolean result = UnaryTestNode.isElementInCollection(collection,
element);
+ assertThat(result).isTrue();
+ }
+
+ @Test
+ void testListInCollection_NoMatch() {
+ List<Integer> element = Arrays.asList(1, 2, 3);
+ List<List<Integer>> collection = Arrays.asList(
+ Arrays.asList(1, 2, 3, 4), Arrays.asList(1, 2));
+
+ Boolean result = UnaryTestNode.isElementInCollection(collection,
element);
+ assertThat(result)
+ .as("List [1,2,3] should not be found in collection ([1,2,3,4],
[1,2])")
+ .isFalse();
+ }
+
+ @Test
+ void testCollectionContainsElement_whenElementExists() {
+ List<Integer> collection = Arrays.asList(1, 2, 3, 4, 5);
+ Integer element = 3;
+
+ Boolean result = UnaryTestNode.isElementInCollection(collection,
element);
+ assertThat(result).isTrue();
+ }
+
+ @Test
+ void testCollectionContainsElement_whenElementDoesNotExist() {
+ List<Integer> collection = Arrays.asList(1, 2, 3, 4, 5);
+ Integer element = 10;
+
+ Boolean result = UnaryTestNode.isElementInCollection(collection,
element);
+ assertThat(result).isFalse();
+ }
+
+ @Test
+ void testCollectionContainsElement_withNullElement() {
+ List<Integer> collection = Arrays.asList(1, null, 3);
+ Object element = null;
+
+ Boolean result = UnaryTestNode.isElementInCollection(collection,
element);
+ assertThat(result).isTrue();
+ }
+
+ @Test
+ void testCollectionContainsElement_withEmptyCollection() {
+ List<Integer> collection = Collections.emptyList();
+ Integer element = 1;
+
+ Boolean result = UnaryTestNode.isElementInCollection(collection,
element);
+ assertThat(result).isFalse();
+ }
+
+ @Test
+ void testAreElementsEqual_withEqualIntegers() {
+ Integer left = 42;
+ Integer right = 42;
+
+ Boolean result = UnaryTestNode.areElementsEqual(left, right);
+ assertThat(result).isTrue();
+ }
+
+ @Test
+ void testAreElementsEqual_withDifferentIntegers() {
+ Integer left = 42;
+ Integer right = 24;
+
+ Boolean result = UnaryTestNode.areElementsEqual(left, right);
+ assertThat(result).isFalse();
+ }
+
+ @Test
+ void testAreElementsEqual_withEqualStrings() {
+ String left = "hello";
+ String right = "hello";
+
+ Boolean result = UnaryTestNode.areElementsEqual(left, right);
+ assertThat(result).isTrue();
+ }
+
+ @Test
+ void testAreElementsEqual_withDifferentStrings() {
+ String left = "hello";
+ String right = "world";
+
+ Boolean result = UnaryTestNode.areElementsEqual(left, right);
+ assertThat(result).isFalse();
+ }
+
+ @Test
+ void testAreElementsEqual_withBothNull() {
+ Object left = null;
+ Object right = null;
+
+ Boolean result = UnaryTestNode.areElementsEqual(left, right);
+ assertThat(result).isTrue();
+ }
+
+ @Test
+ void testAreElementsEqual_withOneNull() {
+ Integer left = 42;
+ Object right = null;
+
+ Boolean result = UnaryTestNode.areElementsEqual(left, right);
+ assertThat(result).isFalse();
+ }
+
+ @Test
+ void testAreElementsEqual_withNullLeft() {
+ Object left = null;
+ Integer right = 42;
+
+ Boolean result = UnaryTestNode.areElementsEqual(left, right);
+ assertThat(result).isFalse();
+ }
+}
Review Comment:
While the unit tests for areCollectionsEqual, isElementInCollection, and
areElementsEqual provide good coverage of the helper methods, there appear to
be no integration tests that verify the actual FEEL expression evaluation for
list comparisons. Consider adding integration tests in FEELListsTest or
FEELOperatorsTest that test expressions like "[1,2,3] = [1,2,3]" or "[1,2,3] in
[[1,2,3], [4,5,6]]" to ensure the full end-to-end functionality works correctly.
##########
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/parser/feel11/ASTBuilderVisitor.java:
##########
@@ -184,14 +184,16 @@ public BaseNode
visitInterval(FEEL_1_1Parser.IntervalContext ctx) {
public BaseNode
visitPositiveUnaryTestIneq(FEEL_1_1Parser.PositiveUnaryTestIneqContext ctx) {
BaseNode value = visit( ctx.endpoint() );
String op = ctx.op.getText();
- UnaryOperator unaryOperator = UnaryOperator.determineOperator(op);
return ASTBuilderFactory.newUnaryTestNode( ctx, op, value );
}
@Override
public BaseNode
visitPositiveUnaryTestIneqInterval(FEEL_1_1Parser.PositiveUnaryTestIneqIntervalContext
ctx) {
BaseNode value = visit(ctx.endpoint());
String op = ctx.op.getText();
+ if (value instanceof ListNode) {
Review Comment:
The code now creates a UnaryTestNode for any operator when the value is a
ListNode, including comparison operators like GT, LT, GTE, and LTE. However,
these comparison operators may not have well-defined semantics when applied to
lists. Consider either restricting this early return to only the EQ operator
(which is the one being fixed based on the issue), or ensuring that
UnaryTestNode.getUnaryTest properly handles or rejects comparison operators
with list values.
```suggestion
if (value instanceof ListNode && UnaryOperator.determineOperator(op)
== UnaryOperator.EQ) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
