From 86ccc92a6d5598b8080b16b0d157b61e6f4f7bf3 Mon Sep 17 00:00:00 2001 From: Michael Pollmeier Date: Fri, 19 Sep 2025 16:36:50 +0200 Subject: [PATCH] DiffGraph: allow simultaneous edge and node removal Prior to this, if a user tried to delete a node and an adjacent edge at the same time, we would run into this: ``` scala.MatchError: flatgraph.DiffGraphBuilder$RemoveEdge@7bcbf3a8 (of class flatgraph.DiffGraphBuilder$RemoveEdge) at flatgraph.DiffGraphApplier.splitUpdate$$anonfun$2(DiffGraphApplier.scala:228) ~[io.joern.flatgraph-core_3-0.1.25.jar:0.1.25] ``` Which was because the condition was on the `case RemoveEdge` filtered out that case and there's no fallback `case _` or similar. This makes it explicit and adds a test. --- .../scala/flatgraph/DiffGraphApplier.scala | 36 +++++----- .../src/test/scala/flatgraph/GraphTests.scala | 66 +++++++++++++++++++ 2 files changed, 86 insertions(+), 16 deletions(-) diff --git a/core/src/main/scala/flatgraph/DiffGraphApplier.scala b/core/src/main/scala/flatgraph/DiffGraphApplier.scala index f0db4ab0..ccd98275 100644 --- a/core/src/main/scala/flatgraph/DiffGraphApplier.scala +++ b/core/src/main/scala/flatgraph/DiffGraphApplier.scala @@ -196,24 +196,28 @@ private[flatgraph] class DiffGraphApplier( new EdgeRepr(inR.src, inR.dst, inR.edgeKind, inR.subSeq, setEdgeProperty.property), graph.schema.neighborOffsetArrayIndex(inR.src.nodeKind, Incoming, inR.edgeKind) ) - case edgeDeletion: RemoveEdge - if !AccessHelpers.isDeleted(edgeDeletion.edge.src) && !AccessHelpers.isDeleted(edgeDeletion.edge.dst) => - ndiff += 1 + case edgeDeletion: RemoveEdge => + if (AccessHelpers.isDeleted(edgeDeletion.edge.src) || AccessHelpers.isDeleted(edgeDeletion.edge.dst)) { + // the adjacent nodes have already been deleted - nothing more for us to do here + // and, importantly, we do not want to fail if that's the case, but simply relax and enjoy + } else { - /** This is the delEdge case. It is massively annoying. - * - * In order to support edge properties, we need to grab the right edge from e.src->e.dst. If we assume that our graph was built - * normally, i.e. edges were sequentially/batched added without the unsafe unidirectional edges, then our graph has the following - * invariant: The kth edge connecting A->B corresponds to the kth edge connecting B<-A This sucks big time, because edge removal - * is potentially O(N**2). The degenerate behavior occurs when we have ~k edges of the same type starting in src = X or ending in - * the same dst = X. Each deletion then costs us ~k, and if we delete all ~k edges we pay ~ k*k. - * - * But k~N is possible where N is the graph size! - */ - val (outR, inR) = normalizeRepresentation(edgeDeletion.edge) - insert(delEdges, outR, graph.schema.neighborOffsetArrayIndex(outR.src.nodeKind, Outgoing, outR.edgeKind)) - insert(delEdges, inR, graph.schema.neighborOffsetArrayIndex(inR.src.nodeKind, Incoming, inR.edgeKind)) + ndiff += 1 + /** This is the delEdge case. It is massively annoying. + * + * In order to support edge properties, we need to grab the right edge from e.src->e.dst. If we assume that our graph was built + * normally, i.e. edges were sequentially/batched added without the unsafe unidirectional edges, then our graph has the + * following invariant: The kth edge connecting A->B corresponds to the kth edge connecting B<-A This sucks big time, because + * edge removal is potentially O(N**2). The degenerate behavior occurs when we have ~k edges of the same type starting in src = + * X or ending in the same dst = X. Each deletion then costs us ~k, and if we delete all ~k edges we pay ~ k*k. + * + * But k~N is possible where N is the graph size! + */ + val (outR, inR) = normalizeRepresentation(edgeDeletion.edge) + insert(delEdges, outR, graph.schema.neighborOffsetArrayIndex(outR.src.nodeKind, Outgoing, outR.edgeKind)) + insert(delEdges, inR, graph.schema.neighborOffsetArrayIndex(inR.src.nodeKind, Incoming, inR.edgeKind)) + } case setNodeProperty: SetNodeProperty if !AccessHelpers.isDeleted(setNodeProperty.node) => ndiff += 1 val iter = setNodeProperty.property match { diff --git a/core/src/test/scala/flatgraph/GraphTests.scala b/core/src/test/scala/flatgraph/GraphTests.scala index 29d7782a..adc39050 100644 --- a/core/src/test/scala/flatgraph/GraphTests.scala +++ b/core/src/test/scala/flatgraph/GraphTests.scala @@ -524,6 +524,72 @@ class GraphTests extends AnyWordSpec with Matchers { testSerialization(g) } + "permit deleting an edge together with it's node" in { + // if a node get's deleted we automatically delete the adjacent edges + // but if a user explicitly removes the edge (additionally) we do not want to fail hard for that... + + var g = mkGraph() + debugDump(g) shouldBe + """#Node numbers (kindId, nnodes) (0: 4), total 4 + |Node kind 0. (eid, nEdgesOut, nEdgesIn): (0, 7 [dense], 7 [dense]), + | V0_0 [0] -> V0_2, V0_1, V0_3, V0_2, V0_1 + | V0_1 [0] <- V0_0, V0_3, V0_0 + | V0_2 [0] <- V0_0, V0_0, V0_3 + | V0_3 [0] -> V0_1, V0_2 + | V0_3 [0] <- V0_0 + |""".stripMargin + + val nodeToDelete = g.node(0, 0) + val edgeToDelete = Accessors.getEdgesOut(nodeToDelete, 0)(0) + DiffGraphApplier.applyDiff( + g, + new DiffGraphBuilder(schema) + .removeNode(nodeToDelete) + .removeEdge(edgeToDelete) + ) + + debugDump(g) shouldBe + """#Node numbers (kindId, nnodes) (0: 4), total 4 + |Node kind 0. (eid, nEdgesOut, nEdgesIn): (0, 2 [dense], 2 [dense]), + | V0_1 [0] <- V0_3 + | V0_2 [0] <- V0_3 + | V0_3 [0] -> V0_1, V0_2 + |""".stripMargin + + g = mkGraph() + DiffGraphApplier.applyDiff( + g, + new DiffGraphBuilder(schema) + .removeNode(g.node(0, 1)) + ) + debugDump(g) shouldBe + """#Node numbers (kindId, nnodes) (0: 4), total 4 + |Node kind 0. (eid, nEdgesOut, nEdgesIn): (0, 4 [dense], 4 [dense]), + | V0_0 [0] -> V0_2, V0_3, V0_2 + | V0_2 [0] <- V0_0, V0_0, V0_3 + | V0_3 [0] -> V0_2 + | V0_3 [0] <- V0_0 + |""".stripMargin + + testSerialization(g) + + g = mkGraph() + DiffGraphApplier.applyDiff( + g, + new DiffGraphBuilder(schema) + .removeNode(g.node(0, 2)) + .removeNode(g.node(0, 3)) + ) + debugDump(g) shouldBe + """#Node numbers (kindId, nnodes) (0: 4), total 4 + |Node kind 0. (eid, nEdgesOut, nEdgesIn): (0, 2 [dense], 2 [dense]), + | V0_0 [0] -> V0_1, V0_1 + | V0_1 [0] <- V0_0, V0_0 + |""".stripMargin + + testSerialization(g) + } + "dont mess up after node deletion" in { val schema = TestSchema.make(1, 0, 1, nodePropertyPrototypes = Array(Array[String]("")), edgePropertyPrototypes = null) val g = new Graph(schema)