Issue 2770 improve internal functions #2771
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request updates the release documentation and SQL error handling for pgRouting functions in version 3.8.0. The placeholder texts in Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🪧 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 (
|
- Internal functions - _pgr_checkquery - _pgr_checkcolumn - re-throw from - pgr_extractVertices - pgr_findCloseEdges - pgr_degree - Adding tests for missing column and relation not found - (pgtap) adjusting tests because of changes on internal SQL - Documentation of the changes
cvvergara
force-pushed
the
issue-2770-improve-internal-functions
branch
from
87ee47d to
373dd66
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
sql/utilities/findCloseEdges.sql (1)
90-100: 🧹 Nitpick (assertive)Clarify Dynamic Query Construction:
In the first function’s body there are multiple branches for constructing the dynamic SQL query depending on whether partial calculation is enabled and whether cap equals 1. For maintainability, consider adding a brief inline comment before each branch (for example, “-- Branch for cap=1 partial mode” above the first concatenation) so that future maintainers can easily follow the logic.
🛑 Comments failed to post (4)
locale/pot/pgrouting_doc_strings.pot (1)
3931-3943: 🧹 Nitpick (assertive)
Consider improving the phrasing of the new doc strings.
“Changes on proposed functions” could be rephrased as “Changes to proposed functions” for clearer English usage. Similarly, “Error messages adjustment.” might be more natural as “Error message adjustments.”
-msgid "Changes on proposed functions" +msgid "Changes to proposed functions" -msgid "Error messages adjustment." +msgid "Error message adjustments."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.msgid "Changes to proposed functions" msgstr "" msgid "pgr_extractVertices" msgstr "" msgid "Error message adjustments." msgstr "" msgid "pgr_findCloseEdges" msgstr "" msgid "pgr_degree"NEWS.md (1)
11-11: 🧹 Nitpick (assertive)
Suggestion: Use a proper markdown heading
Instead of using emphasis (**Changes on proposed functions**) to denote the section title, consider using a proper markdown heading (e.g.,### Changes on proposed functions) to comply with markdown linting rules (MD036).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
11-11: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
pgtap/topology/extractVertices/edge_cases.pg (1)
29-31: 🧹 Nitpick (assertive)
Refactored to use a parameterized prepared statement.
The introduction of a parameterized prepared statement
test_1reduces code duplication and improves maintainability by allowing dynamic column selection in the test queries.sql/topology/extractVertices.sql (1)
56-64:
⚠️ Potential issueEnhanced error handling with diagnostics.
The addition of a nested BEGIN...END block to capture exceptions improves error reporting by retrieving and forwarding diagnostic information from the underlying function.
However, the error message "FOO %" appears to be a placeholder that should be replaced with a meaningful message. Consider removing "FOO " and just using the SQLERRM directly:
- RAISE EXCEPTION 'FOO %', SQLERRM USING HINT = sqlhint, ERRCODE = SQLSTATE; + RAISE EXCEPTION '%', SQLERRM USING HINT = sqlhint, ERRCODE = SQLSTATE;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.sqlhint TEXT; BEGIN BEGIN edges_sql := _pgr_checkQuery($1); EXCEPTION WHEN OTHERS THEN GET STACKED DIAGNOSTICS sqlhint = PG_EXCEPTION_HINT; RAISE EXCEPTION '%', SQLERRM USING HINT = sqlhint, ERRCODE = SQLSTATE; END;
There was a problem hiding this comment.
Actionable comments posted: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
NEWS.md(1 hunks)doc/src/release_notes.rst(1 hunks)doc/topology/pgr_degree.rst(1 hunks)doc/topology/pgr_extractVertices.rst(1 hunks)doc/utilities/pgr_findCloseEdges.rst(1 hunks)locale/en/LC_MESSAGES/pgrouting_doc_strings.po(3 hunks)locale/pot/pgrouting_doc_strings.pot(3 hunks)pgtap/topology/degree/edge_cases.pg(3 hunks)pgtap/topology/extractVertices/edge_cases.pg(1 hunks)pgtap/utilities/findCloseEdges/edge_cases.pg(3 hunks)sql/common/_checkcolumn.sql(1 hunks)sql/common/_checkquery.sql(1 hunks)sql/topology/degree.sql(1 hunks)sql/topology/extractVertices.sql(3 hunks)sql/utilities/findCloseEdges.sql(3 hunks)tools/testers/general_pgtap_tests.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
NEWS.md (1)
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 (1)
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.
locale/en/LC_MESSAGES/pgrouting_doc_strings.po (1)
undefined
<retrieved_learning>
Learnt from: cvvergara
PR: #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:
- SQL level - removal/addition of function signatures
- C/C++ level - deprecation/changes to the underlying implementation
These are documented separately in the release notes.
</retrieved_learning>
🪛 markdownlint-cli2 (0.17.2)
NEWS.md
11-11: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (31)
locale/pot/pgrouting_doc_strings.pot (3)
11-11: Update looks good.The POT creation date has been updated to reflect the recent changes.
3931-3943: Documentation properly reflects the PR's objectives.These new entries in the release notes appropriately document the changes made to error handling for the specified functions (
pgr_extractVertices,pgr_findCloseEdges, andpgr_degree), which aligns perfectly with the PR objectives of enhancing error handling and documentation.
8728-8730: Version tag looks good.Adding the version information (3.8.0) for the
pgr_degreefunction provides clear documentation about when this function was introduced or modified, which helps users understand compatibility requirements.locale/en/LC_MESSAGES/pgrouting_doc_strings.po (2)
11-11: Ensure the correctness of the creation date.
The updatedPOT-Creation-Dateline references a 2025 date and time. Please verify that this accurately reflects the actual pot file generation or intended release timeline for version 3.8.0.
10215-10217: Verify consistency of version references.
Themsgid "Version 3.8.0"line is newly added, matching the updated release version. Ensure it aligns with any other version references throughout the documentation, especially if future version bumps occur before release.doc/src/release_notes.rst (1)
42-61: Centralize and Reference Updated Function Changes
The addition of the "Changes on proposed functions" section is a clear improvement. It aggregates the modifications for pgr_extractVertices, pgr_findCloseEdges, and pgr_degree in one place. Ensure that the include directives (with the proper :start-after: and :end-before: delimiters) capture exactly the intended updated content from the respective documentation files.sql/utilities/findCloseEdges.sql (2)
48-67: Consistent Error Handling for Single-Point Queries
The addedsqlhintvariable and the newBEGIN...EXCEPTIONblock (lines 60–67) ensure that any errors arising from the execution of _pgr_checkQuery and the subsequent column checks are captured with diagnostic details. UsingGET STACKED DIAGNOSTICSto retrievePG_EXCEPTION_HINTprovides useful context when re-raising the exception. This update makes the error reporting more robust and should aid debugging.
174-181: Robust Error Handling for Array-Based Input
Similarly, in the array version of the function (lines 174–181), the consistent pattern of capturingsqlhintviaGET STACKED DIAGNOSTICSand re-raising the error ensures uniform error handling across both function variants. This approach will help users diagnose issues with the provided SQL.doc/utilities/pgr_findCloseEdges.rst (1)
23-29: Documentation version update looks goodThe documentation has been properly updated to include version 3.8.0 with "Error messages adjustment" note while preserving the history of the function being introduced in version 3.4.0. This maintains proper version history while documenting the latest changes.
tools/testers/general_pgtap_tests.sql (2)
33-41: New column_missing function improves test consistencyGood implementation of a helper function that standardizes error handling tests for missing columns. The version check ensures compatibility with the new error format in 3.8.0 while maintaining backward compatibility.
43-51: New wrong_relation function enhances error testingThis helper function properly standardizes error handling tests for non-existent relations across different versions. The implementation correctly uses different error codes and messages based on the PostgreSQL version.
pgtap/utilities/findCloseEdges/edge_cases.pg (3)
23-23: Test plan count updated correctlyThe number of planned tests has been increased from 12 to 16 to account for the new test cases being added.
63-71: Improved error testing for single point queriesThe tests now use the new helper functions for more consistent error checking, and additional test cases for relation errors have been added. This provides better test coverage for error conditions.
90-98: Enhanced error testing for array of points queriesSimilar to the single point case, the tests for array of points now use the standardized error functions and include additional test cases for relation errors. This ensures consistent error handling across different function signatures.
sql/topology/degree.sql (2)
45-59: Improved error handling with proper error propagationThe implementation now properly captures PostgreSQL diagnostics and re-throws exceptions with the original error message and the SQL hint. This helps users understand the context of the error more clearly and provides better debugging information.
64-66: Added validation for required columnsNew validation ensures that at least one of the required edge columns (
in_edgesorout_edges) exists before proceeding. This prevents silent failures and provides a clear error message with the appropriate SQL hint.sql/common/_checkquery.sql (1)
37-48: Check for potential injection handling when differentiating prepared statement names vs. raw SQL.
The condition$1 !~ '[[:space:]]'treats$1as a prepared statement name and otherwise treats it as SQL. Ensure your caller sanitizes$1to avoid unexpected SQL injection risks, especially if$1comes from untrusted inputs.pgtap/topology/extractVertices/edge_cases.pg (6)
29-30: Check input safety for dynamically built statements.
Building a query string viaSELECT ' || $1 || ' FROM edges'can be safe in controlled tests, but consider validating$1in case external inputs slip in.
32-33: Column-missing checks look appropriate.
Usingcolumn_missingis a good approach to test for absent columns.
37-38: Consistent use ofcolumn_missingfor geometry-based checks.
This helps ensure that the geometry attributes (startpoint, endpoint) are properly validated.
42-43: Positive test coverage forgeomcolumn presence.
Tests passing vialives_okconfirm that the geometry column is indeed accessible.
47-48: Good coverage of geometry variants (startpoint, endpoint) undergeom.
These tests help confirm that the presence of additional columns doesn't break extraction.
52-64: Validate that the expected vertex count (17) remains accurate.
These tests hardcode a vertex count of 17, presumably matching your sample dataset. Confirm no changes in the underlying data alter these results.pgtap/topology/degree/edge_cases.pg (5)
23-23: Ensure plan count consistency across PostgreSQL versions.
Raising from plan(12) to plan(16) can break CI if untracked tests are introduced. Verify that all 16 tests are indeed covered.
62-64: Column checks onquery_2,query_3, andquery_4are clear.
Usingcolumn_missingfor specific columns likeidorin_edgesclarifies missing-column errors.
67-68: Validate unknown relations usingwrong_relation.
This explicit check is helpful for user feedback when referencing missing tables.
95-97: Consistent approach for subedges checks withcolumn_missing.
Parallel checks confirm that missing columns on partial-edge sets are correctly handled.
101-117: Empty vertices & empty graph checks enhance edge-case coverage.
The conditional logic formin_version('3.8.0')captures differences in output between new and older versions, improving test coverage on boundary scenarios.sql/topology/extractVertices.sql (3)
47-57: Additions to variable declarations for enhanced error handling and column checking.New boolean variables (
has_st,has_target,has_start,has_end) and thesqlhintvariable have been introduced to support more granular error reporting. These additions align with the PR objective of improved error handling. Please double-check that the default values (set toTRUE) are semantically correct in all cases.
234-262: Handling query construction when source/target columns are present with an ID.A new branch has been added to construct the query when the condition
has_st AND has_idis true. Ensure that the use of the newly definedhas_st(evaluated ashas_source AND has_target) does not inadvertently overlap with the earlier branches (which handle thehas_geomandhas_pointsconditions). Verifying the branch ordering will ensure that the correct query gets executed based on the provided columns.
264-282: Handling query construction when source/target columns are present without an ID.A new branch,
ELSIF has_st AND NOT has_id, was introduced to handle the case when the IDs are not available. As with the previous branch, please verify that the overall conditional structure (i.e., ordering across branches) covers all use cases without unintended overlaps, ensuring that the final query selection logic is unambiguous.
cvvergara
force-pushed
the
issue-2770-improve-internal-functions
branch
from
67f44e9 to
695af1e
Compare
Fixes #2770
Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit
pgr_extractVertices,pgr_findCloseEdges, andpgr_degree.