Aggregate is_deleted over an edge's layers - #2767
Conversation
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Rust Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: c151132 | Previous: 9823ef7 | Ratio |
|---|---|---|---|
lotr_graph/num_edges |
6 ns/iter (± 0) |
0 ns/iter (± 0) |
+∞ |
lotr_graph/num_nodes |
6 ns/iter (± 0) |
1 ns/iter (± 0) |
6 |
lotr_graph/graph_latest |
3 ns/iter (± 0) |
0 ns/iter (± 0) |
+∞ |
lotr_graph_materialise/materialize |
7932723 ns/iter (± 48676) |
1564816 ns/iter (± 35303) |
5.07 |
lotr_graph_window_100/num_nodes |
15 ns/iter (± 0) |
5 ns/iter (± 0) |
3 |
lotr_graph_window_100_materialise/materialize |
8065341 ns/iter (± 61840) |
1669150 ns/iter (± 10700) |
4.83 |
lotr_graph_window_10/has_node_existing |
140 ns/iter (± 9) |
62 ns/iter (± 11) |
2.26 |
lotr_graph_window_10_materialise/materialize |
3550833 ns/iter (± 11443) |
971980 ns/iter (± 4278) |
3.65 |
lotr_graph_subgraph_10pc_materialise/materialize |
2116400 ns/iter (± 30115) |
334634 ns/iter (± 1287) |
6.32 |
lotr_graph_subgraph_10pc_windowed/has_node_existing |
143 ns/iter (± 8) |
62 ns/iter (± 14) |
2.31 |
lotr_graph_subgraph_10pc_windowed_materialise/materialize |
1271172 ns/iter (± 9832) |
230399 ns/iter (± 2617) |
5.52 |
lotr_graph_window_50_layered/num_edges_temporal |
142546 ns/iter (± 2660) |
70121 ns/iter (± 7586) |
2.03 |
lotr_graph_window_50_layered/has_node_existing |
390 ns/iter (± 19) |
129 ns/iter (± 12) |
3.02 |
lotr_graph_window_50_layered/graph_latest |
83571 ns/iter (± 2145) |
36649 ns/iter (± 916) |
2.28 |
lotr_graph_window_50_layered_materialise/materialize |
30940094 ns/iter (± 153084) |
3488825 ns/iter (± 24948) |
8.87 |
lotr_graph_persistent_window_50_layered/num_edges_temporal |
598919 ns/iter (± 20564) |
192686 ns/iter (± 1569) |
3.11 |
lotr_graph_persistent_window_50_layered/has_node_existing |
416 ns/iter (± 353) |
174 ns/iter (± 83) |
2.39 |
lotr_graph_persistent_window_50_layered/iterate_exploded_edges |
3325902 ns/iter (± 24573) |
1659940 ns/iter (± 19402) |
2.00 |
lotr_graph_persistent_window_50_layered/graph_latest |
133686 ns/iter (± 1479) |
57549 ns/iter (± 4809) |
2.32 |
lotr_graph_persistent_window_50_layered_materialise/materialize |
52972775 ns/iter (± 384776) |
5298035 ns/iter (± 147912) |
10.00 |
This comment was automatically generated by workflow using github-action-benchmark.
|
Added two commits carrying over what was worth keeping from #2768 (now closed as a duplicate of the suites that came in with #2754):
One weakness left deliberately untouched, worth a follow-up: in Full suite: 3351 passed / 17 skipped / 9 xfailed. Filter suite: 585 passed. |
What changes were proposed in this pull request?
Fixes Pometry/pometry-storage#378 —
edges[is_deleted()]disagreed withEdgeView.is_deleted()whenever an edge's deletion was recorded on a different layer from the one holding the edge.IsDeletedGraphimplemented the predicate as a per-layer filter, and an edge passes a layer filter when any of its layers passes — so the filter answered "some layer has a deletion" while the method answers "no layer still holds this edge alive". It now implements the whole-edge filter, which receives the view's full layer set, so both spellings aggregate the same way. (IsSelfLoopGraphalready uses that hook, so this is the established shape rather than a new mechanism.)Also in this PR:
test_deletion_layer_semantics.pycrossing the three ways a deletion can sit relative to an edge's layers (same layer, another layer, unlayered delete of a layered edge) with both graph models, asserting filter ≡ method foris_deleted/is_valid, and thatvalid/deletedpartition onPersistentGraph.test_filter_edges_is_deletedand the GraphQLtest_edges_filter_window_is_deletedboth asserted the pre-fix answer.init_graph4adds edge(3, 4)on layerfire_nationand then deletes it without naming a layer, so the tombstone lands on_defaultand the edge stays alive onfire_nation— exactly the case Feature/dev 681 k8s diagram #378 is about. Both are now split by graph model, because the models legitimately differ here; the expectations were checked againstis_deleted()before being changed, and the persistent half additionally asserts thelayer("_default")view where the edge is deleted, so it does not pass merely by expecting nothing.test_edges_collection_filter.pyderives its set-algebra expectations from single-filter results, so a single filter that selects everything makes the derived expectation degenerate (EVERYTHING & X == Xis equally consistent with a correctandand one that dropped a term). On a build where a view filter fails open, everyview & predexpectation collapses onto the predicate and the pins report a live bug as fixed — with a message telling the reader to delete the rule. The pins now assert their baselines are proper subsets first.Why are the changes needed?
The filter and the method are two spellings of one question, so a disagreement makes either answer unsafe to rely on — and it showed up through the default delete path, since
delete_edge(t, src, dst)with no layer always tombstones_default. OnPersistentGraphit also broke an invariant the methods maintain: an edge appeared in bothedges[is_valid()]andedges[is_deleted()], though there an edge is either currently alive or currently deleted.The state itself is deliberate — recording a deletion on a layer the edge was never added to is what lets an out-of-order stream see a deletion before its addition — so the fix is in how the readers aggregate layers, not in rejecting the write.
The pin guard is here because this branch is where the problem is demonstrable: run that file against a build without the edge-collection time-view fix and it reports "now FIXED" for a class that is entirely broken.
Does this PR introduce any user-facing change? If yes is this documented?
Behavioural fix, no API change. On a
PersistentGraph, an edge alive on at least one layer of the view is no longer reported as deleted:The same applies to the GraphQL
isDeletedfilter. Event graphs are unaffected: thereis_valid/is_deletedare independent facts about the history rather than a partition, and filter and method already agreed.How was this patch tested?
cargo check -p raphtoryclean;cargo test -p raphtory --lib70 passed.test_base_install3351 passed / 17 skipped / 9 xfailed. (The fourtest_cli_parsingfailures are the pre-existing startup-timing flake: that test gives the server one second to boot and parses its stdout.)Are there any further changes required?
Yes, tracked in Pometry/pometry-storage#371: filter composition on the edge path still fails open —
A & viewdrops the view,A | viewand~viewreturn everything,~node-filteris not the complement, and node-kind filters are ignored on per-node edge collections. Negation over a composite containing a view (~(A & view)) is the same mechanism and is recorded on that issue. The fix there is a per-edge boolean lowering (create_edge_filter) mirroring whatnodes[...]already does, which needs sequencing against the filter-expression rebase in #2753.