Type-safe filter expression API - #2643
Draft
shivamka1 wants to merge 125 commits into
Draft
Conversation
…oving string dispatch
…ps with direct methods on Id accepting T: Into<GID>
…ing NodeNameFilterBuilder/NodeTypeFilterBuilder
…ing NodeNameFilterBuilder/NodeTypeFilterBuilder
…rs (windowed + layered)
…_expr # Conflicts: # raphtory/src/db/graph/views/filter/model/node_expr.rs
…e full filter-building pipeline on each type
…ilder → TemporalProp/Quantified/Aggregated
…ilterOps from prelude
… to restore dot-syntax property filter ops
…e .property().temporal() instead
Contributor
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: db8d4b1 | 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 |
7737053 ns/iter (± 44875) |
1564816 ns/iter (± 35303) |
4.94 |
lotr_graph_window_100/num_nodes |
15 ns/iter (± 0) |
5 ns/iter (± 0) |
3 |
lotr_graph_window_100_materialise/materialize |
7817487 ns/iter (± 34513) |
1669150 ns/iter (± 10700) |
4.68 |
lotr_graph_window_10/has_node_existing |
138 ns/iter (± 9) |
62 ns/iter (± 11) |
2.23 |
lotr_graph_window_10_materialise/materialize |
3386528 ns/iter (± 12063) |
971980 ns/iter (± 4278) |
3.48 |
lotr_graph_subgraph_10pc_materialise/materialize |
2060727 ns/iter (± 24511) |
334634 ns/iter (± 1287) |
6.16 |
lotr_graph_subgraph_10pc_windowed/has_node_existing |
141 ns/iter (± 7) |
62 ns/iter (± 14) |
2.27 |
lotr_graph_subgraph_10pc_windowed_materialise/materialize |
1294611 ns/iter (± 13512) |
230399 ns/iter (± 2617) |
5.62 |
lotr_graph_window_50_layered/num_edges_temporal |
141029 ns/iter (± 3517) |
70121 ns/iter (± 7586) |
2.01 |
lotr_graph_window_50_layered/has_node_existing |
390 ns/iter (± 20) |
129 ns/iter (± 12) |
3.02 |
lotr_graph_window_50_layered/graph_latest |
81751 ns/iter (± 2733) |
36649 ns/iter (± 916) |
2.23 |
lotr_graph_window_50_layered_materialise/materialize |
30479885 ns/iter (± 151669) |
3488825 ns/iter (± 24948) |
8.74 |
lotr_graph_persistent_window_50_layered/num_edges_temporal |
587231 ns/iter (± 5273) |
192686 ns/iter (± 1569) |
3.05 |
lotr_graph_persistent_window_50_layered/has_node_existing |
415 ns/iter (± 366) |
174 ns/iter (± 83) |
2.39 |
lotr_graph_persistent_window_50_layered/graph_latest |
123939 ns/iter (± 1465) |
57549 ns/iter (± 4809) |
2.15 |
lotr_graph_persistent_window_50_layered_materialise/materialize |
52787032 ns/iter (± 227728) |
5298035 ns/iter (± 147912) |
9.96 |
This comment was automatically generated by workflow using github-action-benchmark.
…tifier ops with list-aware aggregates
…oop edge predicates with per-edge prop ops
…b method collisions on primitive EntityExpr types
# Conflicts: # raphtory-graphql/src/model/graph/edges.rs # raphtory-graphql/src/model/graph/filtering.rs # raphtory-graphql/src/model/graph/graph.rs # raphtory-graphql/src/model/graph/node.rs # raphtory-graphql/src/model/graph/nodes.rs # raphtory-graphql/src/model/graph/path_from_node.rs # raphtory-tests/src/assertions.rs # raphtory-tests/tests/cached_view.rs # raphtory-tests/tests/filter_tests/tests_node_type_filtered_subgraph.rs # raphtory-tests/tests/subgraph_tests.rs # raphtory-tests/tests/test_filters.rs # raphtory-tests/tests/test_layers.rs # raphtory-tests/tests/views_test.rs # raphtory/src/algorithms/components/in_components.rs # raphtory/src/algorithms/components/out_components.rs # raphtory/src/db/api/view/filter_ops.rs # raphtory/src/db/api/view/graph.rs # raphtory/src/db/api/view/internal/filter.rs # raphtory/src/db/graph/views/filter/mod.rs # raphtory/src/db/graph/views/filter/model/and_filter.rs # raphtory/src/db/graph/views/filter/model/degree_filter.rs # raphtory/src/db/graph/views/filter/model/edge_filter.rs # raphtory/src/db/graph/views/filter/model/exploded_edge_filter.rs # raphtory/src/db/graph/views/filter/model/filter_operator.rs # raphtory/src/db/graph/views/filter/model/graph_filter.rs # raphtory/src/db/graph/views/filter/model/is_active_edge_filter.rs # raphtory/src/db/graph/views/filter/model/is_active_node_filter.rs # raphtory/src/db/graph/views/filter/model/is_deleted_filter.rs # raphtory/src/db/graph/views/filter/model/is_self_loop_filter.rs # raphtory/src/db/graph/views/filter/model/is_valid_filter.rs # raphtory/src/db/graph/views/filter/model/latest_filter.rs # raphtory/src/db/graph/views/filter/model/layered_filter.rs # raphtory/src/db/graph/views/filter/model/mod.rs # raphtory/src/db/graph/views/filter/model/node_filter/mod.rs # raphtory/src/db/graph/views/filter/model/node_filter/ops.rs # raphtory/src/db/graph/views/filter/model/node_state_filter.rs # raphtory/src/db/graph/views/filter/model/not_filter.rs # raphtory/src/db/graph/views/filter/model/or_filter.rs # raphtory/src/db/graph/views/filter/model/property_filter/evaluate.rs # raphtory/src/db/graph/views/filter/model/property_filter/mod.rs # raphtory/src/db/graph/views/filter/model/snapshot_filter.rs # raphtory/src/db/graph/views/filter/model/windowed_filter.rs # raphtory/src/errors.rs # raphtory/src/lib.rs # raphtory/src/python/filter/edge_filter_builders.rs # raphtory/src/python/filter/filter_expr.rs # raphtory/src/python/filter/node_filter_builders.rs # raphtory/src/python/filter/property_filter_builders.rs # raphtory/src/python/graph/index.rs # raphtory/src/search/node_filter_executor.rs # raphtory/src/search/query_builder.rs # raphtory/src/search/searcher.rs
Adapts the expression filter API to the two-parameter CreateFilter shape (base graph + pre-transformed evaluation view), restores the machinery lost to silent auto-merges and merge-spliced impls, splits the factories whose names collided across the two branches (PropertyExprFactory / DegreeExpr vs the builder-path factories), adopts the db_v4 dyn-filter layer, and rebuilds per-expression view construction (CreateView) for the windowed, latest, snapshot and layered wrappers. Expression filters report no composite representation through the fallible TryAsCompositeFilter.
PyExpr and the dyn factory traits ride the renamed factories; the boolean predicates (is_active, is_valid, is_deleted, is_self_loop) are expressions built through the Scoped view adapter; ComposableFilter::not is restored for the graphql lowering; the degree benchmark pins the builder-path factory it was written against.
src()/dst() id, name, node_type, property and metadata now build expressions, including the temporal aggregate chains on endpoint properties. The filter_tests parity suite adapts by imports alone; its assertions are unchanged apart from one negation pinned to the composite complement it resolved to before the expression ops were in scope. The June test modules resolve through the expression factories and each ambiguous `not` is pinned to its original semantics.
- edge expressions build their ops from the pre-transformed view, matching the node side and the CreateFilter contract; the orphaned boolean/set ops are removed - constant comparison operands are validated by value castability, matching the runtime coercion; type-level compatibility remains for expression-vs-expression comparisons - two tests pinning pre-db_v4 semantics (windowed is_self_loop, agg overflow) yield to their filter_tests twins - the node_expr unit tests compile against the current API - unused imports left behind by the bridge are removed
Coercing a concrete wrapper (Latest<T>, SnapshotAt<T>, ...) into Arc<dyn DynEdgeFilterFactory> materialises a vtable whose own wrap methods coerce deeper wrappers, so monomorphisation never terminates; the compiler gave up 40 windows deep (E0275) once anything compiled these bindings, which is exactly what cargo test --all-features does. The four wrap methods now erase self before wrapping (the same trick dyn_window already used), closing the set over wrapper-of-erased types. The erased factory picks up the traits those wrappers need through the existing Arc blankets: DynEntityExpr and DynCreateView become supertraits, EdgeFilterFactory is implemented directly, and the blanket's unused EdgeViewFilterOps bound is dropped.
Section markers drop their provenance notes; the FilterValue alias comment explains the two types' roles and that both leave with the composite path.
First execution of the python bindings surfaced defects invisible to
compilation:
- the module registered classes while the entry points are instance
methods; the module attributes are now ready-made root instances
- the erased wrap methods dispatched back into themselves through the
vtable (an unconditional runtime loop); the window family constructs
the wrapper over the erased factory directly with the same bounds
clamping as ViewWrapOps, and the manual Arc impls dispatch through
as_ref() so the blanket on Arc cannot self-select
- comparison and string operators accept plain python values as well as
expressions (extracted as Prop constants)
- temporal() is exposed on PropertyExpr through the existing DynTemporal
- predicates return FilterExpr: comparisons wrap their CreateFilter
impls, factory predicates route through Dyn{Node,Edge}ViewFilterOps
instead of Scoped ops, and NodeWindow carries the NodeViewFilterOps
bound
- the erased expression type forwards prop_type and nullable, so set
coercion and build-time validation see real types instead of Empty
The filter test files use one construction idiom (attribute style,
matching the rust docs).
any() and all() on an expression now return the qualifier expressions
themselves instead of terminating in an implicit eq(true), so the
comparison written after them applies per element and the qualifiers
collapse the results (innermost list level first):
NodeFilter.property("p").temporal().any().eq(7)
lowers to the same op chain as the trailing form. The pieces:
- CreateOp::create_qualified_{node,edge}_op separates leading
qualifiers from the value expression (forwarded through the dyn
layer); AnyExpr/AllExpr strip themselves, aggregates pass qualifiers
through and apply per element
- the comparison, string and set filters lower a qualified lhs to the
list-aware elementwise ops wrapped in the qualifier collapse chain,
validating against the element type
- a bare qualifier used as a filter keeps its old meaning (elementwise
eq(true) then collapse) through one marker-dispatching CreateFilter
Also: the node_type expression yields the storage's default type key
for untyped nodes, matching the composite type mask (negations now
include untyped nodes); node metadata lookups raise the metadata error;
endpoint and exploded-edge properties expose temporal(). Three tests
that had pinned the untyped-node divergence follow their filter_tests
twins.
The expression ops now report the types they actually produce, which turns a family of silent no-matches into build-time errors and makes the coercion rules uniform: - id, name, node_type and degree lowerings carry their static types (ids take the graph's id dtype); temporal ops report List of the property dtype; aggregates report their output structurally (the innermost list level collapses, outer levels survive for pending qualifiers); Arc<dyn EdgeOp> and the endpoint bridge forward prop_type; edge metadata stays untyped since its runtime shape depends on the edge's layers - multi-qualifier chains collapse in the written order: leading qualifiers read outermost-first, trailing ones innermost-first - qualified is_some/is_none apply elementwise through the list-aware unary ops; aggregates validate against the qualified element type; qualifiers over a known scalar are errors - ordering operators are rejected for map and list properties as well as booleans; string operators validate their constant operand; the edge set path coerces values like the node path; map constants compare structurally (partial schemas) but non-map constants against map properties are errors - u64/i64 sums that overflow promote to Decimal and still compare - python comparisons raise TypeError at the call site when the expression's type is statically known and the value cannot coerce; string operands are checked eagerly The python filter tests, which had never been executable, are adjudicated against these semantics: provably wrong hand-written expectations follow the rust parity suite, error-message assertions use the expression-path messages, and mistyped-operand cases build their filters inside the raise assertions.
The wire schema is unchanged; the DynFilter conversion now builds expression filters instead of the composite enums. Field conditions reuse the existing where-translations and dispatch onto typed field expressions, property condition trees walk onto the dyn expression chain (wrappers extend the expression in written order, combinators branch, leaves become predicates), views wrap the lowered filter, and edge endpoints evaluate a full lowered node filter per edge through the new EdgeEndpointNodeFilter adapter. The composite conversions remain for the python remote client, which still serialises filters through the tree representation.
Python filter objects now carry both the compiled expression and the GraphQL wire tree, recorded at construction. The remote client reads the carried tree; expressions with no wire form (an expression on the rhs of a comparison) are rejected at the remote boundary with a clear error. The client error envelope now surfaces each GraphQL error's message text instead of the raw JSON object, so quotes inside diagnostics are no longer escaped. Test expectations updated for expression-engine wording and semantics: bool constants coerce to ints in comparisons, mistyped is_in values are rejected eagerly, and nodes.filter() defers uniformly for every filter kind — narrowing membership is spelled nodes[...] / select (reverses the #2690 special case).
Composite filters no longer execute: the CreateFilter impls on the composite enums, the GraphQL-to-composite conversions, and the TryAsCompositeFilter export trait are gone. The enums survive as plain data — the transportable wire descriptor inside FilterTree — and the composite-to-GraphQL direction stays for client transport. Server resolvers pass wire filters straight to the expression lowering. Recording view chains at construction had inverted non-commuting chains on the wire (window then latest arrived as latest then window). Both wire builders now nest views in application order, with parity tests covering both orders of window+latest at graph, node and edge scope.
The builder factories, ops traits, field builders, validation module and leaf executors are gone, along with their prelude exports. The rust test suite runs on the expression API — the builder half of the A/B test pair is deleted, its two unique tests moved into the expression suite. Aggregations over latest list-valued properties now work on the typed rust surface, matching what python already exposed. The exploded-edge expression filter now passes deletion events through, as the removed executor did; persistent-graph histories pin the behaviour.
The upstream rewrite of the deleted builder ops stays deleted; the OrOp and AndOp const-value fixes and their tests are kept, and the NodeOp trait's const_value_in_domain now receives storage. Upstream's renamed membership test matches the uniform filter semantics on this branch, and its new collection-filter suites pass against the expression engine unchanged.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type-safe filter expression API
What this PR does
Replaces filter construction and execution with a typed expression API, end to
end: rust, python, GraphQL, and the remote python client. The old composite and
builder paths are deleted; the composite enums survive only as the plain-data
wire descriptor the client transports.
Expressions compile to per-entity ops evaluated against the pre-transformed
view (
create_node_op/create_edge_op), with build-time validation(operator/type compatibility, aggregability).
The four layers
PropertyExprFactory,NodeFilterFactory,EntityExprFilterOps,EntityAggOps, view wrappers) is the only filterconstruction path. The builder DSL (
PropertyFilterOps,NodeIdFilterOps,the
*FilterBuildertypes,validate.rs) is deleted, and the rust testsuite runs on expressions.
form (
FilterTree), recorded at construction. Local filtering executes theexpression; nothing is reverse-engineered.
GqlFiltervalues lowerdirectly onto expressions (
expr_lowering.rs). The composite conversionsfrom the wire form are gone.
FilterTreeand sends it asGraphQL variables. Filters with no wire form (an expression on the
right-hand side of a comparison) are rejected at the remote boundary with a
clear error instead of being mistranslated.
Because the wire schema is untouched, stored permission filters in
pometry-storage (GqlFilter JSON) work unchanged.
Qualifiers and typing
Leading and trailing qualifier forms are equivalent and collapse in the
written order —
temporal().any().eq(7)andtemporal().eq(7).any()lowerto the same ops; qualifiers float through aggregates (
temporal().any().sum()= per-snapshot sums, any match). The ops report the types they produce
(field types, graph id dtype,
Listof the property dtype for temporal,structural aggregate outputs), so validation happens at build time: ordering
is rejected for boolean/map/list properties, string operators require
string-castable operands, qualifiers over scalars are errors, and mistyped
constants fail where they are written (python raises
TypeErrorat thecomparison when the type is statically known).
Behaviour changes
nodes.filter()defers uniformly for every filter kind, including nodeid/name filters; narrowing membership is spelled
nodes[...]/select(or agraph-level
g.filter(...)). This matches the same change made upstream inNode ID and OR filter bug #2754, which landed while this branch was open.
(
degree().lt("5")works), non-castable values error where they arewritten. Bool constants coerce to ints (
prop >= Truebehaves like python's60 >= True); ints do not coerce to bool.is_invalues reject eagerly instead of silently matchingnothing — a caller can now distinguish "no matches" from "bad query".
composite engine silently mis-executed (a leading
anybeforeavgused tobe a no-op; it is now an error). In rust, aggregations over scalars no
longer compile at all.
and/orcombinators are rejected rather than defaulting — theprevious match-everything fallback for
orinverted the caller's intent.messagetextinstead of the raw JSON object, so quotes inside diagnostics are no longer
escaped into
\".db_v4semantics were dropped in favour of theirfilter_teststwins: windowedis_self_loop(the window intentionally hasno effect) and aggregate overflow (now promotes to Decimal instead of
excluding).
Follow-ups folded in rather than deferred
NodeFilter.id().eq(x)and.is_in([…])resolve their evaluation domain through the storage index instead of
visiting every node — the narrowing Node ID and OR filter bug #2754 added to the removed builder op,
carried onto expressions. Only equality and set membership narrow; a
constant whose type does not match the graph's id type falls back to the
full domain rather than guessing, and every other predicate is unchanged.
sumdeclares the type it produces. It widens at runtime (any unsignedwidth accumulates into
U64, signed intoI64, floats intoF64, promotingto
Decimalpast those) but declared its element type, so a constant beyondthe element's range was rejected before the sum ever ran — no comparison
above 255 was expressible on a
List<U8>. The declared type now mirrors theevaluator's own match arms. Reductions that return an element (
min,max,first,last) keep the element type..pyistilldescribed builder classes that no longer exist (
PropertyFilterOps,NodeIdFilterBuilder, …) and docstrings pointed callers at them.A bug the rewrite caught
Recording view chains at construction initially inverted non-commuting chains
on the wire:
window(0,5).latest()meant latest-within-window locally butwindow-of-latest remotely. Both wire builders now nest views in application
order (outermost = applied last), and parity tests pin both orders of
window+latestat graph, node and edge scope. Single views and commutingchains were never affected, which is why broad suites missed it.
Implementation notes
Arc<dyn …>before wrapping views;wrapping the concrete factory made monomorphisation non-terminating the
first time anything compiled the bindings under
--all-features.GqlFilterimplementsCreateFilterby lowering on the fly, so serverresolvers pass wire values straight through.
python/filter/wire.rs: each construction steprecords its fragment, predicates finish it into a
FilterTree, combinatorscompose trees structurally.
What the review threads asked for
The six open threads all asked for the same thing — that the old filter
machinery go away. It has:
state/ops/filter.rs("this whole thing should go away",NodeNameFilterOp)OrOp/AndOp/NotOp/Mask), which also carry the #2754 const-value fixesnode_filter/ops.rs,node_filter/validate.rs,property_filter/builders.rsproperty_filter/mod.rs("also left-over?")CreateFilterimpls are deleted.PropertyFilterstays as the wire data type, and itsmatches()survives because namespace metadata filtering evaluates single values through itTesting
filter_testssuite runs the samescenarios through the expression API.
passing, including per-side discrimination checks that rule out silently
dropped filters and both orders of non-commuting view chains.
disabled before being kept, so they pin the narrowing rather than passing
vacuously.
raphtory-authfailsidentically here and against plain
db_v4(pre-existing plugin drift), sonothing here regresses it. On submodule bump the outer workspace needs
dyn-clone,typetagandinventoryin[workspace.dependencies].