fix(cypher): never fabricate an OPTIONAL "no match" after the budget fills - #1385
Open
DeusData wants to merge 1 commit into
Open
fix(cypher): never fabricate an OPTIONAL "no match" after the budget fills#1385DeusData wants to merge 1 commit into
DeusData wants to merge 1 commit into
Conversation
…fills process_edges and expand_var_length carried the expansion budget in the LOOP condition (`ei < edge_count && *new_count < max_new`). Once new_count reached max_new they stopped iterating entirely, so match_count was never incremented for the remaining sources -- even though those sources had real neighbours. expand_pattern_rels' OPTIONAL fallback is ungated (it has a reserved slot per binding since the #1177 sizing fix), so it saw match_count == 0 and emitted an unbound row for every such source. The result is a false negative that reads as a positive assertion: MATCH (f:Function) OPTIONAL MATCH (f)-[:CALLS]->(c) WHERE c IS NULL reports a function as having no callers when it has callers, once an earlier function in the same scan saturated the budget. That is a dead-code query telling a user that live code is dead. The budget caps MATERIALISATION, never detection. Both loops now iterate the full candidate set, increment match_count whenever a real neighbour passes the filters, and gate only the write into new_bindings. The bound proof from #1177 is unchanged -- writes are still capped at max_new and fallbacks are still at most one per binding -- so allocation safety does not depend on this change. Cost: a saturated source still performs its node lookups and filter checks, the same ones the unsaturated path performs, and materialises nothing. Reported by @SEPURI-SAI-KRISHNA while working on the sibling bound-terminal path in #1378; this is the same defect in expand_pattern_rels, which was already live on main rather than introduced there. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
4 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.
Fixes the
expand_pattern_relshalf of the OPTIONAL-fallback problem @SEPURI-SAI-KRISHNA surfaced while working on the sibling bound-terminal path in #1378. This one was already live onmain— it was not introduced there.The bug
process_edgesandexpand_var_lengthcarried the expansion budget in the loop condition:Once
new_countreachedmax_newthey stopped iterating entirely, somatch_countwas never incremented for the remaining sources — even though those sources had real neighbours.expand_pattern_rels' OPTIONAL fallback is ungated (it has a reserved slot per binding since the #1177 sizing fix), so it sawmatch_count == 0and emitted an unbound row.Why that matters more than a dropped row
The failure surfaces as a false negative that reads as a positive assertion:
reports a function as having no callers when it has callers, as soon as an earlier function in the same scan saturated the budget. That is the dead-code query telling a user that live code is dead — and it is the one result a user acts on destructively.
Missing rows are recoverable. Asserting that a match does not exist, when it does, is not.
The fix
The budget caps materialisation, never detection. Both loops now iterate the full candidate set, increment
match_countwhenever a real neighbour passes the filters, and gate only the write intonew_bindings.The bound proof from #1177 is untouched — writes are still capped at
max_new, fallbacks are still at most one per binding — so allocation safety does not depend on this change.Cost: a saturated source still performs its node lookups and filter checks (the same ones the unsaturated path performs) and materialises nothing.
Test
cypher_exec_optional_saturated_does_not_fabricate_no_matchuses the shape that discriminates: A saturates the budget, B genuinely has no callees, C has five. Only B may appear.ASSERT_FALSE(saw_c).Revert-check: restoring the pre-fix loop gate in
process_edgesfails attest_cypher.c:658onASSERT(!(saw_c))— C, which has five callees, is reported as having none. The fabrication reproduces exactly.Deterministic by construction: insertion order fixes the scan order, and every count is exact.
Verification
cypher,store_search,mcp— 425 passed, 2 skippedmake -f Makefile.cbm lint-ci— cleanNote on #1378
That PR makes the bound-terminal path lossless but, as written, introduces this same fabrication there. The
match_countchange requested on it is the same contract as this one; with both in, the two paths agree and neither can fabricate.