Skip to content

fix: mixing directed and undirected edges in motif throws parse error - #754

Merged
SemyonSinchenko merged 3 commits into
graphframes:mainfrom
goungoun:main
Dec 30, 2025
Merged

fix: mixing directed and undirected edges in motif throws parse error#754
SemyonSinchenko merged 3 commits into
graphframes:mainfrom
goungoun:main

Conversation

@goungoun

@goungoun goungoun commented Nov 22, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This PR address the bug report #750
(u)-[e1]-(v);(v)-[e2]->(k) should not fail

Why are the changes needed?

It improves the flexibility to use the motif pattern

Test Case

image
  1. (u)-[e1]-(v)
> g.find("(u)-[e1]-(v)").where("u.id == 0").select("u.id", "v.id", "_pattern", "_direction").show()
+---+---+-------------+----------+
| id| id|     _pattern|_direction|
+---+---+-------------+----------+
|  0|  1|(u)<-[e1]-(v)|        in|
|  0|  2|(u)<-[e1]-(v)|        in|
|  0|  1|(u)-[e1]->(v)|       out|
+---+---+-------------+----------+
  1. (u)-[e1]-(v);(v)-[e2]->(k)
> g.find("(u)-[e1]-(v);(v)-[e2]->(k)").where("u.id == 0").select("u.id", "v.id", "_pattern","_direction", "k.id").show()
+---+---+-------------+----------+---+
| id| id|     _pattern|_direction| id|
+---+---+-------------+----------+---+
|  0|  1|(u)<-[e1]-(v)|        in|  2|
|  0|  1|(u)<-[e1]-(v)|        in|  0|
|  0|  2|(u)<-[e1]-(v)|        in|  0|
|  0|  2|(u)<-[e1]-(v)|        in|  3|
|  0|  1|(u)-[e1]->(v)|       out|  2|
|  0|  1|(u)-[e1]->(v)|       out|  0|
+---+---+-------------+----------+---+

@goungoun goungoun mentioned this pull request Nov 22, 2025
Closed
6 tasks
@goungoun goungoun changed the title [bug] Mixing directed and undirected edges in motif throws parse error [draft] Mixing directed and undirected edges in motif throws parse error Nov 22, 2025
@rjurney

rjurney commented Nov 22, 2025

Copy link
Copy Markdown
Collaborator

What about 2, 3 in the first example?

@rjurney

rjurney commented Nov 22, 2025

Copy link
Copy Markdown
Collaborator

0, 1, 2 repeats

@goungoun

goungoun commented Nov 23, 2025

Copy link
Copy Markdown
Contributor Author

@rjurney It was with the condition "u.id == 0", my mistake in the description. Thanks!

@goungoun

goungoun commented Nov 23, 2025

Copy link
Copy Markdown
Contributor Author

The last commit removes the existing undirected pattern based on a regular expression, as the new one in the findIncremental() method can replace it.

@SemyonSinchenko

SemyonSinchenko commented Nov 26, 2025

Copy link
Copy Markdown
Collaborator

It looks ready for review, isn't it?

@goungoun

goungoun commented Nov 26, 2025

Copy link
Copy Markdown
Contributor Author

It looks ready for review, isn't it?

Let me keep it draft this week.

@goungoun goungoun changed the title [draft] Mixing directed and undirected edges in motif throws parse error Mixing directed and undirected edges in motif throws parse error Nov 30, 2025
@goungoun goungoun changed the title Mixing directed and undirected edges in motif throws parse error [bug] Mixing directed and undirected edges in motif throws parse error Nov 30, 2025
@goungoun goungoun changed the title [bug] Mixing directed and undirected edges in motif throws parse error [fix] Mixing directed and undirected edges in motif throws parse error Nov 30, 2025
@goungoun goungoun changed the title [fix] Mixing directed and undirected edges in motif throws parse error fix: mixing directed and undirected edges in motif throws parse error Nov 30, 2025
@goungoun
goungoun marked this pull request as ready for review November 30, 2025 08:45
@goungoun

goungoun commented Dec 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

0, 1, 2 repeats
@rjurney I added two columns, "_pattern","_direction" in the example to explain the symptom.

@goungoun

goungoun commented Dec 3, 2025

Copy link
Copy Markdown
Contributor Author

@rjurney The result is as expected, is there anything you're still unsure about?

@SemyonSinchenko
SemyonSinchenko self-requested a review December 3, 2025 06:57
@SemyonSinchenko

SemyonSinchenko commented Dec 9, 2025

Copy link
Copy Markdown
Collaborator

@goungoun Hello! Sorry for the delay with a review. I want to finish my work on graph embeddings first and I'm almost done... I will try to review your PR this week. Thanks for understanding!

@goungoun

goungoun commented Dec 9, 2025

Copy link
Copy Markdown
Contributor Author

@goungoun Hello! Sorry for the delay with a review. I want to finish my work on graph embeddings first and I'm almost done... I will try to review your PR this week. Thanks for understanding!

@SemyonSinchenko Thanks for your comment. I know you are running many things, please no rush as I can wait.

SemyonSinchenko
SemyonSinchenko approved these changes Dec 30, 2025
View reviewed changes

@SemyonSinchenko SemyonSinchenko left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice work @goungoun !

@SemyonSinchenko
SemyonSinchenko merged commit 0179298 into graphframes:main Dec 30, 2025
5 checks passed
@goungoun

goungoun commented Dec 30, 2025

Copy link
Copy Markdown
Contributor Author

Thank you @SemyonSinchenko!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SemyonSinchenko SemyonSinchenko SemyonSinchenko approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@goungoun @rjurney @SemyonSinchenko