feat(lineage) Implement CLL impact analysis for inputFields #6426

chriscollins3456 · 2022-11-14T17:57:48Z

Currently, impact analysis only works for fineGrainedLineage but it needs to work for inputFields as well. Both of these aspects use schemaFields so all the infrastructure for fetching impact analysis for schema fields is already there, we just were not indexing inputFields to draw lines between schemaFields like we were for the upstreamLineage aspect yet. This adds that step to draw those edges manually.

Something to note is that we need to encode the fieldPath of the schemaField urn that we generate for the graph index. this is because ingestion always does that for schemaField urns but here we are creating a schemaField urn from the schemaField prop of inputFields and the fieldPath there is not encoded. schemaField urns need to be consistent across our DB and graph index as we request using the encoded urn from the FE and all over.

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

github-actions · 2022-11-14T18:33:14Z

Unit Test Results (build & test)

615 tests +2 611 ✔️ +2 12m 10s ⏱️ +22s
152 suites +1     4 💤 ±0
152 files +1     0 ❌ ±0

Results for commit 61b6dee. ± Comparison against base commit 7ea9797.

♻️ This comment has been updated with latest results.

jjoyce0510 · 2022-11-14T20:36:48Z

...data-jobs/mae-consumer/src/main/java/com/linkedin/metadata/kafka/hook/UpdateIndicesHook.java

@@ -180,15 +183,45 @@ private void updateFineGrainedEdgesAndRelationships(
    }
  }

+  private Urn generateSchemaFieldUrn(@Nonnull String resourceUrn, @Nonnull String fieldPath) {
+    // we rely on schemaField fieldPaths to be encoded since we do that with fineGrainedLineage on the ingestion side


Nice - thanks for the explanation

jjoyce0510 · 2022-11-14T20:37:22Z

...data-jobs/mae-consumer/src/main/java/com/linkedin/metadata/kafka/hook/UpdateIndicesHook.java

+  }
+
+  private void updateInputFieldEdgesAndRelationships(
+      Urn urn,


nit - can any of these arguments be null?

(If yes, add @nullable. If no, add @nonnull - the IDE will help point out scenarios in which you may violate these constraints)

right, adding this nonnull annotations now

jjoyce0510 · 2022-11-14T20:37:45Z

...data-jobs/mae-consumer/src/main/java/com/linkedin/metadata/kafka/hook/UpdateIndicesHook.java

+      List<Edge> edgesToAdd,
+      HashMap<Urn, Set<String>> urnToRelationshipTypesBeingAdded
+  ) {
+    InputFields inputFields = new InputFields(aspect.data());


Consider passing InputFields as a parameter type, instead of RecordTemplate

jjoyce0510 · 2022-11-14T20:38:30Z

...data-jobs/mae-consumer/src/main/java/com/linkedin/metadata/kafka/hook/UpdateIndicesHook.java

+    InputFields inputFields = new InputFields(aspect.data());
+    if (inputFields.hasFields()) {
+      for (InputField field : inputFields.getFields()) {
+        if (field.hasSchemaFieldUrn() && field.hasSchemaField() && field.getSchemaField().hasFieldPath()) {


Doesn't this already have a schemaFieldUrn in this case? Why cannot we use this URN? Is it not encoded?

this is part of the confusing modeling thing here - schemaFieldUrn is actually the upstream urn for this inputField and it could be null. So the schemaFieldUrn and the schemaField properties here are going to be referencing two different schemaFields

and the schemaFieldUrn is encoded

jjoyce0510 · 2022-11-14T20:41:10Z

...data-jobs/mae-consumer/src/main/java/com/linkedin/metadata/kafka/hook/UpdateIndicesHook.java

  private Pair<List<Edge>, HashMap<Urn, Set<String>>> getEdgesAndRelationshipTypesFromAspect(Urn urn, AspectSpec aspectSpec, RecordTemplate aspect) {
    final List<Edge> edgesToAdd = new ArrayList<>();
    final HashMap<Urn, Set<String>> urnToRelationshipTypesBeingAdded = new HashMap<>();

+    // we need to manually set schemaField <-> schemaField edges for fineGrainedLineage and inputFields


Minor: Ideally this domain-specific schema field logic does not reside inside a much more generic UpdateIndicesHook. There should be some abstraction for encapsulating such special case logic and a way to register this logic with the index updater.

(For a future refactor)

The idea of UpdateIndicesHook is to be completely agnostic of domain-specific logic that exists

yeah this all makes sense to me and is good to call out

jjoyce0510 · 2022-11-14T20:43:10Z

...data-jobs/mae-consumer/src/main/java/com/linkedin/metadata/kafka/hook/UpdateIndicesHook.java

      updateFineGrainedEdgesAndRelationships(aspect, edgesToAdd, urnToRelationshipTypesBeingAdded);
    }
+    if (aspectSpec.getName().equals(Constants.INPUT_FIELDS_ASPECT_NAME)) {


Qq - Do we have unit tests for this class? If not, we absolutely need to backfill since it's so important

we do not have anything for this class...

Can we add a TODO to do this on this file?

yeah I'm about to push up the beginning of a test file for this, but will add a TODO at the top of the class to backfill the rest of the functionality!

aditya-radhakrishnan · 2022-11-15T01:18:36Z

...data-jobs/mae-consumer/src/main/java/com/linkedin/metadata/kafka/hook/UpdateIndicesHook.java

@@ -180,15 +183,45 @@ private void updateFineGrainedEdgesAndRelationships(
    }
  }

+  private Urn generateSchemaFieldUrn(@Nonnull String resourceUrn, @Nonnull String fieldPath) {
+    // we rely on schemaField fieldPaths to be encoded since we do that with fineGrainedLineage on the ingestion side
+    String encodedFieldPath = fieldPath.replaceAll("\\(", "%28").replaceAll("\\)", "%29").replaceAll(",", "%2C");


nit: always good to make any fields that aren't going to change final

Also for any function parameters!

sounds good!

aditya-radhakrishnan · 2022-11-15T01:21:16Z

...data-jobs/mae-consumer/src/main/java/com/linkedin/metadata/kafka/hook/UpdateIndicesHook.java

+  private Urn generateSchemaFieldUrn(@Nonnull String resourceUrn, @Nonnull String fieldPath) {
+    // we rely on schemaField fieldPaths to be encoded since we do that with fineGrainedLineage on the ingestion side
+    String encodedFieldPath = fieldPath.replaceAll("\\(", "%28").replaceAll("\\)", "%29").replaceAll(",", "%2C");
+    String urnString = String.format("urn:li:schemaField:(%s,%s)", resourceUrn, encodedFieldPath);


Instead of hardcoding urn:li:schemaField, can we add it a constant somewhere?

good call and in fact i'll do you one better and use EntityKeyUtils.convertEntityKeyToUrn to be even more safe/systematic!

aditya-radhakrishnan · 2022-11-15T01:21:23Z

...data-jobs/mae-consumer/src/main/java/com/linkedin/metadata/kafka/hook/UpdateIndicesHook.java

+  }
+
+  private void updateInputFieldEdgesAndRelationships(
+      Urn urn,


aditya-radhakrishnan · 2022-11-15T01:24:11Z

...data-jobs/mae-consumer/src/main/java/com/linkedin/metadata/kafka/hook/UpdateIndicesHook.java

      updateFineGrainedEdgesAndRelationships(aspect, edgesToAdd, urnToRelationshipTypesBeingAdded);
    }
+    if (aspectSpec.getName().equals(Constants.INPUT_FIELDS_ASPECT_NAME)) {


Can we add a TODO to do this on this file?

aditya-radhakrishnan

LGTM!

aditya-radhakrishnan · 2022-11-15T19:11:48Z

...-jobs/mae-consumer/src/test/java/com/linkedin/metadata/kafka/hook/UpdateIndicesHookTest.java

+import static com.linkedin.metadata.Constants.DATASET_ENTITY_NAME;
+import static com.linkedin.metadata.search.utils.QueryUtils.newRelationshipFilter;
+
+public class UpdateIndicesHookTest {


Thanks for writing these!

…project#6426)

display impact analysis for input fields on charts as well

b783864

github-actions bot added the product PR or Issue related to the DataHub UI/UX label Nov 14, 2022

chriscollins3456 requested a review from aditya-radhakrishnan November 14, 2022 18:21

jjoyce0510 reviewed Nov 14, 2022

View reviewed changes

aditya-radhakrishnan reviewed Nov 15, 2022

View reviewed changes

PR edits

61b6dee

aditya-radhakrishnan approved these changes Nov 15, 2022

View reviewed changes

chriscollins3456 merged commit 1d944be into datahub-project:master Nov 15, 2022

cccs-Dustin pushed a commit to CybercentreCanada/datahub that referenced this pull request Feb 1, 2023

feat(lineage) Implement CLL impact analysis for inputFields (datahub-…

3f176b4

…project#6426)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lineage) Implement CLL impact analysis for inputFields #6426

feat(lineage) Implement CLL impact analysis for inputFields #6426

chriscollins3456 commented Nov 14, 2022

github-actions bot commented Nov 14, 2022 •

edited

Loading

jjoyce0510 Nov 14, 2022

jjoyce0510 Nov 14, 2022

jjoyce0510 Nov 14, 2022

chriscollins3456 Nov 14, 2022

aditya-radhakrishnan Nov 15, 2022

jjoyce0510 Nov 14, 2022

jjoyce0510 Nov 14, 2022

chriscollins3456 Nov 14, 2022

chriscollins3456 Nov 14, 2022

jjoyce0510 Nov 15, 2022

jjoyce0510 Nov 14, 2022

jjoyce0510 Nov 14, 2022

chriscollins3456 Nov 14, 2022

jjoyce0510 Nov 14, 2022

chriscollins3456 Nov 14, 2022

aditya-radhakrishnan Nov 15, 2022

chriscollins3456 Nov 15, 2022

aditya-radhakrishnan Nov 15, 2022

aditya-radhakrishnan Nov 15, 2022

chriscollins3456 Nov 15, 2022

aditya-radhakrishnan Nov 15, 2022

chriscollins3456 Nov 15, 2022

aditya-radhakrishnan Nov 15, 2022

aditya-radhakrishnan Nov 15, 2022

aditya-radhakrishnan left a comment

aditya-radhakrishnan Nov 15, 2022

feat(lineage) Implement CLL impact analysis for inputFields #6426

feat(lineage) Implement CLL impact analysis for inputFields #6426

Conversation

chriscollins3456 commented Nov 14, 2022

Checklist

github-actions bot commented Nov 14, 2022 • edited Loading

Unit Test Results (build & test)

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

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

aditya-radhakrishnan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 14, 2022 •

edited

Loading