Fix: Restore pgr_boyerMyrvold to build and fix compilation bugs#3078
Draft
Mohit242-bit wants to merge 9 commits intopgRouting:developfrom
Draft
Fix: Restore pgr_boyerMyrvold to build and fix compilation bugs#3078Mohit242-bit wants to merge 9 commits intopgRouting:developfrom
Mohit242-bit wants to merge 9 commits intopgRouting:developfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Boyer–Myrvold planar-embedding feature: new C/C++ sources and driver, SQL function and signature updates, packaging and docs, a pgTAP test suite, minor header and SRF output adjustments, and test configuration/result updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as SQL Client
participant PG as PostgreSQL
participant Driver as C/C++ Driver
participant Alg as Boyer–Myrvold Alg
Client->>PG: CALL pgr_boyermyrvold(text)
PG->>Driver: invoke pgr_do_boyerMyrvold(sql, params...)
Driver->>Alg: build undirected graph, run planarity test
Alg-->>Driver: embedding/result
Driver-->>PG: return SRF rows
PG-->>Client: RETURN SETOF rows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR restores the pgr_boyerMyrvold function to a working state by fixing compilation bugs that prevented it from building. The function implements the Boyer-Myrvold planarity testing algorithm to compute planar embeddings of undirected graphs.
Changes:
- Fixed stale include paths by removing
pgr_prefixes from cpp_common headers - Corrected function signature and UndirectedGraph constructor usage
- Updated IID_t_rt structure member access from old
.source/.targetto current.from_vid/.to_vid - Re-enabled build by adding source files to CMakeLists.txt
- Added comprehensive pgTAP test suite covering edge cases, type checks, and crash tests
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/planar/boyerMyrvold_driver.cpp | Updated include paths, fixed UndirectedGraph default constructor, removed unused variable |
| src/planar/boyerMyrvold.c | Updated IID_t_rt member access to use from_vid/to_vid |
| include/drivers/planar/boyerMyrvold_driver.h | Corrected function name from pgr_do_pgr_boyerMyrvold to pgr_do_boyerMyrvold |
| include/planar/boyerMyrvold.hpp | Removed unused is_kuratowski_subgraph.hpp header |
| src/planar/CMakeLists.txt | Added boyerMyrvold source files back to build |
| sql/planar/CMakeLists.txt | Added boyerMyrvold SQL files back to build |
| pgtap/planar/boyerMyrvold/*.pg | Added 4 comprehensive test files following isPlanar patterns |
| docqueries/planar/test.conf | Added boyerMyrvold.pg to test configuration |
| docqueries/planar/boyerMyrvold.result | Test execution results showing correct function behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docqueries/planar/boyerMyrvold.result`:
- Around line 73-101: The q4 example calls pgr_boyerMyrvold with a filter using
component = 14 which doesn't exist in the sample edges, causing the "No edges
found" error; update the q4 example in the doc to either (A) replace component =
14 with a valid component id present in the edges data so pgr_boyerMyrvold
returns a planar embedding, or (B) if demonstrating the error path is
intentional, add an explicit clarifying comment near q4 in the .pg source that
states this is demonstrating the "no matching edges" error; locate the query
using the pgr_connectedComponents call and the pgr_boyerMyrvold invocation to
make the change.
In `@pgtap/planar/boyerMyrvold/edge_cases.pg`:
- Around line 30-144: The test labels and prepared-statement names are
inconsistent (e.g., zeroEdgeTest2, q3, vertexNotPresent4, oneVertexTest6,
twoVerticesTest10, fullGraphTest11, k5Test12, q13, fourVerticesCyclicTest14)
which makes TAP diagnostics confusing; rename all prepared statements and the
human-readable test descriptions to a single sequential scheme (e.g.,
test01_query, test02_boyerMyrvold, etc.) and update every corresponding RETURN
QUERY label string (the second argument to is_empty/set_eq/throws_ok/isnt_empty)
so the numeric ID and prefix match the prepared-statement name and the actual
test order (1..12); ensure you update both the PREPARE statement names and all
subsequent references to those names in SELECTs so names like q5/q9/q13 are
replaced with the new sequential identifiers and labels reflect the same
numbers.
In `@pgtap/planar/boyerMyrvold/no_crash_test.pg`:
- Line 44: Replace the final call to finish() so it matches the test-suite
idiom: change the plain call "finish()" to a set-returning call by using "SELECT
* FROM finish()" where finish() is invoked; update the statement that currently
reads SELECT finish() to SELECT * FROM finish() to ensure the SETOF TEXT is
expanded consistently with other tests.
In `@sql/planar/CMakeLists.txt`:
- Around line 5-10: The package adds new SQL files (_boyerMyrvold.sql,
boyerMyrvold.sql, _isPlanar.sql, isPlanar.sql) which introduce the new
pgr_boyerMyrvold signature—before merging, re-run the repository's
signature-generation step to regenerate the auto-generated pgrouting--*.sig file
so the new pgr_boyerMyrvold entry is captured (do not hand-edit the sig file);
ensure the generation is performed as part of the release or CI step so sql/sigs
reflects the new SQLs.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
docqueries/planar/boyerMyrvold.resultdocqueries/planar/test.confinclude/drivers/planar/boyerMyrvold_driver.hinclude/planar/boyerMyrvold.hpppgtap/planar/boyerMyrvold/edge_cases.pgpgtap/planar/boyerMyrvold/inner_query.pgpgtap/planar/boyerMyrvold/no_crash_test.pgpgtap/planar/boyerMyrvold/types_check.pgsql/planar/CMakeLists.txtsrc/planar/CMakeLists.txtsrc/planar/boyerMyrvold.csrc/planar/boyerMyrvold_driver.cpp
💤 Files with no reviewable changes (1)
- include/planar/boyerMyrvold.hpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pgtap/planar/boyerMyrvold/no_crash_test.pg`:
- Line 9: Update the version check in the test to match the actual SQL API
presence: change the call to min_version('3.2.0') to min_version('4.1.0') so the
conditional in SELECT CASE WHEN min_version(...) THEN plan (5) ELSE plan(1) END;
correctly reflects that the function exists starting in pgrouting 4.1; locate
the min_version call in the no_crash_test.pg test and replace the version string
accordingly.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docqueries/planar/boyerMyrvold.result (1)
73-101: Error output in doc example can confuse readers.
q4 rendersERROR: No edges found. If this is intentional, keep it but ensure the companion.pgsource clarifies the purpose; otherwise, use a component that exists in sample data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docqueries/planar/boyerMyrvold.result` around lines 73 - 101, The example query q4 calling pgr_boyerMyrvold with a WHERE component = 14 causes "ERROR: No edges found"; either update the component filter to a component that exists in the sample data (replace the literal 14 in both pgr_connectedComponents calls) so the query returns edges, or leave the error but add a clarifying comment in the companion .pg source explaining that the error is intentional and why; locate the occurrences by referencing the q4 block and the pgr_connectedComponents calls that build the ARRAY used by pgr_boyermyrvold.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docqueries/planar/boyerMyrvold.result`:
- Around line 73-101: The example query q4 calling pgr_boyerMyrvold with a WHERE
component = 14 causes "ERROR: No edges found"; either update the component
filter to a component that exists in the sample data (replace the literal 14 in
both pgr_connectedComponents calls) so the query returns edges, or leave the
error but add a clarifying comment in the companion .pg source explaining that
the error is intentional and why; locate the occurrences by referencing the q4
block and the pgr_connectedComponents calls that build the ARRAY used by
pgr_boyermyrvold.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pgtap/planar/boyerMyrvold/edge_cases.pg (1)
24-144: 🧹 Nitpick | 🔵 TrivialTest labels remain inconsistent across cases.
This matches the prior review concern about non‑sequential labels and mixed naming (q1/q3 vs zeroEdgeTest2, etc.), which makes TAP diagnostics harder to correlate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pgtap/planar/boyerMyrvold/edge_cases.pg` around lines 24 - 144, The TAP test labels are inconsistent (mixed q1/q3 and descriptive names like zeroEdgeTest2/vertexNotPresent4) which hurts diagnostics; rename and standardize all PREPARE identifiers and their corresponding result-check calls to a consistent sequential naming scheme (e.g., test1_prepare/test1_check or test1_query/test1_result) so each case maps 1:1 — update PREPARE q1 -> PREPARE test1Query, throws_ok zeroEdgeTest2 -> test2Result, PREPARE q3/vertexNotPresent4 -> test3Query/test3Result, q5/oneVertexTest6 -> test4Query/test4Result, q9/twoVerticesTest10 -> test5Query/test5Result, fullGraphTest11 -> test6Query/test6Result, k5Test12 -> test7Query/test7Result, q13/fourVerticesCyclicTest14 -> test8Query/test8Result and adjust the SELECT calls and descriptive TAP messages to use the new names so labels are sequential and consistent across the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pgtap/planar/boyerMyrvold/edge_cases.pg`:
- Around line 24-144: The TAP test labels are inconsistent (mixed q1/q3 and
descriptive names like zeroEdgeTest2/vertexNotPresent4) which hurts diagnostics;
rename and standardize all PREPARE identifiers and their corresponding
result-check calls to a consistent sequential naming scheme (e.g.,
test1_prepare/test1_check or test1_query/test1_result) so each case maps 1:1 —
update PREPARE q1 -> PREPARE test1Query, throws_ok zeroEdgeTest2 -> test2Result,
PREPARE q3/vertexNotPresent4 -> test3Query/test3Result, q5/oneVertexTest6 ->
test4Query/test4Result, q9/twoVerticesTest10 -> test5Query/test5Result,
fullGraphTest11 -> test6Query/test6Result, k5Test12 -> test7Query/test7Result,
q13/fourVerticesCyclicTest14 -> test8Query/test8Result and adjust the SELECT
calls and descriptive TAP messages to use the new names so labels are sequential
and consistent across the file.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
pgtap/planar/boyerMyrvold/edge_cases.pgpgtap/planar/boyerMyrvold/inner_query.pgpgtap/planar/boyerMyrvold/no_crash_test.pgpgtap/planar/boyerMyrvold/types_check.pg
cvvergara
requested changes
Mar 1, 2026
Member
cvvergara
left a comment
cvvergara
left a comment
There was a problem hiding this comment.
@Mohit242-bit
Documentation is missing we need documentation
Looks like there was no documentation created when this was attempted on 2020
Please add documentation.
you will need to add the .rst of the function under doc/planar directory
also modify the doc/_static/page_history.js
It is an experimental function.
Also the SQL files should have the correct version 4.1
For example
--v3.2
CREATE FUNCTION ....
Change to
--v4.1
CREATE FUNCTION
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/planar/pgr_boyerMyrvold.rst`:
- Line 7: Remove the version text from the :single: index entry for the official
function; specifically edit the :single: entry referencing "boyerMyrvold" so it
does not include " - Experimental on v4.1" and leave any version/availability
notes only in the Availability section for the boyerMyrvold function
documentation.
In `@locale/pot/pgrouting_doc_strings.pot`:
- Around line 7543-7550: Duplicate return-description msgids are present: "This
function returns the edges of the planar embedding when the graph is planar."
and "It will return the edges of the planar embedding when the graph is
planar."; keep only one of these return-description sentences in the source .rst
and remove the other to avoid duplicate msgids, while retaining the
implementation note "This implementation uses the Boyer-Myrvold Planarity
Testing." so the doc contains a single concise return sentence plus the
implementation note.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
doc/_static/page_history.jsdoc/planar/CMakeLists.txtdoc/planar/pgr_boyerMyrvold.rstdoc/src/experimental.rstlocale/en/LC_MESSAGES/pgrouting_doc_strings.polocale/pot/pgrouting_doc_strings.potsql/planar/_boyerMyrvold.sqlsql/planar/boyerMyrvold.sql
…fix finish() call
Mohit242-bit
force-pushed
the
fix/clean-restore-boyerMyrvold
branch
from
e6731b1 to
d310928
Compare
cvvergara
requested changes
Mar 4, 2026
| @@ -0,0 +1,135 @@ | |||
| :file: This file is part of the pgRouting project. | |||
| :copyright: Copyright (c) 2020-2026 pgRouting developers | |||
Member
There was a problem hiding this comment.
Actually the file is originally from 2026 so
:copyright: Copyright (c) 2026-2026 pgRouting developers
| Signatures | ||
| ------------------------------------------------------------------------------- | ||
|
|
||
| .. rubric:: Summary |
Member
There was a problem hiding this comment.
There is only one function so no summary word is needed
cvvergara
requested changes
Mar 4, 2026
| -----+--------+--------+------ | ||
| 1 | 14 | 15 | 1 | ||
| (1 row) | ||
| ERROR: No edges found |
Member
There was a problem hiding this comment.
hmmm, the inner query is not returning edges
SELECT node FROM pgr_connectedComponents(
'SELECT id, source, target, cost, reverse_cost FROM edges ')
WHERE component = 14;
node
------
(0 rows)
but this one works:
SELECT node FROM pgr_connectedComponents(
'SELECT id, source, target, cost, reverse_cost FROM edges ')
WHERE component = 13;
node
------
13
14
(2 rows)
Member
|
Because of the unused include I went to read the boost documentation for boyer_myrvold Computing a planar embedding for a graph if it is planar, otherwise finding a set of edges that forms an obstructing Kuratowski subgraph: Right now when the graph is not planar its giving an empty result, the intention is to output the kuratowski_subgraph when its not planar. See this example from boost |
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.
Problem:
pgr_boyerMyrvoldwas developed but was never functional due to several bugs that prevented it from compiling. The source files existed but were excluded from the build system insrc/planar/CMakeLists.txtandsql/planar/CMakeLists.txt.Changes proposed in this pull request:
boyerMyrvold_driver.cpppgr_prefixalloc.hpp,assert.hppetc.boyerMyrvold_driver.hpgr_do_boyerMyrvoldboyerMyrvold_driver.cppUndirectedGraphconstructorboyerMyrvold.cIID_t_rtmembers.source/.targetto.from_vid/.to_vidboyerMyrvold.hppis_kuratowski_subgraph.hppCMakeLists.txt(src/sql)Testing
isPlanarpatterns.Note:
RST documentation is not included in this PR — please let me know if it should be added here or handled in a follow-up.
@pgRouting/admins
Summary by CodeRabbit
New Features
Tests
Documentation
Packaging