Remove deprecated functions #2732
Conversation
WalkthroughThis pull request focuses on documenting function promotions and removals for pgRouting version 4.0.0. The changes involve updating release notes, removing legacy SQL functions, modifying function signatures, and cleaning up deprecated internal functions across multiple files. The modifications streamline the library's functionality by promoting stable functions and eliminating outdated implementations while maintaining backward compatibility. Changes
Sequence DiagramsequenceDiagram
participant Library as pgRouting Library
participant User as Developer
participant Deprecated as Deprecated Functions
participant New as New Functions
Library->>Deprecated: Deprecation Notice
Deprecated-->>User: Warning Message
User->>New: Migrate to New Functions
New->>Library: Use Updated Implementations
Possibly related PRs
Suggested labels
Suggested reviewers
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 |
✅ 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 (17)
doc/src/release_notes.rst(2 hunks)sql/driving_distance/_drivingDistance.sql(0 hunks)sql/legacy/alpha_shape.sql(0 hunks)sql/legacy/legacy-developers.sql(0 hunks)sql/legacy/legacy_experimental.sql(0 hunks)sql/legacy/pgrouting_version.sql(0 hunks)sql/legacy/pointsAsPolygon.sql(0 hunks)sql/legacy/routing_legacy.sql(0 hunks)sql/legacy/tsp/TSPeucledian.sql(0 hunks)sql/legacy/tsp/_makeDistanceMatrix.sql(0 hunks)sql/legacy/tsp/tsp_v2.0_coordinates.sql(0 hunks)sql/legacy/tsp/tsp_v2.0_matrix.sql(0 hunks)sql/legacy/vrppdtw/_gsoc_vrppdtw.sql(0 hunks)sql/legacy/vrppdtw/gsoc_vrppdtw.sql(0 hunks)sql/sigs/pgrouting--4.0.sig(0 hunks)src/driving_distance/driving_distance.c(0 hunks)src/legacy/legacy.c(1 hunks)
💤 Files with no reviewable changes (15)
- sql/legacy/pgrouting_version.sql
- sql/legacy/vrppdtw/gsoc_vrppdtw.sql
- sql/legacy/legacy-developers.sql
- sql/legacy/alpha_shape.sql
- sql/driving_distance/_drivingDistance.sql
- sql/legacy/tsp/_makeDistanceMatrix.sql
- sql/legacy/tsp/TSPeucledian.sql
- sql/legacy/tsp/tsp_v2.0_coordinates.sql
- src/driving_distance/driving_distance.c
- sql/legacy/tsp/tsp_v2.0_matrix.sql
- sql/legacy/vrppdtw/_gsoc_vrppdtw.sql
- sql/legacy/pointsAsPolygon.sql
- sql/legacy/legacy_experimental.sql
- sql/sigs/pgrouting--4.0.sig
- sql/legacy/routing_legacy.sql
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Ubuntu Boost (84)
- GitHub Check: macos (macos-14)
- GitHub Check: macos (macos-latest)
🔇 Additional comments (1)
src/legacy/legacy.c (1)
57-57: LGTM! Verify the migration impact.The addition of
_pgr_drivingdistanceto the deprecated functions list is consistent with the existing pattern and correctly uses the PGROUTING_DEPRECATE macro with version 4.0.0.Let's verify the impact of this deprecation:
✅ Verification successful
Deprecation properly implemented with clean migration path
The deprecated
_pgr_drivingdistancefunction has been correctly moved to legacy.c with:
- New implementation
_pgr_drivingdistancev4available in driving_distance.c- No remaining direct usage of deprecated version
- Clean separation between legacy and new code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to _pgr_drivingdistance that might need updating # Check for any direct usage of the deprecated function echo "Checking for direct usage of _pgr_drivingdistance..." rg --type sql "_pgr_drivingdistance" # Check for any SQL function definitions that might need updating echo "Checking for SQL function definitions..." rg --type sql "CREATE .*FUNCTION.*_pgr_drivingdistance" # Look for any remaining implementation files echo "Checking for implementation files..." fd -e c -e h "driving_distance"Length of output: 635
Script:
#!/bin/bash # Check implementation files for any remaining _pgr_drivingdistance code echo "Checking driving_distance.c content..." rg -A 5 "_pgr_drivingdistance" src/driving_distance/driving_distance.c echo -e "\nChecking header files for declarations..." for file in include/drivers/driving_distance/*.h; do echo "=== Checking $file ===" rg "_pgr_drivingdistance" "$file" done echo -e "\nChecking for any remaining implementations..." ast-grep --pattern 'PG_FUNCTION_INFO_V1(_pgr_drivingdistance)'Length of output: 10225
cvvergara
force-pushed
the
find-deprecated
branch
from
09663cd to
7280dca
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
pgtap/dijkstra/driving_distance/edge_cases/issue_519.pg (1)
Line range hint
47-64: Consider using a constant for client message level.The client message level is set and reset multiple times. Consider defining a constant for the message level to improve maintainability.
+ DECLARE + CONSTANT WARNING_LEVEL constant text := 'WARNING'; + CONSTANT NOTICE_LEVEL constant text := 'NOTICE'; BEGIN - SET client_min_messages TO WARNING; + SET client_min_messages TO WARNING_LEVEL; ... - SET client_min_messages TO NOTICE; + SET client_min_messages TO NOTICE_LEVEL;NEWS.md (2)
Line range hint
81-87: Fix incomplete function signatures.Some function signatures are truncated and missing closing parentheses and parameter types.
Apply this diff to fix the signatures:
-* pgr_trsp(text,integer,double +* pgr_trsp(text,integer,double precision,integer,double precision,boolean,boolean,text)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
50-50: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
57-57: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
73-73: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
Line range hint
106-110: Fix inconsistent backtick usage.Some function signatures have trailing backticks while others don't.
Apply this diff to make backtick usage consistent:
-* _pgr_dijkstra(text,anyarray,anyarray,boolean,boolean,boolean,bigint)`` -* _pgr_dijkstra(text,anyarray,anyarray,boolean,boolean,boolean,bigint,boolean)`` -* _pgr_dijkstra(text,text,boolean,boolean,bigint,boolean)`` -* _pgr_dijkstra(text,text,boolean,boolean,boolean)`` +* _pgr_dijkstra(text,anyarray,anyarray,boolean,boolean,boolean,bigint) +* _pgr_dijkstra(text,anyarray,anyarray,boolean,boolean,boolean,bigint,boolean) +* _pgr_dijkstra(text,text,boolean,boolean,bigint,boolean) +* _pgr_dijkstra(text,text,boolean,boolean,boolean)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
50-50: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
57-57: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
73-73: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
♻️ Duplicate comments (1)
doc/src/release_notes.rst (1)
83-84:⚠️ Potential issueFix incomplete function signatures.
Some function signatures are truncated and missing closing parentheses and parameter types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
NEWS.md(1 hunks)doc/src/release_notes.rst(1 hunks)pgtap/dijkstra/driving_distance/edge_cases/issue_519.pg(3 hunks)pgtap/dijkstra/driving_distance/inner_query.pg(1 hunks)pgtap/dijkstra/driving_distance/no_crash_test.pg(2 hunks)pgtap/dijkstra/driving_distance/types_check.pg(1 hunks)sql/driving_distance/_drivingDistance.sql(0 hunks)sql/sigs/pgrouting--4.0.sig(0 hunks)src/driving_distance/driving_distance.c(2 hunks)
💤 Files with no reviewable changes (2)
- sql/driving_distance/_drivingDistance.sql
- sql/sigs/pgrouting--4.0.sig
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
NEWS.md
50-50: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
57-57: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
73-73: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: macos (macos-14)
- GitHub Check: macos (macos-latest)
- GitHub Check: Codacy Static Code Analysis
- 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: Ubuntu Boost (68)
- GitHub Check: Ubuntu Boost (56)
- GitHub Check: Ubuntu clang
- GitHub Check: Check queries
🔇 Additional comments (9)
pgtap/dijkstra/driving_distance/inner_query.pg (2)
23-23: Double-check the test plan logic.
UsingNOT min_version('3.6.0') AND min_lib_version('5.0.0')to runplan(1)may reduce the original test coverage (108 tests). If this change is intentional, please confirm the desired test scope.
40-40: Confirm the direct call of inner_query.
If this call is part of a planned test, it looks fine. Otherwise, ensure that you capture or log the return set frominner_query()for clarity.pgtap/dijkstra/driving_distance/no_crash_test.pg (1)
23-23: Verify the number of tests executed.
As ininner_query.pg, the conditional logic changes the total test count from 34 to 1 under certain conditions. Confirm if this reduction in test coverage is intentional.pgtap/dijkstra/driving_distance/edge_cases/issue_519.pg (2)
22-22: LGTM! Version check and test plan look good.The version check condition correctly handles both PostgreSQL version and library version compatibility.
44-46: LGTM! Proper deprecation notice.The code correctly handles the deprecated internal function by returning a skip message for version 5.0.0 and above.
src/driving_distance/driving_distance.c (2)
163-165: LGTM! Clear deprecation marking.The code is properly marked for removal in v5 with clear TODO comment.
168-169: LGTM! Function declarations follow standard pattern.The deprecated function declarations follow the standard pattern using PGDLLEXPORT and PG_FUNCTION_INFO_V1.
NEWS.md (1)
Line range hint
88-102: LGTM! Clear documentation of deprecated internal functions.The list of deprecated internal functions is well-documented with complete signatures.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
50-50: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
57-57: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
73-73: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
doc/src/release_notes.rst (1)
Line range hint
106-110: Fix inconsistent backtick usage.Some function signatures have trailing backticks while others don't.
| CREATE OR REPLACE FUNCTION inner_query() RETURNS SETOF TEXT AS | ||
| $BODY$ | ||
| BEGIN | ||
| IF NOT min_version('3.6.0') AND min_lib_version('5.0.0') THEN | ||
| RETURN NEXT skip(1, 'Internal function deprecated on 3.6.0 and removed on 5.0.0'); | ||
| RETURN; | ||
| END IF; | ||
|
|
||
| RETURN QUERY SELECT style_dijkstra('pgr_drivingdistance(', ',2, 1, true)'); | ||
| RETURN QUERY SELECT style_dijkstra('pgr_drivingdistance(', ',ARRAY[2,3], 1, true)'); | ||
| END; | ||
| $BODY$ | ||
| LANGUAGE plpgsql; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider refactoring the conditional logic for clarity.
The nested usage of NOT min_version('3.6.0') AND min_lib_version('5.0.0') can be a bit confusing. You might consider inverting or rewording the condition (e.g., ELSIF structure) for better readability.
| SELECT CASE | ||
| WHEN (min_version('3.6.0')) THEN | ||
| collect_tap( | ||
| set_eq( | ||
| $$SELECT proargnames from pg_proc where proname = 'pgr_drivingdistance'$$, | ||
| $$VALUES | ||
| ('{"","","","directed","seq","depth","start_vid","pred","node","edge","cost","agg_cost"}'::TEXT[]), | ||
| ('{"","","","directed","equicost","seq","depth","start_vid","pred","node","edge","cost","agg_cost"}'::TEXT[]) | ||
| $$), | ||
|
|
||
| IF (min_version('3.6.0')) THEN | ||
| RETURN QUERY | ||
| SELECT set_eq( | ||
| $$SELECT proargnames from pg_proc where proname = 'pgr_drivingdistance'$$, | ||
| $$VALUES | ||
| ('{"","","","directed","seq","depth","start_vid","pred","node","edge","cost","agg_cost"}'::TEXT[]), | ||
| ('{"","","","directed","equicost","seq","depth","start_vid","pred","node","edge","cost","agg_cost"}'::TEXT[]) | ||
| $$); | ||
| set_eq( | ||
| $$SELECT proallargtypes from pg_proc where proname = 'pgr_drivingdistance'$$, | ||
| $$VALUES | ||
| ('{25,20,701,16,20,20,20,20,20,20,701,701}'::OID[]), | ||
| ('{25,2277,701,16,16,20,20,20,20,20,20,701,701}'::OID[]) | ||
| $$)) | ||
| ELSE | ||
| collect_tap( | ||
| set_eq( | ||
| $$SELECT proargnames from pg_proc where proname = 'pgr_drivingdistance'$$, | ||
| $$VALUES | ||
| ('{"","","","directed","seq","node","edge","cost","agg_cost"}'::TEXT[]), | ||
| ('{"","","","directed","equicost","seq","from_v","node","edge","cost","agg_cost"}'::TEXT[]) | ||
| $$), | ||
|
|
||
| RETURN QUERY | ||
| SELECT set_eq( | ||
| $$SELECT proallargtypes from pg_proc where proname = 'pgr_drivingdistance'$$, | ||
| $$VALUES | ||
| ('{25,20,701,16,20,20,20,20,20,20,701,701}'::OID[]), | ||
| ('{25,2277,701,16,16,20,20,20,20,20,20,701,701}'::OID[]) | ||
| $$); | ||
| ELSE | ||
| -- old signatures | ||
| RETURN QUERY | ||
| SELECT set_eq( | ||
| $$SELECT proargnames from pg_proc where proname = 'pgr_drivingdistance'$$, | ||
| $$VALUES | ||
| ('{"","","","directed","seq","node","edge","cost","agg_cost"}'::TEXT[]), | ||
| ('{"","","","directed","equicost","seq","from_v","node","edge","cost","agg_cost"}'::TEXT[]) | ||
| $$); | ||
|
|
||
| RETURN QUERY | ||
| SELECT set_eq( | ||
| $$SELECT proallargtypes from pg_proc where proname = 'pgr_drivingdistance'$$, | ||
| $$VALUES | ||
| ('{25,20,701,16,23,20,20,701,701}'::OID[]), | ||
| ('{25,2277,701,16,16,23,20,20,20,701,701}'::OID[]) | ||
| $$); | ||
| END IF; | ||
| set_eq( | ||
| $$SELECT proallargtypes from pg_proc where proname = 'pgr_drivingdistance'$$, | ||
| $$VALUES | ||
| ('{25,20,701,16,23,20,20,701,701}'::OID[]), | ||
| ('{25,2277,701,16,16,23,20,20,20,701,701}'::OID[]) | ||
| $$)) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unify version checks with other files for consistency.
Here, the condition uses min_version('3.6.0') directly, while other files use a more complex compound check. Adopting a similar pattern throughout might enhance maintainability and readability.
| IF NOT min_version('3.6.0') AND min_lib_version('5.0.0') THEN | ||
| RETURN NEXT skip(1, 'Internal function deprecated on 3.6.0 and removed on 5.0.0'); | ||
| RETURN; | ||
| END IF; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use an ELSIF block for more direct readability.
The condition to skip tests if the function is deprecated is correct, but inverting or restructuring the condition could make the code flow clearer.
| ereport(NOTICE, ( | ||
| errcode(ERRCODE_FEATURE_NOT_SUPPORTED), | ||
| errmsg("A stored procedure is using deprecated C internal function '%s'", __func__), | ||
| errdetail("Library function '%s' was deprecated in pgRouting %s", __func__, "3.6.0"), | ||
| errhint("Consider upgrade pgRouting"))); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using a macro for deprecation notices.
The deprecation notice is hardcoded. Consider creating a macro for consistent deprecation messages across the codebase.
+#define DEPRECATED_IN_5_0(func) \
+ ereport(NOTICE, ( \
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED), \
+ errmsg("A stored procedure is using deprecated C internal function '%s'", func), \
+ errdetail("Library function '%s' was deprecated in pgRouting %s", func, "3.6.0"), \
+ errhint("Consider upgrade pgRouting")))
PGDLLEXPORT Datum
_pgr_drivingdistance(PG_FUNCTION_ARGS) {
- ereport(NOTICE, (
- errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("A stored procedure is using deprecated C internal function '%s'", __func__),
- errdetail("Library function '%s' was deprecated in pgRouting %s", __func__, "3.6.0"),
- errhint("Consider upgrade pgRouting")));
+ DEPRECATED_IN_5_0(__func__);Committable suggestion skipped: line range outside the PR's diff.
Removal of several deprecated functions
@pgRouting/admins
Summary by CodeRabbit
pgRouting v4.0.0 Release Notes
Deprecation and Removal
_pgr_dijkstra,_pgr_trsp, and_pgr_drivingDistanceFunction Promotions
pgr_trsppgr_withPointspgr_withPointsDDpgr_withPointsKSPpgr_aStar,pgr_dijkstraSignature Updates