Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
53975: vendor: bump Pebble to b66f9d0155ab r=jbowens a=jbowens

```
b66f9d01 db: fix nil pointer dereference in compaction
c29cc161 internal/record: write an EOF trailer on LogWriter Close
```

Release justification: These Pebble commits fix high-severity bugs.
Release note: none

53999: builtins: implement ST_RemoveRepeatedPoints r=otan a=erikgrinaker

Release justification: low risk, high benefit changes to existing functionality

Release note (sql change): Implement the geometry builtin
`ST_RemoveRepeatedPoints`.

Closes #49017.

54013: builtins: add ST_LineFromMultiPoint and ST_LineMerge r=otan a=erikgrinaker

One test case for `ST_LineFromMultiPoint` is commented out until #53997 is resolved - this may fail and need code changes.

Release justification: low risk, high benefit changes to existing functionality

Release note (sql change): Implement the geometry builtins
`ST_LineFromMultiPoint` and `ST_LineMerge`.

Closes #48970.
Closes #48974.

54023: sqlliveness/slstorage: fix erroneous use of RLock r=ajwerner a=ajwerner

We cannot RLock the cache as a successful retrieval mutates the underlying LRU.

This commit also adds a test that pushes on the concurrency in the package.
It's perhaps too complex but it instantly reproduces this error and is a
general sanity check.

Fixes #53972

Release justification: bug fixes and low-risk updates to new functionality

Release note: None

54024: opt: fix issue with FastIntSet copy in statisticsBuilder r=rytaft a=rytaft

Release justification: low risk, high benefit changes to existing
functionality

This commit fixes a bug that could happen in rare cases when there were
many columns in a query (due to a single table with many columns or many
tables), causing any `ColSets` maintained by the optimizer to use the `Sparse`
format. When the `Sparse` format is used, it is not safe to perform a shallow
copy of the `ColSet`, because modifying the copy can corrupt the original
version. This  commit fixes two instances in the `statisticsBuilder` where we
were performing a shallow copy of the relational property `NotNullCols` and
modifying it, causing the relational property to be corrupted.

Fixes #54011

Release note (bug fix): Fixed a rare bug where the optimizer incorrectly
classified some columns as not-null, possibly leading to invalid query plans
and incorrect results.

Co-authored-by: Jackson Owens <jackson@cockroachlabs.com>
Co-authored-by: Erik Grinaker <erik@grinaker.org>
Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com>
Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
  • Loading branch information
