reverting some changes from c1b21c2fc8 #2736
Conversation
WalkthroughThis pull request introduces significant updates to pgRouting 4.0.0, focusing on function and signature management. The changes involve promoting several functions to official status, removing deprecated SQL functions, and streamlining internal C/C++ implementations. Key modifications include updating the Changes
Suggested labels
Suggested reviewers
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 review |
|
@cvvergara I'll help review the changes. First, here's a summary of the PR: This PR reverts some changes from commit c1b21c2 that were aimed at simplifying the Dijkstra algorithm implementation. The main changes include:
The reversion is being done to avoid extensive changes that would be needed in the pgtap test suite. Let me review the changes in detail. 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 19588 ✅ 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
NEWS.md(0 hunks)doc/src/release_notes.rst(0 hunks)sql/dijkstra/_dijkstra.sql(2 hunks)sql/dijkstra/dijkstra.sql(5 hunks)sql/dijkstra/dijkstraCost.sql(5 hunks)sql/dijkstra/dijkstraCostMatrix.sql(1 hunks)sql/dijkstra/dijkstraNear.sql(4 hunks)sql/dijkstra/dijkstraNearCost.sql(4 hunks)sql/sigs/pgrouting--4.0.sig(1 hunks)src/dijkstra/dijkstra.c(0 hunks)
💤 Files with no reviewable changes (3)
- src/dijkstra/dijkstra.c
- NEWS.md
- doc/src/release_notes.rst
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Ubuntu Boost (86)
- GitHub Check: Ubuntu Boost (84)
- GitHub Check: Ubuntu Boost (83)
- GitHub Check: Ubuntu Boost (80)
- GitHub Check: Ubuntu Boost (79)
- GitHub Check: Ubuntu Boost (78)
- GitHub Check: Ubuntu Boost (77)
- GitHub Check: Ubuntu Boost (76)
- GitHub Check: Ubuntu Boost (75)
- GitHub Check: macos (macos-14)
- GitHub Check: macos (macos-latest)
- GitHub Check: Ubuntu Boost (68)
- GitHub Check: documentation (en)
- GitHub Check: Ubuntu clang
- GitHub Check: Check queries
- GitHub Check: Ubuntu Boost (56)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (10)
sql/sigs/pgrouting--4.0.sig (2)
123-123: Confirm consistency with rest of codebase.
The parameters match the reverted_pgr_dijkstrasignature, but please ensure downstream references and tests remain synchronized, especially regarding the new boolean and bigint parameters.
128-128: 🧹 Nitpick (assertive)Validate parameter order and ensure documentation is current.
Keep an eye on potential confusion between the multiplebooleanparameters. Consider clarifying or documenting each parameter’s purpose in the official docs.sql/dijkstra/dijkstraCostMatrix.sql (1)
45-45: Confirm that passing the same array for start and end vertices is desired.
Here,$2is used for bothstart_vidsandend_vids. While valid, it may produce unexpected results if the query is intended to generate distinct start-end pairs. Otherwise, this looks consistent with the updated_pgr_dijkstrasignature.sql/dijkstra/_dijkstra.sql (3)
52-54: Evaluate STRICT usage.
Marking the function asSTRICTensures it won’t be called with any NULL arguments, which can prevent potential errors but might limit flexibility. Confirm that all callers provide valid, non-null inputs.
81-82: Added comment for_pgr_dijkstra(…TEXT…).
Consistent documentation annotations are essential. Make sure usage or design notes are accessible for contributors.
55-75: 🧹 Nitpick (assertive)Second
_pgr_dijkstravariant.
Having two variants with slightly different parameter sets can be helpful but can also cause confusion. Double-check consistency across the code (in SQL definitions, C implementations, and doc references).sql/dijkstra/dijkstraNearCost.sql (1)
47-47: Function calls updated consistently with reversion.The changes correctly revert the function calls from
_pgr_dijkstrav4back to_pgr_dijkstrawhile maintaining the same parameter structure and functionality.Also applies to: 69-69, 92-92, 114-114
sql/dijkstra/dijkstraCost.sql (1)
46-46: Function calls consistently reverted across all variants.The changes successfully revert the implementation back to using
_pgr_dijkstrawhile preserving the correct parameter passing and functionality for all variants (ONE to ONE, ONE to MANY, MANY to ONE, MANY to MANY, and Combinations).Also applies to: 69-69, 93-93, 117-117, 137-137
sql/dijkstra/dijkstraNear.sql (1)
52-52: Function calls properly reverted with parameter consistency.The changes correctly revert to using
_pgr_dijkstrawhile maintaining proper parameter passing and functionality across all variants. The boolean flags and cap parameters are correctly preserved.Also applies to: 79-79, 107-107, 134-134
sql/dijkstra/dijkstra.sql (1)
56-56: Core Dijkstra implementation successfully reverted.The changes correctly revert the core implementation back to using
_pgr_dijkstra. All function variants maintain proper parameter passing and expected functionality.Let's verify that all calls to
_pgr_dijkstrav4have been properly removed:Also applies to: 82-82, 108-108, 134-134, 159-159
✅ Verification successful
Verified: Complete removal of _pgr_dijkstrav4 confirmed
The codebase has been thoroughly checked and shows:
- No remaining references to _pgr_dijkstrav4
- Consistent usage of _pgr_dijkstra across all function variants
- Proper parameter passing in all implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify complete removal of _pgr_dijkstrav4 references echo "Checking for any remaining references to _pgr_dijkstrav4..." rg "_pgr_dijkstrav4"Length of output: 149
Script:
#!/bin/bash # Description: Comprehensive verification of Dijkstra implementation echo "Checking SQL files for any variations of dijkstrav4..." rg -t sql "dijkstrav\w+" echo -e "\nVerifying correct _pgr_dijkstra usage in SQL files..." rg -t sql "_pgr_dijkstra\s*\(" -A 1 echo -e "\nChecking for any CREATE FUNCTION statements with dijkstra..." rg -t sql "CREATE(\s+OR\s+REPLACE)?\s+FUNCTION.*dijkstra.*"Length of output: 7738
After a second thought:
Reverting some changes from #c1b21c2fc8
Basically because a lot of changes on pgtap tests would be needed.
@pgRouting/admins
Summary by CodeRabbit
Release Notes for pgRouting 4.0.0
New Features
pgr_trsp,pgr_withPoints, andpgr_aStarImprovements
Deprecations
Breaking Changes