Recorded while closing #5626 (PR #5642).
All three subquery expressions end their evaluate() the same way:
} catch (final Exception e) {
LogManager.instance().log(..., Level.FINE, "Error on evaluating EXISTS subquery '%s'", e, modifiedSubquery);
return false; // 0L for COUNT, an empty list for COLLECT
}So a subquery that fails is reported as a subquery that does not match. The two are not the same thing, and the difference is load-bearing:
MATCH (a), (b) WHERE NOT EXISTS { MATCH (a)-[:E {id: $id}]->(b) } CREATE (a)-[:E {id: $id}]->(b)If that body throws for any reason, EXISTS answers false, NOT EXISTS answers true, and a de-duplicating guard degrades into an unconditional CREATE. Nothing surfaces above FINE.
The comments on all three catch blocks are honest about the history - they name #5464, where a corrupted body absorbed as a silent false stayed invisible "for so long" - and #5626's parse-time validation removes a further class of cause. But the absorption itself is still there, and the causes it can still hide are the ones a parse cannot see: a type error on real data, a ConcurrentModificationException, a lock timeout, a security violation, a broken record.
Worth deciding deliberately rather than by inheritance. Some shapes:
- Let it propagate. Cypher's contract for
EXISTS is a boolean, but a query that cannot compute one should fail, not guess. This is what Neo4j does - a failure inside an existential subquery fails the query.
- Absorb only what genuinely means "no match". Keep the neutral value for that, propagate everything else. Needs a real classification, and the retryable exceptions (
NeedRetryException and friends) must propagate or a server-side auto-retry is lost - absorbing a ConcurrentModificationException as false turns a retryable conflict into a wrong answer.
- Raise the log level. Cheapest, and strictly better than today, but it still answers wrongly - it only makes the wrong answer visible to whoever reads the log.
The retryable case is the one to check first: DatabaseAbstractHandler retries an atomic single-request command on NeedRetryException, and that retry cannot fire for a conflict that a subquery has already swallowed into false.
Recorded while closing #5626 (PR #5642).
All three subquery expressions end their
evaluate()the same way:So a subquery that fails is reported as a subquery that does not match. The two are not the same thing, and the difference is load-bearing:
If that body throws for any reason,
EXISTSanswersfalse,NOT EXISTSanswerstrue, and a de-duplicating guard degrades into an unconditionalCREATE. Nothing surfaces aboveFINE.The comments on all three catch blocks are honest about the history - they name #5464, where a corrupted body absorbed as a silent
falsestayed invisible "for so long" - and #5626's parse-time validation removes a further class of cause. But the absorption itself is still there, and the causes it can still hide are the ones a parse cannot see: a type error on real data, aConcurrentModificationException, a lock timeout, a security violation, a broken record.Worth deciding deliberately rather than by inheritance. Some shapes:
EXISTSis a boolean, but a query that cannot compute one should fail, not guess. This is what Neo4j does - a failure inside an existential subquery fails the query.NeedRetryExceptionand friends) must propagate or a server-side auto-retry is lost - absorbing aConcurrentModificationExceptionasfalseturns a retryable conflict into a wrong answer.The retryable case is the one to check first:
DatabaseAbstractHandlerretries an atomic single-request command onNeedRetryException, and that retry cannot fire for a conflict that a subquery has already swallowed intofalse.