rubenada commented on a change in pull request #2027:

File path: core/src/main/java/org/apache/calcite/plan/
@@ -234,10 +234,10 @@ private ConversionData getConversionData(RelOptPlanner 
planner) {
       return pathMap;
-    public List<Convention> getShortestPath(
+    public int getShortestDistance(

Review comment:
       IMHO this is not the ideal approach, now we would be re-computing 
everything time something (shortest path) that was previously computed only 
once and returned in O(1).
   If we take a few steps back, I think it is clear that we require both pieces 
of information (shortesPath & distance), to be provided ASAP from 
`FrozenGraph`. Then why not just pre-computing both in `Graphs#makeImmutable` 
and storing both of them in `FrozenGraph`, so that we guarantee that 
`getShortestPath` & `getShortestDistance` are executed in O(1)?
   I think we could keep track of shortestPath and distance in 
`Graphs#makeImmutable`, somehow combining the old approach with the newly 
proposed approach, either keeping two maps:
   Map<Pair<V, V>, List<V>> shortestPaths
   Map<Pair<V, V>, int[]> shortestDistances
   Or a single map with the combination of both as value:
   Map<Pair<V, V>, Pair<List<V>, Integer>> shortestPathsAndDistances
   Then we would pass this information as a parameter for FrozeGraph 
constructor, and we would have shortesPath & distance pre-computed from the 
   I'm not sure if what I say makes sense or it is an overkill. What do you 
guys think?

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.

For queries about this service, please contact Infrastructure at:

Reply via email to