Add the process of Relationship V2 in EbeanLocalRelationshipWriterDAO #467

hongliu2024 · 2024-11-06T07:34:07Z

Summary

This PR is to support processLocalRelationshipUpdates for relationship model v2.

Details

Changes include:

GraphUtils.java

Pass source asset urn in the checkSameUrn method for model v2.

GraphUtilsTest.java

Add test cases of model v2 into all scenarios

ModelUtils.java

Make isRelationshipInV2 methods public so they can be used in EbeanLocalRelationshipWriterDAO logic

EbeanLocalRelationshipWriterDAO.java

Add new parameter source asset urn to methods addRelationships, addRelationshipGroup, and processRemovalOption. The logic for model v1 remains the same, but the logic changes to take the passed parameter for model v2.
Update documents
Remove unused constant strings

EbeanLocalRelationshipWriterDAOTest.java

Add model v2 test cases to RemoveFromSource test cases, since that's the one to be supported for V2.

ebean-local-relationship-dao-create-all.sql

Add new relationship table based on the V2 relationship

ebean-local-relationship-create-all-with-non-dollar-virtual-column-names.sql

Add new relationship table based on the V2 relationship

Testing Done

./gradlew build


BUILD SUCCESSFUL in 58s
245 actionable tasks: 37 executed, 208 up-to-date

Add V2 test cases

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)

codecov-commenter · 2024-11-06T13:39:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.95%. Comparing base (cf25612) to head (405a7f2).
Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #467      +/-   ##
============================================
+ Coverage     67.92%   67.95%   +0.03%     
- Complexity     1628     1635       +7     
============================================
  Files           143      143              
  Lines          6247     6260      +13     
  Branches        680      683       +3     
============================================
+ Hits           4243     4254      +11     
- Misses         1712     1714       +2     
  Partials        292      292

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

ybz1013

some comments. Thanks for adding this support!

