Merge pull request #27 from microsoft/fixups

Fixups

Author: niclasko

flowquery-py/src/graph/node_reference.py

@@ -2,27 +2,28 @@
 
 from typing import Any, Optional
 
+from ..parsing.ast_node import ASTNode
 from .node import Node
 
 
 class NodeReference(Node):
     """Represents a reference to an existing node variable."""
 
-    def __init__(self, base: Node, reference: Node) -> None:
+    def __init__(self, base: Node, reference: ASTNode) -> None:
         super().__init__(base.identifier, base.label)
-        self._reference: Node = reference
+        self._reference: ASTNode = reference
         # Copy properties from base
         self._properties = base._properties
         self._outgoing = base.outgoing
         self._incoming = base.incoming
 
     @property
-    def reference(self) -> Node:
+    def reference(self) -> ASTNode:
         return self._reference
 
     # Keep referred as alias for backward compatibility
     @property
-    def referred(self) -> Node:
+    def referred(self) -> ASTNode:
         return self._reference
 
     def value(self) -> Optional[Any]:

flowquery-py/src/graph/relationship.py

@@ -123,9 +123,9 @@ def set_data(self, data: Optional['RelationshipData']) -> None:
     def get_data(self) -> Optional['RelationshipData']:
         return self._data
 
-    def set_value(self, relationship: 'Relationship') -> None:
+    def set_value(self, relationship: 'Relationship', traversal_id: str = "") -> None:
         """Set value by pushing match to collector."""
-        self._matches.push(relationship)
+        self._matches.push(relationship, traversal_id)
         self._value = self._matches.value()
 
     def value(self) -> Optional[Union[RelationshipMatchRecord, List[RelationshipMatchRecord]]]:
@@ -139,11 +139,13 @@ def set_end_node(self, node: 'Node') -> None:
         """Set the end node for the current match."""
         self._matches.end_node = node
 
+    def _left_id_or_right_id(self) -> str:
+        return "left_id" if self._direction == "left" else "right_id"
+
     async def find(self, left_id: str, hop: int = 0) -> None:
         """Find relationships starting from the given node ID."""
         # Save original source node
         original = self._source
-        is_left = self._direction == "left"
         if hop > 0:
             # For hops greater than 0, the source becomes the target of the previous hop
             self._source = self._target
@@ -158,30 +160,26 @@ async def find(self, left_id: str, hop: int = 0) -> None:
                 # No relationship match is pushed since no edge is traversed
                 await self._target.find(left_id, hop)
 
-        def find_match(id_: str, h: int) -> bool:
-            if self._data is None:
-                return False
-            if is_left:
-                return self._data.find_reverse(id_, h)
-            return self._data.find(id_, h)
-        follow_id = 'left_id' if is_left else 'right_id'
-        while self._data and find_match(left_id, hop):
+        while self._data and self._data.find(left_id, hop, self._direction):
             data = self._data.current(hop)
-            if data and self._hops and hop + 1 >= self._hops.min:
-                self.set_value(self)
+            if data is None:
+                continue
+            id = data[self._left_id_or_right_id()]
+            if hop + 1 >= self._hops.min:
+                self.set_value(self, left_id)
                 if not self._matches_properties(hop):
                     continue
-                if self._target and follow_id in data:
-                    await self._target.find(data[follow_id], hop)
-                if self._matches.is_circular():
-                    raise ValueError("Circular relationship detected")
-                if self._hops and hop + 1 < self._hops.max:
-                    await self.find(data[follow_id], hop + 1)
+                if self._target:
+                    await self._target.find(id, hop)
+                if hop + 1 < self._hops.max:
+                    if self._matches.is_circular(id):
+                        self._matches.pop()
+                        continue
+                    await self.find(id, hop + 1)
                 self._matches.pop()
-            elif data and self._hops:
+            else:
                 # Below minimum hops: traverse the edge without yielding a match
-                if follow_id in data:
-                    await self.find(data[follow_id], hop + 1)
+                await self.find(id, hop + 1)
 
         # Restore original source node
         self._source = original

flowquery-py/src/graph/relationship_data.py

@@ -19,13 +19,10 @@ def __init__(self, records: Optional[List[Dict[str, Any]]] = None):
         self._build_index("left_id")
         self._build_index("right_id")
 
-    def find(self, left_id: str, hop: int = 0) -> bool:
-        """Find a relationship by start node ID."""
-        return self._find(left_id, hop, "left_id")
-
-    def find_reverse(self, right_id: str, hop: int = 0) -> bool:
-        """Find a relationship by end node ID (reverse direction)."""
-        return self._find(right_id, hop, "right_id")
+    def find(self, id: str, hop: int = 0, direction: str = "right") -> bool:
+        """Find a relationship by node ID and direction."""
+        key = "right_id" if direction == "left" else "left_id"
+        return self._find(id, hop, key)
 
     def properties(self) -> Optional[Dict[str, Any]]:
         """Get properties of current relationship, excluding left_id and right_id."""

