Query refactoring: RangeQueryBuilder and Parser #11108

cbuescher · 2015-05-12T07:34:33Z

Split the parse(QueryParseContext ctx) method into a parsing and a query building part, adding Streamable for serialization and hashCode(), equals() for better testing.
Add basic unit test for Builder and Parser.

PR goes agains query-refacoring feature branch.

cbuescher · 2015-05-12T07:45:39Z

There are two issues with this PR I'd like to get a second opinion on.

In order to stream the timezone and the corresponding format field I chose to use internal String representation and just create the DateMathParser and DateTimeZone objects if needed right before the query is build. However, for early validation of these fields I saw no other way than to create the same objects in validate() already. Would like to hear thoughts about that.
In order to test the whole query construction part when there are mappers for the field, I'd have to add mappings to the parseContext. Not sure how to do that in the setup of the Base test though.

rjernst · 2015-05-12T17:52:31Z

@cbuescher You can add mappings by constructing your own MapperService and adding types? Right now the dummy parse context you have just has null for the MapperService IIRC, or something effectively like that.

javanna · 2015-05-13T09:22:32Z

src/main/java/org/elasticsearch/index/query/RangeQueryBuilder.java

-     */
-    public RangeQueryBuilder from(float from) {
-        this.from = from;
+    public RangeQueryBuilder from(Object from, boolean includeLower) {


I like this simplication, this is a breaking change for java api users though, we need to note this down somewhere, for instance in the migrate_2.0 guide. Actually we could have our own specific file since we don't know yet if this will make it for 2.0.

The 2-arg setter is just an addition, there is still a from(Object from) that should take care of all the former options (int/long/float/double/String). Those where autoboxed to an Object in the existing code already. Not sure if this breas java api then?

good point, I missed that... I think that is fine then, if the from(Object from) was already there. thanks!

javanna · 2015-05-13T10:01:55Z

left a few comments, looks good though!

javanna · 2015-05-13T12:25:26Z

@cbuescher to answer your question above:

In order to stream the timezone and the corresponding format field I chose to use internal String representation and just create the DateMathParser and DateTimeZone objects if needed right before the query is build. However, for early validation of these fields I saw no other way than to create the same objects in validate() already. Would like to hear thoughts about that.

looks good to me, fail fast fail often ;) if the timezone or format are broken, we want to know asap, let's pay the price for the object creation on the coord node then.

cbuescher · 2015-05-13T14:00:37Z

Went through your comments and adressed most of them, rebased on current head of feature branch also. I'd still like to have a look if the toQuery() branch for the DateFieldMappercase can be tested somehow.

javanna · 2015-05-13T15:28:09Z

src/main/java/org/elasticsearch/index/query/BaseQueryBuilder.java

+     * @param obj the input object
+     * @return the same input object or a {@link BytesRef} representation if input was of type string
+     */
+    public static Object convertToBytesRefIfString(Object obj) {


javanna · 2015-05-13T15:35:22Z

left a couple of minor comments, besides those LGTM though I think it's ready

cbuescher · 2015-05-15T13:23:52Z

Added mappings for a field with date type to the base test and extended the RangeQueryBuilderTest to include the code path where date mapper is used. Unfortunately the resulting lucene query is very difficult to access, so I fall back on checking the resulting toString() representation of the query.

javanna · 2015-05-15T13:31:19Z

src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java

@@ -160,7 +168,7 @@ public void testFromXContent() throws IOException {
    public void testToQuery() throws IOException {
        testQuery = createTestQueryBuilder();
        QueryParseContext context = createContext();
-        context.setMapUnmappedFieldAsString(true);
+        context.setAllowUnmappedFields(true);


this needs to stay cause we want to test unmapped and mapped fields?

That must be a merge glitch, I think we settled in mapping fields to String by default for now as long as we don't need it otherwise. Will revert that.

but then we would never test the unmapped fields codepath? maybe we should do it just rarely?

I need to correct myself, we need to set the default to allow unmapped fields, otherwise the String mapper wins over the date mapper we introduced here. Don't understand why, but setAllowUnmappedFields(true) worked with the other tests so far as well.

otherwise the String mapper wins over the date mapper we introduced here

that seems to me like the wrong reason to have it around :)

Which codepaths are we testing now? unmapped fields only, mapped fields only or both?

With the current setup we test unmapped fields when the random setup sets to/from to numeric fields, and mapped fields if we have date string for to/from. Does this make sense of do we need to extend this?

cbuescher · 2015-05-15T15:26:48Z

@javanna I went through your last comments one more time and decided to add an additional test mapping for the integer type case. This way we cover all three cases that are possible in RangeQueryBuilder. I'm a little hesitant in these cases with that level of tests because they are likely to break fast with subtle changes in the lucene query code, but maybe thats a good thing.

javanna · 2015-05-15T15:34:04Z

src/test/java/org/elasticsearch/index/query/RangeQueryBuilderTest.java

+            Long min = expectedDateLong(queryBuilder.from(), queryBuilder, context);
+            Long max = expectedDateLong(queryBuilder.to(), queryBuilder, context);
+            Query expectedQuery = NumericRangeQuery.newLongRange(DATE_FIELD_NAME, min, max, queryBuilder.includeLower(), queryBuilder.includeUpper());
+            assertEquals(query.rewrite(null), expectedQuery.rewrite(null));


do we need to rewrite both queries?

In this case unfortunately its necessary since for reasons I explained above it very tricky to compare the LateParsingQuery, the wrapped query itself has protected contructor. This is the easiest way I found after some digging around.

javanna · 2015-05-15T15:38:46Z

I went over it again, left a few more comments, it's very close though, just a few minor changes to make I guess

…est.

…r code for String/BytesRef conversion to base class

…te field

… of date type

…() test now

…d integer and date types

…eryBuilder

cbuescher · 2015-05-18T09:04:20Z

I went through your last comments regarding test class and rebased the whole PR on top of current feature branch.

javanna · 2015-05-18T09:09:33Z

src/main/java/org/elasticsearch/index/query/RangeQueryBuilder.java

-    public RangeQueryBuilder from(long from) {
-        this.from = from;
-        return this;
+    public String fieldname() {


s/fieldname/fieldName

javanna · 2015-05-18T09:16:54Z

left a couple of minor comments, looks great besides those

…), to() methods

cbuescher · 2015-05-18T09:29:28Z

@javanna thanks, adressed last couple of changes.

javanna · 2015-05-18T09:53:30Z

LGTM

…est. Split the parse(QueryParseContext ctx) method into a parsing and a query building part, adding Streamable support for serialization and hashCode(), equals() for better testing. This PR also adds test setup for two mappes fields (integer, date) to the BaseQueryTestCase and introduces helper methods for optional conversion of String fields to BytesRef representation that is shared with the already refactored BaseTermQueryBuilder. Relates to #10217 Closes #11108

cbuescher · 2015-05-18T10:45:22Z

Merged with feature branch with b99cb21

…est. Split the parse(QueryParseContext ctx) method into a parsing and a query building part, adding Streamable support for serialization and hashCode(), equals() for better testing. This PR also adds test setup for two mappes fields (integer, date) to the BaseQueryTestCase and introduces helper methods for optional conversion of String fields to BytesRef representation that is shared with the already refactored BaseTermQueryBuilder. Relates to elastic#10217 Closes elastic#11108

cbuescher added review labels May 12, 2015

javanna reviewed May 13, 2015
View reviewed changes

cbuescher force-pushed the feature/query-refactoring-rangequery branch from bca5aeb to 02c9f9d Compare May 13, 2015 13:55

javanna reviewed May 13, 2015
View reviewed changes

cbuescher force-pushed the feature/query-refactoring-rangequery branch 2 times, most recently from c884876 to a4da5bb Compare May 15, 2015 13:20

javanna reviewed May 15, 2015
View reviewed changes

Query Refactoring: Add RangeQueryBuilder and Parser refactoring and t…

1243acc

…est.

cbuescher added 9 commits May 18, 2015 10:34

Fixed problems with lower/upper bounds and serialization, moved helpe…

b88a680

…r code for String/BytesRef conversion to base class

Make helper methods protected, use them in TermQuery

57a4167

Minor changes necessary after rebase on current feature branch

4358def

Extended RangeQueryBuilderTest to cover also cases when to/from is da…

08f270e

…te field

Minor changes to mappings setup, using constant for fieldname that is…

63cd591

… of date type

Adressed last round of comments, using lucene query equals in toQuery…

3e4d7f0

…() test now

Added test code to cover all three code paths, unmapped fields, mappe…

e47a227

…d integer and date types

Simplifying and extending test.

30da19d

Add use of byterefs helper method introduced in this PR to BaseTermQu…

a891ca0

…eryBuilder

cbuescher force-pushed the feature/query-refactoring-rangequery branch from e40dc15 to a891ca0 Compare May 18, 2015 09:03

javanna reviewed May 18, 2015
View reviewed changes

Minor changes naming fieldName, changed conversion direction in from(…

0871ba7

…), to() methods

Adding comments for internal conversion of values to BytesRef

9f3739e

cbuescher removed the review label May 18, 2015

cbuescher closed this May 18, 2015

clintongormley mentioned this pull request Sep 8, 2015

Refactor parsing of queries/filters, aggs, suggester APIs #10217

Closed

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query Refactoring labels Feb 14, 2018

cbuescher deleted the feature/query-refactoring-rangequery branch March 20, 2024 20:15

Query refactoring: RangeQueryBuilder and Parser #11108

Query refactoring: RangeQueryBuilder and Parser #11108

Uh oh!

Conversation

cbuescher commented May 12, 2015

Uh oh!

cbuescher commented May 12, 2015

Uh oh!

rjernst commented May 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented May 13, 2015

Uh oh!

javanna commented May 13, 2015

Uh oh!

cbuescher commented May 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented May 13, 2015

Uh oh!

cbuescher commented May 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher commented May 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented May 15, 2015

Uh oh!

cbuescher commented May 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented May 18, 2015

Uh oh!

cbuescher commented May 18, 2015

Uh oh!

javanna commented May 18, 2015

Uh oh!

cbuescher commented May 18, 2015

Uh oh!

Uh oh!