much better, flexible tag filters. change from json search templates to java api #146

sdole · 2015-02-28T21:35:24Z

Here are the fixes/improvements:

tag filter now supports multiple ways of filtering. inclusions, exclusions by tag, key, value and combinations of those. So, in theory, one can be very very specific in filtering search criteria.
New code no longer uses the json template based search criteria mechanism. Instead, it uses ES Java api.
Code is now a little more spread out into multiple classes instead of being in private methods in a handful of classes - IMO: this improves testability of code, makes it extensible, and also makes it less prone to bugs.
Readme has an updated description.

To follow the path of code/instructions, look at lines 139 to 143 of App.java. The old Searcher is now deprecated, but not deleted. The replacement is SearchRequestHandler. This in turn has "Factory" classes that instantiate the required handlers to execute searches instead of direct dependency on concrete implementations. There are three steps to a search. First is to convert the native spark web request into a PhotonRequest object. During this step, we get the opportunity to validate the incoming request and do any defaults etc. Second step is to optionally setup a tag filter. Finally, the last step is to execute an Elasticsearch search.

I have also added unit tests.

All of the Elasticsearch API setup is in class PhotonQueryBuilder

Added new RequestFactory

fixed a failing test

photon request factory done

…. need to flesh out tests a little more.

… few more final tests.

…g touches.

… to investigate.

Added new RequestFactory

fixed a failing test

photon request factory done

…FactoryTest.java

…ory.java

karussell · 2015-03-01T20:48:52Z

Looks like a big refactoring, nice :) !

Did you already test this in production?

Some questions:

What is the difference between BaseElasticsearchSearcher and BasePhotonSearcher?
Did you thought about avoiding the parsing code and just use a Lucene parser? (to get even more powerful queries)
Also there is still this System.out.println(query);
Why did you uncomment the TagFilterQueryBuilderTest only? Is it not fully replaced or replacable?

…rch jsons for base search and location bias search

sdole · 2015-03-01T23:31:21Z

Yes, it is sort of a big refactor. I understand your concern about testing it in production. However, what do you mean by "production"? My production is not busy enough for a good test, so, the answer is no. However:

I did test from a browser ("/api") against the index file I downloaded from komoot. Do you have test cases that I can manually execute against photon? I searched using my requirements and it looked fine.
I brought back part of TagFilterQueryBuilderTest which you mentioned in your point 4. This now has a test comparing the java output to the erstwhile json template. So, it might be worthwhile to compare the files src/test/resources/json_queries to the file /es/query.json
Further, I have a test named TagFilterQueryBuilderSearchTest that runs through a few test scenarios and those seem to work fine too.
That System.out.println() was added to searcher when I started work to see what the final json looked like before I started work. I forgot to remove it and did not notice it because it was not being executed. I have taken it out now and if you compare the fork vs master, you will notice that the only line changed is @deprecated at the top.
About the difference between BaseElasticsearchSearcher and BasePhotonSearcher: I have removed BasePhotonSearcher. It was not being used.
About avoiding the parsing code... I have not idea how to do that... To be honest, I am very new to elasticsearch and indexing in general, I would not know where to start... Its only because photon already exists that I am able to refactor... creating this from scratch is a whole another universe for me.

Let me know what you think about testing this. I do understand what you are asking for.

sdole · 2015-03-02T00:37:58Z

So you know that no code was lost: there was some custom coding in the Searcher.java that I have taken out into separate classes. Here those are:

karussell · 2015-03-02T08:00:00Z

Please don't take it as critique :), it is a really nice effort as the combinating-template strategy is a lot more error prone IMO.

Regarding the Lucense-parsing: there is an ElasticSearch query which supports this, although this query type cannot be used as direct user input it is really nice to do simple filtering and arbitrary combinations.

sdole · 2015-03-02T11:09:02Z

No, not taking as a critique. :)

OK, I saw the link you sent. I will read some more and try to learn. For now, this pull request simply replicates what was in the json template. That way, there are fewer changes to test. You know what I mean?

christophlingg · 2015-03-02T16:15:15Z

src/main/java/de/komoot/photon/query/PhotonRequestFactory.java

+        try {
+            Double lon = Double.valueOf(webRequest.queryParams("lon"));
+            Double lat = Double.valueOf(webRequest.queryParams("lat"));
+            locationForBias = new GeometryFactory(new PrecisionModel(), 4326).createPoint(new Coordinate(lon, lat));


there is no need to create a geometry factory for every request, i can be a static final field

christophlingg · 2015-03-02T16:30:57Z

cool that's a big pull request with a lot of refactoring! I left some commands and maybe found a bug...

I see logic got separated which is a step forward. You are right, it makes testing much easier.

…If no results are found, then do a lenient search. Added javadoc to explain the filter states in PhotonQueryBuilder Made the GeometryFactory to be private final static instead of being a local variable.

sdole · 2015-03-03T02:31:24Z

You are right @christophlingg about the bug. I committed a fix with changes to the unit test. I also made the geometryfactory private static final. added some java docs.

The difference between ConvertToJson and ConvertToGeoJson is that ConvertToJson converts SearchHits to a list of JSON objects. ConvertToGeoJson then converts to geojson format. This follows the exact pattern of code in Searcher and RequestHandler before refactoring. The Searcher class first converts SearchHits to a json and then operates on it to remove dupes etc. Then, in the RequestHandler, there are a few more lines of code to convert into GeoJson. I wanted to keep those lines of code as is, so, I simply replicated everything - except in different classes instead of private methods.

Thanks for your words. I am glad that you and @karussell feel that this is a good refactor and a step forward. I am also glad that you feel that this makes testing much easier.

Let me know how this looks. Looking forward to this getting into production on your website! :-)

