Node ID and OR filter bug - #2754
Conversation
ljeub-pometry
left a comment
There was a problem hiding this comment.
the or filter is now too pessimistic about its const-ness (which is correct but inefficient)
| return check | ||
|
|
||
|
|
||
| def _combo_offenders(graph, op, join): |
| # The shape behind the RBAC leak: a node-set restriction ANDed with a wide `or` that | ||
| # includes a node_type branch (whose domain is every node). |
There was a problem hiding this comment.
clean up internal comments
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: 9881498 | Previous: 9823ef7 | Ratio |
|---|---|---|---|
lotr_graph/num_edges |
5 ns/iter (± 0) |
0 ns/iter (± 0) |
+∞ |
lotr_graph/num_nodes |
4 ns/iter (± 0) |
1 ns/iter (± 0) |
4 |
lotr_graph/graph_latest |
2 ns/iter (± 0) |
0 ns/iter (± 0) |
+∞ |
lotr_graph_materialise/materialize |
6571473 ns/iter (± 33378) |
1564816 ns/iter (± 35303) |
4.20 |
lotr_graph_window_100_materialise/materialize |
6591879 ns/iter (± 60406) |
1669150 ns/iter (± 10700) |
3.95 |
lotr_graph_window_10_materialise/materialize |
2711230 ns/iter (± 10084) |
971980 ns/iter (± 4278) |
2.79 |
lotr_graph_subgraph_10pc/num_nodes |
15 ns/iter (± 0) |
4 ns/iter (± 0) |
3.75 |
lotr_graph_subgraph_10pc_materialise/materialize |
1801590 ns/iter (± 27769) |
334634 ns/iter (± 1287) |
5.38 |
lotr_graph_subgraph_10pc_windowed_materialise/materialize |
1080459 ns/iter (± 9677) |
230399 ns/iter (± 2617) |
4.69 |
lotr_graph_window_50_layered/has_node_existing |
318 ns/iter (± 23) |
129 ns/iter (± 12) |
2.47 |
lotr_graph_window_50_layered_materialise/materialize |
24688256 ns/iter (± 320228) |
3488825 ns/iter (± 24948) |
7.08 |
lotr_graph_persistent_window_50_layered/num_edges_temporal |
494682 ns/iter (± 7891) |
192686 ns/iter (± 1569) |
2.57 |
lotr_graph_persistent_window_50_layered_materialise/materialize |
41046675 ns/iter (± 186306) |
5298035 ns/iter (± 147912) |
7.75 |
This comment was automatically generated by workflow using github-action-benchmark.
|
|
||
|
|
||
| def _edges(collection): | ||
| return sorted((e.src.name, e.dst.name) for e in collection) |
There was a problem hiding this comment.
This can be e.id
| narrow collection membership — unlike the property filter above. Pins the | ||
| new behavior (and its parity with local) so a regression is visible.""" | ||
| def test_filter_by_node_id_keeps_membership_but_getitem_narrows(): | ||
| """`.filter()` keeps every member for a name/id filter too — `[]` is the one that narrows. |
There was a problem hiding this comment.
.filter only filters to the right, can also add the case where we do g.filer.nodes.id here
| } | ||
|
|
||
| /// The full config schema as a nested dict: every field, including ones unset by default. | ||
| /// |
There was a problem hiding this comment.
Need to add this to the PR description
| if print_config { | ||
| if print_config_schema { | ||
| let schema = server.config().config_schema_json().map_err(|e| { | ||
| std::io::Error::new(std::io::ErrorKind::InvalidData, e.to_string()) |
| } | ||
|
|
||
| pub fn node_list(&self) -> NodeList { | ||
| // Membership is the candidate set (`self.nodes` / the base graph), filtered per node by |
There was a problem hiding this comment.
don't need to this comment
…li schema print, drop comment Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Shivam <4599890+shivamka1@users.noreply.github.com> # Conflicts: # raphtory/src/db/graph/nodes.rs
Node-id equality and set membership resolve their evaluation domain through the storage index instead of visiting every node, carrying the narrowing #2754 added to the removed builder op onto expressions. A constant whose type does not match the graph's id type keeps the full domain rather than guessing, and every other predicate is unchanged. A sum widens at runtime but declared its element type, so a constant beyond that element's range was rejected before the sum ran — no comparison above 255 was expressible on a list of u8. Sum now declares what the evaluator produces; reductions returning an element keep the element type. The generated stubs and the docstrings pointing at them still named builder classes that no longer exist.
What changes were proposed in this pull request?
Fixes three filter-correctness bugs in Raphtory, all found while property-testing filter equivalence, and makes a filtered-read denial legible.
orfilters (filter.rs) — anor-filtered node list was wrongly treated as trusted, so the edge iterator's fast path kept edges to excluded nodes..filter()on a node collection dropped members (nodes.rs) —nodes.filter(name==…)narrowed membership; it should keep every member (that's whatnodes[…]is for). Membership is now read from the base graph, consistently across fields.edges.rs) —edges[before(t)](andafter/window/at/latest) returned every edge instead of narrowing. It combined the filter against the unwindowed base graph, dropping the view; it now chains onto the current view like node collections do.Plus: a filtered-read grant whose resolved filter can't be applied now denies with a clear, logged message instead of an opaque "internal error" (
data.rs).Plus: config-schema introspection —
GraphServer.config_schema()in Python and--print-config-schemaon the CLI return the full config schema as JSON: every field the server and every registered extension accept, including ones unset by default (print_configonly shows set values). Backed by a newconfig_schemahook on server extensions (defaults toto_json).Why are the changes needed?
All three produced silently wrong views; (3) failed open (an edge collection filtered by time processed every edge). None were caught because existing tests only assert on
.idand only hit the paths that happened to work.Does this PR introduce any user-facing change? If yes is this documented?
Behavioural fixes, no API change:
Behaviour now matches the documented
.filtervs[...]semantics; no doc changes needed.New API surface:
GraphServer.config_schema()(Python, documented in the stub) and the--print-config-schemaserver flag (documented in--help).How was this patch tested?
New unit and Python collection tests (
or/and-of-oredge-endpoint invariants; node-collection membership; edge-collection filtering across every filter type and time view; access-filter denial logging), plus the full filter regression suite — all pass.Are there any further changes required?
Yes, tracked separately: time views don't compose reliably on edge collections — a pure time view is fixed here, but combining one with another filter (
and/or/not) can still drop it and fail open. Same class as (3), in the composite filter path. Worth a parametrised suite over {collection path} × {filter type} × {and/or/not}, which currently has large gaps.