hwu2024 commented on PR #3187:
URL:
https://github.com/apache/incubator-kie-optaplanner/pull/3187#issuecomment-3614775829
Hi @Rikkola thanks for the review!
The core problem is that `setupValidNonsequential4OptMove` used
`Collectors.toMap()` with the default `HashMap`, and then iterated
`entityToListSize.entrySet()`. That makes the per‑entity lists depend on the
`HashMap` iteration order, which `HashMap` [makes no guarantees as to the order
of the map](https://docs.oracle.com/javase/8/docs/api/java/util/HashMap.html).
NonDex exposes this by shuffling that iterator and we can also trace the error
by running the debug mode:
```
mvn -pl core/optaplanner-core-impl
edu.illinois:nondex-maven-plugin:2.2.1:debug
-Dtest=org.optaplanner.core.impl.heuristic.selector.move.generic.list.kopt.KOptListMoveIteratorTest#testNonsequentialKOptOnDifferentEntity
```
which the debug stack shows that
```
java.util.HashMap$EntrySet.iterator(HashMap.java:1106)
org.optaplanner.core.impl.heuristic.selector.move.generic.list.kopt.KOptListMoveIteratorTest.setupValidNonsequential4OptMove(KOptListMoveIteratorTest.java:233)
```
We can also reproduce by manually building two maps with the same
keys/values but different iteration order (simulating `HashMap`), and runs the
same list‑building logic as `setupValidNonsequential4OptMove`.
```
@Test
void entityToListDependsOnMapIterationOrder() {
int k = 4;
Object[] data = new Object[2 * k + 8];
for (int i = 0; i < data.length; i++) {
data[i] = "v" + i;
}
Map entityToListSizeOrder1 = new LinkedHashMap<>();
entityToListSizeOrder1.put("e1", 6);
entityToListSizeOrder1.put("e2", 4);
Map entityToListSizeOrder2 = new LinkedHashMap<>();
entityToListSizeOrder2.put("e2", 4);
entityToListSizeOrder2.put("e1", 6);
Map> entityToList1 =
buildEntityToListLikeNonsequentialHelper(entityToListSizeOrder1,
data);
Map> entityToList2 =
buildEntityToListLikeNonsequentialHelper(entityToListSizeOrder2,
data);
assertThat(entityToList1).isEqualTo(entityToList2);
}
private static Map>
buildEntityToListLikeNonsequentialHelper(
Map entityToListSize, Object[] data) {
Map> entityToList = new HashMap<>();
int offset = 0;
for (Map.Entry entry : entityToListSize.entrySet()) {
Object entity = entry.getKey();
int listSize = entry.getValue();
List entityList = new
ArrayList<>(Arrays.asList(data).subList(offset, offset + listSize));
for (int i = 0; i < listSize; i++) {
entityList.add(2 * i + 1, entity + "-extra-" + i);
}
entityList.add(0, entity + "-start");
entityList.add(entity + "-end");
entityToList.put(entity, entityList);
offset += listSize;
}
return entityToList;
}
```
and if we run
```
mvn -pl core/optaplanner-core-impl test
-Dtest=org.optaplanner.core.impl.heuristic.selector.move.generic.list.kopt.KOptListMoveIteratorTest#entityToListDependsOnMapIterationOrder
```
we get
```
expected:
{"e1"=["e1-start",
"v4",
"e1-extra-0",
"v5",
"e1-extra-1",
"v6",
"e1-extra-2",
"v7",
"e1-extra-3",
"v8",
"e1-extra-4",
"v9",
"e1-extra-5",
"e1-end"], "e2"=["e2-start",
"v0",
"e2-extra-0",
"v1",
"e2-extra-1",
"v2",
"e2-extra-2",
"v3",
"e2-extra-3",
"e2-end"]}
but was:
{"e1"=["e1-start",
"v0",
"e1-extra-0",
"v1",
"e1-extra-1",
"v2",
"e1-extra-2",
"v3",
"e1-extra-3",
"v4",
"e1-extra-4",
"v5",
"e1-extra-5",
"e1-end"], "e2"=["e2-start",
"v6",
"e2-extra-0",
"v7",
"e2-extra-1",
"v8",
"e2-extra-2",
"v9",
"e2-extra-3",
"e2-end"]}
```
It shows that the old logic using default `HashMap` was order‑sensitive. The
`LinkedHashMap` change in this PR then removes that bug.
Thanks again for your time to follow up!
--
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]
