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

Add support for querying on range fields in query_string and simple_query_string queries #26555

Closed
martijnvg opened this issue Sep 8, 2017 · 9 comments
Assignees
Labels
>enhancement good first issue low hanging fruit help wanted adoptme :Search/Search Search-related issues that do not fall into other categories

Comments

@martijnvg
Copy link
Member

Currently querying on range fields via query_string and simple_query_string queries results in an error indicating that we do not support this. Whereas querying these fields via the range queries just works as expected.

Adding support for this is trivial. One caveat is that there is no way to specify the relation (like in range query). Therefor if range fields are queries via query_string and simple_query_string then we should default to intersects relation.

@arunagar
Copy link

arunagar commented Sep 9, 2017

i would like to pick this up as my first PR, do i have to assign this to myself to start working on it ?

@martijnvg
Copy link
Member Author

@arunagar You can just open a PR and we can then work on get your change in. No one from Elastic is working on this yet as it hasn't been assigned yet.

@jimczi
Copy link
Contributor

jimczi commented Sep 11, 2017

This is solved tangentially by #26552 because the query_string uses the new signature of MappedFieldType#rangeQuery. As a side effect the query_string will default to intersects when building a range query on a range field.
simple_query_string does not handle range query (there is no syntax for it) so the only thing to do here would be to add unit tests for the query_string.
@arunagar is it something you want to contribute ? Adding unit tests would ensure that we don't break the support in future versions.

@arunagar
Copy link

I will start on this and send a PR, i may take some time to get accustomed to the code, but i will work on this.

@arunagar
Copy link

Hey i was looking at the query_string test for range, there is already existing test in here for range

Is the intention to write a unit test with query_string, which makes sure that the relation is intersect ?

@jimczi
Copy link
Contributor

jimczi commented Sep 13, 2017

@arunagar this test is on a keyword field, what's missing is a query on a range field:
https://www.elastic.co/guide/en/elasticsearch/reference/current/range.html

@lopez83
Copy link

lopez83 commented Nov 22, 2017

@jimczi Is there any problem if I work on the needed test?

@romseygeek romseygeek self-assigned this Dec 11, 2017
@romseygeek
Copy link
Contributor

Range fields are in a separate module, so rather than adding a test to QueryStringQueryBuilderTests I'm going to create a new AbstractQueryTestCase extension in mapper-extras. Does that make sense @jpountz ?

@jpountz
Copy link
Contributor

jpountz commented Dec 11, 2017

@romseygeek 👍

romseygeek added a commit that referenced this issue Dec 12, 2017
[TEST] Add test for *_range fields in query_string queries

Closes #26555
@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
>enhancement good first issue low hanging fruit help wanted adoptme :Search/Search Search-related issues that do not fall into other categories
Projects
None yet
Development

No branches or pull requests

7 participants