Skip to content

Commit

Permalink
Merge #50790 #50962 #50992
Browse files Browse the repository at this point in the history 
50790: geodist: use Point struct instead of a Point interface r=sumeerbhola a=otan


Getting an Edge is expensive as the generating a Point that complies
with the geodist.Point interface involves mallocing from memory. We call
Edge(x) a lot when doing POLYGON <=> POLYGON comparisons.

Instead, change the Point to a struct, which does not require
any malloc to generate interfaces at the cost of code ugliness.

Without bounding box checks, this speeds up a query from 2min to
1min30sec on a ~120000 row join.

Release note: None



50962: util/mon: don't crash when shrinking by too much r=yuzefovich a=yuzefovich

Previously, we called `panic` when `Shrink`ing the memory account by
too much. Such scenario indicates that our memory accounting is off,
but we shouldn't be crashing - we should report an error instead, and
this commit does that.

Fixes: #50804.

Release note (bug fix): Previously, CockroachDB could crash when
internal memory accounting hit a discrepancy. Now it will report an
error instead.

50992: opt: fix error in case of NULL arguments to AddGeometryColumn r=yuzefovich a=rytaft

This commit fixes an error that occurred when `AddGeometryColumn` was
called with `NULL` arguments. The error was caused by an assertion that
the output of `TypeCheck` was a `tree.FuncExpr` when in fact it was
`tree.DNull`.

This commit fixes the error by adding an explicit check for `tree.DNull`
after calling `TypeCheck`.

Fixes #50296

Release note (bug fix): Fixed an internal error that occurred when
AddGeometryColumn was called with NULL arguments. This now results in
a no-op and returns NULL.

Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Rebecca Taft <[email protected]>
  • Loading branch information
@otan @yuzefovich @rytaft
4 people committed Jul 6, 2020
4 parents fb6e129 + 82fb8f2 + 5d15763 + 63fd54f commit 20615e5
Show file tree
Hide file tree
Showing 6 changed files with 103 additions and 104 deletions.
40 changes: 25 additions & 15 deletions pkg/geo/geodist/geodist.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,24 @@
// Package geodist finds distances between two geospatial shapes.
package geodist

import "github.com/cockroachdb/errors"
import (
"github.com/cockroachdb/errors"
"github.com/golang/geo/s2"
"github.com/twpayne/go-geom"
)

