-
-
Notifications
You must be signed in to change notification settings - Fork 153
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.
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(search): Enhances query_string queries #4814
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand this code, but I did have a thought about the test, which I added. I'll let Alberto chime in!
cl/search/tests/tests_es_recap.py
Outdated
params = { | ||
"type": SEARCH_TYPES.RECAP, | ||
"q": 'cause:"31:3730 Qui Tam False Claims Act"', | ||
} | ||
async_to_sync(self._test_article_count)( | ||
params, 2, "faceted_cause_query_string" | ||
) | ||
params = { | ||
"type": SEARCH_TYPES.RECAP, | ||
"cause": '"31:3730 Qui Tam False Claims Act"', | ||
} | ||
async_to_sync(self._test_article_count)(params, 2, "cause_filter") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be helpful to pull the cause string out to show that it's the same in every instance:
params = { | |
"type": SEARCH_TYPES.RECAP, | |
"q": 'cause:"31:3730 Qui Tam False Claims Act"', | |
} | |
async_to_sync(self._test_article_count)( | |
params, 2, "faceted_cause_query_string" | |
) | |
params = { | |
"type": SEARCH_TYPES.RECAP, | |
"cause": '"31:3730 Qui Tam False Claims Act"', | |
} | |
async_to_sync(self._test_article_count)(params, 2, "cause_filter") | |
cause_str = "31:3730 Qui Tam False Claims Act" | |
params = { | |
"type": SEARCH_TYPES.RECAP, | |
# Do it in main query box | |
"q": f'cause:"{cause_str}"', | |
} | |
async_to_sync(self._test_article_count)( | |
params, 2, "faceted_cause_query_string" | |
) | |
params = { | |
"type": SEARCH_TYPES.RECAP, | |
# Do it in the cause field as a phrase | |
"cause": f'"{cause_str}"', | |
} | |
async_to_sync(self._test_article_count)(params, 2, "cause_filter") | |
params = { | |
"type": SEARCH_TYPES.RECAP, | |
# Do it in the cause field without quotes | |
"cause": f'{cause_str}', | |
} | |
async_to_sync(self._test_article_count)( | |
params, 2, "cause_filter_no_phrase" | |
) |
I also added a test without quotes, which I hope will work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be helpful to pull the cause string out to show that it's the same in every instance:
Great idea. I'll update the code to reflect this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good and resolves the issue for quoted queries within filters.
However, we were discussing that the test suggested without quotes in the filter is still failing. This is due to a combination of factors specific to these terms that cause synonyms applied at search time to mismatch the indexed terms. Eduardo will explain the detailed problem in a different issue so we can discuss it and choose the best trade-off to address it.
This is good to merge.
@albertisfu Here's the issue: #4816 |
Fixes #3993 by updating
build_text_filter
to use thequote_field_suffix
parameter.