support relationship v2 #462

hongliu2024 · 2024-10-28T18:08:53Z

Summary

This PR is to ensure the EbeanLocalWriterDAO can support both relationship V1 and relationship V2.
The changes are in Schema Validation for Relationship v2, RecordUtils, ModelUtils, and GraphUtils.

Question though is checkSameUrn logic in GraphUtils.java. They are related to the removal option, and for some options, they check for source Urn. But in Relationship model v2, there is no source field. Should it just skip the check for source field for V2, or should it look at the belonging aspect to find the source Urn? I feel looking at belonging aspect can have problems too because one relationship can be included in multiple different aspects.

Testing Done

Unit test cases added.

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Docs related to the changes have been added/updated (if applicable)

jsdonn · 2024-10-28T20:52:46Z

dao-api/src/main/java/com/linkedin/metadata/dao/utils/ModelUtils.java

+   */
+  static boolean isRelationshipInV2(@Nonnull RecordDataSchema schema) {
+    // check the data type of the source and destination fields in schema and see if it's a union type
+    return schema.getFields().stream().anyMatch(


Yang is saying we should instead keep the source in the base RelationshipV2.proto model. and then specific types of relationships will have 'destination' field only. Check: #456 (comment)

What do you think? The issue with keeping "source" in the relationship model, which is part of the aspect model now is that the source could be different from the asset urn of the aspect.

as discussed offline, I removed the source field from the v2 model

jsdonn · 2024-10-28T20:58:21Z

dao-api/src/main/java/com/linkedin/metadata/dao/utils/ModelUtils.java

+    if (dataMap == null) {
+      ValidationUtils.throwNullFieldException(fieldName);
+    }
+    return dataMap.keySet().iterator().next();


QQ: does this always work because the iterator will only have 1 item?

this field is translated from proto oneOf, which should have only 1 item.

jsdonn · 2024-10-28T20:59:27Z

dao-api/src/main/java/com/linkedin/metadata/dao/utils/ModelUtils.java

+          .getField(fieldName)
+          .getType()
+          .getMembers()
+          .get(0)


can there be index out of bounds exception?

Removed this logic.

jsdonn · 2024-10-28T21:00:55Z

validators/src/main/java/com/linkedin/metadata/validator/RelationshipValidator.java

-    });
-
-    validatePairings(schema);
+    // Relationship V2 uses AuditStamp class, instead of primitive types


IIRC we also need to replace usages of com.linkedin.common.Time with int64. Does this PR cover that?

I'm thinking to use AuditStamp class to replace the AuditStampActor and AuditStampTimeAt

Example TestRelationship.proto

Please let me know wdyt

codecov-commenter · 2024-10-30T07:17:09Z

Codecov Report

Attention: Patch coverage is 70.37037% with 8 lines in your changes missing coverage. Please review.

Project coverage is 67.99%. Comparing base (efa96e0) to head (06849d9).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...va/com/linkedin/metadata/dao/utils/ModelUtils.java	77.77%	2 Missing and 2 partials ⚠️
...a/com/linkedin/metadata/dao/utils/RecordUtils.java	50.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #462      +/-   ##
============================================
+ Coverage     67.61%   67.99%   +0.38%     
- Complexity     1604     1626      +22     
============================================
  Files           143      143              
  Lines          6207     6237      +30     
  Branches        675      678       +3     
============================================
+ Hits           4197     4241      +44     
+ Misses         1726     1707      -19     
- Partials        284      289       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jsdonn · 2024-10-30T19:16:38Z

dao-api/src/main/java/com/linkedin/metadata/dao/utils/ModelUtils.java

+    if (!relationshipInV2) {
+      // if V1, get the urn from the given field name, "source" or "destination"
+      urn = RecordUtils.getRecordTemplateField(relationship, fieldName, urnClassForRelationship(relationship.getClass(), fieldName));
+    } else {


should we validate that fieldName = 'destination' in this case?

jsdonn · 2024-10-30T19:56:19Z

dao-api/src/main/java/com/linkedin/metadata/dao/utils/GraphUtils.java

@@ -25,6 +24,7 @@ public static void checkSameUrn(@Nonnull final List<? extends RecordTemplate> re
      return;
    }

+    // ToDo: how to handle this for Relationship V2?


Should it just skip the check for source field for V2, or should it look at the belonging aspect to find the source Urn? I feel looking at belonging aspect can have problems too because one relationship can be included in multiple different aspects.

We can just check during runtime if the source urn in the relationship(s) match the asset urn being ingested. I will add it in my followup PR.

Sounds good. Should I keep this ToDo here?

jsdonn · 2024-10-30T19:57:41Z

testing/test-models/src/main/pegasus/com/linkedin/testing/AssetWithRelationship.pdl

+  /**
+   * For unit tests
+   */
+  aspect_with_relationship: AspectWithRelationship


nit: use camelcase

ybz1013

One question

ybz1013 · 2024-10-30T21:35:45Z

dao-api/src/main/java/com/linkedin/metadata/dao/utils/ModelUtils.java

@@ -306,7 +318,11 @@ private static <RELATIONSHIP extends RecordTemplate> Urn getUrnFromRelationship(
  @Nonnull
  public static <RELATIONSHIP extends RecordTemplate> Urn getSourceUrnFromRelationship(


EbeanLocalRelationshipWriterDAO.addRelationshipGroup invokes this for each relationship groups when create sql updates.

Since here it will throw exceptions, how are the relationship updates are fired?

That's a great question. I was trying to handle it but it needs to come from aspect.
@jsdonn - I'd like to discuss about this.
For now I think it's ok to push this PR out since V1 isn't impacted by this.

As discussed offline, Hong will help add logic to propagate the source urn passed in during ingestion to the EbeanLocalRelationshipWriterDAO

jsdonn

LGTM thanks for doing this. Let's address the concerns around not including the source field in relationship 2.0 models in a fast-follow PR

support relationship v2

69793b4

jsdonn reviewed Oct 28, 2024

View reviewed changes

add test relationship, update

06849d9

hongliu2024 marked this pull request as ready for review October 30, 2024 07:37

jsdonn reviewed Oct 30, 2024

View reviewed changes

ybz1013 reviewed Oct 30, 2024

View reviewed changes

hongliu2024 added 3 commits November 3, 2024 22:58

add more test case

1044ae5

update

1a50cd9

address comments

ba537cd

jsdonn approved these changes Nov 5, 2024

View reviewed changes

Merge branch 'master' into hongliu/support_relationship_v2

811d02e

hongliu2024 merged commit 9ea5af8 into master Nov 5, 2024
1 of 2 checks passed

hongliu2024 deleted the hongliu/support_relationship_v2 branch November 5, 2024 19:30

hongliu2024 restored the hongliu/support_relationship_v2 branch November 5, 2024 21:09

hongliu2024 deleted the hongliu/support_relationship_v2 branch November 5, 2024 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support relationship v2 #462

support relationship v2 #462

hongliu2024 commented Oct 28, 2024 •

edited

Loading

jsdonn Oct 28, 2024

hongliu2024 Oct 30, 2024

jsdonn Oct 28, 2024

hongliu2024 Oct 30, 2024

jsdonn Oct 28, 2024

hongliu2024 Oct 30, 2024

jsdonn Oct 28, 2024

hongliu2024 Oct 30, 2024

codecov-commenter commented Oct 30, 2024

jsdonn Oct 30, 2024

hongliu2024 Nov 4, 2024

jsdonn Oct 30, 2024

hongliu2024 Oct 30, 2024

jsdonn Oct 30, 2024

hongliu2024 Nov 4, 2024

ybz1013 left a comment

ybz1013 Oct 30, 2024

hongliu2024 Nov 4, 2024

jsdonn Nov 5, 2024

jsdonn left a comment

		@@ -306,7 +318,11 @@ private static <RELATIONSHIP extends RecordTemplate> Urn getUrnFromRelationship(
		@Nonnull
		public static <RELATIONSHIP extends RecordTemplate> Urn getSourceUrnFromRelationship(

support relationship v2 #462

support relationship v2 #462

Conversation

hongliu2024 commented Oct 28, 2024 • edited Loading

Summary

Testing Done

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 30, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ybz1013 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdonn left a comment

Choose a reason for hiding this comment

hongliu2024 commented Oct 28, 2024 •

edited

Loading