Implement multi-source shortest path query support by patelchaitany · Pull Request #654 · turing-db/turingdb

patelchaitany · 2026-05-27T08:33:36Z

Add multiSourceShortestPath processor that computes shortest paths from a set of source nodes to all reachable targets, returning one row per (source, target) pair with distance and path — extends the existing shortestPath which only returns a single best pair. Closes #617

patelchaitany · 2026-05-30T20:44:10Z

@rjb32 if you get time pls review this PR.

rjb32 · 2026-05-31T14:22:41Z

@rjb32 if you get time pls review this PR.

Thank you very much @patelchaitany! I will review it on Monday

patelchaitany · 2026-06-03T14:56:02Z

Hey @rjb32! Whenever you get a moment, I'd love to get your thoughts on this PR. Thanks!

sulaimansuhas · 2026-06-04T10:59:07Z

Hey @patelchaitany - I have looked at the PR and had a few overall comments:

Can you think of anyways we can re-use explored paths between source nodes? This would help make the algorithm a lot more efficient I feel.
I think there is a lot of overlap between the ShortestPath processor code and the multi-path shortest path processor code. It would be nice to wrap up common logic and data structures into a single ShortestPathUtils.h/.cpp file!
I think we need a lot more comments in your code - especially around the shortest path algorithm.

Overall the code looks good and conforms to our standards well:)!

rjb32 · 2026-06-05T15:56:06Z

+                                                      ColumnVector<NodeID>* targetOutputCol,
+                                                      ColumnVector<EdgePropType>* distCol,
+                                                      ColumnVector<Path>* pathCol) {
+    DijkstraHeap heap;


All these structures are going to be recreated per runDijkstra call, you could save them as members of the processor class and clear them on entry in the function

rjb32 · 2026-06-05T15:57:32Z

+class MultiSourceShortestPathProcessor final : public Processor {
+public:
+    using EdgePropType = T::Primitive;
+    using DijkstraHeap = std::priority_queue<MultiSourceDijkstraNode<EdgePropType>,


Probably better to extract all these type definitions and the runDijkstra mechanics into some utility class that would wrap up everything related to Dijkstra algorithm

rjb32 · 2026-06-05T15:59:36Z

+            throw PlannerException("Unsupported Edge Weight Type");
+        }
+    };
+    ValueTypeDispatcher {edgeType._valueType}.execute(process);


Coding style: we prefer to use the parenthesis for constructors instead of braces, such as ValueTypeDispatcher(edgeType._valueType)

patelchaitany · 2026-06-06T18:08:18Z

@rjb32 @sulaimansuhas Feedback addressed! Please take another look and let me know if any further changes are needed. Happy to revise.

sulaimansuhas · 2026-06-10T13:24:24Z

Hey @patelchaitany. I looked over your changes and it mostly looks good! I have two comments:

