Skip to content

Conversation

@cvvergara
Copy link
Member

@cvvergara cvvergara commented Feb 7, 2025

Fixes #2745

Changes proposed in this pull request:

  • SQL removal
  • C NOTICE raised about deprecation

@coderabbitai review

Summary by CodeRabbit

  • New Features
    • Official release of enhanced routing and pathfinding capabilities, providing a more robust and diverse set of operations for users.
  • Refactor
    • Streamlined the library by removing outdated and deprecated functions, along with cleaning up unused internal code to boost performance.
  • Documentation
    • Updated release notes now clearly reflect these improvements and offer current guidance on the expanded functionality in pgRouting 4.0.0.

@pgRouting/admins

* SQL removal
* C NOTICE raised about deprecation
@cvvergara cvvergara added Deprecation kruskal Remove rm of deprecated function or internal function labels Feb 7, 2025
@cvvergara cvvergara added this to the Release 4.0.0 milestone Feb 7, 2025
@cvvergara cvvergara requested a review from robe2 February 7, 2025 19:14
@cvvergara cvvergara self-assigned this Feb 7, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 7, 2025

Walkthrough

The pull request updates pgRouting for the 4.0.0 release by promoting various functions and signatures (e.g., pgr_trsp, pgr_aStar) to official status while removing several deprecated SQL and internal functions—including multiple variants of pgr_trsp and the internal _pgr_kruskal. The changes also document enhancements in release notes and adjust function signatures in SQL files. Additionally, the C implementation now issues a deprecation notice when _pgr_kruskal is invoked.

Changes

File(s) Change Summary
NEWS.md, doc/src/release_notes.rst Updated documentation to reflect promotion of functions/signatures and removal/deprecation of outdated SQL and internal functions.
sql/sigs/pgrouting--4.0.sig Modified function signatures: removed the _pgr_kruskal signature, updated _pgr_ksp with an extra boolean parameter, and added new functions like pgr_withpointsdd.
sql/spanningTree/_kruskal.sql Removed the legacy _pgr_kruskal function and added the new _pgr_kruskalv4 function with a modified parameter list and output structure.
src/spanningTree/kruskal.c Added a deprecation notice via an ereport call in _pgr_kruskal; adjusted internal processing (e.g., reduced array size) while retaining the legacy function structure.

Sequence Diagram(s)

sequenceDiagram
  participant Q as Query Caller
  participant K as _pgr_kruskal (C Function)
  participant L as Log System
  Q->>K: Call _pgr_kruskal(...)
  K->>L: Emit deprecation notice
  K->>K: Process legacy logic
  K-->>Q: Return result
Loading

Assessment against linked issues

