Skip to content

fix(cypher): scan all unlabeled query candidates - #1323

Open
Enferlain wants to merge 1 commit into
DeusData:mainfrom
Enferlain:fix/1196-unlabeled-candidate-limit
Open

fix(cypher): scan all unlabeled query candidates#1323
Enferlain wants to merge 1 commit into
DeusData:mainfrom
Enferlain:fix/1196-unlabeled-candidate-limit

Conversation

@Enferlain

@Enferlain Enferlain commented Jul 28, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes #1196.

Unlabeled Cypher queries previously capped candidate enumeration at the requested result limit before applying WHERE. A valid match beyond that early candidate window could therefore disappear.

This change materializes the complete relevant candidate set with an unbounded store query, applies filtering, and only then enforces the final query limit. It includes a reproduce-first regression test with the matching row beyond the result-limit boundary.

Issue #1364 independently exercises the same early unlabeled-scan cap with RETURN DISTINCT type(r). Its separately observed cap above roughly 100k raw projected rows occurs later, before DISTINCT, and is not claimed as part of this fix.

Verification

  • Rebasing over current main preserves the checked cross_join_nodes failure path introduced by the newer bounds work.
  • nix develop --command make -j8 -f Makefile.cbm test-focused TEST_SUITES=cypher
  • Result: 170 passed, including cypher_exec_unlabeled_where_beyond_result_limit_issue1196 and cypher_cross_join_alloc_rejects_overflow.

Checklist

  • Every commit is signed off (git commit -s) and follows the Contributor License Agreement
  • Full test suite passes locally (make -f Makefile.cbm test) — focused sanitized Cypher suite passed
  • Lint passes (make -f Makefile.cbm lint-ci) — not rerun for this rebase-only update
  • New behavior is covered by a regression test that would fail without this fix

@Enferlain

Copy link
Copy Markdown
Author

These are just how I fixed them in my local build until solved on upstream, and since it worked in tests and were built according to the contributing guidelines, gpt said they could be sent as prs.

They can be used as reference/alternative fixes if it can't be merged to hopefully speed up the process. Applies to the subsequent 3 other prs as well.

@DeusData

Copy link
Copy Markdown
Owner

Reviewed — the root cause is right and this is the correct fix layer. Two things stand between it and a merge, and neither of your failing checks is one of them.

The bug is exactly as you diagnosed. Unlabeled Cypher scans fetched at most max_rows * 10 candidates through cbm_store_search before WHERE was applied, so a matching row beyond that window silently vanished. That is #1196's "confidently wrong empty" — MATCH (n) WHERE n.name = 'existing' returning nothing while the labelled form finds it, and drifting after delete-and-reindex cycles because the search ordering changes. Filtering before limiting, the way labelled scans already do, is the right shape.

I also liked that the replacement is simpler than what it removes: the old shallow-copy-and-NULL-out dance became one allocator with a clear caller-frees contract. In an agent-reachable engine that is a memory-safety improvement in its own right, and the new SQL is a constant string with bound parameters.

Your test is properly binding — 12 nodes, max_rows=1 so the old window was 10, match inserted last, asserting both the row count and the row value. Red on main.

Neither failing check is yours. security / codeql-gate polled for its full budget and gave up with BLOCKED: CodeQL timeout, while the actual analyze job passed on the same PR — the gate's timeout races the analyze job nondeterministically, and it passed in 5m22s on your sibling #1326 with no relevant difference. ci-ok is just the aggregator. Every real leg is green: all three smoke platforms, all unix/windows/tsan legs including macos-15-intel, lint, security-static and license-gate.

What is needed from you: a rebase. Your base predates #1173 and #1176, both merged on 30 July and both touching src/cypher/cypher.c and store.c. Those are rebase-class conflicts rather than anything you did. One thing to re-verify afterwards: #1176 added a bounds-check with error propagation in cross_join_nodes, and your change enlarges the candidate sets feeding it — worth confirming the two compose as expected.

And one question that is the maintainer's, not yours. An unlabeled MATCH (n) WHERE … now materialises every node in the project. At our top scale (~8.5M nodes) that is multi-GB for a single query, and a multi-pattern unlabeled query like MATCH (a), (b) is N×N in the binding allocation.

I want to be fair about this: the exposure is not new. cbm_store_find_nodes_by_label is already unbounded, so MATCH (a:Function), (b:Function) does the same thing on a large repo today, and #1176's freshly-merged bounds check means it now fails cleanly rather than overflowing. Your change makes unlabeled consistent with labelled, which is defensible. But full-scan semantics for unlabeled queries is a policy the maintainer should accept explicitly rather than inherit, and #1196 itself asked for a "fail explicit" option. I have raised it — it should not block the correctness fix.

One small thing: the PR body says the scan goes "through the store visitor", but the implementation is a new materialising store query. Reads like drift from an earlier iteration — worth a one-line correction so the next reviewer is not looking for a visitor.

Rebase and I will pick this straight back up.

Signed-off-by: imi <hoshinoimi@gmail.com>
@Enferlain
Enferlain force-pushed the fix/1196-unlabeled-candidate-limit branch from cb9be33 to 64c5dda Compare July 31, 2026 19:00
@Enferlain

Copy link
Copy Markdown
Author

Addressed the follow-up:

  • rebased onto current main (including the newer cross-join bounds work),
  • resolved the overlap by retaining the unbounded unlabeled scan while preserving checked cross_join_nodes error propagation,
  • reran the complete sanitized Cypher suite: 170 passed, including both cypher_exec_unlabeled_where_beyond_result_limit_issue1196 and cypher_cross_join_alloc_rejects_overflow, and
  • corrected the PR description: this implementation materializes an unbounded store query; it does not use a store visitor.

#1364 independently confirms this PR's early unlabeled-candidate cap. I documented its later roughly-100k pre-DISTINCT projection cap as separate scope rather than silently broadening this fix.

@Enferlain
Enferlain marked this pull request as ready for review July 31, 2026 19:01
@Enferlain
Enferlain requested a review from DeusData as a code owner July 31, 2026 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

query_graph: patterns with no label on the source node silently return empty — same DB and binary previously returned correct rows (v0.9.0)

2 participants