fix: DSV graph should notify updater when cycles are formed or broken

Previously, the updater only considered nodes that were changed from an edge insertion or deletion. However, an edge insertion/deletion can also change the looped status of a disconnected node. For example, if we have a -> b and b -> a, and we remove a -> b, only b is marked as changed. This is incorrect, since a changed indirectly, since it is no longer looped. Thus, the graph implementations now mark any nodes whose SCC change from singleton to multinode (looped) and vice-versa.

Author: Christopher-Chianelli

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/BaseTopologicalOrderGraph.java

@@ -29,7 +29,7 @@ public interface BaseTopologicalOrderGraph {
 
     /**
      * Returns a tuple containing node ID and a number corresponding to its topological order.
-     * In particular, after {@link TopologicalOrderGraph#commitChanges()} is called, the following
+     * In particular, after {@link TopologicalOrderGraph#commitChanges(java.util.BitSet)} is called, the following
      * must be true for any pair of nodes A, B where:
      * <ul>
      * <li>A is a predecessor of B</li>

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultTopologicalOrderGraph.java

@@ -7,6 +7,7 @@
 import java.util.Map;
 import java.util.PrimitiveIterator;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import ai.timefold.solver.core.impl.util.CollectionUtils;
 import ai.timefold.solver.core.impl.util.MutableInt;
@@ -17,20 +18,27 @@ public class DefaultTopologicalOrderGraph implements TopologicalOrderGraph {
     private final Map<Integer, List<Integer>> componentMap;
     private final Set<Integer>[] forwardEdges;
     private final Set<Integer>[] backEdges;
+    private final boolean[] isNodeInLoopedComponent;
 
     @SuppressWarnings({ "unchecked" })
     public DefaultTopologicalOrderGraph(final int size) {
         this.nodeIdToTopologicalOrderMap = new NodeTopologicalOrder[size];
         this.componentMap = CollectionUtils.newLinkedHashMap(size);
         this.forwardEdges = new Set[size];
         this.backEdges = new Set[size];
+        this.isNodeInLoopedComponent = new boolean[size];
         for (var i = 0; i < size; i++) {
             forwardEdges[i] = new HashSet<>();
             backEdges[i] = new HashSet<>();
+            isNodeInLoopedComponent[i] = false;
             nodeIdToTopologicalOrderMap[i] = new NodeTopologicalOrder(i, i);
         }
     }
 
+    List<Integer> getComponent(int node) {
+        return componentMap.get(node);
+    }
+
     @Override
     public void addEdge(int fromNode, int toNode) {
         forwardEdges[fromNode].add(toNode);
@@ -88,7 +96,7 @@ public NodeTopologicalOrder getTopologicalOrder(int node) {
     }
 
     @Override
-    public void commitChanges() {
+    public void commitChanges(BitSet changed) {
         var index = new MutableInt(1);
         var stackIndex = new MutableInt(0);
         var size = forwardEdges.length;
@@ -108,12 +116,23 @@ public void commitChanges() {
         var ordIndex = 0;
         for (var i = components.size() - 1; i >= 0; i--) {
             var component = components.get(i);
-            var componentNodes = new ArrayList<Integer>(component.cardinality());
+            var componentSize = component.cardinality();
+            var isComponentLooped = componentSize != 1;
+            var componentNodes = new ArrayList<Integer>(componentSize);
             for (var node = component.nextSetBit(0); node >= 0; node = component.nextSetBit(node + 1)) {
                 nodeIdToTopologicalOrderMap[node] = new NodeTopologicalOrder(node, ordIndex);
                 componentNodes.add(node);
                 componentMap.put(node, componentNodes);
+
+                if (isComponentLooped != isNodeInLoopedComponent[node]) {
+                    // It is enough to only mark nodes whose component
+                    // status changed; the updater will notify descendants
+                    // since a looped status change force updates descendants.
+                    isNodeInLoopedComponent[node] = isComponentLooped;
+                    changed.set(node);
+                }
                 ordIndex++;
+
                 if (node == Integer.MAX_VALUE) {
                     break;
                 }
@@ -160,4 +179,19 @@ private void strongConnect(int node, MutableInt index, MutableInt stackIndex, in
             components.add(out);
         }
     }
+
+    @Override
+    public String toString() {
+        var out = new StringBuilder();
+        out.append("DefaultTopologicalOrderGraph{\n");
+        for (var node = 0; node < forwardEdges.length; node++) {
+            out.append("    ").append(node).append("(").append(nodeIdToTopologicalOrderMap[node].order()).append(") -> ")
+                    .append(forwardEdges[node].stream()
+                            .sorted()
+                            .map(Object::toString)
+                            .collect(Collectors.joining(",", "[", "]\n")));
+        }
+        out.append("}");
+        return out.toString();
+    }
 }

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DefaultVariableReferenceGraph.java

@@ -126,7 +126,7 @@ public void updateChanged() {
         if (changed.isEmpty()) {
             return;
         }
-        graph.commitChanges();
+        graph.commitChanges(changed);
         affectedEntitiesUpdater.accept(changed);
     }
 

core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/TopologicalOrderGraph.java

@@ -1,5 +1,6 @@
 package ai.timefold.solver.core.impl.domain.variable.declarative;
 
+import java.util.BitSet;
 import java.util.List;
 
 public interface TopologicalOrderGraph extends BaseTopologicalOrderGraph {
@@ -9,7 +10,7 @@ public interface TopologicalOrderGraph extends BaseTopologicalOrderGraph {
      * After this method returns, {@link #getTopologicalOrder(int)}
      * must be accurate for every node in the graph.
      */
-    void commitChanges();
+    void commitChanges(BitSet changed);
 
     /**
      * Called on graph creation to supply metadata about the graph nodes.
@@ -22,7 +23,7 @@ default <Solution_> void withNodeData(List<EntityVariablePair<Solution_>> nodes)
 
     /**
      * Called when a graph edge is added.
-     * The operation is added to a batch and only executed when {@link #commitChanges()} is called.
+     * The operation is added to a batch and only executed when {@link #commitChanges(BitSet)} is called.
      * <p>
      * {@link #getTopologicalOrder(int)} is allowed to be invalid
      * when this method returns.
@@ -31,7 +32,7 @@ default <Solution_> void withNodeData(List<EntityVariablePair<Solution_>> nodes)
 
     /**
      * Called when a graph edge is removed.
-     * The operation is added to a batch and only executed when {@link #commitChanges()} is called.
+     * The operation is added to a batch and only executed when {@link #commitChanges(BitSet)} is called.
      * <p>
      * {@link #getTopologicalOrder(int)} is allowed to be invalid
      * when this method returns.