cockroach (original) (raw)

only minor comments -- looks good overall

Reviewed 13 of 13 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan and @sumeerbhola)

docs/RFCS/20200421_geospatial.md, line 1043 at r2 (raw file):

ST_Contains(g, x), ST_Contains(x, g): use contains(g, x) or contained-by(g, x)

ST_ContainsProperly(g, x), ST_ContainsProperly(x, g): see above.

nit: please repeat the text from the previous bullet

pkg/geo/geomfn/binary_predicates.go, line 49 at r2 (raw file):

There is some redundancy in DE-9IM -- when I tried to construct I came up with TF*FF*FF*, but I think it is equivalent to what you wrote above.

pkg/geo/geomfn/de9im.go, line 31 at r2 (raw file):

// MatchesDE9IM checks whether the given DE-9IM relation matches the DE-91M pattern. // See: https://en.wikipedia.org/wiki/DE-9IM. func MatchesDE9IM(str string, pattern string) (bool, error) {

I am assuming str cannot have * since it represents the actual computed relation.
If yes, can you add that to this function comment.

pkg/geo/geomfn/de9im.go, line 43 at r2 (raw file):

    continue
case 't':
    if str[i] < '0' || str[i] > '2' {

when do we expect < 0?
Can you add a comment that f represents false and that 0, 1, 2 are true?
I think it would be easier to read if we added a conversion function to a bool.

pkg/geo/geos/geos.cc, line 430 at r2 (raw file):

Please add the wikipedia link