ybz1013 · 2024-11-06T21:22:50Z

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

   */
  public static void checkSameUrn(@Nonnull final List<? extends RecordTemplate> relationships,
-      @Nonnull final BaseGraphWriterDAO.RemovalOption removalOption, final String sourceField, final String destinationField) {
+      @Nonnull final BaseGraphWriterDAO.RemovalOption removalOption, final String sourceField,
+      final String destinationField, Urn urn) {


nit: annotation for nullable params

ybz1013 · 2024-11-06T21:24:47Z

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

-    // ToDo: how to handle this for Relationship V2?
-    final Urn sourceUrn = getSourceUrnFromRelationship(relationships.get(0));
+    Urn sourceUrn = urn;
+    if (!ModelUtils.isRelationshipInV2(relationships.get(0).schema())) {


index check before .get(0) needed or no?

There is a null check at the begining of this method, so the index check won't be necessary.

ybz1013 · 2024-11-06T21:29:05Z

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

+      sourceUrn = getSourceUrnFromRelationship(relationships.get(0));
+    }
+    if (sourceUrn == null) {
+      throw new IllegalArgumentException("Source urn is needed for Relationship V2");


The v1 v2 case is a little hard to follow. Could you add some comments or maybe consider this?

if (!ModelUtils.isRelationshipInV2(relationships.get(0).schema())) { // v1 case } else { // v2 case if (urn == null) { throw new IllegalArgumentException("Source urn is needed for Relationship V2"); } sourceUrn = urn; }

Thanks. Moved into a separate method and added comments.

ybz1013 · 2024-11-06T21:33:41Z

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalRelationshipWriterDAO.java

  public <RELATIONSHIP extends RecordTemplate> void addRelationships(@Nonnull List<RELATIONSHIP> relationships,
-      @Nonnull RemovalOption removalOption, boolean isTestMode) {
+      @Nonnull RemovalOption removalOption, boolean isTestMode, Urn urn) {


nit: @nullable Urn urn

urn is nullable for V1

ybz1013 · 2024-11-06T21:36:31Z

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalRelationshipWriterDAO.java

+      // For relationship model V1, this given urn can be source urn or destination urn.
+      // For relationship model V2, this given urn can only be source urn.
+      Urn source = urn;
+      if (!ModelUtils.isRelationshipInV2(relationship.schema())) {


duplicate code... Maybe a helper function getSourceUrnFromEither(relationship, urn)?

Good catch. Put that logic into a separate method, and removed the dups.

ybz1013 · 2024-11-06T21:36:53Z

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalRelationshipWriterDAO.java


    if (removalOption == RemovalOption.REMOVE_NONE) {
      return;
    }

    SqlUpdate deletionSQL = _server.createSqlUpdate(SQLStatementUtils.deleteLocalRelationshipSQL(tableName, removalOption));
-    Urn source = getSourceUrnFromRelationship(relationship);
+    Urn source = urn;


same as the above. removed the dups. Thanks.

jsdonn · 2024-11-06T22:18:44Z

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

   */
  public static void checkSameUrn(@Nonnull final List<? extends RecordTemplate> relationships,
-      @Nonnull final BaseGraphWriterDAO.RemovalOption removalOption, final String sourceField, final String destinationField) {
+      @Nonnull final BaseGraphWriterDAO.RemovalOption removalOption, @Nonnull final String sourceField,
+      @Nonnull final String destinationField, Urn urn) {


nit: @nonnull or @nullable annotation for urn

jsdonn · 2024-11-06T22:19:18Z

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

+   * @return The source asset urn.
+   */
+  public static <RELATIONSHIP extends RecordTemplate> Urn getSourceUrnBasedOnRelationshipVersion(
+      @Nonnull RELATIONSHIP relationship, Urn urn) {


nit: @nonnull or @nullable annotation for urn

jsdonn · 2024-11-06T22:20:33Z

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

  private static void checkSameUrn(@Nonnull List<? extends RecordTemplate> records, @Nonnull String field,
      @Nonnull Urn compare) {
    for (RecordTemplate relation : records) {
+      if (ModelUtils.isRelationshipInV2(relation.schema()) && field == SOURCE) {


nit: string should use .equals comparison

jsdonn · 2024-11-06T22:22:35Z

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalRelationshipWriterDAO.java

    });
  }
+  @Override


nit: newline after closing curly brace

jsdonn · 2024-11-06T22:23:03Z

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalRelationshipWriterDAO.java

  private <RELATIONSHIP extends RecordTemplate> void addRelationshipGroup(@Nonnull final List<RELATIONSHIP> relationshipGroup,
-      @Nonnull RemovalOption removalOption, boolean isTestMode) {
+      @Nonnull RemovalOption removalOption, boolean isTestMode, Urn urn) {


nit: @nonnull or @nullable annotation for urn

jsdonn · 2024-11-06T22:23:19Z

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalRelationshipWriterDAO.java

+   *            Needed for Relationship V2 because source is not included in the relationshipV2 metadata.
+   */
+  private <RELATIONSHIP extends RecordTemplate> void processRemovalOption(@Nonnull String tableName,
+      @Nonnull RELATIONSHIP relationship, @Nonnull RemovalOption removalOption, Urn urn) {


nit: urn annotation

ybz1013

LGTM

jsdonn

LGTM. thanks for the detailed PR description

add support to process Relationship V2

63ee89a

ybz1013 reviewed Nov 6, 2024

View reviewed changes

address comments

7716f39

jsdonn reviewed Nov 6, 2024

View reviewed changes

address comments

405a7f2

ybz1013 approved these changes Nov 6, 2024

View reviewed changes

jsdonn approved these changes Nov 6, 2024

View reviewed changes

ybz1013 merged commit 5d60c0c into master Nov 6, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the process of Relationship V2 in EbeanLocalRelationshipWriterDAO #467

Add the process of Relationship V2 in EbeanLocalRelationshipWriterDAO #467

hongliu2024 commented Nov 6, 2024

codecov-commenter commented Nov 6, 2024 •

edited

Loading

ybz1013 left a comment

ybz1013 Nov 6, 2024

ybz1013 Nov 6, 2024

hongliu2024 Nov 6, 2024

ybz1013 Nov 6, 2024

hongliu2024 Nov 6, 2024

ybz1013 Nov 6, 2024

hongliu2024 Nov 6, 2024

ybz1013 Nov 6, 2024

hongliu2024 Nov 6, 2024

ybz1013 Nov 6, 2024

hongliu2024 Nov 6, 2024

jsdonn Nov 6, 2024

jsdonn Nov 6, 2024

jsdonn Nov 6, 2024

jsdonn Nov 6, 2024

jsdonn Nov 6, 2024

jsdonn Nov 6, 2024

ybz1013 left a comment

jsdonn left a comment

Add the process of Relationship V2 in EbeanLocalRelationshipWriterDAO #467

Add the process of Relationship V2 in EbeanLocalRelationshipWriterDAO #467

Conversation

hongliu2024 commented Nov 6, 2024

Summary

Details

Testing Done

Checklist

codecov-commenter commented Nov 6, 2024 • edited Loading

Codecov Report

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

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

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

jsdonn left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 6, 2024 •

edited

Loading