// Point is an interface that represents a geospatial Point.
type Point interface {
IsShape()
IsPoint()
// Point is a union of the point types used in geometry and geography representation.
// The interfaces for distance calculation defined below are shared for both representations,
// and this union helps us avoid heap allocations by doing cheap copy-by-value of points. The
// code that peers inside a Point knows which of the two fields is populated.
type Point struct {
GeomPoint geom.Coord
GeogPoint s2.Point
}

// IsShape implements the geodist.Shape interface.
func (p *Point) IsShape() {}

// Edge is a struct that represents a connection between two points.
type Edge struct {
V0, V1 Point
Expand Down Expand Up @@ -63,7 +73,7 @@ type Shape interface {
IsShape()
}

var _ Shape = (Point)(nil)
var _ Shape = (*Point)(nil)
var _ Shape = (LineString)(nil)
var _ Shape = (LinearRing)(nil)
var _ Shape = (Polygon)(nil)
Expand Down Expand Up @@ -122,24 +132,24 @@ type DistanceCalculator interface {
// It returns whether the function above should return early.
func ShapeDistance(c DistanceCalculator, a Shape, b Shape) (bool, error) {
switch a := a.(type) {
case Point:
case *Point:
switch b := b.(type) {
case Point:
return c.DistanceUpdater().Update(a, b), nil
case *Point:
return c.DistanceUpdater().Update(*a, *b), nil
case LineString:
return onPointToLineString(c, a, b), nil
return onPointToLineString(c, *a, b), nil
case Polygon:
return onPointToPolygon(c, a, b), nil
return onPointToPolygon(c, *a, b), nil
default:
return false, errors.Newf("unknown shape: %T", b)
}
case LineString:
switch b := b.(type) {
case Point:
case *Point:
c.DistanceUpdater().FlipGeometries()
// defer to restore the order of geometries at the end of the function call.
defer c.DistanceUpdater().FlipGeometries()
return onPointToLineString(c, b, a), nil
return onPointToLineString(c, *b, a), nil
case LineString:
return onShapeEdgesToShapeEdges(c, a, b), nil
case Polygon:
Expand All @@ -149,11 +159,11 @@ func ShapeDistance(c DistanceCalculator, a Shape, b Shape) (bool, error) {
}
case Polygon:
switch b := b.(type) {
case Point:
case *Point:
c.DistanceUpdater().FlipGeometries()
// defer to restore the order of geometries at the end of the function call.
defer c.DistanceUpdater().FlipGeometries()
return onPointToPolygon(c, b, a), nil
return onPointToPolygon(c, *b, a), nil
case LineString:
c.DistanceUpdater().FlipGeometries()
// defer to restore the order of geometries at the end of the function call.
Expand Down
64 changes: 29 additions & 35 deletions pkg/geo/geogfn/distance.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,19 +55,6 @@ func Distance(
// Spheroids
//

// s2GeodistPoint implements geodist.Point.
type s2GeodistPoint struct {
s2.Point
}

var _ geodist.Point = (*s2GeodistPoint)(nil)

// IsShape implements the geodist.Point interface.
func (*s2GeodistPoint) IsShape() {}

// Point implements the geodist.Point interface.
func (*s2GeodistPoint) IsPoint() {}

// s2GeodistLineString implements geodist.LineString.
type s2GeodistLineString struct {
*s2.Polyline
Expand All @@ -83,7 +70,10 @@ func (*s2GeodistLineString) IsLineString() {}

// Edge implements the geodist.LineString interface.
func (g *s2GeodistLineString) Edge(i int) geodist.Edge {
return geodist.Edge{V0: &s2GeodistPoint{Point: (*g.Polyline)[i]}, V1: &s2GeodistPoint{Point: (*g.Polyline)[i+1]}}
return geodist.Edge{
V0: geodist.Point{GeogPoint: (*g.Polyline)[i]},
V1: geodist.Point{GeogPoint: (*g.Polyline)[i+1]},
}
}

// NumEdges implements the geodist.LineString interface.
Expand All @@ -93,7 +83,9 @@ func (g *s2GeodistLineString) NumEdges() int {

// Vertex implements the geodist.LineString interface.
func (g *s2GeodistLineString) Vertex(i int) geodist.Point {
return &s2GeodistPoint{Point: (*g.Polyline)[i]}
return geodist.Point{
GeogPoint: (*g.Polyline)[i],
}
}

// NumVertexes implements the geodist.LineString interface.
Expand All @@ -116,7 +108,10 @@ func (*s2GeodistLinearRing) IsLinearRing() {}

// Edge implements the geodist.LinearRing interface.
func (g *s2GeodistLinearRing) Edge(i int) geodist.Edge {
return geodist.Edge{V0: &s2GeodistPoint{Point: g.Loop.Vertex(i)}, V1: &s2GeodistPoint{Point: g.Loop.Vertex(i + 1)}}
return geodist.Edge{
V0: geodist.Point{GeogPoint: g.Loop.Vertex(i)},
V1: geodist.Point{GeogPoint: g.Loop.Vertex(i + 1)},
}
}

// NumEdges implements the geodist.LinearRing interface.
Expand All @@ -126,7 +121,9 @@ func (g *s2GeodistLinearRing) NumEdges() int {

// Vertex implements the geodist.LinearRing interface.
func (g *s2GeodistLinearRing) Vertex(i int) geodist.Point {
return &s2GeodistPoint{Point: g.Loop.Vertex(i)}
return geodist.Point{
GeogPoint: g.Loop.Vertex(i),
}
}

// NumVertexes implements the geodist.LinearRing interface.
Expand Down Expand Up @@ -168,7 +165,7 @@ var _ geodist.EdgeCrosser = (*s2GeodistEdgeCrosser)(nil)
func (c *s2GeodistEdgeCrosser) ChainCrossing(p geodist.Point) (bool, geodist.Point) {
// Returns nil for the intersection point as we don't require the intersection
// point as we do not have to implement ShortestLine in geography.
return c.EdgeCrosser.ChainCrossingSign(p.(*s2GeodistPoint).Point) != s2.DoNotCross, nil
return c.EdgeCrosser.ChainCrossingSign(p.GeogPoint) != s2.DoNotCross, geodist.Point{}
}

// distanceGeographyRegions calculates the distance between two sets of regions.
Expand Down Expand Up @@ -269,11 +266,9 @@ func (u *geographyMinDistanceUpdater) Distance() float64 {
}

// Update implements the geodist.DistanceUpdater interface.
func (u *geographyMinDistanceUpdater) Update(
aInterface geodist.Point, bInterface geodist.Point,
) bool {
a := aInterface.(*s2GeodistPoint).Point
b := bInterface.(*s2GeodistPoint).Point
func (u *geographyMinDistanceUpdater) Update(aPoint geodist.Point, bPoint geodist.Point) bool {
a := aPoint.GeogPoint
b := bPoint.GeogPoint

sphereDistance := s2.ChordAngleBetweenPoints(a, b)
if sphereDistance < u.minD {
Expand Down Expand Up @@ -331,9 +326,9 @@ func (c *geographyDistanceCalculator) NewEdgeCrosser(
) geodist.EdgeCrosser {
return &s2GeodistEdgeCrosser{
EdgeCrosser: s2.NewChainEdgeCrosser(
edge.V0.(*s2GeodistPoint).Point,
edge.V1.(*s2GeodistPoint).Point,
startPoint.(*s2GeodistPoint).Point,
edge.V0.GeogPoint,
edge.V1.GeogPoint,
startPoint.GeogPoint,
),
}
}
Expand All @@ -342,7 +337,7 @@ func (c *geographyDistanceCalculator) NewEdgeCrosser(
func (c *geographyDistanceCalculator) PointInLinearRing(
point geodist.Point, polygon geodist.LinearRing,
) bool {
return polygon.(*s2GeodistLinearRing).ContainsPoint(point.(*s2GeodistPoint).Point)
return polygon.(*s2GeodistLinearRing).ContainsPoint(point.GeogPoint)
}

// ClosestPointToEdge implements geodist.DistanceCalculator.
Expand All @@ -355,11 +350,10 @@ func (c *geographyDistanceCalculator) PointInLinearRing(
// For visualization and more, see: Section 6 / Figure 4 of
// "Projective configuration theorems: old wine into new wineskins", Tabachnikov, Serge, 2016/07/16
func (c *geographyDistanceCalculator) ClosestPointToEdge(
edge geodist.Edge, pointInterface geodist.Point,
edge geodist.Edge, point geodist.Point,
) (geodist.Point, bool) {
eV0 := edge.V0.(*s2GeodistPoint).Point
eV1 := edge.V1.(*s2GeodistPoint).Point
point := pointInterface.(*s2GeodistPoint).Point
eV0 := edge.V0.GeogPoint
eV1 := edge.V1.GeogPoint

// Project the point onto the normal of the edge. A great circle passing through
// the normal and the point will intersect with the great circle represented
Expand All @@ -368,21 +362,21 @@ func (c *geographyDistanceCalculator) ClosestPointToEdge(
// To find the point where the great circle represented by the edge and the
// great circle represented by (normal, point), we project the point
// onto the normal.
normalScaledToPoint := normal.Mul(normal.Dot(point.Vector))
normalScaledToPoint := normal.Mul(normal.Dot(point.GeogPoint.Vector))
// The difference between the point and the projection of the normal when normalized
// should give us a point on the great circle which contains the vertexes of the edge.
closestPoint := s2.Point{Vector: point.Vector.Sub(normalScaledToPoint).Normalize()}
closestPoint := s2.Point{Vector: point.GeogPoint.Vector.Sub(normalScaledToPoint).Normalize()}
// We then check whether the given point lies on the geodesic of the edge,
// as the above algorithm only generates a point on the great circle
// represented by the edge.
return &s2GeodistPoint{Point: closestPoint}, (&s2.Polyline{eV0, eV1}).IntersectsCell(s2.CellFromPoint(closestPoint))
return geodist.Point{GeogPoint: closestPoint}, (&s2.Polyline{eV0, eV1}).IntersectsCell(s2.CellFromPoint(closestPoint))
}

// regionToGeodistShape converts the s2 Region to a geodist object.
func regionToGeodistShape(r s2.Region) (geodist.Shape, error) {
switch r := r.(type) {
case s2.Point:
return &s2GeodistPoint{Point: r}, nil
return &geodist.Point{GeogPoint: r}, nil
case *s2.Polyline:
return &s2GeodistLineString{Polyline: r}, nil
case *s2.Polygon:
Expand Down
Loading

0 comments on commit 20615e5

Please sign in to comment.