Repository navigation
fix: correlate a subquery body through its WHERE, WITH and UNION branches - #4874
Merged
Merged
Conversation
Contributor
Author
Tracking
Standard development
CI Testing Labels
Documentation checklist
|
…ches
A conjunct carrying an `EXISTS { ... }`, `COUNT { ... }` or `COLLECT { ... }` is
planted once every symbol in its `used_symbols` is bound. That set came from
`UsedSymbolsCollector`, which walks only the body's top-level MATCH pattern
atoms, so it was wrong in both directions:
* a correlation carried by a body WHERE or WITH was missed, leaving the set
empty. The filter was then planted below the scan that binds the outer
variable and the branch read an unwritten frame slot - an abort with
"Expected to generate all filters", or a silently wrong answer;
* a name the body declared and re-used as a later pattern atom
(`WITH x AS q MATCH (q)`, two MATCH clauses, `(n:N), (n:N)`) was counted as
an outer dependency nothing ever binds, so the conjunct was never extracted
and PlanMatching aborted.
The symbol generator already knows both answers exactly: it opens a scope per
body and resolves every identifier itself. It now keeps, per open body, what the
body referenced and what it declared, and stores the difference on the
SubqueryExpression - the symbols the body reads that were bound outside it,
through WHERE, WITH, property maps, nested bodies, pattern comprehensions and
UNION branches alike.
Filters::AnalyzeAndStoreFilter builds its conjuncts with a collector that takes
that set in place of the atom walk, so a conjunct that is a subquery and one
that wraps one (`COUNT { ... } > 0`, `NOT EXISTS { ... }`) are both covered. No
other UsedSymbolsCollector call site changes. The set is also copied onto
SubqueryMatching, where the drain will need it.
EXPLAIN is byte-identical to master for all 2212 pre-existing scenarios of the
memgraph_V1 and openCypher_M09 suites; the only plans that change are the 17 new
scenarios, each of which aborts or answers wrongly on master.
Closes #4361
Closes #4371
Closes #4070
Closes #4414
Closes #4683
Closes #4809
Closes #4595
Closes #4563
Closes #3544
Closes #4541
Closes #4477
imilinovic
force-pushed
the
fix/subquery-external-symbols
branch
from
September 17, 2026 19:12
409f74a to
abf8cc9
Compare
`SubqueryAwareUsedSymbolsCollector` overrode only `PreVisit(SubqueryExpression)`
and inherited the base's `PreVisit(PatternComprehension)`, which walks the
comprehension's pattern and neither its filter nor its result expression. A
subquery living in the filter was therefore never reached, its external symbols
never reached the conjunct, and the conjunct was planted below the operator that
binds the variable the body reads:
MATCH (n:Person), (k:Person)
WHERE size([(n)-[:KNOWS]->(c)
WHERE EXISTS { MATCH (z:Movie) WHERE z.title = k.name } | c]) > 0
RETURN n.name, k.name
The `RollUpApply` landed on the left input of the Cartesian, below the scan that
binds `k`, and the comprehension's whole filter was dropped - every `n` with an
outgoing edge passed for every `k`. Before the previous commit the same query was
refused outright, so what it turned into was a silent wrong answer.
The collector now walks both expressions. It contributes only the subqueries'
own external sets, and subtracts whatever the enclosing comprehensions bind: a
symbol external to the *body* may still be bound inside the branch the
`RollUpApply` drives - the comprehension's own element, in the common
self-correlated spelling - and demanding it would leave the conjunct unplantable.
Collecting identifiers there as well would reach the same names by another route,
so identifier collection stays off for that walk.
The underlying drop for a correlated comprehension filter is older than this
stack and still applies to the spelling that carries no subquery; this only stops
a subquery reaching it.
EXPLAIN is byte-identical to master for all 37 comprehension-bearing queries in
the behavioural suites.
No behaviour change. Verified against master: the 18 behavioural scenarios give
the same answers, and EXPLAIN is byte-identical for all 37 comprehension-bearing
queries in the suites.
- Drop `SubqueryMatching::external_symbols` and its write. Nothing reads it; both
readers of a field by that name take a `PatternComprehensionMatching`, which is
a sibling type. `getSubqueryMatchings()` returns by value, so the unused set was
copied twice per subquery per plan. The drain that needs it can add it back
together with its reader, where the compiler can check the two agree.
- Give `Symbol::position_` a default of -1. `Symbol() = default` left it
indeterminate while `operator==` and `std::hash` both read it. One arm of
`Visit(Identifier &)` falls through to the shared tail without assigning the
symbol; that arm cannot be reached while a subquery body is open today, because
the body clause allowlist admits only MATCH, WHERE, WITH and RETURN. Admitting
UNWIND or a nested CALL would reach it.
- Replace the `++in_subquery_depth` balancing trick in
`SubqueryAwareUsedSymbolsCollector` with an explicit no-op `PostVisit`. The
increment was load-bearing, since `Accept` runs `PostVisit` even when `PreVisit`
returns false, but nothing said so.
- Say why `SubqueryFrame` cannot be replaced by a scope-index test: `CALL (v) {}`
copies an outer symbol into the imported scope without `CreateSymbol`, so it
resolves inside the body but was created outside it.
- Note that `SubqueryReadSymbolsCollector` is a deliberate superset of
`external_symbols_`, and why narrowing it would be wrong.
- Rename `SubquerySymbolCollector` to `SubqueryResultSymbolCollector`. It collects
result symbols, not read symbols, and sat one word away from
`SubqueryReadSymbolsCollector`.
Comments across the stack shortened.
…ort boundary
A MATCH stashes the identifiers in its pattern property maps and variable-length
bounds, then resolves them once the whole clause has been visited, so they can
reference a variable bound later in the same MATCH. That deferred resolution
asked `HasSymbol` - which searches every scope outwards - and then wrote the
answer with `scope.symbols[name]`, which is `unordered_map::operator[]` on one
scope. When the name lived outside an un-imported `CALL {}`, the lookup said
"visible" and the write silently inserted a default-constructed Symbol.
The identifier was then mapped to position -1, and `SymbolTable::at` turned that
into a `std::out_of_range` that no query-error path catches:
WITH 2 AS k MATCH (m:XP) CALL { MATCH (n:XP)-[r:XK*1..k]->(q) RETURN n }
RETURN count(*)
An unknown exception occurred, this is unexpected.
The query is invalid - `k` is never imported, and the same rule already raises
`UnboundVariableError` for the non-deferred spellings - so the fix is to resolve
with the boundary `Visit(Identifier &)` uses, `call_subquery_base`, and report it
like everything else. It now says `Unbound variable: k.`
Before `Symbol::position_` was given a default this read an indeterminate index,
so the shape was undefined behaviour rather than an error: it happened to answer
12 in a Release build and could have indexed a valid symbol and answered wrongly
in another. Every valid spelling is unchanged - `CALL (k) {}`, `CALL (*) {}`, a
property map imported by name, and the same pattern at top level all give the
answers they gave before.
Also stop the one arm of `Visit(Identifier &)` that never assigns `symbol` from
falling through to the shared tail, so a default-constructed Symbol cannot reach
`MapTo` or the subquery frames regardless of what a future clause allowlist
admits.
`memgraph_V1` and `subquery_expressions.feature` are unchanged: 1322 pass / 69
fail, the same failing set as before, and 187/187.
`SubqueryExpression::external_symbols_` is what filter placement rests on, and
nothing asserted it. A behave scenario can only observe the set through a distant
planner symptom - the conjunct lands below the operator that binds an outer
variable and the body reads an unwritten frame slot, or the conjunct is
unplantable and `PlanMatching` aborts - so an over-broad set and a correct one
can look the same until some unrelated query aborts.
Assert the set itself after `MakeSymbolTable`, for five shapes, each rerun as
EXISTS, COUNT and COLLECT:
- correlated only through the body's `WHERE`, which the atom walk this replaced
left empty,
- correlated through a `WITH` that renames the caller's variable,
- a name the body declares and re-uses as a later pattern atom, which the atom
walk counted as an outer dependency nothing binds,
- nesting: a name the outer body declares is external to the inner body and
internal to the outer one, so the outer set is empty,
- correlated only in the second `UNION` branch.
The shapes assert both non-empty and empty sets, so an implementation that always
over-collects and one that always under-collects each fail a different case.
Five gql_behave scenarios for the paths with no coverage: a subquery reading the
path its enclosing comprehension binds (the one spelling where subtracting the
comprehension's own variables is load-bearing - deleting that guard left every
existing scenario green), the `COLLECT` fold, a scoped `CALL (a) {}` import, and a
simple `CASE` whose test is a correlated EXISTS, which is visited once per WHEN
arm. Answers taken from Neo4j 2026.02.2 on the scenario's own fixture; each is
selective, so a dropped correlation shows up as extra rows rather than the same
rows. Two more - the scoped `CALL` and a property-map correlation - pass before
this branch as well and say so in their comments: they guard the design and the
rewrite, they do not pin the fix.
…tions
No behaviour change. `SubqueryFrame` kept a set of every symbol declared while the
body was open, filled by a call from `CreateSymbol` on every symbol the query
creates, only to ask one question at the end: was this symbol made inside the
body?
`SymbolTable::CreateSymbol` assigns `position = table_.size()` and appends, so
positions are handed out in creation order and never reused. Sampling
`max_position()` when the body opens answers the same question with a comparison:
a referenced symbol below the mark predates the body and is external.
This stays a provenance test, not a visibility one, which is the property the set
existed to provide: `CALL (v) { ... }` copies an outer symbol into the imported
scope without `CreateSymbol`, and the copy keeps its original, lower position, so
it still reads as external. A scope-index test would get that wrong, which is why
the frame cannot simply be dropped.
Removes a function, a set per open body, and the only work `CreateSymbol` did for
this feature on every symbol of every query.
The one behavioural difference is unreachable: an anonymous symbol created inside
a body now counts as declared, where before it was in neither set.
`CreateAnonymousSymbol` maps onto an expression node rather than an `Identifier`,
and `RecordSubqueryReference` is only reached from `Visit(Identifier &)`, so such
a symbol can never be in `referenced` for the difference to show.
No behaviour change. `SubqueryAwareUsedSymbolsCollector::PreVisit(PatternComprehension &)` saved `comprehension_bound_` and `subquery_externals_only_`, then restored both by assignment after walking the filter and result expressions. `symbol_table_.at` is a `deque::at`, so a throw between the two left the collector dirty. Use `utils::OnScopeExit`, as the planner already does at three sites. Also record why `comprehension_bound_` suppresses on insert rather than erasing on exit. It holds every pattern atom identifier, including an outer name the pattern re-uses as its anchor; erasing at exit would drop that genuine correlation, while skipping cannot lose it, because the base class walks the pattern and collects the anchor before the suppression is armed. Without that note the two look interchangeable and the erase idiom the base class uses elsewhere looks like the tidier choice.
`SymbolTable::CreateSymbol` assigns `position_` from the table size and appends, so positions are handed out in creation order and never reused. A position therefore identifies a symbol within its table, and equal symbols always share one -- all `operator==` asks of a hash. The old hash also folded in the name and the type, so every insert and lookup in the 134 `std::unordered_set<Symbol>` sets across the planner paid a `std::hash<std::string>`. Dropping it measures 1.3-1.7x faster per operation and distributes better: over 512 symbols the longest bucket chain falls from 5 to 1, because contiguous positions spread evenly over prime bucket counts. This reorders every symbol-set traversal, so the risk is plan drift rather than correctness. Verified against the parent commit: `EXPLAIN` is identical for all 2474 plannable queries in the behave suites (0 differing, 0 newly failing), both suites keep their exact pass counts and failing scenario names, and the five planner unit suites are unchanged. Also drops a narrowing: the old code fed an `int64_t` position to `std::hash<int>`.
Comment-only. The creation-order property was explained three times across the branch; it is now stated once at the hash and once as the provenance trap that a future reader would otherwise "simplify" into a scope-index test.
imilinovic
force-pushed
the
fix/subquery-external-symbols
branch
from
September 21, 2026 19:28
f2f89a2 to
a0e827a
Compare
`PostVisit(Match &)` resolves the identifiers a MATCH deferred - the ones in
pattern property maps and variable-length bounds. It recorded each one in the
open subquery frames. That call can never fire.
Reaching the deferred arm of `Visit(Identifier &)` needs `in_node_atom` or
`visiting_edge` to be set, otherwise the pattern-name arm above it matches
first. Inside a subquery body the first arm tests exactly that pair and matches
first, so the deferred arm is unreachable there. A bare-pattern subquery pushes
a frame but has no MATCH, so `in_match` is never set. A frame is therefore never
open when this loop runs.
Measured with a probe on the line: zero hits across 1414 memgraph_V1 scenarios,
891 openCypher_M09 scenarios and 266 query_semantic tests. Suites unchanged
after removal - 1345/1414, 781/891, 266/266.
The import-boundary fix in the same loop stays: it applies inside `CALL {}`,
where no frame is open.
`SubqueryFrame` collided with `query::Frame`, the execution-time row of values, and both are
indexed by symbol position. `declared_from` collided with the `from` parameter of `HasSymbol`,
which is an index into `scopes_` rather than into the symbol table.
SubqueryFrame -> OpenSubquery
declared_from -> first_own_position
subquery_frames_ -> open_subqueries_
Also states why `PostVisit(SubqueryExpression &)` overwrites rather than merges: `CASE x WHEN ...`
plants one `x` node under every WHEN, and each visit re-creates the body's symbols, so the earlier
visits' symbols are orphans that nothing binds.
No behaviour change.
`PruningBFSRewriter` collected an operator's expression with `UsedSymbolsCollector`, whose subquery
walk stops at the body's top-level pattern atoms. An edge list the body reads through its `WHERE`
never reached `used_symbols_`, so the guard on the edge symbol passed and the expansion was pruned.
A pruning BFS keeps only the shortest path to each node, so every longer path the body selects was
dropped.
MATCH (a:Node {name: 'a'})-[r*1..3]->(z) WHERE EXISTS { MATCH (x:Len) WHERE x.n = size(r) }
RETURN DISTINCT z.name
On a graph with a one-hop and a two-hop path to the same node and `(:Len {n: 2})`, this returned no
rows instead of that node. The shape needs `deduplicates_`, so it appears under `DISTINCT` or an
all-distinct aggregate.
Use the subquery-aware collector, which takes the set `SymbolGenerator` already computed. A control
query without a subquery still plans `PruningBFSExpand`, so the rewrite is not lost.
memgraph_V1 1346/1415, openCypher_M09 781/891, query_semantic 266/266, query_plan 293/293,
query_plan_pruning_bfs 8/8.
`SubqueryAwareUsedSymbolsCollector` took a subquery's dependencies from
`SubqueryExpression::external_symbols_`, but only two call sites used it. Every other
`UsedSymbolsCollector` site still walked the body's top-level pattern atoms. That walk misses
a `WHERE`, `WITH` or `UNION` correlation and reports the body's own names as outer ones.
Fold the subclass into the base. The base now takes `external_symbols_` and checks a
comprehension's filter and result for subqueries, skipping names the comprehension binds.
`SubqueryReadSymbolsCollector` walks the body itself, so it now decrements its own depth.
This fixes `PropertyFilter::is_symbol_in_value_`. With an index on `:N(v)`,
MATCH (n:N) WHERE n.v = COUNT { MATCH (x:X) WHERE x.id = n.id } RETURN n.id
sought the index by a value that reads `n`, and failed with "nothing evaluated this COUNT".
It now keeps the conjunct as a filter and returns the unindexed answer.
The group-by remember-list does not need the old superset: the subquery's result symbol is
remembered separately, and every projection branch runs below the Aggregate.
EXPLAIN is identical for all 1844 plannable gql_behave queries. memgraph_V1 1346/1415,
openCypher_M09 781/891, same failing scenarios. 12 planner unit suites pass. The new unit
test and the new scenario each fail with the change reverted.
imilinovic
added
CI -build=coverage -test=clang_tidy
CI -build=release -test=e2e
…a body `UsedSymbolsCollector` no longer enters a subquery body, so it only read `in_subquery_depth`, which stayed 0. `SubqueryReadSymbolsCollector` is the one class that walks a body and changes the count. Move the counter there, with a `Visit(Identifier &)` that skips an anonymous identifier inside a body before deferring to the base. Also drop a comment in `PruningBFSRewriter` that restated what the collector does. No behaviour change: EXPLAIN is identical for all 2236 plannable gql_behave queries, and the planner unit suites pass.
Remove scenarios that pin a path another scenario already covers, or whose fixture gives the same answer whether the planner is right or wrong: - WITH import dropping every row: the null-test twin covers the same path. - Uncorrelated body reusing its WITH name, keeping every row: its no-match twin covers it. - OPTIONAL MATCH naming one variable twice: the same mechanism as two MATCH clauses sharing one. - Comprehension filter comparing a node to a path: the answer is `[]` either way. - Comprehension filter comparing to a node of its path: the own-path scenario covers it. - COLLECT correlated through its filter: the fold does not change placement; COUNT covers it. - Scoped `CALL (a)` import: it passes on master, and inside the branch `a` is bound at the root, so a filter placed too low still sees it. - Simple CASE with one WHEN arm: the test node is visited once, so the multi-visit path it names never runs. Also drop the COUNT and COLLECT reruns of the external-symbol shapes. The symbol generator reads the fold only to name it in errors, and three of the shapes are bodies a COLLECT may not have.
…body SymbolGenerator marks a node or edge atom that declares a name inside a subquery body as not user declared, as it does an anonymous one. The comment named only the anonymous case.
Say what the code does and why, and drop the history of the atom walk it replaced. That history is in the commit messages.
imilinovic
marked this pull request as ready for review
September 23, 2026 07:16
|
imilinovic
added a commit
that referenced
this pull request
Sep 23, 2026
…symbol generator `SubqueryMatchingCollector::PreVisit(PatternComprehension &)` computed the set by walking the filter and the result expression, then patching four positions the walk cannot reach: the pattern's own property filters, a nested comprehension's externals, and two subtractions for what the comprehension and its nested ones declare. Every pattern position that is neither filter nor result was still invisible, and the patches over-collected as much as they fixed. Open a record per comprehension, the way #4874 does for a subquery body, and take the set from it. A symbol referenced anywhere inside - a property map, a variable-length bound, a filter or weight lambda, a nested comprehension, a subquery body - is external exactly when it was created before the comprehension opened. Four shapes change answer. Three under-collected, so the RollUpApply was spliced below the operator that writes the symbol and the branch read an unwritten slot: a variable-length bound reading a FOREACH variable failed with "Variable expansion bound must be an int", and a BFS filter lambda or a wShortest weight lambda reading one silently matched nothing. The fourth over-collected: the two filters a variable-length edge's property map produces carry the expansion's own inner edge and node in their `used_symbols`, the old set took those for outer names, and `PlanPatternComprehension` declared them bound - which `MakeExpansionOperator` asserts against. `MATCH (a) RETURN size([(a)-[:R*1..2 {w: 1}]->(x) | x])` aborts the process on master. EXPLAIN is byte-identical on 820 scenarios of the comprehension-bearing suites bar one, which answers wrongly before and after (a comprehension anchored on a list comprehension's iteration variable - a separate, known gap). -43 lines.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Sep 23, 2026
imilinovic
added a commit
that referenced
this pull request
Sep 23, 2026
…symbol generator `SubqueryMatchingCollector::PreVisit(PatternComprehension &)` computed the set by walking the filter and the result expression, then patching four positions the walk cannot reach: the pattern's own property filters, a nested comprehension's externals, and two subtractions for what the comprehension and its nested ones declare. Every pattern position that is neither filter nor result was still invisible, and the patches over-collected as much as they fixed. Open a record per comprehension, the way #4874 does for a subquery body, and take the set from it. A symbol referenced anywhere inside - a property map, a variable-length bound, a filter or weight lambda, a nested comprehension, a subquery body - is external exactly when it was created before the comprehension opened. Four shapes change answer. Three under-collected, so the RollUpApply was spliced below the operator that writes the symbol and the branch read an unwritten slot: a variable-length bound reading a FOREACH variable failed with "Variable expansion bound must be an int", and a BFS filter lambda or a wShortest weight lambda reading one silently matched nothing. The fourth over-collected: the two filters a variable-length edge's property map produces carry the expansion's own inner edge and node in their `used_symbols`, the old set took those for outer names, and `PlanPatternComprehension` declared them bound - which `MakeExpansionOperator` asserts against. `MATCH (a) RETURN size([(a)-[:R*1..2 {w: 1}]->(x) | x])` aborts the process on master. EXPLAIN is byte-identical on 820 scenarios of the comprehension-bearing suites bar one, which answers wrongly before and after (a comprehension anchored on a list comprehension's iteration variable - a separate, known gap). -43 lines.
77 of 87 tasks
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.
What
EXISTS { ... },COUNT { ... }andCOLLECT { ... }now work in aWHEREwhen the body's only link tothe caller is a
WHERE, aWITHor a laterUNIONbranch, and when the body reuses a name it declareditself as a later pattern atom. These shapes aborted with Expected to generate all filters or answered
wrongly. A label-property index no longer breaks a
WHERE n.prop = COUNT { ... }whose body readsn.How
SymbolGeneratoralready resolves every identifier in a body. It now records on eachSubqueryExpressionthe symbols the body reads that were created outside it (
external_symbols_): every reference made whilethe body is open, minus the symbols created after it opened.
UsedSymbolsCollector(plan/preprocess.hpp) takes that set instead of walking the body's top-levelpattern atoms. The walk missed a
WHERE,WITHorUNIONcorrelation and counted the body's own names asouter ones. Every caller now gets the corrected set: filter placement, the index-seek guard
PropertyFilter::is_symbol_in_value_,PruningBFSRewriter, range filters and the group-by remember-list.The collector also walks a pattern comprehension's filter and result for subqueries, skipping names the
comprehension binds.
Why
A conjunct is placed once every symbol in its
used_symbolsis bound, so a wrong set broke placement inboth directions. Too few symbols put the filter below the scan that binds the outer variable. Too many made
it impossible to place. The same shapes already worked in a
RETURNorWITHposition, so the bodies wereplannable; only the placement of the conjunct was wrong.
Not fixed here (pre-existing on master)
MATCH (a:A), (n:N) WHERE size([(n)-->(m) WHERE m.v = a.id | m]) > 0returns rows the comprehensionrejects. The fix is to record comprehension external symbols in
SymbolGeneratortoo, which also letsSubqueryReadSymbolsCollectorgo. Its own PR.uncorrelated:
MATCH (n:P) WHERE n.x = COUNT { MATCH (m:Q) }with an index on:P(x)raisesnothing evaluated this COUNT. Its own PR.
Closes #4361
Closes #4371
Closes #4070
Closes #4414
Closes #4683
Closes #4809
Closes #4595
Closes #4563
Closes #3544
Closes #4541
Closes #4477