Skip to content

fix(query): correct frontier eviction in shortest path priority queue#9678

Open
shaunpatterson wants to merge 1 commit intodgraph-io:mainfrom
shaunpatterson:sp/fix-frontier-eviction
Open

fix(query): correct frontier eviction in shortest path priority queue#9678
shaunpatterson wants to merge 1 commit intodgraph-io:mainfrom
shaunpatterson:sp/fix-frontier-eviction

Conversation

@shaunpatterson
Copy link
Copy Markdown
Contributor

Summary

  • Replace pq.Pop() with removeMax() — the old code removed the last element of the underlying slice (arbitrary in a min-heap) instead of the highest-cost node. removeMax() does a linear scan and uses heap.Remove() to properly evict the least promising candidate while maintaining the heap invariant.
  • Push before evict — the old code evicted before pushing the new node, so the newcomer was never considered for eviction. A high-cost new node would be admitted while a lower-cost existing node was dropped. Now we push first, then evict the max from the full candidate set.
  • Guard MaxFrontierSize > 0 — skip eviction when the limit is disabled (zero value), preventing accidental eviction when no limit is configured.

Builds on the fix in #9607 which identified the pq.Pop() vs removeMax() issue, and addresses two additional correctness problems (evict-before-push ordering and boundary condition) found during review.

Test plan

  • 24 unit tests with 100% code coverage on all priority queue functions (indexOf, Len, Less, Swap, Push, Pop, removeMax)
  • 10 integration tests exercising maxfrontiersize in DQL shortest path queries:
    • Optimal path found with tight frontier
    • maxfrontiersize=1 doesn't crash (single-path and k-shortest)
    • Large frontier produces identical results to unconstrained query
    • Linear chain graph with small frontier
    • Key regression: high-cost node evicted instead of low-cost node on optimal path
    • Key regression: push-then-evict ensures newcomer competes for eviction

🤖 Generated with Claude Code

Three fixes to the MaxFrontierSize eviction logic in shortest path:

1. Replace pq.Pop() with removeMax() — the old code removed an arbitrary
   element (the last in the underlying slice) instead of the highest-cost
   node. removeMax() does a linear scan to find and remove the actual
   maximum via heap.Remove(), preserving the heap invariant.

2. Push before evict — the old code evicted before pushing the new node,
   so the new node was never considered for eviction. A high-cost new
   node would be admitted while a lower-cost existing node was evicted.
   Now we push first, then evict the max from the full set.

3. Guard MaxFrontierSize > 0 — skip eviction when the limit is disabled
   (zero value), matching the existing Params default.

Tests:
- 24 unit tests achieving 100% coverage on all priority queue and
  eviction functions (indexOf, Len, Less, Swap, Push, Pop, removeMax)
- 10 integration tests exercising maxfrontiersize in DQL shortest path
  queries against a live cluster, covering: optimal path discovery,
  very small frontiers, large frontiers matching unconstrained results,
  k-shortest-path, linear chains, and the specific regression scenarios
  for all three bugs

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@shaunpatterson shaunpatterson requested a review from a team as a code owner April 7, 2026 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant