gregfelice commented on PR #2430: URL: https://github.com/apache/age/pull/2430#issuecomment-4763263433
Reviewed the full diff — traced the BFS/enumeration against the regression cases and confirmed the supporting helpers. This is solid, well-tested work; I'd merge it after a couple of minor cleanups. (I read the code carefully but did not rebuild locally — the 37/37 installcheck result is the author's.) ### Strengths - **Algorithm is correct.** BFS pops in non-decreasing depth order, so single-path mode's break-on-first-pop yields the true shortest length, and all-paths mode correctly keeps expanding, prunes at `du >= target_depth`, and records every equal-depth predecessor. I checked that this does not double-count parallel edges or produce spurious back-edge predecessors in undirected mode. The "every emitted path is simple because BFS depth strictly increases" reasoning holds. - **The non-existent-endpoint guard is a genuine crash fix** (previously `source == end` on a missing vertex matched at depth 0 and dereferenced a missing vertex), and it has dedicated regression coverage. - **Coverage is thorough:** directed/undirected, label filtering, parallel edges, self-loops, `max_hops`, zero-length, unreachable, the not-supported stubs, NULL/non-existent/empty-graph guards, a 120-node large graph, and the `cypher() RETURN` integration path that exercises the `hasTargetSRFs` + `transform_FuncCall` changes. - `CHECK_FOR_INTERRUPTS()` is correctly placed in the BFS, the recursive enumerator, and both DFS loops. ### Suggestions (non-blocking) 1. **Error messages hardcode `"age_shortest_path"`.** Since `age_all_shortest_paths` shares `sp_compute_paths` / `sp_agtype_to_*`, a bad argument to the all-paths function reports an `age_shortest_path:` prefix. Consider threading the real function name through (or dropping the prefix). 2. **No result-count bound on `age_all_shortest_paths`.** The shortest-path DAG can yield exponentially many paths, all materialized up front in `multi_call_memory_ctx`. Only `CHECK_FOR_INTERRUPTS` bounds it. Worth documenting, ideally a configurable cap. 3. **Scratch-memory lifetime.** `visited`, the `preds` lists/`sp_pred` nodes, and the `sp_queue` buffer live in `multi_call_memory_ctx` for the whole SRF though they're only needed during computation. A short-lived child context reset after building the path Datums would bound peak memory. 4. **PR description scope.** The body describes `age_global_graph.c` changes (`get_label_relation`->`InvalidOid`, the `manage_GRAPH_global_contexts` self-deadlock fix), but those aren't in this diff — they already landed on master. Trimming the description to the 8 files actually touched would help reviewers. Nice work — the crash guard and the test matrix in particular are well done. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
