-
Notifications
You must be signed in to change notification settings - Fork 58
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.
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
feat(model 2.0): implement extractRelationshipsFromAspect util method #449
Merged
jsdonn
merged 5 commits into
linkedin:master
from
jsdonn:extract_relationship_from_aspect
Oct 16, 2024
Merged
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
aab78b0
feat(model 2.0): implement extractRelationshipsFromAspect util method
jsdonn 3240818
specify in javadocs that only top-level relationships can be extracted
jsdonn 5433dcd
fix checkstyle
jsdonn d1dc935
add more test cases and address comments
jsdonn be8c548
fix unit tests
jsdonn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package com.linkedin.metadata.dao.utils; | ||
|
||
import com.google.common.io.Resources; | ||
import com.linkedin.data.template.RecordTemplate; | ||
import com.linkedin.data.template.StringArray; | ||
import com.linkedin.metadata.aspect.AuditedAspect; | ||
import com.linkedin.metadata.dao.EbeanLocalAccess; | ||
|
@@ -13,7 +14,13 @@ | |
import com.linkedin.metadata.query.LocalRelationshipValue; | ||
import com.linkedin.metadata.query.RelationshipField; | ||
import com.linkedin.metadata.query.UrnField; | ||
import com.linkedin.testing.AnnotatedAspectBarWithRelationshipFields; | ||
import com.linkedin.testing.AnnotatedRelationshipBar; | ||
import com.linkedin.testing.AnnotatedRelationshipBarArray; | ||
import com.linkedin.testing.AnnotatedRelationshipFoo; | ||
import com.linkedin.testing.AnnotatedRelationshipFooArray; | ||
import com.linkedin.testing.AspectFoo; | ||
import com.linkedin.testing.AnnotatedAspectFooWithRelationshipField; | ||
import com.linkedin.testing.urn.BurgerUrn; | ||
import com.linkedin.testing.urn.FooUrn; | ||
import io.ebean.Ebean; | ||
|
@@ -33,6 +40,7 @@ | |
import org.testng.annotations.Test; | ||
|
||
import static com.linkedin.testing.TestUtils.*; | ||
import static org.junit.Assert.*; | ||
import static org.mockito.Mockito.*; | ||
import static org.testng.Assert.assertFalse; | ||
import static org.testng.Assert.assertTrue; | ||
|
@@ -567,4 +575,55 @@ public void testBuildRelationshipFieldCriterionWithAspectField() { | |
relationshipField); | ||
assertEquals(relationshipField, filterCriterion.getField().getRelationshipField()); | ||
} | ||
|
||
@Test | ||
public void testExtractRelationshipsFromAspect() { | ||
// case 1: aspect model does not contain any relationship typed fields | ||
// expected: return null | ||
AspectFoo foo = new AspectFoo(); | ||
assertTrue(EBeanDAOUtils.extractRelationshipsFromAspect(foo).isEmpty()); | ||
|
||
// case 2: aspect model contains only a non-null relationship type field | ||
// expected: return the non-null relationship | ||
AnnotatedRelationshipFoo relationshipFoo = new AnnotatedRelationshipFoo(); | ||
AnnotatedRelationshipFooArray relationshipFoos = new AnnotatedRelationshipFooArray(relationshipFoo); | ||
AnnotatedAspectFooWithRelationshipField fooWithRelationshipField = new AnnotatedAspectFooWithRelationshipField() | ||
.setRelationshipFoo(relationshipFoos); | ||
|
||
List<List<RecordTemplate>> results = EBeanDAOUtils.extractRelationshipsFromAspect(fooWithRelationshipField); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if EBeanDAOUtils.extractRelationshipsFromAspect returns List<List>, what's the point for the generic type RELATIONSHIP |
||
assertEquals(1, results.size()); | ||
assertEquals(1, results.get(0).size()); | ||
assertEquals(relationshipFoo, results.get(0).get(0)); | ||
|
||
// case 3: aspect model contains only a null relationship type field | ||
// expected: return null | ||
AnnotatedAspectFooWithRelationshipField fooWithNullRelationshipField = new AnnotatedAspectFooWithRelationshipField(); | ||
assertTrue(EBeanDAOUtils.extractRelationshipsFromAspect(fooWithNullRelationshipField).isEmpty()); | ||
|
||
// case 4: aspect model contains multiple relationship fields, some null and some non-null | ||
jsdonn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// expected: return only the non-null relationships | ||
relationshipFoos = new AnnotatedRelationshipFooArray(new AnnotatedRelationshipFoo(), new AnnotatedRelationshipFoo()); | ||
AnnotatedRelationshipBarArray relationshipBars = new AnnotatedRelationshipBarArray(new AnnotatedRelationshipBar()); | ||
// given: | ||
// aspect = { | ||
// value -> "abc" | ||
// relationshipFoos -> [foo1, foo2] | ||
// relationshipBars -> [bar1] | ||
// moreRelationshipFoos -> not present | ||
// } | ||
// expect: | ||
// [[foo1, foo2], [bar1]] | ||
AnnotatedAspectBarWithRelationshipFields barWithRelationshipFields = new AnnotatedAspectBarWithRelationshipFields() | ||
.setValue("abc") | ||
.setRelationshipFoos(relationshipFoos) | ||
.setRelationshipBars(relationshipBars); // don't set moreRelationshipFoos field | ||
|
||
results = EBeanDAOUtils.extractRelationshipsFromAspect(barWithRelationshipFields); | ||
assertEquals(2, results.size()); | ||
assertEquals(2, results.get(0).size()); | ||
assertEquals(1, results.get(1).size()); | ||
assertEquals(new AnnotatedRelationshipFoo(), results.get(0).get(0)); | ||
assertEquals(new AnnotatedRelationshipFoo(), results.get(0).get(1)); | ||
assertEquals(new AnnotatedRelationshipBar(), results.get(1).get(0)); | ||
} | ||
} |
This file contains 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
28 changes: 28 additions & 0 deletions
28
...models/src/main/pegasus/com/linkedin/testing/AnnotatedAspectBarWithRelationshipFields.pdl
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
namespace com.linkedin.testing | ||
|
||
/** | ||
* For unit tests | ||
*/ | ||
@gma.aspect.column.name = "annotatedaspectbarwithrelationshipfields" | ||
@gma.model = "ASPECT" | ||
record AnnotatedAspectBarWithRelationshipFields { | ||
/** | ||
* For unit tests | ||
*/ | ||
value: string | ||
|
||
/** | ||
* For unit tests | ||
*/ | ||
relationshipFoos: optional array[AnnotatedRelationshipFoo] | ||
|
||
/** | ||
* For unit tests | ||
*/ | ||
relationshipBars: optional array[AnnotatedRelationshipBar] | ||
|
||
/** | ||
* For unit tests | ||
*/ | ||
moreRelationshipFoos: optional array[AnnotatedRelationshipFoo] | ||
} |
18 changes: 18 additions & 0 deletions
18
...-models/src/main/pegasus/com/linkedin/testing/AnnotatedAspectFooWithRelationshipField.pdl
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
namespace com.linkedin.testing | ||
|
||
/** | ||
* For unit tests | ||
*/ | ||
@gma.aspect.column.name = "annotatedaspectfoowithrelationshipfield" | ||
@gma.model = "ASPECT" | ||
record AnnotatedAspectFooWithRelationshipField { | ||
/** | ||
* For unit tests | ||
*/ | ||
value: string | ||
|
||
/** | ||
* For unit tests | ||
*/ | ||
relationshipFoo: optional array[AnnotatedRelationshipFoo] | ||
} |
24 changes: 24 additions & 0 deletions
24
...nnotations-test-models/src/main/pegasus/com/linkedin/testing/AnnotatedRelationshipBar.pdl
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
namespace com.linkedin.testing | ||
|
||
import com.linkedin.common.Urn | ||
|
||
/** | ||
* For unit tests | ||
*/ | ||
@pairings = [ { | ||
jsdonn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"destination" : "com.linkedin.common.urn.Urn", | ||
"source" : "com.linkedin.common.urn.Urn" | ||
} ] | ||
@gma.model = "RELATIONSHIP" | ||
record AnnotatedRelationshipBar { | ||
|
||
/** | ||
* For unit tests | ||
*/ | ||
source: Urn | ||
|
||
/** | ||
* For unit tests | ||
*/ | ||
destination: Urn | ||
} |
24 changes: 24 additions & 0 deletions
24
...nnotations-test-models/src/main/pegasus/com/linkedin/testing/AnnotatedRelationshipFoo.pdl
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
namespace com.linkedin.testing | ||
|
||
import com.linkedin.common.Urn | ||
|
||
/** | ||
* For unit tests | ||
*/ | ||
@pairings = [ { | ||
"destination" : "com.linkedin.common.urn.Urn", | ||
"source" : "com.linkedin.common.urn.Urn" | ||
} ] | ||
@gma.model = "RELATIONSHIP" | ||
record AnnotatedRelationshipFoo { | ||
|
||
/** | ||
* For unit tests | ||
*/ | ||
source: Urn | ||
|
||
/** | ||
* For unit tests | ||
*/ | ||
destination: Urn | ||
} |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean an Aspect can contain contain one relationship type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few thoughts:
have a cache for each Aspect type, and the relationship type and path, since these information are deterministic through the application life cycle. This cache maintains
<Aspect.class, List<<Relationship.class, List<field_path>>>>
the method could be List extractRelationshipFromAspect(Aspect aspect, Class relationshipType)
The implementation of 2 can be extended with recursive lookup or get List<field_path> for relationship type in an Aspect for fast access.
What's more important over the performance improvement is we can remove the 1:1 mapping from relationship to aspect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A cache is a good suggestion, although a caveat is that some of the relationship fields of an aspect can be not present when we allow an aspect to have multiple relationships. For example, aspect with model:
On the first ingestion of this aspect, the relationshipBars is present but relationshipFooBars may not be present. so the cache will not register that field. So on the next ingestion of this aspect, we need to check every field again. I will leave this optimization for the future if we see that this logic has noticeable performance impact.