Variable length path #127

swilly22 · 2018-09-13T10:01:25Z

No description provided.

jeffreylovitz · 2018-09-13T15:55:27Z

src/parser/grammar.y

+	A = New_AST_LinkEntity(B.strval, C.strval, E, N_DIR_UNKNOWN, D);
+}
+
+// Edge with length [alias:*1..3]


Cypher doesn't support edges that have aliases but no labels in general (a change we implemented about a month ago, I think), which remains true with length specifiers:

MATCH (a)-[alias:*]->(b) RETURN b > Invalid input '*': expected whitespace or a rel type name

We should, however, support edges with labels and length specifications, like:
MATCH (a)-[:KNOWS*..2]->(b) RETURN b

The Cypher Query Language Reference (version 9, pages 11-12) doesn't explicitly mention edges with labels, aliases, and length specifications, but they are supported in Neo.
MATCH (a)-[alias:KNOWS*]->(b) RETURN b

You're correct, although I don't see a reason why we shouldn't support this
I think its quite powerful to ask (A)-[:*1..20]->(B)

@swilly22 As a minor clarification, (A)-[*1..20]->(B) (returning all matching paths without providing an alias to refer to those paths in other parts of the query) is totally canonical syntax, and we should support it!

Cypher does not explicitly support (A)-[alias:*1..20]->(B), and - conjecturing - I suspect that is because it's proven computationally challenging to merge the properties of all matching paths into a single edge alias that can be filtered, returned, and so forth. I can't think of any syntactical reason for this constraint other than that it taxes the system, especially given that Cypher provides support for multiple explicit edge labels with syntax like (a)-[:KNOWS|HAS_MET)->(b).

I have no objection to going beyond the language specification, but providing support for edge aliases that don't correspond to specific labels will introduce a fair few cases we'll need to add tooling for. As an example, if one wants to filter on a property found along a path, we'll need to find and track the properties of all paths matching the query, as opposed to the masking approach suggested in this PR, which at each hop only considers edges that can modify the paths being built. I'm specifically referring to the _VarLenTraverseOpHop logic that builds and employs a mask to ignore traversals that, on a boolean level, have been made redundant, but I expect that is not the only edge case we'd need to compensate for.

This is just food for thought - I'm on board with providing more functionality whenever someone might find it useful! Let me know what you think.

jeffreylovitz · 2018-09-13T15:55:29Z

src/parser/grammar.y

