Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 39 commits into from
Oct 4, 2022
Merged

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

Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
8310925
stashing progress
gabe-lyons Aug 23, 2022
617ecbd
adding remove option
gabe-lyons Aug 23, 2022
021e7f3
more progress
gabe-lyons Aug 24, 2022
9086469
editing
gabe-lyons Aug 24, 2022
f1a5ba4
further in
gabe-lyons Aug 24, 2022
f5381dc
Merge branch 'master' into gabe--advancedSearch
gabe-lyons Aug 24, 2022
aac65b2
additional rendering improvements
gabe-lyons Aug 25, 2022
8a2771e
Merge branch 'master' into gabe--advancedSearch
gabe-lyons Sep 12, 2022
450eeda
stashing adv search progress
gabe-lyons Sep 12, 2022
0118f8b
stashing more progress
gabe-lyons Sep 13, 2022
ee60ec1
propagating not filters back to UI
gabe-lyons Sep 13, 2022
2e70ad1
more frontend progress
gabe-lyons Sep 15, 2022
8223072
more filters working
gabe-lyons Sep 20, 2022
e6567cd
getting ready for data platform selector
gabe-lyons Sep 20, 2022
eb1e77f
add platform select functionality
gabe-lyons Sep 20, 2022
5f97685
Merge branch 'master' into gabe--advancedSearch
gabe-lyons Sep 20, 2022
3c965d8
locking out switching btwn advanced and standard filters
gabe-lyons Sep 21, 2022
42c88cc
final polish
gabe-lyons Sep 22, 2022
6dad0b0
remove unneeded code
gabe-lyons Sep 23, 2022
7f0388a
added unit and cypress tests
gabe-lyons Sep 26, 2022
733811c
Merge branch 'master' into gabe--advancedSearch
gabe-lyons Sep 27, 2022
82ca21c
resolutions after merge
gabe-lyons Sep 27, 2022
ffb612d
adding documentation
gabe-lyons Sep 27, 2022
d4c3523
Merge branch 'master' into gabe--advancedSearch
gabe-lyons Sep 30, 2022
8ba3012
cleaning up & refactoring
gabe-lyons Sep 30, 2022
ee21612
removing console.log
gabe-lyons Sep 30, 2022
86b88ee
minor ui fix & removing unneeded code
gabe-lyons Sep 30, 2022
7694c76
fixing lineage search
gabe-lyons Sep 30, 2022
e7d083e
fixing lints
gabe-lyons Sep 30, 2022
3c4fa4e
fix display of degree
gabe-lyons Sep 30, 2022
ccd6240
fixing test
gabe-lyons Sep 30, 2022
6bfa481
fixing lint
gabe-lyons Sep 30, 2022
05568e0
responding to comments
gabe-lyons Oct 3, 2022
3b237e6
Merge branch 'master' into gabe--advancedSearch
gabe-lyons Oct 3, 2022
0e2f4a8
fixing tests
gabe-lyons Oct 3, 2022
df815b8
fix smoke tests
gabe-lyons Oct 3, 2022
c0f8a1c
fixing cypress
gabe-lyons Oct 3, 2022
4da8e38
fixing cypress test
gabe-lyons Oct 4, 2022
8eef5f4
responding to comments
gabe-lyons Oct 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 61 additions & 6 deletions datahub-graphql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/ResolverUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
import com.datahub.authentication.Authentication;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.collect.ImmutableSet;
import com.linkedin.data.template.StringArray;
import com.linkedin.datahub.graphql.QueryContext;
import com.linkedin.datahub.graphql.exception.ValidationException;
import com.linkedin.datahub.graphql.generated.FacetFilterInput;

import com.linkedin.metadata.query.filter.Condition;
import com.linkedin.metadata.query.filter.Criterion;
import com.linkedin.metadata.query.filter.CriterionArray;
import com.linkedin.metadata.query.filter.Filter;
Expand Down Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no chance this can throw an NPE right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

});

return facetFilters;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: conjunctive criterion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

// arrays, rather than just one for the AND case.
public static ConjunctiveCriterionArray buildConjunctiveCriterionArrayWithOr(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: annotation this @Nonnull

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved!

final List<Criterion> andCriterions,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need an annotation for andCriterions too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer relevant

@Nonnull List<FacetFilterInput> orFilters
) {
return new ConjunctiveCriterionArray(orFilters.stream().map(orFilter -> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CriterionArray andCriterionsForOr = new CriterionArray(andCriterions);
andCriterionsForOr.add(criterionFromFilter(orFilter));

return new ConjunctiveCriterion().setAnd(
andCriterionsForOr
Copy link
Collaborator

@jjoyce0510 jjoyce0510 Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ] ]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we convert into an OR of ANDs

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 !

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resovled!

);
}
).collect(Collectors.toList()));
}

@Nullable
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())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

return null;
}
return new Filter().setOr(new ConjunctiveCriterionArray(new ConjunctiveCriterion().setAnd(new CriterionArray(facetFilterInputs.stream()
.map(filter -> new Criterion().setField(getFilterField(filter.getField())).setValue(filter.getValue()))
.collect(Collectors.toList())))));

final List<Criterion> andCriterions = andFilters != null && !andFilters.isEmpty()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved in my update- clearer now!

? andFilters.stream()
.map(filter -> criterionFromFilter(filter))
.collect(Collectors.toList()) : Collections.emptyList();

// if we have OR filters, we need to build a series of CriterionArrays
if (orFilters != null && !orFilters.isEmpty()) {
return new Filter().setOr(buildConjunctiveCriterionArrayWithOr(andCriterions, orFilters));
}

// if we do not have any OR filters, just set one ConjectiveCriterionArray
return new Filter().setOr(new ConjunctiveCriterionArray(new ConjunctiveCriterion().setAnd(new CriterionArray(andCriterions))));
}

// Translates a FacetFilterInput (graphql input class) into Criterion (our internal model)
public static Criterion criterionFromFilter(final FacetFilterInput filter) {
Criterion result = new Criterion();
result.setField(getFilterField(filter.getField()));
if (filter.getValues() != null) {
result.setValues(new StringArray(filter.getValues()));
if (!filter.getValues().isEmpty()) {
result.setValue(filter.getValues().get(0));
} else {
result.setValue("");
}
}

if (filter.getCondition() != null) {
result.setCondition(Condition.valueOf(filter.getCondition().toString()));
} else {
result.setCondition(Condition.EQUAL);
}

if (filter.getNegated() != null) {
result.setNegated(filter.getNegated());
}

return result;
}

private static String getFilterField(final String originalField) {
Expand Down
9 changes: 5 additions & 4 deletions ...main/java/com/linkedin/datahub/graphql/resolvers/assertion/AssertionRunEventResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,12 @@
import com.linkedin.datahub.graphql.generated.AssertionRunStatus;
import com.linkedin.datahub.graphql.generated.FacetFilterInput;
import com.linkedin.datahub.graphql.generated.FilterInput;
import com.linkedin.datahub.graphql.generated.SearchCondition;
import com.linkedin.datahub.graphql.types.dataset.mappers.AssertionRunEventMapper;
import com.linkedin.entity.client.EntityClient;
import com.linkedin.metadata.Constants;
import com.linkedin.metadata.aspect.EnvelopedAspect;
import com.linkedin.metadata.query.filter.ConjunctiveCriterion;
import com.linkedin.metadata.query.filter.ConjunctiveCriterionArray;
import com.linkedin.metadata.query.filter.Criterion;
import com.linkedin.metadata.query.filter.CriterionArray;
import com.linkedin.metadata.query.filter.Filter;
import com.linkedin.r2.RemoteInvocationException;
Expand Down Expand Up @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this a constant?

Copy link
Collaborator

@jjoyce0510 jjoyce0510 Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can do this in a followup

filter.setValues(ImmutableList.of(status));
facetFilters.add(filter);
}
if (filtersInput != null) {
facetFilters.addAll(filtersInput.getAnd());
}
return new Filter().setOr(new ConjunctiveCriterionArray(new ConjunctiveCriterion().setAnd(new CriterionArray(facetFilters.stream()
.map(filter -> new Criterion().setField(filter.getField()).setValue(filter.getValue()))
.map(filter -> criterionFromFilter(filter))
.collect(Collectors.toList())))));
}
}
5 changes: 3 additions & 2 deletions ...e/src/main/java/com/linkedin/datahub/graphql/resolvers/auth/ListAccessTokensResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.linkedin.datahub.graphql.resolvers.auth;

import com.google.common.collect.ImmutableList;
import com.linkedin.datahub.graphql.QueryContext;
import com.linkedin.datahub.graphql.authorization.AuthorizationUtils;
import com.linkedin.datahub.graphql.exception.AuthorizationException;
Expand Down Expand Up @@ -55,7 +56,7 @@ public CompletableFuture<ListAccessTokenResult> get(DataFetchingEnvironment envi
new SortCriterion().setField(EXPIRES_AT_FIELD_NAME).setOrder(SortOrder.DESCENDING);

final SearchResult searchResult = _entityClient.search(Constants.ACCESS_TOKEN_ENTITY_NAME, "",
buildFilter(filters), sortCriterion, start, count,
buildFilter(filters, Collections.emptyList()), sortCriterion, start, count,
getAuthentication(environment));

final List<AccessTokenMetadata> tokens = searchResult.getEntities().stream().map(entity -> {
Expand Down Expand Up @@ -94,6 +95,6 @@ public CompletableFuture<ListAccessTokenResult> get(DataFetchingEnvironment envi
*/
private boolean isListingSelfTokens(final List<FacetFilterInput> filters, final QueryContext context) {
return AuthorizationUtils.canGeneratePersonalAccessToken(context) && filters.stream()
.anyMatch(filter -> filter.getField().equals("ownerUrn") && filter.getValue().equals(context.getActorUrn()));
.anyMatch(filter -> filter.getField().equals("ownerUrn") && filter.getValues().equals(ImmutableList.of(context.getActorUrn())));
}
}
3 changes: 1 addition & 2 deletions ...e/src/main/java/com/linkedin/datahub/graphql/resolvers/load/TimeSeriesAspectResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import com.linkedin.metadata.authorization.PoliciesConfig;
import com.linkedin.metadata.query.filter.ConjunctiveCriterion;
import com.linkedin.metadata.query.filter.ConjunctiveCriterionArray;
import com.linkedin.metadata.query.filter.Criterion;
import com.linkedin.metadata.query.filter.CriterionArray;
import com.linkedin.metadata.query.filter.Filter;
import com.linkedin.r2.RemoteInvocationException;
Expand Down Expand Up @@ -111,7 +110,7 @@ private Filter buildFilters(@Nullable FilterInput maybeFilters) {
return null;
}
return new Filter().setOr(new ConjunctiveCriterionArray(new ConjunctiveCriterion().setAnd(new CriterionArray(maybeFilters.getAnd().stream()
.map(filter -> new Criterion().setField(filter.getField()).setValue(filter.getValue()))
.map(filter -> criterionFromFilter(filter))
.collect(Collectors.toList())))));
}
}
9 changes: 5 additions & 4 deletions ...va/com/linkedin/datahub/graphql/resolvers/recommendation/ListRecommendationsResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.linkedin.datahub.graphql.resolvers.recommendation;

import com.google.common.collect.ImmutableList;
import com.linkedin.common.urn.Urn;
import com.linkedin.datahub.graphql.generated.ContentParams;
import com.linkedin.datahub.graphql.generated.EntityProfileParams;
Expand All @@ -14,7 +15,6 @@
import com.linkedin.datahub.graphql.generated.SearchParams;
import com.linkedin.datahub.graphql.resolvers.EntityTypeMapper;
import com.linkedin.datahub.graphql.types.common.mappers.UrnToEntityMapper;
import com.linkedin.metadata.query.filter.Criterion;
import com.linkedin.metadata.query.filter.CriterionArray;
import com.linkedin.metadata.recommendation.EntityRequestContext;
import com.linkedin.metadata.recommendation.RecommendationsService;
Expand All @@ -31,7 +31,7 @@
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;

import static com.linkedin.datahub.graphql.resolvers.ResolverUtils.bindArgument;
import static com.linkedin.datahub.graphql.resolvers.ResolverUtils.*;


@Slf4j
Expand Down Expand Up @@ -88,7 +88,7 @@ private com.linkedin.metadata.recommendation.RecommendationRequestContext mapReq
searchRequestContext.setFilters(new CriterionArray(requestContext.getSearchRequestContext()
.getFilters()
.stream()
.map(facetField -> new Criterion().setField(facetField.getField()).setValue(facetField.getValue()))
.map(facetField -> criterionFromFilter(facetField))
.collect(Collectors.toList())));
}
mappedRequestContext.setSearchRequestContext(searchRequestContext);
Expand Down Expand Up @@ -148,7 +148,8 @@ private RecommendationParams mapRecommendationParams(
searchParams.setFilters(params.getSearchParams()
.getFilters()
.stream()
.map(criterion -> Filter.builder().setField(criterion.getField()).setValue(criterion.getValue()).build())
.map(criterion -> Filter.builder().setField(criterion.getField()).setValues(
ImmutableList.of(criterion.getValue())).build())
.collect(Collectors.toList()));
}
mappedParams.setSearchParams(searchParams);
Expand Down
2 changes: 1 addition & 1 deletion ...main/java/com/linkedin/datahub/graphql/resolvers/search/SearchAcrossEntitiesResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved in my update

} catch (Exception e) {
log.error(
"Failed to execute search for multiple entities: entity types {}, query {}, filters: {}, start: {}, count: {}",
Expand Down
4 changes: 2 additions & 2 deletions .../main/java/com/linkedin/datahub/graphql/resolvers/search/SearchAcrossLineageResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public CompletableFuture<SearchAcrossLineageResults> get(DataFetchingEnvironment
urn, resolvedDirection, input.getTypes(), input.getQuery(), filters, start, count);
return UrnSearchAcrossLineageResultsMapper.map(
_entityClient.searchAcrossLineage(urn, resolvedDirection, entityNames, sanitizedQuery,
maxHops, ResolverUtils.buildFilter(filters), null, start, count,
maxHops, ResolverUtils.buildFilter(filters, input.getOrFilters()), null, start, count,
ResolverUtils.getAuthentication(environment)));
} catch (RemoteInvocationException e) {
log.error(
Expand All @@ -89,7 +89,7 @@ public CompletableFuture<SearchAcrossLineageResults> get(DataFetchingEnvironment
private Integer getMaxHops(List<FacetFilterInput> filters) {
Set<String> degreeFilterValues = filters.stream()
.filter(filter -> filter.getField().equals("degree"))
.map(FacetFilterInput::getValue)
.flatMap(filter -> filter.getValues().stream())
.collect(Collectors.toSet());
Integer maxHops = null;
if (!degreeFilterValues.contains("3+")) {
Expand Down
14 changes: 7 additions & 7 deletions ...phql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/search/SearchResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,17 @@ public CompletableFuture<SearchResults> get(DataFetchingEnvironment environment)

return CompletableFuture.supplyAsync(() -> {
try {
log.debug("Executing search. entity type {}, query {}, filters: {}, start: {}, count: {}", input.getType(),
input.getQuery(), input.getFilters(), start, count);
log.debug("Executing search. entity type {}, query {}, filters: {}, orFilters: {}, start: {}, count: {}", input.getType(),
input.getQuery(), input.getFilters(), input.getOrFilters(), start, count);
return UrnSearchResultsMapper.map(
_entityClient.search(entityName, sanitizedQuery, ResolverUtils.buildFilter(input.getFilters()), null, start,
_entityClient.search(entityName, sanitizedQuery, ResolverUtils.buildFilter(input.getFilters(), input.getOrFilters()), null, start,
count, ResolverUtils.getAuthentication(environment)));
} catch (Exception e) {
log.error("Failed to execute search: entity type {}, query {}, filters: {}, start: {}, count: {}",
input.getType(), input.getQuery(), input.getFilters(), start, count);
log.error("Failed to execute search: entity type {}, query {}, filters: {}, orFilters: {}, start: {}, count: {}",
input.getType(), input.getQuery(), input.getFilters(), input.getOrFilters(), start, count);
throw new RuntimeException(
"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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nontrivial semantic model debt I think...

}
});
}
Expand Down
24 changes: 17 additions & 7 deletions datahub-graphql-core/src/main/resources/recommendation.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ type SearchParams {
"""
Entity types to be searched. If this is not provided, all entities will be searched.
"""
types: [EntityType!]
types: [EntityType!]

"""
Search query
Expand All @@ -237,12 +237,22 @@ type Filter {
"""
Name of field to filter by
"""
field: String!
field: String!

"""
Value of the field to filter by
"""
value: String!
"""
Values, one of which the intended field should match.
"""
values: [String!]!

"""
If the filter should or should not be matched
"""
negated: Boolean

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

FilterOperator? or something

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed

}

"""
Expand All @@ -269,4 +279,4 @@ type ContentParams {
Number of entities corresponding to the recommended content
"""
count: Long
}
}
39 changes: 27 additions & 12 deletions datahub-graphql-core/src/main/resources/search.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,14 @@ input SearchInput {
count: Int

"""
Facet filters to apply to search results
Facet filters to apply to search results. These will be 'AND'-ed together.
"""
filters: [FacetFilterInput!]

"""
Filters are by default 'AND'-ed together. This lets you provide a set of OR filters
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point

"""
orFilters: [FacetFilterInput!]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Collaborator

@jjoyce0510 jjoyce0510 Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 } ]
       }
    ]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated!

}

"""
Expand Down Expand Up @@ -95,9 +100,14 @@ input SearchAcrossEntitiesInput {
count: Int

"""
Faceted filters applied to search results
Facet filters to apply to search results. These will be 'AND'-ed together.
"""
filters: [FacetFilterInput!]

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

"""
Expand Down Expand Up @@ -135,9 +145,14 @@ input SearchAcrossLineageInput {
count: Int

"""
Faceted filters applied to search results
Facet filters to apply to search results. These will be 'AND'-ed together.
"""
filters: [FacetFilterInput!]

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

"""
Expand All @@ -150,22 +165,17 @@ input FacetFilterInput {
field: String!

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""
Values of the field to filter by
Values, one of which the intended field should match.
"""
values: [String!]
values: [String!]!

"""
If the filter should or should not be matched
"""
negated: Boolean

"""
Condition for the values. If unset, assumed to be equality
Condition for the values. How to If unset, assumed to be equality
"""
condition: SearchCondition
}
Expand Down Expand Up @@ -508,9 +518,14 @@ input BrowseInput {
count: Int

"""
Faceted filters applied to browse results
Faceted filters applied to browse results. These are joined together using `AND`.
"""
filters: [FacetFilterInput!]

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

"""
Expand Down
Loading