sdole · 2015-03-04T04:21:33Z

If there is anything else you have questions about, please ask. I will be happy to change code if you find issues or have concerns.

christophlingg · 2015-03-04T20:28:49Z

sorry @sdole , at the moment i am quite busy. i'll make an 0.2.2 release tomorow

sdole · 2015-03-04T20:34:33Z

Absolutely no worries. Thanks for the reply.

    Sent by Outlook for Android

On Wed, Mar 4, 2015 at 12:28 PM -0800, "Christoph Lingg" notifications@github.com wrote:

sorry @sdole , at the moment i am quite busy. i'll make an 0.2.2 release tomorow

—
Reply to this email directly or view it on GitHub.

much better, flexible tag filters. change from json search templates to java api

ec2-user added 30 commits February 14, 2015 16:52

decouple searcher from request handler

a652329

Added new RequestFactory

decouple searcher from request handler

dff82ee

fixed a failing test

decouple searcher from request handler

f725bbd

photon request factory done

decouple searcher from request handler

92902b5

photon request factory done

decouple searcher from request handler

3641ca3

photon request factory done

decouple searcher from request handler

78b5a0e

photon request factory done

decouple searcher from request handler

84ac12a

photon request factory done

decouple searcher from request handler

3e90e49

photon request factory done

fixed the query part for collector

3690d44

decouple searcher from request handler

eea166d

photon request factory done

decouple searcher from request handler

00a614c

photon request factory done

added the three new "Without" methods. still need to test and implement

2f7cce0

added all methods and tests, however tests are incomplete and do fail…

eaa0e92

…. need to flesh out tests a little more.

done with live tests against a test index. now to integrate it.

53067fe

done with live tests against a test index. now to integrate it.

c8f0216

interim check in

4826101

another interim check in. getting closer with integration.

5d27fa2

integration works. now, downloading latest data and will be running a…

081a9a8

… few more final tests.

integration works. now, downloading latest data and will be running a…

e627348

… few more final tests.

limits not working. continue to debug.

7b6e7e6

wrote a new test for find with key not value

fee1cbb

testing with real index data looks good. now to clean up and finishin…

d260694

…g touches.

added a couple tests, improved javadoc. There are tests failing, need…

af51231

… to investigate.

all done. now adding some documentation

2074f6f

decouple searcher from request handler

2016643

Added new RequestFactory

decouple searcher from request handler

f3c7cb7

fixed a failing test

decouple searcher from request handler

4cf6e6c

photon request factory done

decouple searcher from request handler

8b18162

photon request factory done

decouple searcher from request handler

fe70187

photon request factory done

decouple searcher from request handler

2855b37

photon request factory done

ec2-user added 8 commits February 28, 2015 13:57

added documention. now to update readme.

a2775c3

added documention. now to update readme.

07548dd

added documention. now to update readme.

db8f604

added documention. now to update readme.

d48aa46

Rename ElasticSearchSearcherFactoryTest.java to ElasticsearchSearcher…

cc51481

…FactoryTest.java

Rename ElasticSearchSearcher.java to ElasticsearchSearcher.java

fb24db6

Rename ElasticSearchSearcherFactory.java to ElasticsearchSearcherFact…

d81a559

…ory.java

Rename BaseElasticSearchSearcher.java to BaseElasticsearchSearcher.java

b15019e

sdole mentioned this pull request Feb 28, 2015

Allow searching on osm_key and osm_value #68

Closed

ec2-user added 2 commits March 1, 2015 17:18

bring back two tests to ensure that new search json match the old sea…

48f1577

…rch jsons for base search and location bias search

removed unused files.

9bdf66b

ec2-user added 2 commits March 1, 2015 18:30

removed unused files.

1a9db8a

only javadoc formatting.

fa1ad30

christophlingg reviewed Mar 2, 2015
View reviewed changes

fixed the bug that @christophlingg found. First, do a strict search. …

8e0418e

…If no results are found, then do a lenient search. Added javadoc to explain the filter states in PhotonQueryBuilder Made the GeometryFactory to be private final static instead of being a local variable.

christophlingg added a commit that referenced this pull request Mar 6, 2015

Merge pull request #146 from sdole/master

96bd7e4

much better, flexible tag filters. change from json search templates to java api

christophlingg merged commit 96bd7e4 into komoot:master Mar 6, 2015

christophlingg mentioned this pull request May 8, 2015

Added Reverse Geocoding API & Remove old code support #164

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

much better, flexible tag filters. change from json search templates to java api #146

much better, flexible tag filters. change from json search templates to java api #146

sdole commented Feb 28, 2015

karussell commented Mar 1, 2015

sdole commented Mar 1, 2015

sdole commented Mar 2, 2015

karussell commented Mar 2, 2015

sdole commented Mar 2, 2015

christophlingg Mar 2, 2015

christophlingg commented Mar 2, 2015

sdole commented Mar 3, 2015

sdole commented Mar 4, 2015

christophlingg commented Mar 4, 2015

sdole commented Mar 4, 2015

much better, flexible tag filters. change from json search templates to java api #146

much better, flexible tag filters. change from json search templates to java api #146

Conversation

sdole commented Feb 28, 2015

karussell commented Mar 1, 2015

sdole commented Mar 1, 2015

sdole commented Mar 2, 2015

karussell commented Mar 2, 2015

sdole commented Mar 2, 2015

christophlingg Mar 2, 2015

Choose a reason for hiding this comment

christophlingg commented Mar 2, 2015

sdole commented Mar 3, 2015

sdole commented Mar 4, 2015

christophlingg commented Mar 4, 2015

sdole commented Mar 4, 2015