+}
+
+// Edge with length [alias:*1..3]
+edge(A) ::= LEFT_BRACKET UQSTRING(B) COLON edgeLength(C) properties(D) RIGHT_BRACKET . { 


edgeLength should not be preceded by a colon.

jeffreylovitz · 2018-09-13T15:55:31Z

src/parser/grammar.y

+}
+
+// Edge with length [:*1..3]
+edge(A) ::= LEFT_BRACKET COLON edgeLength(B) properties(C) RIGHT_BRACKET . { 


edgeLength should not be preceded by a colon.

jeffreylovitz · 2018-09-13T15:55:33Z

src/parser/grammar.y

+}
+
+// *minHops..maxHops
+edgeLength(A) ::= MUL INTEGER(B) DOT DOT INTEGER(C). {


The DOT DOT logic isn't working for me:

127.0.0.1:6379> GRAPH.QUERY social "MATCH (a)-[:*1..2]->(b) RETURN a, b" (error) Syntax error at offset 18 near '.2'

As a guess, it may be trying to parse a double instead of an int?

introducing DOTDOT

jeffreylovitz · 2018-09-13T15:55:35Z

src/parser/grammar.y

+edgeLength(A) ::= MUL INTEGER(B) DOT DOT INTEGER(C). {
+	A = New_AST_LinkLength(B.intval, C.intval);
+}
+


It should also be legal to omit a bound, as in:
(a)-[*..3]->(b)
or
(a)-[*3..]->(b)

jeffreylovitz · 2018-09-13T15:55:42Z

src/parser/ast_common.h

@@ -35,13 +35,20 @@ typedef struct {
 	char *property;
 } AST_Variable;

+typedef struct {
+	unsigned int minHops;
+	float maxHops;


I'm guessing we're using a float for maxHops to provide simpler support for * (infinite) length paths? That makes sense, and is probably fine, but I wouldn't mind defining UINT_MAX as an infinity value either.

The smallest integer value that can't be represented as a float is 2^24 + 1, which is not a path length we reasonably need to be concerned about, so it's not a big deal - your call!

jeffreylovitz · 2018-09-13T15:55:49Z

src/execution_plan/ops/op_var_len_traverse.c

+        NULL,                   // Accumulator
+        GxB_LOR_LAND_BOOL,      // Semiring
+        op->A,                  // First matrix
+        op->A,                  // Second matrix


I don't understand what this does! We use the same matrix for both inputs?

jeffreylovitz · 2018-09-13T15:59:51Z

src/execution_plan/ops/op_var_len_traverse.c

+    GrB_free(&op->mask);
+    GrB_Matrix_new(&op->mask, GrB_BOOL, Graph_NodeCount(op->g), Graph_NodeCount(op->g));
+    GrB_Matrix_dup(&op->mask, op->A);
+    TuplesIter_clear(op->it);


What does this accomplish? The iterator will return DEPLETED on its next invocation, but it does not actually get invoked before the TuplesIter_reuse call on 107.

jeffreylovitz · 2018-09-18T12:06:29Z

tests/unit/test_all_paths.cpp

+#endif
+
+#include "../../src/algorithms/algorithms.h"
+#include "../../src/util/arr.h"


I think we need to move the function definitions in arr.h into a separate .c file before these tests will compile correctly for me/Circle.

Already solved, simple casting.

jeffreylovitz · 2018-09-18T12:06:35Z

src/parser/grammar.y

+edge(A) ::= LEFT_BRACKET UQSTRING(B) COLON UQSTRING(C) edgeLength(D) properties(E) RIGHT_BRACKET . { 
+	A = New_AST_LinkEntity(B.strval, C.strval, E, N_DIR_UNKNOWN, D);
+}
+


These rules look good! We still need to add one for edges that just have length specifiers, and no alias or label.

jeffreylovitz · 2018-09-18T12:06:43Z

src/parser/ast_common.c

@@ -37,6 +38,13 @@ AST_LinkEntity* New_AST_LinkEntity(char *alias, char *label, Vector *properties,
 	return le;
 }

+AST_LinkLength* New_AST_LinkLength(unsigned int minHops, unsigned int maxHops) {


It would be good here or elsewhere to verify that minHops <= maxHops

jeffreylovitz · 2018-09-18T12:06:47Z

src/graph/graph.h

@@ -101,11 +101,19 @@ void Graph_GetEdgesConnectingNodes (
    Vector *edges
 );

-// Get both incoming and outgoing node edges.
+// Get node edges.


Comment belongs above Graph_GetNodeEdges

jeffreylovitz · 2018-09-18T12:06:51Z

src/graph/graph.c

@@ -617,9 +623,15 @@ GrB_Matrix Graph_GetLabel(const Graph *g, int label_idx) {
 }

 GrB_Matrix Graph_GetRelation(const Graph *g, int relation_idx) {


size_t relation_idx

jeffreylovitz · 2018-09-18T12:07:02Z

src/execution_plan/ops/op_cond_var_len_traverse.c

+    while(array_len(op->paths) == 0) {
+        res = child->consume(child, graph);
+        if(res != OP_OK) return res;
+        printf("op->maxHops: %d\n", op->maxHops);


This is of course just a debug line, but the print specifier should be %u

jeffreylovitz · 2018-09-18T12:07:11Z

src/algorithms/path.c

+    if(pathLen == 0) return NULL;
+
+    Edge *e = p[pathLen-1];
+    array_pop(p);


These two lines are equivalent to:
Edge *e = array_pop(p);
Or can even return array_pop(p)

jeffreylovitz · 2018-09-18T12:07:15Z

src/algorithms/path.c

+
+Path Path_append(Path p, Edge *e) {
+    p = array_append(p, e);
+    return p;


Can simply return array_append

jeffreylovitz · 2018-09-18T12:07:20Z

src/algorithms/path.c

+
+Path Path_new(size_t len) {
+    Path p = array_new(Edge*, len);
+    return p;


Can simply return array_new

jeffreylovitz · 2018-09-18T12:07:25Z

src/algorithms/all_paths.c

+            Path_pop(*path);
+
+            // Mark edge as unvisited.
+            GrB_Matrix_setElement_BOOL(visited, false, neighborID, frontierID);


This probably necessitates a zero vector or GxB_NONZERO reduction later on, right?

Actually, never mind - I suppose it does not so long as we never try to iterate over visited with a TuplesIter or similar.

visited, won't be iterated, it only serves as a marker for checking if an edge been traversed.

jeffreylovitz · 2018-09-20T13:54:58Z

src/graph/graph.c

@@ -392,43 +392,49 @@ void Graph_GetEdgesConnectingNodes(const Graph *g, NodeID src, NodeID dest, int

 /* Retrieves all either incoming or outgoing edges 
 * to/from given node N, depending on given direction. */
-void Graph_GetNodeEdges(const Graph *g, const Node *n, Vector *edges) {
+void Graph_GetNodeEdges(const Graph *g, const Node *n, Vector *edges, GRAPH_EDGE_DIR dir, int edgeType) {
    assert(g && n && edges);
    NodeID srcNodeID;
    NodeID destNodeID;
    TuplesIter *tupleIter;


TuplesIter *tupleIter = NULL will suppress a compiler warning for me.

jeffreylovitz · 2018-09-20T13:55:15Z

src/algorithms/path.c

+    return array_len(p);
+}
+
+Path Path_clone(const Path p) {


It seems to me like we could just perform a memcpy on the array object to accomplish this in one instruction (arr.h provides an array_sizeof macro already). That's not particularly important, though, we can just keep it in mind for the future.

I thought about that, although this involves tinkering with arr.h internals, if we'll find out that this is a bottleneck then we could revisit this.

jeffreylovitz · 2018-09-20T13:55:19Z

src/algorithms/path.c

+
+void Path_print(Path p) {
+    assert(p);
+    Edge *e;


Edge *e = NULL will suppress a compiler warning for me.

jeffreylovitz

This looks great! There are two spots where I get "x may be used uninitialized in this function" compiler warnings from GCC - if you could NULL-initialize those, that would resolve the problem.

After that, 👍

* variable length must be preceded by a label * implement all paths * isolate variable length algebraic expression

WIP

180f979

swilly22 added the enhancement label Sep 13, 2018

swilly22 added this to the GA milestone Sep 13, 2018

swilly22 self-assigned this Sep 13, 2018

swilly22 requested a review from jeffreylovitz September 13, 2018 10:01

jeffreylovitz reviewed Sep 13, 2018

View reviewed changes

swilly22 added 2 commits September 14, 2018 09:15

variable length must be preceded by a label

5fac39c

implement all paths

7ffeaf8

jeffreylovitz reviewed Sep 18, 2018

View reviewed changes

swilly22 changed the title ~~WIP~~ Variable length path Sep 20, 2018

jeffreylovitz reviewed Sep 20, 2018

View reviewed changes

jeffreylovitz previously approved these changes Sep 20, 2018

View reviewed changes

isolate variable length algebraic expression

8cbc464

swilly22 dismissed jeffreylovitz’s stale review via 8cbc464 September 20, 2018 14:24

swilly22 force-pushed the var_length_expand branch from 009af9e to 8cbc464 Compare September 20, 2018 14:24

swilly22 merged commit 0654705 into master Sep 20, 2018

swilly22 deleted the var_length_expand branch September 20, 2018 14:31

pnxguide pushed a commit to CMU-SPEED/RedisGraph that referenced this pull request Mar 22, 2023

Variable length path (RedisGraph#127)

a1ccfc5

* variable length must be preceded by a label * implement all paths * isolate variable length algebraic expression

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Variable length path #127

Variable length path #127

swilly22 commented Sep 13, 2018

jeffreylovitz Sep 13, 2018

swilly22 Sep 14, 2018

jeffreylovitz Sep 14, 2018

jeffreylovitz Sep 13, 2018

jeffreylovitz Sep 13, 2018

jeffreylovitz Sep 13, 2018

swilly22 Sep 14, 2018

jeffreylovitz Sep 13, 2018

jeffreylovitz Sep 13, 2018

jeffreylovitz Sep 13, 2018

jeffreylovitz Sep 13, 2018

jeffreylovitz Sep 18, 2018

swilly22 Sep 18, 2018

jeffreylovitz Sep 18, 2018

jeffreylovitz Sep 18, 2018

jeffreylovitz Sep 18, 2018

jeffreylovitz Sep 18, 2018

jeffreylovitz Sep 18, 2018

jeffreylovitz Sep 18, 2018

jeffreylovitz Sep 18, 2018

jeffreylovitz Sep 18, 2018

jeffreylovitz Sep 18, 2018

jeffreylovitz Sep 18, 2018

swilly22 Sep 18, 2018

jeffreylovitz Sep 20, 2018

jeffreylovitz Sep 20, 2018

swilly22 Sep 20, 2018

jeffreylovitz Sep 20, 2018

jeffreylovitz left a comment

		@@ -617,9 +623,15 @@ GrB_Matrix Graph_GetLabel(const Graph *g, int label_idx) {
		}

		GrB_Matrix Graph_GetRelation(const Graph *g, int relation_idx) {

Variable length path #127

Variable length path #127

Conversation

swilly22 commented Sep 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffreylovitz left a comment

Choose a reason for hiding this comment