From 6554e9624f25289a20aacc3509861a6496fbf28d Mon Sep 17 00:00:00 2001 From: Marty Schoch Date: Fri, 31 Mar 2017 08:41:40 -0400 Subject: [PATCH] geo review comments from sreekanth also one fix came from steve, i must have forgotten to push that commit up before merging --- geo/parse.go | 6 +- query.go | 4 +- search/query/geo_boundingbox.go | 2 +- search/query/geo_distance.go | 6 +- search/searcher/search_geoboundingbox.go | 84 ++++++++++++------- search/searcher/search_geoboundingbox_test.go | 4 + search/searcher/search_geopointdistance.go | 71 +++++++++------- 7 files changed, 107 insertions(+), 70 deletions(-) diff --git a/geo/parse.go b/geo/parse.go index 92bd42c2..04a57538 100644 --- a/geo/parse.go +++ b/geo/parse.go @@ -65,14 +65,12 @@ func ExtractGeoPoint(thing interface{}) (lon, lat float64, success bool) { if lval, ok := l["lat"]; ok { lat, foundLat = extractNumericVal(lval) } - return lon, lat, foundLon && foundLat } // now try reflection on struct fields if thingVal.IsValid() && thingVal.Kind() == reflect.Struct { for i := 0; i < thingVal.NumField(); i++ { - field := thingTyp.Field(i) - fieldName := field.Name + fieldName := thingTyp.Field(i).Name if strings.HasPrefix(strings.ToLower(fieldName), "lon") { if thingVal.Field(i).CanInterface() { fieldVal := thingVal.Field(i).Interface() @@ -112,7 +110,7 @@ func ExtractGeoPoint(thing interface{}) (lon, lat float64, success bool) { return lon, lat, foundLon && foundLat } -// extract numeric value (if possible) and returna s float64 +// extract numeric value (if possible) and returns a float64 func extractNumericVal(v interface{}) (float64, bool) { val := reflect.ValueOf(v) typ := val.Type() diff --git a/query.go b/query.go index 5c6fdae0..4311c994 100644 --- a/query.go +++ b/query.go @@ -193,9 +193,9 @@ func NewGeoBoundingBoxQuery(topLeftLon, topLeftLat, bottomRightLon, bottomRightL } // NewGeoDistanceQuery creates a new Query for performing geo bounding -// box searches. The arguments describe a position and a distance. Docuements +// box searches. The arguments describe a position and a distance. Documents // which have an indexed geo point which is less than or equal to the provided -// distance will be returned. +// distance from the given position will be returned. func NewGeoDistanceQuery(lon, lat float64, distance string) *query.GeoDistanceQuery { return query.NewGeoDistanceQuery(lon, lat, distance) } diff --git a/search/query/geo_boundingbox.go b/search/query/geo_boundingbox.go index ee2b7883..de6be4a5 100644 --- a/search/query/geo_boundingbox.go +++ b/search/query/geo_boundingbox.go @@ -104,7 +104,7 @@ func (q *GeoBoundingBoxQuery) UnmarshalJSON(data []byte) error { q.TopLeft = []float64{lon, lat} lon, lat, found = geo.ExtractGeoPoint(tmp.BottomRight) if !found { - return fmt.Errorf("geo location top_left not in a valid format") + return fmt.Errorf("geo location bottom_right not in a valid format") } q.BottomRight = []float64{lon, lat} q.FieldVal = tmp.FieldVal diff --git a/search/query/geo_distance.go b/search/query/geo_distance.go index f257d433..ef3aa88c 100644 --- a/search/query/geo_distance.go +++ b/search/query/geo_distance.go @@ -56,7 +56,8 @@ func (q *GeoDistanceQuery) Field() string { return q.FieldVal } -func (q *GeoDistanceQuery) Searcher(i index.IndexReader, m mapping.IndexMapping, options search.SearcherOptions) (search.Searcher, error) { +func (q *GeoDistanceQuery) Searcher(i index.IndexReader, m mapping.IndexMapping, + options search.SearcherOptions) (search.Searcher, error) { field := q.FieldVal if q.FieldVal == "" { field = m.DefaultSearchField() @@ -67,7 +68,8 @@ func (q *GeoDistanceQuery) Searcher(i index.IndexReader, m mapping.IndexMapping, return nil, err } - return searcher.NewGeoPointDistanceSearcher(i, q.Location[0], q.Location[1], dist, field, q.BoostVal.Value(), options) + return searcher.NewGeoPointDistanceSearcher(i, q.Location[0], q.Location[1], + dist, field, q.BoostVal.Value(), options) } func (q *GeoDistanceQuery) Validate() error { diff --git a/search/searcher/search_geoboundingbox.go b/search/searcher/search_geoboundingbox.go index a7dcc62a..691b1a03 100644 --- a/search/searcher/search_geoboundingbox.go +++ b/search/searcher/search_geoboundingbox.go @@ -36,7 +36,10 @@ type GeoBoundingBoxSearcher struct { searcher *DisjunctionSearcher } -func NewGeoBoundingBoxSearcher(indexReader index.IndexReader, minLon, minLat, maxLon, maxLat float64, field string, boost float64, options search.SearcherOptions, checkBoundaries bool) (*GeoBoundingBoxSearcher, error) { +func NewGeoBoundingBoxSearcher(indexReader index.IndexReader, minLon, minLat, + maxLon, maxLat float64, field string, boost float64, + options search.SearcherOptions, checkBoundaries bool) ( + *GeoBoundingBoxSearcher, error) { var openedSearchers []search.Searcher cleanupOpenedSearchers := func() { for _, s := range openedSearchers { @@ -72,49 +75,57 @@ func NewGeoBoundingBoxSearcher(indexReader index.IndexReader, minLon, minLat, ma var filterOnBoundarySearcher search.Searcher if len(termsOnBoundary) > 0 { - onBoundarySearcher, err := NewDisjunctionSearcher(indexReader, termsOnBoundary, 0, options) + onBoundarySearcher, err := NewDisjunctionSearcher(indexReader, + termsOnBoundary, 0, options) if err != nil { cleanupOpenedSearchers() return nil, err } - filterOnBoundarySearcher = NewFilteringSearcher(onBoundarySearcher, func(d *search.DocumentMatch) bool { - var lon, lat float64 - var found bool - err = indexReader.DocumentVisitFieldTerms(d.IndexInternalID, []string{field}, func(field string, term []byte) { - // only consider the values which are shifted 0 - prefixCoded := numeric.PrefixCoded(term) - var shift uint - shift, err = prefixCoded.Shift() - if err == nil && shift == 0 { - var i64 int64 - i64, err = prefixCoded.Int64() - if err == nil { - lon = geo.MortonUnhashLon(uint64(i64)) - lat = geo.MortonUnhashLat(uint64(i64)) - found = true - } + filterOnBoundarySearcher = NewFilteringSearcher(onBoundarySearcher, + func(d *search.DocumentMatch) bool { + var lon, lat float64 + var found bool + err = indexReader.DocumentVisitFieldTerms(d.IndexInternalID, + []string{field}, func(field string, term []byte) { + // only consider the values which are shifted 0 + prefixCoded := numeric.PrefixCoded(term) + var shift uint + shift, err = prefixCoded.Shift() + if err == nil && shift == 0 { + var i64 int64 + i64, err = prefixCoded.Int64() + if err == nil { + lon = geo.MortonUnhashLon(uint64(i64)) + lat = geo.MortonUnhashLat(uint64(i64)) + found = true + } + } + }) + if err == nil && found { + return geo.BoundingBoxContains(lon, lat, + minLon, minLat, maxLon, maxLat) } + return false }) - if err == nil && found { - return geo.BoundingBoxContains(lon, lat, minLon, minLat, maxLon, maxLat) - } - return false - }) openedSearchers = append(openedSearchers, filterOnBoundarySearcher) } - notOnBoundarySearcher, err := NewDisjunctionSearcher(indexReader, termsNotOnBoundary, 0, options) + notOnBoundarySearcher, err := NewDisjunctionSearcher(indexReader, + termsNotOnBoundary, 0, options) if err != nil { cleanupOpenedSearchers() return nil, err } openedSearchers = append(openedSearchers, notOnBoundarySearcher) - // if there is no filterOnBoundary searcher, just return the notOnBoundarySearcher + // if there is no filterOnBoundary searcher, + // just return the notOnBoundarySearcher if filterOnBoundarySearcher == nil { rv.searcher = notOnBoundarySearcher return rv, nil } - rv.searcher, err = NewDisjunctionSearcher(indexReader, []search.Searcher{filterOnBoundarySearcher, notOnBoundarySearcher}, 0, options) + rv.searcher, err = NewDisjunctionSearcher(indexReader, + []search.Searcher{filterOnBoundarySearcher, notOnBoundarySearcher}, + 0, options) if err != nil { cleanupOpenedSearchers() return nil, err @@ -134,11 +145,13 @@ func (s *GeoBoundingBoxSearcher) SetQueryNorm(qnorm float64) { s.searcher.SetQueryNorm(qnorm) } -func (s *GeoBoundingBoxSearcher) Next(ctx *search.SearchContext) (*search.DocumentMatch, error) { +func (s *GeoBoundingBoxSearcher) Next(ctx *search.SearchContext) ( + *search.DocumentMatch, error) { return s.searcher.Next(ctx) } -func (s *GeoBoundingBoxSearcher) Advance(ctx *search.SearchContext, ID index.IndexInternalID) (*search.DocumentMatch, error) { +func (s *GeoBoundingBoxSearcher) Advance(ctx *search.SearchContext, + ID index.IndexInternalID) (*search.DocumentMatch, error) { return s.searcher.Advance(ctx, ID) } @@ -178,10 +191,17 @@ func (s *GeoBoundingBoxSearcher) relateAndRecurse(start, end uint64, res uint) { level := ((geo.GeoBits << 1) - res) >> 1 - within := res%document.GeoPrecisionStep == 0 && geo.RectWithin(minLon, minLat, maxLon, maxLat, s.minLon, s.minLat, s.maxLon, s.maxLat) - if within || (level == geoDetailLevel && geo.RectIntersects(minLon, minLat, maxLon, maxLat, s.minLon, s.minLat, s.maxLon, s.maxLat)) { - s.rangeBounds = append(s.rangeBounds, newGeoRange(start, res, level, !within)) - } else if level < geoDetailLevel && geo.RectIntersects(minLon, minLat, maxLon, maxLat, s.minLon, s.minLat, s.maxLon, s.maxLat) { + within := res%document.GeoPrecisionStep == 0 && + geo.RectWithin(minLon, minLat, maxLon, maxLat, + s.minLon, s.minLat, s.maxLon, s.maxLat) + if within || (level == geoDetailLevel && + geo.RectIntersects(minLon, minLat, maxLon, maxLat, + s.minLon, s.minLat, s.maxLon, s.maxLat)) { + s.rangeBounds = append(s.rangeBounds, + newGeoRange(start, res, level, !within)) + } else if level < geoDetailLevel && + geo.RectIntersects(minLon, minLat, maxLon, maxLat, + s.minLon, s.minLat, s.maxLon, s.maxLat) { s.computeRange(start, res-1) } } diff --git a/search/searcher/search_geoboundingbox_test.go b/search/searcher/search_geoboundingbox_test.go index a93dc298..1de938db 100644 --- a/search/searcher/search_geoboundingbox_test.go +++ b/search/searcher/search_geoboundingbox_test.go @@ -39,6 +39,10 @@ func TestGeoBoundingBox(t *testing.T) { {0.001, 0.001, 0.002, 0.002, "loc", []string{"a"}}, {0.001, 0.001, 1.002, 1.002, "loc", []string{"a", "b"}}, {0.001, 0.001, 9.002, 9.002, "loc", []string{"a", "b", "c", "d", "e", "f", "g", "h", "i", "j"}}, + // same upper-left, bottom-right point + {25, 25, 25, 25, "loc", nil}, + // box that would return points, but points reversed + {0.002, 0.002, 0.001, 0.001, "loc", nil}, } i := setupGeo(t) diff --git a/search/searcher/search_geopointdistance.go b/search/searcher/search_geopointdistance.go index 18442798..adb3f74e 100644 --- a/search/searcher/search_geopointdistance.go +++ b/search/searcher/search_geopointdistance.go @@ -34,7 +34,9 @@ type GeoPointDistanceSearcher struct { searcher *FilteringSearcher } -func NewGeoPointDistanceSearcher(indexReader index.IndexReader, centerLon, centerLat, dist float64, field string, boost float64, options search.SearcherOptions) (*GeoPointDistanceSearcher, error) { +func NewGeoPointDistanceSearcher(indexReader index.IndexReader, centerLon, + centerLat, dist float64, field string, boost float64, + options search.SearcherOptions) (*GeoPointDistanceSearcher, error) { rv := &GeoPointDistanceSearcher{ indexReader: indexReader, centerLon: centerLon, @@ -45,23 +47,28 @@ func NewGeoPointDistanceSearcher(indexReader index.IndexReader, centerLon, cente } // compute bounding box containing the circle - topLeftLon, topLeftLat, bottomRightLon, bottomRightLat := geo.ComputeBoundingBox(centerLon, centerLat, dist) + topLeftLon, topLeftLat, bottomRightLon, bottomRightLat := + geo.ComputeBoundingBox(centerLon, centerLat, dist) var boxSearcher search.Searcher if bottomRightLon < topLeftLon { // cross date line, rewrite as two parts - leftSearcher, err := NewGeoBoundingBoxSearcher(indexReader, -180, bottomRightLat, bottomRightLon, topLeftLat, field, boost, options, false) + leftSearcher, err := NewGeoBoundingBoxSearcher(indexReader, + -180, bottomRightLat, bottomRightLon, topLeftLat, + field, boost, options, false) if err != nil { return nil, err } - rightSearcher, err := NewGeoBoundingBoxSearcher(indexReader, topLeftLon, bottomRightLat, 180, topLeftLat, field, boost, options, false) + rightSearcher, err := NewGeoBoundingBoxSearcher(indexReader, + topLeftLon, bottomRightLat, 180, topLeftLat, field, boost, options, false) if err != nil { _ = leftSearcher.Close() return nil, err } - boxSearcher, err = NewDisjunctionSearcher(indexReader, []search.Searcher{leftSearcher, rightSearcher}, 0, options) + boxSearcher, err = NewDisjunctionSearcher(indexReader, + []search.Searcher{leftSearcher, rightSearcher}, 0, options) if err != nil { _ = leftSearcher.Close() _ = rightSearcher.Close() @@ -71,37 +78,41 @@ func NewGeoPointDistanceSearcher(indexReader index.IndexReader, centerLon, cente // build geoboundinggox searcher for that bounding box var err error - boxSearcher, err = NewGeoBoundingBoxSearcher(indexReader, topLeftLon, bottomRightLat, bottomRightLon, topLeftLat, field, boost, options, false) + boxSearcher, err = NewGeoBoundingBoxSearcher(indexReader, + topLeftLon, bottomRightLat, bottomRightLon, topLeftLat, field, boost, + options, false) if err != nil { return nil, err } } // wrap it in a filtering searcher which checks the actual distance - rv.searcher = NewFilteringSearcher(boxSearcher, func(d *search.DocumentMatch) bool { - var lon, lat float64 - var found bool - err := indexReader.DocumentVisitFieldTerms(d.IndexInternalID, []string{field}, func(field string, term []byte) { - // only consider the values which are shifted 0 - prefixCoded := numeric.PrefixCoded(term) - shift, err := prefixCoded.Shift() - if err == nil && shift == 0 { - i64, err := prefixCoded.Int64() - if err == nil { - lon = geo.MortonUnhashLon(uint64(i64)) - lat = geo.MortonUnhashLat(uint64(i64)) - found = true + rv.searcher = NewFilteringSearcher(boxSearcher, + func(d *search.DocumentMatch) bool { + var lon, lat float64 + var found bool + err := indexReader.DocumentVisitFieldTerms(d.IndexInternalID, + []string{field}, func(field string, term []byte) { + // only consider the values which are shifted 0 + prefixCoded := numeric.PrefixCoded(term) + shift, err := prefixCoded.Shift() + if err == nil && shift == 0 { + i64, err := prefixCoded.Int64() + if err == nil { + lon = geo.MortonUnhashLon(uint64(i64)) + lat = geo.MortonUnhashLat(uint64(i64)) + found = true + } + } + }) + if err == nil && found { + dist := geo.Haversin(lon, lat, rv.centerLon, rv.centerLat) + if dist <= rv.dist/1000 { + return true } } + return false }) - if err == nil && found { - dist := geo.Haversin(lon, lat, rv.centerLon, rv.centerLat) - if dist <= rv.dist/1000 { - return true - } - } - return false - }) return rv, nil } @@ -118,11 +129,13 @@ func (s *GeoPointDistanceSearcher) SetQueryNorm(qnorm float64) { s.searcher.SetQueryNorm(qnorm) } -func (s *GeoPointDistanceSearcher) Next(ctx *search.SearchContext) (*search.DocumentMatch, error) { +func (s *GeoPointDistanceSearcher) Next(ctx *search.SearchContext) ( + *search.DocumentMatch, error) { return s.searcher.Next(ctx) } -func (s *GeoPointDistanceSearcher) Advance(ctx *search.SearchContext, ID index.IndexInternalID) (*search.DocumentMatch, error) { +func (s *GeoPointDistanceSearcher) Advance(ctx *search.SearchContext, + ID index.IndexInternalID) (*search.DocumentMatch, error) { return s.searcher.Advance(ctx, ID) }