fix(graph service): only query for entities that should have lineage [Breaking Change] #5539

gabe-lyons · 2022-08-01T23:29:30Z

Our graph service historically queried for all entity types. However, the set of relationship types it queries is the set of relationship types that may exist in some lineage. This means entities that are not included in lineage were getting pulled in. Rather than querying for specific entity->relationship type->entity pairs, I opted for a simper approach of changing the graph service interface to accept a list of entity types.

This approach sets us up to be able to select the set of entity types we are interested in querying as a config or from the UI.

This is a breaking change for anyone who has built their own implementation of the GraphService interface

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

gabe-lyons · 2022-08-01T23:29:52Z

datahub-web-react/src/app/entity/dataset/profile/OperationsTab.tsx

@@ -147,7 +147,7 @@ export const OperationsTab = () => {
            inputs: run?.inputs?.relationships.map((relationship) => relationship.entity),
            outputs: run?.outputs?.relationships.map((relationship) => relationship.entity),
            externalUrl: run?.externalUrl,
-            parentTemplate: run?.parentTemplate?.relationships?.[0].entity,
+            parentTemplate: run?.parentTemplate?.relationships?.[0]?.entity,


small bug fix i noticed while testing

gabe-lyons · 2022-08-01T23:30:00Z

datahub-web-react/src/app/recommendations/renderer/component/CompactEntityNameList.tsx

@@ -19,6 +19,8 @@ export const CompactEntityNameList = ({ entities, onClick, linkUrlParams, showTo
    return (
        <>
            {entities.map((entity, index) => {
+                if (!entity) return <></>;


same, small bug fix here ^

Is entity not a required field? Didn't realize... Nice!

github-actions · 2022-08-01T23:36:20Z

Unit Test Results (build & test)

499 tests ±0 499 ✔️ ±0 8m 23s ⏱️ -32s
115 suites ±0     0 💤 ±0
115 files ±0     0 ❌ ±0

Results for commit 63fb9e2. ± Comparison against base commit 626196d.

♻️ This comment has been updated with latest results.

jjoyce0510 · 2022-08-03T03:42:45Z

entity-registry/src/main/java/com/linkedin/metadata/models/registry/LineageRegistry.java

@@ -86,6 +89,14 @@ public LineageSpec getLineageSpec(String entityName) {
    return _lineageSpecMap.get(entityName.toLowerCase());
  }

+  public Set<String> getEntitiesWithLineage() {
+    Map<String, EntitySpec> specs = _entityRegistry.getEntitySpecs();
+    return _lineageSpecMap.keySet().stream().filter(key ->


Minor: You maybe could use _lineageSpecMap.entrySet() to avoid the .get(key) calls

jjoyce0510 · 2022-08-03T03:44:40Z

metadata-io/src/main/java/com/linkedin/metadata/graph/GraphFilters.java

+
+@Data
+@AllArgsConstructor
+public class GraphFilters {


For other services (e.g. search service) we use PDLs to model the filter types...Any reason why we shouldn't keep that consistent here?

These aren't exposed via API yet so ill keep them just in java for now so its easier to change. Once we've settled on them let's move them to pdl.

jjoyce0510 · 2022-08-03T03:47:23Z

metadata-io/src/main/java/com/linkedin/metadata/graph/GraphService.java

@@ -79,11 +80,12 @@ public interface GraphService {
   *   - RelatedEntity("DownstreamOf", "dataset three")
   */
  @Nonnull
-  RelatedEntitiesResult findRelatedEntities(@Nullable final String sourceType, @Nonnull final Filter sourceEntityFilter,
-      @Nullable final String destinationType, @Nonnull final Filter destinationEntityFilter,
+  RelatedEntitiesResult findRelatedEntities(@Nullable final List<String> sourceTypes, @Nonnull final Filter sourceEntityFilter,


Minor thing : We don't really need to remove this API, we could just have the old API call the new API with the different arguments.

RelatedEntitiesResult findRelatedEntities(final String sourceType, Filter filter,...) { return findRelatedEntities(ImmutableList.of(sourceType, ...); } RelatedEntitiesResult findRelatedEntities(final List<String> sourceType, Filter filter,...) { }

This actually creates ambiguity if the user provides null for both fields (which is something we support), so I'll keep it as is for now.

jjoyce0510 · 2022-08-03T03:53:23Z

metadata-io/src/main/java/com/linkedin/metadata/graph/GraphService.java

+   * Unless overridden, it uses the lineage registry to fetch valid edge types and queries for them
+   */
+  @Nonnull
+  default EntityLineageResult getLineage(@Nonnull Urn entityUrn, @Nonnull LineageDirection direction,


Is there a way we can only filter for entity types that the current URN is related to via a lineage edge?

It seems we are just filtering for anything that has lineage... That will solve our immedaite problem with data process instance but in the long tem could this pop up again?

addressed- only relevant entity types will be pulled

jjoyce0510 · 2022-08-03T03:59:30Z

metadata-io/src/main/java/com/linkedin/metadata/graph/dgraph/DgraphGraphService.java

@@ -291,16 +291,20 @@ protected static String getQueryForRelatedEntities(@Nullable String sourceType,
        List<String> destinationFilterNames = new ArrayList<>();
        List<String> relationshipTypeFilterNames = new ArrayList<>();

-        if (sourceType != null) {
+        if (sourceTypes != null && sourceTypes.size() > 0) {


jjoyce0510 · 2022-08-03T04:02:00Z

metadata-io/src/main/java/com/linkedin/metadata/graph/elastic/ESGraphQueryDAO.java

@@ -294,9 +295,22 @@ public QueryBuilder getQueryForLineage(List<Urn> urns, List<EdgeInfo> lineageEdg
      incomingEdgeQuery.must(buildEdgeFilters(incomingEdges));
      query.should(incomingEdgeQuery);
    }
+
+    if (graphFilters != null) {


minor nit: this function is getting a bit long.. consider breaking into sub methods

jjoyce0510 · 2022-08-03T04:04:49Z

metadata-io/src/main/java/com/linkedin/metadata/graph/elastic/ESGraphWriteDAO.java

@@ -50,8 +51,9 @@ public BulkByScrollResponse deleteByQuery(@Nullable final String sourceType, @No
      @Nullable final String destinationType, @Nonnull final Filter destinationEntityFilter,
      @Nonnull final List<String> relationshipTypes, @Nonnull final RelationshipFilter relationshipFilter) {
    BoolQueryBuilder finalQuery =
-        buildQuery(sourceType, sourceEntityFilter, destinationType, destinationEntityFilter, relationshipTypes,
-            relationshipFilter);
+        buildQuery(sourceType == null ? ImmutableList.of() : ImmutableList.of(sourceType), sourceEntityFilter,


you can also use Collections.emptyList for an empty list : )

also, this method can take null right?

if it shouldn't take null values you can mark the param as @nonnull

jjoyce0510 · 2022-08-03T04:05:10Z

metadata-io/src/main/java/com/linkedin/metadata/graph/elastic/ElasticSearchGraphService.java

@@ -108,9 +109,9 @@ public void addEdge(@Nonnull final Edge edge) {

  @Nonnull
  public RelatedEntitiesResult findRelatedEntities(
-      @Nullable final String sourceType,
+      @Nullable final List<String> sourceTypes,


should we keep nullable?

see below- yes

jjoyce0510 · 2022-08-03T04:06:49Z

metadata-io/src/main/java/com/linkedin/metadata/graph/neo4j/Neo4jGraphService.java

@@ -94,9 +95,9 @@ public void addEdge(@Nonnull final Edge edge) {

  @Nonnull
  public RelatedEntitiesResult findRelatedEntities(
-      @Nullable final String sourceType,
+      @Nullable final List<String> sourceTypes,


should we keep nullable? if we can avoid less program states we should go for it!

yes, null is going to represent all as it has historically

jjoyce0510 · 2022-08-03T04:10:42Z

metadata-io/src/main/java/com/linkedin/metadata/graph/neo4j/Neo4jGraphService.java

-    // Build Statement strings
+    String whereClause = "";
+
+    Boolean hasSourceTypes = sourceTypes != null && !sourceTypes.isEmpty();


any chance we can break this longer block into a sep method?

maybe

computeEntityTypeWhereClause

jjoyce0510 · 2022-08-03T04:14:08Z

metadata-io/src/main/java/com/linkedin/metadata/graph/neo4j/Neo4jGraphService.java

@@ -359,6 +389,12 @@ private static String criterionToString(@Nonnull CriterionArray criterionArray)
    return joiner.length() <= 2 ? "" : joiner.toString();
  }

+  /**


Should this be here?

jjoyce0510 · 2022-08-03T04:15:19Z

metadata-io/src/test/java/com/linkedin/metadata/graph/GraphServiceTestBase.java

    doTestFindRelatedEntitiesEntityType(anyType, null, downstreamOf, outgoingRelationships, service, downstreamOfDatasetOneRelatedEntity);

    service.addEdge(new Edge(datasetOneUrn, nullUrn, downstreamOf));
    syncAfterWrite();
-    doTestFindRelatedEntitiesEntityType(anyType, "null", downstreamOf, outgoingRelationships, service, nullRelatedEntity);
+    doTestFindRelatedEntitiesEntityType(anyType, ImmutableList.of("null"), downstreamOf, outgoingRelationships, service, nullRelatedEntity);


Is this right? Immutable list of null?

(Same as above)

yep! in the test there is actually an entity type of "null"

counterintuitive but okay :)

metadata-io/src/test/java/com/linkedin/metadata/graph/GraphServiceTestBase.java

jjoyce0510 · 2022-08-03T04:18:31Z

...restli-servlet-impl/src/main/java/com/linkedin/metadata/resources/lineage/Relationships.java

@@ -67,7 +68,7 @@ private RelatedEntitiesResult getRelatedEntities(String rawUrn, List<String> rel
    start = start == null ? 0 : start;
    count = count == null ? MAX_DOWNSTREAM_CNT : count;

-    return _graphService.findRelatedEntities("", newFilter("urn", rawUrn), "", QueryUtils.EMPTY_FILTER,
+    return _graphService.findRelatedEntities(ImmutableList.of(), newFilter("urn", rawUrn), ImmutableList.of(), QueryUtils.EMPTY_FILTER,


It seems like its going to become confusing to provide either null or list... one other thing we couild do to simplify is just add another method that doesn't take either source types or dest types!

addressed- made null all and empty list none

jjoyce0510

Overall looks good!

Few questions or comments about simplifying things. Otherwise looks good

gabe-lyons added 3 commits July 29, 2022 10:51

allow filtering multiple entity types in graph service

e919d48

fixing up tests

a28d572

fixing imports

d5eadfd

gabe-lyons commented Aug 1, 2022

View reviewed changes

gabe-lyons added 4 commits August 1, 2022 17:14

lint fix

9257178

another lint fix

96d8b18

fixing another lint

5b35291

Merge branch 'master' into gabe--fixingGraphServiceNotFilteringEntities

fbdc258

maggiehays added the product PR or Issue related to the DataHub UI/UX label Aug 2, 2022

jjoyce0510 reviewed Aug 3, 2022

View reviewed changes

metadata-io/src/test/java/com/linkedin/metadata/graph/GraphServiceTestBase.java Show resolved Hide resolved

jjoyce0510 reviewed Aug 3, 2022

View reviewed changes

jjoyce0510 approved these changes Aug 3, 2022

View reviewed changes

gabe-lyons added 4 commits August 10, 2022 12:44

Merge branch 'master' into gabe--fixingGraphServiceNotFilteringEntities

138a5fa

responding to comments

a2f96e8

fixing entity registry test

e37b030

always fetch yourself

63fb9e2

gabe-lyons merged commit 2e332d8 into datahub-project:master Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(graph service): only query for entities that should have lineage [Breaking Change] #5539

fix(graph service): only query for entities that should have lineage [Breaking Change] #5539

gabe-lyons commented Aug 1, 2022

gabe-lyons Aug 1, 2022

gabe-lyons Aug 1, 2022

jjoyce0510 Aug 3, 2022

github-actions bot commented Aug 1, 2022 •

edited

Loading

jjoyce0510 Aug 3, 2022

jjoyce0510 Aug 3, 2022

gabe-lyons Aug 10, 2022

jjoyce0510 Aug 3, 2022

gabe-lyons Aug 10, 2022

jjoyce0510 Aug 3, 2022

jjoyce0510 Aug 3, 2022

gabe-lyons Aug 11, 2022

jjoyce0510 Aug 3, 2022

jjoyce0510 Aug 3, 2022

gabe-lyons Aug 11, 2022

jjoyce0510 Aug 3, 2022

jjoyce0510 Aug 3, 2022

gabe-lyons Aug 11, 2022

jjoyce0510 Aug 3, 2022

gabe-lyons Aug 11, 2022

jjoyce0510 Aug 3, 2022

gabe-lyons Aug 11, 2022

jjoyce0510 Aug 3, 2022

gabe-lyons Aug 11, 2022

jjoyce0510 Aug 3, 2022

jjoyce0510 Aug 3, 2022

gabe-lyons Aug 10, 2022

jjoyce0510 Aug 11, 2022

jjoyce0510 Aug 3, 2022

gabe-lyons Aug 11, 2022

jjoyce0510 left a comment

fix(graph service): only query for entities that should have lineage [Breaking Change] #5539

fix(graph service): only query for entities that should have lineage [Breaking Change] #5539

Conversation

gabe-lyons commented Aug 1, 2022

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 1, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjoyce0510 left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 1, 2022 •

edited

Loading