feat(advanced-search): Complete Advanced Search: backend changes & tying UI together #6068

gabe-lyons · 2022-09-27T20:09:34Z

This pr accomplishes a few things:

Adds advanced search components introduced in previous PRs into the Search + Embedded Search + Impact Analysis pages.
Sends advanced search filters to the backend
Makes a breaking change to the graphql api to remove value and replace with values for FacetFilterInput graphql param. This means we will only send collections of values back for each filter. If only one value is provided, the array will have one element.
Adds backend logic to use values instead of value from facet filter input, handle different conditions, handle negation, and handle OR/AND union types.
Adds tests for new backend search logic + integration tests in cypress to apply a few different advanced search filter concepts
Adds documentation for using advanced filters.

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-09-27T20:32:41Z

Unit Test Results (build & test)

584 tests 580 ✔️ 12m 54s ⏱️
143 suites     4 💤
143 files     0 ❌

Results for commit 8eef5f4.

♻️ This comment has been updated with latest results.

github-actions · 2022-09-27T20:45:36Z

Unit Test Results (metadata ingestion)

      8 files       8 suites 59m 3s ⏱️
  719 tests   716 ✔️ 3 💤 0 ❌
1 440 runs 1 434 ✔️ 6 💤 0 ❌

Results for commit 8eef5f4.

♻️ This comment has been updated with latest results.

gabe-lyons · 2022-09-30T15:46:22Z

datahub-graphql-core/src/main/resources/search.graphql

-  Value of the field to filter by (soon to be deprecated)
-  """
-  value: String!
-


note: this is a breaking change (we are removing the value field from FacetFilterInput

gabe-lyons · 2022-09-30T15:56:48Z

datahub-web-react/src/app/search/SimpleSearchFilters.tsx

    loading: boolean;
 }

-export const SearchFilters = ({ facets, selectedFilters, onFilterSelect, loading }: Props) => {
+export const SimpleSearchFilters = ({ facets, selectedFilters, onFilterSelect, loading }: Props) => {


rename here to avoid confusion with advanced search components.

gabe-lyons · 2022-09-30T16:28:51Z

metadata-io/src/main/java/com/linkedin/metadata/search/utils/ESUtils.java

+    if (pairMatch.isPresent()) {
+      final BoolQueryBuilder orQueryBuilder = new BoolQueryBuilder();
+      String[] pairMatchValue = pairMatch.get();
+      for(String field: pairMatchValue) {
+        Criterion criterionToQuery = new Criterion();
+        criterionToQuery.setCondition(criterion.getCondition());
+        criterionToQuery.setNegated(criterion.isNegated());
+        criterionToQuery.setValue(criterion.getValue());
+        criterionToQuery.setField(field + KEYWORD_SUFFIX);
+        orQueryBuilder.should(getQueryBuilderFromCriterionForSingleField(criterionToQuery));
+      }


this logic makes sure we filter by editable and non-editable

chriscollins3456

nice! there's a good amount of code here and stuff i'm less familiar with, but I don't have anything major here by any means.

chriscollins3456 · 2022-09-30T20:27:44Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ResolverUtils.java

+        result.setValue(filter.getValues().get(0));
+        if (filter.getValues() != null) {


should you move line 133 inside of this not null check to prevent a null-pointer guy? and maybe a check that it's not an empty list?

chriscollins3456 · 2022-09-30T20:33:57Z

...graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/search/SearchResolver.java

-            "Failed to execute search: " + String.format("entity type %s, query %s, filters: %s, start: %s, count: %s",
-                input.getType(), input.getQuery(), input.getFilters(), start, count), e);
+            "Failed to execute search: " + String.format("entity type %s, query %s, filters: %s, orFilters: %s, start: %s, count: %s",
+                input.getType(), input.getQuery(), input.getFilters(), input.getOrFilters(), start, count), e);


so wherever it's getFilters that means "and" filters?

unfortunately yes. Otherwise, we'd need to make breaking changes to the graphql api which we know folks are already making programatic calls to.

I'd prefer to see us taking our GraphQL API and making it an OR of ANDs

This is nontrivial semantic model debt I think...

chriscollins3456 · 2022-09-30T20:35:01Z

datahub-graphql-core/src/main/resources/search.graphql

@@ -68,6 +68,11 @@ input SearchInput {
  Facet filters to apply to search results
  """
  filters: [FacetFilterInput!]
+
+  """
+  Filters are by default 'AND'-ed together. This lets you provide a set of OR filters


answers my question. might be nice to make note of that "and" nature on the doc string for filters above and elsewhere as well

chriscollins3456 · 2022-09-30T20:38:35Z

datahub-web-react/src/app/entity/shared/components/styled/search/EmbeddedListSearchModal.tsx

                    onChangeQuery={onChangeQuery}
                    onChangeFilters={onChangeFilters}
                    onChangePage={onChangePage}
+                    onChangeUnionType={setUnionType}


why onChangeUnionType and not just keep passing as setUnionType?

@chriscollins3456 just to keep consistency with the other function names in EmbeddedListSearch

chriscollins3456 · 2022-09-30T21:16:18Z

docs/how/search.md

+
+#### Adding an Advanced Filter
+
+Currently, Advanced Filters support filtering by Column name, Container, Domain, Description (entity or column level), Tag & Glossary Term (entity or column level), Owner, Entity Type, Subtype, Environment and soft-deleted status.


should the & between Tag & Glossary Term just be a comma?

yes good call

chriscollins3456 · 2022-09-30T21:18:20Z

...ta-io/src/main/java/com/linkedin/metadata/search/aggregator/AllEntitiesSearchAggregator.java

@@ -57,6 +61,7 @@ public AllEntitiesSearchAggregator(
    _searchRanker = Objects.requireNonNull(searchRanker);
    _cachingEntitySearchService = Objects.requireNonNull(cachingEntitySearchService);
    _entityDocCountCache = new EntityDocCountCache(entityRegistry, entitySearchService);
+    _entityRegistry = entityRegistry;


why add entityRegistry here if it's not being used? Same thing with the two imports above

what do you mean two imports above?

this comment was out of date! there were a few unused imports in this file I believe. but alas they are no longer there

chriscollins3456 · 2022-09-30T21:27:57Z

metadata-io/src/main/java/com/linkedin/metadata/search/utils/ESUtils.java

    if (condition == Condition.EQUAL) {
      // If values is set, use terms query to match one of the values
      if (!criterion.getValues().isEmpty()) {
+        if (BOOLEAN_FIELDS.contains(fieldName) && criterion.getValues().size() == 1) {


if this is a boolean field then values shouldn't be anything other than 1, right? keeping the check regardless isn't a bad idea, just wanted to check for myself.

yes correct!

chriscollins3456

looks good to me!

aditya-radhakrishnan · 2022-10-04T00:12:08Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ResolverUtils.java

@@ -81,20 +83,73 @@ public static Map<String, String> buildFacetFilters(@Nullable List<FacetFilterIn
            if (!validFacetFields.contains(facetFilterInput.getField())) {
                throw new ValidationException(String.format("Unrecognized facet with name %s provided", facetFilterInput.getField()));
            }
-            facetFilters.put(facetFilterInput.getField(), facetFilterInput.getValue());
+            facetFilters.put(facetFilterInput.getField(), facetFilterInput.getValues().get(0));


no chance this can throw an NPE right?

aditya-radhakrishnan · 2022-10-04T02:24:29Z

metadata-io/src/test/java/com/linkedin/metadata/search/SearchServiceTest.java

+
+    Urn urn = new TestEntityUrn("test", "testUrn", "VALUE_1");
+    ObjectNode document = JsonNodeFactory.instance.objectNode();
+    document.set("urn", JsonNodeFactory.instance.textNode(urn.toString()));


Nit: can we pull out some of these constants that we reuse? Also keeps things more extensible if we add more tests

aditya-radhakrishnan · 2022-10-04T02:30:43Z

docs/how/search.md

@@ -39,6 +39,41 @@ The filters sidebar sits on the left hand side of search results, and lets users
 <img width="70%"  src="https://raw.githubusercontent.com/datahub-project/static-assets/main/imgs/filters_highlighted.png" />
 </p>

+### Advanced Filters


Thank you so much for updating these docs!

of course :)

aditya-radhakrishnan · 2022-10-04T02:31:07Z

docs/how/search.md

+
+#### Adding an Advanced Filter
+
+Currently, Advanced Filters support filtering by Column name, Container, Domain, Description (entity or column level), Tag (entity or column level), Glossary Term (entity or column level), Owner, Entity Type, Subtype, Environment and soft-deleted status.


Nit: I would capitalize Column Name

aditya-radhakrishnan · 2022-10-04T02:32:24Z

smoke-test/tests/cypress/cypress/integration/search/search.js

-	  cy.contains(/of [0-9]+ results/);
-  });
+describe("search", () => {
+  // it("can hit all entities search, see some results (testing this any more is tricky because it is cached for now)", () => {


are these meant to be commented out....?

this has been reverted already :)

aditya-radhakrishnan · 2022-10-04T02:33:36Z

...rc/main/java/com/linkedin/datahub/graphql/resolvers/assertion/AssertionRunEventResolver.java

@@ -102,13 +100,16 @@ private Filter buildFilter(@Nullable FilterInput filtersInput, @Nullable final S
    }
    List<FacetFilterInput> facetFilters = new ArrayList<>();
    if (status != null) {
-      facetFilters.add(new FacetFilterInput("status", status, ImmutableList.of(status), false, SearchCondition.EQUAL));
+      FacetFilterInput filter = new FacetFilterInput();
+      filter.setField("status");


can we make this a constant?

Looks like Gabe didn't originally make this but yeah I think it should be. Either in this PR or a followup

we can do this in a followup

aditya-radhakrishnan · 2022-10-04T02:34:41Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ResolverUtils.java

+    // In the case that user sends filters to be or-d together, we need to build a series of conjective criterion
+    // arrays, rather than just one for the AND case.
+    public static ConjunctiveCriterionArray buildConjunctiveCriterionArrayWithOr(
+        final List<Criterion> andCriterions,


do we need an annotation for andCriterions too?

no longer relevant

aditya-radhakrishnan · 2022-10-04T02:38:10Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ResolverUtils.java

-    public static Filter buildFilter(@Nullable List<FacetFilterInput> facetFilterInputs) {
-        if (facetFilterInputs == null || facetFilterInputs.isEmpty()) {
+    public static Filter buildFilter(@Nullable List<FacetFilterInput> andFilters, @Nullable List<FacetFilterInput> orFilters) {
+        if ((andFilters == null || andFilters.isEmpty()) && (orFilters == null || orFilters.isEmpty())) {


can you separate these two conditions out into boolean variables? it's a little hard to read, and you can use the the same variables down below when building the andCriterions and orCriterions.

Maybe hasAndFilters and hasOrFilters?

aditya-radhakrishnan · 2022-10-04T02:58:54Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ResolverUtils.java

        });

        return facetFilters;
    }

+    // In the case that user sends filters to be or-d together, we need to build a series of conjective criterion
+    // arrays, rather than just one for the AND case.
+    public static ConjunctiveCriterionArray buildConjunctiveCriterionArrayWithOr(


nit: annotation this @Nonnull

jjoyce0510 · 2022-10-04T13:37:21Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ResolverUtils.java

        });

        return facetFilters;
    }

+    // In the case that user sends filters to be or-d together, we need to build a series of conjective criterion


nit: conjunctive criterion?

jjoyce0510 · 2022-10-04T13:39:35Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ResolverUtils.java

        });

        return facetFilters;
    }

+    // In the case that user sends filters to be or-d together, we need to build a series of conjective criterion
+    // arrays, rather than just one for the AND case.
+    public static ConjunctiveCriterionArray buildConjunctiveCriterionArrayWithOr(


Just leaving this here -> The name of this function + the parameters it takes are not immediately making it clear to me what this function does

jjoyce0510 · 2022-10-04T13:40:14Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ResolverUtils.java

+        final List<Criterion> andCriterions,
+        @Nonnull List<FacetFilterInput> orFilters
+    ) {
+        return new ConjunctiveCriterionArray(orFilters.stream().map(orFilter -> {


So we are combining the existing "and" filters with a set of OR filters? Why is this necessary?

(Will try to understand on my own just leaving my initial reactions)

jjoyce0510 · 2022-10-04T13:52:32Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ResolverUtils.java

+                andCriterionsForOr.add(criterionFromFilter(orFilter));
+
+                return new ConjunctiveCriterion().setAnd(
+                    andCriterionsForOr


So here we are doing something like this...

If you pass a list of and criterion:

[ field equals value, field2 equals value2 ]

and some set of ORs

[ field3 equals value3, field4 equals value4 ]

then we will essentially build an OR of ANDs?

OR [ [ field equals value] AND [field2 equals value2 ] AND [ field3 equals value3 ], [ field equals value] AND [field2 equals value2 ] AND [ field4 equals value4 ] ]

So we convert into an OR of ANDs

So I had to work through this in my head for some time. We should consider adding some comments or something to give an example !

jjoyce0510 · 2022-10-04T13:56:42Z

datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ResolverUtils.java

-            .map(filter -> new Criterion().setField(getFilterField(filter.getField())).setValue(filter.getValue()))
-            .collect(Collectors.toList())))));
+
+        final List<Criterion> andCriterions = andFilters != null && !andFilters.isEmpty()


This function is a bit confusing now. I was imagining we'd just have a nested OR or ANDs coming in from the FacetFilterInput. It's kinda hard to understand that if you provide both AND and OR filters that we'll combine them (and how)

resolved in my update- clearer now!

jjoyce0510 · 2022-10-04T14:04:15Z

...rc/main/java/com/linkedin/datahub/graphql/resolvers/search/SearchAcrossEntitiesResolver.java

@@ -52,7 +52,7 @@ public CompletableFuture<SearchResults> get(DataFetchingEnvironment environment)
            "Executing search for multiple entities: entity types {}, query {}, filters: {}, start: {}, count: {}",
            input.getTypes(), input.getQuery(), input.getFilters(), start, count);
        return UrnSearchResultsMapper.map(_entityClient.searchAcrossEntities(entityNames, sanitizedQuery,
-            ResolverUtils.buildFilter(input.getFilters()), start, count, ResolverUtils.getAuthentication(environment)));
+            ResolverUtils.buildFilter(input.getFilters(), input.getOrFilters()), start, count, ResolverUtils.getAuthentication(environment)));


A bit odd - input.getFilters vs input.getOrFilters -> are we saying we are just taking on this model semantics debt ?

resolved in my update

jjoyce0510 · 2022-10-04T14:06:48Z

datahub-graphql-core/src/main/resources/recommendation.graphql

+  """
+  Condition for the values. How to If unset, assumed to be equality
+  """
+  condition: SearchCondition


SearchCondition? These operators seem a bit independent of search. What if we called this

FilterOperator? or something

jjoyce0510 · 2022-10-04T14:09:09Z

datahub-graphql-core/src/main/resources/search.graphql

+  """
+  Filters are by default 'AND'-ed together. This lets you provide a set of OR filters
+  """
+  orFilters: [FacetFilterInput!]


Could we use a nested model instead of reusing FacetFilterInput?

basically:

type OrFilter { and: [FacetFilterInput!] }

This is a standard way to define filters, it's Disjunctive Normal Form

The reason i call this out is that once we commit this code it's going to become very nontrivial to change this, for the same reason its hard to change the existing semantics around AND filters..

And unsurprisingly, this is how we chose to model the Filter.pdl model already, because we were looking ahead at this.

It's shaped as

{ or: [ { and: [ { field: field, value: value } ] } ] }

gabe-lyons added 21 commits August 23, 2022 15:24

stashing progress

8310925

adding remove option

617ecbd

more progress

021e7f3

editing

9086469

further in

f1a5ba4

Merge branch 'master' into gabe--advancedSearch

f5381dc

additional rendering improvements

aac65b2

Merge branch 'master' into gabe--advancedSearch

8a2771e

stashing adv search progress

450eeda

stashing more progress

0118f8b

propagating not filters back to UI

ee60ec1

more frontend progress

2e70ad1

more filters working

8223072

getting ready for data platform selector

e6567cd

add platform select functionality

eb1e77f

Merge branch 'master' into gabe--advancedSearch

5f97685

locking out switching btwn advanced and standard filters

3c965d8

final polish

42c88cc

remove unneeded code

6dad0b0

added unit and cypress tests

7f0388a

Merge branch 'master' into gabe--advancedSearch

733811c

github-actions bot added ingestion PR or Issue related to the ingestion of metadata product PR or Issue related to the DataHub UI/UX labels Sep 27, 2022

gabe-lyons added 3 commits September 27, 2022 15:51

resolutions after merge

82ca21c

adding documentation

ffb612d

Merge branch 'master' into gabe--advancedSearch

d4c3523

gabe-lyons commented Sep 30, 2022

View reviewed changes

gabe-lyons added 2 commits September 30, 2022 09:37

cleaning up & refactoring

8ba3012

removing console.log

ee21612

gabe-lyons changed the title ~~[WIP] advanced search~~ feat(advanced-search): Complete Advanced Search: backend changes & tying UI together Sep 30, 2022

gabe-lyons added 6 commits September 30, 2022 10:05

minor ui fix & removing unneeded code

86b88ee

fixing lineage search

7694c76

fixing lints

e7d083e

fix display of degree

3c4fa4e

fixing test

ccd6240

fixing lint

6bfa481

chriscollins3456 reviewed Sep 30, 2022

View reviewed changes

gabe-lyons added 2 commits October 3, 2022 08:46

responding to comments

05568e0

Merge branch 'master' into gabe--advancedSearch

3b237e6

chriscollins3456 approved these changes Oct 3, 2022

View reviewed changes

gabe-lyons added 3 commits October 3, 2022 10:26

fixing tests

0e2f4a8

fix smoke tests

df815b8

fixing cypress

c0f8a1c

aditya-radhakrishnan reviewed Oct 4, 2022

View reviewed changes

aditya-radhakrishnan approved these changes Oct 4, 2022

View reviewed changes

fixing cypress test

4da8e38

jjoyce0510 reviewed Oct 4, 2022

View reviewed changes

responding to comments

8eef5f4

gabe-lyons merged commit ce90310 into datahub-project:master Oct 4, 2022

		result.setValue(filter.getValues().get(0));
		if (filter.getValues() != null) {


		#### Adding an Advanced Filter

		Currently, Advanced Filters support filtering by Column name, Container, Domain, Description (entity or column level), Tag & Glossary Term (entity or column level), Owner, Entity Type, Subtype, Environment and soft-deleted status.

feat(advanced-search): Complete Advanced Search: backend changes & tying UI together #6068

feat(advanced-search): Complete Advanced Search: backend changes & tying UI together #6068

Conversation

gabe-lyons commented Sep 27, 2022 • edited Loading

Checklist

github-actions bot commented Sep 27, 2022 • edited Loading

Unit Test Results (build & test)

github-actions bot commented Sep 27, 2022 • edited Loading

Unit Test Results (metadata ingestion)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscollins3456 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

chriscollins3456 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

jjoyce0510 Oct 4, 2022 • edited Loading

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 Oct 4, 2022 • edited Loading

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 Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabe-lyons commented Sep 27, 2022 •

edited

Loading

github-actions bot commented Sep 27, 2022 •

edited

Loading

github-actions bot commented Sep 27, 2022 •

edited

Loading

jjoyce0510 Oct 4, 2022 •

edited

Loading

jjoyce0510 Oct 4, 2022 •

edited

Loading

jjoyce0510 Oct 4, 2022 •

edited

Loading