New signature pgr contraction #2789
Conversation
|
""" WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User Query
participant F as pgr_contraction()
participant I as _pgr_contraction()
alt Using new signature
U->>F: Call with (Edges SQL, directed, methods, cycles, forbidden)
else Using deprecated signature
U->>F: Call with (Edges SQL, contraction order, max_cycles, forbidden_vertices, directed)
F->>LOG: RAISE NOTICE (Deprecation Warning)
end
F->>I: Forward parameters to internal function
I-->>F: Return contracted graph data
F-->>U: Return results (type, id, contracted_vertices, source, target, cost)
Assessment against linked issues
Possibly related issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 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 (
|
WalkthroughThis pull request updates the interface and implementation of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant pgr_func as pgr_contraction()
participant Processor as Graph Contraction Logic
Client->>pgr_func: Call with Edges SQL, [directed, methods, cycles, forbidden]
pgr_func->>Processor: Apply contraction methods (Dead End, Linear)
Processor-->>pgr_func: Return contracted graph data
pgr_func->>Client: Send result (modified vertices, new edges)
Assessment against linked issues
Possibly related issues
Poem
🪧 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 (
|
WalkthroughThe changes update the signature and behavior of the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Tester
participant F as pgr_contraction Function
participant I as _pgr_contraction Internal Function
U->>F: Call pgr_contraction(Edges SQL, options)
F->>F: Validate parameters (directed, methods, cycles, forbidden)
alt New signature
F->>I: Call _pgr_contraction with new parameters
else Deprecated signature
F->>F: Emit deprecation notice
F->>I: Call legacy contraction process
end
I-->>F: Return contraction changes
F-->>U: Return updated result set
Assessment against linked issues
Possibly related issues
Possibly related PRs
Poem
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
pgtap/contraction/contraction/edge_cases/remaining_tests.pg (1)
141-239: 🛠️ Refactor suggestionThe test suite lacks coverage for the new signature.
While the tests thoroughly cover the original (now deprecated) signature of pgr_contraction, I don't see tests specifically for the new signature format with the optional 'methods' parameter and the new parameter order.
Consider adding test cases that verify the new function signature works correctly:
-- Test new signature format PREPARE q_new_sig AS SELECT * FROM pgr_contraction( 'SELECT id, source, target, cost, reverse_cost FROM edges', directed := true, methods := ARRAY[1]::integer[]); -- Test new signature with all parameters PREPARE q_new_sig_all_params AS SELECT * FROM pgr_contraction( 'SELECT id, source, target, cost, reverse_cost FROM edges', directed := false, methods := ARRAY[1]::integer[], max_cycles := 1, forbidden_vertices := ARRAY[]::bigint[]); SELECT lives_ok('q_new_sig'); SELECT lives_ok('q_new_sig_all_params');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (26)
NEWS.md(1 hunks)doc/contraction/contraction-family.rst(1 hunks)doc/contraction/pgr_contraction.rst(5 hunks)doc/src/release_notes.rst(1 hunks)docqueries/contraction/CMakeLists.txt(0 hunks)docqueries/contraction/contraction-family.pg(0 hunks)docqueries/contraction/contraction-family.result(0 hunks)docqueries/contraction/contraction.pg(1 hunks)docqueries/contraction/contraction.result(3 hunks)docqueries/contraction/test.conf(0 hunks)locale/en/LC_MESSAGES/pgrouting_doc_strings.po(9 hunks)locale/pot/pgrouting_doc_strings.pot(9 hunks)pgtap/contraction/contraction/edge_cases/compare_dijkstra_combined.pg(1 hunks)pgtap/contraction/contraction/edge_cases/compare_dijkstra_deadend.pg(1 hunks)pgtap/contraction/contraction/edge_cases/compare_dijkstra_linear.pg(1 hunks)pgtap/contraction/contraction/edge_cases/edge_cases.pg(1 hunks)pgtap/contraction/contraction/edge_cases/edge_cases_deadend.pg(1 hunks)pgtap/contraction/contraction/edge_cases/edge_cases_linear.pg(1 hunks)pgtap/contraction/contraction/edge_cases/linear/directed/edge_cases.pg(1 hunks)pgtap/contraction/contraction/edge_cases/remaining_tests.pg(1 hunks)pgtap/contraction/contraction/inner_query.pg(1 hunks)pgtap/contraction/contraction/no_crash_test.pg(2 hunks)pgtap/contraction/contraction/types_check.pg(1 hunks)sql/contraction/contraction.sql(2 hunks)sql/sigs/pgrouting--3.8.sig(1 hunks)tools/testers/contraction_tapfuncs.sql(1 hunks)
💤 Files with no reviewable changes (4)
- docqueries/contraction/test.conf
- docqueries/contraction/CMakeLists.txt
- docqueries/contraction/contraction-family.result
- docqueries/contraction/contraction-family.pg
🧰 Additional context used
🧠 Learnings (2)
sql/sigs/pgrouting--3.8.sig (1)
Learnt from: cvvergara
PR: pgRouting/pgrouting#2764
File: sql/sigs/pgrouting--3.8.sig:125-136
Timestamp: 2025-03-18T01:02:51.111Z
Learning: The file sql/sigs/pgrouting--*.sig is automatically generated by a command and cannot be manually modified. Comments about naming conventions or other improvements should be directed at the source code that generates these signatures, not at the signature file itself.
locale/en/LC_MESSAGES/pgrouting_doc_strings.po (2)
undefined
<retrieved_learning>
Learnt from: cvvergara
PR: #2764
File: locale/en/LC_MESSAGES/pgrouting_doc_strings.po:0-0
Timestamp: 2025-03-18T01:02:51.111Z
Learning: Modifications to locale files in the pgRouting project should be handled through an action commit or through the Weblate translation platform, not via direct edits.
</retrieved_learning>
<retrieved_learning>
Learnt from: cvvergara
PR: #2764
File: locale/en/LC_MESSAGES/pgrouting_doc_strings.po:2740-2740
Timestamp: 2025-03-18T01:02:51.111Z
Learning: Modifications to locale files in pgRouting (like .po files) are done through action commits or in weblate, not through direct manual edits in PRs.
</retrieved_learning>
🪛 markdownlint-cli2 (0.17.2)
NEWS.md
30-30: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
40-40: Multiple consecutive blank lines
Expected: 1; Actual: 2
(MD012, no-multiple-blanks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Ubuntu Boost (77)
- GitHub Check: Check queries
- GitHub Check: macos (macos-14)
- GitHub Check: macos (macos-latest)
- GitHub Check: Ubuntu Boost (84)
- GitHub Check: Ubuntu Boost (83)
- GitHub Check: Ubuntu Boost (76)
- GitHub Check: Ubuntu Boost (86)
- GitHub Check: Ubuntu Boost (75)
- GitHub Check: Ubuntu Boost (79)
- GitHub Check: Ubuntu Boost (78)
- GitHub Check: Ubuntu Boost (56)
- GitHub Check: Ubuntu Boost (80)
- GitHub Check: Ubuntu clang
- GitHub Check: Ubuntu Boost (68)
🔇 Additional comments (91)
locale/pot/pgrouting_doc_strings.pot (10)
11-11: POT-Creation-Date updated correctly.The POT-Creation-Date has been correctly updated to reflect when this translation template was generated.
2728-2728: Spelling correction from "symetrical" to "symmetrical".Good catch on correcting the spelling from "symetrical" to "symmetrical" for better accuracy in the translation strings.
3754-3770: New signature documentation properly captures API changes.This section properly documents the API changes for pgr_contraction, aligning with issue #2786:
- Previously mandatory parameter "Contraction order" is now optional and renamed to "methods"
- New name and order for optional parameters
- Appropriate deprecation notice for the old signature
The documentation aligns with the PR objectives of introducing a new signature and deprecating the previous one.
8017-8018: Version information properly added.The version information is correctly added to indicate when this function's changes are being introduced.
8029-8060: Clear explanation of function behavior and return values.This section provides a clear explanation of:
- What the function does and doesn't return
- The types of values returned (new edges and modified vertices)
- How the returned values are ordered
- The types of contraction methods included
The documentation is comprehensive and will be helpful for translators to understand the context.
8062-8084: New function signature and options properly documented.The new function signature with optional parameters is clearly documented:
- Updated signature format showing options are now optional
- Clear specification of all available options
- Default behavior is properly described
- The "methods" parameter documentation shows the new optionality
This aligns with the PR objectives of introducing a new signature for the contraction functionality.
8095-8114: Optional parameters well documented.The optional parameters are clearly documented with their:
- Parameter names
- Data types
- Default values (where applicable)
- Purpose descriptions
This provides translators with sufficient context to accurately translate these strings.
8122-8134: Return value type descriptions are clear and consistent.The descriptions of the return values are clear and consistent, explaining:
- The meaning of the "type" column values
- How to interpret positive vs. negative IDs
- The distinctness of values in the column
This ensures translators understand the technical context of these strings.
8155-8156: Contracted_vertices field properly labeled.The "contracted_vertices" field is properly labeled for translation.
8179-8319:✅ Verification successful
Extensive documentation removal requires verification.
A substantial amount of documentation has been removed, including sections on:
- The cycle explanation
- Sample data examples
- Graph construction in the database
- Storage of contraction information
- Usage of the contracted graph
While simplifying documentation can be beneficial, please verify that this information:
- Has been moved elsewhere in the documentation (rather than completely removed)
- Is no longer relevant due to the API changes
- Won't negatively impact users' understanding of how to use the function
🏁 Script executed:
#!/bin/bash # Check if this content exists elsewhere in the documentation # Look for key phrases from the removed content in other documentation files echo "Searching for related content in other documentation files..." rg -i "contracting a graph, can be done with more than one operation" --type rst rg -i "the order of the operations affect the resulting contracted graph" --type rst rg -i "using the contracted graph" --type rst rg -i "construction of the graph in the database" --type rst # Check if there are any new files that might contain this information echo "Checking for new documentation files related to contraction..." fd -e rst -e md contractionLength of output: 1125
Documentation Migration Verified
The removed documentation—in particular, the explanations on graph contraction, cycle operations, graph construction in the database, and usage of the contracted graph—has been successfully relocated. Our verification shows that:
- Key phrases (e.g., “Contracting a graph, can be done with more than one operation”, “the order of the operations affect the resulting contracted graph”, “using the contracted graph”, “construction of the graph in the database”) now reside in doc/contraction/pgr_contraction.rst.
- Additional documentation related to contraction is present in doc/contraction/contraction-family.rst.
Based on these findings, the documentation removals from
locale/pot/pgrouting_doc_strings.potappear appropriate. Please ensure that all internal links and references point to the new documentation locations.sql/sigs/pgrouting--3.8.sig (1)
91-91: The new signature for pgr_contraction looks good.This addition implements the new function signature with optional parameters as described in the PR objectives. The previous signature on line 90 is kept for backward compatibility, which is a good practice for API changes.
pgtap/contraction/contraction/edge_cases/remaining_tests.pg (1)
22-22: Test count adjustment is appropriate.The change from 49 to 48 planned tests likely reflects the removal of a test that was checking parameters that have been changed in the new function signature.
doc/src/release_notes.rst (1)
62-69: Well-structured documentation update for the pgr_contraction function.The changes appropriately add pgr_contraction to the "Official functions changes" section of the 3.8.0 release notes, following the same format as other function updates. The reference to issue #2786 provides good traceability to the underlying changes.
pgtap/contraction/contraction/no_crash_test.pg (2)
22-22: Good version-aware test planning.The conditional plan count ensures the appropriate number of tests are executed based on the PostgreSQL version, which is necessary given the function signature changes in 3.8.0.
48-62: Well-implemented version compatibility for the new function signature.The conditional block correctly handles version-specific testing by:
- Skipping tests for the new signature on older versions
- Creating appropriate test parameters for the new signature on 3.8.0+
- Executing the tests with the updated parameters
This approach maintains backward compatibility while thoroughly testing the new function signature.
doc/contraction/contraction-family.rst (1)
296-296: Fixed typo: "symmetrical" instead of "symetrical".Good correction that improves documentation accuracy.
pgtap/contraction/contraction/types_check.pg (3)
21-21: Good version-aware test planning.The conditional plan count properly adjusts the number of tests based on PostgreSQL version, accounting for the different checks needed for the old and new function signatures.
23-60: Well-structured function for version-specific type checking.The
types_check()function elegantly handles:
- Testing the existence and return type of the function for 3.8.0+
- Verifying the updated parameter names and types in 3.8.0+
- Maintaining checks for the older parameter structure in previous versions
This encapsulation makes the test more maintainable and clearly shows the differences between versions.
65-65: Good refactoring to use the new types_check function.Calling the encapsulated function improves code organization and makes the test flow more logical.
pgtap/contraction/contraction/inner_query.pg (6)
1-3: Copyright year is set to 2025The copyright year is set to 2025, which is future-dated compared to the current year (2024). While this may be intentional to cover future updates, consider updating it to reflect the current year when the code was written.
22-22: Conditional test planning based on PostgreSQL versionGood practice to conditionally plan tests based on version compatibility, ensuring older PostgreSQL versions skip irrelevant tests rather than failing them.
29-32: Version compatibility checkGood implementation of version checking, skipping tests if version requirement isn't met. This ensures backward compatibility while allowing testing of new features.
34-40: Comprehensive signature testingExcellent test coverage of various parameter combinations for the new
pgr_contractionsignature. This ensures all valid parameter combinations work as expected.
42-59: Thorough error condition testing for methods parameterComprehensive testing of error conditions related to the methods parameter, including type checking and empty array validation.
61-96: Complete validation of forbidden parameter typesThorough testing of various data types for the forbidden parameter, checking both valid and invalid types. This ensures proper type checking in the function.
pgtap/contraction/contraction/edge_cases/compare_dijkstra_deadend.pg (5)
1-3: Copyright year is set to 2025The copyright year is set to 2025, which is future-dated compared to the current year (2024). While this may be intentional to cover future updates, consider updating it to reflect the current year when the code was written.
24-25: Table alterations to support testingGood approach adding necessary columns to the tables instead of creating new tables. This makes the test setup more maintainable.
27-95: Well-structured test functionThe
check_cgfunction is well-designed with clear separation between different test scenarios based on the input parameter. The expected values are defined explicitly which makes the tests self-documenting.
101-107: Test execution for both directed and undirected graphsGood practice testing both directed (true) and undirected (false) scenarios. This ensures the contraction works correctly regardless of graph directionality.
21-21:❓ Verification inconclusive
Large number of planned tests
The test plans for 654 tests when PostgreSQL version is at least 3.8.0. This is a large number - verify that all these tests are necessary and that they run in a reasonable time.
🏁 Script executed:
#!/bin/bash # Check execution time of this test file time psql -c "BEGIN; SELECT * FROM pgtap.runtests('pgtap/contraction/contraction/edge_cases/compare_dijkstra_deadend'); ROLLBACK;"Length of output: 270
Subject: Verify Execution Environment and Necessity of 654 Planned Tests
It appears that the runtime check for the test file failed due to the
psqlcommand not being available. This prevents a proper assessment of whether the 654 planned tests (for PostgreSQL version ≥3.8.0) are indeed necessary and run in a reasonable time. Please ensure that:
- The PostgreSQL CLI (
psql) is installed and accessible in the testing environment.- You manually verify the test execution time and test relevance once
psqlis available.Thanks for addressing these points.
pgtap/contraction/contraction/edge_cases/compare_dijkstra_linear.pg (3)
1-3: Copyright year is set to 2025The copyright year is set to 2025, which is future-dated compared to the current year (2024). While this may be intentional to cover future updates, consider updating it to reflect the current year when the code was written.
116-122: Testing linear contractionExcellent approach testing linear contraction separately with
ARRAY[2]as the methods parameter. This ensures that each contraction method works correctly in isolation.
27-111: Detailed verification of contracted graph structureThe function thoroughly verifies both the structure (edges, vertices) and the numeric values (costs) of the contracted graph, ensuring the contraction algorithm produces the expected results.
pgtap/contraction/contraction/edge_cases/compare_dijkstra_combined.pg (3)
1-3: Copyright year is set to 2025The copyright year is set to 2025, which is future-dated compared to the current year (2024). While this may be intentional to cover future updates, consider updating it to reflect the current year when the code was written.
113-119: Testing combined contraction methodsExcellent approach testing the combination of contraction methods with
ARRAY[1,2]. This ensures that multiple contraction methods work correctly when applied together.
38-106: Different expected graphs based on directionalityThe function correctly maintains different expected graph structures for directed vs. undirected graphs, ensuring the contraction behaves correctly in both scenarios.
pgtap/contraction/contraction/edge_cases/linear/directed/edge_cases.pg (2)
77-78: Confirm test coverage.
By removing theidcolumn in this SELECT, ensure that no subsequent code or checks rely on the now-missing field. This helps avoid unexpected test failures or mismatches in result comparisons.
83-87: Check consistent references to columns.
Removal ofidfrom both the query and the VALUES block looks aligned with the revised function output. Verify that the rest of the test logic or any set_eq calls no longer referenceid.doc/contraction/pgr_contraction.rst (7)
11-11: Minor doc index addition.
No issues found. The added index entry aligns with the references to pgr_contraction in this documentation.
25-34: Docs align with new function signature.
These lines accurately introduce version 3.8.0 updates and clarify thatmethodsreplaces the oldcontraction_orderparameter.
35-45: Clarification of older version changes.
Documenting the removal of theseqcolumn and the name change frompgr_contractGraphhelps maintain clarity on the function's evolution.
50-51: Updated doc directives.
These lines refine the doc structure around the function characteristics. No functional concerns.
59-79: Detailed doc on returned values.
The explanation of ordering between vertices and edges, along with partial-graph return clarifications, is consistent with the revised function.
129-152: New optional parameters.
The table neatly describesmethods,cycles, andforbidden. Helps users understand the new usage and defaults.
224-225: Local contents references.
The added subsection references are consistent with the updated doc structure.sql/contraction/contraction.sql (5)
30-30: No functional change for this comment line.
Documenting version 3.8 is informative.
34-36: New parameters for pgr_contraction.
Definingmethods,cycles, andforbiddenwith defaults aligns well with the doc updates.
46-47: Use of _pgr_contraction.
Querying_pgr_contractionwith the updated parameters (methods, cycles, forbidden, directed) is consistent.
51-51: Updated COMMENT block.
Reflects the new signature and the revised parameter descriptions.
64-93: Backward-compatibility approach.
Providing a deprecated function with a RAISE NOTICE is a good strategy to ease migration. It preserves existing usage while encouraging the latest signature.docqueries/contraction/contraction.pg (11)
5-5: Explicit directed parameter.
Usingfalsehere makes it clear the contraction runs on an undirected scenario. This is consistent with the optional boolean param in the new signature.
9-9: Named parameter usage.
Specifyingmethods => ARRAY[1]is cleaner and clearer than positional array arguments.
13-13: Different method array.
Invokingmethods => ARRAY[2]tests the linear-only contraction path. Good coverage.
16-22: Schema update.
Addingis_contractedandcontracted_verticescolumns tovertices, plusis_newandcontracted_verticestoedges, provides the necessary fields for storing contraction results.
24-25: Another pgr_contraction invocation.
No issues. Shows consistent usage with a boolean second parameter.
27-29: Results to table.
Capturing contraction data intocontraction_resultsis beneficial for subsequent analysis and updates.
31-48: Vertex and edge updates.
Marking vertices as contracted and creating new edges from thepgr_contractionoutput is logically sound. Confirm references to original IDs remain valid.
52-59: Filtering edges for the contracted graph.
The subquery ensures only edges connecting non-contracted vertices remain. This logic is correct for building partial graphs.
61-67: Creating contracted_graph view.
Properly excludes edges with contracted endpoints. Straightforward approach for a live view of the updated structure.
69-84: path_cost function.
Good example of selectively merging newly formed edges and original edges for path calculations. This ensures correct routing over the contracted graph.
99-103: Test calls to path_cost.
Callingpath_cost(10, 12),(15, 12), and(15, 1)ensures coverage across multiple scenarios.pgtap/contraction/contraction/edge_cases/edge_cases.pg (4)
1-18: License notice is properly included
These lines provide the standard GNU header. No issues identified.
64-196: Comprehensive sample-data checks
These repeatedRETURN QUERYstatements appear consistent with the design to test various edge forbiddance parameters. No syntax or logical errors noted.
240-274: Transactional flow for tests
The approach of callingSELECT edge_cases(); SELECT finish(); ROLLBACK;is consistent with typical pgTap usage. No issues noted.
19-23:❓ Verification inconclusive
Verify normalization of edge costs
Usingsign(cost)andsign(reverse_cost)will collapse all cost values to -1, 0, or 1. Ensure that this is intentional for your test coverage.Would you like to confirm via a quick script that this normalization doesn't omit relevant test cases for intermediate cost values?
🏁 Script executed:
#!/bin/bash # Searching references to edges to see if sign(...) usage might skip other cost-based tests rg 'edges' -A 5Length of output: 67104
Action: Verify That Edge Cost Normalization Is Intentional
The update usessign(cost)andsign(reverse_cost)in the edge updates, which limits all cost values to only -1, 0, or 1. Based on our repository search, we couldn’t identify clear test cases exercising a range of intermediate cost values. Please confirm that this normalization strategy is intentional for the test coverage and that no important scenarios with intermediate costs are being skipped.
• Verify that tests expecting detailed cost information (beyond just the sign) are either updated or intentionally omitted.
• Reassure that the behavior inpgtap/contraction/contraction/edge_cases/edge_cases.pgaligns with the overall test strategy.pgtap/contraction/contraction/edge_cases/edge_cases_deadend.pg (6)
1-18: License header is properly maintained
No concerns with the GNU license block.
19-23: Revalidate edge cost normalization
As in the previous file, updatingcostandreverse_costusingsign()might omit meaningful non-unit cost tests. Confirm it’s intentional.
24-63: Creation of the 'graphs' table
No explicit indexing ondead_caseor other columns, but likely sufficient for test usage. No major performance concerns for small data.
64-119:resultstable insertion
Data consistency looks fine for storing test outcomes. Ensure eachresult_caseis documented to avoid confusion.
120-219: Edge case checks
edge_cases_sampledata(directed text)robustly tests different forbidden vertex configurations. The flow is logical and uses prepared statements effectively.
247-321: Use of rollback and finish
Concluding withSELECT finish();andROLLBACK;is standard in pgTap scripts. Implementation looks consistent with the intended test flow.pgtap/contraction/contraction/edge_cases/edge_cases_linear.pg (8)
1-18: GNU license header included
No concerns or improvements needed for standard licensing.
19-23: Confirm test plan count matches actual tests
The usage ofplan(112)undermin_version('3.8.0')should reflect the total number of expected tests. Verify alignment with actual test coverage.
24-73: Graph definitions for linear edge cases
Creatinggraphstable and populating test data is coherent. Likely sufficient for smaller data sets. No issues.
74-119: Expanded 'results' table usage
The table is used to verify edges, costs, and contracted vertices. Looks structurally consistent.
120-219: Additional edge cases inedge_cases_sampledata
Queries forset_eqandis_emptyare well-structured. The scattering of forbid checks is thorough.
224-267: Range-based tests inedge_cases(directed text)
Properly checks emptiness vs. non-emptiness of generated graphs. The direct calls topgr_contractionwithformatare valid.
284-290: Prepared statements
graph1&graphstatements use param arrays for cost & forbidden vertices. Declaration is consistent across the codebase.
301-305: Wrap-up withSELECT finish()andROLLBACK
Standard approach again for tests. No issues found.tools/testers/contraction_tapfuncs.sql (6)
34-49: Verify dynamic SQL usage to avoid injection.
The procedure dynamically executes SQL usingEXECUTE format(...)with thefnparameter. Iffncan be user-supplied, there is a potential security risk of SQL injection. Ensure you only pass trusted function names or properly sanitize the parameter (e.g., usingquote_ident).
25-30: Version check approach looks good.
TheIF NOT min_version('3.8.0') THEN RETURN; END IF;guard is a straightforward and clear way to ensure compatibility. Resetting the edges and vertices here is also consistent with cleaning up test data.
51-75: Confirm consistency when re-inserting edges.
You're inserting new edges fromcontraction_infoafter clearing high IDs. This is suitable in a controlled test environment. However, if concurrently used, check for potential conflicts with existing IDs or concurrency issues.
80-106: Dynamic building of the Dijkstra query is safe for numeric parameters.
Since$1and$2are typed asBIGINT, this string concatenation won't be prone to injection. The function is otherwise straightforward.
107-128: Comparison logic is clear.
The function callspath_cost_contractionand compares it to a directpgr_dijkstraquery. This is a good approach to validate that contraction results align with a baseline.
155-265: Functionality to up/down-shift edge IDs is well-organized.
The code comprehensively modifies IDs, runs tests, and reverts back. This is an effective approach for verifying ID manipulations.docqueries/contraction/contraction.result (6)
7-7: Double-check new boolean parameter usage.
You're callingpgr_contraction(..., false)which aligns with the new signature. Just confirm that this correctly sets the “directed” or “methods” argument as intended.
22-22: Good demonstration of themethods => ARRAY[...]usage.
Changing the parameter to accept an integer array for contraction methods is consistent with the new signature.
35-35: Expanding testing with a different methods array.
Usingmethods => ARRAY[2]further verifies that multiple configurations work.
43-51: Adding columns to track contraction status.
New columnsis_contractedandcontracted_verticeson both tables are a clear step toward better introspection of the contraction process.
67-113: Steps to store and apply contraction results are coherent.
Selecting contraction results intocontraction_results, then updatingverticesand inserting new edges, logically follows from the new approach.
160-190:path_costfunction effectively combines contracted_graph with expansions.
The approach of unioning contracted edges with expansions extends the query to handle partial contractions elegantly.
There was a problem hiding this comment.
Actionable comments posted: 21
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (26)
NEWS.md(1 hunks)doc/contraction/contraction-family.rst(1 hunks)doc/contraction/pgr_contraction.rst(5 hunks)doc/src/release_notes.rst(1 hunks)docqueries/contraction/CMakeLists.txt(0 hunks)docqueries/contraction/contraction-family.pg(0 hunks)docqueries/contraction/contraction-family.result(0 hunks)docqueries/contraction/contraction.pg(1 hunks)docqueries/contraction/contraction.result(3 hunks)docqueries/contraction/test.conf(0 hunks)locale/en/LC_MESSAGES/pgrouting_doc_strings.po(9 hunks)locale/pot/pgrouting_doc_strings.pot(9 hunks)pgtap/contraction/contraction/edge_cases/compare_dijkstra_combined.pg(1 hunks)pgtap/contraction/contraction/edge_cases/compare_dijkstra_deadend.pg(1 hunks)pgtap/contraction/contraction/edge_cases/compare_dijkstra_linear.pg(1 hunks)pgtap/contraction/contraction/edge_cases/edge_cases.pg(1 hunks)pgtap/contraction/contraction/edge_cases/edge_cases_deadend.pg(1 hunks)pgtap/contraction/contraction/edge_cases/edge_cases_linear.pg(1 hunks)pgtap/contraction/contraction/edge_cases/linear/directed/edge_cases.pg(1 hunks)pgtap/contraction/contraction/edge_cases/remaining_tests.pg(1 hunks)pgtap/contraction/contraction/inner_query.pg(1 hunks)pgtap/contraction/contraction/no_crash_test.pg(2 hunks)pgtap/contraction/contraction/types_check.pg(1 hunks)sql/contraction/contraction.sql(2 hunks)sql/sigs/pgrouting--3.8.sig(1 hunks)tools/testers/contraction_tapfuncs.sql(1 hunks)
💤 Files with no reviewable changes (4)
- docqueries/contraction/CMakeLists.txt
- docqueries/contraction/test.conf
- docqueries/contraction/contraction-family.result
- docqueries/contraction/contraction-family.pg
🧰 Additional context used
🧠 Learnings (1)
sql/sigs/pgrouting--3.8.sig (1)
Learnt from: cvvergara
PR: pgRouting/pgrouting#2764
File: sql/sigs/pgrouting--3.8.sig:125-136
Timestamp: 2025-03-18T01:02:51.111Z
Learning: The file sql/sigs/pgrouting--*.sig is automatically generated by a command and cannot be manually modified. Comments about naming conventions or other improvements should be directed at the source code that generates these signatures, not at the signature file itself.
🪛 markdownlint-cli2 (0.17.2)
NEWS.md
30-30: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
40-40: Multiple consecutive blank lines
Expected: 1; Actual: 2
(MD012, no-multiple-blanks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Ubuntu Boost (77)
- GitHub Check: Check queries
- GitHub Check: macos (macos-14)
- GitHub Check: macos (macos-latest)
- GitHub Check: Ubuntu Boost (84)
- GitHub Check: Ubuntu Boost (83)
- GitHub Check: Ubuntu Boost (76)
- GitHub Check: Ubuntu Boost (86)
- GitHub Check: Ubuntu Boost (75)
- GitHub Check: Ubuntu Boost (79)
- GitHub Check: Ubuntu Boost (78)
- GitHub Check: Ubuntu Boost (56)
- GitHub Check: Ubuntu Boost (80)
- GitHub Check: Ubuntu clang
- GitHub Check: Ubuntu Boost (68)
🔇 Additional comments (130)
sql/sigs/pgrouting--3.8.sig (1)
91-91: New signature for pgr_contraction looks good.The addition of the new function signature
pgr_contraction(text,boolean,integer[],integer,bigint[])aligns with the PR objective to make the previously compulsory "Contraction order" parameter (now called "methods") optional and reorder the parameters.doc/src/release_notes.rst (1)
62-69: Documentation update for pgr_contraction looks complete.The changes properly document the updates to the pgr_contraction function under the "Official functions changes" section, with a reference to issue #2786 and inclusion of the appropriate sections from the function's documentation file.
pgtap/contraction/contraction/edge_cases/remaining_tests.pg (1)
22-22: Test plan count updated correctly.The test plan count has been reduced from 49 to 48, which aligns with the removal of a parameter test block mentioned in the AI summary. This change correctly maintains the test plan's integrity.
pgtap/contraction/contraction/no_crash_test.pg (2)
22-22: Version-specific test planning implemented.The test plan now correctly adapts based on the version, running 11 tests for version 3.8.0+ and 8 tests for older versions. This ensures backward compatibility while supporting the new function signature.
48-62: Proper version checking for new function signature tests.This conditional block correctly handles the new pgr_contraction signature introduced in version 3.8.0. For older versions, tests are appropriately skipped with a clear message, while newer versions get proper testing with the updated parameter structure.
doc/contraction/contraction-family.rst (1)
296-296: Corrected spelling error.Fixed the spelling of "symmetrical" which improves documentation accuracy.
locale/pot/pgrouting_doc_strings.pot (6)
11-11: No issues found.
The updated creation date is appropriate and doesn't seem to cause any issues.
3754-3771: Documentation changes look good.
The new references to the deprecation of the old signature and the introduction of themethodsparameter align well with the PR objectives.
8017-8019: Version references are consistent.
The “Version 3.8.0” label matches the Project-Id-Version metadata. No concerns.
8095-8114: No issues found with the optional parameters.
The newcyclesandforbiddenparameters are explained clearly.
8122-8134: Documentation of ID usage is clear.
This explanation ensures that users can distinguish between vertices and edges based on positive and negative IDs.
8155-8157: Clear definition of contracted vertices.
Remains consistent with the usage ofis_contractedin the subsequent section where the data is updated.pgtap/contraction/contraction/inner_query.pg (5)
22-22: LGTM: Good test planning approach.The conditional test planning based on PostgreSQL version is a good practice. It ensures that tests are skipped gracefully on unsupported versions.
24-32: LGTM: Clean version check implementation.The function correctly checks for the minimum required PostgreSQL version before executing the tests. The early return with a skip message is a good pattern for version-specific tests.
34-41: LGTM: Comprehensive test coverage for pgr_contraction.The tests cover various parameter combinations for the new signature of pgr_contraction, which aligns with the PR objectives of introducing a new signature.
42-59: LGTM: Thorough testing of error cases.The tests properly validate error cases for invalid inputs, including incorrect array types and empty arrays. The error messages and codes are checked against expected values.
61-96: LGTM: Good coverage of edge cases and type validations.The tests thoroughly check both error and success cases for the
forbiddenparameter with different array types. This provides good coverage for testing type validation and compatibility.pgtap/contraction/contraction/edge_cases/compare_dijkstra_deadend.pg (5)
21-22: LGTM: Good conditional test planning and data setup.The test planning based on PostgreSQL version and the edge cost updates are well implemented to prepare the test environment.
24-25: LGTM: Good schema extensions for tracking contracted states.Adding these boolean and array columns to track contracted vertices and new edges is a clean approach to validate the contraction algorithm's behavior.
27-35: LGTM: Clean version check implementation.The function correctly checks for the minimum required PostgreSQL version before executing the tests, which aligns with the PR's focus on introducing a new signature in version 3.8.0.
37-65: LGTM: Comprehensive validation for the true parameter case.The function thoroughly validates both the contraction information and the resulting contracted graph structure when directed=true. The expected values are well-defined and the validation is comprehensive.
101-107: LGTM: Thorough test execution for both directed and undirected cases.The test appropriately creates contracted graphs with both true and false parameters for the directed flag, validating the behavior in both scenarios. This provides good coverage of the functionality.
locale/en/LC_MESSAGES/pgrouting_doc_strings.po (9)
2973-2973: Verify the usage of “linearity is not symmetrical.”
Double-check that the statement aligns with the intended meaning in this context, especially if referencing undirected graphs where symmetry could be assumed.
4192-4215: Confirm the partial option removal references.
Ensure that the mention of removing the “partial” option is consistent with the actual implementation, and confirm that referencing issue #2786 is correct.
9343-9345: Validate version references.
You mention “Version 3.8.0.” Confirm that all newly added references and doc updates are properly aligned with this version.
9359-9416: Documentation clarity on contraction output.
Stating that the function returns only the changes to the graph provides great clarity for users. This section is well-articulated.
9427-9445: Accurate descriptions for 'cycles' and 'forbidden.'
The new parameters are clearly documented, ensuring proper guidance for usage.
9454-9466: Clear explanation of row types.
Describing the row type as 'v' or 'e' and clarifying positive vs. negative IDs is a good addition.
9489-9491: Useful guidance on contracted_vertices.
Providing explicit mention of the “Array of contracted vertex identifiers” makes the documentation more intuitive.
9519-9688: Thorough step-by-step documentation for contracted graphs.
Great job illustrating the multi-cycle approach and explaining how to handle edge and vertex cases. This will help users implement the feature efficiently.
11-11: 🧹 Nitpick (assertive)Consider updating the PO-Revision-Date.
The POT creation date was updated, but the PO-Revision-Date might also need to be refreshed to reflect the most recent translation changes.⛔ Skipped due to learnings
Learnt from: cvvergara PR: pgRouting/pgrouting#2764 File: locale/en/LC_MESSAGES/pgrouting_doc_strings.po:0-0 Timestamp: 2025-03-18T01:02:51.111Z Learning: Modifications to locale files in the pgRouting project should be handled through an action commit or through the Weblate translation platform, not via direct edits.Learnt from: cvvergara PR: pgRouting/pgrouting#2764 File: locale/en/LC_MESSAGES/pgrouting_doc_strings.po:2740-2740 Timestamp: 2025-03-18T01:02:51.111Z Learning: Modifications to locale files in pgRouting (like .po files) are done through action commits or in weblate, not through direct manual edits in PRs.Learnt from: cvvergara PR: pgRouting/pgrouting#2771 File: locale/en/LC_MESSAGES/pgrouting_doc_strings.po:4407-4419 Timestamp: 2025-03-18T01:02:51.111Z Learning: In the pgRouting project, locale files (.po files) should not be manually edited as they are managed through automated localization tools.pgtap/contraction/contraction/types_check.pg (4)
21-21: Validate test plan count for version-based conditional.
The logic of choosing betweenplan(7)andplan(5)based onmin_version('3.8.0')is sound; however, please confirm that these plan counts reflect the actual number of tests being run in each code path, to avoid potential mismatches between planned and executed tests.Could you run all tests locally for both versions (above and below
3.8.0) to confirm the plan count is correct?
23-43: Centralize function existence and signature checks.
Encapsulating these checks intotypes_check()is a neat approach. It improves maintainability and helps ensure consistent version-based logic. The new checks for argument names, types, and return type match the updatedpgr_contractionsignature.
45-55: Confirm backward compatibility block.
The alternate path for versions older than3.8.0correctly verifies the earlierpgr_contractionsignature. This maintains backward compatibility in tests. No issues observed.
65-65: Good practice to invoke the new test function.
Invokingtypes_check()at the end ensures these checks are actually executed. This will help catch signature mismatches early.pgtap/contraction/contraction/edge_cases/linear/directed/edge_cases.pg (2)
77-81: Removal of the 'id' column from the result set.
Dropping theidcolumn and selecting only(type, contracted_vertices, source, target, cost)suggests that the identifier is no longer needed for these tests. Ensure that any downstream logic or result verification that relied onidhas been updated accordingly.
83-87: Synchronized sample solution with new column set.
Aligning the prepared solution (graph_e_1_4_sol1) with the new columns ingraph_e_1_4_q1is consistent. The result set no longer includesid, matching the updated contraction output.pgtap/contraction/contraction/edge_cases/compare_dijkstra_linear.pg (4)
21-21: Large plan count for newer versions.
Usingplan(654)for PostgreSQL ≥ 3.8.0 vs.plan(4)otherwise is a significant difference. Confirm that 654 tests indeed run as expected to avoid unintended no-op or partial test coverage.
27-43: New function check_cg for contraction validation.
This function thoroughly verifies the contracted graph’s structure. Good use ofset_eqfor comparing results. Theskip()call for older versions is a clean solution to skip irrelevant checks.
85-104: Preparation of expected graph data.
Precomputing the graph structure via a prepared statement (c_expected_graph) ensures clarity in comparing actual vs. expected outcomes. The methodology looks solid.
116-123: Invocations of create_contracted_graph and follow-up checks.
Calling bothcreate_contracted_graph(false, ...)andcreate_contracted_graph(true, ...)with subsequent validations is a comprehensive approach to ensuring correctness for both scenarios.pgtap/contraction/contraction/edge_cases/compare_dijkstra_combined.pg (2)
23-23: Confirm 0-cost edge behavior.
The use ofsign(cost)can yield zero for edges with an original cost of 0. If your aim is to ensure no zero-cost edges, confirm that having a 0 sign component (then adding0.001 * id * id) is acceptable for your test logic.Do you want to verify that this approach is consistent for edges that might have zero cost initially? If desired, I can create a quick script to locate edges with zero cost and confirm the resulting final cost.
113-122: Good test coverage with multiple outcomes.
The approach of callingcreate_contracted_graphwith bothfalseandtrueto test the different branches ofcheck_cgandcompare_dijkstra_contractionis excellent, ensuring thorough coverage.sql/contraction/contraction.sql (2)
34-37: Parameter defaults align well with flexible usage.
The newly introduced parameters (methods,cycles, andforbidden) with sensible defaults improve flexibility. This design helps avoid code duplication when only some parameters need customization.
47-47: Ensure_pgr_get_statement($1)is safe against injection.
This function constructs dynamic SQL. Confirm_pgr_get_statementproperly escapes or parameters are otherwise vetted to avoid potential SQL injection if user inputs are passed directly.Would you like a script to check references to
_pgr_get_statementacross the codebase to confirm that the input is validated or sanitized?tools/testers/contraction_tapfuncs.sql (1)
52-56: Recheck forcedreverse_cost = -1if directed paths matter.
For newly inserted edges, settingreverse_costto-1might impact route calculations whendirected = falseor for other test variations. Verify that this forced one-way cost is the intended test scenario.pgtap/contraction/contraction/edge_cases/edge_cases_linear.pg (5)
1-18: No major concerns in the licensing banner.
The licensing header and comment block appear intact and properly formatted.
224-268: Consider comprehensive edge case coverage inedge_cases(directed text).
While the test queries look extensive, double-check coverage for isolated edges, multiple forbidden vertices, and cyclical references to ensure all edge cases are effectively tested.
273-305: Rollback logic is valid for ephemeral testing.
Wrapping the tests with aBEGIN; ... ROLLBACK;helps keep the database state clean. This approach is suitable for test scripts where permanent changes are not required.
24-62:✅ Verification successful
Ensure negative costs are intentional.
Some inserted rows specify negative reverse_cost (-1). Confirm that negative edge costs are expected and won't cause unexpected behavior in the contraction logic.
🏁 Script executed:
#!/bin/bash # Searches for references to negative cost usage across the code to verify it's intentional. rg -A 3 "reverse_cost.*-1"Length of output: 13122
Negative Reverse Cost Usage Confirmed
After examination, the negative reverse_cost values (specifically -1) are used consistently across the codebase—in both the C++ implementations (e.g., in
src/trsp/GraphDefinition.cpp,src/cpp_common/pgdata_fetchers.cpp, etc.) and the tests/documentation (notably in the pgr_lineGraph docs and translation files). This consistent usage indicates that the negative costs are intentional and serve their designed purpose.No changes are necessary in this location.
19-23:❓ Verification inconclusive
Validate the test-plan count for version check.
Theplan(112)call presumes there are 112 tests whenmin_version('3.8.0')is true. Verify that the number of tests actually matches 112.Run the following script to count all test checks:
🏁 Script executed:
#!/bin/bash # This script searches for tap test function calls (plan, isnt_empty, is_empty, etc.) to estimate test counts. rg "plan\(|is_empty\(|isnt_empty\(|set_eq\(|collect_tap\(" -A 2Length of output: 67256
Verify Test-Plan Count in edge_cases_linear.pg
The code in
pgtap/contraction/contraction/edge_cases/edge_cases_linear.pgsets the TAP plan to 112 tests whenmin_version('3.8.0')holds true. Although our global search shows many test plan calls across the repository, we couldn’t locate any detailed breakdown for the contraction module in the grep output. Please double-check that this file indeed executes 112 assertions (including all conditional tests) so that the plan accurately reflects the number of tests.
- Confirm that the file contains exactly 112 TAP assertions when the version is ≥ 3.8.0.
- Update the plan call if any tests have been added or removed.
docqueries/contraction/contraction.pg (2)
5-13: Confirm new optional parameter usage.
Lines 5, 9, and 13 use themethods => ARRAY[...]parameter inpgr_contraction. Verify that internal calls and documentation are updated accordingly in the rest of the codebase.
24-59: Use consistent reverse_cost values when inserting new edges.
Line 45 setsreverse_costto-1for newly inserted edges. Confirm that this aligns with the expected logic across all contraction workflows.pgtap/contraction/contraction/edge_cases/edge_cases.pg (4)
1-18: GPL license block looks correct.
No issues found with the standard pgRouting GPL header.
19-23: Transaction control and plan checks.
Similar to other test scripts, ensure that theplan(128)call reflects the actual test count. An incorrect plan number can cause confusion during test execution.
201-260: Conditionally handling directed graphs.
Your usage ofIF directed THENis a good pattern for multi-graph testing. Consider capturing any special or additional edge cases whendirectedis true, like self-loops or zero-cost edges, to further strengthen coverage.
261-273: Rollback usage ensures no persistent side effects.
Ending withROLLBACK;is a standard approach for ephemeral testing, preventing test data from polluting the main database. This is consistent and advisable.doc/contraction/pgr_contraction.rst (39)
11-11: No issue found.
25-31: Good version update documentation.
The mention of version 3.8.0 and a new signature highlights the changes effectively.
33-33: No issue found.
35-35: Documentation consistency.
No issues with this reference to version 3.0.0.
37-40: No issue found.
42-43: No issue found.
50-50: No issue found.
75-79: No issue found.
130-130: No issue found.
142-144: No issue found.
148-151: Parameters documentation check.
Thecyclesparameter is described clearly. No issues found.
224-225: No issue found.
227-229: No issue found.
234-235: No issue found.
241-242: No issue found.
253-258: No issue found.
261-264: No issue found.
265-268: No issue found.
269-280: No issue found.
281-282: No issue found.
283-284: No issue found.
285-294: No issue found.
295-302: No issue found.
303-307: No issue found.
308-312: No issue found.
313-322: No issue found.
323-337: No issue found.
338-341: No issue found.
342-350: No issue found.
351-359: No issue found.
360-372: No issue found.
373-382: No issue found.
383-387: No issue found.
388-391: No issue found.
393-400: No issue found.
401-415: No issue found.
416-422: No issue found.
423-439: No issue found.
137-140:✅ Verification successful
Ensure default parameter correctness.
The documented default[1,2]formethodsshould match the implementation. Please confirm that the code indeed defaults to[1,2].
🏁 Script executed:
#!/bin/bash # Searching references for default usage of `methods`. rg -A 3 'ARRAY\[1,2\]'Length of output: 4965
Default Parameter Verification: Implementation Matches Documentation
The codebase confirms the default parameter is implemented correctly as
[1,2]. The filesql/contraction/contraction.sqlexplicitly sets
methods INTEGER[] DEFAULT ARRAY[1,2]::INTEGER[]- and uses
ARRAY[1,2]in related assignments and test cases.No further updates are needed.
pgtap/contraction/contraction/edge_cases/edge_cases_deadend.pg (15)
1-3: License header.
No issues found.
24-31: No issues found while creating the "graphs" table.
33-33: No issue found.
34-38: Insert statements for "graphs".
No issues found.
40-62: Multiple insert statements.
No functional issues identified.
64-92: Creating and populating "results" table.
Values appear to align with the expected contraction scenarios.
94-99: Continuation of "results" inserts.
Looks consistent.
101-113: Additional rows in "results".
No issues noted.
120-189: Function edge_cases_sampledata(directed text).
No major issues; the pgtap tests look thorough.
190-219: Function edge_cases(directed text).
No major issues, covers an array of scenarios.
222-242: Function edge_cases().
Aggregates tests for both directed and undirected. No concerns.
246-276: No issues found.
281-282: Function closure.
No issues noted.
116-119:❓ Verification inconclusive
Prepared statement "get_solution".
Verify $3 is always passed as a valid boolean in usage.
🏁 Script executed:
#!/bin/bash rg -A 5 'get_solution'Length of output: 28451
Action Required: Validate Boolean Casting for get_solution
The prepared statement "get_solution" in both
•pgtap/contraction/contraction/edge_cases/edge_cases_linear.pg
•pgtap/contraction/contraction/edge_cases/edge_cases_deadend.pguses
$3::BOOLEAN. In every invocation, the third parameter is created by string concatenation with thedirectedvariable from theedge_cases_sampledata(directed text)function. This means that the caller must ensure thatdirectedis always passed as a valid boolean literal (i.e., "true" or "false").Please verify one of the following:
• Enforce that thedirectedparameter always contains a valid boolean value before being concatenated into the statement.
• Alternatively, consider changing the type of the third parameter of "get_solution" to BOOLEAN if that suits the overall design.
21-21:❓ Verification inconclusive
Verify sign() usage for zero-cost edges.
When cost=0, sign(cost) returns 0. Confirm that zero cost is desired here.
🏁 Script executed:
#!/bin/bash # Searching if there's any usage or clarification for edges with cost=0 rg -A 3 'cost=0'Length of output: 16
Action Required: Confirm Zero-Cost Edge Handling
We searched for references clarifying how zero cost edges should be handled but did not find any details that confirm whether returning 0 from
sign(0)aligns with intended behavior. Please manually verify if having edges with a cost of 0 (i.e. whencost=0,sign(cost)returns 0) is desired in this context. If the zero-cost scenario needs special handling (for example, converting 0 to a different value), update the logic accordingly.
- File:
pgtap/contraction/contraction/edge_cases/edge_cases_deadend.pg- Line: 21
docqueries/contraction/contraction.result (19)
19-19: No issue found.
22-22: Explicit usage of "methods => ARRAY[1]".
Corresponds to dead end contraction only.
35-35: Explicit usage of "methods => ARRAY[2]".
Corresponds to linear contraction only.
43-51: Altering vertices & edges tables.
Adds columns for “is_contracted”/“is_new” and “contracted_vertices.” Matches the doc changes.
53-64: Calling pgr_contraction with "false."
No immediate issues identified.
66-75: SELECT INTO "contraction_results".
Straightforward usage. No concerns.
77-85: Marking vertices as contracted.
Logic is consistent with the new schema columns.
87-99: Retrieving updated vertices.
Useful for debugging or verifying contraction states.
101-107: Populating "contracted_vertices" in the vertices table.
Implementation follows the documented approach.
109-113: Inserting newly formed edges.
No issues found.
114-125: Filtering active vertices for the contracted graph.
No problems noted.
127-146: Selecting edges that remain post-contraction.
The logic checks for retained vertices. Looks good.
149-158: Establishing "contracted_graph" view.
Implementation matches the prior design.
160-189: Function "path_cost" for queries on the contracted graph.
Leverages pgr_dijkstraCost with dynamic queries. Appears sound.
191-195: Verification of path_cost(10,12).
No issues; result is consistent with the code.
198-202: Verification of path_cost(15,12).
No issues found.
205-209: Verification of path_cost(15,1).
No issues found.
212-212: Rolling back transaction.
No concerns.
7-7:✅ Verification successful
Use of “false” for second parameter.
Ensure that passing false aligns correctly with the new, optional syntax for “directed” or other boolean parameter.
🏁 Script executed:
#!/bin/bash rg -A 3 "pgr_contraction"Length of output: 74192
Validation on the 'directed' parameter usage in pgr_contraction
After reviewing the function signatures and test cases across the codebase, passing
falseas the second parameter (which corresponds to thedirectedflag) is confirmed to be correct and consistent with the new optional syntax. Test files—especially those targeting undirected scenarios—demonstrate that providingfalseyields the correct behavior.
- The function is defined with a signature accepting a BOOLEAN for the directed flag (defaulting to true).
- Multiple test cases (e.g., in the edge cases for undirected graphs) validate that an explicit
falsecorrectly triggers undirected processing.- Optionally, for improved clarity and consistency, consider using named parameters (e.g.,
directed:= false) in future revisions.
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (26)
NEWS.md(1 hunks)doc/contraction/contraction-family.rst(1 hunks)doc/contraction/pgr_contraction.rst(5 hunks)doc/src/release_notes.rst(1 hunks)docqueries/contraction/CMakeLists.txt(0 hunks)docqueries/contraction/contraction-family.pg(0 hunks)docqueries/contraction/contraction-family.result(0 hunks)docqueries/contraction/contraction.pg(1 hunks)docqueries/contraction/contraction.result(3 hunks)docqueries/contraction/test.conf(0 hunks)locale/en/LC_MESSAGES/pgrouting_doc_strings.po(9 hunks)locale/pot/pgrouting_doc_strings.pot(9 hunks)pgtap/contraction/contraction/edge_cases/compare_dijkstra_combined.pg(1 hunks)pgtap/contraction/contraction/edge_cases/compare_dijkstra_deadend.pg(1 hunks)pgtap/contraction/contraction/edge_cases/compare_dijkstra_linear.pg(1 hunks)pgtap/contraction/contraction/edge_cases/edge_cases.pg(1 hunks)pgtap/contraction/contraction/edge_cases/edge_cases_deadend.pg(1 hunks)pgtap/contraction/contraction/edge_cases/edge_cases_linear.pg(1 hunks)pgtap/contraction/contraction/edge_cases/linear/directed/edge_cases.pg(1 hunks)pgtap/contraction/contraction/edge_cases/remaining_tests.pg(1 hunks)pgtap/contraction/contraction/inner_query.pg(1 hunks)pgtap/contraction/contraction/no_crash_test.pg(2 hunks)pgtap/contraction/contraction/types_check.pg(1 hunks)sql/contraction/contraction.sql(2 hunks)sql/sigs/pgrouting--3.8.sig(1 hunks)tools/testers/contraction_tapfuncs.sql(1 hunks)
💤 Files with no reviewable changes (4)
- docqueries/contraction/CMakeLists.txt
- docqueries/contraction/test.conf
- docqueries/contraction/contraction-family.pg
- docqueries/contraction/contraction-family.result
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
NEWS.md
30-30: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
40-40: Multiple consecutive blank lines
Expected: 1; Actual: 2
(MD012, no-multiple-blanks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Ubuntu Boost (77)
- GitHub Check: macos (macos-14)
- GitHub Check: macos (macos-latest)
- GitHub Check: Ubuntu Boost (84)
- GitHub Check: Ubuntu Boost (83)
- GitHub Check: Ubuntu Boost (76)
- GitHub Check: Ubuntu Boost (86)
- GitHub Check: Ubuntu Boost (75)
- GitHub Check: Ubuntu Boost (79)
- GitHub Check: Ubuntu Boost (78)
- GitHub Check: Ubuntu Boost (56)
- GitHub Check: Ubuntu Boost (80)
- GitHub Check: Ubuntu clang
- GitHub Check: Ubuntu Boost (68)
🔇 Additional comments (119)
pgtap/contraction/contraction/edge_cases/remaining_tests.pg (1)
22-22:❓ Verification inconclusive
Update test count after signature changes
The number of planned tests has been reduced from 49 to 48, which aligns with the removal of tests for the deprecated function signature mentioned in the PR objectives. This ensures that the test count matches the actual number of tests executed.
However, I notice that while this file tests various edge cases for the
pgr_contractionfunction, there's no explicit test for the new optionalmethodsparameter that replaced the mandatorycontraction_orderparameter (as mentioned in the PR summary). Consider adding tests that specifically validate the behavior of this new parameter.
🏁 Script executed:
#!/bin/bash # Check if there are tests for the new methods parameter elsewhere find pgtap/ -type f -name "*.pg" | xargs grep -l "methods" | grep contractionLength of output: 167
Update test count and validate coverage for new
methodsparameterThe planned test count has been updated to 48 to match the removal of deprecated signature tests. While this file covers several edge cases for the
pgr_contractionfunction, it doesn’t specifically test the new optionalmethodsparameter. However, verification shows that tests referring tomethodscan be found in:
pgtap/contraction/contraction/inner_query.pgpgtap/contraction/contraction/types_check.pgPlease confirm that these tests comprehensively validate the behavior of the new
methodsparameter as outlined in the PR summary.locale/en/LC_MESSAGES/pgrouting_doc_strings.po (10)
11-11: POT-Creation-Date updated to 2025-03-21The PO template creation date has been updated to March 21, 2025. This ensures the translation file is in sync with the latest template.
2973-2973: Clarification about non-linear white node behaviorThis addition clarifies why a white node is not considered linear when the linearity is asymmetrical, which is important for users to understand the contraction behavior.
4192-4214: Documentation for new pgr_contraction signatureThese changes properly document the updates to the pgr_contraction function, including:
- Making the "Contraction order" parameter optional and renaming it to "methods"
- Changing the order of optional parameters
- Marking the old signature as deprecated
The documentation clearly communicates these changes to users, which aligns with the PR objectives.
9343-9344: Version information additionAdded "Version 3.8.0" section to indicate when specific changes were made. This helps users understand the evolution of the function.
9359-9381: Enhanced function behavior documentationThis section clearly explains the function's behavior:
- It returns only changes to the graph, not the full contracted graph
- Details what types of changes are returned (new edges, modified vertices)
- Documents the ordering of returned results
These additions improve usability by setting clear expectations for users about function results.
9383-9406: Improved contraction methods documentationThis section adds important details about:
- The types of contraction methods available
- References to the contraction family documentation
- Updated function signature with optional parameters
- Default behavior (dead end and linear contraction on undirected graphs)
These changes help users understand the function's capabilities and default behavior.
9407-9446: Parameter documentation updatesThe documentation now properly describes the optional parameters with:
- Parameter names
- Data types
- Default values
- Descriptions
Particularly important is the clarification that "methods" (formerly "Contraction order") is now optional with a default value of ARRAY[1,2].
9454-9466: Clarified row type documentationThe description of the return values has been improved with:
- Clear explanation of the "type" column
- Distinction between vertex rows (positive ID) and edge rows (negative ID)
This helps users correctly interpret the function results.
9489-9490: Parameter name added for clarityAdded field name "contracted_vertices" to make the documentation more complete and consistent.
9519-9690:✅ Verification successful
Verify that removed content is properly documented elsewhere
A substantial amount of documentation has been removed, including sections about:
- The contraction cycle
- Sample data examples
- Construction of contracted graphs
- Using contracted graphs
Since this is a significant removal of content, please ensure this information is available elsewhere in the documentation if it's still relevant to users.
🏁 Script executed:
#!/bin/bash # Check if the removed content has been moved to other documentation files echo "Checking for contraction documentation in other files..." # Look for contraction family documentation fd "contraction-family.rst" --type f # Check if the removed examples are in other files rg "Contracting sample data" --type rst # Look for any documentation about creating contracted graphs rg "Construction of the graph in the database" --type rst echo "Checking for alternative documentation of removed concepts..." # Check if there are other examples of using contracted graphs rg "Using the contracted graph" --type rstLength of output: 679
Documentation Removals Verified
The removed sections are now documented in alternative files. Specifically:
- The contraction cycle is covered in doc/contraction/contraction-family.rst.
- Content related to contracting sample data, construction of contracted graphs, and using contracted graphs is available in doc/contraction/pgr_contraction.rst.
Please ensure that the user documentation clearly references these files so the migrated content is easily accessible.
locale/pot/pgrouting_doc_strings.pot (8)
11-11: POT file timestamp updated appropriately.The timestamp update reflects the generation date of the translation template file, which is a standard operation when documentation is modified.
2728-2728: Good spelling correction!The fix from "symetrical" to "symmetrical" improves documentation accuracy.
3754-3770: Well-documented function signature changes.The new documentation strings clearly explain the changes to
pgr_contraction:
- Parameter renamed from "Contraction order" to "methods" and made optional
- New parameter ordering
- Properly marked deprecated signature
These changes align perfectly with issue #2786 mentioned in the PR objectives.
8017-8018: Good version documentation.Adding the version information where these changes were introduced (3.8.0) is excellent practice for maintaining clear documentation.
8029-8072: Enhanced function behavior documentation.The added documentation strings significantly improve user understanding by clearly stating:
- The function returns only graph changes, not the full contracted graph
- The specific types of returned values (new edges, modified vertices)
- The ordering of results
- Available contraction methods with links to detailed documentation
This provides much clearer guidance for users of the function.
8074-8114: Well-structured parameter documentation.The documentation now clearly describes all optional parameters with:
- Parameter names and types
- Default values
- Clear descriptions of parameter purposes
This structured approach makes the function interface much more understandable.
8122-8134: Improved return value documentation.The documentation now clearly explains the meaning of the return value types and their ID conventions:
- Type 'v' (vertex) rows have positive ID values
- Type 'e' (edge) rows have negative ID values
This clarification helps prevent confusion when interpreting results.
8155-8156: Correctly formatted contracted_vertices documentation.The documentation for the contracted_vertices field has been properly structured.
NEWS.md (1)
30-39: Well-documented function changeThe documentation for the
pgr_contractionfunction changes is clear and detailed. It correctly outlines that the previously compulsory parameter Contraction order is now optional and renamed tomethods, mentions the new parameter order, and documents the deprecated signature.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
30-30: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
sql/sigs/pgrouting--3.8.sig (1)
91-91: New signature addition looks goodThe addition of the new signature
pgr_contraction(text,boolean,integer[],integer,bigint[])properly implements the changes documented in the release notes, while maintaining the original signature for backward compatibility.doc/src/release_notes.rst (1)
62-69: Good documentation of changes in release notesThe addition of the section for "Official functions changes" with a reference to issue #2786 for
pgr_contractionis well-structured. Including the content frompgr_contraction.rstthrough the include directive provides a clean way to avoid duplication while ensuring the release notes are comprehensive.pgtap/contraction/contraction/no_crash_test.pg (2)
22-22: Good version-aware test planUsing a conditional statement to adjust the number of planned tests based on the version is a good practice. This ensures the test file works correctly regardless of whether it's run against an old or new version of pgRouting.
48-62: Well-structured version check for new signatureThe conditional logic to handle the new function signature only for version 3.8.0 or higher is implemented correctly. The test properly skips testing the new signature on older versions and provides a clear skip message. The parameters for the new signature test are appropriate for testing the simplified parameter list.
doc/contraction/contraction-family.rst (1)
296-296: Good correction in terminology.Fixed terminology from "symetrical" to "symmetrical" in the explanation of why a white node is not considered linear in a directed graph. This improves document accuracy and readability.
pgtap/contraction/contraction/edge_cases/compare_dijkstra_deadend.pg (5)
1-110: Well-structured test for pgr_contraction dead-end functionality.This new test file properly validates the dead-end contraction functionality with comprehensive test cases. The conditional execution based on version 3.8.0 aligns with the PR objective of introducing a new signature for the pgr_contraction function.
The
check_cgfunction effectively tests both parameter variations (true/false) and compares the results against expected values, ensuring consistency in the contraction behavior.
21-21: Proper version-aware test planning.The conditional test planning based on PostgreSQL version ensures compatibility while properly testing the new functionality only when available.
24-25: Good schema preparation for tests.The additional boolean and array columns in both tables will enable tracking of contraction operations, which is essential for proper validation of the contraction functionality.
27-98: Comprehensive test function implementation.The
check_cgfunction is well-structured with:
- Version compatibility check
- Conditional testing based on the input parameter
- Expected vs. actual graph comparison using properly rounded values
This ensures reliable testing of the contraction functionality's correctness.
101-107: Complete test coverage with both parameter options.Testing with both boolean parameters (true and false) ensures comprehensive validation of the contraction functionality in different modes of operation.
pgtap/contraction/contraction/inner_query.pg (4)
1-106: Well-implemented test for pgr_contraction signature validation.This test file thoroughly validates the various signatures and parameter handling of the new pgr_contraction function interface. The tests cover both valid use cases and error conditions, ensuring robust API behavior.
22-22: Proper version-aware test planning.The conditional test count based on PostgreSQL version ensures appropriate testing scope depending on feature availability.
34-40: Good validation of function overloads.These tests verify that all the intended function signatures for pgr_contraction are properly implemented and accessible.
42-96: Comprehensive error handling validation.The tests for various error conditions (invalid array types, empty arrays, wrong dimensions) ensure that the function behaves predictably with improper inputs, providing appropriate error messages.
pgtap/contraction/contraction/types_check.pg (3)
21-21: Proper version-specific test planning.The plan count is now conditional on the PostgreSQL version, which ensures appropriate test coverage based on feature availability.
23-61: Well-structured type validation function.The new
types_check()function properly validates the function signature, return types, and parameter names based on the PostgreSQL version. This ensures that:
- For 3.8+ versions: The new function signature with the optional
methodsparameter is validated- For earlier versions: The original signature with required parameters is validated
This comprehensive type checking supports the PR objective of introducing a new signature for the pgr_contraction function.
65-65: Proper function call replacement.The direct type checks have been replaced with a call to the more comprehensive
types_check()function, which improves maintainability and version-specific testing.pgtap/contraction/contraction/edge_cases/linear/directed/edge_cases.pg (1)
83-87: Values clause updated to remove theidfield.This change mirrors the updated query structure by dropping the
idcolumn in favor of only includingtype,contracted_vertices,source,target, andcost. Confirm no references to the removedidfield remain, and verify that the test expectations align with this new schema.pgtap/contraction/contraction/edge_cases/edge_cases.pg (4)
1-23: License header and initial setup look good.No issues with the license block or core test setup logic. The
plan(128)for pgTap is standard.
24-63: Creation oftest_deadendtable and initial inserts.The usage of an identity primary key and the columns for source/target/cost are consistent with pgRouting test conventions. Ensure that negative or zero
costscenarios are tested if relevant for dead-end paths, but this may be intentional for these specific edge cases.Would you like to broaden coverage to include negative cost edges or zero-cost edges to test more corner cases?
201-239: Definition ofedge_cases(directed text)function.This function tests edge presence, emptiness, and expected graph structures. Everything appears consistent:
- The calls to
is_empty/isnt_emptyandset_eqare standard for verifying expected outcomes in pgTap.- The usage of
format(...)to build queries is correct for dynamic strings.
240-274: Definition ofedge_cases()function and concluding statements.The conditional check for
min_version('3.8.0')and fallback behavior are well-handled. The final calls to runedge_cases()and thenfinish()followed byROLLBACK;are typical for this testing pattern.pgtap/contraction/contraction/edge_cases/compare_dijkstra_combined.pg (4)
1-23: License header and initial setup.The license block and
plan(654)call for pgTap indicate an extensive set of tests, which is appropriate for contraction features.
25-27: Altering tables to addis_new,is_contracted, andcontracted_verticescolumns.
- Adding these columns for edges and vertices is consistent with storing extra metadata about graph contraction.
- Keep in mind potential integer-array size limitations if extremely large graphs are tested.
Have you validated that storing large arrays of contracted vertices won’t exceed any practical limits for your use case?
28-111: Creation ofcheck_cg(BOOLEAN)function.
- The function’s logic accommodates pre-3.8.0 versions by skipping tests.
- The tests compare expected contracted graph components (
contraction_info,edges, etc.) to the newly generated or altered graph structures.- Use of
PREPARE c_expected_graphfor verifying the final graph is a clear approach, ensuring that floating-point cost comparisons are correctly rounded to three decimals.
113-123: Invocations ofcreate_contracted_graph,check_cg, andcompare_dijkstra_contraction.
- Calling both with
trueandfalseensures coverage of directed/undirected or standard/extra parameter scenarios.- The sequence of calls to
finish()followed byROLLBACK;is standard for finalizing tests without persisting changes.pgtap/contraction/contraction/edge_cases/compare_dijkstra_linear.pg (3)
21-21: Validate test count inplan(654).
The test count is set to 654 whenmin_version('3.8.0')is true, and 4 otherwise. Ensure that all tests are actually performed or intended. An excessively high plan number could cause confusion if there are fewer actual tests.Would you like a script to confirm that all 654 tests are legitimately accounted for?
22-22: Check for possible floating precision issues inUPDATE edges.
cost = sign(cost) + 0.001 * id * idmay introduce floating precision risks ifidis large. Consider verifying that numeric values remain within acceptable ranges for your tests.Please confirm the typical maximum
idin the testing dataset.
116-122: Confirm correctness of repeated calls tocreate_contracted_graph.
You are invokingcreate_contracted_graphmultiple times with different boolean flags. Verify that each scenario is well-documented and that test expectations are reset or rolled back correctly between calls. This looks fine in a test context, but clarity around each scenario’s purpose helps future test maintainers.pgtap/contraction/contraction/edge_cases/edge_cases_deadend.pg (3)
21-21: Consistent usage ofsign(cost)in test setup.
Updating all edges withcost = sign(cost)andreverse_cost = sign(reverse_cost)can help unify test data. Just ensure that edge cases involving zero or negative costs are handled properly, and verify that sign(0) behaves as expected for your test intentions.
24-31: Tablegraphscreation appears correct.
The schema is straightforward. Check that thedead_casecolumn and default cost values are used properly in the rest of the tests, especially for your dead-end scenarios.
287-309: Version-gating logic foredge_cases()function.
Similar to the other file, the test gracefully skips for versions below 3.8.0. Ensure that you maintain or remove older version references once support is phased out.sql/contraction/contraction.sql (2)
30-43: Proper introduction of new parameters.
The newly addedmethods,cycles, andforbiddenparameters (lines 34–36) align well with the updated signature. Confirm that you have comprehensive documentation so users understand what each array or parameter does.
46-47: Direct SQL call to_pgr_contraction.
Forwarding parameters asmethods::BIGINT[], cycles, forbidden, directedis correct based on your new signature. Ensure you have a matching definition in_pgr_contractionand that type casting fromINTEGER[]toBIGINT[]is consistently handled.doc/contraction/pgr_contraction.rst (44)
11-11: Inclusion of new index reference
No code impact.
25-26: Version update
Use consistent version references in the doc if referencing multiple version changes.
27-31: Documenting new signature
The doc changes correctly reflect the new optional parameter "methods" and the revised order of optional parameters.
33-33: Deprecated signature note
Ensure the docs remain consistent about the deprecation timeline and usage.
35-35: Historical reference
No concerns.
37-37: Result columns note
No concerns.
39-39: Old function name note
No concerns.
42-45: Version 2.3.0 reference
No concerns.
50-51: Introduction of characteristics block
No code logic.
59-60: Clarification on returned graph
Helpful to mention partial result scope.
61-61: Explanation for partial results
No concerns.
63-67: Detailed listing of returned values
Documentation is thorough.
68-73: Order of result mention
Clear ordering clarifications. Looks good.
75-80: Methods mention
Accurately references the new 'methods' parameter.
92-95: Signature reference
Matches the new optional parameters (directed, methods, cycles, forbidden).
97-97: Example usage
No concerns.
128-128: Table style auto
No concerns.
137-144: New 'methods' param
Documentation aligns with the updated code changes.
145-145: cycles param
No concerns.
146-146: cycles param doc
Clear explanation of repeated contraction passes.
148-153: Handling forbidden vertices
Doc is consistent with the new parameter approach.
182-182: Documentation note
No concerns.
185-185: Clarification for vertex type
No concerns.
188-188: Clarification for edge type
No concerns.
221-221: Examples heading
No concerns.
224-236: Additional examples
No concerns.
241-251: Multiple cycles doc
Provides clarity on multiple operations usage.
253-255: Algorithm snippet
No concerns.
257-258: Doc snippet
No concerns.
261-265: Contextual example
No concerns.
271-271: Doc structure
No concerns.
273-293: Implementation details
No concerns.
297-300: Image references
No concerns.
302-303: Doc heading
No concerns.
305-372: Detailed steps for building the contraction graph
Well-documented procedure.
373-382: Contracted graph final explanation
No concerns.
383-392: Images references
No concerns.
394-394: Doc heading
No concerns.
396-408: Covers multiple use cases
No concerns.
410-410: Doc instruction
No concerns.
412-422: Demonstration function
No concerns.
424-439: Detailed use cases
No concerns.
442-442: Doc references
No concerns.
445-445: Index reference
No concerns.tools/testers/contraction_tapfuncs.sql (4)
19-79: Procedure create_contracted_graph
- Good logic resetting the edges table (and sequence) and updating columns.
- The version check with min_version('3.8.0') is a sensible approach for backward compatibility.
- Dynamic SQL blocks appear correctly parameterized, but do ensure any user input is sanitized in practice.
107-126: compare_dijkstra_contraction (3-arg)
- Cross-checks path_cost_contraction results with standard pgr_dijkstra.
- Using set_eq helps confirm identical result sets.
129-153: compare_dijkstra_contraction (1-arg)
- Iterates over vertex pairs and calls the 3-arg version for thorough coverage.
- Good for systematic testing, though caution on performance if large graphs are tested.
80-106:✅ Verification successful
Function path_cost_contraction
- Properly merges additional vertices from cul_de_sac and linears_to_expand.
- Return query uses pgr_dijkstra with a custom subquery. This is likely correct for specialized path cost calculations.
Please confirm that negative edge IDs do not cause unintended side effects. If needed, run this script to locate references to “edge = -1” outside this function:
🏁 Script executed:
#!/bin/bash rg -A 5 $'edge = -1'Length of output: 19523
Negative Edge ID Usage Confirmed
- The grep output shows consistent use of
WHERE edge = -1across various parts of the codebase (tests, query scripts, and C++ modules), confirming that negative edge IDs serve as a deliberate sentinel value.- There is no evidence to suggest that the use of negative edge IDs in
path_cost_contractionintroduces unintended side effects.- The implementation in this function adheres to the established pattern observed elsewhere.
docqueries/contraction/contraction.pg (10)
5-5: Default usage
Ensuring 'false' for directed is consistent with the updated doc references.
9-9: Use of new 'methods' parameter
Specifically requesting Dead End Contraction only.
13-13: Use of new 'methods' parameter
Specifically requesting Linear Contraction only.
16-23: Schema update
- Adds columns is_contracted, is_new, and contracted_vertices to store contraction states.
- This design matches the documented approach from pgr_contraction.
24-30: pgr_contraction usage
Consistent usage with the new function signature and optional directed param.
31-47: Applying contraction results
- Correctly marks vertices as contracted.
- Inserts newly generated edges with is_new set to true.
49-59: Retrieving valid edges
Filters out edges whose endpoints are contracted. Aligns with the approach described in the docs.
61-68: Creating 'contracted_graph' view
View definition is aligned with the updates in is_contracted logic.
69-97: path_cost function
- Integrates pgr_dijkstraCost with a union approach to combine newly generated edges and original edges.
- Implementation is consistent with the earlier doc and helps users handle the partially contracted graph.
99-104: Function usage examples
Checking path costs on sample pairs. Demonstrates correctness.pgtap/contraction/contraction/edge_cases/edge_cases_linear.pg (4)
21-21: Double-check the sign usage on costs.Using
sign(cost)andsign(reverse_cost)to normalize edge costs is fine for testing scenarios. However, ensure this is intentional if negative or zero costs are needed in edge case validations. For example, a0cost would remain0, and a negative cost would become-1, which may omit nuances in your tests.Would you like to confirm whether negative costs must be preserved for certain test cases?
293-295: Verify coverage of both directed and undirected cases.These lines confirm that tests are run in both directed (
'true') and undirected ('false') modes. Ensure all essential edge cases are covered for each mode (particularly around reversed edges in directed graphs).
303-305: Confirm ROLLBACK usage for pgtap tests.Issuing
ROLLBACK;at the end of the file is a common pattern in pgtap to revert any modifications. Ensure external dependencies, if any, are not left hanging or undone inadvertently. If everything is self-contained, this approach is appropriate.
218-218:✅ Verification successful
Ensure reliable gating by version checks.
The usage of
IF NOT min_version('3.8.0') THEN ... ELSE ... END IF;ensures backward compatibility. Please confirm that any future references to a different minimum version are also updated consistently across your tests to avoid version mismatch in other test files.Run a quick search across the codebase to confirm there are no references to other minimum versions that might conflict:
🏁 Script executed:
#!/bin/bash rg "min_version\(.*\)" --context 3Length of output: 75394
Confirmed: Version gating consistency is maintained.
The search across the codebase indicates that the use of
min_versionis intentional—each test file has its expected minimum version (e.g.,3.8.0in the file under review, while others use3.4.0,3.1.0,3.2.0, etc.). There are no conflicting references detected. Please continue to ensure that any future changes to the minimum version requirements are updated consistently across all tests.docqueries/contraction/contraction.result (3)
7-7: Confirm removal of named parameter usage.Here, replacing
directed => falsewith a simplefalsealigns with the updatedpgr_contractionsignature. Check that any remaining references todirected =>have also been updated for consistency throughout the documentation.
22-22: Validate the newmethodsparameter array.Passing
methods => ARRAY[1]reflects the revised function signature. Ensure that the parameter meaning is thoroughly documented and matches the actual logic inpgr_contraction.Do other tests or documentation references accurately describe what the numeric entries in
methodsrepresent?
102-106: Verify UPDATE alignment with contracted vertices.This update sets
contracted_verticesinverticesbased on matching IDs fromcontraction_results. Confirm thattype = 'v'remains the correct condition for identifying contracted vertices, and ensure the selected rows incontraction_resultsare indeed vertex rows.
pgtap/contraction/contraction/edge_cases/compare_dijkstra_linear.pg
Outdated
Show resolved
Hide resolved
pgtap/contraction/contraction/edge_cases/compare_dijkstra_linear.pg
Outdated
Show resolved
Hide resolved
Fixes #2786
Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
pgr_contractionfunction.Refactor