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.

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

Remove quotes around placeholders in Elasticsearch custom query #762

Merged
merged 3 commits into from
Jan 25, 2021

Conversation

tanaysoni
Copy link
Contributor

@tanaysoni tanaysoni commented Jan 22, 2021

This PR removes quotes around placeholders in Elasticsearch custom_query parameter.

Before

The placeholder terms($query, $years) had quotes around them. For instance,

retriever = ElasticsearchRetriever(
        document_store=document_store,
        custom_query="""
                {
                    "size": 10, 
                    "query": {
                        "bool": {
                            "should": [{
                                "multi_match": {"query": "${query}", "type": "most_fields", "fields": ["text"]}}],
                                "filter": [{"term": {"year": "${years}"}}]}}}"""
)

Now

The placeholders must be used without quotes. For instance,

retriever = ElasticsearchRetriever(
        document_store=document_store,
        custom_query="""
                {
                    "size": 10, 
                    "query": {
                        "bool": {
                            "should": [{
                                "multi_match": {"query": ${query}, "type": "most_fields", "fields": ["text"]}}],
                                "filter": [{"term": {"year": ${years}}}]}}}"""
)

@tanaysoni tanaysoni changed the title WIP: Fix docs for Elasticsearch custom_query & add a test case Fix docstring for Elasticsearch custom_query & add a test case Jan 22, 2021
@tanaysoni tanaysoni requested a review from tholor January 22, 2021 15:01
@tanaysoni tanaysoni self-assigned this Jan 22, 2021
"query": {
"bool": {
"should": [{
"multi_match": {"query": "${query}", "type": "most_fields", "fields": ["text"]}}],
Copy link
Member

Choose a reason for hiding this comment

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

Why is it "${query}"here but one line afterwards it's only ${years} without quotation marks?

Copy link
Contributor Author

@tanaysoni tanaysoni Jan 25, 2021

Choose a reason for hiding this comment

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

Yes, that's inconsistent. I removed quotes around the query placeholder as well.

@tanaysoni tanaysoni requested a review from tholor January 25, 2021 11:33
Copy link
Member

@tholor tholor left a comment

Choose a reason for hiding this comment

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

LGTM

@tholor
Copy link
Member

tholor commented Jan 25, 2021

Can you please add a "before vs. after" code snippet for the breaking change (so that we can easily copy & paste it to the next release notes)?

@tanaysoni tanaysoni changed the title Fix docstring for Elasticsearch custom_query & add a test case Remove quotes around placeholders in Elasticsearch custom query Jan 25, 2021
@tanaysoni tanaysoni merged commit 46307d1 into master Jan 25, 2021
@tanaysoni tanaysoni deleted the custom-query branch January 25, 2021 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants