Remove deprecated signatures of pgr_withPointsKSP #2896
Conversation
cvvergara
added
withPoints
KSP
Remove
|
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 WalkthroughThis change removes deprecated SQL signatures and functions related to Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SQL_API
participant C_Functions
participant Internal_v4
User->>SQL_API: Call pgr_withPointsKSP(...)
SQL_API->>Internal_v4: Call _pgr_withPointsKSP_v4(...)
Internal_v4->>C_Functions: Execute C SRF logic
C_Functions-->>Internal_v4: Return path tuples
Internal_v4-->>SQL_API: Return results
SQL_API-->>User: Return final result set
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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 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: 8
🔭 Outside diff range comments (3)
sql/ksp/_withPointsKSP.sql (2)
32-55: 🛠️ Refactor suggestion
new_kspIN parameter is defined in SQL but silently ignored in CThe first
_pgr_withPointsKSP_v4variant advertises four BOOLEAN inputs (directed, heaps, details, new_ksp).
The backing C routine (see withPoints_ksp.c) consumes only the first three booleans; the 4ᵗʰ is never read, so its value is lost and callers get no feedback.Options:
- Drop the unused parameter from the SQL declaration.
- Wire the argument through to
process()(or at least read it in C so that call sites don’t mis-count).Failing to do either will break
CREATE EXTENSION UPDATEchecks and is confusing for users.
59-83: 🧹 Nitpick (assertive)Keep both overloads consistent
The second overload (with
combinationsTEXT) exposes only 3 BOOLEANs, causing an asymmetric API.
Ifnew_kspreally matters, add it here too; if not, remove it from the first overload so that callers do not have to remember two different arities for the same logical option set.src/ksp/withPoints_ksp.c (1)
66-70: 🧹 Nitpick (assertive)Zero-length
ksilently accepted
process()rejects only negative k (p_k < 0).
k = 0produces no paths but still allocates and pushes an SPI plan; it may be clearer (and cheaper) to treat 0 as an error early.if (p_k <= 0) { pgr_throw_error("Parameter k must be positive", NULL); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
NEWS.md(2 hunks)doc/src/release_notes.rst(2 hunks)locale/en/LC_MESSAGES/pgrouting_doc_strings.po(3 hunks)locale/pot/pgrouting_doc_strings.pot(3 hunks)pgtap/ksp/withPointsKSP/edge_cases/point_in_vertex.pg(1 hunks)pgtap/ksp/withPointsKSP/inner_query.pg(2 hunks)pgtap/ksp/withPointsKSP/no_crash_test.pg(2 hunks)pgtap/ksp/withPointsKSP/types_check.pg(2 hunks)sql/ksp/_withPointsKSP.sql(3 hunks)sql/ksp/withPointsKSP.sql(5 hunks)sql/scripts/build-extension-update-files.pl(0 hunks)sql/sigs/pgrouting--4.0.sig(1 hunks)src/ksp/withPoints_ksp.c(2 hunks)tools/testers/general_pgtap_tests.sql(1 hunks)
💤 Files with no reviewable changes (1)
- sql/scripts/build-extension-update-files.pl
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: cvvergara
PR: pgRouting/pgrouting#2744
File: NEWS.md:63-63
Timestamp: 2025-02-06T20:50:00.577Z
Learning: In pgRouting's NEWS.md, the "Removal of SQL deprecated internal functions" section lists SQL signatures being removed, while "Deprecation of internal C/C++ functions" section lists the underlying C/C++ implementations being deprecated. A function can appear in both sections when its SQL signature is removed while its C/C++ implementation is deprecated.
Learnt from: cvvergara
PR: pgRouting/pgrouting#2744
File: doc/src/release_notes.rst:94-94
Timestamp: 2025-02-06T20:50:07.967Z
Learning: In pgRouting, changes to functions can happen at two levels:
1. SQL level - removal/addition of function signatures
2. C/C++ level - deprecation/changes to the underlying implementation
These are documented separately in the release notes.
Learnt from: cvvergara
PR: pgRouting/pgrouting#2725
File: src/coloring/edgeColoring_driver.cpp:87-87
Timestamp: 2025-01-19T16:17:53.496Z
Learning: Changes to error message handling in pgRouting functions require corresponding updates to pgtap tests to maintain test coverage.
NEWS.md (3)
Learnt from: cvvergara
PR: pgRouting/pgrouting#2744
File: NEWS.md:63-63
Timestamp: 2025-02-06T20:50:00.577Z
Learning: In pgRouting's NEWS.md, the "Removal of SQL deprecated internal functions" section lists SQL signatures being removed, while "Deprecation of internal C/C++ functions" section lists the underlying C/C++ implementations being deprecated. A function can appear in both sections when its SQL signature is removed while its C/C++ implementation is deprecated.
Learnt from: cvvergara
PR: pgRouting/pgrouting#2744
File: doc/src/release_notes.rst:94-94
Timestamp: 2025-02-06T20:50:07.967Z
Learning: In pgRouting, changes to functions can happen at two levels:
1. SQL level - removal/addition of function signatures
2. C/C++ level - deprecation/changes to the underlying implementation
These are documented separately in the release notes.
Learnt from: cvvergara
PR: pgRouting/pgrouting#0
File: :0-0
Timestamp: 2025-01-27T01:49:25.212Z
Learning: In pgRouting, when deprecating internal functions:
1. Add deprecation notice in C code using ereport with ERRCODE_FEATURE_NOT_SUPPORTED
2. Update tests with version checks (min_version, min_lib_version)
3. Document changes in NEWS.md under appropriate sections
sql/ksp/_withPointsKSP.sql (1)
Learnt from: cvvergara
PR: pgRouting/pgrouting#2764
File: doc/withPoints/pgr_withPoints.rst:30-35
Timestamp: 2025-02-27T16:42:23.807Z
Learning: For pgr_withPoints, the original function was proposed in version 2.2.0, while the Combinations signature overload was added in version a3.2.0.
doc/src/release_notes.rst (2)
Learnt from: cvvergara
PR: pgRouting/pgrouting#2744
File: doc/src/release_notes.rst:94-94
Timestamp: 2025-02-06T20:50:07.967Z
Learning: In pgRouting, changes to functions can happen at two levels:
1. SQL level - removal/addition of function signatures
2. C/C++ level - deprecation/changes to the underlying implementation
These are documented separately in the release notes.
Learnt from: cvvergara
PR: pgRouting/pgrouting#2744
File: NEWS.md:63-63
Timestamp: 2025-02-06T20:50:00.577Z
Learning: In pgRouting's NEWS.md, the "Removal of SQL deprecated internal functions" section lists SQL signatures being removed, while "Deprecation of internal C/C++ functions" section lists the underlying C/C++ implementations being deprecated. A function can appear in both sections when its SQL signature is removed while its C/C++ implementation is deprecated.
sql/sigs/pgrouting--4.0.sig (4)
Learnt from: cvvergara
PR: pgRouting/pgrouting#2764
File: sql/sigs/pgrouting--3.8.sig:125-136
Timestamp: 2025-02-27T16:33:56.959Z
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.
Learnt from: cvvergara
PR: pgRouting/pgrouting#2764
File: sql/sigs/pgrouting--3.8.sig:274-275
Timestamp: 2025-02-27T16:34:57.518Z
Learning: The file `sql/sigs/pgrouting--3.8.sig` is auto-generated by a command and should not be modified manually.
Learnt from: cvvergara
PR: pgRouting/pgrouting#2744
File: doc/src/release_notes.rst:94-94
Timestamp: 2025-02-06T20:50:07.967Z
Learning: In pgRouting, changes to functions can happen at two levels:
1. SQL level - removal/addition of function signatures
2. C/C++ level - deprecation/changes to the underlying implementation
These are documented separately in the release notes.
Learnt from: cvvergara
PR: pgRouting/pgrouting#2744
File: NEWS.md:63-63
Timestamp: 2025-02-06T20:50:00.577Z
Learning: In pgRouting's NEWS.md, the "Removal of SQL deprecated internal functions" section lists SQL signatures being removed, while "Deprecation of internal C/C++ functions" section lists the underlying C/C++ implementations being deprecated. A function can appear in both sections when its SQL signature is removed while its C/C++ implementation is deprecated.
locale/pot/pgrouting_doc_strings.pot (1)
undefined
<retrieved_learning>
Learnt from: cvvergara
PR: #2744
File: NEWS.md:63-63
Timestamp: 2025-02-06T20:50:00.577Z
Learning: In pgRouting's NEWS.md, the "Removal of SQL deprecated internal functions" section lists SQL signatures being removed, while "Deprecation of internal C/C++ functions" section lists the underlying C/C++ implementations being deprecated. A function can appear in both sections when its SQL signature is removed while its C/C++ implementation is deprecated.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Ubuntu Boost (84)
- GitHub Check: macos (macos-latest)
- GitHub Check: Ubuntu Boost (79)
- GitHub Check: macos (macos-14)
- GitHub Check: Ubuntu Boost (86)
- GitHub Check: Ubuntu Boost (83)
- GitHub Check: Ubuntu Boost (68)
- GitHub Check: Ubuntu Boost (80)
- GitHub Check: Ubuntu Boost (78)
- GitHub Check: Ubuntu Boost (75)
- GitHub Check: Ubuntu Boost (77)
- GitHub Check: Ubuntu Boost (56)
- GitHub Check: Ubuntu Boost (76)
- GitHub Check: Ubuntu clang
🔇 Additional comments (20)
tools/testers/general_pgtap_tests.sql (1)
116-116: Improved test message clarityThe message string has been updated from "Function args names" to "Function has args names" to more clearly indicate the purpose of the test.
pgtap/ksp/withPointsKSP/edge_cases/point_in_vertex.pg (1)
32-35: Updated version check logic to focus on 4.0.0 signaturesThe skip message has been updated to indicate that only valid signatures for version 4.0.0 are being tested, aligning with the removal of deprecated signatures in this PR.
sql/ksp/withPointsKSP.sql (5)
54-54: Updated function to use new _v4 implementationThe function call has been updated to use
_pgr_withPointsKSP_v4instead of_pgr_withPointsKSP, which aligns with the objective of removing deprecated functions and signatures.
86-86: Updated function to use new _v4 implementationThe function call has been updated to use
_pgr_withPointsKSP_v4instead of_pgr_withPointsKSP, which aligns with the objective of removing deprecated functions and signatures.
118-118: Updated function to use new _v4 implementationThe function call has been updated to use
_pgr_withPointsKSP_v4instead of_pgr_withPointsKSP, which aligns with the objective of removing deprecated functions and signatures.
150-150: Updated function to use new _v4 implementationThe function call has been updated to use
_pgr_withPointsKSP_v4instead of_pgr_withPointsKSP, which aligns with the objective of removing deprecated functions and signatures.
181-181: Updated function to use new _v4 implementationThe function call has been updated to use
_pgr_withPointsKSP_v4instead of_pgr_withPointsKSP, which aligns with the objective of removing deprecated functions and signatures.pgtap/ksp/withPointsKSP/inner_query.pg (3)
23-23: Updated test plan for version conditionalsThe test plan has been updated to run 432 tests when version is >= 3.6.0 but only 1 test otherwise, which is consistent with the PR objective of focusing tests on current function signatures.
29-32: Added early return for non-supported versionsAdded a conditional check to skip all tests and return early with a message about testing only valid signatures on 4.0.0 if the PostgreSQL version is below 3.6.0. This aligns with the removal of deprecated signatures in this PR.
50-50: Removed tests for deprecated signaturesRemoved code that was previously used to test deprecated function signatures, which aligns with the PR objective of cleaning up the codebase by eliminating outdated function signatures.
NEWS.md (1)
73-77: Double-check completeness of the deprecated signature list
pgr_withpointskspexposes five public SQL signatures (the same five that are still asserted in the updated pgTap tests).
Only one of them is currently listed in the “Removal of SQL deprecated signatures” section.
If the intention is to drop all historical signatures, the four variants that takeanyarrayparameters are still missing from the changelog and could become undocumented breaking changes.Please verify the exact set of signatures that were removed in the SQL patch and mirror the same list here to avoid future confusion for users reading the NEWS file.
pgtap/ksp/withPointsKSP/types_check.pg (1)
46-63: Great – positive equality checks introduced for 4.0+Switching from the looser
*_has()to the stricter*_eq()ensures we lock the
signature and result columns for future releases. Nice touch!doc/src/release_notes.rst (1)
103-107: Mirror NEWS.md remark – possible incomplete signature listOnly one
pgr_withpointskspsignature is documented as removed, while four
other historical variants still appear in the pgTap tests.
If more than one public signature was dropped, list them all here to keep the
release notes exhaustive and in sync with the codebase.locale/en/LC_MESSAGES/pgrouting_doc_strings.po (3)
11-11: Timestamp updated appropriately.The POT-Creation-Date has been updated to reflect the recent regeneration of translation templates, which is expected when function signatures are modified.
4040-4046: Documentation additions align with issue #2895 objective.The addition of both the issue reference and the official function signature for
pgr_withpointskspproperly documents the maintained API after removing deprecated signatures, consistent with the PR objectives.
4143-4154: Internal function signatures appropriately documented.The addition of these internal function signatures correctly reflects the supported implementation after removing deprecated versions. This aligns with pgRouting's practice of separately documenting SQL-level changes (function signatures) and C/C++ level implementations as noted in the retrieved learnings.
locale/pot/pgrouting_doc_strings.pot (2)
8-14: Update POT-Creation-Date header for translation template
The updatedPOT-Creation-Date: 2025-05-22 03:02+0000correctly reflects regeneration of the translation strings. Ensure you ranxgettext(or your preferred extraction tool) at this timestamp, and verify that other header fields (e.g.,PO-Revision-Date) remain accurate.
3703-3715:✅ Verification successful
Include internal C function msgids for pgr_withPointsKSP variants
The internal_pgr_withpointsddand the three_pgr_withpointskspvariants have been added. Ensure that any deprecated/internal signatures not matching these current variants have been removed.Use this script to check for stale internal signatures:
🏁 Script executed:
#!/bin/bash # Count internal _pgr_withpointsksp msgids count=$(rg -c '_pgr_withpointsksp' locale/pot/pgrouting_doc_strings.pot) if [ "$count" -ne 3 ]; then echo "Unexpected number of _pgr_withpointsksp entries: $count (expected 3)" exit 1 fi echo "Internal signatures count OK: $count"Length of output: 165
I’ll verify that we only have the expected internal signatures for all pgr_withPoints variants (dd, ksp, via):
🏁 Script executed:
#!/bin/bash # Verify counts for all internal _pgr_withpoints variants dd=$(rg -c 'msgid "_pgr_withpointsdd' locale/pot/pgrouting_doc_strings.pot) ksp=$(rg -c 'msgid "_pgr_withpointsksp' locale/pot/pgrouting_doc_strings.pot) via=$(rg -c 'msgid "_pgr_withpointsvia' locale/pot/pgrouting_doc_strings.pot) if [ "$dd" -ne 1 ] || [ "$ksp" -ne 3 ] || [ "$via" -ne 1 ]; then echo "Unexpected counts: dd=$dd, ksp=$ksp, via=$via (expected 1,3,1)" exit 1 fi echo "All internal _pgr_withpoints signatures count OK: dd=$dd; ksp=$ksp; via=$via"Length of output: 444
Internal pgr_withPoints signatures count OK
Verified that there is exactly 1_pgr_withpointsdd, 3_pgr_withpointsksp, and 1_pgr_withpointsviaentry inlocale/pot/pgrouting_doc_strings.pot. All deprecated/internal signatures not matching these variants have been removed—no further action required.pgtap/ksp/withPointsKSP/no_crash_test.pg (1)
49-50: Confirm function signature after thenew_kspflag addition
has_function('pgr_withpointsksp', ARRAY['text', ... , 'boolean','boolean','boolean'])checks the 9-argument wrapper, but the array–array overload now has 10 input parameters (extranew_ksp).
If you intend to validate only the “departures/destinations” overload, add the extra BOOLEAN, otherwise restrict the check to the 8-arg combinations overload.sql/sigs/pgrouting--4.0.sig (1)
289-290: Implementation of new internal function signatures forwithPointsKSPThese two new function signatures with the
_v4suffix represent the updated internal implementation that replaces multiple deprecated signatures of_pgr_withpointsksp. This aligns with the PR objective to remove deprecated functions related topgr_withPointsKSP.The versioning approach is consistent with other internal functions in pgRouting that use the
_v4suffix (e.g.,_pgr_drivingdistancev4,_pgr_kruskalv4).
- Removing deprecated internal function _pgr_withPointsDD - Removing deprecated C code to legacy and adjusting remaining C code - (pgtap) Testing only valid signatures in 4.0.0 - Keeping the deprecated C code for the duration of v4
cvvergara
force-pushed
the
remove-deprecated-signatures-of-pgr_withPointsKSP
branch
from
c2d13fe to
ed51bdd
Compare
Fixes #2895
Changes proposed in this pull request:
pgr_withPointsKSPSummary by CodeRabbit
pgr_withPointsKSPfunction, providing updated internal logic and extended output columns.@pgRouting/admins