I think the Dijkstra Search Algorithm itself should be pulled out into a common function, as there is a lot of overlapping logic that would be painful to keep in sync between these two implementations.
I feel that there is scope to optimise this code - we should be able to cache already explored paths in previous runs with different source nodes. Please give me your thoughts on this (even why it wouldn't be feasible).

thank you for your good work!

patelchaitany · 2026-06-11T12:41:42Z

I'm going to expand ShortestPathUtils into a DijkstraRunner<T> class that owns the heap and value map as members (reused across calls instead of reallocated) and holds the core Dijkstra loop - expansion, stale filtering, relaxation, and path reconstruction. A stopAtFirst flag on run() handles both use cases: ShortestPathProcessor stops at the first settled target, MultiSourceShortestPathProcessor keeps going until all are found, so both processors shrink down to just port wiring and result extraction. On caching across source nodes - the shortest-path trees are source-dependent so intermediate results don't transfer cleanly; the early-exit we already have cuts out the main waste, and anything beyond that (bidirectional Dijkstra, landmarks) feels like a separate effort. Will also fix the ValueTypeDispatcher brace style Remy flagged.

sulaimansuhas · 2026-06-12T15:47:33Z

Hey @patelchaitany - your code changes sound good, I'll be waiting to review! As for your comments on the shortest path optimisations:

If we take the shortest path between node a and z to be a->p->x->z, then for that path we are guaranteed that every subpath of the path is the optimal shortest path from the source node of that subpath to the target. So from this single traversal we can infer that the shortest path from p to z is p->x->z.

Given this finding, two optimisations I can think of off the top of my head:

if p was in the source node set we already have a shortest path to a node in the target set (z).
if in any other traversal we pop p from the top of the stack we immediately can know that there is a path to a target node and the size of the path.

these are the ideas that gave me the intuition for optimisation in this case. I think it'd be a shame to get this PR through without exploring these kind of optimisations as I think they have the potential to reduce a lot of computation.

I'd like to hear your thoughts on this, and thank you for all your good work!

Add multiSourceShortestPath query support that computes shortest paths from a set of source nodes to all reachable targets, returning one row per (source, target) pair with distance and path.

shortest path

patelchaitany · 2026-06-16T06:54:04Z

@sulaimansuhas - subpath caching is in. After each run we cache intermediate-to-target subpaths, then before the next source we check the cache and skip Dijkstra if we already have the answer. Mid-traversal hits are handled as pending results that only finalize when the heap confirms optimality.

Right now the cache is query-scoped - thinking a cross-query version keyed to commit hashes (with eviction like ShardCache does for vectors) would be a natural follow-up. Thoughts?

patelchaitany · 2026-07-02T04:21:58Z

Hey @rjb32 , @sulaimansuhas Is there any changes are required.

sulaimansuhas · 2026-07-02T07:27:34Z

Hey @patelchaitany, sorry for taking so long! I looked through the PR it looks pretty good. I'll have some final comments that I'll add later today.

sulaimansuhas

Hey @patelchaitany, the overall logic of your change is sound. I have left quite a few comments explaining what would be needed to make this mergeable. I think we need a lot more comments around the algorithm - in places my previous comments have been removed where my code was copied. If you are using AI coding tools - I request that you write the comments yourself because we generally find the AI generated comments to not be that clear to human readers, and it will also help you reason about the logic itself.

sulaimansuhas · 2026-07-02T10:00:50Z

+void DijkstraRunner<T>::run(const DijkstraHeap<EdgePropType>& initialHeap,
+                            const DijkstraValueMap<EdgePropType>& initialValues,
+                            const std::unordered_set<NodeID>& targetNodes,
+                            bool stopAtFirst,


I don't like having the bool flag, I think it should be a number. This would make it trivial to create a top K shortest paths algorithm in the future.

sulaimansuhas · 2026-07-02T10:04:26Z

+        const DijkstraNode<EdgePropType> val = _heap.top();
+        _heap.pop();
+
+        const auto it = _heapValueMap.find(val.id);


Please add a comment explaining that we are removing the stale values here.

sulaimansuhas · 2026-07-02T10:31:54Z

+
+        // Finalize pending cache-hit results whose distance cannot be beaten
+        // by any future path (all remaining nodes have distance >= val.distance).
+        for (auto pendingIt = _pendingResults.begin(); pendingIt != _pendingResults.end(); ) {


Nice! Please add more comments explaining what is happening here. And expand on the why - these are complex algorithms that we want to as readable as possible.

sulaimansuhas · 2026-07-02T10:45:51Z

+            }
+        }
+
+        // Consult the subpath cache: if this settled node has known shortest


This comment is not clear at all, please clearly explain the logic behind this cache search better. Something like:
If the current popped node is found in the cache then we know we have the shortest path from our source to the Target Node in the cache entry.

Also please clarify the explanation on why we need the pending result sets. It is useful but it took me a while to understand it just based on the code.

sulaimansuhas · 2026-07-02T10:47:31Z

+            const auto cacheIt = cache->find(val.id);
+            if (cacheIt != cache->end()) {
+                for (const SubpathCacheEntry<EdgePropType>& entry : cacheIt->second) {
+                    if (!targetNodes.contains(entry.targetNode)) {


both these if conditions should be one statement.

sulaimansuhas · 2026-07-02T11:05:20Z

+private:
+    DijkstraHeap<EdgePropType> _heap;
+    DijkstraValueMap<EdgePropType> _heapValueMap;
+    std::unordered_set<NodeID> _settledTargets;


I think it makes more sense to pass the constructed target nodes to the Utils class as a const, and have the settledTargets passed to the function itself. This way we don't have to create a copy of the targetNodes set every time. This would be especially useful if the target nodes set itself is quite large.

sulaimansuhas · 2026-07-02T11:05:44Z

+    std::vector<DijkstraResult<EdgePropType>> _results;
+    std::unordered_map<NodeID, DijkstraResult<EdgePropType>> _pendingResults;
+
+    ColumnNodeIDs* _inputNodes {nullptr};


We can forward declare all these pointer members.

sulaimansuhas · 2026-07-02T11:06:58Z

+}
+
+template <SupportedType T>
+void DijkstraRunner<T>::expandNode(const DijkstraNode<EdgePropType>& node) {


please add back all the comments that were removed here ( see original algo)

sulaimansuhas · 2026-07-02T11:30:16Z

+    // Start with the cached path suffix: [target, edge, ..., edge, settledNode]
+    outputPath = cacheEntry.pathSuffix;
+
+    // Append the predecessor chain from settledNode back to the source.
+    auto lastNode = settledNode.prevNode;
+    auto edge = settledNode.edge;
+    while (lastNode.isValid()) {
+        outputPath.push_back(edge.getValue());
+        outputPath.push_back(lastNode.getValue());
+
+        const auto& pathInfo = _heapValueMap[lastNode];
+        lastNode = pathInfo.prevNode;
+        edge = pathInfo.edge;
+    }
+}


Suggested change

// Start with the cached path suffix: [target, edge, ..., edge, settledNode]

outputPath = cacheEntry.pathSuffix;

// Append the predecessor chain from settledNode back to the source.

auto lastNode = settledNode.prevNode;

auto edge = settledNode.edge;

while (lastNode.isValid()) {

outputPath.push_back(edge.getValue());

outputPath.push_back(lastNode.getValue());

const auto& pathInfo = _heapValueMap[lastNode];

lastNode = pathInfo.prevNode;

edge = pathInfo.edge;

}

}

// Start with the cached path suffix without the starting node: [target, edge, ..., edge]

outputPath = cacheEntry.pathSuffix | ranges::views::drop_last(1);

reconstructPath(settledNode, outputPath);

}

}

