Repository: incubator-usergrid Updated Branches: refs/heads/two-dot-o 2aeb7c631 -> 4887b0235
Added fixes to do distance ordered by queries. Project: http://git-wip-us.apache.org/repos/asf/incubator-usergrid/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-usergrid/commit/4887b023 Tree: http://git-wip-us.apache.org/repos/asf/incubator-usergrid/tree/4887b023 Diff: http://git-wip-us.apache.org/repos/asf/incubator-usergrid/diff/4887b023 Branch: refs/heads/two-dot-o Commit: 4887b02354d699b0abf5bbd5278a197b46907d3e Parents: 2aeb7c6 Author: GERey <gre...@apigee.com> Authored: Wed Aug 5 14:55:20 2015 -0700 Committer: GERey <gre...@apigee.com> Committed: Wed Aug 5 14:55:20 2015 -0700 ---------------------------------------------------------------------- .../cassandra/QueryProcessorImpl.java | 8 +- .../org/apache/usergrid/persistence/GeoIT.java | 114 +++++++++++-------- .../index/impl/EsEntityIndexImpl.java | 44 ++++--- .../persistence/index/impl/EsQueryVistor.java | 66 ++++++----- .../usergrid/persistence/index/query/Query.java | 16 +++ .../index/query/tree/QueryVisitor.java | 10 +- .../applications/queries/GeoPagingTest.java | 43 +++++++ 7 files changed, 209 insertions(+), 92 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-usergrid/blob/4887b023/stack/core/src/main/java/org/apache/usergrid/persistence/cassandra/QueryProcessorImpl.java ---------------------------------------------------------------------- diff --git a/stack/core/src/main/java/org/apache/usergrid/persistence/cassandra/QueryProcessorImpl.java b/stack/core/src/main/java/org/apache/usergrid/persistence/cassandra/QueryProcessorImpl.java index 94569d5..dc1728a 100644 --- a/stack/core/src/main/java/org/apache/usergrid/persistence/cassandra/QueryProcessorImpl.java +++ b/stack/core/src/main/java/org/apache/usergrid/persistence/cassandra/QueryProcessorImpl.java @@ -71,6 +71,7 @@ import org.apache.usergrid.persistence.query.ir.result.ScanColumn; import org.apache.usergrid.persistence.schema.CollectionInfo; import org.elasticsearch.index.query.FilterBuilder; import org.elasticsearch.index.query.QueryBuilder; +import org.elasticsearch.search.sort.GeoDistanceSortBuilder; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -469,7 +470,7 @@ public class QueryProcessorImpl implements QueryProcessor { // change the property name to coordinates nodes.push( new WithinNode( op.getProperty().getIndexedName(), op.getDistance().getFloatValue(), - op.getLatitude().getFloatValue(), op.getLongitude().getFloatValue(), ++contextCount ) ); + op.getLatitude().getFloatValue(), op.getLongitude().getFloatValue(), ++contextCount ) ); } @@ -628,6 +629,11 @@ public class QueryProcessorImpl implements QueryProcessor { public FilterBuilder getFilterBuilder() { throw new UnsupportedOperationException("Not supported by this vistor implementation."); } + + @Override + public GeoDistanceSortBuilder getGeoDistanceSortBuilder() { + throw new UnsupportedOperationException("Not supported by this vistor implementation."); + } } http://git-wip-us.apache.org/repos/asf/incubator-usergrid/blob/4887b023/stack/core/src/test/java/org/apache/usergrid/persistence/GeoIT.java ---------------------------------------------------------------------- diff --git a/stack/core/src/test/java/org/apache/usergrid/persistence/GeoIT.java b/stack/core/src/test/java/org/apache/usergrid/persistence/GeoIT.java index a1ac4ff..68a4ae0 100644 --- a/stack/core/src/test/java/org/apache/usergrid/persistence/GeoIT.java +++ b/stack/core/src/test/java/org/apache/usergrid/persistence/GeoIT.java @@ -419,53 +419,6 @@ public class GeoIT extends AbstractCoreIT { assertEquals(numEntities, count); } - - @Test - public void testSamePointPaging() throws Exception { - - EntityManager em = app.getEntityManager(); - assertNotNull(em); - - // save objects in a diagonal line from -90 -180 to 90 180 - - int numEntities = 500; - - for (int i = 0; i < numEntities; i++) { - Map<String, Object> data = new HashMap<String, Object>(2); - data.put("name", String.valueOf(i)); - setPos(data, 0, 0); - - em.create("store", data); - } - - em.refreshIndex(); - - Query query = new Query(); - // earth's circumference is 40,075 kilometers. Up it to 50,000kilometers - // just to be save - query.addFilter("location within 50000000 of 0, 0"); - query.setLimit(100); - - int count = 0; - Results results; - - do { - results = em.searchCollection(em.getApplicationRef(), "stores", query); - - for (Entity entity : results.getEntities()) { - assertEquals(String.valueOf(count), entity.getName()); - count++; - } - - // set for the next "page" - query.setCursor(results.getCursor()); - } - while (results.getCursor() != null); - - // check we got back all 500 entities - assertEquals(numEntities, count); - } - @Test public void testDistanceByLimit() throws Exception { @@ -580,6 +533,73 @@ public class GeoIT extends AbstractCoreIT { assertEquals(startDelta - (size - max), count); } + /** + * Verify that elasticsearch does a secondary ordering on paging such that we get consistent results + * back from a cursor despite having a geoquery with all the positions in the same location. + * @throws Exception + */ + @Test + public void testSamePointConsistantPaging() throws Exception { + + EntityManager em = app.getEntityManager(); + assertNotNull(em); + + // save objects in a diagonal line from -90 -180 to 90 180 + + int numEntities = 500; + + for (int i = 0; i < numEntities; i++) { + Map<String, Object> data = new HashMap<String, Object>(2); + data.put("name", String.valueOf(i)); + setPos(data, 0, 0); + + em.create("store", data); + } + + em.refreshIndex(); + + Query query = new Query(); + // earth's circumference is 40,075 kilometers. Up it to 50,000kilometers + // just to be save + query.addFilter("location within 50000000 of 0, 0"); + query.setLimit(100); + List<String> names = new ArrayList<String>(); + + int count = 0; + Results results; + //get arraylist of entities from a search + do { + results = em.searchCollection(em.getApplicationRef(), "stores", query); + + for (Entity entity : results.getEntities()) { + names.add( count,entity.getName() ); + count++; + } + + // set for the next "page" + query.setCursor(results.getCursor()); + } + while (results.getCursor() != null); + //verify that entities come out in the same order when doing the same query against the same data. + //aka make sure the elasticsearch does a secondary search. + count = 0; + do { + results = em.searchCollection(em.getApplicationRef(), "stores", query); + + for (Entity entity : results.getEntities()) { + assertEquals( names.get( count ),entity.getName() ); + count++; + } + + // set for the next "page" + query.setCursor(results.getCursor()); + } + while (results.getCursor() != null); + + // check we got back all 500 entities + assertEquals(numEntities, count); + } + @Test public void testDenseSearch() throws Exception { http://git-wip-us.apache.org/repos/asf/incubator-usergrid/blob/4887b023/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/impl/EsEntityIndexImpl.java ---------------------------------------------------------------------- diff --git a/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/impl/EsEntityIndexImpl.java b/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/impl/EsEntityIndexImpl.java index 9add426..84672a1 100644 --- a/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/impl/EsEntityIndexImpl.java +++ b/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/impl/EsEntityIndexImpl.java @@ -36,6 +36,7 @@ import org.apache.usergrid.persistence.index.exceptions.IndexException; import org.apache.usergrid.persistence.index.query.CandidateResult; import org.apache.usergrid.persistence.index.query.CandidateResults; import org.apache.usergrid.persistence.index.query.Query; +import org.apache.usergrid.persistence.index.query.tree.QueryVisitor; import org.apache.usergrid.persistence.map.MapManager; import org.apache.usergrid.persistence.map.MapManagerFactory; import org.apache.usergrid.persistence.map.MapScope; @@ -58,8 +59,10 @@ import org.elasticsearch.action.search.SearchRequestBuilder; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.search.SearchScrollRequestBuilder; import org.elasticsearch.client.AdminClient; +import org.elasticsearch.common.geo.GeoDistance; import org.elasticsearch.common.settings.ImmutableSettings; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.unit.DistanceUnit; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.index.query.*; @@ -293,7 +296,7 @@ public class EsEntityIndexImpl implements AliasedEntityIndex { @Override public String[] getIndexes(final AliasType aliasType) { - return aliasCache.getIndexes(alias, aliasType); + return aliasCache.getIndexes( alias, aliasType ); } @@ -387,6 +390,7 @@ public class EsEntityIndexImpl implements AliasedEntityIndex { final String[] entityTypes = searchTypes.getTypeNames(); QueryBuilder qb = query.createQueryBuilder(context); + QueryVisitor queryVisitor = query.getQueryVisitor(); SearchResponse searchResponse; @@ -423,24 +427,32 @@ public class EsEntityIndexImpl implements AliasedEntityIndex { // that you can order by: string, number and boolean and we ask ElasticSearch // to ignore any fields that are not present. - final String stringFieldName = STRING_PREFIX + sp.getPropertyName(); - final FieldSortBuilder stringSort = SortBuilders.fieldSort( stringFieldName ) - .order( order ).ignoreUnmapped( true ); - srb.addSort( stringSort ); + if(fb instanceof GeoDistanceFilterBuilder){ + srb.addSort( queryVisitor.getGeoDistanceSortBuilder().order( SortOrder.ASC ) + .unit( DistanceUnit.KILOMETERS ) + .geoDistance( GeoDistance.SLOPPY_ARC ) ); - logger.debug( " Sort: {} order by {}", stringFieldName, order.toString() ); + logger.info( " Geo Sort: {} order by {}", sp.getPropertyName(), order.toString() ); + } + else { - final String numberFieldName = NUMBER_PREFIX + sp.getPropertyName(); - final FieldSortBuilder numberSort = SortBuilders.fieldSort( numberFieldName ) - .order( order ).ignoreUnmapped( true ); - srb.addSort( numberSort ); - logger.debug( " Sort: {} order by {}", numberFieldName, order.toString() ); + final String stringFieldName = STRING_PREFIX + sp.getPropertyName(); + final FieldSortBuilder stringSort = SortBuilders.fieldSort( stringFieldName ).order( order ).ignoreUnmapped( true ); + srb.addSort( stringSort ); - final String booleanFieldName = BOOLEAN_PREFIX + sp.getPropertyName(); - final FieldSortBuilder booleanSort = SortBuilders.fieldSort( booleanFieldName ) - .order( order ).ignoreUnmapped( true ); - srb.addSort( booleanSort ); - logger.debug( " Sort: {} order by {}", booleanFieldName, order.toString() ); + logger.debug( " Sort: {} order by {}", stringFieldName, order.toString() ); + + final String numberFieldName = NUMBER_PREFIX + sp.getPropertyName(); + final FieldSortBuilder numberSort = SortBuilders.fieldSort( numberFieldName ).order( order ).ignoreUnmapped( true ); + srb.addSort( numberSort ); + logger.debug( " Sort: {} order by {}", numberFieldName, order.toString() ); + + final String booleanFieldName = BOOLEAN_PREFIX + sp.getPropertyName(); + final FieldSortBuilder booleanSort = SortBuilders.fieldSort( booleanFieldName ).order( order ) + .ignoreUnmapped( true ); + srb.addSort( booleanSort ); + logger.debug( " Sort: {} order by {}", booleanFieldName, order.toString() ); + } } http://git-wip-us.apache.org/repos/asf/incubator-usergrid/blob/4887b023/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/impl/EsQueryVistor.java ---------------------------------------------------------------------- diff --git a/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/impl/EsQueryVistor.java b/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/impl/EsQueryVistor.java index f012bab..53613b5 100644 --- a/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/impl/EsQueryVistor.java +++ b/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/impl/EsQueryVistor.java @@ -30,6 +30,8 @@ import org.elasticsearch.index.query.FilterBuilder; import org.elasticsearch.index.query.FilterBuilders; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryBuilders; +import org.elasticsearch.search.sort.GeoDistanceSortBuilder; +import org.elasticsearch.search.sort.SortBuilders; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -58,7 +60,7 @@ import static org.apache.usergrid.persistence.index.impl.IndexingUtils.STRING_PR /** - * Visits tree of parsed Query operands and populates + * Visits tree of parsed Query operands and populates * ElasticSearch QueryBuilder that represents the query. */ public class EsQueryVistor implements QueryVisitor { @@ -66,8 +68,12 @@ public class EsQueryVistor implements QueryVisitor { Stack<QueryBuilder> stack = new Stack<QueryBuilder>(); List<FilterBuilder> filterBuilders = new ArrayList<FilterBuilder>(); + Boolean geoFieldPresent = false; + + GeoDistanceSortBuilder geoSortBuilder = null; + + - @Override public void visit( AndOperand op ) throws IndexException { @@ -157,14 +163,14 @@ public class EsQueryVistor implements QueryVisitor { Object value = op.getLiteral().getValue(); BoolQueryBuilder qb = QueryBuilders.boolQuery(); // let's do a boolean OR - qb.minimumNumberShouldMatch(1); + qb.minimumNumberShouldMatch(1); // field is an entity/array that needs no name prefix qb = qb.should( QueryBuilders.matchQuery( name, value ) ); // OR field is a string and needs the prefix on the name qb = qb.should( QueryBuilders.matchQuery( addPrefix( value.toString(), name, true), value)); - + stack.push( qb ); } @@ -186,7 +192,10 @@ public class EsQueryVistor implements QueryVisitor { FilterBuilder fb = FilterBuilders.geoDistanceFilter( name ) .lat( lat ).lon( lon ).distance( distance, DistanceUnit.METERS ); filterBuilders.add( fb ); - } + + + geoSortBuilder = SortBuilders.geoDistanceSort( GEO_PREFIX + "location" ).point( lat,lon ); + } @Override @@ -222,19 +231,19 @@ public class EsQueryVistor implements QueryVisitor { qb.minimumNumberShouldMatch(1); // field is an entity/array that does not need a prefix on its name - // TODO is this right now that we've updated our doc structure? + // TODO is this right now that we've updated our doc structure? // Should this be "must" instead of should? qb = qb.should( QueryBuilders.wildcardQuery( name, svalue ) ); - + // or field is just a string that does need a prefix if ( svalue.indexOf("*") != -1 ) { qb = qb.should( QueryBuilders.wildcardQuery( addPrefix( value, name ), svalue ) ); } else { qb = qb.should( QueryBuilders.termQuery( addPrefix( value, name ), value )); - } + } stack.push( qb ); return; - } + } // assume all other types need prefix stack.push( QueryBuilders.termQuery( addPrefix( value, name ), value )); @@ -276,7 +285,7 @@ public class EsQueryVistor implements QueryVisitor { if ( parts.length > 1 ) { name = parts[ parts.length - 1 ]; } - + if ( value instanceof String && analyzed ) { name = addAnalyzedStringPrefix( name ); @@ -293,12 +302,12 @@ public class EsQueryVistor implements QueryVisitor { name = addStringPrefix( name ); } - // re-create nested property name + // re-create nested property name if ( parts.length > 1 ) { parts[parts.length - 1] = name; Joiner joiner = Joiner.on(".").skipNulls(); return joiner.join(parts); - } + } return name; } @@ -307,34 +316,34 @@ public class EsQueryVistor implements QueryVisitor { private String addAnalyzedStringPrefix( String name ) { if ( name.startsWith( ANALYZED_STRING_PREFIX ) ) { return name; - } + } return ANALYZED_STRING_PREFIX + name; - } - + } + private String addStringPrefix( String name ) { if ( name.startsWith( STRING_PREFIX ) ) { return name; - } + } return STRING_PREFIX + name; - } - + } + private String addNumberPrefix( String name ) { if ( name.startsWith( NUMBER_PREFIX ) ) { return name; - } + } return NUMBER_PREFIX + name; - } - + } + private String addBooleanPrefix( String name ) { if ( name.startsWith( BOOLEAN_PREFIX ) ) { return name; - } + } return BOOLEAN_PREFIX + name; - } - + } + @Override public QueryBuilder getQueryBuilder() { @@ -354,14 +363,19 @@ public class EsQueryVistor implements QueryVisitor { for ( FilterBuilder fb : filterBuilders ) { if ( andFilter == null ) { andFilter = FilterBuilders.andFilter( fb ); - } else { + } else { andFilter = FilterBuilders.andFilter( andFilter, fb ); } - } + } } else if ( !filterBuilders.isEmpty() ) { return filterBuilders.get(0); } return null; } + + @Override + public GeoDistanceSortBuilder getGeoDistanceSortBuilder(){ + return geoSortBuilder; + } } http://git-wip-us.apache.org/repos/asf/incubator-usergrid/blob/4887b023/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/query/Query.java ---------------------------------------------------------------------- diff --git a/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/query/Query.java b/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/query/Query.java index 9a7a867..c0361fc 100644 --- a/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/query/Query.java +++ b/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/query/Query.java @@ -213,6 +213,22 @@ public class Query { return filterBuilder; } + public QueryVisitor getQueryVisitor() { + + if ( getRootOperand() != null ) { + QueryVisitor v = new EsQueryVistor(); + try { + getRootOperand().visit( v ); + + } catch ( IndexException ex ) { + throw new RuntimeException( "Error building ElasticSearch query", ex ); + } + return v; + } + + return null; + } + /** * Create a query instance from the QL. If the string is null, return an empty query http://git-wip-us.apache.org/repos/asf/incubator-usergrid/blob/4887b023/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/query/tree/QueryVisitor.java ---------------------------------------------------------------------- diff --git a/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/query/tree/QueryVisitor.java b/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/query/tree/QueryVisitor.java index d295c92..10203ad 100644 --- a/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/query/tree/QueryVisitor.java +++ b/stack/corepersistence/queryindex/src/main/java/org/apache/usergrid/persistence/index/query/tree/QueryVisitor.java @@ -24,6 +24,7 @@ import org.apache.usergrid.persistence.index.exceptions.NoIndexException; import org.apache.usergrid.persistence.index.exceptions.IndexException; import org.elasticsearch.index.query.FilterBuilder; import org.elasticsearch.index.query.QueryBuilder; +import org.elasticsearch.search.sort.GeoDistanceSortBuilder; /** @@ -93,10 +94,15 @@ public interface QueryVisitor { */ public void visit( GreaterThanEqual op ) throws NoIndexException; - /** + /** * Returns resulting query builder. */ public QueryBuilder getQueryBuilder(); - public FilterBuilder getFilterBuilder(); + public FilterBuilder getFilterBuilder(); + + /** + * Returns resulting geo distance sort builder. + */ + public GeoDistanceSortBuilder getGeoDistanceSortBuilder(); } http://git-wip-us.apache.org/repos/asf/incubator-usergrid/blob/4887b023/stack/rest/src/test/java/org/apache/usergrid/rest/applications/queries/GeoPagingTest.java ---------------------------------------------------------------------- diff --git a/stack/rest/src/test/java/org/apache/usergrid/rest/applications/queries/GeoPagingTest.java b/stack/rest/src/test/java/org/apache/usergrid/rest/applications/queries/GeoPagingTest.java index f9fe5b3..7bab8e2 100644 --- a/stack/rest/src/test/java/org/apache/usergrid/rest/applications/queries/GeoPagingTest.java +++ b/stack/rest/src/test/java/org/apache/usergrid/rest/applications/queries/GeoPagingTest.java @@ -289,4 +289,47 @@ public class GeoPagingTest extends AbstractRestIT { assertEquals("Expected 0 results", 0, collection.getResponse().getEntityCount()); } } + + + /** + * Test that geo-query returns co-located entities in expected order. + */ + @Test + public void groupQueriesWithDistanceOrderedResults() throws IOException { + + int maxRangeLimit = 9; + Entity[] cats = new Entity[maxRangeLimit+1]; + + // 1. Create several entities + for (int i = maxRangeLimit; i >= 0; i--) { + Entity cat = new Entity(); + cat.put("name", "cat" + i); + cat.put( "location", + new MapUtils.HashMapBuilder<String, Double>().map( "latitude", 37.0 + i ) + .map( "longitude", ( -75.0 + i) ) ); + cats[i] = cat; + this.app().collection("cats").post(cat); + } + this.refreshIndex(); + + QueryParameters params = new QueryParameters(); + String query = String.format( + "location within 15000000 of 37,-75"); + params.setQuery(query); + Collection collection = this.app().collection("cats").get(params); + assertNotNull( collection ); + List entities = collection.getResponse().getEntities(); + assertNotNull( entities ); + + for (int consistent = 0; consistent < maxRangeLimit; consistent++) { + //got entities back, just need to page through them and make sure that i got them in location order. + Entity entity = (Entity) entities.get( consistent ); + assertNotNull( entity ); + LinkedHashMap location = ( LinkedHashMap ) entity.get( "location" ); + assertEquals( 37.0+ consistent,location.get( "latitude" ) ); + assertEquals( -75.0+ consistent , location.get( "longitude" ) ); + + } + } + }
