diff --git a/src/nameres/handlers/README.md b/src/nameres/handlers/README.md index 36c9075..3d70277 100644 --- a/src/nameres/handlers/README.md +++ b/src/nameres/handlers/README.md @@ -144,295 +144,128 @@ def _sanitize_lookup_query(self, lookup_strings: list[str]) -> list[tuple[str]]: ##### filters -We have 4 different filters we have to apply to our query depending on what the user -supplies +Four optional filter categories can constrain a lookup: -1) biolink-type -If the user supplies a biolink-type or a collection of biolink-types, we have to apply a filter -to the search to only include results which match the specification. The query by itself is a simple -`term` based filter within a `should` clause for each biolink-type specified +- `biolink_type` and `biolink_types` produce exact `term` queries on `biolink_types`. +- `only_prefixes` produces `prefix` queries on `curie`. +- `only_taxa` produces exact `term` queries on `taxa`. +- `exclude_prefixes` produces `prefix` queries under `must_not`. -```JSON -{ - "should": [ - { - "term": {"biolink_types": } - }, - { - "term": {"biolink_types": } - }, - ... - { - "term": {"biolink_types": } - }, - ] -} - -2) only-prefixes -Same as case 1, but in this case looking for filtering by specified CURIE prefix. Still leveraged -in a `should` clause, but leverages `prefix` instead of `term` +Values within each positive category are combined with OR. The positive categories themselves are +separate entries under `bool.filter`, so supplying more than one category combines them with AND. +Excluded prefixes are placed under `must_not`. These clauses run in filter context and therefore do +not change the text relevance score. For example: ```JSON { - "should": [ + "filter": [ { - "prefix": {"curie": } + "bool": { + "should": [ + {"term": {"biolink_types": "Disease"}}, + {"term": {"biolink_types": "PhenotypicFeature"}} + ], + "minimum_should_match": 1 + } }, { - "prefix": {"curie": } + "bool": { + "should": [ + {"prefix": {"curie": "MONDO"}}, + {"prefix": {"curie": "HP"}} + ], + "minimum_should_match": 1 + } }, - ... { - "prefix": {"curie": } - }, - ] -} - -3) exclude-prefixes -The inversion of case 2, this filters curie prefixes that we don't want included in the final -results. Leverages a `must_not` clause with the `prefix` query - -```JSON -{ + "bool": { + "should": [ + {"term": {"taxa": "NCBITaxon:9606"}} + ], + "minimum_should_match": 1 + } + } + ], "must_not": [ - { - "prefix": {"curie": } - }, - { - "prefix": {"curie": } - }, - ... - { - "prefix": {"curie": } - }, + {"prefix": {"curie": "UMLS"}} ] } ``` -4) only-taxa -Same as case 1, but in this case looking for filtering by specified taxon. Still leveraged -in a `should` clause, along with the same `term` query - -```JSON -{ - "should": [ - { - "term": {"taxa": } - }, - { - "term": {"taxa": } - }, - ... - { - "term": {"taxa": } - }, - ] -} -``` - -This is different from solr, but only syntatically. We have to include these filters within -the main query in elasticsearch, whereas solr provides a filter in the query that includes -boolean logic combining the field:value pairs in a similar fashion to our `should` and `must_not` -clauses above +Empty categories are omitted. See `_build_lookup_filters` in `lookup.py` for the request parsing and +query construction. -```python -def _build_lookup_filters(self) -> dict: - """Handles the parsing and building of various elasticsearch boolean logic queries. - - We have two types of boolean logic queries we need to build for this endpoint - - 1) should - In this case we want to boolean OR specific different types of required - fields we want in the results output - - 2) must_not - In this case we to boolean AND NOT specific different types of required - fields we want to ensure `don't` exist in the results output - """ - biolink_types = self.get_argument("biolink_types", default=[], strip=True) - - filter_delimiter = "|" - - only_prefixes = self.get_argument("only_prefixes", default="", strip=True) - only_prefixes = only_prefixes.split(filter_delimiter) - try: - only_prefixes.remove("") - except ValueError: - pass - - exclude_prefixes = self.get_argument("exclude_prefixes", default="", strip=True) - exclude_prefixes = exclude_prefixes.split(filter_delimiter) - try: - exclude_prefixes.remove("") - except ValueError: - pass - - only_taxa = self.get_argument("only_taxa", default="", strip=True) - only_taxa = only_taxa.split(filter_delimiter) - try: - only_taxa.remove("") - except ValueError: - pass - - # Apply filters as needed. - filters = {"should": [], "must_not": []} - - # Biolink type filter - # Elasticsearch should - for biolink_type in biolink_types: - biolink_type = biolink_type.strip() - if biolink_type is not None: - should_filter = {"term": {"biolink_types": biolink_type.remove("biolink:")}} - filters["should"].append(should_filter) - - # Prefix: only filter - # Elasticsearch should + Match boolean prefix query - for prefix in only_prefixes: - prefix = prefix.strip() - should_filter = {"prefix": {"curie": prefix}} - filters["should"].append(should_filter) - - # Prefix: exclude filter - # Elasticsearch must not - for prefix in exclude_prefixes: - prefix = prefix.strip() - must_not_filter = {"prefix": {"curie": prefix}} - filters["must_not"].append(must_not_filter) - - # Taxa filter. - # only_taxa is like: 'NCBITaxon:9606|NCBITaxon:10090|NCBITaxon:10116|NCBITaxon:7955' - # Elasticsearch should - for taxon in only_taxa: - taxon = taxon.strip() - should_filter = {"term": {"taxa": taxon}} - filters["should"].append(should_filter) - - # We also need to include entries that don't have taxa specified. - # TODO Skipping for the moment as we need to update the index - # filters["should"].append({ "term" : { "taxon_specific" : False } } - - return filters -``` - - -##### build elasticsearch query +##### build Elasticsearch query -So this query is fairly complicated because we have a lot of specifications we want to achieve from -our lookup. The overall structure of the query is the following: +The text-matching `dis_max` is the only scoring clause in the inner `bool` query. Positive filter +categories and excluded prefixes are attached to `filter` and `must_not`, respectively, so they +constrain matches without contributing to the text score. A top-level `function_score` then applies +the same logarithmic clique-size multiplier used by the Solr implementation. ```JSON { - "bool": { - "must": [ - { - "dis_max": { - "queries": [ - { - "multi_match": { - "query": lookup_string0, - "type": "best_fields", - "fields": ["preferred_name^25", "name^10"], - } - }, - { - "multi_match": { - "query": lookup_string1, - "type": "best_fields", - "fields": ["preferred_name^25", "name^10"], - } - }, - - # autocomplete queries - - { - "multi_match": { - "query": lookup_string0, - "type": "phrase", - "fields": ["preferred_name^30", "name^20"], - } - }, - { - "multi_match": { - "query": lookup_string1, - "type": "phrase", - "fields": ["preferred_name^30", "name^20"], - } + "function_score": { + "query": { + "bool": { + "must": [ + { + "dis_max": { + "queries": [ + { + "multi_match": { + "query": "", + "type": "best_fields", + "fields": ["preferred_name^25", "names^10"] + } + }, + { + "multi_match": { + "query": "", + "type": "phrase_prefix", + "fields": ["preferred_name^30", "names^20"] + } + } + ] } - ] - } - }, - { - "should":[] + } + ], + "filter": [ + { + "bool": { + "should": [ + {"term": {"biolink_types": "Disease"}} + ], + "minimum_should_match": 1 + } + } + ], + "must_not": [ + {"prefix": {"curie": "UMLS"}} + ] } - ] - }, - "must_not": [] + }, + "field_value_factor": { + "field": "clique_identifier_count", + "modifier": "log1p", + "missing": 0 + }, + "boost_mode": "multiply" + } } ``` -The `dis_max` (disjunction maximization) filter in this case will return documents that match one of -more of the provided queries. If multiple match than it selects amongest the highest relevance -scoring with tie breaking capabilities based off additional submatching. The original solr index -leveraged a more advanced version called the extended disjunction max query that is specific to -solr. Elasticsearch doesn't currently implement this version so we leverage the standard `dis_max`. -From the string search santization we break each query into a separate `multi_match`. This is also -how we incorporate the autocomplete version, as we also extend additional queries to leverage -`phrase` based matches compared to the standard of `best_fields` - - - -```python - -# elasticsearch query -def _build_elasticsearch_query(lookup_query: list[LookupQuery], filters: dict) -> dict: - queries = [] - - # Base Query - for lookup_string in lookup_query.string: - queries.append( - { - "multi_match": { - "query": lookup_string, - "type": "best_fields", - "fields": ["preferred_name^25", "name^10"], - } - } - ) - - # https://www.elastic.co/search-labs/blog/elasticsearch-autocomplete-search#2.-query-time - if lookup_query.autocomplete: - for lookup_string in lookup_query.string: - queries.append( - { - "multi_match": { - "query": lookup_string, - "type": "phrase", - "fields": ["preferred_name^30", "name^20"], - } - } - ) +One `best_fields` query is generated for every sanitized lookup string. The `phrase_prefix` queries +are included only for autocomplete requests. `log1p` is the common logarithm after adding one, so +the final score is: - compound_lookup_query = { - "bool": { - "must": [ - { - "dis_max": { - "queries": queries, - } - } - ] - } - } - if len(filters["should"]) > 0: - compound_lookup_query["bool"]["must"].append({"bool": {"should": [*filters["should"]]}}) +`text score * log10(clique_identifier_count + 1)` - if len(filters["must_not"]) > 0: - compound_lookup_query["bool"]["must_not"] = [*filters["must_not"]] +See `_build_elasticsearch_query` in `lookup.py` for the authoritative implementation. - return compound_lookup_query +For comparison, the Solr implementation applies the same multiplier: -... +```python # solr query if highlighting: @@ -478,7 +311,6 @@ params = { ##### Future Work and Optimizations * Future Work - * Need to figure out how incorporate boosting leveraging the `clique_identifier_count` * Add the `taxon_specific` field to the index. I missed this when looking through the solr schema. Only used in taxon filtering at the moment * We have a difference in the index as they created custom field types that duplicate the diff --git a/src/nameres/handlers/lookup.py b/src/nameres/handlers/lookup.py index a66624c..c627ce2 100644 --- a/src/nameres/handlers/lookup.py +++ b/src/nameres/handlers/lookup.py @@ -251,17 +251,11 @@ def _sanitize_lookup_query(self, lookup_strings: list[str]) -> list[tuple[str, t return sanitized_lookup_strings def _build_lookup_filters(self) -> dict: - """Handles the parsing and building of various elasticsearch boolean logic queries. + """Build non-scoring Elasticsearch filters for lookup requests. - We have two types of boolean logic queries we need to build for this endpoint - - 1) should - In this case we want to boolean OR specific different types of required - fields we want in the results output - - 2) must_not - In this case we to boolean AND NOT specific different types of required - fields we want to ensure `don't` exist in the results output + Values within a positive filter category are combined with OR, while + separate categories are combined with AND. Excluded prefixes are + represented as ``must_not`` clauses. """ # to cover both the singular and plural biolink_type arguments, we combine them into a single list @@ -291,43 +285,30 @@ def _build_lookup_filters(self) -> dict: pass # Apply filters as needed. - filters = {"should": [], "must_not": []} - - # Biolink type filter - # Elasticsearch should - for biolink_type in biolink_types: - biolink_type = biolink_type.strip() - if biolink_type: - should_filter = {"term": {"biolink_types": biolink_type.removeprefix("biolink:")}} - filters["should"].append(should_filter) - - # Prefix: only filter - # Elasticsearch should + Match boolean prefix query - for prefix in only_prefixes: - prefix = prefix.strip() - should_filter = {"prefix": {"curie": prefix}} - filters["should"].append(should_filter) + es_filters = {"filter": [], "must_not": []} + + # OR-relationship within each group, chained with AND-relationship between groups. + for values, build in [ + (biolink_types, lambda v: {"term": {"biolink_types": v.removeprefix("biolink:")}}), + (only_prefixes, lambda v: {"prefix": {"curie": v}}), + (only_taxa, lambda v: {"term": {"taxa": v}}), + ]: + should_filters = [build(s) for v in values if (s := v.strip())] + if should_filters: + es_filters["filter"].append({"bool": {"should": should_filters, "minimum_should_match": 1}}) # Prefix: exclude filter # Elasticsearch must not for prefix in exclude_prefixes: - prefix = prefix.strip() - must_not_filter = {"prefix": {"curie": prefix}} - filters["must_not"].append(must_not_filter) - - # Taxa filter. - # only_taxa is like: 'NCBITaxon:9606|NCBITaxon:10090|NCBITaxon:10116|NCBITaxon:7955' - # Elasticsearch should - for taxon in only_taxa: - taxon = taxon.strip() - should_filter = {"term": {"taxa": taxon}} - filters["should"].append(should_filter) + if prefix := prefix.strip(): + must_not_filter = {"prefix": {"curie": prefix}} + es_filters["must_not"].append(must_not_filter) # We also need to include entries that don't have taxa specified. # TODO Skipping for the moment as we need to update the index - # filters["should"].append({ "term" : { "taxon_specific" : False } } + # { "term" : { "taxon_specific" : False } } - return filters + return es_filters class NameResolutionLookupHandler(BaseNameResolutionLookupHandler): @@ -488,10 +469,20 @@ def _build_elasticsearch_query(lookup_query: LookupQuery, filters: dict) -> dict ] } } - if len(filters["should"]) > 0: - compound_lookup_query["bool"]["must"].append({"bool": {"should": [*filters["should"]]}}) - - if len(filters["must_not"]) > 0: - compound_lookup_query["bool"]["must_not"] = [*filters["must_not"]] - - return compound_lookup_query + # Keep constraints in filter context so they do not affect name-match scores. + for key in ["filter", "must_not"]: + if len(filters[key]) > 0: + compound_lookup_query["bool"].setdefault(key, []).extend(filters[key]) + + # Match Solr's multiplicative log(sum(clique_identifier_count, 1)) boost. + return { + "function_score": { + "query": compound_lookup_query, + "field_value_factor": { + "field": "clique_identifier_count", + "modifier": "log1p", + "missing": 0, + }, + "boost_mode": "multiply", + } + } diff --git a/test/test_lookup_query.py b/test/test_lookup_query.py index 86de52e..73393d0 100644 --- a/test/test_lookup_query.py +++ b/test/test_lookup_query.py @@ -3,6 +3,10 @@ from nameres.handlers.lookup import BaseNameResolutionLookupHandler, LookupQuery, _build_elasticsearch_query +def _text_query(query: dict) -> dict: + return query["function_score"]["query"] + + def test_biolink_type_filters_accept_singular_and_plural_arguments(): handler = Mock() query_arguments = { @@ -15,15 +19,45 @@ def test_biolink_type_filters_accept_singular_and_plural_arguments(): filters = BaseNameResolutionLookupHandler._build_lookup_filters(handler) assert filters == { - "should": [ - {"term": {"biolink_types": "Disease"}}, - {"term": {"biolink_types": "Gene"}}, - {"term": {"biolink_types": "PhenotypicFeature"}}, + "filter": [ + { + "bool": { + "should": [ + {"term": {"biolink_types": "Disease"}}, + {"term": {"biolink_types": "Gene"}}, + {"term": {"biolink_types": "PhenotypicFeature"}}, + ], + "minimum_should_match": 1, + } + } ], "must_not": [], } +def test_clique_identifier_count_multiplies_text_score(): + lookup_query = LookupQuery( + raw_string="insulin", + query_strings=("insulin",), + autocomplete=False, + highlighting=False, + offset=0, + limit=10, + ) + + query = _build_elasticsearch_query(lookup_query, {"filter": [], "must_not": []}) + + assert set(query) == {"function_score"} + function_score = query["function_score"] + assert set(function_score) == {"query", "field_value_factor", "boost_mode"} + assert function_score["field_value_factor"] == { + "field": "clique_identifier_count", + "modifier": "log1p", + "missing": 0, + } + assert function_score["boost_mode"] == "multiply" + + def test_autocomplete_query_treats_final_term_as_prefix(): lookup_query = LookupQuery( raw_string="diabe", @@ -34,7 +68,7 @@ def test_autocomplete_query_treats_final_term_as_prefix(): limit=10, ) - query = _build_elasticsearch_query(lookup_query, {"should": [], "must_not": []}) + query = _text_query(_build_elasticsearch_query(lookup_query, {"filter": [], "must_not": []})) dis_max_queries = query["bool"]["must"][0]["dis_max"]["queries"] assert dis_max_queries == [ @@ -65,7 +99,7 @@ def test_non_autocomplete_query_does_not_add_prefix_match(): limit=10, ) - query = _build_elasticsearch_query(lookup_query, {"should": [], "must_not": []}) + query = _text_query(_build_elasticsearch_query(lookup_query, {"filter": [], "must_not": []})) assert query["bool"]["must"][0]["dis_max"]["queries"] == [ { @@ -76,3 +110,142 @@ def test_non_autocomplete_query_does_not_add_prefix_match(): } } ] + + +def test_each_filter_category_becomes_its_own_group(): + handler = Mock() + query_arguments = {"biolink_type": ["biolink:Disease"]} + string_arguments = {"only_prefixes": "MONDO| |HP", "only_taxa": " NCBITaxon:9606 "} + handler.get_arguments.side_effect = lambda name: query_arguments.get(name, []) + handler.get_argument.side_effect = lambda name, default, strip: string_arguments.get(name, default) + + filters = BaseNameResolutionLookupHandler._build_lookup_filters(handler) + + # One group per supplied category. Clauses are OR'd within a group; the groups + # themselves are AND'd against each other by _build_elasticsearch_query. + assert filters == { + "filter": [ + { + "bool": { + "should": [{"term": {"biolink_types": "Disease"}}], + "minimum_should_match": 1, + } + }, + { + "bool": { + "should": [{"prefix": {"curie": "MONDO"}}, {"prefix": {"curie": "HP"}}], + "minimum_should_match": 1, + } + }, + { + "bool": { + "should": [{"term": {"taxa": "NCBITaxon:9606"}}], + "minimum_should_match": 1, + } + }, + ], + "must_not": [], + } + + +def test_omitted_filter_categories_contribute_no_group(): + handler = Mock() + handler.get_arguments.side_effect = lambda _name: [] + handler.get_argument.side_effect = lambda _name, default, strip: default + + filters = BaseNameResolutionLookupHandler._build_lookup_filters(handler) + + assert filters == {"filter": [], "must_not": []} + + +def test_blank_excluded_prefixes_are_ignored(): + handler = Mock() + handler.get_arguments.side_effect = lambda _name: [] + handler.get_argument.side_effect = lambda name, default, strip: ( + " | UMLS | | " if name == "exclude_prefixes" else default + ) + + filters = BaseNameResolutionLookupHandler._build_lookup_filters(handler) + + assert filters == { + "filter": [], + "must_not": [{"prefix": {"curie": "UMLS"}}], + } + + +def test_filter_groups_are_added_without_replacing_the_search_query(): + lookup_query = LookupQuery( + raw_string="insulin", + query_strings=("insulin",), + autocomplete=False, + highlighting=False, + offset=0, + limit=10, + ) + filters = { + "filter": [ + { + "bool": { + "should": [{"term": {"biolink_types": "Disease"}}], + "minimum_should_match": 1, + } + }, + { + "bool": { + "should": [{"prefix": {"curie": "MONDO"}}, {"prefix": {"curie": "HP"}}], + "minimum_should_match": 1, + } + }, + ], + "must_not": [{"prefix": {"curie": "UMLS"}}], + } + + query = _text_query(_build_elasticsearch_query(lookup_query, filters)) + + # The name-matching dis_max remains the only scoring must clause. + assert len(query["bool"]["must"]) == 1 + assert "dis_max" in query["bool"]["must"][0] + # Each category stays a separate required bool under non-scoring filter context. + assert query["bool"]["filter"] == filters["filter"] + assert query["bool"]["must_not"] == [{"prefix": {"curie": "UMLS"}}] + + +def test_exclude_prefixes_populate_must_not_without_any_filter_group(): + lookup_query = LookupQuery( + raw_string="insulin", + query_strings=("insulin",), + autocomplete=False, + highlighting=False, + offset=0, + limit=10, + ) + + query = _text_query( + _build_elasticsearch_query( + lookup_query, + {"filter": [], "must_not": [{"prefix": {"curie": "UMLS"}}]}, + ) + ) + + assert len(query["bool"]["must"]) == 1 + assert "dis_max" in query["bool"]["must"][0] + assert "filter" not in query["bool"] + assert query["bool"]["must_not"] == [{"prefix": {"curie": "UMLS"}}] + + +def test_unfiltered_query_adds_no_filter_clauses(): + lookup_query = LookupQuery( + raw_string="insulin", + query_strings=("insulin",), + autocomplete=False, + highlighting=False, + offset=0, + limit=10, + ) + + query = _text_query(_build_elasticsearch_query(lookup_query, {"filter": [], "must_not": []})) + + assert len(query["bool"]["must"]) == 1 + assert "dis_max" in query["bool"]["must"][0] + assert "filter" not in query["bool"] + assert "must_not" not in query["bool"]