Refactor MutablePathResult

Co-authored-by: Veselin Nikolov <veselin.nikolov@neotechnology.com>

Author: IoannisPanagiotas

algo/src/main/java/org/neo4j/gds/paths/yens/MutablePathResult.java

@@ -134,7 +134,6 @@ boolean matchesExactly(MutablePathResult path, int index) {
         if (relationshipIds.length == 0 || path.relationshipIds.length == 0) {
             return matches(path, index);
         }
-        //System.out.println(Arrays.toString(Arrays.stream(relationshipIds).toArray()));
         for (int i = 0; i < index; i++) {
             if (nodeIds[i] != path.nodeIds[i]) {
                 return false;
@@ -157,7 +156,9 @@ boolean matchesExactly(MutablePathResult path, int index) {
      * The cost value associated with the last value in this path, is added to
      * the costs for each node in the second path.
      */
-    void append(MutablePathResult path, boolean shouldstore) {
+
+
+    private void append(MutablePathResult path, long[] relationships) {
         // spur node is end of first and beginning of second path
         assert nodeIds[nodeIds.length - 1] == path.nodeIds[0];
 
@@ -166,15 +167,10 @@ void append(MutablePathResult path, boolean shouldstore) {
         var newNodeIds = new long[oldLength + path.nodeIds.length - 1];
         var newCosts = new double[oldLength + path.nodeIds.length - 1];
 
-        var oldRelationshipIdsLength = relationshipIds.length;
-        var newRelationshipIds = new long[oldRelationshipIdsLength + path.relationshipIds.length];
-
         // copy node ids
         System.arraycopy(this.nodeIds, 0, newNodeIds, 0, oldLength);
         System.arraycopy(path.nodeIds, 1, newNodeIds, oldLength, path.nodeIds.length - 1);
-        // copy relationship ids
-        System.arraycopy(this.relationshipIds, 0, newRelationshipIds, 0, oldRelationshipIdsLength);
-        System.arraycopy(path.relationshipIds, 0, newRelationshipIds, oldRelationshipIdsLength, path.relationshipIds.length);
+
         // copy costs
         System.arraycopy(this.costs, 0, newCosts, 0, oldLength);
         System.arraycopy(path.costs, 1, newCosts, oldLength, path.costs.length - 1);
@@ -186,10 +182,41 @@ void append(MutablePathResult path, boolean shouldstore) {
         }
 
         this.nodeIds = newNodeIds;
-        this.relationshipIds = shouldstore ? newRelationshipIds : new long[0];
+        this.relationshipIds = relationships;
         this.costs = newCosts;
     }
 
+    void append(MutablePathResult path) {
+
+        var oldRelationshipIdsLength = relationshipIds.length;
+        var newRelationshipIds = new long[oldRelationshipIdsLength + path.relationshipIds.length];
+        // copy relationship ids
+        System.arraycopy(this.relationshipIds, 0, newRelationshipIds, 0, oldRelationshipIdsLength);
+        System.arraycopy(
+            path.relationshipIds,
+            0,
+            newRelationshipIds,
+            oldRelationshipIdsLength,
+            path.relationshipIds.length
+        );
+        
+        append(path, newRelationshipIds);
+    }
+
+    /**
+     * Appends the given path to this path without creating an explicit relationship array.
+     *
+     * The last node in this path, must match the first node in the given path.
+     * This node will only appear once in the resulting path.
+     * The cost value associated with the last value in this path, is added to
+     * the costs for each node in the second path.
+     */
+    void appendWithoutRelationshipIds(MutablePathResult path) {
+        // spur node is end of first and beginning of second path
+        append(path, new long[0]);
+    }
+
+
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;

algo/src/main/java/org/neo4j/gds/paths/yens/Yens.java

@@ -39,6 +39,7 @@
 import java.util.Comparator;
 import java.util.Optional;
 import java.util.PriorityQueue;
+import java.util.function.BiConsumer;
 import java.util.function.ToLongBiFunction;
 import java.util.stream.Stream;
 
@@ -55,6 +56,8 @@ public final class Yens extends Algorithm<DijkstraResult> {
     private final LongObjectScatterMap<LongHashSet> relationshipAvoidList;
     private final ToLongBiFunction
         <MutablePathResult, Integer> relationshipAvoidMapper;
+    private final BiConsumer<MutablePathResult, PathResult> pathAppender;
+
 
     /**
      * Configure Yens to compute at most one source-target shortest path.
@@ -109,10 +112,12 @@ private Yens(Graph graph, Dijkstra dijkstra, ShortestPathYensBaseConfig config,
             // if we are in a multi-graph, we  must store the relationships ids as they are
             //since two nodes may be connected by multiple relationships and we must know which to avoid
             relationshipAvoidMapper = (path, position) -> path.relationship(position);
+            pathAppender = (rootPath, spurPath) -> rootPath.append(MutablePathResult.of(spurPath));
         } else {
             //otherwise the graph has surely no parallel edges, we do not need to explicitly store relationship ids
             //we can just store endpoints, so that we know which nodes a node should avoid
             relationshipAvoidMapper = (path, position) -> path.node(position + 1);
+            pathAppender = (rootPath, spurPath) -> rootPath.appendWithoutRelationshipIds(MutablePathResult.of(spurPath));
         }
         dijkstra.withRelationshipFilter((source, target, relationshipId) ->
             !nodeAvoidList.contains(target)
@@ -196,7 +201,8 @@ public DijkstraResult compute() {
                 }
 
                 // Entire path is made up of the root path and spur path.
-                rootPath.append(MutablePathResult.of(spurPath.get()), graph.isMultiGraph());
+                pathAppender.accept(rootPath, spurPath.get());
+                
                 // Add the potential k-shortest path to the heap.
                 if (!candidates.contains(rootPath)) {
                     candidates.add(rootPath);
@@ -214,7 +220,7 @@ public DijkstraResult compute() {
         progressTracker.endSubTask();
 
         progressTracker.endSubTask();
-    
+
         return new DijkstraResult(kShortestPaths.stream().map(MutablePathResult::toPathResult));
     }
 

algo/src/test/java/org/neo4j/gds/paths/yens/MutablePathResultTest.java

@@ -170,7 +170,7 @@ void append() {
         var path1 = testPath(0, 1, 2);
         var path2 = testPath(2, 3, 4);
 
-        path1.append(path2, true);
+        path1.append(path2);
         var expected = testPath(0, 1, 2, 3, 4);
 
         assertEquals(expected, path1);
@@ -184,7 +184,7 @@ void appendWithOffset() {
         var expected = MutablePathResult.of(ImmutablePathResult.builder().nodeIds(0, 1, 2, 3, 4).relationshipIds(0, 1, 2, 3).costs(0, 1, 42, 55, 79).sourceNode(0).targetNode(2).index(2).build());
         //@formatter:on
 
-        p1.append(p2, true);
+        p1.append(p2);
 
         assertEquals(expected, p1);
         assertEquals(expected.totalCost(), p1.totalCost());
@@ -195,6 +195,6 @@ void appendFailedAssertion() {
         var path1 = testPath(0, 1, 2);
         var path2 = testPath(4, 3, 5);
 
-        assertThatThrownBy(() -> path1.append(path2, true)).isInstanceOf(AssertionError.class);
+        assertThatThrownBy(() -> path1.append(path2)).isInstanceOf(AssertionError.class);
     }
 }