Re: [PR] [Incubator kie issues#2028] Comparing lists using in does not work [incubator-kie-drools]
yesamer merged PR #6557: URL: https://github.com/apache/incubator-kie-drools/pull/6557 -- 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]
Re: [PR] [Incubator kie issues#2028] Comparing lists using in does not work [incubator-kie-drools]
gitgabrio commented on code in PR #6557:
URL:
https://github.com/apache/incubator-kie-drools/pull/6557#discussion_r2711945093
##
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 left = Arrays.asList(1, 2, 3);
Review Comment:
Thanks @AthiraHari77
All the methods invoking ""UnaryTest.areCollectionsEqual") could be replaced
with a single method and using the `Parameterized` semantic: this would make
test reading/writing easier (this comment has been done multiple times, in the
past, by some very expert member in that area)
--
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]
Re: [PR] [Incubator kie issues#2028] Comparing lists using in does not work [incubator-kie-drools]
gitgabrio commented on code in PR #6557:
URL:
https://github.com/apache/incubator-kie-drools/pull/6557#discussion_r2711946597
##
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 left = Arrays.asList(1, 2, 3);
+List right = Arrays.asList(1, 2, 3);
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isTrue();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withDifferentSizes() {
+List left = Arrays.asList(1, 2, 3);
+List right = Arrays.asList(1, 2, 3, 4);
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isFalse();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withDifferentElements() {
+List left = Arrays.asList(1, 2, 3);
+List right = Arrays.asList(1, 5, 3);
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isFalse();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withDifferentOrder() {
+List left = Arrays.asList(1, 2, 3);
+List right = Arrays.asList(3, 2, 1);
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isFalse();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withEmptyCollections() {
+List left = Collections.emptyList();
+List right = Collections.emptyList();
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isTrue();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withNullElements() {
+List left = Arrays.asList(1, null, 3);
+List right = Arrays.asList(1, null, 3);
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isTrue();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withMismatchedNulls() {
+List left = Arrays.asList(1, null, 3);
+List right = Arrays.asList(1, 2, 3);
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isFalse();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withEqualStringCollections() {
+List left = Arrays.asList("a", "b", "c");
+List right = Arrays.asList("a", "b", "c");
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isTrue();
+}
+
+@Test
+void testCollectionContainsListElement() {
+List element = Arrays.asList(1, 2, 3);
+List> 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() {
Review Comment:
same comment as before
--
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]
Re: [PR] [Incubator kie issues#2028] Comparing lists using in does not work [incubator-kie-drools]
gitgabrio commented on code in PR #6557:
URL:
https://github.com/apache/incubator-kie-drools/pull/6557#discussion_r2711934534
##
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java:
##
@@ -158,15 +159,66 @@ 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 elements in left object are contained, in the same
order, 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 rightIterator = right.iterator();
+return left.stream()
+.allMatch(leftElement -> {
+Object rightElement = rightIterator.next();
+return areElementsEqual(leftElement, rightElement);
+});
+}
+
+/**
+ * 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:
@yesamer @AthiraHari77 @ChinchuAjith
That comment seems reasonable (not the suggested code).
But, the original implementation was the Collection.contains: 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: [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]
Re: [PR] [Incubator kie issues#2028] Comparing lists using in does not work [incubator-kie-drools]
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.Collect
Re: [PR] [Incubator kie issues#2028] Comparing lists using in does not work [incubator-kie-drools]
Copilot commented on code in PR #6557:
URL:
https://github.com/apache/incubator-kie-drools/pull/6557#discussion_r2708223770
##
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/lang/ast/UnaryTestNodeTest.java:
##
@@ -0,0 +1,222 @@
+/*
+ * 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 left = Arrays.asList(1, 2, 3);
+List right = Arrays.asList(1, 2, 3);
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isTrue();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withDifferentSizes() {
+List left = Arrays.asList(1, 2, 3);
+List right = Arrays.asList(1, 2, 3, 4);
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isFalse();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withDifferentElements() {
+List left = Arrays.asList(1, 2, 3);
+List right = Arrays.asList(1, 5, 3);
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isFalse();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withDifferentOrder() {
+List left = Arrays.asList(1, 2, 3);
+List right = Arrays.asList(3, 2, 1);
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isFalse();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withEmptyCollections() {
+List left = Collections.emptyList();
+List right = Collections.emptyList();
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isTrue();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withNullElements() {
+List left = Arrays.asList(1, null, 3);
+List right = Arrays.asList(1, null, 3);
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isTrue();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withMismatchedNulls() {
+List left = Arrays.asList(1, null, 3);
+List right = Arrays.asList(1, 2, 3);
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isFalse();
+}
+
+
+@Test
+void testAreCollectionsEqualInOrder_withEqualCollections1() {
+List left = Arrays.asList("a", "b", "c");
+List right = Arrays.asList("a", "b", "c");
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isTrue();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withEqualCollections2() {
+List left = Arrays.asList(1, 2, 3);
+List right = Arrays.asList(1, 2, 3);
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isTrue();
+}
+
+@Test
+void testAreCollectionsEqualInOrder_withEqualCollections3() {
+List left = Arrays.asList(1, 2, 3);
+List right = Arrays.asList(1, 2, 3);
+
+Boolean result = UnaryTestNode.areCollectionsEqual(left, right);
+assertThat(result).isTrue();
+}
Review Comment:
The test methods testAreCollectionsEqualInOrder_withEqualCollections1,
testAreCollectionsEqualInOrder_withEqualCollections2, and
testAreCollectionsEqualInOrder_withEqualCollections3 are duplicates. They all
test the exact same scenario
(testAreCollectionsEqualInOrder_withEqualCollections2 and
testAreCollectionsEqualInOrder_withEqualCollections3 both use the same inputs
as the first test at lines 32-38). These duplicate tests should be removed or
modified to test distinct scenarios.
##
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/lang/ast/UnaryTestNode.java:
##
@@ -158,15 +159,66 @@ public UnaryTest getUnaryTest() {
* For a Unary Test an = (equal) semantic depends on t