Objective Addressed Explanation
Remove deprecated SQL function _pgr_kruskal and add deprecation notice in C code [#2745]

Possibly related PRs

Suggested labels

Prim

Suggested reviewers

  • robe2

Poem

In my warren of code, I hop with glee,
Deprecated functions fall like leaves from a tree.
New signatures bloom in the digital light,
A notice rings out—deprecated functions take flight.
Hop along with me, in routes clear and bright,
A rabbit's cheer for pgRouting done right!


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1fa5da4 and 6552cce.

📒 Files selected for processing (5)
  • NEWS.md (2 hunks)
  • doc/src/release_notes.rst (2 hunks)
  • sql/sigs/pgrouting--4.0.sig (0 hunks)
  • sql/spanningTree/_kruskal.sql (0 hunks)
  • src/spanningTree/kruskal.c (2 hunks)
💤 Files with no reviewable changes (2)
  • sql/sigs/pgrouting--4.0.sig
  • sql/spanningTree/_kruskal.sql
🧰 Additional context used
📓 Learnings (4)
📓 Common learnings
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.
src/spanningTree/kruskal.c (1)
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
NEWS.md (2)
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.
doc/src/release_notes.rst (2)
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
src/spanningTree/kruskal.c (3)

173-177: LGTM! Clear TODO comment for future planning.

The TODO comment clearly indicates the plan to move this code to legacy in v5.


189-193: LGTM! Proper deprecation notice implementation.

The deprecation notice follows the standard format using ereport with ERRCODE_FEATURE_NOT_SUPPORTED, providing clear information about:

  • Which function is deprecated
  • When it was deprecated (version 3.7.0)
  • What action users should take (upgrade pgRouting)

235-236: Verify the impact of reducing the values array size.

The number of values has been reduced from 8 to 7. This change needs verification to ensure it doesn't break existing functionality.

✅ Verification successful

🏁 Script executed:

#!/bin/bash
# Description: Check if the values array size reduction is safe by looking for any code that expects 8 values.

# Search for any references to 8 values in tests or related code
rg -A 5 "_pgr_kruskal.*8"

# Search for any test expectations that might be affected
rg -A 5 "kruskal.*test.*expect"

Length of output: 61


Let's gather a bit more evidence regarding the usage of the values array in "src/spanningTree/kruskal.c" to ensure no code is explicitly expecting 8 elements. I'll generate additional shell scripts to search for any hard-coded index or loop conditions that refer to 8 (which may indicate an expectation to have 8 slots).


🏁 Script executed:

#!/bin/bash
# Search for any references in kruskal.c where the values array is accessed with index 8.
rg "values\\[\\s*8\\s*\\]" -n src/spanningTree/kruskal.c

# Search for loop conditions that explicitly use the literal 8 (which might indicate an expectation of 8 values).
rg "for\\s*\\(.*<\\s*8\\s*\\)" -n src/spanningTree/kruskal.c

Length of output: 112


Verification: Reduction from 8 to 7 Elements Appears Safe

After an additional review of the code in src/spanningTree/kruskal.c, there were no findings of any hard-coded usages, loop conditions, or test expectations referencing an 8‑element array. All direct checks (such as accesses to index 8 or loops with a bound of 8) produced no output, implying that the code now appropriately uses the allocated 7 elements of the values array.

• No instances were found that explicitly expect an 8‑element array.
• Loop conditions and direct array accesses are consistent with the new allocation size.
• There is no evidence from test searches or code references suggesting that functionality is broken by this change.

Based on this investigation, the reduction of the values array size from 8 to 7 does not appear to adversely impact functionality.

NEWS.md (2)

63-63: LGTM! Correctly documented SQL signature removal.

The _pgr_kruskal function is properly added to the "Removal of SQL deprecated internal functions" section.


79-79: LGTM! Correctly documented C/C++ implementation deprecation.

The _pgr_kruskal function is properly added to the "Deprecation of internal C/C++ functions" section.

doc/src/release_notes.rst (2)

94-94: LGTM! Correctly documented SQL signature removal.

The _pgr_kruskal function is properly added to the "Removal of SQL deprecated internal functions" section.


110-110: LGTM! Correctly documented C/C++ implementation deprecation.

The _pgr_kruskal function is properly added to the "Deprecation of internal C/C++ functions" section.


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@cvvergara
Copy link
Member Author

@coderabbitai review
@coderabbitai summary

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 7, 2025

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the PR diff
gh pr diff 2746

# Search for files containing kruskal
rg -l "kruskal" --type cpp --type sql

# Get NEWS.md content
cat NEWS.md

Length of output: 56925


@cvvergara The changes look good. The PR implements:

  1. Removal of deprecated SQL signature _pgr_kruskal
  2. Deprecation notice in the C implementation
  3. Documentation updates in NEWS.md

🧠 Learnings used
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.
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cvvergara cvvergara requested a review from sanak February 11, 2025 02:07
Copy link
Member

@sanak sanak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cvvergara cvvergara merged commit 0665496 into pgRouting:develop Feb 11, 2025
26 checks passed
@cvvergara cvvergara deleted the remove-deprecated-internal-function-_pgr_kruskal branch February 11, 2025 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Function/Internal kruskal Remove rm of deprecated function or internal function

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants