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

fix query string example for boolean query #28881

Merged
merged 2 commits into from
Mar 30, 2018
Merged

Conversation

fbaligand
Copy link
Contributor

@fbaligand fbaligand commented Mar 2, 2018

Replace (ny OR (new AND york)) city) by city:(ny OR (new AND york))

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@javanna
Copy link
Member

javanna commented Mar 2, 2018

hi @fbaligand thanks for sending a PR. I think that the example is correct as-is though. city in the example query is a word that we are searching for rather than the field name to search against.

@javanna javanna closed this Mar 2, 2018
@fbaligand
Copy link
Contributor Author

fbaligand commented Mar 2, 2018

Hi @javanna,

Why I really think it is a mistake :

  • first, the text below the example that explains it, doesn't mention 'city' and mention only 'ny' and 'new york'
  • secondly, with this example query, a document with only "city" term will match the query
  • thirdly, the query string is invalid : there are 3 ) for 2 (

@javanna
Copy link
Member

javanna commented Mar 2, 2018

heya @fbaligand thanks for clarifying, I am reopening this. I still think that city was meant to be part of the query, yet even in that case the explanation and the parentheses would need to be fixed.

@jimczi given that you added this example a while ago, what do you think?

@javanna javanna reopened this Mar 2, 2018
@javanna javanna requested a review from jimczi March 2, 2018 13:45
@javanna javanna added the >docs General docs changes label Mar 2, 2018
@elastic elastic deleted a comment from elasticmachine Mar 2, 2018
@elastic elastic deleted a comment from elasticmachine Mar 2, 2018
@fbaligand
Copy link
Contributor Author

Thanks for reopening the issue !

Concerning ‘city’, for having tested it on Elasticsearch 6.2.1, a document with « Boston City » will match, which is not the aim.

@fbaligand
Copy link
Contributor Author

@jimczi
What do you think about this doc fix ?

@jimczi
Copy link
Contributor

jimczi commented Mar 19, 2018

Sorry for the late reply @fbaligand . city is not a field name in this example, it is just to show how the synonyms are merged with normal terms, in this case the default operator is OR so it is expected that documents that contain only city are returned. IMO the only problem here is the number of parenthesis ;).

@fbaligand
Copy link
Contributor Author

fbaligand commented Mar 19, 2018

Hi @jimczi

Thanks for your answer.
OK, I understand the idea of synonyms with "ny city" and "new york city".
So that this example query can't return results like "salt lake city" or even "city" alone, what do you think about this fix :
(ny OR (new AND york)) AND city

@jimczi
Copy link
Contributor

jimczi commented Mar 23, 2018

Sorry but I think that this example is fine, the important block of this example is that new and york must match together since it's a multi word synonyms even if the default operator is OR so I'd prefer to keep it as is to emphasize this relation.

@fbaligand
Copy link
Contributor Author

Hi @jimczi

OK, no problem.

So I finally only removed last parenthesis.
Is it OK for you ?

@jimczi jimczi merged commit 437ad06 into elastic:master Mar 30, 2018
@jimczi
Copy link
Contributor

jimczi commented Mar 30, 2018

Sorry for the late response, I merged your pr in master. Thanks @fbaligand !

@fbaligand
Copy link
Contributor Author

Thanks @jimczi for the merge !

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 3, 2018
* master: (80 commits)
  Remove HTTP max content length leniency (elastic#29337)
  Begin moving XContent to a separate lib/artifact (elastic#29300)
  Java versions for ci (elastic#29320)
  Minor cleanup in the InternalEngine (elastic#29241)
  Clarify expectations of false positives/negatives (elastic#27964)
  Update docs on vertex ordering (elastic#27963)
  Revert "REST high-level client: add support for Indices Update Settings API (elastic#28892)" (elastic#29323)
  [test] remove Streamable serde assertions (elastic#29307)
  Improve query string docs (elastic#28882)
  fix query string example for boolean query (elastic#28881)
  Resolve unchecked cast warnings introduced with elastic#28892
  REST high-level client: add support for Indices Update Settings API (elastic#28892)
  Search: Validate script query is run with a single script (elastic#29304)
  [DOCS] Added info on WGS-84. Closes issue elastic#3590 (elastic#29305)
  Increase timeout on Netty client latch for tests
  Build: Use branch specific refspec sysprop for bwc builds (elastic#29299)
  TEST: trim unsafe commits before opening engine
  Move trimming unsafe commits from engine ctor to store (elastic#29260)
  Fix incorrect geohash for lat 90, lon 180 (elastic#29256)
  Do not load global state when deleting a snapshot (elastic#29278)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>docs General docs changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants