Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Neo4j multihop support #6104

Merged
merged 23 commits into from
Nov 15, 2022
Merged

Neo4j multihop support #6104

merged 23 commits into from
Nov 15, 2022

Conversation

djordje-mijatovic
Copy link
Contributor

@djordje-mijatovic djordje-mijatovic commented Oct 3, 2022 •
edited
Loading

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

resolves #5994

@github-actions github-actions bot added the product PR or Issue related to the DataHub UI/UX label Oct 3, 2022
@github-actions
Copy link

github-actions bot commented Oct 3, 2022 •
edited
Loading

Unit Test Results (build & test)

613 tests   609 ✔️  11m 53s ⏱️
151 suites      4 💤
151 files        0 ❌

Results for commit be2c812.

♻️ This comment has been updated with latest results.

@djordje-mijatovic djordje-mijatovic changed the title Neo4j multihopp support Neo4j multihopp support - resolves #5994 Oct 5, 2022
@djordje-mijatovic djordje-mijatovic changed the title Neo4j multihopp support - resolves #5994 Neo4j multihop support - resolves #5994 Oct 5, 2022
@djordje-mijatovic djordje-mijatovic changed the title Neo4j multihop support - resolves #5994 Neo4j multihop support Oct 5, 2022
@gabe-lyons gabe-lyons self-requested a review October 5, 2022 17:03
@gabe-lyons
Copy link
Contributor

@djordje-mijatovic running tests now!

@maggiehays maggiehays added the community-contribution PR or Issue raised by member(s) of DataHub Community label Oct 13, 2022
@maggiehays maggiehays added the hacktoberfest-accepted Acceptance for hacktoberfest https://hacktoberfest.com/participation/ label Oct 21, 2022
jjoyce0510
jjoyce0510 reviewed Nov 15, 2022
View reviewed changes
metadata-io/src/main/java/com/linkedin/metadata/graph/neo4j/Neo4jGraphService.java
}
});

EntityLineageResult result = new EntityLineageResult().setStart(offset)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor formatting:

new EntityLineageResult()
    .setStart(offset)

jjoyce0510
jjoyce0510 reviewed Nov 15, 2022
View reviewed changes
metadata-io/src/main/java/com/linkedin/metadata/graph/neo4j/Neo4jGraphService.java
final String statementIndirect = String.format(multiHopTemplateIndirect, entityUrn, dowStreamRel, maxHops, allowedEntityTypes);

String statement = null;
if (upstreamRel.length() > 0 && dowStreamRel.length() > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

naming nit: dowStreamRel -> downStreamRel

jjoyce0510
jjoyce0510 reviewed Nov 15, 2022
View reviewed changes
metadata-io/src/test/java/com/linkedin/metadata/graph/GraphServiceTestBase.java

downstreamLineage = service.getLineage(datasetThreeUrn, LineageDirection.DOWNSTREAM, 0, 1000, 2);
assertEquals(downstreamLineage.getTotal().intValue(), 0);
assertEquals(downstreamLineage.getRelationships().size(), 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice test.

Any cases for when there are no downstream or upstream edges for the source urn?

@jjoyce0510
Copy link
Collaborator

Most important piece is around removing tests. Ready to submit as soon as I hear back

jjoyce0510
jjoyce0510 approved these changes Nov 15, 2022
View reviewed changes
Copy link
Collaborator

@jjoyce0510 jjoyce0510 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you

@jjoyce0510 jjoyce0510 merged commit ca67e79 into datahub-project:master Nov 15, 2022
cccs-Dustin pushed a commit to CybercentreCanada/datahub that referenced this pull request Feb 1, 2023
@djordje-mijatovic @cccs-Dustin
feat(neo4j): Neo4j multihop support (datahub-project#6104)
2142ea5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabe-lyons gabe-lyons gabe-lyons left review comments

@jjoyce0510 jjoyce0510 jjoyce0510 approved these changes

@chriscollins3456 chriscollins3456 Awaiting requested review from chriscollins3456

Assignees
No one assigned
Labels
community-contribution PR or Issue raised by member(s) of DataHub Community hacktoberfest-accepted Acceptance for hacktoberfest https://hacktoberfest.com/participation/ product PR or Issue related to the DataHub UI/UX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Neo4j getLineage with multi-hop support
4 participants
@djordje-mijatovic @gabe-lyons @jjoyce0510 @maggiehays