feat: add search.node/search.node_all query-module procedures#4460
Merged
Conversation
… mgp API Add an mgp C API for label-property index range lookups and a MAGE module that finds nodes by a label->property(s) map plus an operator and value. Kernel: - mgp_graph_has_label_property_index: check a ready label-property index exists. - mgp_label_property_range: opaque builder (make/add_property/destroy) with an mgp_bound_type enum; NULL bound = unbounded. Kept opaque for C-ABI stability across a future composite-index lookup. - mgp_graph_vertices_by_label_property_range: index-only range scan returning a mgp_vertices_iterator (enforces label-level read auth like plain iteration). - C++ wrappers in mgp.hpp (Graph::HasLabelPropertyIndex, Graph::NodesByLabelPropertyRange) and _mgp.hpp. Module (search.node, search.node_all): - Property list per label is a disjunction; results unioned. search.node dedups by node id, search.node_all keeps duplicates. - Index fast path for =, <, <=, >, >=, starts with (prefix successor bound); scan fallback for contains, ends with, =~, <> and when no index exists. - One shared comparison predicate across both paths; only string-typed properties match (except <>); value is a nullable string (null -> no rows); operators are trimmed and case-insensitive; =~ uses std::regex_match. - Accepts the label-property map as a map or a JSON string. - Name-map the equivalent identifiers in config/mappings.json. - e2e tests under mage/tests/e2e/search_test.
Contributor
Author
Tracking
Standard development
CI Testing Labels
Documentation checklist
|
Address review findings on search.node/search.node_all: - destroy the range spec exactly once in Graph::NodesByLabelPropertyRange (the prior structure could double-free on the null-iterator path) - guard the index-only range scan with a ready-index check so a missing index fails cleanly instead of relying on a DMG_ASSERT compiled out in Release - compile the =~ pattern once per call instead of once per scanned node - drop a redundant registration comment - add e2e coverage for the JSON-string map form and an indexed comparison operator
imilinovic
force-pushed
the
feat/search-node-parity
branch
from
July 21, 2026 20:49
5420773 to
a7da8d6
Compare
Replace the hand-rolled JSON parser for the label-property map string form with nlohmann_json (fetched into the MAGE build like fmt/googletest, pinned to v3.11.3). Behaviour is unchanged for valid input (object of string or array-of-string values; empty object yields no rows) and malformed input now reports precise parse errors.
Reimplement search.node and search.node_all to build a query per (label, property) pair and run it through the interpreter (mgp_execute_query) instead of driving the label-property index-scan mgp API directly. This lets the planner choose the access path, which is faster in almost every case (label-scoped scans and plan-time-resolved property reads, versus a full all-nodes scan with a per-node property getter), and runs each search in its own transaction. Results are unchanged across all operators, the disjunction/dedup semantics, the null-value and invalid-operator edge cases, and the map/JSON-string input forms. The one access path the planner does not yet cover is a prefix range for `starts with`; it currently falls back to a label scan plus filter. Teaching the planner to use the label-property index for `starts with` is left to a separate change.
search.node/node_all now run through the query interpreter and no longer use the label-property index range mgp API, so remove that API from this branch; it lives in its own change. Also drop the nlohmann_json dependency, which nothing links anymore.
…reter path The move to the query interpreter accidentally reinstated the hand-rolled JSON parser and dropped nlohmann_json. Restore nlohmann_json parsing for the label-property map's JSON-string form (and its build dependency) on top of the interpreter-based execution, so both improvements stand together.
imilinovic
changed the title
search.node/node_all ran a sub-query per (label, property) and emitted the resulting node accessors directly. Those accessors are bound to the sub-query's transaction, which is destroyed when its result is freed, so reading a returned node's labels or properties was a use-after-free. Return id(n) from the sub-query and re-resolve each id against the caller's graph (guarded by ContainsNode) so emitted nodes live in a transaction that outlives result consumption. Collapse the duplicate result-field constant.
Add e2e cases for non-string properties under text operators, <> across types, codepoint string ordering (uppercase before lowercase), multi-label dedup for node vs node_all, empty map / empty property list, empty value, the JSON list form, and an unknown-operator error; extend the comparison case with a name past the lower bound.
STARTS WITH / ENDS WITH / CONTAINS raise on a non-string property, and the sub-query's error is reported as end-of-results, so a search over a property of mixed types silently truncated. Guard these operators with a CASE that only applies them to string values, so a non-string property yields no match rather than raising. The comparison operators already handle non-string operands (no match, except <> which matches), so they are left unguarded. Also drop an unreachable zero-column check and note the intended move to the caller's transaction.
Cover non-string properties under text operators, <> across types, codepoint string ordering, multi-label dedup for node vs node_all, empty map / empty property list, empty value, the JSON list form, and an unknown-operator error.
Condense multi-line comments to single lines where the extra prose was redundant, keeping the load-bearing rationale (transaction lifetime, text-operator guard) intact.
Add a >= string case-ordering case (lowercase >= 'a', uppercase sorts before), matching Neo4j+APOC. Remove test_multi_label_node_all (its coverage is the intersection of test_node_all_duplicates and test_multi_label_node), and drop the no-op ORDER BY from single-row tests.
`>= 'Z'` is more illustrative than `>= 'a'`: an uppercase bound pulls in every lowercase letter (Z=90 < a=97 < z=122) plus Z-prefixed names, while excluding earlier-uppercase names. Verified against Neo4j+APOC.
A null label-property map is rejected (only a null value is allowed, yielding 0 rows), matching Neo4j+APOC.
Remove comments that just restated the code (the operator-map, identifier-escape and JSON-parse helpers are self-evident) and drop external-system references from the remaining rationale comments.
Spell out that STARTS WITH / ENDS WITH / CONTAINS raise on a non-string property (surfacing here as a silently truncated result), and why the guard must be a CASE rather than an AND: the planner does not preserve AND operand order, so the type check is not guaranteed to run before the operator.
imilinovic
force-pushed
the
feat/search-node-parity
branch
from
July 22, 2026 21:43
d02e9d8 to
920c125
Compare
…nter sees both The query-module count smoke test statically scans source for module_add_read_procedure call sites. Registering both procedures through a lambda invoked twice left a single call site, so the scanner counted one procedure while the runtime registered two, failing the count assertion.
as51340
requested changes
Jul 23, 2026
Address review feedback on search.cpp: - Extract the canonical Cypher operators into an `op::` constant namespace, reused instead of scattered string literals. - Add IsStringOnlyOperator() so the string-only guard lives beside the operator constants instead of being duplicated in Run(). - Build the sub-query with fmt::format instead of chained `+` to avoid the intermediate temporary strings.
imilinovic
force-pushed
the
feat/search-node-parity
branch
from
July 23, 2026 07:51
5958ba2 to
8e9e447
Compare
11 tasks
|
as51340
approved these changes
Jul 23, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 23, 2026
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.
Adds
search.nodeandsearch.node_allquery-module procedures that find nodes by a{label: property}map, a comparison operator, and a value.What.
CALL search.node(labelPropertyMap, operator, value) YIELD nodereturns nodes whose property satisfies the operator. A property list per label is a disjunction (results unioned).search.nodededuplicates by node id;search.node_allkeeps duplicates. Operators:=/exact,<>,<,<=,>,>=,starts with,ends with,contains,=~. The label-property map is accepted as a map or a JSON string (parsed with nlohmann_json);valueis a nullable string (null → no rows); operators are trimmed and case-insensitive.How. Each
(label, property)pair is turned into a Cypher query run through the interpreter (mgp_execute_query), so the planner selects the access path (label-property index where available, label scan otherwise) and each search runs in its own transaction. Going through the interpreter also enforces label- and property-level read permissions natively via the evaluator.Why. Delegating to the interpreter is faster than a hand-rolled scan in nearly every case and inherits future planner improvements. Note: the planner does not yet use the label-property index for
STARTS WITH(implemented in #4464) that operator currently falls back to a label scan + filter.Kernel-free — no changes outside
mage/andconfig/mappings.json. An earlier revision added a label-property range mgp C API; that has been split out to #4465 and is not required by these procedures.