sulaimansuhas · 2026-07-02T11:30:47Z

+struct SubpathCacheEntry {
+    NodeID targetNode;
+    T distance {0};
+    Path pathSuffix;


pathSuffix is not the correct name for this. Path would be fine.

patelchaitany · 2026-07-02T12:33:22Z

@sulaimansuhas, Okay I will address your comments and not try to use AI for comments

patelchaitany requested a review from rjb32 as a code owner May 27, 2026 08:33

rjb32 reviewed Jun 5, 2026

View reviewed changes

patelchaitany requested a review from rjb32 June 6, 2026 18:06

patelchaitany added 4 commits June 13, 2026 22:53

Implement multi-source shortest path processor

ffa3251

Add multiSourceShortestPath query support that computes shortest paths from a set of source nodes to all reachable targets, returning one row per (source, target) pair with distance and path.

Extract Dijkstra types into ShortestPathUtils

87dc7c3

Extract shared DijkstraRunner and add subpath caching for multi-source

ae58192

shortest path

Update MultiSourceShortestPathQueryTest to use SystemAccessor

dc37525

patelchaitany force-pushed the feature/617-multi-source-shortest-path branch from d6adf47 to dc37525 Compare June 13, 2026 19:08

sulaimansuhas requested changes Jul 2, 2026

View reviewed changes

Uh oh!

Conversation

patelchaitany commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patelchaitany commented May 30, 2026

Uh oh!

rjb32 commented May 31, 2026

Uh oh!

patelchaitany commented Jun 3, 2026

Uh oh!

sulaimansuhas commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patelchaitany commented Jun 6, 2026

Uh oh!

sulaimansuhas commented Jun 10, 2026

Uh oh!

patelchaitany commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sulaimansuhas commented Jun 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patelchaitany commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patelchaitany commented Jul 2, 2026

Uh oh!

sulaimansuhas commented Jul 2, 2026

Uh oh!

sulaimansuhas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patelchaitany commented Jul 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

patelchaitany commented May 27, 2026 •

edited

Loading

sulaimansuhas commented Jun 4, 2026 •

edited

Loading

patelchaitany commented Jun 11, 2026 •

edited

Loading

sulaimansuhas commented Jun 12, 2026 •

edited

Loading

patelchaitany commented Jun 16, 2026 •

edited

Loading