test(dsl): add modern-graph regression tests for built-in graph algorithms - #829
test(dsl): add modern-graph regression tests for built-in graph algorithms#829CalebWang0126 wants to merge 1 commit into
Conversation
…rality, lpa, common_neighbors and jaccard_similarity Add five CALL ... YIELD .sql/.txt pairs on the standard modern graph for algorithms with thin coverage in GQLAlgorithmTest, with all expected outputs verified by hand (fixes apache#794).
| @@ -0,0 +1 @@ | |||
| 1,1.0 | |||
There was a problem hiding this comment.
This expectation appears to lock in incorrect behavior for a disconnected directed graph. From vertex 1, only vertices 2, 3, 4, and 5 are reachable; vertex 6 is unreachable, and the sum of the finite distances is 1 + 1 + 1 + 2 = 5.
The current implementation counts all five other vertices in the numerator while omitting the unreachable distance from the denominator, which produces 1.0 and allows an unreachable vertex to inflate the centrality score.
Could we first define the intended semantics for disconnected graphs and then update both the implementation and this expectation? For example, Wasserman-Faust normalization would give (4 / 5) * (4 / 5) = 0.64 here, while the classical definition with an infinite distance would give 0.
| USE GRAPH modern; | ||
|
|
||
| INSERT INTO result_tb | ||
| CALL jaccard_similarity(1, 4) YIELD (vertex_a, vertex_b, jaccard_coefficient) |
There was a problem hiding this comment.
This case currently passes for the wrong reason and does not exercise the known non-adjacent-vertex path.
Because vertices 1 and 4 are directly adjacent, vertex 1 receives vertex 4's type-0 inquiry in iteration 2 and counts it as a common-neighbor confirmation. The actual common neighbor, vertex 3, sends its confirmation during iteration 2, so that message only arrives in iteration 3; however, JaccardSimilarity has no iteration-3 handler.
The incorrect intersection count is also 1, so the implementation happens to return the mathematically correct value 0.2. The existing (1, 3) test has the same adjacent-vertices/one-common-neighbor shape.
Please use a discriminating case such as (4, 6), whose expected result is 1 / 3, and fix the iteration-3 aggregation. If fixing the algorithm is outside this PR's scope, I suggest removing this case and tracking the bug in a separate linked issue.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds end-to-end regression coverage for several built-in graph algorithms on the standard modern graph, expanding GQLAlgorithmTest to exercise additional CALL ... YIELD paths and validate their outputs.
Changes:
- Add 5 new GQL query fixtures targeting
modern_graph.sql(khop,closeness_centrality,lpa, plus new parameter cases forcommon_neighborsandjaccard_similarity). - Add corresponding expected sink outputs for each new query.
- Register 5 new JUnit tests in
GQLAlgorithmTest.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| geaflow/geaflow-dsl/geaflow-dsl-runtime/src/test/resources/query/gql_algorithm_lpa_modern.sql | New lpa() query fixture on modern graph writing to file sink |
| geaflow/geaflow-dsl/geaflow-dsl-runtime/src/test/resources/query/gql_algorithm_khop_modern.sql | New khop(1, 2) query fixture on modern graph |
| geaflow/geaflow-dsl/geaflow-dsl-runtime/src/test/resources/query/gql_algorithm_jaccard_similarity_002.sql | Adds a second jaccard_similarity parameter case on modern graph |
| geaflow/geaflow-dsl/geaflow-dsl-runtime/src/test/resources/query/gql_algorithm_common_neighbors_002.sql | Adds a second common_neighbors parameter case on modern graph |
| geaflow/geaflow-dsl/geaflow-dsl-runtime/src/test/resources/query/gql_algorithm_closeness_centrality_modern.sql | New closeness_centrality(1) query fixture on modern graph |
| geaflow/geaflow-dsl/geaflow-dsl-runtime/src/test/resources/expect/gql_algorithm_lpa_modern.txt | Expected output for lpa() on modern graph |
| geaflow/geaflow-dsl/geaflow-dsl-runtime/src/test/resources/expect/gql_algorithm_khop_modern.txt | Expected output for khop(1,2) on modern graph |
| geaflow/geaflow-dsl/geaflow-dsl-runtime/src/test/resources/expect/gql_algorithm_jaccard_similarity_002.txt | Expected output for jaccard_similarity(1,4) |
| geaflow/geaflow-dsl/geaflow-dsl-runtime/src/test/resources/expect/gql_algorithm_common_neighbors_002.txt | Expected output for common_neighbors(4,6) |
| geaflow/geaflow-dsl/geaflow-dsl-runtime/src/test/resources/expect/gql_algorithm_closeness_centrality_modern.txt | Expected output for closeness_centrality(1) |
| geaflow/geaflow-dsl/geaflow-dsl-runtime/src/test/java/org/apache/geaflow/dsl/runtime/query/GQLAlgorithmTest.java | Registers 5 new tests wired to modern_graph.sql and new query fixtures |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| INSERT INTO result_tb | ||
| CALL common_neighbors(4, 6) YIELD (id) | ||
| RETURN cast (id as int) |
|
|
||
| INSERT INTO result_tb | ||
| CALL jaccard_similarity(1, 4) YIELD (vertex_a, vertex_b, jaccard_coefficient) | ||
| RETURN cast(vertex_a as int), cast(vertex_b as int), jaccard_coefficient |
|
|
||
| INSERT INTO result_tb | ||
| CALL khop(1, 2) YIELD (vid, kValue) | ||
| RETURN cast (vid as int), kValue |
| @Test | ||
| public void testAlgorithmCommonNeighbors002() throws Exception { |
| @Test | ||
| public void testAlgorithmJaccardSimilarity002() throws Exception { |
Fixes #794
What & why
Several built-in algorithms were never exercised on the standard
moderngraph byGQLAlgorithmTest:khop,closeness_centralityandlpawere only tested on ad-hoc graphs (g4/g5), andcommon_neighbors/jaccard_similarityhad only a single parameter case. This PR adds five end-to-endCALL ... YIELDpairs onmodern_graph.sql.Expected outputs — verified by hand
The modern graph has vertices 1..6 and directed edges 1->2, 1->3, 1->4, 4->3, 4->5, 6->3 (algorithms using
EdgeDirection.BOTHtreat it as undirected).(1,0),(2,1),(3,1),(4,1),(5,2): BFS distances from vertex 1 within 2 hops; vertex 6 is unreachable. Consistent with the existing SSSP expectations on the same graph.1.0: sum of shortest distances from 1 to reachable vertices = 1+1+1+2 = 5, and n-1 = 5, so 5/5 = 1.0.1: simulated round by round; on ties the implementation keeps the lexicographically smallest label, which makes the outcome deterministic on this graph.3: N(4) = {1,3,5}, N(6) = {3}, intersection = {3}.0.2: N(1) = {2,3,4}, N(4) = {1,3,5}, intersection = {3}, union size = 5, so 1/5 = 0.2.Test
mvn test -Dtest=GQLAlgorithmTestpasses (39 tests, including the 5 new ones). Checkstyle passes.Notes
While adding the jaccard case I noticed that for non-adjacent vertex pairs the algorithm always returns 0.0: common-neighbor confirmations are sent in iteration 2 but only reach vertex A in iteration 3, which the
process()method does not handle. E.g.jaccard_similarity(4, 6)returns 0.0 although the mathematical value is 1/3. The existing (1,3) case passes only because 1 and 3 happen to be adjacent. This seems worth a separate issue; happy to file one if the maintainers agree.Louvain / ASSP were intentionally not added: their tie-breaking depends on HashMap iteration order, which makes hand-verified expectations impractical.