pgr_contraction_hierarchies with squashed commits #249
Conversation
WalkthroughThis pull request integrates the new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/SQL Client
participant SQL as PostgreSQL Engine
participant F as _pgr_contraction_hierarchies (C Function)
participant G as ContractionGraph Module
participant R as Result Formatter
U->>SQL: CALL pgr_contraction_hierarchies(edges_sql, forbidden_vertices, directed)
SQL->>F: Invoke _pgr_contraction_hierarchies with parameters
F->>G: Initialize graph, set forbidden vertices, process shortcuts
G-->>F: Return contracted nodes and shortcut edges
F->>SQL: Construct result tuples
SQL->>U: Return contraction hierarchies results
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
AuBousquet
changed the title
AuBousquet
force-pushed
the
contraction_hierarchies_squashed
branch
2 times, most recently
from
fa6f2bb to
ee6eed4
Compare
AuBousquet
force-pushed
the
contraction_hierarchies_squashed
branch
from
1fc2794 to
8ed0144
Compare
|
@coderabbitai review |
🧩 Analysis chainI'll review and summarize this pull request for you. First, let me gather information about the changes in PR #249. 🏁 Script executed: Length of output: 303 It seems there was an issue with the GitHub CLI command. Let me try again with the correct repository specification. 🏁 Script executed: Length of output: 9698 Now let me examine some of the key files in this PR to provide a detailed review and summary. 🏁 Script executed: Length of output: 62319 @cvvergara: I've reviewed the pull request #249 which adds the new ReviewThis PR implements contraction hierarchies for pgRouting, which is a technique to significantly speed up shortest path calculations in large graphs like road networks. The main features include:
The implementation is comprehensive with:
The function will be valuable for applications dealing with large road networks or any graph where shortest path computation performance is critical. SummaryThis PR adds [summary] ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (35)
pgtap/contraction/contraction_hierarchies/inner_query.pg (2)
27-27: Consider adding documentation about the test plan.The script plans for 54 tests, but it's not immediately clear how these tests are structured or what specifically they're validating.
SELECT plan(54); +-- Tests for pgr_contraction_hierarchies with inner queries +-- Testing various combinations of parameters and edge cases
31-31: Remove unnecessary empty line.For code consistency, consider removing this empty line.
SELECT style_dijkstra('pgr_contraction_hierarchies(', ', ARRAY[1]::BIGINT[], true)'); - SELECT finish();locale/en/LC_MESSAGES/pgrouting_doc_strings.po (5)
2778-2793: Use "the first two" instead of "the two first ones."
Minor grammar improvement recommended for clarity in documentation.- The two first ones correspond to the method ``pgr_contraction``: + The first two correspond to the method ``pgr_contraction``:
2802-2816: Clarify handling for non-positive edge costs.
Consider adding a brief note about how negative or zero-cost edges are handled (e.g., if they're unsupported or filtered out) to prevent user confusion.
3063-3205: Minor grammar improvement: remove “be” or reorder the phrase “can be then used.”- The graph built with this method can be then used with ... + The graph built with this method can then be used with ...
3448-3503: Prefer “changes made to” instead of “changes done to.”
Minor grammar fix for clarity.- They represent the changes done to the graph + They represent the changes made to the graph
10040-10116: Minor grammar improvement: “As with the pgr_contraction method.”- As for ``pgr_contraction`` method, it does not return the full contracted graph, + As with the ``pgr_contraction`` method, it does not return the full contracted graph,include/c_types/contraction_hierarchies_rt.h (1)
2-2: Incorrect file name in the header comment.The comment states "File: contracted_rt.h" but the actual file name is "contraction_hierarchies_rt.h". This should be corrected for consistency.
-File: contracted_rt.h +File: contraction_hierarchies_rt.hsql/contraction/_contraction_hierarchies.sql (2)
37-52: Function Declaration and Signature:The PostgreSQL C function
_pgr_contraction_hierarchiesis defined with clear input parameters and output columns. The use ofRETURNS SETOF RECORD,LANGUAGE C,VOLATILE, andSTRICTis consistent with expected pgRouting conventions for internal functions.Suggestion: For improved clarity and potential consumers of this function, consider using the
RETURNS TABLE(...)syntax to explicitly name the output columns. This minor refactor could help document the expected schema directly in the function signature.
54-57: Function Documentation Comment:The SQL comment attached to the function indicates that it is an internal pgRouting function. While this is acceptable for internal usage, you might consider expanding the comment in the future with more detailed explanations of the parameters, expected behavior, and any side effects. This added documentation could enhance maintainability as the implementation grows in complexity.
locale/pot/pgrouting_doc_strings.pot (1)
3079-3111: Fix minor spelling error at line 3085.Change "he following query" to "The following query" for clarity:
- he following query shows the original data involved + The following query shows the original data involvedinclude/visitors/dijkstra_visitors.hpp (1)
70-75: Consider adding documentation for the new visitor class.The purpose of this visitor appears to be tracking vertices within a maximum distance threshold. Adding brief documentation would help users understand how to properly use this class.
doc/contraction/pgr_contraction_hierarchies.rst (3)
1-9: Clarify Licensing HeaderThis block provides the licensing terms. Ensure consistency with the project's standard heading format in other documentation files (e.g., confirm alignment with
doc/index.rstor other reference pages).
36-45: Refine Example Context for Graph UsageThe phrase "like the road graphs or electric networks" might be made clearer or more general. For instance, referencing "road networks or power grids" clarifies the type of large-scale networks the function can handle.
- In large graphs, like the road graphs or electric networks, ... + In large graphs, such as road networks or power grids, ...
139-168: Highlight the Distinction Between Vertex and Edge RowsThe result table details that vertex rows have positive IDs and edge rows have negative IDs. It may be helpful to add a short note stating an example ID for each type to avoid confusion for new users.
src/contraction/contractionHierarchies_driver.cpp (1)
90-104: Encapsulate Contraction LogicThe call to
detail::perform_contraction_hierarchiesis well-structured. Any future expansions or concurrency concerns, such as parallel processing of large graphs, might be addressed by further abstracting or wrapping the logic in a dedicated class or manager.doc/contraction/contraction-family.rst (1)
48-49: Maintain consistent voice.The phrase “in the future, currently” can be streamlined. Consider removing “in the future” or rephrasing for clarity.
Possible revision:
- in the future, currently, it supports three algorithms. + it currently supports three algorithms, with more planned for future releases.src/common/ch_vertex.cpp (1)
42-45: Consider using a const reference parameter.If
contracted_vertices_idswon't be modified externally while setting, consider declaring the parameter as a const reference for clarity and to prevent accidental changes:- void CH_vertex::set_contracted_vertices( - Identifiers<int64_t>& contracted_vertices_ids) { + void CH_vertex::set_contracted_vertices( + const Identifiers<int64_t>& contracted_vertices_ids) { m_contracted_vertices = contracted_vertices_ids; }src/contraction/contractionHierarchies.c (1)
1-29: File naming and licensing.The licensing header is correctly formatted. However, consider renaming the file to align with
_pgr_contraction_hierarchiesfor clarity (e.g.,contractionHierarchies.c).include/cpp_common/ch_vertex.hpp (5)
45-46: Add doc comments for new fields.
It would be helpful for future maintainers if you briefly document the purposes ofvertex_orderandmetric—especially as they appear central to the contraction workflows.
47-47: Consider initializing the new members in the constructor.
Currently,vertex_orderandmetricdefault to zero. If these members need explicit initial values, consider using an initialization list in the constructor for clearer intent.
51-51: Consider passing by const reference to reflect immutability.
Ifset_contracted_verticesdoesn't modify its parameter, switching toconst Identifiers<int64_t>&can help clarify usage and prevent unintended changes.
56-57: Unify the addition methods for consistency.
You have two methods to add contracted vertices (single and multiple). Consider overloading or consistently naming them to avoid duplication and improve discoverability.
63-63: Add or verify automated tests for the << operator.
Ensuring youroperator<<is tested can help maintain consistent formatting of debug output as additional fields are added.include/drivers/contraction/contractionHierarchies_driver.h (1)
60-68: Add detailed documentation for function parameters.
Providing parameter-level detail (e.g., explaining the ArrayType*, bool, and output pointers) will help contributors understand how to call and extend the function.include/contraction/linearContraction.hpp (2)
65-65: Avoid repeated calls if efficiency matters.
Ifgraph.getForbiddenVertices()is non-trivial, consider storing a reference locally or caching the result. Otherwise, this is acceptable for smaller datasets.
79-79: Confirm the parameter-passing semantics.
graph.setForbiddenVertices(forbidden_vertices);might be safer if usingconst Identifiers<V>&unless the graph modifies the container internally.include/cpp_common/to_postgres.hpp (2)
81-86: Document thedirectedparameter.
The doc comment references forbidden vertices and logs but doesn’t clarify the impact of thedirectedboolean on contraction. A brief explanation would help maintainers.
109-114: Catch specific exceptions where possible.
Catching...makes debugging harder by silencing specific error info. If feasible, catchstd::exception&to log the actual exception message before rethrowing.include/contraction/contractionHierarchies.hpp (3)
68-77: Clarify docstring return value invertex_contractionThe docstring for
vertex_contractionreferences the returned "metric" but doesn’t explicitly clarify how it might be used (e.g., reinsert into a priority queue, or for logging or debugging). Adding a note could help maintainers understand the role of the returned metric.
99-110: Duplicate adjacency logic for directed vs. undirectedThe function checks adjacency lists differently for directed vs. undirected graphs. Consider unifying or extracting this logic into a helper function to avoid duplication, making the code more maintainable.
153-233: Potential performance concern incompute_shortcutsRunning Dijkstra for each vertex contraction can be costly for very large graphs. If performance becomes an issue, consider parallelizing across different vertices or reusing partial results. Additionally, you might investigate specialized data structures or more advanced pruning logic, given the repetitive nature of shortest path computations in contraction hierarchies.
include/contraction/contractionGraph.hpp (3)
68-73: Constructor clarityThe constructor initializes
min_edge_id = 0, but the code immediately decrements it inget_next_id(), flooding negative IDs. This is deliberate for shortcuts but might merit a clarifying comment or docstring so future maintainers understand the negative ID design.
385-417:is_shortcut_possiblebranchingThe method uses compound boolean checks for directed vs. undirected scenarios. It might be clearer to break each scenario into separate condition blocks or helper methods. Consider a small refactor to improve readability.
479-516: Consistent usage of process shortcutsThe logic in
process_ch_shortcutandprocess_shortcutis quite similar. If duplication grows, consider merging them or refactoring to a single shared method that logs and adds edges.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
doc/contraction/images/Fig6-unidirected-contraction-hierarchies.pngis excluded by!**/*.png
📒 Files selected for processing (51)
NEWS.md(1 hunks)doc/conf.py.in(1 hunks)doc/contraction/CMakeLists.txt(1 hunks)doc/contraction/contraction-family.rst(3 hunks)doc/contraction/images/CMakeLists.txt(1 hunks)doc/contraction/pgr_contraction_hierarchies.rst(1 hunks)doc/src/release_notes.rst(1 hunks)docqueries/contraction/CMakeLists.txt(1 hunks)docqueries/contraction/contraction-family.pg(1 hunks)docqueries/contraction/contraction-family.result(1 hunks)docqueries/contraction/contraction_hierarchies.pg(1 hunks)docqueries/contraction/contraction_hierarchies.result(1 hunks)docqueries/contraction/test.conf(1 hunks)include/c_types/contraction_hierarchies_rt.h(1 hunks)include/contraction/ch_graphs.hpp(1 hunks)include/contraction/contract.hpp(1 hunks)include/contraction/contractionGraph.hpp(4 hunks)include/contraction/contractionHierarchies.hpp(1 hunks)include/contraction/deadEndContraction.hpp(3 hunks)include/contraction/linearContraction.hpp(3 hunks)include/cpp_common/base_graph.hpp(1 hunks)include/cpp_common/ch_edge.hpp(2 hunks)include/cpp_common/ch_vertex.hpp(1 hunks)include/cpp_common/identifiers.hpp(1 hunks)include/cpp_common/to_postgres.hpp(2 hunks)include/drivers/contraction/contractionHierarchies_driver.h(1 hunks)include/visitors/dijkstra_visitors.hpp(1 hunks)include/visitors/found_goals.hpp(1 hunks)locale/en/LC_MESSAGES/pgrouting_doc_strings.po(7 hunks)locale/es/LC_MESSAGES/pgrouting_doc_strings.po(0 hunks)locale/ja/LC_MESSAGES/pgrouting_doc_strings.po(0 hunks)locale/ko/LC_MESSAGES/pgrouting_doc_strings.po(0 hunks)locale/pot/pgrouting_doc_strings.pot(7 hunks)locale/zh_Hans/LC_MESSAGES/pgrouting_doc_strings.po(0 hunks)pgtap/contraction/contraction_hierarchies/edge_cases/directed.pg(1 hunks)pgtap/contraction/contraction_hierarchies/edge_cases/undirected.pg(1 hunks)pgtap/contraction/contraction_hierarchies/inner_query.pg(1 hunks)pgtap/contraction/contraction_hierarchies/no_crash_test.pg(1 hunks)pgtap/contraction/contraction_hierarchies/types_check.pg(1 hunks)sql/contraction/CMakeLists.txt(1 hunks)sql/contraction/_contraction_hierarchies.sql(1 hunks)sql/contraction/contraction_hierarchies.sql(1 hunks)sql/sigs/pgrouting--4.0.sig(1 hunks)src/common/ch_edge.cpp(3 hunks)src/common/ch_vertex.cpp(2 hunks)src/contraction/CMakeLists.txt(1 hunks)src/contraction/contractGraph_driver.cpp(1 hunks)src/contraction/contractionHierarchies.c(1 hunks)src/contraction/contractionHierarchies_driver.cpp(1 hunks)tools/testers/contraction_hierarchies_data.sql(1 hunks)tools/testers/setup_db.sh(1 hunks)
💤 Files with no reviewable changes (4)
- locale/zh_Hans/LC_MESSAGES/pgrouting_doc_strings.po
- locale/es/LC_MESSAGES/pgrouting_doc_strings.po
- locale/ko/LC_MESSAGES/pgrouting_doc_strings.po
- locale/ja/LC_MESSAGES/pgrouting_doc_strings.po
✅ Files skipped from review due to trivial changes (2)
- docqueries/contraction/contraction_hierarchies.pg
- tools/testers/contraction_hierarchies_data.sql
🔇 Additional comments (81)
docqueries/contraction/test.conf (1)
8-8: Configuration updated to include contraction hierarchies test file.The new
contraction_hierarchies.pgfile has been correctly added to the test configuration, ensuring that the new functionality will be properly tested.doc/contraction/CMakeLists.txt (1)
5-5: Documentation file for the new contraction hierarchies function added correctly.The
pgr_contraction_hierarchies.rstfile has been properly included in theLOCAL_FILESlist, ensuring it will be processed during documentation build.doc/contraction/images/CMakeLists.txt (1)
6-6: Image file for contraction hierarchies successfully added.The
Fig6-unidirected-contraction-hierarchies.pngimage has been properly included in the build configuration, which will enhance the visual documentation of the contraction hierarchies feature.src/contraction/CMakeLists.txt (1)
5-6: Source files for contraction hierarchies implementation correctly added.Both the C implementation file (
contractionHierarchies.c) and C++ driver file (contractionHierarchies_driver.cpp) have been properly added to the library build configuration.NEWS.md (1)
27-27: Documentation accurately reflects the new function additionThe addition of
pgr_contraction_hierarchiesto the list of officially promoted functions aligns with the PR's purpose of introducing this new functionality to pgRouting.docqueries/contraction/CMakeLists.txt (1)
5-5: Proper integration with documentation build systemThe addition of
contraction_hierarchiesto theLOCAL_FILESlist ensures that the documentation queries for the new function will be properly configured and included in the build process.include/visitors/found_goals.hpp (1)
32-34: Well-structured addition of new exception type for visitor terminationThe addition of the
max_dist_reachedstruct follows the established pattern for defining visitor termination exceptions. The comment update correctly reflects that there are now multiple exception types defined in this file.doc/src/release_notes.rst (1)
58-58: Documentation properly updated in release notesThe addition of
pgr_contraction_hierarchiesto the list of officially promoted functions in the reStructuredText format ensures the function will be properly documented in the generated release notes.include/cpp_common/identifiers.hpp (1)
54-59: LGTM! The newset_idsmethod provides a useful way to modify identifiers.The method implementation looks good and follows the class's existing style and conventions. This addition enhances the API by allowing modification of the internal set of identifiers after object construction, which could be useful for the contraction hierarchies functionality.
include/cpp_common/base_graph.hpp (1)
181-181: LGTM! Documentation addition forUndirectedUniqueGraph.The added documentation line properly describes the
UndirectedUniqueGraphtype and follows the existing documentation format.sql/contraction/CMakeLists.txt (1)
5-6: LGTM! Added new contraction hierarchies SQL files.The changes correctly add two new SQL files to the
LOCAL_FILESlist, following the established naming convention where files with a leading underscore typically contain internal implementation details.docqueries/contraction/contraction-family.pg (1)
199-202: LGTM! Added example forpgr_contraction_hierarchies.Good addition of a usage example that demonstrates the new contraction hierarchies functionality. The query structure is consistent with other examples in the file and shows how to use the function with the
directedparameter.tools/testers/setup_db.sh (1)
35-35: Addition of contraction hierarchies test data looks good.This change correctly adds the new SQL data file needed for testing the contraction hierarchies functionality.
pgtap/contraction/contraction_hierarchies/inner_query.pg (2)
26-26: Verify that this data modification is appropriate for all test cases.The update to
costandreverse_costvalues will affect all edges in the test database. Ensure this won't interfere with other test scenarios or expected outcomes.
29-29: LGTM - Test invocation using style_dijkstra helper function.The approach of using a helper function to generate test cases is a good practice for reducing test code duplication.
locale/en/LC_MESSAGES/pgrouting_doc_strings.po (2)
11-11: No issues found.
4643-4648: No issues found.docqueries/contraction/contraction-family.result (1)
391-422: Good demonstration of the new pgr_contraction_hierarchies function!The example effectively showcases the function's capabilities with an undirected graph. The output clearly displays both vertex and edge information with all the relevant columns (type, id, contracted_vertices, source, target, cost, metric, vertex_order).
doc/conf.py.in (1)
381-381: Appropriate documentation placeholder for contraction hierarchies result type.The replacement directive correctly specifies the structure of the result type returned by the pgr_contraction_hierarchies function, matching its implementation and making it easier to maintain consistent documentation.
pgtap/contraction/contraction_hierarchies/types_check.pg (1)
1-37: Well-structured type check tests for pgr_contraction_hierarchies.The tests properly verify the function's existence, signature, parameter types, and return type. The test style follows pgTap conventions and provides good basic verification of the function interface.
include/c_types/contraction_hierarchies_rt.h (1)
40-50: Well-structured definition for contraction hierarchies result type.The structure correctly defines all necessary fields for representing contraction hierarchies results, with appropriate data types. The included fields support both vertex and edge information needed for the algorithm's output.
include/contraction/ch_graphs.hpp (1)
53-56: Good addition of CHUniqueUndirectedGraph type alias.The new
CHUniqueUndirectedGraphusingboost::setSas the edge container is a valuable addition. UsingsetSinstead oflistSensures edge uniqueness and provides O(log N) complexity for edge lookups, which is beneficial for the contraction hierarchies algorithm where duplicate edges should be avoided.include/contraction/contract.hpp (1)
109-109: Improved design by centralizing forbidden vertices management.Moving the responsibility of setting forbidden vertices from
deadendContractorto thegraphitself is a good architectural change. This centralizes the management of forbidden vertices in one place, making the code more maintainable and ensuring consistent behavior across different contractor types.include/cpp_common/ch_edge.hpp (3)
8-12: Updated copyright information correctly.The copyright section has been appropriately updated to include Aurélie Bousquet as a contributor to the function, with proper contact information.
51-52: Good addition of set_contracted_vertices method.This method provides a way to replace the entire set of contracted vertices associated with an edge, which is useful for managing shortcuts in the contraction hierarchies algorithm.
57-57: Good addition of add_contracted_vertices method.This method allows adding multiple contracted vertices at once to an edge, which is more efficient than adding them one by one. This is particularly useful when combining shortcuts during the contraction process.
sql/sigs/pgrouting--4.0.sig (1)
89-90: Correctly added function signatures for contraction hierarchies.The two new function signatures for both the internal (
_pgr_contraction_hierarchies) and public (pgr_contraction_hierarchies) API properly follow the pgRouting naming convention and parameter pattern. The function signatures are well-placed alphabetically in the file, maintaining the organization of the signature list.sql/contraction/_contraction_hierarchies.sql (2)
1-28: Header and License Information:The header section provides clear attribution and licensing details, including both the original developers and the function's developer. The GPL license text is properly included, ensuring legal clarity for distribution and modification.
29-36: Visual Separators & Version Annotation:The decorative comment lines (dashes and section labels) and the version marker (
--v4.0) help in visually segmenting the file and clarifying the version context. They enhance readability without affecting functionality.locale/pot/pgrouting_doc_strings.pot (6)
11-11: Validate the updated POT creation date.This line shows a future date (2025-02-28). Please confirm that this is intentional or automatically updated by build scripts.
2563-2572: Documentation segment: No issues found.The added references to
pgr_contraction_hierarchiesin this segment are clear and consistent with the theme of graph contraction.
2578-2587: Contraction functions: No apparent problems.The descriptions of the different contraction approaches (dead-end, linear, and hierarchies) are cohesive.
2797-2878: Detailed explanation looks good.The principles of contraction hierarchies are clearly explained, referencing node ordering, shortcuts, and the destructive nature of the process. No concerns identified.
4078-4081: References to pgr_contraction_hierarchies are consistent.All mentions align with the introduced function. No issues found.
8575-8634: Introduction of the new experimental function is well-documented.The explanation of the function’s scope, return format, and constraints (e.g., forbidden vertices) is thorough.
docqueries/contraction/contraction_hierarchies.result (1)
1-61: The example queries demonstrate proper usage of the new functionality.The file contains two example queries showing how to use the
pgr_contraction_hierarchiesfunction with both default parameters and with specified forbidden vertices. The queries and their results provide good examples of expected behavior.sql/contraction/contraction_hierarchies.sql (1)
42-60: The function signature and implementation look good.This correctly implements a SQL wrapper for the internal C++ implementation of contraction hierarchies. The parameter handling and function structure follow pgRouting conventions.
include/visitors/dijkstra_visitors.hpp (1)
55-82:❓ Verification inconclusive
Verify the max_dist_reached exception definition.
The new
dijkstra_max_distance_visitorclass throws amax_dist_reachedexception, but I don't see where this exception class is defined. It may be a subclass offound_goalsin the included header file.
🏁 Script executed:
#!/bin/bash # Check if max_dist_reached is defined in found_goals.hpp rg -A 5 "class max_dist_reached" include/visitors/found_goals.hppLength of output: 67
Attention: Missing Definition of
max_dist_reachedExceptionIt appears that the search for the
max_dist_reachedexception ininclude/visitors/found_goals.hpp(and in an extended repository scan) did not return any results. Please verify the following:
- Confirm if
max_dist_reachedis defined elsewhere in the codebase.- If it is intended to be a subclass of
found_goalsor another exception, ensure that its definition is correctly implemented and included where needed.- If it is missing, add the proper exception definition to avoid potential runtime issues.
include/contraction/deadEndContraction.hpp (3)
57-57: Good refactoring to use graph's forbidden vertices.The code now correctly retrieves forbidden vertices from the graph object rather than maintaining a separate copy within this class. This improves code maintainability.
89-92: Improved edge retrieval with const reference.Using a const reference for the edge variable prevents unnecessary copying.
106-108: Consistent use of graph's forbidden vertices check.This modification maintains consistency with the earlier changes, properly checking forbidden vertices through the graph object.
src/common/ch_edge.cpp (3)
36-39: Well-implemented setter method for contracted vertices.This new method provides a clean way to set the contracted vertices collection, which is essential for the contraction hierarchies algorithm.
77-79: Good implementation of contracted vertices addition.This method complements the existing contracted vertices management methods (
add_contracted_vertexandadd_contracted_edge_vertices) and follows the same pattern of using the+=operator.
11-12: Copyright notice correctly updated.The copyright information has been properly updated to include the new contributor.
pgtap/contraction/contraction_hierarchies/no_crash_test.pg (2)
31-49: Well-structured test function for basic functionality.The test function appropriately tests the new
pgr_contraction_hierarchiesfunction with theno_crash_testutility. This ensures the function doesn't crash with different parameters, which is a good basic test.
22-28: Good test data preparation.The test properly updates the edges table and prepares a meaningful test dataset to ensure the tests are effective.
pgtap/contraction/contraction_hierarchies/edge_cases/directed.pg (3)
39-43: Appropriate version check for a new function.The version check ensures the test only runs on PostgreSQL 4.0.0 or newer, which is correct if the function is new in that version.
45-65: Good test case for a simple directed graph.This test case validates the function's behavior with a minimal graph (two vertices connected by one edge), which is an important edge case to test.
67-195: Comprehensive test coverage for various directed graph scenarios.The test cases progress from simple to complex scenarios, including testing with forbidden vertices and subgraphs. This thorough approach helps ensure the function works correctly across different use cases.
pgtap/contraction/contraction_hierarchies/edge_cases/undirected.pg (3)
39-43: Appropriate version check for a new function.The version check ensures the test only runs on PostgreSQL 4.0.0 or newer, consistent with the directed graph tests.
45-65: Good test case for a simple undirected graph.This test case correctly validates the function's behavior with an undirected graph, using the
costparameter instead ofcost, reverse_cost.
67-188: Comprehensive test coverage for various undirected graph scenarios.The test cases mirror those in the directed test file but with appropriate adjustments for undirected graphs. This parallel structure ensures consistent testing across both graph types.
doc/contraction/pgr_contraction_hierarchies.rst (1)
202-212: Ensure Cross-Referencing "contraction-family" is Up-to-DateReferencing
:doc:\contraction-family`` is a good practice, but verify that the link leads to the most relevant or updated section. If there's additional or reorganized content in other files, adjust the link accordingly.src/contraction/contractGraph_driver.cpp (1)
81-82: Validate Retrieval of Modified Vertices & ShortcutsThese calls to
graph.get_modified_vertices()andgraph.get_shortcuts()appear correct. However, verify that upstream logic indeed populates these vectors, especially if new contraction logic has changed the definitions of “modified” or “shortcut” edges.src/contraction/contractionHierarchies_driver.cpp (3)
1-9: License & Headers Look GoodThe license terms and initial headers follow the standard for pgRouting source files.
47-70: Enforce Consistent Initialization & Error ChecksAll pointer and counting variables are asserted to be uninitialized before use. This is a good pattern; continue to maintain consistency by checking for null pointers (e.g.,
edges.empty()check).
106-135: Good Exception HandlingThe catch blocks are comprehensive, covering assertion failures, standard exceptions, and unknown exceptions, freeing resources consistently. This is aligned with robust error-handling practices.
doc/contraction/contraction-family.rst (6)
24-28: Documenting new experimental feature.These lines add a new experimental section for
pgr_contraction_hierarchies. The structure is consistent with the existing format of official/experimental markers, so no issues here.
34-34: New reference topgr_contraction_hierarchies.This reference in the ToC nicely integrates the new function into the contraction family docs. Everything looks good.
42-45: Clarity on contraction benefits.The added explanation succinctly conveys how contraction reduces the graph size. No concerns with correctness or clarity.
54-55: Highlighting new method.The line emphasizes that a third algorithm is the contraction hierarchies method. Great introduction.
57-60: Contraction features coverage.These lines explain key differences in user-configurable settings. No issues noted.
413-420: Introduction to contraction hierarchies.This new section clearly explains the hierarchy concept and how it relates to bidirectional searches. Excellent addition.
src/common/ch_vertex.cpp (3)
37-40: Constructor initialization.Initializing
vertex_orderandmetricto -1 is clear and consistent with typical sentinel values. Good practice.
68-70: Adhering to naming consistency.The method name
add_contracted_vertex_idis clear. No immediate concerns.
72-75: Appending multiple IDs in bulk.Method for adding multiple contracted vertices is straightforward. No issues found.
src/contraction/contractionHierarchies.c (3)
49-82:process()function design.Uses
pgr_SPI_connect()and measures runtime withclock(). The flow is clear, and error handling resets result arrays on failure. Looks correct.
84-115: Memory context handling on first call.The switch to
multi_call_memory_ctxis a standard approach in set-returning functions. Implementation aligns with best practices.
117-203: Constructing and returning tuples.The code carefully constructs the output array, sets up the tuple descriptor, and frees memory for the contracted vertices. No leaks or double-free issues are evident. Good job.
include/contraction/linearContraction.hpp (3)
106-108: Ensure no uses occur before initialization.
bool found_e, found_f;is assigned viaboost::tie, so it’s properly set. Just confirm no usage of these variables happens before thetiecalls.
113-116: Edge existence check looks solid.
Conditionally processing shortcuts only when both edges exist is a good practice. This prevents possible out-of-range operations.
130-130: Consider verifying edge existence for undirected paths.
You handle directed edges with explicit checks. If undirected edges might be missing, consider validating them before callingprocess_shortcut.include/cpp_common/to_postgres.hpp (2)
142-147: Check memory ownership for contracted vertices.
You allocate arrays viapgr_allocbut rely on downstream code to free them. Verify that this memory is properly released to avoid leaks.
159-178: Reevaluate memory handling for shortcut edges.
You use the same approach to allocatecontracted_verticesfor each shortcut. Apply the same diligence about freeing these buffers to avoid memory leaks.include/contraction/contractionHierarchies.hpp (3)
1-27: **License Header **These lines contain only the license and file header, so no functional code to review.
34-45: **Include Statements **The choice of headers (queue, functional, vector, limits, set, string, utility, boost) is appropriate for implementing Dijkstra and managing data structures used by contraction hierarchies.
237-321: **Effective orchestrator for Contraction Hierarchies **The overall flow in
contraction_hierarchies, including first labeling, queue-based lazy reordering, and final contraction, is a standard approach. The internal logging and priority queue checks appear consistent with typical usage. This implementation should be straightforward to maintain.include/contraction/contractionGraph.hpp (4)
39-43: **Priority queue type alias **Using the custom PQ type alias for storing metric-vertex pairs improves readability. No issues spotted.
90-96: **Implementation of forbidden vertices **The getter and setter for
forbiddenVerticesare straightforward and well-encapsulated, allowing the caller to manage which vertices remain uncontracted. This approach cleanly separates concerns.
346-354:add_shortcutreturn valueReturning a boolean indicating whether the insertion of a shortcut edge was successful is good for error handling. Ensure any caller checks this return value to handle failed insertions or duplicates.
536-537:min_edge_idas privateKeeping
min_edge_idprivate ensures correct usage of the ID generator. This is a solid encapsulation practice.
| COMMENT ON FUNCTION pgr_contraction_hierarchies(TEXT, BIGINT[], BOOLEAN) | ||
| IS 'pgr_contraction | ||
| - Parameters: | ||
| - Edges SQL with columns: id, source, target, cost [,reverse_cost] | ||
| - Optional Parameters | ||
| - forbidden_vertices := ARRAY[]::BIGINT[] | ||
| - directed := true | ||
| - Documentation: | ||
| - ${PROJECT_DOC_LINK}/pgr_contraction.html | ||
| '; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix documentation reference in comment.
The function comment references "pgr_contraction" instead of "pgr_contraction_hierarchies", and the documentation link points to "pgr_contraction.html".
COMMENT ON FUNCTION pgr_contraction_hierarchies(TEXT, BIGINT[], BOOLEAN)
-IS 'pgr_contraction
+IS 'pgr_contraction_hierarchies
- Parameters:
- Edges SQL with columns: id, source, target, cost [,reverse_cost]
- Optional Parameters
- forbidden_vertices := ARRAY[]::BIGINT[]
- directed := true
- Documentation:
- - ${PROJECT_DOC_LINK}/pgr_contraction.html
+ - ${PROJECT_DOC_LINK}/pgr_contraction_hierarchies.html
';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| COMMENT ON FUNCTION pgr_contraction_hierarchies(TEXT, BIGINT[], BOOLEAN) | |
| IS 'pgr_contraction | |
| - Parameters: | |
| - Edges SQL with columns: id, source, target, cost [,reverse_cost] | |
| - Optional Parameters | |
| - forbidden_vertices := ARRAY[]::BIGINT[] | |
| - directed := true | |
| - Documentation: | |
| - ${PROJECT_DOC_LINK}/pgr_contraction.html | |
| '; | |
| COMMENT ON FUNCTION pgr_contraction_hierarchies(TEXT, BIGINT[], BOOLEAN) | |
| IS 'pgr_contraction_hierarchies | |
| - Parameters: | |
| - Edges SQL with columns: id, source, target, cost [,reverse_cost] | |
| - Optional Parameters | |
| - forbidden_vertices := ARRAY[]::BIGINT[] | |
| - directed := true | |
| - Documentation: | |
| - ${PROJECT_DOC_LINK}/pgr_contraction_hierarchies.html | |
| '; |
| We finally show an example of contraction hierarchies building, on the same | ||
| sample graph as before, also with an undirected graph. | ||
|
|
||
| he following query shows the original data involved in the contraction |
There was a problem hiding this comment.
Fix minor typographical error.
Change “he following query...” to “The following query...”
- he following query shows the original data involved in the contraction
+ The following query shows the original data involved in the contraction📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| he following query shows the original data involved in the contraction | |
| The following query shows the original data involved in the contraction |
| @@ -0,0 +1,73 @@ | |||
| /*PGR-GNU***************************************************************** | |||
| File: contractGraph_driver.h | |||
There was a problem hiding this comment.
File name mismatch.
Line 2 indicates “File: contractGraph_driver.h,” but the actual filename is “contractionHierarchies_driver.h.” Please update to ensure consistency.
PR with squashed commits
Summary by CodeRabbit
New Features
Documentation
Tests