Repository navigation
bdDijkstra: Fix #3145 more expensive route - #3174
AdityaRanaX wants to merge 3 commits into
Conversation
Tests for pgr_bdDijkstra, pgr_bdDijkstraCost and pgr_bdDijkstraCostMatrix returning a costlier route than the matching dijkstra function. All graphs are directed, or have cost = reverse_cost on every edge.
pgr_bdDijkstra now records a meeting point as soon as a cost is written, and stops only when no cheaper route can exist. pgr_bdAstar is not changed.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Meeting-route tracking and search termination include/cpp_common/bidirectional.hpp, include/bdDijkstra/bdDijkstra.hpp |
The base class adds record_meeting, which updates the best route when it finds a lower combined cost. Pgr_bdDijkstra calls it after forward and backward relaxations and adds a stopping check based on queue minima and best_cost. |
Issue 3145 regression checks pgtap/bdDijkstra/bdDijkstra/edge_cases/issue_3145.pg, pgtap/bdDijkstra/bdDijkstraCost/edge_cases/issue_3145.pg, pgtap/bdDijkstra/bdDijkstraCostMatrix/edge_cases/issue_3145.pg |
New pgTAP tests compare bidirectional route, cost, and cost-matrix results with standard Dijkstra results. They cover reported graphs, same-node behavior, and straight-chain controls. |
Expected routes and release notes docqueries/bdDijkstra/bdDijkstra.result, docqueries/src/migration.result, NEWS.md, doc/src/release_notes.rst, locale/en/LC_MESSAGES/pgrouting_doc_strings.po, locale/pot/pgrouting_doc_strings.pot |
Expected routes change in the query results. The 4.1.0 bug-fix notes and translation catalogs add issue 3145. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix · Severity of issue fixed: Medium
Suggested reviewers: cvvergara
Merge Risk: 
66849
-
No actionable merge-blocking risk remains; the route correction is ready for normal merge checks.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "bdDijkstra: Fix #3145more expensive route" names the affected function, the issue number, and the defect. It matches the main change: bdDijkstra now records meeting candidates and uses a c…Linked Issues check Issue #3145asks that pgr_bdDijkstra return a minimum-cost route. Its proposed fix is to update the best cost from tentative costs and to stop when the queue-top lower bound rules out a cheaper route.…Out of Scope Changes check All changed paths relate to the #3145fix or its verification. include/cpp_common/bidirectional.hpp: found() becomes virtual so that Pgr_bdDijkstra can override it. record_meeting() is added. The base…Full details: Title check
-
Explanation
The title "bdDijkstra: Fix
#3145more expensive route" names the affected function, the issue number, and the defect. It matches the main change: bdDijkstra now records meeting candidates and uses a corrected stopping bound so it returns the cheapest route, as issue#3145requests. The title is short, specific, and free of file lists or vague wording. The title does not mention the shared pgr_bdDijkstraCost and pgr_bdDijkstraCostMatrix coverage, but the PR objectives describe that as secondary.
Full details: Linked Issues check
-
Explanation
Issue
#3145asks that pgr_bdDijkstra return a minimum-cost route. Its proposed fix is to update the best cost from tentative costs and to stop when the queue-top lower bound rules out a cheaper route. The PR implements both parts at head. (1) Pgr_bidirectional::record_meeting() (include/cpp_common/bidirectional.hpp) updates best_cost and v_min_node when a cost is written. Pgr_bdDijkstra calls it after each relaxation in explore_forward() and explore_backward() (include/bdDijkstra/bdDijkstra.hpp). Each meeting is therefore recorded when both costs are finite, not only when both nodes are settled. (2) Pgr_bdDijkstra::found() stops only when forward_queue.top() + backward_queue.top() >= best_cost. The queues hold real costs, so this sum is a valid lower bound on any route not yet recorded. I traced the issue reprex through the new loop. best_cost becomes 11 at node p after the backward pop of t. The stop test then fails for the next pop, so the search breaks and returns 11, which matches pgr_dijkstra. The requirement is covered for pgr_bdDijkstra. The PR states that pgr_bdDijkstraCost and pgr_bdDijkstraCostMatrix use the same class, so they inherit the fix. I did not run the new pgTAP files. The PR reports them as passing with the full suite (504 files, 51770 tests, 0 failures). I rely on that report and on the code trace.
Full details: Out of Scope Changes check
-
Explanation
All changed paths relate to the
#3145fix or its verification. include/cpp_common/bidirectional.hpp: found() becomes virtual so that Pgr_bdDijkstra can override it. record_meeting() is added. The base found() body is unchanged. include/bdDijkstra/bdDijkstra.hpp: the stopping override, the record_meeting() calls, and the using-declarations required by the new code. pgtap/bdDijkstra/{bdDijkstra,bdDijkstraCost,bdDijkstraCostMatrix}/edge_cases/issue_3145.pg: regression tests for the issue example. docqueries/*.result: regenerated outputs. The PR states that tie-breaking changed for equal-cost routes. Costs and aggregate costs did not change. This follows from the fix and is disclosed. NEWS.md, doc/src/release_notes.rst, and locale POT/PO files: one release-note line and the generated catalog update for the same issue. pgr_bdAstar is unaffected. It does not define found(), so the virtual change does not alter its dispatch. Its explore functions do not call record_meeting(). The undirected-graph cost issue is excluded from this PR. No unrelated behavior change was found in the reviewed diffs.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the paths at dawn,
Then hops where lower costs are drawn.
Two queues advance, the best route stays,
Until their sums close out the maze.
The tests confirm the tracks anew.
Comment @coderabbitai help to get the list of available commands.
Fixes #3145
Problem
pgr_bdDijkstra could return a route that costs more than the cheapest one.
With the example from the issue it returns 12, while pgr_dijkstra returns 11.
pgr_bdDijkstraCost and pgr_bdDijkstraCostMatrix share the same code and
returned the same wrong cost.
Both causes are in include/cpp_common/bidirectional.hpp:
node (found()). A cheaper meeting through a node that only one side had
reached was never recorded. In the issue's example, 10 + 1 = 11 was
known but never taken as the best cost.
best one found. That does not prove that no cheaper route exists.
The two parts have to be fixed together: a stopping rule based on the queue
tops is only valid if the best cost is updated when a cost is written.
Fix
added to Pgr_bidirectional. Nothing else in that file changed.
explore_forward() and explore_backward().
node on the forward side plus the cheapest waiting node on the backward
side is at least best_cost. Any route not yet recorded must pass through
a waiting node on each side, so it cannot be cheaper. In this class the
queues hold the real cost, so that sum is a valid lower bound.
same node. Otherwise both searches meet at a neighbour and report a
round trip. The result stays empty, as before.
Tests
New pgTAP files (10 assertions):
They use the example from the issue and a second example where a one-step
route (cost 4) lost to a two-step route (cost 5). They compare against
pgr_dijkstra, and also check that start = end still returns no rows. They
fail before the fix and pass after it.
Full pgTAP suite on PostgreSQL 14: 504 files, 51770 tests, 0 failures.
cpplint passes on the two changed headers.
Documentation queries
docqueries/bdDijkstra/bdDijkstra.result and docqueries/src/migration.result
were regenerated with doc_queries_generator.pl. In a few rows two routes
have exactly the same cost, and a different one of them is now returned
(6 -> 17 in both files, 15 -> 7 in bdDijkstra.result). Every cost and
agg_cost is unchanged and equal to pgr_dijkstra's, and I checked that the
new routes are valid paths without repeated nodes. I tried "<=" instead of
"<" in record_meeting() to keep the old routes, but that gives other tied
routes, so the old output cannot be kept without undoing the fix.
NEWS.md and doc/src/release_notes.rst have one new line, and NEWS.md was
generated with tools/release-scripts/notes2news.pl.
Not included
unchanged on purpose. Its queue holds cost + heuristic, so the stopping
rule used here is not valid for it, and it has its own separate problem
in bdAstar.hpp. Before and after outputs for pgr_bdAstar are identical.
I will report it in a separate issue.
cost and reverse_cost differ, each edge is charged twice when the path
is rebuilt (path.hpp). For example, a chain of 5 edges
with cost 7 and reverse_cost 2 gives 45 instead of 10. It is present in
4.0.0 and this PR does not change its results. The new tests therefore
use directed graphs, or undirected graphs with cost = reverse_cost. I
will report it in a separate issue. (issue - 3180)
The "Update locale" commit comes from the project's own locale workflow.
@pgRouting/admins
Summary by CodeRabbit