New version not using vertex table #149
Conversation
- Cost Path annotations on the middle of the path - One Path is not used anymore
- Using the many to many signature - One Path is not used anymore
WalkthroughRefactored path geometry to compute endpoints via new pgr query helpers and build lines from those endpoints; draw logic now executes those queries and adds midpoint text annotations. Multiple functions had pgRouting minimum-version and UI control updates; docs, metadata, CRS utility, tests, and build config were updated; drivingDistance removed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Plugin UI
participant FB as FunctionBase.drawCostPaths
participant PgrQ as utilities.pgr_queries
participant DB as PostGIS/pgRouting
participant Map as QGIS Map
User->>UI: Request route rendering
UI->>FB: drawCostPaths(args, results)
FB->>PgrQ: getCostLine(args, departure, arrival)
PgrQ-->>FB: SQL (uses getEndPoint)
FB->>DB: EXECUTE cost line SQL
DB-->>FB: Line geometry (ST_MakeLine of endpoints)
FB->>PgrQ: getMidPoint()
PgrQ-->>FB: midpoint SQL expression
FB->>DB: EXECUTE midpoint query
DB-->>FB: Midpoint point
FB->>Map: Render line and add text annotation (path name, cost)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
functions/pgr_KSP.py (1)
78-81: Fix column indices used for drawing paths.Current indices map to
_path_seq,_end_vid, and_path_id, which breaks edge/node selection and grouping. Use path_name, node, edge:- columns = [2, 4, 5] + columns = [1, 6, 7]
🧹 Nitpick comments (5)
requirements.txt (1)
3-3: Is setuptools required at runtime?QGIS plugins typically don’t need setuptools at runtime. If it’s only for packaging/docs, move it to build/dev requirements to avoid shipping unnecessary deps. If you do need it, consider pinning a minimal version.
help/source/index.rst (1)
1-1: RST title underline length may be mismatched.After changing the title, ensure the underline length matches the title length; otherwise Sphinx emits “Title underline too short”.
Also applies to: 5-6
help/source/conf.py (2)
26-26: Autodoc enabled: ensure import path/installation for builds.With autodoc on, ensure modules are importable during docs build. Either:
- uncomment and set sys.path insertions, or
- install the package in the build env (e.g., RTD config or CI).
49-51: Version set to 4.0 — align plugin metadata.Update
metadata.txtversion=to 4.0.0 to match docs.functions/pgr_KSP.py (1)
39-43: Redundant version check.
FunctionBase.isSupportedVersionalready compares againstminPGRversion. This override duplicates that logic. Consider removing to rely on the base implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
functions/CostBase.py(1 hunks)functions/FunctionBase.py(4 hunks)functions/pgr_KSP.py(2 hunks)help/source/conf.py(3 hunks)help/source/index.rst(1 hunks)metadata.txt(2 hunks)pb_tool.cfg(0 hunks)pgRoutingLayer.py(2 hunks)pgRoutingLayer_utils.py(1 hunks)requirements.txt(1 hunks)utilities/pgr_queries.py(1 hunks)
💤 Files with no reviewable changes (1)
- pb_tool.cfg
🧰 Additional context used
🧬 Code graph analysis (3)
functions/pgr_KSP.py (3)
functions/FunctionBase.py (7)
FunctionBase(39-305)isSupportedVersion(98-100)getQuery(114-115)getExportQuery(118-119)getJoinResultWithEdgeTable(128-145)getExportMergeQuery(122-123)getExportManySourceManyTargetMergeQuery(147-188)functions/AstarBase.py (1)
getQuery(45-58)functions/DijkstraBase.py (3)
getQuery(52-62)getExportQuery(64-65)getExportMergeQuery(67-68)
functions/FunctionBase.py (1)
utilities/pgr_queries.py (2)
getCostLine(81-88)getMidPoint(90-91)
pgRoutingLayer.py (3)
connectors/postgis.py (1)
connect(124-149)dbConnection.py (1)
connect(150-158)utilities/pgr_queries.py (1)
get_closestVertexInfo(93-120)
🪛 LanguageTool
metadata.txt
[grammar] ~11-~11: There might be a mistake here.
Context: ...SP version=3.0.2 qgisMinimumVersion=3.10 qgisMaximumVersion=3.99 author=Anita Gra...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...imumVersion=3.10 qgisMaximumVersion=3.99 author=Anita Graser, Ko Nagase, Vicky Ve...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...gara, Cayetano Benavent, Aasheesh Tiwari email=[email protected] changelog=4....
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...heesh Tiwari email=[email protected] changelog=4.0.0 - Works best for pgR...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...il=[email protected] changelog=4.0.0 - Works best for pgRouting 4.0.0 - N...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...0 - No longer Support of QGIS < 3.10 - Using PyQt5 3.0.1 - Support fo...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...analysis icon=icon.png experimental=True homepage=https://qgis.pgrouting.org/ tra...
(QB_NEW_EN)
🪛 Ruff (0.14.0)
functions/FunctionBase.py
287-287: Local variable midPointstr is assigned to but never used
Remove assignment to unused variable midPointstr
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (8)
utilities/pgr_queries.py (1)
90-92: Midpoint helper LGTM.Returning WKT is fine for annotation placement; keep as-is.
functions/pgr_KSP.py (1)
58-70: Many-to-many KSP call LGTM.Signature and projection (start_vid/end_vid/path_id) look correct for PGR ≥ 3.6.
functions/FunctionBase.py (5)
36-36: LGTM!The import alias addition supports the new query helper usage in
drawCostPaths.
248-248: LGTM!Initializing
resultNodesTextAnnotationsfromcanvasItemList['annotations']properly prepares the annotation storage for the new edge-labeling logic.
250-251: LGTM!Creating separate cursors for the main query and midpoint calculation is appropriate for independent query execution.
268-276: LGTM!The refactoring to use
PgrQ.getCostLinefor geometry construction is clean and leverages the new query helper. The approach of building the line from endpoints is consistent with the broader PR changes.
285-301: LGTM!The edge-labeling logic correctly computes the midpoint and creates a text annotation displaying the path name and cost. The annotation is properly attached to the map canvas and stored in the annotations list.
pgRoutingLayer.py (1)
957-957: LGTM!Changing from
sql.SQL(" -1 ")tosql.Identifier(...)is correct when reverse cost is enabled. This properly references the column identifier from the input field, consistent with how other column fields are handled (see lines 949-952).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
functions/FunctionBase.py (2)
41-41: Verify minimum version consistency across all function modules.The minimum pgRouting version has been updated to 3.0 in this base class, which aligns with the PR objectives. However, as noted in previous reviews, several function modules still declare older versions (2.x) and need to be updated for consistency.
As per the previous review, the following files still need their
minPGRversionupdated to 3.0:
- functions/pgr_aStar.py
- functions/pgr_dijkstraCost.py
- functions/pgr_dijkstra.py
- functions/pgr_bdDijkstraCost.py
- functions/pgr_bdDijkstra.py
- functions/pgr_bdAstarCost.py
- functions/pgr_bdAstar.py
- functions/pgr_aStarCost.py
286-286: Remove the unused variable.The variable
midPointstris assigned but never used, as flagged by static analysis.Apply this diff to remove the unused variable:
# Label the edge midPoint = PgrQ.getMidPoint() - midPointstr = midPoint.as_string(con) midPointCursor.execute(midPoint,(line,)) pointRow = midPointCursor.fetchone()
🧹 Nitpick comments (1)
functions/FunctionBase.py (1)
284-300: LGTM! Text annotations correctly implemented.The edge labeling feature has been properly implemented:
- Calculates the midpoint of each cost line using
PgrQ.getMidPoint()- Creates a
QgsTextAnnotationwith the path name and cost- Positions the annotation at the midpoint with appropriate offset
- Stores the annotation for lifecycle management
This aligns with the PR objective of placing cost line annotations at the midpoint of lines.
Consider modernizing the string formatting to use f-strings for consistency:
- textDocument = QTextDocument("{0!s}:{1}".format(args['result_path_name'], args['result_cost'])) + textDocument = QTextDocument(f"{args['result_path_name']}:{args['result_cost']}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
functions/DijkstraBase.py(1 hunks)functions/FunctionBase.py(4 hunks)functions/pgr_KSP.py(2 hunks)tests/qgis_models.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
functions/pgr_KSP.py (2)
functions/FunctionBase.py (8)
FunctionBase(39-304)isSupportedVersion(98-100)getQuery(114-115)getExportQuery(118-119)getJoinResultWithEdgeTable(128-145)getExportMergeQuery(122-123)getExportManySourceManyTargetMergeQuery(147-188)draw(125-126)functions/DijkstraBase.py (4)
getQuery(52-61)getExportQuery(63-64)getExportMergeQuery(66-67)draw(69-71)
functions/FunctionBase.py (1)
utilities/pgr_queries.py (2)
getCostLine(81-88)getMidPoint(90-91)
🪛 Ruff (0.14.0)
functions/FunctionBase.py
286-286: Local variable midPointstr is assigned to but never used
Remove assignment to unused variable midPointstr
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
tests/qgis_models.py (1)
32-32: LGTM! PyQt5 import aligns with QGIS 3.10+ requirement.The migration from PyQt4 to PyQt5 is correct and necessary for compatibility with QGIS 3.10+, as stated in the PR objectives.
functions/DijkstraBase.py (1)
54-61: LGTM! Query updated for many-to-many signature.The SQL query has been properly updated to work with the many-to-many signature. The removal of
lead(agg_cost) over()window function and direct use ofagg_costis appropriate as pgRouting 3.0+ returns cumulative costs directly from the routing functions.functions/pgr_KSP.py (4)
34-42: LGTM! Version gating correctly implemented.The minimum version is properly set to 3.6 to align with the many-to-many signature requirement for
pgr_KSP. TheisSupportedVersionmethod correctly enforces this constraint.
53-54: LGTM! Control names correctly pluralized.The control names have been appropriately updated to plural forms (
SourceIds,TargetIds) to reflect the many-to-many signature where multiple source and target nodes can be specified.
59-67: LGTM! Query correctly updated for many-to-many signature.The SQL query has been properly updated to:
- Use
start_vid/end_vidfields from the many-to-many signature- Include
path_idin the path name to distinguish multiple K-shortest paths- Accept
source_idsandtarget_idsarrays instead of single values
77-77: LGTM! Column indices corrected.The column indices have been fixed to correctly reference
[_path_seq, _node, _edge]at positions[2, 5, 6]instead of the previous incorrect indices[2, 4, 5]which would have referenced[_path_seq, _end_vid, _node].functions/FunctionBase.py (1)
36-36: LGTM! Import added for query utilities.The import of
pgr_queriesutilities is correctly added to support the new cost line generation and midpoint calculation functionality.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
metadata.txt (1)
15-19: Polish changelog wording (grammar).Fix phrasing for clarity.
Apply this diff:
-changelog=4.0.0 - - Works best for pgRouting 4.0.0 - - No longer Support of QGIS < 3.10 - - Using PyQt5 +changelog=4.0.0 + - Works best with pgRouting 4.0.0 + - Dropped support for QGIS < 3.10 + - Using PyQt5
🧹 Nitpick comments (1)
functions/FunctionBase.py (1)
248-299: Guard against NULL cost lines and reuse cursors (perf/robustness).Avoid per-iteration cursor creation; skip rows with no line result to prevent WKT parse errors.
Apply this diff:
- resultNodesTextAnnotations = canvasItemList['annotations'] - for row in rows: - cursor = con.cursor() - midPointCursor = con.cursor() + resultNodesTextAnnotations = canvasItemList['annotations'] + cursor = con.cursor() + midPointCursor = con.cursor() + for row in rows: args['result_path_id'] = row[0] args['result_source_id'] = sql.Literal(row[1]) args['result_target_id'] = sql.Literal(row[2]) args['result_cost'] = row[3] args['result_path_name'] = row[4] @@ - if args['result_cost'] != -1: + if args['result_cost'] != -1: costLine = PgrQ.getCostLine(args, sql.Literal(row[1]), sql.Literal(row[2])) # Utils.logMessage(costLine.as_string(cursor)) - cursor.execute(costLine) - row2 = cursor.fetchone() - line = str(row2[0]) + cursor.execute(costLine) + row2 = cursor.fetchone() + if not row2 or row2[0] is None: + continue + line = str(row2[0]) # Utils.logMessage(line) geom = QgsGeometry().fromWkt(line) @@ # Label the edge midPoint = PgrQ.getMidPoint() - midPointCursor.execute(midPoint,(line,)) - pointRow = midPointCursor.fetchone() + midPointCursor.execute(midPoint, (line,)) + pointRow = midPointCursor.fetchone() + if not pointRow or pointRow[0] is None: + continue # Utils.logMessage("The point:" + str(pointRow[0])) ptgeom = QgsGeometry().fromWkt(str(pointRow[0]))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
functions/CostBase.py(1 hunks)functions/FunctionBase.py(4 hunks)functions/pgr_aStar.py(0 hunks)functions/pgr_aStarCost.py(0 hunks)functions/pgr_bdAstar.py(0 hunks)functions/pgr_bdAstarCost.py(0 hunks)functions/pgr_bdDijkstra.py(0 hunks)functions/pgr_bdDijkstraCost.py(0 hunks)functions/pgr_dijkstra.py(0 hunks)functions/pgr_dijkstraCost.py(0 hunks)metadata.txt(1 hunks)pgRoutingLayer.py(2 hunks)pgRoutingLayer_utils.py(1 hunks)utilities/pgr_queries.py(1 hunks)
💤 Files with no reviewable changes (8)
- functions/pgr_dijkstra.py
- functions/pgr_bdDijkstra.py
- functions/pgr_aStar.py
- functions/pgr_bdDijkstraCost.py
- functions/pgr_dijkstraCost.py
- functions/pgr_aStarCost.py
- functions/pgr_bdAstarCost.py
- functions/pgr_bdAstar.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-15T19:43:31.111Z
Learnt from: cvvergara
PR: pgRouting/pgRoutingLayer#149
File: functions/FunctionBase.py:256-256
Timestamp: 2025-10-15T19:43:31.111Z
Learning: In functions/FunctionBase.py, the drawCostPaths method is only called by Cost functions (those inheriting from CostBase) such as pgr_dijkstraCost, pgr_aStarCost, etc. These functions consistently use CostBase.getQuery which returns columns in the order: seq, start_vid, end_vid, cost, path_name, making row[4] the correct index for path_name. Non-Cost functions like pgr_KSP use drawManyPaths instead.
Applied to files:
functions/FunctionBase.py
🧬 Code graph analysis (2)
functions/FunctionBase.py (1)
utilities/pgr_queries.py (2)
getCostLine(84-91)getMidPoint(93-94)
pgRoutingLayer.py (3)
connectors/postgis.py (1)
connect(124-149)dbConnection.py (1)
connect(150-158)utilities/pgr_queries.py (1)
get_closestVertexInfo(96-123)
🪛 LanguageTool
metadata.txt
[grammar] ~10-~10: There might be a mistake here.
Context: ...dDijstraCost - pgr_KSP version=4.0.0 qgisMinimumVersion=3.10 qgisMaximumVersi...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...SP version=4.0.0 qgisMinimumVersion=3.10 qgisMaximumVersion=3.99 author=Anita Gra...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...imumVersion=3.10 qgisMaximumVersion=3.99 author=Anita Graser, Ko Nagase, Vicky Ve...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...gara, Cayetano Benavent, Aasheesh Tiwari email=[email protected] changelog=4....
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...heesh Tiwari email=[email protected] changelog=4.0.0 - Works best for pgR...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...il=[email protected] changelog=4.0.0 - Works best for pgRouting 4.0.0 - N...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...0 - No longer Support of QGIS < 3.10 - Using PyQt5 3.0.2 - Support fo...
(QB_NEW_EN)
🪛 Ruff (0.14.0)
pgRoutingLayer.py
1115-1116: try-except-pass detected, consider logging the exception
(S110)
1115-1115: Do not catch blind exception: Exception
(BLE001)
utilities/pgr_queries.py
75-75: Expected an indented block after function definition
(invalid-syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
pgRoutingLayer_utils.py (1)
97-97: CRS creation fix is correct (use EPSG auth string).Switching to createFromString("EPSG:{srid}") correctly replaces deprecated usage.
pgRoutingLayer.py (1)
957-958: Reverse cost as Identifier is correct.Passing column name through sql.Identifier is appropriate.
functions/CostBase.py (1)
64-97: CTE-based depart/arrive construction looks solid.Correctly qualifies tables, uses {geometry}, and ensures one row per (start_vid,end_vid) via DISTINCT ON with deterministic ORDER BY.
functions/FunctionBase.py (1)
36-41: Import and min pgRouting version bump are appropriate.Adopts new helpers and aligns minimum supported pgRouting to 3.0.
Needs 100% rewrite so better to start over
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pgRoutingLayer.py (1)
1092-1119: Remove redundant connection close in try block.Line 1102 closes the database connection before the return statements, and then the finally block (lines 1114-1118) attempts to close it again. This redundant pattern should be simplified by removing line 1102 and letting the finally block handle all connection cleanup.
Apply this diff:
db = None try: dbname = str(self.dock.comboConnections.currentText()) db = self.actionsDb[dbname].connect() connection = db.con cursor = connection.cursor() #Utils.logMessage(PgrQ.get_closestVertexInfo(args).as_string(connection)) cursor.execute(PgrQ.get_closestVertexInfo(args)) data = cursor.fetchone() - db.con.close() if data: return True, data[0], data[2] else: return False, None, None except psycopg2.DatabaseError as e: QApplication.restoreOverrideCursor() QMessageBox.critical(self.dock, self.dock.windowTitle(), '%s' % e) return False, None, None finally: if db and db.con: try: db.con.close() except Exception: pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
functions/drivingDistance.py(0 hunks)help/source/conf.py(3 hunks)pgRoutingLayer.py(2 hunks)utilities/pgr_queries.py(1 hunks)
💤 Files with no reviewable changes (1)
- functions/drivingDistance.py
🚧 Files skipped from review as they are similar to previous changes (1)
- help/source/conf.py
🧰 Additional context used
🧬 Code graph analysis (1)
pgRoutingLayer.py (3)
connectors/postgis.py (1)
connect(124-149)dbConnection.py (1)
connect(150-158)utilities/pgr_queries.py (1)
get_closestVertexInfo(99-126)
🪛 Ruff (0.14.0)
pgRoutingLayer.py
1117-1118: try-except-pass detected, consider logging the exception
(S110)
1117-1117: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
utilities/pgr_queries.py (3)
74-81: LGTM! Past critical issue resolved.The COALESCE pattern with deterministic ORDER BY ensures a single endpoint is returned, fixing the "more than one row returned by subquery" error from the previous implementation.
83-94: LGTM! CRS transformation properly applied.The cost line geometry is correctly transformed to the canvas CRS using the transform wrappers, addressing the previous major issue where drawn WKT could render incorrectly.
96-97: LGTM! Clean utility for midpoint annotation.The function provides a straightforward SQL expression for computing the midpoint of a line geometry, consistent with other query helpers in this module.
pgRoutingLayer.py (1)
954-957: LGTM! Correct use of Identifier for reverse cost column.When reverse cost is enabled, the column name is properly wrapped as an Identifier, consistent with how other column names (source, target, cost) are handled.
2 participants
Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores