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

Jump to bottom

fix(search): Elasticsearch bool query should #11536

Merged
merged 8 commits into from
Oct 4, 2024
Merged

fix(search): Elasticsearch bool query should #11536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ea5282b
debug(ci): debug ci failure in metadata-io test
david-leifker Oct 2, 2024
d3a86dc
test if schema fields effects test
david-leifker Oct 2, 2024
8878bba
Revert "test if schema fields effects test"
david-leifker Oct 2, 2024
0bd25b9
fix test warnings
david-leifker Oct 2, 2024
7ed2d95
revert cache test
david-leifker Oct 2, 2024
b639cfb
revert rerun-tasks
david-leifker Oct 2, 2024
72aa42b
test fix - search should() handling
david-leifker Oct 3, 2024
9bca617
test updates
david-leifker Oct 3, 2024
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
22 changes: 20 additions & 2 deletions metadata-io/src/main/java/com/linkedin/metadata/graph/elastic/ESGraphQueryDAO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,9 @@ private static void addFilterToQueryBuilder(
criterion.getValues())));
orQuery.should(andQuery);
}
if (!orQuery.should().isEmpty()) {
orQuery.minimumShouldMatch(1);
}
rootQuery.filter(orQuery);
}

Expand Down Expand Up @@ -177,21 +180,26 @@ private SearchResponse executeGroupByLineageSearchQuery(
// directions for lineage
// set up filters for each relationship type in the correct direction to limit buckets
BoolQueryBuilder sourceFilterQuery = QueryBuilders.boolQuery();
sourceFilterQuery.minimumShouldMatch(1);

validEdges.stream()
.filter(pair -> RelationshipDirection.OUTGOING.equals(pair.getValue().getDirection()))
.forEach(
pair ->
sourceFilterQuery.should(
getAggregationFilter(pair, RelationshipDirection.OUTGOING)));
if (!sourceFilterQuery.should().isEmpty()) {
sourceFilterQuery.minimumShouldMatch(1);
}

BoolQueryBuilder destFilterQuery = QueryBuilders.boolQuery();
destFilterQuery.minimumShouldMatch(1);
validEdges.stream()
.filter(pair -> RelationshipDirection.INCOMING.equals(pair.getValue().getDirection()))
.forEach(
pair ->
destFilterQuery.should(getAggregationFilter(pair, RelationshipDirection.INCOMING)));
if (!destFilterQuery.should().isEmpty()) {
destFilterQuery.minimumShouldMatch(1);
}

FilterAggregationBuilder sourceRelationshipTypeFilters =
AggregationBuilders.filter(FILTER_BY_SOURCE_RELATIONSHIP, sourceFilterQuery);
Expand Down Expand Up @@ -347,6 +355,9 @@ public static BoolQueryBuilder buildQuery(
relationshipType ->
relationshipQuery.should(
QueryBuilders.termQuery(RELATIONSHIP_TYPE, relationshipType)));
if (!relationshipQuery.should().isEmpty()) {
relationshipQuery.minimumShouldMatch(1);
}
finalQuery.filter(relationshipQuery);
}

Expand Down Expand Up @@ -697,6 +708,9 @@ public static QueryBuilder getLineageQuery(
urns, edgesPerEntityType.get(entityType), graphFilters));
}
});
if (!entityTypeQueries.should().isEmpty()) {
entityTypeQueries.minimumShouldMatch(1);
}

BoolQueryBuilder finalQuery = QueryBuilders.boolQuery();

Expand Down Expand Up @@ -741,6 +755,10 @@ static QueryBuilder getLineageQueryForEntityType(
query.should(getIncomingEdgeQuery(urns, incomingEdges, graphFilters));
}

if (!query.should().isEmpty()) {
query.minimumShouldMatch(1);
}

return query;
}

Expand Down
11 changes: 7 additions & 4 deletions metadata-io/src/main/java/com/linkedin/metadata/graph/elastic/GraphFilterUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,14 @@ public static QueryBuilder getUrnStatusQuery(
if (removed) {
finalQuery.filter(QueryBuilders.termQuery(statusField, removed.toString()));
} else {
finalQuery.minimumShouldMatch(1);
finalQuery.should(QueryBuilders.termQuery(statusField, removed.toString()));
finalQuery.should(QueryBuilders.boolQuery().mustNot(QueryBuilders.existsQuery(statusField)));
}

if (!finalQuery.should().isEmpty()) {
finalQuery.minimumShouldMatch(1);
}

return finalQuery;
}

Expand Down Expand Up @@ -102,7 +105,7 @@ public static QueryBuilder getEdgeTimeFilterQuery(
* 2. The createdOn and updatedOn window does not exist on the edge at all (support legacy cases)
* 3. Special lineage case: The edge is marked as a "manual" edge, meaning that the time filters should NOT be applied.
*/
BoolQueryBuilder timeFilterQuery = QueryBuilders.boolQuery();
BoolQueryBuilder timeFilterQuery = QueryBuilders.boolQuery().minimumShouldMatch(1);
timeFilterQuery.should(buildTimeWindowFilter(startTimeMillis, endTimeMillis));
timeFilterQuery.should(buildTimestampsMissingFilter());
timeFilterQuery.should(buildManualLineageFilter());
Expand Down Expand Up @@ -158,7 +161,7 @@ public static QueryBuilder getEdgeTimeFilterQuery(
*/
private static QueryBuilder buildTimeWindowFilter(
final long startTimeMillis, final long endTimeMillis) {
final BoolQueryBuilder timeWindowQuery = QueryBuilders.boolQuery();
final BoolQueryBuilder timeWindowQuery = QueryBuilders.boolQuery().minimumShouldMatch(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be helpful from a clarity perspective to colocate this with the should calls below, seems odd to have that pattern for the other two, but not this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's because we always populate the should, no potential cases where it might be empty.


/*
* To perform comparison:
Expand Down Expand Up @@ -198,7 +201,7 @@ private static QueryBuilder buildTimestampsMissingFilter() {

private static QueryBuilder buildNotExistsFilter(String fieldName) {
// This filter returns 'true' if the field DOES NOT EXIST or it exists but is equal to 0.
final BoolQueryBuilder notExistsFilter = QueryBuilders.boolQuery();
final BoolQueryBuilder notExistsFilter = QueryBuilders.boolQuery().minimumShouldMatch(1);
notExistsFilter.should(QueryBuilders.boolQuery().mustNot(QueryBuilders.existsQuery(fieldName)));
notExistsFilter.should(QueryBuilders.boolQuery().must(QueryBuilders.termQuery(fieldName, 0L)));
return notExistsFilter;
Expand Down
5 changes: 4 additions & 1 deletion ...java/com/linkedin/metadata/search/elasticsearch/query/filter/BaseQueryFilterRewriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,12 @@ private BoolQueryBuilder handleNestedFilters(
mustNotQueryBuilders.forEach(expandedQueryBuilder::mustNot);
expandedQueryBuilder.queryName(boolQueryBuilder.queryName());
expandedQueryBuilder.adjustPureNegative(boolQueryBuilder.adjustPureNegative());
expandedQueryBuilder.minimumShouldMatch(boolQueryBuilder.minimumShouldMatch());
expandedQueryBuilder.boost(boolQueryBuilder.boost());

if (!expandedQueryBuilder.should().isEmpty()) {
expandedQueryBuilder.minimumShouldMatch(1);
}

return expandedQueryBuilder;
}

Expand Down
13 changes: 7 additions & 6 deletions .../com/linkedin/metadata/search/elasticsearch/query/request/AutocompleteRequestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,7 @@ public SearchRequest getSearchRequest(
QueryConfiguration customQueryConfig =
customizedQueryHandler.lookupQueryConfig(input).orElse(null);

BoolQueryBuilder baseQuery = QueryBuilders.boolQuery();
baseQuery.minimumShouldMatch(1);
BoolQueryBuilder baseQuery = QueryBuilders.boolQuery().minimumShouldMatch(1);

// Initial query with input filters
BoolQueryBuilder filterQuery =
Expand Down Expand Up @@ -176,12 +175,15 @@ public BoolQueryBuilder getQuery(
BoolQueryBuilder finalQuery =
Optional.ofNullable(customAutocompleteConfig)
.flatMap(cac -> CustomizedQueryHandler.boolQueryBuilder(objectMapper, cac, query))
.orElse(QueryBuilders.boolQuery())
.minimumShouldMatch(1);
.orElse(QueryBuilders.boolQuery());

getAutocompleteQuery(customAutocompleteConfig, autocompleteFields, query)
.ifPresent(finalQuery::should);

if (!finalQuery.should().isEmpty()) {
finalQuery.minimumShouldMatch(1);
}

return finalQuery;
}

Expand All @@ -200,8 +202,7 @@ private Optional<QueryBuilder> getAutocompleteQuery(

private static BoolQueryBuilder defaultQuery(
List<String> autocompleteFields, @Nonnull String query) {
BoolQueryBuilder finalQuery = QueryBuilders.boolQuery();
finalQuery.minimumShouldMatch(1);
BoolQueryBuilder finalQuery = QueryBuilders.boolQuery().minimumShouldMatch(1);

// Search for exact matches with higher boost and ngram matches
MultiMatchQueryBuilder autocompleteQueryBuilder =
Expand Down
15 changes: 12 additions & 3 deletions ...ain/java/com/linkedin/metadata/search/elasticsearch/query/request/SearchQueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,7 @@ private QueryBuilder buildInternalQuery(
cqc ->
CustomizedQueryHandler.boolQueryBuilder(
opContext.getObjectMapper(), cqc, sanitizedQuery))
.orElse(QueryBuilders.boolQuery())
.minimumShouldMatch(1);
.orElse(QueryBuilders.boolQuery());

if (fulltext && !query.startsWith(STRUCTURED_QUERY_PREFIX)) {
getSimpleQuery(opContext.getEntityRegistry(), customQueryConfig, entitySpecs, sanitizedQuery)
Expand Down Expand Up @@ -135,6 +134,10 @@ private QueryBuilder buildInternalQuery(
}
}

if (!finalQuery.should().isEmpty()) {
finalQuery.minimumShouldMatch(1);
}

return finalQuery;
}

Expand Down Expand Up @@ -368,6 +371,10 @@ private Optional<QueryBuilder> getSimpleQuery(
simplePerField.should(simpleBuilder);
});

if (!simplePerField.should().isEmpty()) {
simplePerField.minimumShouldMatch(1);
}

result = Optional.of(simplePerField);
}

Expand Down Expand Up @@ -454,7 +461,9 @@ private Optional<QueryBuilder> getPrefixAndExactMatchQuery(
}
});

return finalQuery.should().size() > 0 ? Optional.of(finalQuery) : Optional.empty();
return finalQuery.should().size() > 0
? Optional.of(finalQuery.minimumShouldMatch(1))
: Optional.empty();
}

private Optional<QueryBuilder> getStructuredQuery(
Expand Down
9 changes: 4 additions & 5 deletions metadata-io/src/main/java/com/linkedin/metadata/search/utils/ESUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,6 @@ public static BoolQueryBuilder buildFilterQuery(
searchableFieldTypes,
opContext,
queryFilterRewriteChain)));
// The default is not always 1 (ensure consistent default)
finalQueryBuilder.minimumShouldMatch(1);
} else if (filter.getCriteria() != null) {
// Otherwise, build boolean query from the deprecated "criteria" field.
log.warn("Received query Filter with a deprecated field 'criteria'. Use 'or' instead.");
Expand All @@ -187,7 +185,8 @@ public static BoolQueryBuilder buildFilterQuery(
}
});
finalQueryBuilder.should(andQueryBuilder);
// The default is not always 1 (ensure consistent default)
}
if (!finalQueryBuilder.should().isEmpty()) {
finalQueryBuilder.minimumShouldMatch(1);
}
return finalQueryBuilder;
Expand Down Expand Up @@ -533,7 +532,7 @@ private static QueryBuilder getQueryBuilderFromCriterionForFieldToExpand(
final Map<String, Set<SearchableAnnotation.FieldType>> searchableFieldTypes,
@Nonnull OperationContext opContext,
@Nonnull QueryFilterRewriteChain queryFilterRewriteChain) {
final BoolQueryBuilder orQueryBuilder = new BoolQueryBuilder();
final BoolQueryBuilder orQueryBuilder = new BoolQueryBuilder().minimumShouldMatch(1);
for (String field : fields) {
orQueryBuilder.should(
getQueryBuilderFromCriterionForSingleField(
Expand Down Expand Up @@ -619,7 +618,7 @@ private static QueryBuilder buildWildcardQueryWithMultipleValues(
@Nullable String queryName,
@Nonnull AspectRetriever aspectRetriever,
String wildcardPattern) {
BoolQueryBuilder boolQuery = QueryBuilders.boolQuery();
BoolQueryBuilder boolQuery = QueryBuilders.boolQuery().minimumShouldMatch(1);

for (String value : criterion.getValues()) {
boolQuery.should(
Expand Down
1 change: 0 additions & 1 deletion metadata-io/src/test/java/com/linkedin/metadata/search/fixtures/GoldenTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import static com.linkedin.metadata.utils.CriterionUtils.buildCriterion;
import static io.datahubproject.test.search.SearchTestUtils.searchAcrossEntities;
import static org.testng.Assert.*;
import static org.testng.AssertJUnit.assertNotNull;

import com.google.common.collect.ImmutableList;
import com.linkedin.common.urn.Urn;
Expand Down
11 changes: 8 additions & 3 deletions ...c/test/java/com/linkedin/metadata/search/query/filter/ContainerExpansionRewriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,16 +311,19 @@ public void testNestedBoolQueryRewrite() {
new RelatedEntities(
"IsPartOf", childUrn, parentUrn, RelationshipDirection.OUTGOING, null))));

BoolQueryBuilder testQuery = QueryBuilders.boolQuery();
BoolQueryBuilder testQuery = QueryBuilders.boolQuery().minimumShouldMatch(1);
testQuery.filter(
QueryBuilders.boolQuery()
.filter(
QueryBuilders.boolQuery().filter(QueryBuilders.termsQuery(FIELD_NAME, childUrn))));
testQuery.filter(QueryBuilders.existsQuery("someField"));
testQuery.should(
QueryBuilders.boolQuery()
.minimumShouldMatch(1)
.should(
QueryBuilders.boolQuery().should(QueryBuilders.termsQuery(FIELD_NAME, childUrn))));
QueryBuilders.boolQuery()
.minimumShouldMatch(1)
.should(QueryBuilders.termsQuery(FIELD_NAME, childUrn))));
testQuery.should(QueryBuilders.existsQuery("someField"));
testQuery.must(
QueryBuilders.boolQuery()
Expand All @@ -332,7 +335,7 @@ public void testNestedBoolQueryRewrite() {
QueryBuilders.boolQuery().mustNot(QueryBuilders.termsQuery(FIELD_NAME, childUrn))));
testQuery.mustNot(QueryBuilders.existsQuery("someField"));

BoolQueryBuilder expectedRewrite = QueryBuilders.boolQuery();
BoolQueryBuilder expectedRewrite = QueryBuilders.boolQuery().minimumShouldMatch(1);
expectedRewrite.filter(
QueryBuilders.boolQuery()
.filter(
Expand All @@ -341,8 +344,10 @@ public void testNestedBoolQueryRewrite() {
expectedRewrite.filter(QueryBuilders.existsQuery("someField"));
expectedRewrite.should(
QueryBuilders.boolQuery()
.minimumShouldMatch(1)
.should(
QueryBuilders.boolQuery()
.minimumShouldMatch(1)
.should(QueryBuilders.termsQuery(FIELD_NAME, childUrn, parentUrn))));
expectedRewrite.should(QueryBuilders.existsQuery("someField"));
expectedRewrite.must(
Expand Down
20 changes: 15 additions & 5 deletions .../src/test/java/com/linkedin/metadata/search/query/filter/DomainExpansionRewriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,17 +312,23 @@ public void testNestedBoolQueryRewrite() {
new RelatedEntities(
"IsPartOf", childUrn, parentUrn, RelationshipDirection.INCOMING, null))));

BoolQueryBuilder testQuery = QueryBuilders.boolQuery();
BoolQueryBuilder testQuery = QueryBuilders.boolQuery().minimumShouldMatch(1);
testQuery.filter(
QueryBuilders.boolQuery()
.filter(
QueryBuilders.boolQuery().filter(QueryBuilders.termsQuery(FIELD_NAME, parentUrn))));
testQuery.filter(QueryBuilders.boolQuery().filter(QueryBuilders.existsQuery("someField")));
testQuery.should(
QueryBuilders.boolQuery()
.minimumShouldMatch(1)
.should(
QueryBuilders.boolQuery().should(QueryBuilders.termsQuery(FIELD_NAME, parentUrn))));
testQuery.should(QueryBuilders.boolQuery().should(QueryBuilders.existsQuery("someField")));
QueryBuilders.boolQuery()
.minimumShouldMatch(1)
.should(QueryBuilders.termsQuery(FIELD_NAME, parentUrn))));
testQuery.should(
QueryBuilders.boolQuery()
.minimumShouldMatch(1)
.should(QueryBuilders.existsQuery("someField")));
testQuery.must(
QueryBuilders.boolQuery()
.must(QueryBuilders.boolQuery().must(QueryBuilders.termsQuery(FIELD_NAME, parentUrn))));
Expand All @@ -334,7 +340,7 @@ public void testNestedBoolQueryRewrite() {
.mustNot(QueryBuilders.termsQuery(FIELD_NAME, parentUrn))));
testQuery.mustNot(QueryBuilders.boolQuery().mustNot(QueryBuilders.existsQuery("someField")));

BoolQueryBuilder expectedRewrite = QueryBuilders.boolQuery();
BoolQueryBuilder expectedRewrite = QueryBuilders.boolQuery().minimumShouldMatch(1);
expectedRewrite.filter(
QueryBuilders.boolQuery()
.filter(
Expand All @@ -344,11 +350,15 @@ public void testNestedBoolQueryRewrite() {
QueryBuilders.boolQuery().filter(QueryBuilders.existsQuery("someField")));
expectedRewrite.should(
QueryBuilders.boolQuery()
.minimumShouldMatch(1)
.should(
QueryBuilders.boolQuery()
.minimumShouldMatch(1)
.should(QueryBuilders.termsQuery(FIELD_NAME, childUrn, parentUrn))));
expectedRewrite.should(
QueryBuilders.boolQuery().should(QueryBuilders.existsQuery("someField")));
QueryBuilders.boolQuery()
.minimumShouldMatch(1)
.should(QueryBuilders.existsQuery("someField")));
expectedRewrite.must(
QueryBuilders.boolQuery()
.must(
Expand Down
Loading
Loading