Hiroshima sql-function chapter#281
Open
cvvergara wants to merge 7 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.
WalkthroughThis PR rewrites the Chapter 7 SQL function and withPoints workshop exercises. The ChangesSQL function workshop rewrite
withPoints workshop rewrite
Workshop location config
Sequence Diagram(s)sequenceDiagram
participant Script as sql_function.sql
participant Vehicle as vehicle_net
participant Penalized as vehicle_penalized_net
participant Views as vw_initial / vw_fixed / vw_final
Script->>Vehicle: wrk_dijkstra(source, target)
Vehicle-->>Script: seq, id, azimuth, readable, geom
Script->>Vehicle: wrk_dijkstra_fixed(source, target)
Vehicle-->>Script: corrected geom, azimuth
Script->>Penalized: wrk_dijkstra_final(source, target)
Penalized-->>Script: final route + azimuth + geom
Script->>Views: CREATE vw_initial, vw_fixed, vw_final
sequenceDiagram
participant Script as withPoints.sql
participant Map as points_on_map
participant WithPoints as wrk_withPoints
participant Penalized as vehicle_penalized_net
participant Example as example view
Script->>Map: SELECT * FROM points_on_map
Script->>WithPoints: wrk_withPoints(lat1, lon1, lat2, lon2)
WithPoints->>Penalized: pgr_withPoints via points_sql
Penalized-->>WithPoints: route rows
WithPoints-->>Script: seq, id, azimuth, readable, geom
Script->>Example: CREATE OR REPLACE VIEW example AS SELECT *
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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: 4
🤖 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`:
- Line 160: The exercise numbering in the SQL function chapter is out of
sequence, with Exercise 5 appearing before Exercise 4, which breaks the
walkthrough flow. Update the exercise labels in the affected section so the
numbers increase sequentially in chapter order, and verify the related exercise
headings around the azimuth and subsequent step use the corrected numbering
consistently.
In `@docs/basic/withPoints.rst`:
- Around line 35-38: The chapter contract for wrk_withPoints is stale: it still
describes an edges table name even though the current implementation only
accepts the point coordinates and do_debug. Update the parameter list in
withPoints.rst to match the actual wrk_withPoints interface, removing the
edges-table input and aligning the surrounding prose with the accepted
lat1/lon1/lat2/lon2/do_debug arguments so readers see the correct arity.
- Line 104: Fix the user-facing typos in the withPoints.rst documentation text:
change “appropiate” to “appropriate” and reword “that is been generated” to a
correct phrase such as “that is generated” or “that is being generated”. Apply
the same wording cleanup to the repeated occurrence noted in the same document,
keeping the surrounding sentence meaning unchanged.
In `@docs/scripts/basic/withPoints/withPoints.sql`:
- Around line 71-99: The query in withPoints.sql is including the terminal
sentinel row from pgr_withPoints because it aliases edge to id and joins it
through results/additional, which leaves an extra blank segment. Update the
resuts_query/final_query flow to filter out the row where edge = -1 before the
LEFT JOIN to vehicle_net (or before building additional), so only real path
segments are returned and the example no longer includes NULL metadata.
🪄 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: e7209a1c-a500-4bb0-8cc3-ee86795b6fab
⛔ 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 (10)
CMakeLists.txtdocs/basic/images/sql_function/CMakeLists.txtdocs/basic/sql_function.rstdocs/basic/withPoints.rstdocs/scripts/basic/sql_function/CMakeLists.txtdocs/scripts/basic/sql_function/images.sqldocs/scripts/basic/sql_function/sql_function.sqldocs/scripts/basic/withPoints/CMakeLists.txtdocs/scripts/basic/withPoints/images.sqldocs/scripts/basic/withPoints/withPoints.sql
💤 Files with no reviewable changes (2)
- docs/scripts/basic/sql_function/images.sql
- docs/scripts/basic/withPoints/images.sql
|
|
||
|
|
||
| Exercise 3: Route geometry (binary format) | ||
| Exercise 5: Get the azimuth |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Renumber the exercises to keep the chapter flow sequential.
Line 160 introduces Exercise 5, but Line 188 later introduces Exercise 4. That makes the walkthrough order confusing when readers follow the chapter step by step.
Also applies to: 188-188
🤖 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` at line 160, The exercise numbering in the SQL
function chapter is out of sequence, with Exercise 5 appearing before Exercise
4, which breaks the walkthrough flow. Update the exercise labels in the affected
section so the numbers increase sequentially in chapter order, and verify the
related exercise headings around the azimuth and subsequent step use the
corrected numbering consistently.
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.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the stale edge-table input from the chapter contract.
Line 37 still says wrk_withPoints takes an edges table name, but the implementation now hardcodes the network and only accepts lat1/lon1/lat2/lon2/do_debug. This contradicts the parameter table below and will lead readers to call the function with the wrong arity.
🤖 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, The chapter contract for
wrk_withPoints is stale: it still describes an edges table name even though the
current implementation only accepts the point coordinates and do_debug. Update
the parameter list in withPoints.rst to match the actual wrk_withPoints
interface, removing the edges-table input and aligning the surrounding prose
with the accepted lat1/lon1/lat2/lon2/do_debug arguments so readers see the
correct arity.
|
|
||
| * Build the geometry of the points with the appropiate SRID. (lines **4** and | ||
| **10**) | ||
| * Build the geometry of the points with the appropiate SRID. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the new user-facing typos.
appropiate → appropriate, and that is been generated should be reworded (that is generated / that is being generated).
Also applies to: 262-262
🤖 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` at line 104, Fix the user-facing typos in the
withPoints.rst documentation text: change “appropiate” to “appropriate” and
reword “that is been generated” to a correct phrase such as “that is generated”
or “that is being generated”. Apply the same wording cleanup to the repeated
occurrence noted in the same document, keeping the surrounding sentence meaning
unchanged.
Comment on lines
71
to
+99
| resuts_query := format( | ||
| $$SELECT seq, edge AS id, node, cost AS seconds | ||
| FROM pgr_withPoints( | ||
| 'SELECT * FROM %1$I', | ||
| '%2$s', | ||
| 'SELECT * FROM vehicle_penalized_net', | ||
| '%1$s', | ||
| -1, -2) | ||
| $$, edges_subset, closest_query); | ||
| $$, points_sql); | ||
|
|
||
| -- 2 | ||
|
|
||
| additional_query := format( | ||
| $$SELECT | ||
| seq, id, seconds, name, length, | ||
| CASE | ||
| WHEN node = source THEN ST_AsText(geom) | ||
| ELSE ST_AsText(ST_Reverse(geom)) | ||
| END AS readable, | ||
|
|
||
| CASE | ||
| WHEN node = source THEN geom | ||
| ELSE ST_Reverse(geom) | ||
| END AS geom | ||
| FROM results | ||
| LEFT JOIN %1$I USING (id) | ||
| ORDER BY seq | ||
| $$, edges_subset); | ||
| final_query := ' | ||
| WITH | ||
| results AS (' || resuts_query || ' ), | ||
|
|
||
| additional AS ( | ||
| SELECT | ||
| seq, id, seconds, name, length, | ||
| CASE | ||
| WHEN node = source THEN geom | ||
| ELSE ST_Reverse(geom) | ||
| END AS geom | ||
| FROM results | ||
| LEFT JOIN vehicle_net USING (id) | ||
| ORDER BY seq) | ||
|
|
||
| SELECT seq, id, seconds, name, length, | ||
| degrees(ST_azimuth(ST_StartPoint(geom), ST_EndPoint(geom))) AS azimuth, | ||
| ST_AsText(geom), geom | ||
| FROM additional ORDER BY seq; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Filter the terminal edge = -1 row before joining metadata.
pgr_withPoints appends a sentinel row with edge = -1. This code aliases it to id and left-joins vehicle_net, so the function returns an extra row with NULL name/length/geom. That also makes example include a blank “segment”.
Suggested fix
resuts_query := format(
$$SELECT seq, edge AS id, node, cost AS seconds
FROM pgr_withPoints(
'SELECT * FROM vehicle_penalized_net',
'%1$s',
-1, -2)
+ WHERE edge <> -1
$$, points_sql);📝 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
| resuts_query := format( | |
| $$SELECT seq, edge AS id, node, cost AS seconds | |
| FROM pgr_withPoints( | |
| 'SELECT * FROM %1$I', | |
| '%2$s', | |
| 'SELECT * FROM vehicle_penalized_net', | |
| '%1$s', | |
| -1, -2) | |
| $$, edges_subset, closest_query); | |
| $$, points_sql); | |
| -- 2 | |
| additional_query := format( | |
| $$SELECT | |
| seq, id, seconds, name, length, | |
| CASE | |
| WHEN node = source THEN ST_AsText(geom) | |
| ELSE ST_AsText(ST_Reverse(geom)) | |
| END AS readable, | |
| CASE | |
| WHEN node = source THEN geom | |
| ELSE ST_Reverse(geom) | |
| END AS geom | |
| FROM results | |
| LEFT JOIN %1$I USING (id) | |
| ORDER BY seq | |
| $$, edges_subset); | |
| final_query := ' | |
| WITH | |
| results AS (' || resuts_query || ' ), | |
| additional AS ( | |
| SELECT | |
| seq, id, seconds, name, length, | |
| CASE | |
| WHEN node = source THEN geom | |
| ELSE ST_Reverse(geom) | |
| END AS geom | |
| FROM results | |
| LEFT JOIN vehicle_net USING (id) | |
| ORDER BY seq) | |
| SELECT seq, id, seconds, name, length, | |
| degrees(ST_azimuth(ST_StartPoint(geom), ST_EndPoint(geom))) AS azimuth, | |
| ST_AsText(geom), geom | |
| FROM additional ORDER BY seq; | |
| resuts_query := format( | |
| $$SELECT seq, edge AS id, node, cost AS seconds | |
| FROM pgr_withPoints( | |
| 'SELECT * FROM vehicle_penalized_net', | |
| '%1$s', | |
| -1, -2) | |
| WHERE edge <> -1 | |
| $$, points_sql); | |
| -- 2 | |
| final_query := ' | |
| WITH | |
| results AS (' || resuts_query || ' ), | |
| additional AS ( | |
| SELECT | |
| seq, id, seconds, name, length, | |
| CASE | |
| WHEN node = source THEN geom | |
| ELSE ST_Reverse(geom) | |
| END AS geom | |
| FROM results | |
| LEFT JOIN vehicle_net USING (id) | |
| ORDER BY seq) | |
| SELECT seq, id, seconds, name, length, | |
| degrees(ST_azimuth(ST_StartPoint(geom), ST_EndPoint(geom))) AS azimuth, | |
| ST_AsText(geom), geom | |
| FROM additional ORDER BY seq; |
🤖 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 71 - 99, The query
in withPoints.sql is including the terminal sentinel row from pgr_withPoints
because it aliases edge to id and joins it through results/additional, which
leaves an extra blank segment. Update the resuts_query/final_query flow to
filter out the row where edge = -1 before the LEFT JOIN to vehicle_net (or
before building additional), so only real path segments are returned and the
example no longer includes NULL metadata.
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
1 participant
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 sql-function chapter
@pgRouting/admins
Summary by CodeRabbit
New Features
Bug Fixes