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

Lessen leniency of the query dsl. #18276

Merged
merged 1 commit into from May 16, 2016

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented May 11, 2016

This change does the following:

  • Queries that are currently unsupported such as prefix queries on numeric
    fields or term queries on geo fields now throw an error rather than returning
    a query that does not match anything.
  • Fuzzy queries on numeric, date and ip fields are now unsupported: they used
    to create range queries, we now expect users to use range queries directly.
    Fuzzy, regexp and prefix queries are now only supported on text/keyword
    fields (including _all).
  • The _uid and _id fields do not support prefix or range queries anymore as
    it would prevent us to store them more efficiently in the future, eg. by
    using a binary encoding.

Note that it is still possible to ignore these errors by using the lenient
option of the match or query_string queries.

@jpountz
Copy link
Contributor Author

jpountz commented May 11, 2016

If this change is approved I'll work on adding deprecation warnings on 2.x.

public abstract class StringFieldType extends TermBasedFieldType {

public StringFieldType() {
super();
Copy link
Member

Choose a reason for hiding this comment

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

Having an empty ctor body will also use the default super ctor, so I think this super() is not necessary?

@rjernst
Copy link
Member

rjernst commented May 15, 2016

LGTM, I left a couple minor comments.

This change does the following:
 - Queries that are currently unsupported such as prefix queries on numeric
   fields or term queries on geo fields now throw an error rather than returning
   a query that does not match anything.
 - Fuzzy queries on numeric, date and ip fields are now unsupported: they used
   to create range queries, we now expect users to use range queries directly.
   Fuzzy, regexp and prefix queries are now only supported on text/keyword
   fields (including `_all`).
 - The `_uid` and `_id` fields do not support prefix or range queries anymore as
   it would prevent us to store them more efficiently in the future, eg. by
   using a binary encoding.

Note that it is still possible to ignore these errors by using the `lenient`
option of the `match` or `query_string` queries.
@jpountz jpountz merged commit 864ed04 into elastic:master May 16, 2016
@jpountz jpountz deleted the fix/supported_queries branch May 17, 2016 10:23
@jpountz
Copy link
Contributor Author

jpountz commented May 17, 2016

@clintongormley just informed me that removing the support for fuzzy queries on numeric fields in query strings had been previously rejected in #4076. @kimchy Do you still have concerns about stopping to support fuzzy queries on numeric/date/ip fields?

@danielmitterdorfer
Copy link
Member

@jpountz we have an open issue to remove fuzzy query entirely in 6.0 (see #15760). So we should close this one too if we don't remove it...

@jpountz
Copy link
Contributor Author

jpountz commented May 17, 2016

@danielmitterdorfer If my understanding is correct, the concern is more about removing support for the fuzzy operator ~ in query strings than removing the fuzzy query.

@danielmitterdorfer
Copy link
Member

@jpountz Ok, I was just double-checking.

@kimchy
Copy link
Member

kimchy commented May 17, 2016

@jpountz I find the ability to use the fuzzy operator in a query string to be simpler to use in things like Kibana query string and so on. Is there a reason you want to remove it? I always viewed it as a "range query" operator, just apply to other types. To me, it does make sense to have age:10~2 instead of doing the long form range query sample. But if I am in a minority, no problem with removing.

@jpountz
Copy link
Contributor Author

jpountz commented May 19, 2016

First I wanted to remove it for ip addresses since it does not make much sense now that we support ipv6 (the deltas would need to be huge for it to be useful). So then I questioned whether this is useful at all on numeric/date fields and I could not remember of any bug/discussion about this feature, even though it certainly has bugs (for instance, it did not check for overflows). So I am leaning towards removing it, especially given that the range is easy to type too, and this gives us a single way to search ranges of values, which I think prevents confusion.

@kimchy
Copy link
Member

kimchy commented May 19, 2016

Adrien, lead the way :)

jpountz added a commit to jpountz/elasticsearch that referenced this pull request May 23, 2016
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query DSL labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking >enhancement :Search/Search Search-related issues that do not fall into other categories v5.0.0-alpha3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants