Refactoring of GeoBoundingBoxQueryBuilder and -Parser #11969

MaineC · 2015-07-01T08:11:26Z

... -Builder/-Parser. This add equals, hashcode, read/write methods, separates toQuery and JSON parsing and adds first tests.

Deprecates two types of initializing the bounding box: In our documentation we speak about specifying top/left and bottom/right corner of a bounding box. Here we also allow for top/right and bottom/left. This adds not only to the amount of code but also testing needed w/o too much benefit for the user other than more chances to confuse top/right/bottom/left/latitude/longitude IMHO.

Missing: validation, there are some easy opportunities for cleaning parser and builder.

The test path that checks the toQuery method if type is set to "indexed" fails at the moment due to missing Guice initialisations. I tried to add everything needed but felt like the result looked a lot like it would actually like to be an ElasticsearchSingleNodeTest so not sure how to proceed here?

Relates to #10217

MaineC · 2015-07-01T08:14:54Z

This is very much work in progress. Posting the PR mostly to seek input on how to proceed with testing toQuery in the type="indexed" setting:

Running as is it complains in the following assertion - trying to fix the assertion by providing the needed dependencies through Guice in our test setup felt like this would be easier to do in an ElasticsearchIntegration/SingleNodeTest. There must be some simpler option to be able to run the toQuery method in a test harness that doesn't need a cluster spun up which I don't see?

java.lang.AssertionError
at __randomizedtesting.SeedInfo.seed([41C2A3231736F031:B639A11D66B535DB]:0)
at org.elasticsearch.indices.fielddata.cache.IndicesFieldDataCache$IndexFieldCache.(IndicesFieldDataCache.java:155)
at org.elasticsearch.indices.fielddata.cache.IndicesFieldDataCache.buildIndexFieldDataCache(IndicesFieldDataCache.java:103)
at org.elasticsearch.index.fielddata.IndexFieldDataService.getForField(IndexFieldDataService.java:268)
at org.elasticsearch.index.query.QueryParseContext.getForField(QueryParseContext.java:183)
at org.elasticsearch.index.query.GeoBoundingBoxQueryBuilderTest.createExpectedQuery(GeoBoundingBoxQueryBuilderTest.java:132)

javanna · 2015-07-01T17:08:40Z

core/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java

    private double[] box = {Double.NaN, Double.NaN, Double.NaN, Double.NaN};

    private String queryName;
    private String type;
+    private String fieldName;


this can still be final no?

javanna · 2015-07-01T17:13:14Z

@MaineC this is one of the few cases that might make it very hard to test things without a node. I need to look into this deeper. Can you leave out for now that specific case in testing, we can look at it while the rest is ready.

MaineC · 2015-07-01T18:06:21Z

Sure - no problem. Leaving it out is a matter of not setting the type to "indexed" when creating the test query builders. I'll also open an issue as a reminder to get back to this test if you don't mind.

MaineC · 2015-07-02T10:04:31Z

Rebased, took the failing test out, added validation - some test setup for query creation still failing. Digging. Missing: cleanup.

MaineC · 2015-07-06T19:39:31Z

Note: Keep #12016 in mind for the test setup.

MaineC · 2015-07-07T09:57:25Z

I believe this is ready for a first round of feedback. Here's what I did:

Split JSON parsing and Lucene query generation, add the usual suspects (equals, hashCode etc.) and a unit test.
Deprecated initialising the bounding box with topRight and bottomLeft corners. In our documentation I only found references to topLeft and bottomRight corners. This leaves REST users two options: Either supply topLeft/bottomRight or supply top/left/bottom/right. Java API users are left with supplying topLeft and bottomRight. Correct me if I'm wrong but IIRC this reflects what is commonly used including what users might already know from Google Maps or Nokia Maps APIs.
Instead of having a double array to hold the lat/lon pairs that define the bounding box and four constants that hold the indices into that array we now have two points of type GeoPoint to define the bounding box.
Instead of having a variable of type String to hold either of two type values switched to using an enum instead.
Returns MatchNoDocs in case no mappings could be found as proposed in Queries and unmapped fields #12016 (mostly to get the unit test to run, doesn't fully address Queries and unmapped fields #12016)

The previous bounding box test still passes, so does the newly added one (as well as anything that runs with mvn clean install).

Caveat: Neither test checks the type=memory code path.

@nik9000 I've been told that you are the Geo Magician - would be great if you could double check what I've done and point out any mistakes as well.

Note: I've left the refactoring steps in separate commits for now if you want to look at them separately to see which step introduced which change.

javanna · 2015-07-07T10:00:48Z

wrong Nik @MaineC that should have been @nknize ;)

MaineC · 2015-07-07T18:35:20Z

Too many Niks - thanks for correcting me. :)

MaineC · 2015-07-09T08:59:02Z

Just skimmed (didn't look particularly deep at all) the changes with @cbuescher - it might make sense to move the geo point/ geo bounding box validation methods into a separate class so it can be used elsewhere. Same for the geo point/ box generation code in the test methods. Both - random point generation in the test and validation in the builder are candidates where I would appreciate input from @nknize - I believe all the rest should be standard query refactoring stuff.

cbuescher · 2015-07-09T09:42:32Z

core/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java

+        } else if (Type.MEMORY.equals(type)) {
+            IndexGeoPointFieldData indexFieldData = parseContext.getForField(fieldType);
+            result = new InMemoryGeoBoundingBoxQuery(topLeft, bottomRight, indexFieldData);
+        } else {


Was wondering if with the Type enum, is this final else check necessary?

MaineC · 2015-09-10T19:57:37Z

Current state: Rebased, updated according to both comments, squashed.

When running the tests from the command line with a "mvn clean install -Dtests.jvms=4" with JAVA_HOME set to /home/isabel/bin/jdk1.8.0_60/ the following two tests are failing reproducably for me:

org.elasticsearch.NamingConventionTests.testNamingConventions
org.elasticsearch.index.store.IndexStoreTests.testStoreDirectoryDefault

Checking what's wrong at the moment.

(After running the same on the feature/query-refactoring branch:

org.elasticsearch.index.store.IndexStoreTests.testStoreDirectoryDefault

is failing there. As CI for our branch is silent I assume there's something off with my current setup.)

cbuescher · 2015-09-10T20:22:20Z

org.elasticsearch.NamingConventionTests.testNamingConventions

I think that has to do with your test name. All the test now have to end in "_Tests" instead of "_Test", that should take care of this error.

MaineC · 2015-09-11T20:50:58Z

Rebased, squashed, fixed failing tests (thanks @cbuescher for the pointer, the other failure indeed was an issue with my machine's setup).

cbuescher · 2015-09-14T13:19:19Z

core/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java

    /**
     * Sets the type of executing of the geo bounding box. Can be either `memory` or `indexed`. Defaults
     * to `memory`.
     */
+    public GeoBoundingBoxQueryBuilder type(GeoExecType type) {
+        this.type = (type != null) ? type : DEFAULT_TYPE;


I started changing the setters that turn null into default value back to throwing IAE, parser shouldn't allow null type value here either. But we can also do that later.

cbuescher · 2015-09-14T13:31:32Z

@MaineC did a quick round, left a few minor comments but I think this is ready, unless you want to give it another look or want someone else to have a look at it again.

MaineC · 2015-09-14T19:15:24Z

Rebased, squashed, fixed the issues @cbuescher found.

cbuescher · 2015-09-16T09:43:37Z

@MaineC heads up, I just pushed the GeoDistanceQueryBuilder refactoring you reviewed, so this PR probably needs one more rebasing, but other than that I think it is ready to go in.

s1monw · 2015-09-17T08:39:23Z

core/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java

+    /**
+     * For serialization only.
+     * */
+    private GeoBoundingBoxQueryBuilder() {


can you just pass an empty string to the ctor with a fieldName for the prototype

Can do, but then I can't check/throw an exception in the constructor if the fieldName argument is an empty string. See change for illustration.

s1monw · 2015-09-17T08:59:31Z

left some comments LGTM otherwise

MaineC · 2015-09-17T10:08:34Z

Thanks for the review. Done fixing most of your comments, about to fix the now failing validation tests.

MaineC · 2015-09-17T20:07:47Z

@s1monw Addressed your comments.

Re-organised some tests as a result (and fixed several issues the original tests had which I hadn't spotted when re-reading them over and over again), rebased and squashed.

s1monw · 2015-09-18T19:14:17Z

LGTM

This add equals, hashcode, read/write methods, validation, separates toQuery and JSON parsing and adds serialization and query generation tests. Deprecates two types of initializing the bounding box: In our documentation we speak about specifying top/left and bottom/right corner of a bounding box. Here we also allow for top/right and bottom/left. This adds not only to the amount of code but also testing needed w/o too much benefit for the user other than more chances to confuse top/right/bottom/left/latitude/longitude IMHO. Missing: The toQuery method with type set to "indexed" is not tested at the moment. Cleanup changes unrelated to base refactoring: * Switched from type String to enum for types in GeoBoundingBoxQueryBuilder. * Switched to using type GeoPoint for storing the bounding box coordinates instead of array of double values. Relates to elastic#10217 for the query refactoring part. Relates to elastic#12016 for how missing mappings are handled. Adds a utility class for generating random geo data. Adds some missing documentation. Extend test to MEMORY type config Fix final review comments and rebase

…oring Refactoring of GeoBoundingBoxQueryBuilder and -Parser

MaineC added the :Query Refactoring label Jul 1, 2015

MaineC self-assigned this Jul 1, 2015

javanna reviewed Jul 1, 2015
View reviewed changes

MaineC force-pushed the feature/geo-bounding-box-refactoring branch from 14ae9ea to 47506cb Compare July 2, 2015 10:03

clintongormley mentioned this pull request Jul 6, 2015

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

Closed

MaineC added the in progress label Jul 7, 2015

MaineC force-pushed the feature/geo-bounding-box-refactoring branch from 47506cb to a476427 Compare July 7, 2015 09:13

MaineC changed the title ~~First steps towards refactoring GeoBoundingBox~~ Refactoring of GeoBoundingBoxQueryBuilder and -Parser Jul 7, 2015

MaineC added the review label Jul 7, 2015

MaineC removed the in progress label Jul 8, 2015

cbuescher reviewed Jul 9, 2015
View reviewed changes

cbuescher mentioned this pull request Sep 11, 2015

Refactors GeoDistanceQueryBuilder/-Parser #13496

Closed

MaineC force-pushed the feature/geo-bounding-box-refactoring branch from 5b758bb to 166e9fb Compare September 11, 2015 20:50

cbuescher reviewed Sep 14, 2015
View reviewed changes

MaineC force-pushed the feature/geo-bounding-box-refactoring branch from 166e9fb to 07bb317 Compare September 14, 2015 19:12

MaineC force-pushed the feature/geo-bounding-box-refactoring branch from 07bb317 to 902ddf5 Compare September 17, 2015 08:34

s1monw reviewed Sep 17, 2015
View reviewed changes

MaineC force-pushed the feature/geo-bounding-box-refactoring branch from 902ddf5 to d6382bd Compare September 17, 2015 20:05

MaineC force-pushed the feature/geo-bounding-box-refactoring branch from d6382bd to 91b97a6 Compare September 20, 2015 18:58

MaineC pushed a commit that referenced this pull request Sep 20, 2015

Merge pull request #11969 from MaineC/feature/geo-bounding-box-refact…

77ffabc

…oring Refactoring of GeoBoundingBoxQueryBuilder and -Parser

MaineC merged commit 77ffabc into elastic:feature/query-refactoring Sep 20, 2015

clintongormley mentioned this pull request Jan 17, 2016

[GEO] Consolidate duplicate Bounding Box classes #10617

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring of GeoBoundingBoxQueryBuilder and -Parser #11969

Refactoring of GeoBoundingBoxQueryBuilder and -Parser #11969

MaineC commented Jul 1, 2015

MaineC commented Jul 1, 2015

javanna Jul 1, 2015

javanna commented Jul 1, 2015

MaineC commented Jul 1, 2015

MaineC commented Jul 2, 2015

MaineC commented Jul 6, 2015

MaineC commented Jul 7, 2015

javanna commented Jul 7, 2015

MaineC commented Jul 7, 2015

MaineC commented Jul 9, 2015

cbuescher Jul 9, 2015

MaineC commented Sep 10, 2015

cbuescher commented Sep 10, 2015

MaineC commented Sep 11, 2015

cbuescher Sep 14, 2015

cbuescher commented Sep 14, 2015

MaineC commented Sep 14, 2015

cbuescher commented Sep 16, 2015

s1monw Sep 17, 2015

MaineC Sep 17, 2015

s1monw commented Sep 17, 2015

MaineC commented Sep 17, 2015

MaineC commented Sep 17, 2015

s1monw commented Sep 18, 2015

Refactoring of GeoBoundingBoxQueryBuilder and -Parser #11969

Refactoring of GeoBoundingBoxQueryBuilder and -Parser #11969

Conversation

MaineC commented Jul 1, 2015

MaineC commented Jul 1, 2015

javanna Jul 1, 2015

Choose a reason for hiding this comment

javanna commented Jul 1, 2015

MaineC commented Jul 1, 2015

MaineC commented Jul 2, 2015

MaineC commented Jul 6, 2015

MaineC commented Jul 7, 2015

javanna commented Jul 7, 2015

MaineC commented Jul 7, 2015

MaineC commented Jul 9, 2015

cbuescher Jul 9, 2015

Choose a reason for hiding this comment

MaineC commented Sep 10, 2015

cbuescher commented Sep 10, 2015

MaineC commented Sep 11, 2015

cbuescher Sep 14, 2015

Choose a reason for hiding this comment

cbuescher commented Sep 14, 2015

MaineC commented Sep 14, 2015

cbuescher commented Sep 16, 2015

s1monw Sep 17, 2015

Choose a reason for hiding this comment

MaineC Sep 17, 2015

Choose a reason for hiding this comment

s1monw commented Sep 17, 2015

MaineC commented Sep 17, 2015

MaineC commented Sep 17, 2015

s1monw commented Sep 18, 2015