Two things found while closing #5626 (PR #5642) that are not covered by #5656. They are unrelated to each other; filed together because both came out of the same work and both are about the variable-kind / clause-shape layer rather than about subquery execution.
1. UnionStatement silently answers ten clause accessors with its first branch
UnionStatement implements CypherStatement, and ten of the accessors it inherits delegate to queries.getFirst():
// UnionStatement.java:107-195
public List<MatchClause> getMatchClauses() { return queries.getFirst().getMatchClauses(); }
public WhereClause getWhereClause() { return queries.getFirst().getWhereClause(); }
public ReturnClause getReturnClause() { return queries.getFirst().getReturnClause(); }
public CreateClause getCreateClause() { return queries.getFirst().getCreateClause(); }
public SetClause getSetClause() { return queries.getFirst().getSetClause(); }
public DeleteClause getDeleteClause() { return queries.getFirst().getDeleteClause(); }
public MergeClause getMergeClause() { return queries.getFirst().getMergeClause(); }
public List<UnwindClause> getUnwindClauses() { return queries.getFirst().getUnwindClauses(); }
public List<WithClause> getWithClauses() { return queries.getFirst().getWithClauses(); }
public List<ClauseEntry> getClausesInOrder() { return queries.getFirst().getClausesInOrder(); }So any code holding a CypherStatement and asking what it matches, creates, deletes or binds gets branch 1's answer, with nothing to indicate that branches 2..n were dropped. MATCH (a:P) RETURN a.x AS c UNION MATCH q = (m:P)-->() RETURN q.name AS c reports one MatchClause and no path variable.
Four of those accessors alone have ~150 call sites under com.arcadedb.query.opencypher:
| accessor |
call sites (main) |
getReturnClause() |
67 |
getMatchClauses() |
41 |
getClausesInOrder() |
29 |
getWithClauses() |
12 |
Exactly five files guard with instanceof UnionStatement: CypherExecutionPlanner, CypherExpressionWalker, CypherSemanticValidator, CypherExecutionPlan, SubqueryStep. It works today because those are the entry points, and they route a union to branch-aware handling before anything downstream sees it.
This is a latent-defect generator, not a latent theory. It caught #5626 in review. The shared scope builder added there had to grow an explicit guard, because without it a CALL { ... UNION ... } body would have had branch 1's clauses walked as if they were the union's:
// CypherSemanticValidator.nestedVarTypes, as merged
// (UnionStatement.getClausesInOrder() answers with its FIRST branch's clauses, which is why this returns before
// delegating rather than letting the shared build walk them as if they were the union's.)
if (nested instanceof UnionStatement)
return new HashMap<>(outer);
Nothing would have failed if that guard had been forgotten. The kinds declared by branches 2..n would simply not exist, and the checks that read them would quietly not fire - which is the same failure mode #5626 was filed about.
Severity, stated fairly. The return-column accessors are partly protected: validateUnion enforces DifferentColumnsInUnion, so every branch must project the same column names, and branch 1's names are the union's names. The clause accessors have no such constraint - branch 2 may have entirely different MATCH / CREATE / WITH clauses - so getMatchClauses, getClausesInOrder, getWithClauses, getCreateClause, getMergeClause, getDeleteClause, getSetClause, getUnwindClauses and getWhereClause are the exposed ones. I did not find a reachable user-visible wrong answer on current main; every path I traced is guarded. The report is that the next one will not be, and will not announce itself.
Suggested fix. Not a comment, and not an audit of the 150 call sites - both decay. Make the type stop answering a question it cannot answer:
- Segregate the interface, so
UnionStatement is not a thing you can ask for "the" match clauses at all, and a caller that needs them must reach getQueries() and decide what a union means for its purpose; or
- have the ten throw
UnsupportedOperationException, which turns every unguarded path into a loud failure the first time a union reaches it instead of a silent half-answer.
The first is better if the call sites can absorb it. The second is mechanical and can land immediately. Either converts an invisible wrong answer into something a test can catch.
2. Variable kinds are an end-state map, applied regardless of where the expression sits
validateVariableTypes (statement) and nestedVarTypes (subquery body) share one construction since #5626, and it builds a single map per scope which is then checked against every expression in that scope, wherever it appears. So in
CALL { MATCH (m:P) WHERE type(m) = 'KNOWS' WITH 1 AS m RETURN m }the type(m) in the MATCH is checked against the post-WITH scope, where m is no longer a relationship, and the check does not fire.
This is deliberate and documented as such - it is the same approximation the top-level statement has always used, and answering a body more precisely than the query around it would have reintroduced exactly the clause-dependent asymmetry #5602 and #5626 exist to remove. It is recorded on the method:
What comes back is the body's end state, one map applied to every expression in it wherever that expression sits [...] It errs one way only: a name a later WITH re-binds to something kindless loses its kind for the clauses before it too, so a check is missed, never invented.
It cannot produce a false rejection, and that holds structurally rather than by luck: a false rejection would need a name to be read where its kind is K and then rebound to PATH later in the same scope. Cypher does not allow a bound name to be rebound by a pattern (Variable 'x' already declared), and the one rebinding it does allow - a WITH projection - can drop a kind but never manufacture a path. So the end state is never more path-ish than any position preceding it.
Low priority for that reason. But it is the remaining honest gap in "a body is validated like the query around it", and if it is fixed it has to be fixed on both sides at once - making only the nested side positional would recreate the asymmetry.
Suggested fix. Give the walk a per-clause scope: CypherExpressionWalker already visits clauses in order, so a Visitor hook at each clause boundary would let the kind check advance its scope as it goes. Two things need care: a MATCH's pattern must declare its variables before that clause's own expressions are checked (an inline property predicate references them), and a WITH's projection expressions are evaluated in the pre-projection scope while its WHERE is evaluated in the post-projection one.
Two things found while closing #5626 (PR #5642) that are not covered by #5656. They are unrelated to each other; filed together because both came out of the same work and both are about the variable-kind / clause-shape layer rather than about subquery execution.
1.
UnionStatementsilently answers ten clause accessors with its first branchUnionStatementimplementsCypherStatement, and ten of the accessors it inherits delegate toqueries.getFirst():So any code holding a
CypherStatementand asking what it matches, creates, deletes or binds gets branch 1's answer, with nothing to indicate that branches 2..n were dropped.MATCH (a:P) RETURN a.x AS c UNION MATCH q = (m:P)-->() RETURN q.name AS creports oneMatchClauseand no path variable.Four of those accessors alone have ~150 call sites under
com.arcadedb.query.opencypher:getReturnClause()getMatchClauses()getClausesInOrder()getWithClauses()Exactly five files guard with
instanceof UnionStatement:CypherExecutionPlanner,CypherExpressionWalker,CypherSemanticValidator,CypherExecutionPlan,SubqueryStep. It works today because those are the entry points, and they route a union to branch-aware handling before anything downstream sees it.This is a latent-defect generator, not a latent theory. It caught #5626 in review. The shared scope builder added there had to grow an explicit guard, because without it a
CALL { ... UNION ... }body would have had branch 1's clauses walked as if they were the union's:Nothing would have failed if that guard had been forgotten. The kinds declared by branches 2..n would simply not exist, and the checks that read them would quietly not fire - which is the same failure mode #5626 was filed about.
Severity, stated fairly. The return-column accessors are partly protected:
validateUnionenforcesDifferentColumnsInUnion, so every branch must project the same column names, and branch 1's names are the union's names. The clause accessors have no such constraint - branch 2 may have entirely differentMATCH/CREATE/WITHclauses - sogetMatchClauses,getClausesInOrder,getWithClauses,getCreateClause,getMergeClause,getDeleteClause,getSetClause,getUnwindClausesandgetWhereClauseare the exposed ones. I did not find a reachable user-visible wrong answer on current main; every path I traced is guarded. The report is that the next one will not be, and will not announce itself.Suggested fix. Not a comment, and not an audit of the 150 call sites - both decay. Make the type stop answering a question it cannot answer:
UnionStatementis not a thing you can ask for "the" match clauses at all, and a caller that needs them must reachgetQueries()and decide what a union means for its purpose; orUnsupportedOperationException, which turns every unguarded path into a loud failure the first time a union reaches it instead of a silent half-answer.The first is better if the call sites can absorb it. The second is mechanical and can land immediately. Either converts an invisible wrong answer into something a test can catch.
2. Variable kinds are an end-state map, applied regardless of where the expression sits
validateVariableTypes(statement) andnestedVarTypes(subquery body) share one construction since #5626, and it builds a single map per scope which is then checked against every expression in that scope, wherever it appears. So inthe
type(m)in theMATCHis checked against the post-WITHscope, wheremis no longer a relationship, and the check does not fire.This is deliberate and documented as such - it is the same approximation the top-level statement has always used, and answering a body more precisely than the query around it would have reintroduced exactly the clause-dependent asymmetry #5602 and #5626 exist to remove. It is recorded on the method:
It cannot produce a false rejection, and that holds structurally rather than by luck: a false rejection would need a name to be read where its kind is K and then rebound to
PATHlater in the same scope. Cypher does not allow a bound name to be rebound by a pattern (Variable 'x' already declared), and the one rebinding it does allow - aWITHprojection - can drop a kind but never manufacture a path. So the end state is never more path-ish than any position preceding it.Low priority for that reason. But it is the remaining honest gap in "a body is validated like the query around it", and if it is fixed it has to be fixed on both sides at once - making only the nested side positional would recreate the asymmetry.
Suggested fix. Give the walk a per-clause scope:
CypherExpressionWalkeralready visits clauses in order, so aVisitorhook at each clause boundary would let the kind check advance its scope as it goes. Two things need care: aMATCH's pattern must declare its variables before that clause's own expressions are checked (an inline property predicate references them), and aWITH's projection expressions are evaluated in the pre-projection scope while itsWHEREis evaluated in the post-projection one.