Skip to content

fix query string syntax doc when OR operator is missed #28882

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
Merged
merged 2 commits into from
Mar 30, 2018
Merged

fix query string syntax doc when OR operator is missed #28882

merged 2 commits into from
Mar 30, 2018

Conversation

fbaligand
Copy link
Contributor

@jimczi
I complete here your PR #28798 to fix issue #28719

@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 javanna requested a review from jimczi March 2, 2018 10:47
@javanna javanna added the >docs General docs changes label Mar 2, 2018
jimczi
jimczi requested changes Mar 4, 2018
View reviewed changes
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

I left some comments, I think we should just remove the sentence that you changed.

docs/reference/query-dsl/query-string-syntax.asciidoc Outdated
@@ -24,7 +24,7 @@ search terms, but it is possible to specify other fields in the query syntax:
status:active

* where the `title` field contains `quick` or `brown`.
If you omit the OR operator the default operator will be used
If you omit the OR operator, the default operator will be used (except if `title` field is typed `keyword`)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove this sentence completely. We don't split on whitespace anymore so we shouldn't assume that quick and brown are going to be separated. It depends only on the tokenizer that is use for the field. We've added a warning section at the beginning of the docs regarding keyword field so I think it's enough.

docs/reference/query-dsl/query-string-syntax.asciidoc
@@ -36,7 +36,7 @@ search terms, but it is possible to specify other fields in the query syntax:
* where any of the fields `book.title`, `book.content` or `book.date` contains
`quick` or `brown` (note how we need to escape the `*` with a backslash):

book.\*:(quick brown)
book.\*:(quick OR brown)
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to add an explicit OR

@fbaligand
Copy link
Contributor Author

@jimczi
I agree with you.
So I removed the sentence, and the associated example title:(quick brown)

@fbaligand
Copy link
Contributor Author

@jimczi
Could you review my last change and tell if it answers your requested changes ?

@fbaligand
Copy link
Contributor Author

Hi @jimczi
Could you review my last commit ?

jimczi
jimczi approved these changes Mar 30, 2018
View reviewed changes
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

Thanks @fbaligand

@jimczi jimczi merged commit 199d131 into elastic:master Mar 30, 2018
@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
@jasontedor
Merge branch 'master' into align-thread-pool-info-cat-api
1112584 
* 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
Reviewers

@jimczi jimczi jimczi approved these changes

Assignees
No one assigned
Labels
>docs General docs changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fbaligand @elasticmachine @jimczi @javanna