flowquery-py/src/graph/relationship_match_collector.py

@@ -24,7 +24,7 @@ def __init__(self) -> None:
         self._matches: List[RelationshipMatchRecord] = []
         self._node_ids: List[str] = []
 
-    def push(self, relationship: 'Relationship') -> RelationshipMatchRecord:
+    def push(self, relationship: 'Relationship', traversal_id: str = "") -> RelationshipMatchRecord:
         """Push a new match onto the collector."""
         start_node_value = relationship.source.value() if relationship.source else None
         rel_data = relationship.get_data()
@@ -36,8 +36,7 @@ def push(self, relationship: 'Relationship') -> RelationshipMatchRecord:
             "properties": rel_props,
         }
         self._matches.append(match)
-        if isinstance(start_node_value, dict):
-            self._node_ids.append(start_node_value.get("id", ""))
+        self._node_ids.append(traversal_id)
         return match
 
     @property
@@ -76,7 +75,6 @@ def matches(self) -> List[RelationshipMatchRecord]:
         """Get all matches."""
         return self._matches
 
-    def is_circular(self) -> bool:
-        """Check if the collected relationships form a circular pattern."""
-        seen = set(self._node_ids)
-        return len(seen) < len(self._node_ids)
+    def is_circular(self, next_id: str = "") -> bool:
+        """Check if traversing to the given node id would form a cycle."""
+        return next_id in self._node_ids

flowquery-py/src/parsing/operations/group_by.py

@@ -1,5 +1,6 @@
 """GroupBy implementation for aggregate operations."""
 
+import json
 from typing import Any, Dict, Generator, List, Optional
 
 from ..ast_node import ASTNode
@@ -8,6 +9,15 @@
 from .projection import Projection
 
 
+def _make_hashable(value: Any) -> Any:
+    """Convert a value to a hashable form for use as a dict key."""
+    if isinstance(value, dict):
+        return json.dumps(value, sort_keys=True, default=str)
+    if isinstance(value, list):
+        return json.dumps(value, sort_keys=True, default=str)
+    return value
+
+
 class GroupByNode:
     """Represents a node in the group-by tree."""
 
@@ -60,10 +70,11 @@ def _map(self) -> None:
         node = self._current
         for mapper in self.mappers:
             value = mapper.value()
-            child = node.children.get(value)
+            key = _make_hashable(value)
+            child = node.children.get(key)
             if child is None:
                 child = GroupByNode(value)
-                node.children[value] = child
+                node.children[key] = child
             node = child
         self._current = node
 

flowquery-py/src/parsing/parser.py

@@ -499,7 +499,7 @@ def _parse_node(self) -> Optional[Node]:
                 inner = ref_child.referred
                 if isinstance(inner, Node):
                     reference = inner
-            if reference is None or not isinstance(reference, Node):
+            if reference is None or (not isinstance(reference, Node) and not isinstance(reference, Unwind)):
                 raise ValueError(f"Undefined node reference: {identifier}")
             node = NodeReference(node, reference)
         elif identifier is not None:

flowquery-py/tests/compute/test_runner.py

@@ -1279,8 +1279,8 @@ async def test_circular_graph_pattern(self):
         assert len(results) == 2
 
     @pytest.mark.asyncio
-    async def test_circular_graph_pattern_with_variable_length_should_throw_error(self):
-        """Test circular graph pattern with variable length should throw error."""
+    async def test_circular_graph_pattern_with_variable_length_should_not_revisit_nodes(self):
+        """Test circular graph pattern with variable length should not revisit nodes."""
         await Runner(
             """
             CREATE VIRTUAL (:CircularVarPerson) AS {
@@ -1309,8 +1309,10 @@ async def test_circular_graph_pattern_with_variable_length_should_throw_error(se
             RETURN p AS pattern
             """
         )
-        with pytest.raises(ValueError, match="Circular relationship detected"):
-            await match.run()
+        await match.run()
+        results = match.results
+        # Circular graph 1↔2: cycles are skipped, only acyclic paths are returned
+        assert len(results) == 6
 
     @pytest.mark.asyncio
     async def test_multi_hop_match_with_min_hops_constraint_1(self):
@@ -2212,4 +2214,43 @@ async def test_where_with_contains_combined_with_and(self):
         await runner.run()
         results = runner.results
         assert len(results) == 1
-        assert results[0]["fruit"] == "pineapple"
+        assert results[0]["fruit"] == "pineapple"
+
+    @pytest.mark.asyncio
+    async def test_collected_nodes_and_re_matching(self):
+        """Test that collected nodes can be unwound and used as node references in subsequent MATCH."""
+        await Runner("""
+            CREATE VIRTUAL (:Person) AS {
+                unwind [
+                    {id: 1, name: 'Person 1'},
+                    {id: 2, name: 'Person 2'},
+                    {id: 3, name: 'Person 3'},
+                    {id: 4, name: 'Person 4'}
+                ] as record
+                RETURN record.id as id, record.name as name
+            }
+        """).run()
+        await Runner("""
+            CREATE VIRTUAL (:Person)-[:KNOWS]-(:Person) AS {
+                unwind [
+                    {left_id: 1, right_id: 2},
+                    {left_id: 2, right_id: 3},
+                    {left_id: 3, right_id: 4}
+                ] as record
+                RETURN record.left_id as left_id, record.right_id as right_id
+            }
+        """).run()
+        runner = Runner("""
+            MATCH (a:Person)-[:KNOWS*0..3]->(b:Person)
+            WITH collect(a) AS persons, b
+            UNWIND persons AS p
+            match (p)-[:KNOWS]->(:Person)
+            return p.name AS name
+        """)
+        await runner.run()
+        results = runner.results
+        assert len(results) == 9
+        names = [r["name"] for r in results]
+        assert "Person 1" in names
+        assert "Person 2" in names
+        assert "Person 3" in names

src/graph/node_reference.ts

@@ -1,8 +1,9 @@
+import ASTNode from "../parsing/ast_node";
 import Node from "./node";
 
 class NodeReference extends Node {
-    private _reference: Node | null = null;
-    constructor(base: Node, reference: Node) {
+    private _reference: ASTNode | null = null;
+    constructor(base: Node, reference: ASTNode) {
         super();
         this._identifier = base.identifier;
         this._label = base.label;
@@ -11,7 +12,7 @@ class NodeReference extends Node {
         this._incoming = base.incoming;
         this._reference = reference;
     }
-    public get reference(): Node | null {
+    public get reference(): ASTNode | null {
         return this._reference;
     }
     public async next(): Promise<void> {

src/graph/relationship.ts

@@ -71,8 +71,8 @@ class Relationship extends ASTNode {
     public get hops(): Hops | null {
         return this._hops;
     }
-    public setValue(relationship: Relationship): void {
-        const match: RelationshipMatchRecord = this._matches.push(relationship);
+    public setValue(relationship: Relationship, traversalId: string = ""): void {
+        const match: RelationshipMatchRecord = this._matches.push(relationship, traversalId);
         this._value = this._matches.value();
     }
     public set source(node: Node | null) {
@@ -108,10 +108,12 @@ class Relationship extends ASTNode {
     public setEndNode(node: Node): void {
         this._matches.endNode = node;
     }
+    public _left_id_or_right_id(): string {
+        return this._direction === "left" ? "left_id" : "right_id";
+    }
     public async find(left_id: string, hop: number = 0): Promise<void> {
         // Save original source node
         const original = this._source;
-        const isLeft = this._direction === "left";
         if (hop > 0) {
             // For hops greater than 0, the source becomes the target of the previous hop
             this._source = this._target;
@@ -127,28 +129,26 @@ class Relationship extends ASTNode {
                 await this._target.find(left_id, hop);
             }
         }
-        const findMatch = isLeft
-            ? (id: string, h: number) => this._data!.findReverse(id, h)
-            : (id: string, h: number) => this._data!.find(id, h);
-        const followId = isLeft ? "left_id" : "right_id";
-        while (findMatch(left_id, hop)) {
+        while (this._data!.find(left_id, hop, this._direction)) {
             const data: RelationshipRecord = this._data?.current(hop) as RelationshipRecord;
+            const id = data[this._left_id_or_right_id()];
             if (hop + 1 >= this.hops!.min) {
-                this.setValue(this);
+                this.setValue(this, left_id);
                 if (!this._matchesProperties(hop)) {
                     continue;
                 }
-                await this._target?.find(data[followId], hop);
-                if (this._matches.isCircular()) {
-                    throw new Error("Circular relationship detected");
-                }
+                await this._target?.find(id, hop);
                 if (hop + 1 < this.hops!.max) {
-                    await this.find(data[followId], hop + 1);
+                    if (this._matches.isCircular(id)) {
+                        this._matches.pop();
+                        continue;
+                    }
+                    await this.find(id, hop + 1);
                 }
                 this._matches.pop();
             } else {
                 // Below minimum hops: traverse the edge without yielding a match
-                await this.find(data[followId], hop + 1);
+                await this.find(id, hop + 1);
             }
         }
         // Restore original source node

src/graph/relationship_data.ts

@@ -8,11 +8,8 @@ class RelationshipData extends Data {
         super._buildIndex("left_id");
         super._buildIndex("right_id");
     }
-    public find(left_id: string, hop: number = 0): boolean {
-        return super._find(left_id, hop, "left_id");
-    }
-    public findReverse(right_id: string, hop: number = 0): boolean {
-        return super._find(right_id, hop, "right_id");
+    public find(id: string, hop: number = 0, direction: "left" | "right" = "right"): boolean {
+        return super._find(id, hop, direction === "left" ? "right_id" : "left_id");
     }
     /*
     ** Get the properties of the current relationship record