Conversation
Distilled from #1245 by Andrew Hundt (upstream b835b5e), re-derived on main's cypher.c rather than cherry-picked -- the file has moved ~9k lines since the branch point. The WHERE clause runs twice: once early, on seed rows where only the first alias is bound, to prune; and once late, when every alias is bound, to decide. The early pass used the same two-valued evaluator as the late one. That evaluator answers "true" for a comparison on an alias it cannot see -- the permissive default that makes the late pass work -- but NOT and XOR invert whatever they are given, so MATCH (a)-[:CALLS]->(b) WHERE NOT b.name CONTAINS "x" evaluated NOT(true) on every seed and pruned every row. Zero results, no error, for a query that is correct. The early pass now uses a three-valued evaluator. A condition on an alias that is not yet bound is UNKNOWN; NOT of UNKNOWN stays UNKNOWN; AND and OR short-circuit only on a definite operand; XOR with any UNKNOWN operand is UNKNOWN. Only a definite FALSE prunes. The late pass is unchanged and still gives the final two-valued answer, so nothing that was correct before changes. Three deliberate differences from the upstream diff: - the unbound-alias rule lives in one leaf, eval_condition_partial, so the expression-tree path and main's legacy flat AND/OR path cannot drift apart; - that leaf guards c->variable == NULL. On main a condition whose left side is a function of literals, coalesce(0, 1) >= 2, carries no variable at all; the upstream version would have passed NULL to strcmp; - the flat path keeps main's rule that an empty condition list is true. RED before the fix, all four with row_count == 0, expected 1: cypher_exec_where_not_on_relationship_target, cypher_exec_where_mixed_alias_and, ..._or, ..._xor. GREEN after: cypher 208 passed; cypher + mcp 526 passed, 4 skipped (the Windows-only skips). eval_expr_partial recurses and is added to CBM_RECURSION_WHITELIST. Co-authored-by: Andrew Hundt <ATHundt@gmail.com> Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
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
1 participant
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.
Distilled from #1245 by @ahundt (upstream b835b5e), carried with
Co-authored-by. Re-derived on main'scypher.crather than cherry-picked — the file has moved ~9k lines since the branch point. #1245 stays open until its distills land.The bug
WHERE runs twice: early on seed rows (only the first alias bound) to prune, and late (all aliases bound) to decide. Both passes used the same two-valued evaluator, which answers true for a comparison on an alias it cannot see — the permissive default the late pass relies on. NOT and XOR invert whatever they are given, so:
evaluated
NOT(true)on every seed and pruned every row. Zero results, no error, for a correct query.The fix
The early pass now uses a three-valued evaluator: unbound alias → UNKNOWN; NOT of UNKNOWN stays UNKNOWN; AND/OR short-circuit only on a definite operand; XOR with any UNKNOWN is UNKNOWN. Only a definite FALSE prunes. The late pass is untouched, so anything that was correct before is unchanged.
Three deliberate differences from upstream: one shared leaf rule for the tree path and main's legacy flat AND/OR path; a
c->variable == NULLguard (on main a literal-only condition likecoalesce(0, 1) >= 2carries no variable — upstream would have passed NULL tostrcmp); the flat path keeps main's empty-list-is-true rule.RED → GREEN
Before the fix, all four new tests fail with
row_count == 0, expected 1:cypher_exec_where_not_on_relationship_target,cypher_exec_where_mixed_alias_and,_or,_xor.After:
cypher208 passed;cypher+mcp526 passed, 4 skipped (the Windows-only skips).eval_expr_partialrecurses and is added toCBM_RECURSION_WHITELIST;scripts/check-nolint-whitelist.shpasses.Formatting note: the whitelist macro was repacked by Homebrew clang-format 22 after the insertion lengthened a line.
AlignEscapedNewlinesand column packing are version-stable, but CI's clang-format 20 is the judge — if lint flags it, that is the only place to look.