5 people committed Sep 8, 2020
6 parents e20f17e + fb8bfd9 + b71341b + cf6130f + 9f653a6 + 17ad8cd commit c119b04
Show file tree
Hide file tree
Showing 17 changed files with 713 additions and 17 deletions.
9 changes: 9 additions & 0 deletions docs/generated/sql/functions.md
Original file line number Diff line number Diff line change
Expand Up @@ -1790,6 +1790,8 @@ calculated, the result is transformed back into a Geography with SRID 4326.</p>
<p>Note ST_Length is only valid for LineString - use ST_Perimeter for Polygon.</p>
<p>This function utilizes the GEOS module.</p>
</span></td></tr>
<tr><td><a name="st_linefrommultipoint"></a><code>st_linefrommultipoint(geometry: geometry) &rarr; geometry</code></td><td><span class="funcdesc"><p>Creates a LineString from a MultiPoint geometry.</p>
</span></td></tr>
<tr><td><a name="st_linefromtext"></a><code>st_linefromtext(str: <a href="string.html">string</a>, srid: <a href="int.html">int</a>) &rarr; geometry</code></td><td><span class="funcdesc"><p>Returns the Geometry from a WKT or EWKT representation with an SRID. If the shape underneath is not LineString, NULL is returned. If the SRID is present in both the EWKT and the argument, the argument value is used.</p>
</span></td></tr>
<tr><td><a name="st_linefromtext"></a><code>st_linefromtext(val: <a href="string.html">string</a>) &rarr; geometry</code></td><td><span class="funcdesc"><p>Returns the Geometry from a WKT or EWKT representation. If the shape underneath is not LineString, NULL is returned.</p>
Expand All @@ -1809,6 +1811,9 @@ calculated, the result is transformed back into a Geography with SRID 4326.</p>
<p>Note If the result has zero or one points, it will be returned as a POINT. If it has two or more points, it will be returned as a MULTIPOINT.</p>
<p>This function utilizes the GEOS module.</p>
</span></td></tr>
<tr><td><a name="st_linemerge"></a><code>st_linemerge(geometry: geometry) &rarr; geometry</code></td><td><span class="funcdesc"><p>Returns a LineString or MultiLineString by joining together constituents of a MultiLineString with matching endpoints. If the input is not a MultiLineString or LineString, an empty GeometryCollection is returned.</p>
<p>This function utilizes the GEOS module.</p>
</span></td></tr>
<tr><td><a name="st_linestringfromtext"></a><code>st_linestringfromtext(str: <a href="string.html">string</a>, srid: <a href="int.html">int</a>) &rarr; geometry</code></td><td><span class="funcdesc"><p>Returns the Geometry from a WKT or EWKT representation with an SRID. If the shape underneath is not LineString, NULL is returned. If the SRID is present in both the EWKT and the argument, the argument value is used.</p>
</span></td></tr>
<tr><td><a name="st_linestringfromtext"></a><code>st_linestringfromtext(val: <a href="string.html">string</a>) &rarr; geometry</code></td><td><span class="funcdesc"><p>Returns the Geometry from a WKT or EWKT representation. If the shape underneath is not LineString, NULL is returned.</p>
Expand Down Expand Up @@ -1989,6 +1994,10 @@ Negative azimuth values and values greater than 2π (360 degrees) are supported.
</span></td></tr>
<tr><td><a name="st_removepoint"></a><code>st_removepoint(line_string: geometry, index: <a href="int.html">int</a>) &rarr; geometry</code></td><td><span class="funcdesc"><p>Removes the Point at the given 0-based index and returns the modified LineString geometry.</p>
</span></td></tr>
<tr><td><a name="st_removerepeatedpoints"></a><code>st_removerepeatedpoints(geometry: geometry) &rarr; geometry</code></td><td><span class="funcdesc"><p>Returns a geometry with repeated points removed.</p>
</span></td></tr>
<tr><td><a name="st_removerepeatedpoints"></a><code>st_removerepeatedpoints(geometry: geometry, tolerance: <a href="float.html">float</a>) &rarr; geometry</code></td><td><span class="funcdesc"><p>Returns a geometry with repeated points removed, within the given distance tolerance.</p>
</span></td></tr>
<tr><td><a name="st_reverse"></a><code>st_reverse(geometry: geometry) &rarr; geometry</code></td><td><span class="funcdesc"><p>Returns a modified geometry by reversing the order of its vertices.</p>
</span></td></tr>
<tr><td><a name="st_rotate"></a><code>st_rotate(g: geometry, angle_radians: <a href="float.html">float</a>) &rarr; geometry</code></td><td><span class="funcdesc"><p>Returns a modified Geometry whose coordinates are rotated around the origin by a rotation angle.</p>
Expand Down
4 changes: 2 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ require (
github.com/cockroachdb/go-test-teamcity v0.0.0-20191211140407-cff980ad0a55
github.com/cockroachdb/gostdlib v1.13.0
github.com/cockroachdb/logtags v0.0.0-20190617123548-eb05cc24525f
github.com/cockroachdb/pebble v0.0.0-20200831143935-e6a9f9a3c936
github.com/cockroachdb/pebble v0.0.0-20200904203921-b66f9d0155ab
github.com/cockroachdb/redact v1.0.5
github.com/cockroachdb/returncheck v0.0.0-20200612231554-92cdbca611dd
github.com/cockroachdb/sentry-go v0.6.1-cockroachdb.2
Expand Down Expand Up @@ -145,7 +145,7 @@ require (
github.com/zabawaba99/go-gitignore v0.0.0-20200117185801-39e6bddfb292
go.etcd.io/etcd v0.0.0-00010101000000-000000000000
golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9
golang.org/x/exp v0.0.0-20200821190819-94841d0725da
golang.org/x/exp v0.0.0-20200901203048-c4f52b2c50aa
golang.org/x/lint v0.0.0-20200130185559-910be7a94367
golang.org/x/mod v0.3.0 // indirect
golang.org/x/net v0.0.0-20200602114024-627f9648deb9
Expand Down
8 changes: 4 additions & 4 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,8 @@ github.com/cockroachdb/grpc-gateway v1.14.6-0.20200519165156-52697fc4a249 h1:pZu
github.com/cockroachdb/grpc-gateway v1.14.6-0.20200519165156-52697fc4a249/go.mod h1:UJ0EZAp832vCd54Wev9N1BMKEyvcZ5+IM0AwDrnlkEc=
github.com/cockroachdb/logtags v0.0.0-20190617123548-eb05cc24525f h1:o/kfcElHqOiXqcou5a3rIlMc7oJbMQkeLk0VQJ7zgqY=
github.com/cockroachdb/logtags v0.0.0-20190617123548-eb05cc24525f/go.mod h1:i/u985jwjWRlyHXQbwatDASoW0RMlZ/3i9yJHE2xLkI=
github.com/cockroachdb/pebble v0.0.0-20200831143935-e6a9f9a3c936 h1:MaPvrDBbbWMzmI3uuBMKvcpDfZDY/QHx22T1pb9J6YI=
github.com/cockroachdb/pebble v0.0.0-20200831143935-e6a9f9a3c936/go.mod h1:hU7vhtrqonEphNF+xt8/lHdaBprxmV1h8BOGrd9XwmQ=
github.com/cockroachdb/pebble v0.0.0-20200904203921-b66f9d0155ab h1:NBGEsgZzdE1OuFzndcoSsX36law5tjEqfsn4U8EnZYs=
github.com/cockroachdb/pebble v0.0.0-20200904203921-b66f9d0155ab/go.mod h1:hU7vhtrqonEphNF+xt8/lHdaBprxmV1h8BOGrd9XwmQ=
github.com/cockroachdb/redact v0.0.0-20200622112456-cd282804bbd3 h1:2+dpIJzYMSbLi0587YXpi8tOJT52qCOI/1I0UNThc/I=
github.com/cockroachdb/redact v0.0.0-20200622112456-cd282804bbd3/go.mod h1:BVNblN9mBWFyMyqK1k3AAiSxhvhfK2oOZZ2lK+dpvRg=
github.com/cockroachdb/redact v1.0.5 h1:yxqIMS6G2Bvi6GiSHFmsrFGO3aP1rwt8cOm4pixw9eY=
Expand Down Expand Up @@ -773,8 +773,8 @@ golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL
golang.org/x/exp v0.0.0-20190306152737-a1d7652674e8/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA=
golang.org/x/exp v0.0.0-20200513190911-00229845015e h1:rMqLP+9XLy+LdbCXHjJHAmTfXCr93W7oruWA6Hq1Alc=
golang.org/x/exp v0.0.0-20200513190911-00229845015e/go.mod h1:4M0jN8W1tt0AVLNr8HDosyJCDCDuyL9N9+3m7wDWgKw=
golang.org/x/exp v0.0.0-20200821190819-94841d0725da h1:vfV2BR+q1+/jmgJR30Ms3RHbryruQ3Yd83lLAAue9cs=
golang.org/x/exp v0.0.0-20200821190819-94841d0725da/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU=
golang.org/x/exp v0.0.0-20200901203048-c4f52b2c50aa h1:i1+omYRtqpxiCaQJB4MQhUToKvMPFqUUJKvRiRp0gtE=
golang.org/x/exp v0.0.0-20200901203048-c4f52b2c50aa/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU=
golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js=
golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0=
golang.org/x/lint v0.0.0-20180702182130-06c8688daad7/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE=
Expand Down
5 changes: 5 additions & 0 deletions pkg/geo/geomfn/coord.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,8 @@ func coordNorm(c geom.Coord) float64 {
func coordEqual(a geom.Coord, b geom.Coord) bool {
return a.X() == b.X() && a.Y() == b.Y()
}

// coordMag2 returns the magnitude^2 of a coordinate if the coord was a vector.
func coordMag2(c geom.Coord) float64 {
return coordDot(c, c)
}
36 changes: 36 additions & 0 deletions pkg/geo/geomfn/linestring.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,46 @@ package geomfn

import (
"github.com/cockroachdb/cockroach/pkg/geo"
"github.com/cockroachdb/cockroach/pkg/geo/geos"
"github.com/cockroachdb/errors"
"github.com/twpayne/go-geom"
)

// LineStringFromMultiPoint generates a linestring from a multipoint.
func LineStringFromMultiPoint(g geo.Geometry) (geo.Geometry, error) {
t, err := g.AsGeomT()
if err != nil {
return geo.Geometry{}, err
}
mp, ok := t.(*geom.MultiPoint)
if !ok {
return geo.Geometry{}, errors.Wrap(geom.ErrUnsupportedType{Value: t},
"geometry must be a MultiPoint")
}
if mp.NumPoints() == 1 {
return geo.Geometry{}, errors.Newf("a LineString must have at least 2 points")
}
lineString := geom.NewLineString(mp.Layout()).SetSRID(mp.SRID())
lineString, err = lineString.SetCoords(mp.Coords())
if err != nil {
return geo.Geometry{}, err
}
return geo.MakeGeometryFromGeomT(lineString)
}

// LineMerge merges multilinestring constituents.
func LineMerge(g geo.Geometry) (geo.Geometry, error) {
// Mirrors PostGIS behavior
if g.Empty() {
return g, nil
}
ret, err := geos.LineMerge(g.EWKB())
if err != nil {
return geo.Geometry{}, err
}
return geo.ParseGeometryFromEWKB(ret)
}

// AddPoint adds a point to a LineString at the given 0-based index. -1 appends.
func AddPoint(lineString geo.Geometry, index int, point geo.Geometry) (geo.Geometry, error) {
g, err := lineString.AsGeomT()
Expand Down
103 changes: 103 additions & 0 deletions pkg/geo/geomfn/linestring_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,113 @@ import (
"testing"

"github.com/cockroachdb/cockroach/pkg/geo"
"github.com/cockroachdb/cockroach/pkg/geo/geopb"
"github.com/stretchr/testify/require"
"github.com/twpayne/go-geom"
)

func TestLineStringFromMultiPoint(t *testing.T) {
testCases := []struct {
wkt string
expected string
}{
{"MULTIPOINT EMPTY", "LINESTRING EMPTY"},
{"MULTIPOINT (1 2, 3 4, 5 6)", "LINESTRING (1 2, 3 4, 5 6)"},
// The following test case mirrors PostGIS behavior of duplicating the previous point for
// EMPTY points, although the correct behavior would probably be to omit the duplicate.
// FIXME https://github.com/cockroachdb/cockroach/issues/53997
//{"MULTIPOINT (1 2, EMPTY, 3 4)", "LINESTRING (1 2, 1 2, 3 4)"},
}

for _, tc := range testCases {
t.Run(tc.wkt, func(t *testing.T) {
srid := geopb.SRID(4000)
g, err := geo.ParseGeometryFromEWKT(geopb.EWKT(tc.wkt), srid, true)
require.NoError(t, err)

result, err := LineStringFromMultiPoint(g)
require.NoError(t, err)
wkt, err := geo.SpatialObjectToWKT(result.SpatialObject(), 0)
require.NoError(t, err)
require.EqualValues(t, tc.expected, wkt)
require.EqualValues(t, srid, result.SRID())
})
}

errorTestCases := []struct {
wkt string
}{
{"MULTIPOINT (1 1)"},
{"POINT EMPTY"},
{"POINT (1 1)"},
{"LINESTRING (1 1, 2 2)"},
{"MULTILINESTRING ((1 1, 2 2))"},
{"POLYGON ((1 2, 3 4, 5 6, 1 2))"},
{"MULTIPOLYGON (((1 2, 3 4, 5 6, 1 2)))"},
{"GEOMETRYCOLLECTION (MULTIPOINT (1 1, 2 2))"},
}

t.Run("Errors on invalid input", func(t *testing.T) {
for _, tc := range errorTestCases {
t.Run(tc.wkt, func(t *testing.T) {
g, err := geo.ParseGeometryFromEWKT(geopb.EWKT(tc.wkt), geopb.DefaultGeometrySRID, true)
require.NoError(t, err)

_, err = LineStringFromMultiPoint(g)
require.Error(t, err)
})
}
})
}

func TestLineMerge(t *testing.T) {
testCases := []struct {
wkt string
expected string
}{
{"MULTILINESTRING EMPTY", "MULTILINESTRING EMPTY"},
{
"MULTILINESTRING ((1 2, 2 3, 3 4), (3 4, 4 5, 5 6), (5 6, 6 7, 7 8))",
"LINESTRING (1 2, 2 3, 3 4, 4 5, 5 6, 6 7, 7 8)",
},
{
"MULTILINESTRING ((1 2, 2 3, 3 4), EMPTY, (3 4, 4 5, 5 6), EMPTY, (5 6, 6 7, 7 8))",
"LINESTRING (1 2, 2 3, 3 4, 4 5, 5 6, 6 7, 7 8)",
},
{
"MULTILINESTRING ((1 2, 2 3, 3 4), (3 4, 4 5, 5 6), (6 7, 7 8, 8 9), (8 9, 9 10, 10 11))",
"MULTILINESTRING ((1 2, 2 3, 3 4, 4 5, 5 6), (6 7, 7 8, 8 9, 9 10, 10 11))",
},
{"POINT EMPTY", "POINT EMPTY"},
{"POINT (1 1)", "GEOMETRYCOLLECTION EMPTY"},
{"MULTIPOINT EMPTY", "MULTIPOINT EMPTY"},
{"MULTIPOINT (1 1, 2 2)", "GEOMETRYCOLLECTION EMPTY"},
{"LINESTRING EMPTY", "LINESTRING EMPTY"},
{"LINESTRING (1 2, 3 4)", "LINESTRING (1 2, 3 4)"},
{"POLYGON EMPTY", "POLYGON EMPTY"},
{"POLYGON ((1 2, 3 4, 5 6, 1 2))", "GEOMETRYCOLLECTION EMPTY"},
{"MULTIPOLYGON EMPTY", "MULTIPOLYGON EMPTY"},
{"MULTIPOLYGON (((1 2, 3 4, 5 6, 1 2)))", "GEOMETRYCOLLECTION EMPTY"},
{"GEOMETRYCOLLECTION EMPTY", "GEOMETRYCOLLECTION EMPTY"},
{"GEOMETRYCOLLECTION (MULTILINESTRING ((1 2, 2 3, 3 4)))", "GEOMETRYCOLLECTION EMPTY"},
}

for _, tc := range testCases {
t.Run(tc.wkt, func(t *testing.T) {
srid := geopb.SRID(4000)
g, err := geo.ParseGeometryFromEWKT(geopb.EWKT(tc.wkt), srid, true)
require.NoError(t, err)

result, err := LineMerge(g)
require.NoError(t, err)
wkt, err := geo.SpatialObjectToWKT(result.SpatialObject(), 0)
require.NoError(t, err)
require.EqualValues(t, tc.expected, wkt)
require.EqualValues(t, srid, result.SRID())
})
}
}

func TestAddPoint(t *testing.T) {
testCases := []struct {
lineString *geom.LineString
Expand Down
119 changes: 119 additions & 0 deletions pkg/geo/geomfn/remove_repeated_points.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
// Copyright 2020 The Cockroach Authors.
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package geomfn

import (
"github.com/cockroachdb/cockroach/pkg/geo"
"github.com/cockroachdb/errors"
"github.com/twpayne/go-geom"
)

// RemoveRepeatedPoints returns the geometry with repeated points removed.
func RemoveRepeatedPoints(g geo.Geometry, tolerance float64) (geo.Geometry, error) {
t, err := g.AsGeomT()
if err != nil {
return geo.Geometry{}, err
}
// Use the square of the tolerance to avoid taking the square root of distance results.
t, err = removeRepeatedPointsFromGeomT(t, tolerance*tolerance)
if err != nil {
return geo.Geometry{}, err
}
return geo.MakeGeometryFromGeomT(t)
}

func removeRepeatedPointsFromGeomT(t geom.T, tolerance2 float64) (geom.T, error) {
switch t := t.(type) {
case *geom.Point:
case *geom.LineString:
if coords, modified := removeRepeatedCoords(t.Layout(), t.Coords(), tolerance2, 2); modified {
return t.SetCoords(coords)
}
case *geom.Polygon:
if coords, modified := removeRepeatedCoords2(t.Layout(), t.Coords(), tolerance2, 4); modified {
return t.SetCoords(coords)
}
case *geom.MultiPoint:
if coords, modified := removeRepeatedCoords(t.Layout(), t.Coords(), tolerance2, 0); modified {
return t.SetCoords(coords)
}
case *geom.MultiLineString:
if coords, modified := removeRepeatedCoords2(t.Layout(), t.Coords(), tolerance2, 2); modified {
return t.SetCoords(coords)
}
case *geom.MultiPolygon:
if coords, modified := removeRepeatedCoords3(t.Layout(), t.Coords(), tolerance2, 4); modified {
return t.SetCoords(coords)
}
case *geom.GeometryCollection:
for _, g := range t.Geoms() {
if _, err := removeRepeatedPointsFromGeomT(g, tolerance2); err != nil {
return nil, err
}
}
default:
return nil, errors.AssertionFailedf("unknown geometry type: %T", t)
}
return t, nil
}

func removeRepeatedCoords(
layout geom.Layout, coords []geom.Coord, tolerance2 float64, minCoords int,
) ([]geom.Coord, bool) {
modified := false
switch tolerance2 {
case 0:
for i := 1; i < len(coords) && len(coords) > minCoords; i++ {
if coords[i].Equal(layout, coords[i-1]) {
coords = append(coords[:i], coords[i+1:]...)
modified = true
i--
}
}
default:
for i := 1; i < len(coords) && len(coords) > minCoords; i++ {
if coordMag2(coordSub(coords[i], coords[i-1])) <= tolerance2 {
coords = append(coords[:i], coords[i+1:]...)
modified = true
i--
}
}
}
return coords, modified
}

func removeRepeatedCoords2(
layout geom.Layout, coords2 [][]geom.Coord, tolerance2 float64, minCoords int,
) ([][]geom.Coord, bool) {
modified := false
for i, coords := range coords2 {
if c, m := removeRepeatedCoords(layout, coords, tolerance2, minCoords); m {
coords2[i] = c
modified = true
}
}
return coords2, modified
}

func removeRepeatedCoords3(
layout geom.Layout, coords3 [][][]geom.Coord, tolerance2 float64, minCoords int,
) ([][][]geom.Coord, bool) {
modified := false
for i, coords2 := range coords3 {
for j, coords := range coords2 {
if c, m := removeRepeatedCoords(layout, coords, tolerance2, minCoords); m {
coords3[i][j] = c
modified = true
}
}
}
return coords3, modified
}
Loading

0 comments on commit c119b04

Please sign in to comment.