fix(engine) #5626: walk the subquery bodies the parse-time checks claimed were validated elsewhere - #5642
Conversation
…imed were validated elsewhere
CypherExpressionWalker stopped at EXISTS { }, COUNT { } and COLLECT { }, and its Javadoc said those bodies
were "parsed on their own and validated then". They are not: each keeps its body as text and re-parses it
once per outer row, absorbing a failure into the expression's neutral value. A CALL { } body was never
handed to this phase at all.
So abs('x') was rejected before the query ran when written in a WHERE and accepted one level in, which is
the clause-dependent asymmetry #5602 set out to remove.
The three expressions now carry their body as an AST built from the parse tree ANTLR already produced, and
the walk descends into it and into a CALL { } body. A body is a scope of its own, so a check that reads
variable kinds re-binds itself to what the body declares over what it inherits.
Two anonymous BooleanExpression adapters - one for an EXISTS written as a bare WHERE predicate, one for a
function call used as one - were leaves for the same reason and are now the equivalent
BooleanCoercionExpression. Procedure CALL arguments and the LOAD CSV url are walked too.
|
Tick the box to add this pull request to the merge queue (same as
|
…ry-parse-validation # Conflicts: # docs/release-26.8.1.md
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 54 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
Review: PR #5642 - walk subquery bodies at parse time Read the full diff and surrounding source (CypherExpressionWalker, CypherSemanticValidator, BooleanCoercionExpression, CallClause, SimpleCypherStatement). This is a clean, well-scoped fix. The problem statement is precise, the AST-alongside-text approach is the right one, and the test suite is unusually complete: it covers each body kind, each check (type / arity / unknown-fn / path-access), every syntactic position a body can appear in, plus explicit no-false-rejection cases. Nice work. What I verified
I could not execute the test suite in this review environment (test runs required an approval that was not available here), so I am relying on the reported green runs for the dynamic behavior. One substantive question - scope inheritance into an implicit CALL { } nestedVarTypes inherits every non-shadowed outer variable kind into the body scope, for all nested statements. For the three correlated expressions (EXISTS / COUNT / COLLECT) that is exactly right. For an implicit CALL { } - which the PR description itself notes "imports nothing" - the body arguably should not see outer variables at all, yet it inherits their kinds anyway (minus shadowed names). The shadowing test (anUnimportedOuterNameDoesNotDecideTheBodysCheck) passes because the body re-declares p. But consider a body that references an outer variable it neither imports nor re-declares, e.g. MATCH p = (a:P)-[:KNOWS]->(b:P) CALL { RETURN p.name } RETURN * : here p is not imported and not re-bound, so it inherits VarType.PATH, and this phase would raise "InvalidArgumentType: Property access on a path variable" - whereas the accurate diagnostic (and Neo4j) is that p is simply not defined in the body. This is not a false rejection of a valid query, and this validator is not the unbound-variable checker, so the practical impact is just a mismatched error message on an already-invalid query. Worth a confirming test either way; if the intent is that an implicit CALL { } body starts from an empty scope, the inherited map should be gated on whether the CALL actually imports. Minor nits (non-blocking)
Behavior change The Issue5228CallMatchEdgeLabelEntityTest change is justified: type(b) on a node was always a type error, and rejecting it identically inside and outside a subquery is the whole point of the issue. The release note flags the potentially-breaking nature clearly. Overall: solid, focused fix with excellent test and documentation discipline. Only the implicit-CALL scope-inheritance question is worth a second look before merge. |
…es in order
Review follow-up. The nested scope was built as "declarations, minus every name the body binds by any
means", which dropped the kind of a variable a body merely passes through: an importing WITH p re-projects
the outer path under the same name, and the drop made p stop being a path, so
CALL { WITH p RETURN p.name } was accepted where the equivalent CALL (p) { RETURN p.name } was rejected.
The body's clauses are now walked in order, the way validateVariableTypes walks a statement's: a pattern
declares a kind, a WITH resets the scope to what it projects and carries a kind through only for a plain
variable reference (so WITH p and WITH p AS q keep the path and WITH 1 AS p does not), WITH * passes the
scope through, and an UNWIND / YIELD / LOAD CSV binding drops whatever the name held. Simpler than the
two-collection version it replaces, and it drops the null-guard inconsistency the review noted.
On the review's question: an implicit CALL { } does inherit kinds for names its body cannot reference, and
that is harmless because validateVariableScope runs first - an unimported reference is reported as the
undefined variable it is, never as a kind error. Asserted rather than argued.
|
Thanks - the scope question was the right thing to pull on. Probing it turned up a genuine bug next door to it, so the whole Your case: confirmed harmless, and now asserted. What the probe found instead. The old rule was "declarations, minus every name the body binds by any means". An importing MATCH p = (a:P)-[:KNOWS]->(b:P) CALL { WITH p RETURN p.name AS n } RETURN n -- accepted (wrong)
MATCH p = (a:P)-[:KNOWS]->(b:P) CALL (p) { RETURN p.name AS n } RETURN n -- rejected (right)Two spellings of the same import, two answers - the asymmetry this PR exists to remove, reintroduced one level down. The rewrite. The body's clauses are walked in order, the way Six cases added across three tests: the import spellings (explicit scope, leading On the WITH double-walk - agreed it is real and idempotent. Leaving it: collapsing it needs identity tracking across two collections that some builder paths populate and others do not, which is more moving parts than the duplicate work it saves at parse time. Green after the change: engine |
|
Review: #5642 - walk the subquery bodies the parse-time checks claimed were validated elsewhere Thorough, well-scoped fix with an unusually complete test suite. The core move - carrying each subquery body as a Code quality
One substantive limitation worth noting (not blocking) This is a missed rejection, never a false rejection (merged keeps a name only where branches agree, so nothing spurious is ever flagged), and it is strictly better than the pre-PR behaviour where nothing in the body was walked at all - the runtime guard still catches it when the branch executes. The literal-argument checks (arity, unknown function, bad literal type) are scope-independent and do fire per branch, which is why Minor
Performance Tests Note: I could not run Maven in the review environment (permission-gated), so the suite was not independently executed here; the review is static. I did confirm every AST accessor the new code calls ( Conventions Overall: LGTM. The UNION-branch scope intersection is the only thing I would flag for a follow-up (or a documenting comment); everything else is polish. |
…t each WITH once
Review follow-up, both points raised as non-blocking.
A UNION body was collapsed to the intersection of its branches' variable kinds and every branch was then
walked with that one map, so a kind-dependent check on a variable only one branch declares was silently
skipped - CALL { MATCH (a:P) RETURN a.name AS x UNION MATCH q=(m:P)-->() RETURN q.name AS x } let the
path-property read through. Each branch is now entered as a nested statement in its own right, so it builds
its own scope over what it inherits and the intersection is gone. Asserted from both branch positions, so a
scope accidentally taken from one branch cannot pass.
The walk also visited a WITH twice when it was registered both on the ordered clause list and on the
statement. Harmless for this phase's checks, which are pure, but the walker is a general utility now and a
visitor that keeps state would have double-counted. Which WITHs the ordered walk reached is tracked by
identity, so the second pass adds only what it missed.
|
Both points fixed rather than documented, in e9c26cb - each turned out to be a few lines. The UNION intersection is gone. Your example is exactly right, and it was reachable: One trap worth recording, since it is why the union arm could not simply fall through to the generic path: The double WITH walk is gone too. You are right that "documented footgun" is the wrong resting place for a class that is now a general-purpose utility - a visitor that keeps state is a reasonable thing for the next caller to write, and it would have silently double-counted. Which WITHs the ordered walk reached is tracked by identity, so the second pass adds only what it genuinely missed. The The explicit Four cases added (both union orderings, a clean union that must still pass, the explicit empty scope). Green: engine On the per-row re-parse: agreed it is the real cost and out of scope here. I will file it as a follow-up - now that the body exists as an AST, correlating it as an AST instead of splicing text would also retire |
|
Review: #5626 - walking subquery bodies at parse time Reviewed the full diff plus the surrounding source. This is a high-quality, well-reasoned change: it closes a real asymmetry (a bad call was rejected in an outer WHERE but silently absorbed one level in), the PR description is exact about what escaped and why, and the test suite is genuinely comprehensive - reproducer per body kind, every check (type/arity/unknown-name/path-property), every body position, the no-false-rejection cases, and the evaluate-still-works cases. Correctness - verified, holds up
Observations (non-blocking)
Performance Negligible and well-argued: the body AST is built off the parse tree ANTLR already produced ( Style / conventions Matches the codebase: final throughout, imported types over FQNs, tests use Nice work - the reasoning quality in both the code comments and the PR body is unusually high. The three observations above are all optional polish; nothing blocks merge. |
Review: #5626 - walk the subquery bodies the parse-time checks skippedReviewed all four commits, the walker/validator changes, and the tests against the surrounding code. This is a high-quality, well-scoped fix. Summary below with a few non-blocking notes. What works well
Non-blocking notes
None of these block merge. Nice work: the fix is correct, the cleanup is real, and the test suite is thorough. |
…and record the scope approximation The IdentityHashMap dedup made "each expression is visited once" true; nothing proved it. A counting visitor over a query whose WITH is registered on both collections now does, and the test asserts that precondition first, so it cannot pass for the trivial reason that there was nothing to double-visit. Dropping the dedup makes it fail on eight expressions, two of them inside the CALL body. nestedVarTypes returns the body's end state and applies it to every expression in the body wherever it sits. That is the same approximation the top-level statement runs on, and matching it is the point: answering a body more precisely than the query around it would put back a clause-dependent asymmetry. Recorded in the Javadoc, along with why it can only miss a check and never invent one - the only rebinding Cypher allows on a bound name is a WITH projection, which drops a kind but never turns a name into a path. FOREACH collects nothing but the loop variable's removal, because what its inner clauses bind is not in scope after the loop; the walk still checks those clauses' expressions. Noted where the arm is.
|
All three observations addressed in 0e51705 - the first two turned into something enforced rather than documented. 1. The end-state scope. Right, and deliberately so: matching the top-level approximation is the point. Worth adding to your direction analysis, because it makes the approximation safe rather than merely conservative: it can only err one way, and not by luck. A false rejection would need a name to be read at a position where its kind is K, then rebound to 2. "Visited exactly once." Fair to call out an advertised guarantee nothing proved. It is proved now:
The contract stays narrow in the wording: guaranteed for the two registration paths this traversal reconciles; elsewhere once-only falls out of each arm walking a part of the tree no other arm does - a property to preserve when adding an arm, and now called out as such at the arm list rather than implied by the interface. 3. FOREACH. Intentional, and the asymmetry with Green after the change: engine |
|
Review of issue 5626 fix - walking subquery bodies through the parse-time checks Reviewed the full diff (walker, validator, the three subquery expression nodes, the two builder changes, and both test files). This is a high-quality, carefully reasoned change. Summary and a few notes below. What is good
Minor notes / questions (non-blocking)
Nice work - the reasoning in the Javadoc and commit messages made this easy to verify. LGTM pending the CI green you cited. |
…e second pass was never needed The walk covered WITH from two collections and deduplicated by identity to keep from visiting one twice. Both halves were unnecessary: StatementBuilder.addWith is the only place a WITH is registered and it puts the same object on getWithClauses() and on getClausesInOrder(), and StatementBuilder.build() is the only construction site, so the second pass could never reach a clause the first missed. Instrumenting it to raise on a statement-only WITH and running query.opencypher.** confirmed it: 7813 tests, not one hit. So the second pass and the IdentityHashMap guarding it are gone. Visiting once is structural again - each arm covers a part of the tree no other arm does - rather than a property restored by a set allocated per statement, and the latent question of what happens if the two collections ever hold different instances goes with it. everyExpressionIsVisitedExactlyOnce now pins the invariant the single pass rests on: every WITH on the statement is the same instance on the ordered list. A builder path that broke that would make the walk skip the clause, and this is what would say so. Also: the subquery-body AST is a full statement build over an existing sub-tree, rewriters included, not the "one tree walk" the Javadoc claimed - corrected, since a reader sizing the cost should not be told less than it is. And a note at the COLLECT call site on why it passes no pattern/where context: a COLLECT body has no bare-pattern form in the grammar.
|
All four notes addressed in 5b37491. The third one turned out to be pointing at code that should not exist. 3. The double- Confirmed rather than argued: I replaced the second pass with a So the pass and the
1. "One tree walk" understated it. Correct, and fixed: 2. The 4. Whole-body scope. Agreed, and it stays - matching the top-level approximation is the point, since answering a body more precisely than the query around it would put back a clause-dependent asymmetry. Worth adding that it is safe by construction, not by luck: a false rejection would need a name to be read at a position where its kind is K and rebound to Green: |
Review: #5626 walk subquery bodies at parse timeReviewed the full diff plus the surrounding validator/builder/walker code. This is a careful, well-scoped fix that closes a real asymmetry: A few observations, mostly minor: 1. Duplicated scope-building logic (maintainability). 2. Memory / eager build. Each 3. Stricter walk contract. The old validator defensively walked 4. Correctness spot-checks that passed: the null-body path ( I did not run the build myself; relying on the stated green results ( Overall: solid, well-tested, well-documented. None of the above blocks merge; items 1 and 2 are the ones worth a follow-up. |
…st not fail the query Building the body AST put the statement builder on the parse path of something that until now was carried as text and only re-parsed at execution. A construct ANTLR accepts but the builder cannot assemble would therefore have turned a query that used to run into a parse failure, which is a regression this issue has no business causing. The build is now best-effort: on failure the body stays text only, the walk treats the expression as a leaf exactly as before #5626, and execution re-parses the text as it always has. Logged at FINE, so a body with no AST - a body the parse-time checks do not see - stays discoverable instead of becoming permanent. aSubqueryExpressionWithNoBodyIsALeafRatherThanAFailure pins what that relies on. Its visitor dereferences the statement it is handed, the way the validator's does when it builds the inner scope, because the guarantee is not "the walk survives a null body" but "the boundary hook is never called with one" - and only a visitor that reads the statement tells those apart. Removing the guard in walkNested fails it with an NPE; a visitor that ignores its argument would have passed either way. Also cites the grammar at the COLLECT call site rather than asserting it: collectExpression has the single queryWithLocalDefinitions alternative where existsExpression and countExpression have two, so a COLLECT body can never arrive without an AST and quietly skip the checks.
|
Both notes acted on in 2ec34f2. The first one was the more important of the two. 1. The parse-time AST build as a new failure surface. You are right that this is the one behavioural surface beyond "rejected earlier", and it is the wrong direction to leave to luck: a construct ANTLR accepts but So the build is now best-effort. On failure the body stays text only, the expression keeps a null AST, the walk treats it as a leaf exactly as before #5626, and execution re-parses the text as it always has - the pre-issue behaviour, restored precisely for the case that would otherwise regress. Logged at
2. One alternative where the other two have two, because a 3. The follow-up. Agreed, and it is the reason the AST is attached to the expression rather than built and discarded inside the validator: correlating the body as an AST retires both the per-row re-parse and the text scanning in Green: |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 42 |
🟢 Coverage 92.92% diff coverage · -6.68% coverage variation
Metric Results Coverage variation ✅ -6.68% coverage variation Diff coverage ✅ 92.92% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (7081057) 149675 112546 75.19% Head commit (9284669) 181821 (+32146) 124580 (+12034) 68.52% (-6.68%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#5642) 240 223 92.92% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Review: #5626 - walk the subquery bodies the parse-time checks claimed were validated elsewhereI read the full diff (all 7 commits), the surrounding code in What holds up under scrutiny
Observations worth a look (none blocking)
VerificationI did not run the suite here, but the author reports green across Overall: clear problem, minimal-surface fix, strong tests, and the review-follow-up commits each tighten a real edge (WITH pass-through kinds, per-branch UNION scope, best-effort fallback). Nice work. |
…tatement and by a subquery body Phase 2a and the nested scope builder were two copies of "walk the clauses in order and work out what each name is". Two copies drift, and these already had: the statement's tracked SCALAR for a literal or map projection, the body's dropped the name; the statement's raised VariableTypeConflict on a kind clash, the body's overwrote. Writing that walk twice is what let the two spellings of the same import disagree in the first place, so it is now one method with the two differences as parameters - the scope it starts from, and whether a kind clash raises - rather than two methods that resemble each other. Folding them together closed an asymmetry older than this issue. The statement's copy dropped every kind at a WITH * because it kept only what the projection names, while the body's passed the incoming scope through. So MATCH p = (a:P)-[:KNOWS]->(b:P) WITH * RETURN p.name was accepted at the top level and the same access rejected inside a body - the shape #5602 and #5626 exist to remove, in the phase that decides what a name is. Both now reject it, pinned from both positions by aKindSurvivesWithStarInsideAndOutsideASubqueryBody; making the star arm unconditional again fails it. The statement side also picks up the kindless-binding arms the body side had - UNWIND, LOAD CSV, YIELD and a nested subquery's output drop whatever the name held - which can only lose a kind, so it narrows what is checked rather than widening what is rejected.
…der that broke The best-effort fallback caught Exception and logged at FINE, which put two different things in the same quiet place. A CommandParsingException is the case the fallback exists for - the builder saying it does not support this body - and it is not news. Anything else is a defect, and its cost is invisible: the query still runs, the checks just stop seeing inside that body. The second arm now logs at WARNING, which is the difference between a degraded body being discoverable and being theoretical. Also puts the LogManager import back in alphabetical order, and records in the release notes the second potentially-breaking shape the shared scope construction produces: a kind now survives WITH *, so a path property read after one is rejected where the top-level scope used to have forgotten what the name was.
|
Two rounds of feedback landed together here, so this covers both: the maintainability point from the earlier review and the three observations in the latest. Pushed as 669f469 and 0b108de. Duplicated scope construction - unified, and it was hiding something. You were right that two copies of "walk the clauses in order and work out what each name is" would drift, and right that they already had. Rather than a Folding them together closed an asymmetry older than this issue. The statement's copy dropped every kind at a MATCH p = (a:P)-[:KNOWS]->(b:P) WITH * RETURN p.name -- accepted (wrong)
MATCH (n:P) RETURN COLLECT { MATCH p = (a:P)-->(b:P) WITH * RETURN p.name } -- rejected (right)The same access, two answers, decided by whether it was written inside a body - the shape #5602 and #5626 exist to remove, sitting in the phase that decides what a name is. Both reject it now. The statement side also picks up the kindless-binding arms the body side had - 1. The broad 2. Parse-time cost. Agreed, and no further change: the Javadoc says a full statement build including rewriters, and the follow-up that pays it back (correlating the body as an AST to retire the per-row re-parse) is exactly why the AST is attached to the expression rather than built and discarded inside the validator. 3. Import order. Fixed - One suggestion I am not taking, from the earlier review: nulling Green: |
Review: #5626 - walk subquery bodies in parse-time validationReviewed all 9 commits end to end. This is a high-quality, carefully reasoned change: the traversal is generalized cleanly, two byte-for-byte anonymous adapters are collapsed into the named A few observations, mostly minor / for consideration: 1. Intra-body variable-type conflicts are no longer detected (minor consistency gap)
The identical 2. Single-collection WITH walk rests on a builder invariantCommit 6 drops the second 3. Performance: full statement build per subquery body at parse time
4. Best-effort AST build defers some errors to runtime (deliberate)The Nits
Overall: solid, well-tested, and the reasoning in the commit messages made this easy to follow. The only item I'd suggest tracking is #1 (intra-body kind clash) as a small follow-up. Note: I could not execute |
…wing an outer name
Folding the two scope builders together left one flag deciding whether a kind clash raises, off for every
subquery body. That allowed the thing it was meant to allow - a body binding a name the enclosing query uses for
something else, which an implicit CALL { } may do because it imports nothing - but it also suppressed a clash
between two declarations inside the same body. MATCH (x:P) MATCH ()-[x]->() raised VariableTypeConflict written
in the query and passed written in a body, which is the asymmetry this issue exists to remove, one level down
from where it was found.
The two are different questions, so the flag is gone rather than refined: the build now tracks which names the
scope declared itself, and a clash is a clash only between two of those. A statement inherits nothing, so every
clash there is one and the top-level behaviour is unchanged; a body raises on its own second declaration and
stays silent on its first re-declaration of an inherited name. A WITH's projection counts as declaring what it
projects, so past WITH p the body has restated p and a later clash with it is real.
aKindClashInsideABodyIsAConflictWhileShadowingAnOuterNameIsNot asserts both directions, since a check that got
only one right would either miss the conflict or reject the shadow. Both mutations fail it: never raising loses
the conflict, always raising rejects the shadow.
|
Point 1 was a real gap and it is fixed rather than tracked - 9284669. Points 2 to 4 and the nits below. 1. Intra-body kind clashes - fixed, by removing the flag rather than refining it. You put your finger on exactly the wrong shape: one boolean was answering two different questions. Shadowing an inherited name and clashing with one declared here are different questions, so the build now tracks which names the scope declared itself, and a clash is a clash only between two of those: MATCH (n:P) WHERE EXISTS { MATCH (x:P) MATCH ()-[x]->() RETURN x } RETURN n -- VariableTypeConflict, as outside
MATCH p = (a:P)-[:KNOWS]->(b:P) CALL { MATCH (p:P) RETURN p.name AS n } RETURN n -- shadowing, still fineA statement inherits nothing, so every clash there is one and top-level behaviour is unchanged. A
2. The single-collection WITH walk rests on a builder invariant. Agreed on the shape of the risk, and I am leaving it as is. A 3. Parse-time cost / 4. best-effort fallback deferring some errors. Both correct as stated, both deliberate, no change. The fallback's hole in "rejected before it starts" is the price of not regressing queries ANTLR accepts and the builder does not fully support, and it is the smaller of the two harms. Nit - On the Green meanwhile: |
Review: #5626 - walk the subquery bodies the parse-time checks claimed were validated elsewhereThorough, well-reasoned change. I read the whole diff and spot-checked the two invariants it rests on. Both hold, and the test suite is unusually complete for this kind of scope work. Summary: no blocking issues found; a few notes below. What I verified
Code quality
Notes / things to keep an eye on (non-blocking)
Test coverageExcellent. 22 new tests under Nice work. |
|
Thanks - all three notes answered, one with a code change, two with reasons rather than changes. 2. The plan cache does key on raw query text - confirmed, and recorded where it is claimed. On the nesting shape specifically: it adds rather than multiplies. A body reached from 1. A metric for the fallback - not taking it, and the reason is structural. I agree the coverage loss is the invisible kind. But 3. Release note. Shipping in this PR - Still owed: the clean |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5642 +/- ##
============================================
+ Coverage 66.48% 66.50% +0.02%
+ Complexity 1114 1113 -1
============================================
Files 1771 1771
Lines 149675 149829 +154
Branches 31743 31775 +32
============================================
+ Hits 99504 99650 +146
- Misses 37173 37174 +1
- Partials 12998 13005 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Closes #5626.
CypherExpressionWalkerstopped at a subquery held as text, and its class Javadoc explained why: "those are parsed on their own and validated then, and there is no AST here to walk." The issue is right that the second half is false. It turns out a fourth body was escaping too, for a different reason, and two whole predicates were escaping for a third.What actually escaped
EXISTS { }/COUNT { }/COLLECT { }CALL { }walkClause'sdefaultarm skipped it, on the same "validated when it is parsed" beliefWHERE EXISTS { ... }BooleanExpression, a leaf to the walkWHERE isEmpty(x)- a function call as a bare predicateThe last two are the trap this class's own Javadoc warns about ("the
defaultarms treat an unrecognised type as a leaf ... with nothing failing to say so"), and they meant the reproducer in the issue was under-stating the reach: the first of the three forms was invisible even before the body question.The fix
The three expressions now carry their body as a
CypherStatementalongside the text, and the walk descends into it - as it now does into aCALL { }body.The issue's second option, taken, with one correction to its rationale: this does not remove the re-parse. What executes is not the body as written but the body after
CorrelatedSubqueryRewriterhas pinned the outer row into it, which differs row by row, so the text has to stay and the runtime parse with it. What the AST costs is therefore not free - but it is not a second lex either: it is built from the parse tree ANTLR already produced for the body (ctx.queryWithLocalDefinitions(), or thepatternList/whereClauseof a bare-pattern body), so it is one tree walk at parse time, once per distinct query text.The two anonymous adapters are gone. Both were byte-for-byte
BooleanCoercionExpression- sameevaluate, sameevaluateTernary, samegetText- so they are now that named node, which the walk already knew. The function-call one turned out to be wholly redundant with the generic arm three lines below it (both callparseExpressionFromText(expr6)and wrap the result), and its removal also fixes a latent NPE: it wrapped a possibly-null expression, where the generic arm falls back properly.A body is a scope of its own. Reusing the enclosing
varTypesmap for it would have been wrong in both directions, so the visitor re-binds itself at the boundary (Visitor.forNestedStatement): the kinds the body declares, over the ones it inherits, with any name the body binds by any means dropped from what it inherits. That is what keepslegal - an implicit
CALL { }imports nothing, so the body'spis its own node andp.nameis a plain property read, not property access on the outer path.Two more expression positions the same traversal now reaches: procedure
CALLarguments and theLOAD CSV FROMurl.Behaviour change
Same shape as #5602's: a bad call inside a subquery body is now rejected before the query starts instead of failing at runtime - or, where the subquery matched no row, instead of quietly answering
false/0/[]. No check is new; only its reach.One existing test changed.
Issue5228CallMatchEdgeLabelEntityTestprobedCALL () { MATCH (b:EdgeLabel) RETURN labels(b), type(b) }and asserted zero rows;type(b)on a node is the type error it already was outside a subquery, so that half of the probe now asserts the rejection - and asserts it identically inside and outside, which is the point of the issue.Verified against Neo4j
Neo4j runs semantic analysis over the whole query including subquery bodies:
abs('x')isType mismatch: expected Float or Integer but was Stringat compile time wherever it is written, andtype(node)isType mismatch: expected Relationship but was Node. Neither is a Cypher usage mistake on the reporter's side.Tests
CypherSubqueryParseTimeValidationIssue5626Test- 22 tests, all underEXPLAINso the query is parsed and planned but never executed:WHEREWITH ... WHERE, nested one subquery inside another, aUNIONbranch of aCALLbodyCALLarguments andLOAD CSV FROMEXISTS/COUNT/COLLECTstill evaluate to the same answers after the adapter swapGreen: engine
query.**+function.**11311/0,query.opencypher.**7808/0, bolt 93/0, the server Cypher HTTP ITs 20/0.Follow-up worth filing separately
The per-row re-parse is still there, and is a much larger cost than anything here:
EXISTS { }lexes, parses, validates and plans its body once for every outer row. Now that the body exists as an AST, correlating it as an AST - binding the outer row into parameters rather than splicing text - would remove that, and would also retireCorrelatedSubqueryRewriter's text scanning, which is where #4995, #5165, #5461 and #5464 all came from.