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

@@ -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 := thingTyp.Field(i).Name
-			fieldName := field.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()

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)
 }

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

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 {

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 {
+		filterOnBoundarySearcher = NewFilteringSearcher(onBoundarySearcher,
-			var lon, lat float64
+			func(d *search.DocumentMatch) bool {
-			var found bool
+				var lon, lat float64
-			err = indexReader.DocumentVisitFieldTerms(d.IndexInternalID, []string{field}, func(field string, term []byte) {
+				var found bool
-				// only consider the values which are shifted 0
+				err = indexReader.DocumentVisitFieldTerms(d.IndexInternalID,
-				prefixCoded := numeric.PrefixCoded(term)
+					[]string{field}, func(field string, term []byte) {
-				var shift uint
+						// only consider the values which are shifted 0
-				shift, err = prefixCoded.Shift()
+						prefixCoded := numeric.PrefixCoded(term)
-				if err == nil && shift == 0 {
+						var shift uint
-					var i64 int64
+						shift, err = prefixCoded.Shift()
-					i64, err = prefixCoded.Int64()
+						if err == nil && shift == 0 {
-					if err == nil {
+							var i64 int64
-						lon = geo.MortonUnhashLon(uint64(i64))
+							i64, err = prefixCoded.Int64()
-						lat = geo.MortonUnhashLat(uint64(i64))
+							if err == nil {
-						found = true
+								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)
+	within := res%document.GeoPrecisionStep == 0 &&
-	if within || (level == geoDetailLevel && geo.RectIntersects(minLon, minLat, maxLon, maxLat, s.minLon, s.minLat, s.maxLon, s.maxLat)) {
+		geo.RectWithin(minLon, minLat, maxLon, maxLat,
-		s.rangeBounds = append(s.rangeBounds, newGeoRange(start, res, level, !within))
+			s.minLon, s.minLat, s.maxLon, s.maxLat)
-	} else if level < geoDetailLevel && geo.RectIntersects(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)
 	}
 }

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)

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 {
+	rv.searcher = NewFilteringSearcher(boxSearcher,
-		var lon, lat float64
+		func(d *search.DocumentMatch) bool {
-		var found bool
+			var lon, lat float64
-		err := indexReader.DocumentVisitFieldTerms(d.IndexInternalID, []string{field}, func(field string, term []byte) {
+			var found bool
-			// only consider the values which are shifted 0
+			err := indexReader.DocumentVisitFieldTerms(d.IndexInternalID,
-			prefixCoded := numeric.PrefixCoded(term)
+				[]string{field}, func(field string, term []byte) {
-			shift, err := prefixCoded.Shift()
+					// only consider the values which are shifted 0
-			if err == nil && shift == 0 {
+					prefixCoded := numeric.PrefixCoded(term)
-				i64, err := prefixCoded.Int64()
+					shift, err := prefixCoded.Shift()
-				if err == nil {
+					if err == nil && shift == 0 {
-					lon = geo.MortonUnhashLon(uint64(i64))
+						i64, err := prefixCoded.Int64()
-					lat = geo.MortonUnhashLat(uint64(i64))
+						if err == nil {
-					found = true
+							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)
 }