You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/06/16 06:10:43 UTC
[GitHub] [calcite] liyafan82 opened a new pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
liyafan82 opened a new pull request #2027:
URL: https://github.com/apache/calcite/pull/2027
According to the discussion in the JIRA, we revert the changes to ConsList class, and replace shortest distances with the shortest paths.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r446867196
##########
File path: core/src/test/java/org/apache/calcite/util/graph/DirectedGraphTest.java
##########
@@ -64,14 +64,21 @@
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, C, D]", shortestPath(g, "D", "D").toString());
Review comment:
Ok.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r445405947
##########
File path: core/src/test/java/org/apache/calcite/util/graph/DirectedGraphTest.java
##########
@@ -64,14 +64,21 @@
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, C, D]", shortestPath(g, "D", "D").toString());
Review comment:
This seems a regression, I think we should put somewhere (`getPaths`? `findPaths`?) that `if (from.equals(to))` the empty list is a valid path (as it happened previously in the old `getShortestPath` method).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r446590442
##########
File path: core/src/test/java/org/apache/calcite/util/graph/DirectedGraphTest.java
##########
@@ -64,14 +64,21 @@
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, C, D]", shortestPath(g, "D", "D").toString());
Review comment:
Sounds reasonable. I have revised the code to include the empty path to itself.
BTW, the empty path from node D to itself should be denoted "[D]"? as it helps to maintain the relation:
pathLength == path.size() - 1
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] xndai commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
xndai commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r441156674
##########
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:
Can you also fix the getPaths() to return shortest paths first? This is to make sure we choose the shortest path during convert.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r446590701
##########
File path: core/src/main/java/org/apache/calcite/util/graph/Graphs.java
##########
@@ -102,41 +99,28 @@ public int size() {
*/
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.
Review comment:
Thanks a lot for your careful review.
It seems there is a minor difference between increasing and non-decreasing order. A sequence is said to be in increasing order, if
```
a0 < a1 < ... < an
```
It is said to be in non-decreasing order, if
```
a0 <= a1 <= ... <= an
```
For paths between two nodes in a graph, it is possible that there are multiple paths with the same length. So the sequence of path lengths should be in non-decreasing order. What do you 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:
users@infra.apache.org
[GitHub] [calcite] xndai commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
xndai commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r441681036
##########
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:
@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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r445406357
##########
File path: core/src/main/java/org/apache/calcite/util/graph/Graphs.java
##########
@@ -102,41 +99,28 @@ public int size() {
*/
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.
Review comment:
minor: I'd use "increasing order" instead of "non-decreasing order"
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#issuecomment-651583172
LGTM
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r441518623
##########
File path: core/src/main/java/org/apache/calcite/util/graph/Graphs.java
##########
@@ -56,42 +54,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 changeCount = false;
Review comment:
Revised. Thanks for your kind reminder.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r440911347
##########
File path: core/src/main/java/org/apache/calcite/util/graph/Graphs.java
##########
@@ -56,42 +54,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 changeCount = false;
Review comment:
minor thing: now that this variable is a `boolean`, maybe it should be renamed (`changeCount` -> `change` ?)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r441518399
##########
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:
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada merged pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
rubenada merged pull request #2027:
URL: https://github.com/apache/calcite/pull/2027
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] xndai commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
xndai commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r442023404
##########
File path: core/src/test/java/org/apache/calcite/util/graph/DirectedGraphTest.java
##########
@@ -66,7 +66,7 @@
assertNull(shortestPath(g, "A", "E"), "There is no path from A to E");
assertEquals("[]", 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());
Review comment:
where are the tests for getShortestDistance()?
##########
File path: core/src/main/java/org/apache/calcite/util/graph/Graphs.java
##########
@@ -134,7 +135,44 @@ public int size() {
if (from.equals(to)) {
Review comment:
I think we can just remove this method. Nobody is using it anymore.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#issuecomment-651495510
@rubenada @xndai Thanks a lot for your good comments. I have squashed the commits into one.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r442101018
##########
File path: core/src/test/java/org/apache/calcite/util/graph/DirectedGraphTest.java
##########
@@ -66,7 +66,7 @@
assertNull(shortestPath(g, "A", "E"), "There is no path from A to E");
assertEquals("[]", 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());
Review comment:
We have DirectedGraphTest#testDistance
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] xndai commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
xndai commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r445712632
##########
File path: core/src/main/java/org/apache/calcite/util/graph/Graphs.java
##########
@@ -102,41 +99,28 @@ public int size() {
*/
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.
Review comment:
+1
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r446864534
##########
File path: core/src/main/java/org/apache/calcite/util/graph/Graphs.java
##########
@@ -102,41 +99,28 @@ public int size() {
*/
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.
Review comment:
@liyafan82 you're right, thanks for the clarification.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] xndai commented on pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
xndai commented on pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#issuecomment-651237132
Looks good to me. Thanks @rubenada and @liyafan82
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r442019191
##########
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:
@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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#issuecomment-651023112
@xndai do you have any other comments regarding this PR?
I think it is in a pretty good shape, we could consider to merge it (after squashing commits into a single on).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] liyafan82 commented on a change in pull request #2027: [CALCITE-4049] Improve the implementation of the shortest-path algorithm
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #2027:
URL: https://github.com/apache/calcite/pull/2027#discussion_r442100756
##########
File path: core/src/main/java/org/apache/calcite/util/graph/Graphs.java
##########
@@ -134,7 +135,44 @@ public int size() {
if (from.equals(to)) {
Review comment:
Removed. Maybe we can add it back some time in the future, if we find a requirement for it.
----------------------------------------------------------------
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:
users@infra.apache.org