Skip to content

[CALCITE-4049] Improve the implementation of the shortest-path algorithm #2027

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rubenada merged 1 commit into from
Jun 30, 2020
Merged

[CALCITE-4049] Improve the implementation of the shortest-path algorithm #2027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions core/src/main/java/org/apache/calcite/plan/ConventionTraitDef.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ public boolean canConvert(
Convention toConvention) {
ConversionData conversionData = getConversionData(planner);
return fromConvention.canConvertConvention(toConvention)
|| conversionData.getShortestPath(fromConvention, toConvention) != null;
|| conversionData.getShortestDistance(fromConvention, toConvention) != -1;
}

private ConversionData getConversionData(RelOptPlanner planner) {
Expand Down Expand Up @@ -234,10 +234,10 @@ private Graphs.FrozenGraph<Convention, DefaultEdge> getPathMap() {
return pathMap;
}

public List<Convention> getShortestPath(
public int getShortestDistance(
Copy link
Contributor

@xndai xndai Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also fix the getPaths() to return shortest paths first? This is to make sure we choose the shortest path during convert.

Copy link
Contributor Author

@liyafan82 liyafan82 Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it would be an overkill to call getPaths() only to get the shortest path.
To support this scenario, I have restored the getShortestPath API, and implement it with BFS. It should be more efficient than the Dijkstra and Floyd algorihtms.

Copy link
Contributor

@rubenada rubenada Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@xndai xndai Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liyafan82 I am not saying getPaths() to return shortest paths. What I mean is you should put shortest path at the front of list, so during convert() we always choose the shortest converted path if possible. This only requires a sort after generating all possible path.

@rubenada I don't think we need to cache shortest path. This is only one time used and there's no benefit to cache it.

Copy link
Contributor Author

@liyafan82 liyafan82 Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xndai I have revised the getPaths() method accordingly. Thank you.

@rubenada Thanks a lot for your suggestion. I tend to agree with @xndai. If getting shortest paths is not a frequently used operation, there is no need to store all pairs of shortest paths. Here, we preserve the getShortestPath API, mainly for the sake of backward compatibility.

Convention fromConvention,
Convention toConvention) {
return getPathMap().getShortestPath(fromConvention, toConvention);
return getPathMap().getShortestDistance(fromConvention, toConvention);
}
}
}
4 changes: 2 additions & 2 deletions core/src/main/java/org/apache/calcite/plan/RelOptMaterializations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,8 +254,8 @@ private static boolean usesTable(
Set<RelOptTable> usedTables,
Graphs.FrozenGraph<List<String>, DefaultEdge> usesGraph) {
for (RelOptTable queryTable : usedTables) {
if (usesGraph.getShortestPath(queryTable.getQualifiedName(), qualifiedName)
!= null) {
if (usesGraph.getShortestDistance(queryTable.getQualifiedName(), qualifiedName)
!= -1) {
return true;
}
}
Expand Down
9 changes: 6 additions & 3 deletions core/src/main/java/org/apache/calcite/runtime/ConsList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
public class ConsList<E> extends AbstractImmutableList<E> {
private final E first;
private final List<E> rest;
private final int size;

/** Creates a ConsList.
* It consists of an element pre-pended to another list.
Expand All @@ -52,7 +51,6 @@ public static <E> List<E> of(E first, List<? extends E> rest) {
private ConsList(E first, List<E> rest) {
this.first = first;
this.rest = rest;
this.size = 1 + rest.size();
}

public E get(int index) {
Expand All @@ -68,7 +66,12 @@ public E get(int index) {
}

public int size() {
return size;
int s = 1;
for (ConsList c = this;; c = (ConsList) c.rest, ++s) {
if (!(c.rest instanceof ConsList)) {
return s + c.rest.size();
}
}
}

@Override public int hashCode() {
Expand Down
64 changes: 25 additions & 39 deletions core/src/main/java/org/apache/calcite/util/graph/Graphs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,13 @@

import java.util.AbstractList;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

import static org.apache.calcite.util.Static.cons;

/**
* Miscellaneous graph utilities.
*/
Expand All @@ -56,42 +55,40 @@ public int size() {
public static <V, E extends DefaultEdge> FrozenGraph<V, E> makeImmutable(
DirectedGraph<V, E> graph) {
DefaultDirectedGraph<V, E> graph1 = (DefaultDirectedGraph<V, E>) graph;
Map<Pair<V, V>, List<V>> shortestPaths = new HashMap<>();
Map<Pair<V, V>, int[]> shortestDistances = new HashMap<>();
for (DefaultDirectedGraph.VertexInfo<V, E> arc
: graph1.vertexMap.values()) {
for (E edge : arc.outEdges) {
final V source = graph1.source(edge);
final V target = graph1.target(edge);
shortestPaths.put(Pair.of(source, target),
ImmutableList.of(source, target));
shortestDistances.put(Pair.of(source, target), new int[] {1});
}
}
while (true) {
// Take a copy of the map's keys to avoid
// ConcurrentModificationExceptions.
final List<Pair<V, V>> previous =
ImmutableList.copyOf(shortestPaths.keySet());
int changeCount = 0;
ImmutableList.copyOf(shortestDistances.keySet());
boolean changed = false;
for (E edge : graph.edgeSet()) {
for (Pair<V, V> edge2 : previous) {
if (edge.target.equals(edge2.left)) {
final Pair<V, V> key = Pair.of(graph1.source(edge), edge2.right);
List<V> bestPath = shortestPaths.get(key);
List<V> arc2Path = shortestPaths.get(edge2);
if ((bestPath == null)
|| (bestPath.size() > (arc2Path.size() + 1))) {
shortestPaths.put(key,
cons(graph1.source(edge), arc2Path));
changeCount++;
int[] bestDistance = shortestDistances.get(key);
int[] arc2Distance = shortestDistances.get(edge2);
if ((bestDistance == null)
|| (bestDistance[0] > (arc2Distance[0] + 1))) {
shortestDistances.put(key, new int[] {arc2Distance[0] + 1});
changed = true;
}
}
}
}
if (changeCount == 0) {
if (!changed) {
break;
}
}
return new FrozenGraph<>(graph1, shortestPaths);
return new FrozenGraph<>(graph1, shortestDistances);
}

/**
Expand All @@ -102,39 +99,29 @@ public static <V, E extends DefaultEdge> FrozenGraph<V, E> makeImmutable(
*/
public static class FrozenGraph<V, E extends DefaultEdge> {
private final DefaultDirectedGraph<V, E> graph;
private final Map<Pair<V, V>, List<V>> shortestPaths;
private final Map<Pair<V, V>, int[]> shortestDistances;

/** Creates a frozen graph as a copy of another graph. */
FrozenGraph(DefaultDirectedGraph<V, E> graph,
Map<Pair<V, V>, List<V>> shortestPaths) {
Map<Pair<V, V>, int[]> shortestDistances) {
this.graph = graph;
this.shortestPaths = shortestPaths;
this.shortestDistances = shortestDistances;
}

/**
* Returns an iterator of all paths between two nodes, shortest first.
* Returns an iterator of all paths between two nodes,
* in non-decreasing order of path lengths.
*
* <p>The current implementation is not optimal.</p>
*/
public List<List<V>> getPaths(V from, V to) {
List<List<V>> list = new ArrayList<>();
findPaths(from, to, list);
return list;
}

/**
* Returns the shortest path between two points, null if there is no path.
*
* @param from From
* @param to To
*
* @return A list of arcs, null if there is no path.
*/
public List<V> getShortestPath(V from, V to) {
if (from.equals(to)) {
return ImmutableList.of();
list.add(ImmutableList.of(from));
}
return shortestPaths.get(Pair.of(from, to));
findPaths(from, to, list);
list.sort(Comparator.comparingInt(List::size));
return list;
}

/**
Expand All @@ -147,13 +134,12 @@ public int getShortestDistance(V from, V to) {
if (from.equals(to)) {
return 0;
}
List<V> path = shortestPaths.get(Pair.of(from, to));
return path == null ? -1 : path.size() - 1;
int[] distance = shortestDistances.get(Pair.of(from, to));
return distance == null ? -1 : distance[0];
}

private void findPaths(V from, V to, List<List<V>> list) {
final List<V> shortestPath = shortestPaths.get(Pair.of(from, to));
if (shortestPath == null) {
if (getShortestDistance(from, to) == -1) {
return;
}
// final E edge = graph.getEdge(from, to);
Expand Down
30 changes: 19 additions & 11 deletions core/src/test/java/org/apache/calcite/util/graph/DirectedGraphTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,21 @@ class DirectedGraphTest {
g.addEdge("B", "D");
assertEquals("[A, B, D]", shortestPath(g, "A", "D").toString());
assertNull(shortestPath(g, "A", "E"), "There is no path from A to E");
assertEquals("[]", shortestPath(g, "D", "D").toString());
assertEquals("[D]", shortestPath(g, "D", "D").toString());
assertNull(shortestPath(g, "X", "A"), "Node X is not in the graph");
assertEquals("[[A, B, C, D], [A, B, D]]", paths(g, "A", "D").toString());
assertEquals("[[A, B, D], [A, B, C, D]]", paths(g, "A", "D").toString());
Copy link
Contributor

@xndai xndai Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where are the tests for getShortestDistance()?

Copy link
Contributor Author

@liyafan82 liyafan82 Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have DirectedGraphTest#testDistance

}

private <V> List<V> shortestPath(DirectedGraph<V, DefaultEdge> g,
V source, V target) {
return Graphs.makeImmutable(g).getShortestPath(source, target);
List<List<V>> paths = Graphs.makeImmutable(g).getPaths(source, target);
return paths.isEmpty() ? null : paths.get(0);
}

private <V> List<V> shortestPath(Graphs.FrozenGraph<V, DefaultEdge> g,
V source, V target) {
List<List<V>> paths = g.getPaths(source, target);
return paths.isEmpty() ? null : paths.get(0);
}

private <V> List<List<V>> paths(DirectedGraph<V, DefaultEdge> g,
Expand Down Expand Up @@ -217,15 +224,16 @@ private DefaultDirectedGraph<String, DefaultEdge> createDag1() {
final DefaultDirectedGraph<String, DefaultEdge> graph = createDag1();
final Graphs.FrozenGraph<String, DefaultEdge> frozenGraph =
Graphs.makeImmutable(graph);
assertEquals("[A, B]", frozenGraph.getShortestPath("A", "B").toString());
assertEquals("[A, B]", shortestPath(frozenGraph, "A", "B").toString());
assertEquals("[[A, B]]", frozenGraph.getPaths("A", "B").toString());
assertEquals("[A, D, E]", frozenGraph.getShortestPath("A", "E").toString());
assertEquals("[[A, B, C, E], [A, D, E]]",
assertEquals("[A, D, E]", shortestPath(frozenGraph, "A", "E").toString());
assertEquals("[[A, D, E], [A, B, C, E]]",
frozenGraph.getPaths("A", "E").toString());
assertNull(frozenGraph.getShortestPath("B", "A"));
assertNull(frozenGraph.getShortestPath("D", "C"));
assertNull(shortestPath(frozenGraph, "B", "A"));

assertNull(shortestPath(frozenGraph, "D", "C"));
assertEquals("[[D, E]]", frozenGraph.getPaths("D", "E").toString());
assertEquals("[D, E]", frozenGraph.getShortestPath("D", "E").toString());
assertEquals("[D, E]", shortestPath(frozenGraph, "D", "E").toString());
}

@Test void testDistances() {
Expand Down Expand Up @@ -355,9 +363,9 @@ private Set<String> getB(DefaultDirectedGraph<String, DefaultEdge> graph,
g.addEdge("B", "D", 1);
assertEquals("[A, B, D]", shortestPath(g, "A", "D").toString());
assertNull(shortestPath(g, "A", "E"), "There is no path from A to E");
assertEquals("[]", shortestPath(g, "D", "D").toString());
assertEquals("[D]", shortestPath(g, "D", "D").toString());
assertNull(shortestPath(g, "X", "A"), "Node X is not in the graph");
assertEquals("[[A, B, C, D], [A, B, D]]", paths(g, "A", "D").toString());
assertEquals("[[A, B, D], [A, B, C, D]]", paths(g, "A", "D").toString());
assertThat(g.addVertex("B"), is(false));

assertThat(Iterables.size(g.getEdges("A", "B")), is(1));
Expand Down