test(dsl): add modern-graph regression tests for built-in graph algorithms - #829
Open
CalebWang0126 wants to merge 1 commit into
Open
test(dsl): add modern-graph regression tests for built-in graph algorithms#829CalebWang0126 wants to merge 1 commit into
CalebWang0126 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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.