Skip to content

fix(query): correct frontier trimming in shortest path#9599

Open
Schwarf wants to merge 3 commits intodgraph-io:mainfrom
Schwarf:fix/issue-9577-frontier-trim
Open

fix(query): correct frontier trimming in shortest path#9599
Schwarf wants to merge 3 commits intodgraph-io:mainfrom
Schwarf:fix/issue-9577-frontier-trim

Conversation

@Schwarf
Copy link

@Schwarf Schwarf commented Feb 15, 2026

Description

Fix incorrect frontier trimming in shortest path queries when maxfrontiersize
is set.

Previously, the code used pq.Pop() to trim the priority queue. This removes
the last element of the heap rather than the highest-cost
element, which can discard valid low-cost candidates and lead to incorrect
shortest-path results.

The fix replaces slice-based removal with heap.Remove of the maximum-cost
element.

Applied in both:

  • shortestPath
  • runKShortestPaths

A regression unit test has been added to ensure the highest-cost element is
removed when trimming.

Fixes #9577

@Schwarf Schwarf requested a review from a team as a code owner February 15, 2026 12:38
@Schwarf Schwarf force-pushed the fix/issue-9577-frontier-trim branch from ddde7e8 to ae5a482 Compare February 15, 2026 12:47
@Schwarf
Copy link
Author

Schwarf commented Mar 23, 2026

Hi @themavik,
Thanks for pointing this out.

From what I see, both PRs fix the same underlying issue (removing an arbitrary element instead of the highest-cost one), but they differ slightly in eviction policy:

My understanding is that push-then-trim better preserves the intended frontier under a size cap, because it effectively keeps the best M elements from old_frontier ∪ {new_node}. A trim-before-push policy makes the eviction decision without considering the new candidate, which can drop a better existing entry while keeping a worse new one.

In addition, this PR includes a regression test (query/shortest_test.go) to guard this behavior.

That said, I’m happy to align with whichever eviction policy you prefer, or close this PR and contribute the test to #9607 if that’s the direction you’d like to take.

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.

k shortest path does not return the shortest path when using MaxFrontierSize

2 participants