-
-
Notifications
You must be signed in to change notification settings - Fork 377
Standardize pgr_bdDijkstra output columns #2912
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Standardize pgr_bdDijkstra output columns #2912
Conversation
- Adjusted the build update script - Updated the queries results for the documentation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 summary |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@cvvergara I'll gather the PR information and perform both a summary and review for you. Let me start by examining the PR details and changes. 🏁 Script executed: Length of output: 42918 @cvvergara Here's the summary and review for PR #2912: SummaryPR #2912: Standardize pgr_bdDijkstra output columns This PR implements a breaking change to standardize the output columns of the Key Changes:
Detailed Review✅ Strengths
🔍 Technical Implementation ReviewSQL Changes ( -- Old signature (One-to-One)
OUT seq INTEGER, OUT path_seq INTEGER, OUT node BIGINT, OUT edge BIGINT, OUT cost FLOAT, OUT agg_cost FLOAT
-- New signature (One-to-One)
OUT seq INTEGER, OUT path_seq INTEGER, OUT start_vid BIGINT, OUT end_vid BIGINT, OUT node BIGINT, OUT edge BIGINT, OUT cost FLOAT, OUT agg_cost FLOATThe implementation correctly:
Build Script Updates: push @commands, drop_special_case_function("pgr_bddijkstra(text,bigint,bigint,boolean)");
push @commands, drop_special_case_function("pgr_bddijkstra(text,anyarray,bigint,boolean)");
push @commands, drop_special_case_function("pgr_bddijkstra(text,bigint,anyarray,boolean)");🎯 Migration StrategyThe migration guide provides clear examples for users to maintain compatibility: -- New format
SELECT * FROM pgr_bdDijkstra(...);
-- To get old format, filter columns:
SELECT seq, path_seq, node, edge, cost, agg_cost
FROM pgr_bdDijkstra(...);✅ Overall AssessmentThis is a well-executed breaking change that:
Recommendation: ✅ APPROVE The PR successfully standardizes the |
Saloni-2005
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve reviewed the function signatures, test coverage, migration guide, and release documentation. The additions of start_vid and end_vid follow pgRouting standardization efforts. Migration paths are clear and the update is well-documented. This is a solid step for v4.0.0.
Fixes #2906
Changes proposed in this pull request:
@pgRouting/admins