Up to sdg3#282
Open
cvvergara wants to merge 38 commits into
Open
Conversation
Align BYPRODUCTS with files actually generated by sql_function.sql, in the order they appear. Add missing DEPENDS sql_function.sql.
Replaces old OUTPUT/DEPENDS pattern so psql always runs every build, keeping DB objects in sync regardless of file timestamps.
Remove stale data dependency (data.sh only lists tables, doesn't create DB objects) and redundant transitive dependency on graphs. Add dependency graph comment. Align vehicles BYPRODUCTS with files actually generated by vehicles.sql.
WalkthroughThis PR overhauls the pgRouting workshop: CI now tests apt and source osm2pgrouting installs, minimum required osm2pgrouting version is lowered with compatibility SQL for older column names, sql_function and withPoints chapters are rewritten with new function signatures, UN SDG 3/7/11 chapters consolidate scripts into single SQL files targeting standardized table names, CMake build wiring is refactored throughout, and locale catalogs are regenerated. ChangesCI/Build Infrastructure
Data Setup and Compatibility
Basic Chapter: sql_function Rewrite
Basic Chapter: withPoints Rewrite
Basic Scripts Build Wiring
UN SDG 11/3/7 Chapter Rework
Miscellaneous
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/un_sdg/sdg3-health.rst (1)
585-588: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReference
roads_nethere, notroads_ways.The SQL included below now reads from
roads_net, so this paragraph still names a table that no longer exists in this chapter and will mislead readers trying to map the text to the query.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/un_sdg/sdg3-health.rst` around lines 585 - 588, The paragraph describing the hospital-served road edges still refers to the old table name, so update the prose in the surrounding `sdg3-health.rst` section to match the SQL that now uses `roads_net` instead of `roads_ways`. Keep the rest of the explanation intact, but replace the outdated table reference wherever this query is described so readers can correctly map the text to the `roads_net` subquery and edge selection logic.
♻️ Duplicate comments (1)
docs/un_sdg/sdg7-energy.rst (1)
276-280: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStale comparison numbers removed without replacement — likely masking the root-cause issue in
sdg7.sql.The hardcoded "Total Road Length: 55.68 km" / "Optimal Network Length: 29.89 km" example values were dropped. This is consistent with the fact that, after
sdg7.sql's Exercise 2 now deletes non-largest-component rows directly fromroads.ways(see comment ondocs/scripts/un_sdg/sdg7/sdg7.sqllines 49-69), Exercise 5's total-length query onroads.waysno longer reflects the true total road network, so the comparison would no longer be meaningful with fresh numbers either. Removing the stale hardcoded output doesn't fix that root cause.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/un_sdg/sdg7-energy.rst` around lines 276 - 280, The example output in the `sdg7-energy.rst` documentation was removed because it no longer matches the data after `sdg7.sql` changes in `Exercise 2`, so update the docs in `sdg7-energy.rst` to reflect the current behavior instead of just dropping the numbers. Reconcile the comparison text around the `Total Road Length` and `Optimal Network Length` example with the `Exercise 5` query against `roads.ways`, and either replace it with fresh, valid values or rewrite the section to explain the changed result caused by the `sdg7.sql` workflow.
🧹 Nitpick comments (1)
docs/scripts/get_data/osm2pgrouting_compat.sql (1)
1-2: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider idempotency guard for repeat runs.
If the compat script is ever re-run against a database where the rename already happened (e.g., a rebuild without a fresh DB),
RENAME COLUMNwill fail sincegid/the_geomno longer exist. Since execution is gated byOsm2pgrouting_VERSION VERSION_LESS "3.0.0"in CMake (perdocs/scripts/get_data/CMakeLists.txt), this is unlikely in normal flows but worth a defensive check for reruns.Optional defensive guard
-ALTER TABLE ways RENAME COLUMN gid TO id; -ALTER TABLE ways RENAME COLUMN the_geom TO geom; +DO $$ +BEGIN + IF EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'ways' AND column_name = 'gid') THEN + ALTER TABLE ways RENAME COLUMN gid TO id; + END IF; + IF EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'ways' AND column_name = 'the_geom') THEN + ALTER TABLE ways RENAME COLUMN the_geom TO geom; + END IF; +END $$;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/scripts/get_data/osm2pgrouting_compat.sql` around lines 1 - 2, Add an idempotency guard to the osm2pgrouting_compat.sql rename steps so reruns don’t fail if ways already has id/geom. Update the two ALTER TABLE statements to first check the current column names on ways, and only rename gid to id and the_geom to geom when those source columns still exist. Keep the fix localized to the compat script and make it safe for repeat execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/basic/sql_function.rst`:
- Around line 160-166: The exercise numbering in the sql_function documentation
is out of order, with the azimuth section labeled as Exercise 5 before the
directionality section labeled as Exercise 4. Update the exercise labels in the
affected doc blocks so the numbering matches the document flow in order, and
adjust the titles for the sections around the azimuth and route geometry
directionality exercises to keep the chapter sequence monotonic.
In `@docs/basic/withPoints.rst`:
- Around line 35-38: Remove the stale “edges table name” bullet from the
parameter list in withPoints documentation so it matches the current
`wrk_withPoints` SQL signature. Update the input description to reflect only the
four coordinate arguments plus `do_debug`, and ensure the surrounding
prose/examples in this chapter stay consistent with `wrk_withPoints` and its
current contract.
In `@docs/scripts/basic/sql_function/sql_function.sql`:
- Around line 21-30: The wrk_dijkstra output still returns the raw vehicle_net
geometry in the wrong direction, so align the SQL body with the documented
wrk_dijkstra(source, target) contract by ensuring the route geometry is
corrected before selecting geom, readable, and azimuth. Update the
wrk_dijkstra/pgr_dijkstra query logic so the emitted geom matches the travel
direction and remove the intentional wrong-direction behavior from the final
result.
- Around line 159-161: The helper queries still use the old `@ID_1`@, `@ID_2`@, and
`@ID_3`@ placeholders, which should be updated to the renamed CH7 placeholders
used elsewhere in sql_function.sql. Fix the two SELECT statements under the \o
helpers.txt block to use the `@CH7_ID_`*@ symbols so configure_file produces valid
SQL and the final psql execution stays consistent with the rest of the chapter.
In `@docs/scripts/basic/withPoints/withPoints.sql`:
- Around line 18-26: The pgr_withPoints example is malformed and must be
restored to a valid two-point query. In withPoints.sql, fix the SQL block so the
first pgr_findCloseEdges call is properly closed, the missing UNION between the
two point subqueries is reintroduced, and both pid 1 and pid 2 are passed into
pgr_withPoints. Use the existing pgr_withPoints and pgr_findCloseEdges example
structure to ensure the generated route_withPoints.txt reflects a correct query.
In `@docs/scripts/get_data/CMakeLists.txt`:
- Around line 65-108: The setup/import steps in this CMake block ignore
failures, so configure can keep going after broken script or psql runs. Update
each execute_process call in the setup_city_routing, setup_mumbai,
setup_bangladesh, and Osm2pgrouting_VERSION compat import paths to capture the
exit status with RESULT_VARIABLE and stop configuration with message(FATAL_ERROR
...) when non-zero. Use the existing execute_process invocations and the
surrounding if (Osm2pgrouting_VERSION VERSION_LESS "3.0.0") blocks to locate all
affected calls.
In `@docs/scripts/get_data/setup_bangladesh.sh`:
- Around line 12-35: The Bangladesh setup script imports with a hard-coded user
role, but it never creates that role first, so the import can fail on a fresh
database. Add the missing bootstrap for the `user` role in `setup_bangladesh.sh`
before the `@Osm2pgrouting_EXECUTABLE@` import step, using the same
role-creation pattern as `setup_city_routing.sh`, so the `-U user -W user`
connection succeeds.
In `@docs/scripts/get_data/setup_mumbai.sh`:
- Around line 22-43: The Mumbai setup script assumes a pre-existing PostgreSQL
user role, so make it self-contained by creating the `user` role before the
`osm2pgrouting` imports in the setup flow. Add the role creation logic in
`setup_mumbai.sh` near the existing import commands, using the same approach as
`setup_city_routing.sh`, so the `@Osm2pgrouting_EXECUTABLE@` invocations for
roads and buildings can run on a clean install.
In `@docs/scripts/un_sdg/sdg11/CMakeLists.txt`:
- Line 29: Update the psql invocation in the sdg11 CMakeLists command so SQL
execution stops immediately on any error. In the CMake command that runs psql
for sdg11.sql, add ON_ERROR_STOP=1 and also use -X to prevent local ~/.psqlrc
settings from changing behavior. Keep the change scoped to the existing psql
command so the sdg11 script fails fast instead of leaving partial txt outputs.
- Around line 1-29: The un_sdg11_scripts target is non-incremental because
add_custom_target plus the current add_custom_command setup runs psql -f
sdg11.sql every build. Update the CMake logic around un_sdg11_scripts and
configure_file/sdg11.sql so the command is tied to a real stamp or output file
and only reruns when inputs change, or make the SQL idempotent so repeated
executions against bangladesh do not fail.
In `@docs/scripts/un_sdg/sdg3/CMakeLists.txt`:
- Line 37: The sdg3 CMake target runs psql via the command in CMakeLists.txt,
but it does not stop on the first SQL error. Update that psql invocation to
enable ON_ERROR_STOP so the sdg3.sql execution fails immediately if any
statement errors, preventing stale or partially regenerated outputs from being
treated as successful.
In `@docs/scripts/un_sdg/sdg3/sdg3.sql`:
- Around line 198-205: The UNION currently mixes different meanings in the
fourth column because subquery outputs agg_cost AS minutes while connected_edges
uses length in that position. Update connected_edges in the adjacent-road query
so the same column consistently represents minutes, either by outputting NULL AS
minutes or by computing a travel-time value for those rows, and keep the column
order aligned with subquery and the final UNION ALL.
- Around line 170-179: The `pgr_drivingDistance` calls use `SELECT vid FROM
buildings WHERE tag_id = '318'` as a scalar start vertex, which can return
multiple rows when there is more than one hospital. Update the affected
`pgr_drivingDistance` queries to either select one specific hospital row
uniquely or switch to the array-based start-vertex form if the intent is to
serve all hospitals, and apply the same fix to each repeated occurrence in the
chapter.
- Around line 57-63: The sdg3 build script is not rerunnable because it
unconditionally alters roads.ways and creates vertices with SELECT ... INTO,
both of which fail on a second run. Update the script around the ALTER TABLE
roads.ways and the vertices creation step to be idempotent by checking for
existing schema objects before adding the component column and by recreating or
replacing vertices safely instead of using a one-time INTO. Ensure the
un_sdg3_scripts target can be run repeatedly against the same database without
error.
In `@docs/scripts/un_sdg/sdg7/sdg7.sql`:
- Around line 49-69: The destructive DELETE in sdg7.sql is mutating the
canonical roads.ways import table, which breaks the later total-length
calculation; update the logic around the all_components/the_component CTEs so
you derive a filtered working copy instead of deleting from roads.ways, similar
to the roads_net pattern used in sdg3.sql. Keep roads.ways untouched and make
the Exercise 5 total_km query read from an untouched source or from a value
captured before filtering, so the total-vs-optimal comparison remains valid.
- Line 8: The reset step in the roads.ways table alteration is not idempotent
because dropping component fails when the column is already absent. Update the
ALTER TABLE statement in the sdg7 SQL reset script to use DROP COLUMN IF EXISTS
so reruns are safe and do not error. Locate the change in the schema reset
section that currently drops component from roads.ways.
In `@docs/un_sdg/sdg11-cities.rst`:
- Around line 239-243: The narrative in the SDG 11 docs uses the wrong vertex
table name, which can mislead readers. Update the paragraph around the
connected-components workflow to reference the actual table name used by the
SQL, matching the symbol/table name waterways.waterways_vertices instead of
waterways.waterway_vertices, and keep the rest of the explanation unchanged.
In `@docs/un_sdg/sdg3-health.rst`:
- Around line 420-423: The `literalinclude` for the Exercise 8 snippet currently
extends past the building-road test query and pulls in later markers from
`sdg3.sql`. Tighten the `.. literalinclude::` in the documentation so it starts
at `test_building_road.txt` but ends before `nearest_vertex.txt`, ensuring only
the building-road test query is rendered and the `get_vertex` function/test are
excluded.
In `@locale/en/LC_MESSAGES/basic/withPoints.po`:
- Around line 10-12: Update the catalog header metadata in the withPoints
translation file to match the regenerated template. In the header block for the
PO catalog, sync the Project-Id-Version value used by the basic/withPoints
catalog with the one in the regenerated withPoints.pot template so the shipped
English metadata is consistent; keep POT-Creation-Date aligned as already
refreshed.
---
Outside diff comments:
In `@docs/un_sdg/sdg3-health.rst`:
- Around line 585-588: The paragraph describing the hospital-served road edges
still refers to the old table name, so update the prose in the surrounding
`sdg3-health.rst` section to match the SQL that now uses `roads_net` instead of
`roads_ways`. Keep the rest of the explanation intact, but replace the outdated
table reference wherever this query is described so readers can correctly map
the text to the `roads_net` subquery and edge selection logic.
---
Duplicate comments:
In `@docs/un_sdg/sdg7-energy.rst`:
- Around line 276-280: The example output in the `sdg7-energy.rst` documentation
was removed because it no longer matches the data after `sdg7.sql` changes in
`Exercise 2`, so update the docs in `sdg7-energy.rst` to reflect the current
behavior instead of just dropping the numbers. Reconcile the comparison text
around the `Total Road Length` and `Optimal Network Length` example with the
`Exercise 5` query against `roads.ways`, and either replace it with fresh, valid
values or rewrite the section to explain the changed result caused by the
`sdg7.sql` workflow.
---
Nitpick comments:
In `@docs/scripts/get_data/osm2pgrouting_compat.sql`:
- Around line 1-2: Add an idempotency guard to the osm2pgrouting_compat.sql
rename steps so reruns don’t fail if ways already has id/geom. Update the two
ALTER TABLE statements to first check the current column names on ways, and only
rename gid to id and the_geom to geom when those source columns still exist.
Keep the fix localized to the compat script and make it safe for repeat
execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bff18ca2-d34f-4286-934c-5007d9d3dc1d
⛔ Files ignored due to path filters (14)
docs/basic/images/sql_function/good_directionality.pngis excluded by!**/*.pngdocs/basic/images/sql_function/route_azimuth.pngis excluded by!**/*.pngdocs/basic/images/sql_function/route_final.pngis excluded by!**/*.pngdocs/basic/images/sql_function/route_names.pngis excluded by!**/*.pngdocs/basic/images/sql_function/route_readable.pngis excluded by!**/*.pngdocs/basic/images/sql_function/sql_azimuth_fixed.pngis excluded by!**/*.pngdocs/basic/images/sql_function/sql_route_geom.pngis excluded by!**/*.pngdocs/basic/images/sql_function/sql_route_geom_detail.pngis excluded by!**/*.pngdocs/basic/images/sql_function/sql_route_names.pngis excluded by!**/*.pngdocs/basic/images/sql_function/sql_route_readable.pngis excluded by!**/*.pngdocs/basic/images/sql_function/wrong_direccionality.pngis excluded by!**/*.pngdocs/basic/images/sql_function/wrong_directionality.pngis excluded by!**/*.pngdocs/basic/images/withPoints/points_on_map.pngis excluded by!**/*.pngdocs/basic/images/withPoints/points_routing.pngis excluded by!**/*.png
📒 Files selected for processing (67)
.github/workflows/ubuntu.yml.github/workflows/website.ymlCMakeLists.txtcmake/FindOsm2pgrouting.cmakedocs/basic/data.rstdocs/basic/images/sql_function/CMakeLists.txtdocs/basic/sql_function.rstdocs/basic/withPoints.rstdocs/index.rstdocs/interactions/code/QGISfunctions.sqldocs/interactions/code/create-test.shdocs/interactions/code/functions.sqldocs/scripts/basic/CMakeLists.txtdocs/scripts/basic/data/CMakeLists.txtdocs/scripts/basic/data/data.shdocs/scripts/basic/graphs/CMakeLists.txtdocs/scripts/basic/graphs/graphs.sqldocs/scripts/basic/pedestrian/CMakeLists.txtdocs/scripts/basic/sql_function/CMakeLists.txtdocs/scripts/basic/sql_function/images.sqldocs/scripts/basic/sql_function/sql_function.sqldocs/scripts/basic/vehicles/CMakeLists.txtdocs/scripts/basic/vehicles/images.sqldocs/scripts/basic/withPoints/CMakeLists.txtdocs/scripts/basic/withPoints/images.sqldocs/scripts/basic/withPoints/withPoints.sqldocs/scripts/configuration/osmid_configuration.sqldocs/scripts/get_data/CMakeLists.txtdocs/scripts/get_data/buildings.xmldocs/scripts/get_data/osm2pgrouting_compat.sqldocs/scripts/get_data/osm2pgrouting_compat_bangladesh.sqldocs/scripts/get_data/osm2pgrouting_compat_mumbai.sqldocs/scripts/get_data/setup_bangladesh.shdocs/scripts/get_data/setup_city_routing.shdocs/scripts/get_data/setup_mumbai.shdocs/scripts/get_data/waterways.xmldocs/scripts/un_sdg/CMakeLists.txtdocs/scripts/un_sdg/sdg11/CMakeLists.txtdocs/scripts/un_sdg/sdg11/create_bangladesh.shdocs/scripts/un_sdg/sdg11/get_bangladesh.shdocs/scripts/un_sdg/sdg11/import_bangladesh_waterways.shdocs/scripts/un_sdg/sdg11/sdg11.sqldocs/scripts/un_sdg/sdg3/CMakeLists.txtdocs/scripts/un_sdg/sdg3/all_exercises_sdg3.sqldocs/scripts/un_sdg/sdg3/create_mumbai.shdocs/scripts/un_sdg/sdg3/get_mumbai.shdocs/scripts/un_sdg/sdg3/import_mumbai_buildings.shdocs/scripts/un_sdg/sdg3/import_mumbai_roads.shdocs/scripts/un_sdg/sdg3/sdg3.sqldocs/scripts/un_sdg/sdg7/CMakeLists.txtdocs/scripts/un_sdg/sdg7/sdg7.sqldocs/un_sdg/data.rstdocs/un_sdg/sdg11-cities.rstdocs/un_sdg/sdg3-health.rstdocs/un_sdg/sdg7-energy.rstlocale/en/LC_MESSAGES/basic/data.polocale/en/LC_MESSAGES/basic/sql_function.polocale/en/LC_MESSAGES/basic/withPoints.polocale/en/LC_MESSAGES/un_sdg/data.polocale/en/LC_MESSAGES/un_sdg/sdg11-cities.polocale/en/LC_MESSAGES/un_sdg/sdg3-health.polocale/pot/basic/data.potlocale/pot/basic/sql_function.potlocale/pot/basic/withPoints.potlocale/pot/un_sdg/data.potlocale/pot/un_sdg/sdg11-cities.potlocale/pot/un_sdg/sdg3-health.pot
💤 Files with no reviewable changes (13)
- cmake/FindOsm2pgrouting.cmake
- docs/scripts/un_sdg/sdg11/create_bangladesh.sh
- docs/scripts/un_sdg/sdg3/import_mumbai_roads.sh
- docs/scripts/un_sdg/sdg11/import_bangladesh_waterways.sh
- docs/scripts/un_sdg/sdg3/import_mumbai_buildings.sh
- docs/scripts/un_sdg/sdg3/get_mumbai.sh
- docs/scripts/basic/sql_function/images.sql
- docs/scripts/un_sdg/sdg11/get_bangladesh.sh
- docs/scripts/basic/withPoints/images.sql
- docs/scripts/un_sdg/sdg3/create_mumbai.sh
- docs/scripts/basic/vehicles/images.sql
- docs/scripts/un_sdg/sdg3/all_exercises_sdg3.sql
- docs/scripts/un_sdg/CMakeLists.txt
Comment on lines
+160
to
166
| Exercise 5: Get the azimuth | ||
| ------------------------------------------------------------------------------- | ||
|
|
||
| .. image:: images/sql_function/sql_route_geom.png | ||
| .. image:: images/sql_function/route_azimuth.png | ||
| :width: 300pt | ||
| :alt: From |ch7_place_1| to |ch7_place_2| | ||
| :alt: Route showing azimuth | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Renumber this exercise to keep the chapter flow monotonic.
Exercise 5: Get the azimuth appears before Exercise 4: Route geometry directionality, so the narrative and regenerated catalogs now jump backwards. Please swap the labels so the exercise order matches the document order.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/basic/sql_function.rst` around lines 160 - 166, The exercise numbering
in the sql_function documentation is out of order, with the azimuth section
labeled as Exercise 5 before the directionality section labeled as Exercise 4.
Update the exercise labels in the affected doc blocks so the numbering matches
the document flow in order, and adjust the titles for the sections around the
azimuth and route geometry directionality exercises to keep the chapter sequence
monotonic.
Comment on lines
35
to
+38
| * The function takes as input parameters: | ||
|
|
||
| - The edges table name | ||
| - Latitude/longitude of two point | ||
| - Latitude/longitude of two points |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Drop the stale “edges table name” input.
wrk_withPoints no longer accepts an edges-table argument—the SQL signature now takes four coordinates plus do_debug. Keeping this bullet makes the chapter contract disagree with both the implementation and the examples.
Suggested fix
* The function takes as input parameters:
- - The edges table name
- - Latitude/longitude of two points
+ - Latitude/longitude of the departure and destination points
+ - Optional ``do_debug`` flag📝 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.
Suggested change
| * The function takes as input parameters: | |
| - The edges table name | |
| - Latitude/longitude of two point | |
| - Latitude/longitude of two points | |
| * The function takes as input parameters: | |
| - Latitude/longitude of the departure and destination points | |
| - Optional ``do_debug`` flag |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/basic/withPoints.rst` around lines 35 - 38, Remove the stale “edges
table name” bullet from the parameter list in withPoints documentation so it
matches the current `wrk_withPoints` SQL signature. Update the input description
to reflect only the four coordinate arguments plus `do_debug`, and ensure the
surrounding prose/examples in this chapter stay consistent with `wrk_withPoints`
and its current contract.
Comment on lines
21
to
+30
| FROM pgr_dijkstra( | ||
| 'SELECT * FROM ' || $1, | ||
| -- on purpose to have wrong direccionality | ||
| 'SELECT * FROM vehicle_net', | ||
| source, target) | ||
| ) | ||
| SELECT | ||
| seq, id, seconds, name, length, | ||
| ST_AsText(geom) | ||
| degrees(ST_azimuth(ST_StartPoint(geom), ST_EndPoint(geom))), | ||
| ST_AsText(geom), | ||
| geom |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
wrk_dijkstra still violates the chapter’s published output contract.
docs/basic/sql_function.rst says wrk_dijkstra(source, target) returns geom/readable/azimuth in the route’s correct direction, but this body still emits raw vehicle_net.geom and even documents it as intentionally wrong. Because the corrected logic lives under different function names, callers that follow the documented wrk_dijkstra API still get inconsistent directionality.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/scripts/basic/sql_function/sql_function.sql` around lines 21 - 30, The
wrk_dijkstra output still returns the raw vehicle_net geometry in the wrong
direction, so align the SQL body with the documented wrk_dijkstra(source,
target) contract by ensuring the route geometry is corrected before selecting
geom, readable, and azimuth. Update the wrk_dijkstra/pgr_dijkstra query logic so
the emitted geom matches the travel direction and remove the intentional
wrong-direction behavior from the final result.
Comment on lines
+159
to
+161
| \o helpers.txt | ||
| SELECT seq, a.azimuth = b.azimuth FROM wrk_dijkstra_fixed(@ID_1@, @ID_2@) a JOIN wrk_dijkstra(@ID_1@, @ID_2@) b USING (seq, id, seconds, name, length) WHERE a.azimuth != b.azimuth; | ||
| SELECT seq, a.azimuth = b.azimuth FROM wrk_dijkstra_fixed(@ID_1@, @ID_3@) a JOIN wrk_dijkstra(@ID_1@, @ID_3@) b USING (seq, id, seconds, name, length) WHERE a.azimuth != b.azimuth; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the renamed CH7 placeholders in these helper queries.
Everything else in this chapter uses @CH7_ID_*@. Leaving @ID_1@, @ID_2@, and @ID_3@ here risks generating invalid SQL during configure_file, which then fails the final psql -f sql_function.sql step.
Suggested fix
-SELECT seq, a.azimuth = b.azimuth FROM wrk_dijkstra_fixed(`@ID_1`@, `@ID_2`@) a JOIN wrk_dijkstra(`@ID_1`@, `@ID_2`@) b USING (seq, id, seconds, name, length) WHERE a.azimuth != b.azimuth;
-SELECT seq, a.azimuth = b.azimuth FROM wrk_dijkstra_fixed(`@ID_1`@, `@ID_3`@) a JOIN wrk_dijkstra(`@ID_1`@, `@ID_3`@) b USING (seq, id, seconds, name, length) WHERE a.azimuth != b.azimuth;
+SELECT seq, a.azimuth = b.azimuth FROM wrk_dijkstra_fixed(`@CH7_ID_1`@, `@CH7_ID_2`@) a JOIN wrk_dijkstra(`@CH7_ID_1`@, `@CH7_ID_2`@) b USING (seq, id, seconds, name, length) WHERE a.azimuth != b.azimuth;
+SELECT seq, a.azimuth = b.azimuth FROM wrk_dijkstra_fixed(`@CH7_ID_1`@, `@CH7_ID_3`@) a JOIN wrk_dijkstra(`@CH7_ID_1`@, `@CH7_ID_3`@) b USING (seq, id, seconds, name, length) WHERE a.azimuth != b.azimuth;📝 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.
Suggested change
| \o helpers.txt | |
| SELECT seq, a.azimuth = b.azimuth FROM wrk_dijkstra_fixed(@ID_1@, @ID_2@) a JOIN wrk_dijkstra(@ID_1@, @ID_2@) b USING (seq, id, seconds, name, length) WHERE a.azimuth != b.azimuth; | |
| SELECT seq, a.azimuth = b.azimuth FROM wrk_dijkstra_fixed(@ID_1@, @ID_3@) a JOIN wrk_dijkstra(@ID_1@, @ID_3@) b USING (seq, id, seconds, name, length) WHERE a.azimuth != b.azimuth; | |
| \o helpers.txt | |
| SELECT seq, a.azimuth = b.azimuth FROM wrk_dijkstra_fixed(`@CH7_ID_1`@, `@CH7_ID_2`@) a JOIN wrk_dijkstra(`@CH7_ID_1`@, `@CH7_ID_2`@) b USING (seq, id, seconds, name, length) WHERE a.azimuth != b.azimuth; | |
| SELECT seq, a.azimuth = b.azimuth FROM wrk_dijkstra_fixed(`@CH7_ID_1`@, `@CH7_ID_3`@) a JOIN wrk_dijkstra(`@CH7_ID_1`@, `@CH7_ID_3`@) b USING (seq, id, seconds, name, length) WHERE a.azimuth != b.azimuth; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/scripts/basic/sql_function/sql_function.sql` around lines 159 - 161, The
helper queries still use the old `@ID_1`@, `@ID_2`@, and `@ID_3`@ placeholders, which
should be updated to the renamed CH7 placeholders used elsewhere in
sql_function.sql. Fix the two SELECT statements under the \o helpers.txt block
to use the `@CH7_ID_`*@ symbols so configure_file produces valid SQL and the final
psql execution stays consistent with the rest of the chapter.
Comment on lines
18
to
26
| SELECT * FROM pgr_withPoints( | ||
| 'SELECT id, source, target, cost, reverse_cost from vehicle_net', | ||
| 'SELECT id, source, target, cost, reverse_cost FROM vehicle_net', | ||
| $$ | ||
| SELECT 2 AS pid, * from pgr_findCloseEdges( | ||
| 'SELECT id, geom from vehicle_net', | ||
| ST_SetSRID(ST_Point(@POINT1_LON@, @POINT1_LAT@), 4326), 0.5) | ||
| UNION | ||
| SELECT 1 AS pid, * from pgr_findCloseEdges( | ||
| 'SELECT id, geom from vehicle_net', | ||
| SELECT 2 AS pid, * FROM pgr_findCloseEdges( | ||
| 'SELECT id, geom FROM vehicle_net', | ||
| SELECT 1 AS pid, * FROM pgr_findCloseEdges( | ||
| 'SELECT id, geom FROM vehicle_net', | ||
| ST_SetSRID(ST_Point(@POINT2_LON@, @POINT2_LAT@), 4326), 0.5) | ||
| $$, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Repair the broken pgr_withPoints example.
The Points SQL block is malformed: the first pgr_findCloseEdges(...) call never closes, the UNION disappeared, and point 1 is no longer passed at all. That will fail when generating route_withPoints.txt or, worse, teach readers the wrong query.
Suggested fix
SELECT * FROM pgr_withPoints(
'SELECT id, source, target, cost, reverse_cost FROM vehicle_net',
$$
- SELECT 2 AS pid, * FROM pgr_findCloseEdges(
- 'SELECT id, geom FROM vehicle_net',
SELECT 1 AS pid, * FROM pgr_findCloseEdges(
'SELECT id, geom FROM vehicle_net',
- ST_SetSRID(ST_Point(`@POINT2_LON`@, `@POINT2_LAT`@), 4326), 0.5)
+ ST_SetSRID(ST_Point(`@POINT1_LON`@, `@POINT1_LAT`@), 4326), 0.5)
+ UNION
+ SELECT 2 AS pid, * FROM pgr_findCloseEdges(
+ 'SELECT id, geom FROM vehicle_net',
+ ST_SetSRID(ST_Point(`@POINT2_LON`@, `@POINT2_LAT`@), 4326), 0.5)
$$,
-1, -2);📝 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.
Suggested change
| SELECT * FROM pgr_withPoints( | |
| 'SELECT id, source, target, cost, reverse_cost from vehicle_net', | |
| 'SELECT id, source, target, cost, reverse_cost FROM vehicle_net', | |
| $$ | |
| SELECT 2 AS pid, * from pgr_findCloseEdges( | |
| 'SELECT id, geom from vehicle_net', | |
| ST_SetSRID(ST_Point(@POINT1_LON@, @POINT1_LAT@), 4326), 0.5) | |
| UNION | |
| SELECT 1 AS pid, * from pgr_findCloseEdges( | |
| 'SELECT id, geom from vehicle_net', | |
| SELECT 2 AS pid, * FROM pgr_findCloseEdges( | |
| 'SELECT id, geom FROM vehicle_net', | |
| SELECT 1 AS pid, * FROM pgr_findCloseEdges( | |
| 'SELECT id, geom FROM vehicle_net', | |
| ST_SetSRID(ST_Point(@POINT2_LON@, @POINT2_LAT@), 4326), 0.5) | |
| $$, | |
| SELECT * FROM pgr_withPoints( | |
| 'SELECT id, source, target, cost, reverse_cost FROM vehicle_net', | |
| $$ | |
| SELECT 1 AS pid, * FROM pgr_findCloseEdges( | |
| 'SELECT id, geom FROM vehicle_net', | |
| ST_SetSRID(ST_Point(`@POINT1_LON`@, `@POINT1_LAT`@), 4326), 0.5) | |
| UNION | |
| SELECT 2 AS pid, * FROM pgr_findCloseEdges( | |
| 'SELECT id, geom FROM vehicle_net', | |
| ST_SetSRID(ST_Point(`@POINT2_LON`@, `@POINT2_LAT`@), 4326), 0.5) | |
| $$, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/scripts/basic/withPoints/withPoints.sql` around lines 18 - 26, The
pgr_withPoints example is malformed and must be restored to a valid two-point
query. In withPoints.sql, fix the SQL block so the first pgr_findCloseEdges call
is properly closed, the missing UNION between the two point subqueries is
reintroduced, and both pid 1 and pid 2 are passed into pgr_withPoints. Use the
existing pgr_withPoints and pgr_findCloseEdges example structure to ensure the
generated route_withPoints.txt reflects a correct query.
|
|
||
| DROP TABLE IF EXISTS roads.roads_vertices; | ||
| ALTER TABLE roads.roads_ways DROP component; | ||
| ALTER TABLE roads.ways DROP component; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby lines.
nl -ba docs/scripts/un_sdg/sdg7/sdg7.sql | sed -n '1,40p'
# Look for similar ALTER TABLE DROP usage in this area for style/context.
rg -n "DROP( COLUMN)?( IF EXISTS)? component|DROP COLUMN IF EXISTS|DROP component" docs/scripts/un_sdg -SRepository: pgRouting/workshop
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file =="
cat -n docs/scripts/un_sdg/sdg7/sdg7.sql | sed -n '1,40p'
echo
echo "== nearby DROP usages =="
rg -n "DROP( COLUMN)?( IF EXISTS)? component|DROP COLUMN IF EXISTS|DROP component" docs/scripts/un_sdg -S || trueRepository: pgRouting/workshop
Length of output: 1553
Use DROP COLUMN IF EXISTS for this reset step. ALTER TABLE roads.ways DROP component; will fail when component is absent, so making it idempotent avoids breaking a clean rerun.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/scripts/un_sdg/sdg7/sdg7.sql` at line 8, The reset step in the
roads.ways table alteration is not idempotent because dropping component fails
when the column is already absent. Update the ALTER TABLE statement in the sdg7
SQL reset script to use DROP COLUMN IF EXISTS so reruns are safe and do not
error. Locate the change in the schema reset section that currently drops
component from roads.ways.
Comment on lines
49
to
69
| WITH | ||
| all_components AS (SELECT component, count(*) FROM roads_ways GROUP BY component), | ||
| max_component AS (SELECT max(count) from all_components) | ||
| all_components AS (SELECT component, count(*) FROM roads.ways GROUP BY component), | ||
| max_component AS (SELECT max(count) FROM all_components) | ||
| SELECT component FROM all_components WHERE count = (SELECT max FROM max_component); | ||
|
|
||
| \o only_connected7.txt | ||
|
|
||
| WITH | ||
| all_components AS (SELECT component, count(*) FROM roads_ways GROUP BY component), | ||
| max_component AS (SELECT max(count) from all_components), | ||
| all_components AS (SELECT component, count(*) FROM roads.ways GROUP BY component), | ||
| max_component AS (SELECT max(count) FROM all_components), | ||
| the_component AS (SELECT component FROM all_components WHERE count = (SELECT max FROM max_component)) | ||
| DELETE FROM roads_ways WHERE component != (SELECT component FROM the_component); | ||
| DELETE FROM roads.ways WHERE component != (SELECT component FROM the_component); | ||
|
|
||
| \o only_connected8.txt | ||
|
|
||
| WITH | ||
| all_components AS (SELECT component, count(*) FROM roads_vertices GROUP BY component), | ||
| max_component AS (SELECT max(count) from all_components), | ||
| max_component AS (SELECT max(count) FROM all_components), | ||
| the_component AS (SELECT component FROM all_components WHERE count = (SELECT max FROM max_component)) | ||
| DELETE FROM roads_vertices WHERE component != (SELECT component FROM the_component); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Destructive DELETE FROM roads.ways mutates the canonical import table and invalidates the total-length comparison.
Line 60 deletes non-largest-component rows directly from the schema-qualified roads.ways table (the osm2pgrouting import target), instead of deriving a filtered copy as the sdg3 chapter does. In sdg3.sql, the equivalent step builds a new roads_net table from roads.ways and only filters the derived vertices table, leaving roads.ways intact:
"CREATE TABLE roads_net AS ... FROM roads.ways w JOIN the_component USING (component);"
Here, sdg7.sql instead does DELETE FROM roads.ways WHERE component != (SELECT component FROM the_component);, permanently removing edges from the canonical table.
This has a concrete downstream effect: Exercise 5's "Compute total length of roads" (line 88-89) runs SELECT sum(length_m)/1000 AS total_km FROM roads.ways; — but since roads.ways was already pruned to the largest component by the earlier DELETE, this no longer represents the true total road network length. It will now return (close to) the same value as the MST length computed in Exercise 4, making the "Total vs Optimal length" comparison in sdg7-energy.rst meaningless. Notably, the rst diff removes the old hardcoded comparison numbers (55.68 km vs 29.89 km) without providing replacements, consistent with this regression.
Consider computing the total length from a value captured before the DELETE (or from a separate untouched copy of the raw import), similar to how sdg3 preserves roads.ways and derives roads_net.
Also applies to: 88-89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/scripts/un_sdg/sdg7/sdg7.sql` around lines 49 - 69, The destructive
DELETE in sdg7.sql is mutating the canonical roads.ways import table, which
breaks the later total-length calculation; update the logic around the
all_components/the_component CTEs so you derive a filtered working copy instead
of deleting from roads.ways, similar to the roads_net pattern used in sdg3.sql.
Keep roads.ways untouched and make the Exercise 5 total_km query read from an
untouched source or from a value captured before filtering, so the
total-vs-optimal comparison remains valid.
Comment on lines
239
to
243
| A sub-query is created to find out all the connected components. After that, | ||
| the ``component`` column is updated using the results obtained from the sub-query. | ||
| This helps in storing the component id in the ``waterways_ways_vertices_pgr`` table. | ||
| Next query uses this output and stores the component id in the waterways_ways | ||
| This helps in storing the component id in the ``waterways.waterway_vertices`` table. | ||
| Next query uses this output and stores the component id in the waterways.ways | ||
| (edges) table. Follow the steps given below to complete this task. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the actual vertex-table name in the narrative.
The SQL creates waterways.waterways_vertices, but this paragraph says waterways.waterway_vertices. That will send readers to a table that does not exist.
Suggested change
-This helps in storing the component id in the ``waterways.waterway_vertices`` table.
+This helps in storing the component id in the ``waterways.waterways_vertices`` table.📝 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.
Suggested change
| A sub-query is created to find out all the connected components. After that, | |
| the ``component`` column is updated using the results obtained from the sub-query. | |
| This helps in storing the component id in the ``waterways_ways_vertices_pgr`` table. | |
| Next query uses this output and stores the component id in the waterways_ways | |
| This helps in storing the component id in the ``waterways.waterway_vertices`` table. | |
| Next query uses this output and stores the component id in the waterways.ways | |
| (edges) table. Follow the steps given below to complete this task. | |
| A sub-query is created to find out all the connected components. After that, | |
| the ``component`` column is updated using the results obtained from the sub-query. | |
| This helps in storing the component id in the ``waterways.waterways_vertices`` table. | |
| Next query uses this output and stores the component id in the waterways.ways | |
| (edges) table. Follow the steps given below to complete this task. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/un_sdg/sdg11-cities.rst` around lines 239 - 243, The narrative in the
SDG 11 docs uses the wrong vertex table name, which can mislead readers. Update
the paragraph around the connected-components workflow to reference the actual
table name used by the SQL, matching the symbol/table name
waterways.waterways_vertices instead of waterways.waterway_vertices, and keep
the rest of the explanation unchanged.
Comment on lines
+420
to
+423
| .. literalinclude:: ../scripts/un_sdg/sdg3/sdg3.sql | ||
| :start-after: test_building_road.txt | ||
| :end-before: clean_buildings.txt | ||
| :language: sql |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Narrow this literalinclude to the building-road test query.
Ending at clean_buildings.txt pulls in the nearest_vertex.txt marker, the get_vertex function, and its test from docs/scripts/un_sdg/sdg3/sdg3.sql. Exercise 8 should stop before nearest_vertex.txt, otherwise the rendered “Test the function” snippet shows the next exercise too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/un_sdg/sdg3-health.rst` around lines 420 - 423, The `literalinclude` for
the Exercise 8 snippet currently extends past the building-road test query and
pulls in later markers from `sdg3.sql`. Tighten the `.. literalinclude::` in the
documentation so it starts at `test_building_road.txt` but ends before
`nearest_vertex.txt`, ensuring only the building-road test query is rendered and
the `get_vertex` function/test are excluded.
Comment on lines
10
to
+12
| "Project-Id-Version: Workshop FOSS4G Belém 3.0\n" | ||
| "Report-Msgid-Bugs-To: \n" | ||
| "POT-Creation-Date: 2026-06-14 15:20+0000\n" | ||
| "POT-Creation-Date: 2026-07-01 17:09+0000\n" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Sync this catalog header with the regenerated template.
Only POT-Creation-Date was refreshed here; the file still identifies itself as “Workshop FOSS4G Belém 3.0”, while locale/pot/basic/withPoints.pot now carries “Workshop FOSS4G Hiroshima 3.2”. That leaves the shipped English metadata half-regenerated.
Suggested fix
-"Project-Id-Version: Workshop FOSS4G Belém 3.0\n"
+"Project-Id-Version: Workshop FOSS4G Hiroshima 3.2\n"📝 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.
Suggested change
| "Project-Id-Version: Workshop FOSS4G Belém 3.0\n" | |
| "Report-Msgid-Bugs-To: \n" | |
| "POT-Creation-Date: 2026-06-14 15:20+0000\n" | |
| "POT-Creation-Date: 2026-07-01 17:09+0000\n" | |
| "Project-Id-Version: Workshop FOSS4G Hiroshima 3.2\n" | |
| "Report-Msgid-Bugs-To: \n" | |
| "POT-Creation-Date: 2026-07-01 17:09+0000\n" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@locale/en/LC_MESSAGES/basic/withPoints.po` around lines 10 - 12, Update the
catalog header metadata in the withPoints translation file to match the
regenerated template. In the header block for the PO catalog, sync the
Project-Id-Version value used by the basic/withPoints catalog with the one in
the regenerated withPoints.pot template so the shipped English metadata is
consistent; keep POT-Creation-Date aligned as already refreshed.
robe2
approved these changes
Jul 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updates the workshop up to UN's SDG3 chapter
@pgRouting/admins
Summary by CodeRabbit
New Features
osm2pgroutingsetups, including compatibility handling and updated import workflows for multiple workshop databases.Bug Fixes
Documentation