rubenada commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r441533364
##########
File path: core/src/main/java/org/apache/calcite/plan/ConventionTraitDef.java
##########
@@ -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
beginning.
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:
us...@infra.apache.org
