Sql function chapter rewrite #259
Conversation
WalkthroughReorders two CMake variable initializations; replaces several chapter images and rewrites the SQL function chapter and related script/build files; renames Sphinx substitution tokens; removes wrk_image and adds/expands wrk_dijkstra outputs and a sql_route_geom view; adds ID lookup queries; refreshes localization strings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant View as View (using_vehicle / sql_route_geom)
participant FN as wrk_dijkstra(edges_subset, source, target)
participant PGR as pgr_dijkstra
participant Net as edges table (vehicle_net)
Client->>View: SELECT ... FROM view
View->>FN: call wrk_dijkstra('vehicle_net', CH7_ID_1, CH7_ID_2)
FN->>PGR: pgr_dijkstra(using dynamic edges_subset, source, target)
PGR-->>FN: routing rows (seq, edge, node, cost)
FN->>Net: JOIN routing rows to edges_subset to fetch geom/attrs
Note over FN: construct outputs → seq,id,seconds,name,length\noptionally route_readable / route_geom / azimuth
FN-->>View: SETOF route rows
View-->>Client: final result set (e.g., seq,id,geom or seq,azimuth)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
🔇 Additional comments (1)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
docs/basic/images/sql_function/ch7-e4.pngis excluded by!**/*.pngdocs/basic/images/sql_function/ch7-e5.pngis excluded by!**/*.pngdocs/basic/images/sql_function/ch7-e6.pngis excluded by!**/*.pngdocs/basic/images/sql_function/ch7-e7.pngis excluded by!**/*.pngdocs/basic/images/sql_function/ch7-e8-1.pngis excluded by!**/*.pngdocs/basic/images/sql_function/ch7-e8.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!**/*.png
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)docs/basic/images/sql_function/CMakeLists.txt(1 hunks)docs/basic/sql_function.rst(13 hunks)docs/conf.py(1 hunks)docs/scripts/basic/sql_function/CMakeLists.txt(1 hunks)docs/scripts/basic/sql_function/images.sql(1 hunks)docs/scripts/basic/sql_function/sql_function.sql(1 hunks)docs/scripts/configuration/osmid_configuration.sql(1 hunks)locale/en/LC_MESSAGES/basic/sql_function.po(3 hunks)locale/pot/basic/sql_function.pot(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-04T19:55:45.645Z
Learnt from: cvvergara
PR: pgRouting/workshop#257
File: docs/basic/pedestrian.rst:160-161
Timestamp: 2025-10-04T19:55:45.645Z
Learning: In the pgRouting workshop's pedestrian routing chapter (docs/basic/pedestrian.rst), the same image can be used for both many-to-one and one-to-many routing exercises because pedestrian routes are undirected (bidirectional). The visual representation of the path is identical regardless of direction.
Applied to files:
docs/basic/sql_function.rst
⏰ 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: Build
🔇 Additional comments (17)
CMakeLists.txt (1)
82-84: LGTM!Reordering the CH7 variable initializations groups them together logically. The values remain unchanged, and this improves readability.
docs/scripts/configuration/osmid_configuration.sql (1)
23-29: LGTM!The new WITH blocks for CH7_OSMID_1 and CH7_OSMID_2 follow the same pattern as the existing ID_ blocks. The output is correctly prefixed with 'CH7_ID_' to distinguish these identifiers, which aligns with the broader refactoring in this PR.
locale/pot/basic/sql_function.pot (2)
2-11: LGTM!The POT file metadata has been updated correctly to reflect the 2025 copyright, version 3.1, and the latest creation date. These changes align with the project's versioning and documentation refresh.
36-567: Localization strings updated to reflect documentation changes.The extensive updates to msgid entries throughout this POT file accurately reflect the documentation rewrite, including:
- Terminology changes (source_osm → source, target_osm → target)
- New output descriptions (azimuth, route_readable, route_geom)
- Exercise restructuring and renaming
As this is an auto-generated localization file, these changes are expected and consistent with the broader PR.
docs/basic/sql_function.rst (9)
14-14: Verify that the new image file exists.The image reference has been changed from
pedestrian_one_to_one.pngtosql_route_names.png. Ensure this new image file exists in thedocs/basic/images/sql_function/directory.This verification is covered by the script in the first file review.
29-48: LGTM!The section heading change from "The application requirements" to "The function requirements" better reflects that this chapter focuses on creating a SQL function wrapper. The updated list clearly specifies the function's purpose (wrapping
pgr_dijkstra), supported networks, and required outputs.
61-62: LGTM!Simplifying parameter names from
source_osm/target_osmtosource/targetimproves clarity and aligns with the broader identifier refactoring in this PR (moving from OSMID-based to ID-based references).
162-174: Verify exercise 2 literalinclude references.Similar to exercise 1, verify that the markers
get_read_geom.txtandget_geom.txtexist insql_function.sqland that the emphasized lines (6, 11, 19) are correct.This verification is covered by the script in the previous comment.
200-210: Verify exercise 3 literalinclude references.Verify that the markers
get_geom.txtandwrong_directionality.txtexist insql_function.sqland that the emphasized lines (7, 21) are correct.This verification is covered by the script above.
266-275: Verify exercise 4 literalinclude references.Verify that the markers
fix_directionality.txtandgood_directionality.txtexist insql_function.sql.This verification is covered by the script above.
316-326: Verify exercise 5 literalinclude references.Verify that the markers
use_directionality.txtandusing_fn1.txtexist insql_function.sqland that the emphasized lines (8, 18, 35) are correct.This verification is covered by the script above.
90-210: Consider consolidating image verification comments.Multiple image files are referenced throughout this document (
sql_route_names.png,sql_route_readable.png,sql_route_geom.png,sql_route_geom_detail.png,sql_azimuth_fixed.png). The verification script in the first file review already checks for all these images.
118-128: All markers and emphases verified. Theget_more_info.txtandget_read_geom.txtmarkers exist, and lines 4, 12, and 17 in the snippet correctly highlight the function signature, the start of the main SELECT block, and the end of the subquery.docs/basic/images/sql_function/CMakeLists.txt (1)
6-11: New images present and old files removed
All fivesql_*.pngfiles exist indocs/basic/images/sql_function/and there are no remainingch7-*.pngfiles or references.docs/scripts/basic/sql_function/images.sql (2)
2-4: No action needed – wrk_dijkstra signature confirmed: wrk_dijkstra is defined in docs/scripts/basic/sql_function/sql_function.sql and accepts (IN edges_subset REGCLASS, IN source BIGINT, IN target BIGINT).
6-9: Removed views not referenced; new view linked correctly
✓ No occurrences of using_taxi, using_walk, or wrk_image found in docs.
✓ sql_route_geom is referenced in documentation and image assets.docs/scripts/basic/sql_function/sql_function.sql (1)
191-196: Construct comment:**Join should honor the edges_subset argument** Hard-coded join to vehicle_net ...Add fix suggestion maybe unstoppable.
Let's craft comment with short explanation and diff.
Diff propose converting join to dynamic referencing edges subset? Provide diff snippet.
Maybe simple fix:
LEFT JOIN vehicle_netreplaced withLEFT JOIN pg_catalog.quote_ident? can't in static diff. maybe quick fix:- LEFT JOIN vehicle_net USING (id) + LEFT JOIN pg_temp_edges_subset USING (id)But unstoppable.
Better to propose convert to plpgsql? diff more complex.
We can provide high-level suggestion without diff? instructions prefer diff but only if applicable; we can skip if fix not straightforward.
But we can show diff converting to dynamic
EXECUTE format. Could be large though; but mania.Maybe more manageable to wrap join to use
format('%s', edges_subset)by callingREGCLASSedges_subsetwithin SQL function by usingformat('%s', edges_subset)? Wait within SQL function we can't callformat? There isformatfunction accessibleformat('%s', edges_subset)inside plain SQL? we can callSELECT format('%s', edges_subset);but we can't parameterize table name. But we can try to useLEFT JOIN (SELECT * FROM public.vehicle_net)??can't.Therefore recommending restructure to plpgsql.
Let's craft comment referencing this change and propose to convert function to plpgsql so dynamic join uses the regclass. Provide diff.
Due to instructions to be precise, we can supply diff snippet but intangible.
Maybe glimpsed:
-$BODY$ -... LEFT JOIN vehicle_net USING (id) +DECLARE + edges_sql text := format('%s', edges_subset); +BEGIN + RETURN QUERY EXECUTE format($f$ + WITH results AS (... 'SELECT * FROM %1$s' ... ) + SELECT ... + FROM results + LEFT JOIN %1$s USING (id) + ORDER BY seq + $f$, edges_sql) USING source, target; +END; +$BODY$ -LANGUAGE SQL; +LANGUAGE plpgsql;Need to include TOT? We'll craft diff accordingly.
Let's produce comment now.
Finally close file_end. Done.
Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores