geomfn: implement validity operators #51484
Conversation
|
This change is |
otan
force-pushed
the
validity_checking
branch
2 times, most recently
from
340acf1
to
dc09e28
Compare
There was a problem hiding this comment.
Reviewed 8 of 8 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @otan)
pkg/geo/geomfn/validity_check.go, line 21 at r1 (raw file):
type ValidDetail struct { IsValid bool // Reason is only populated if IsValid = true.
shouldn't this be IsValid=false?
pkg/geo/geomfn/validity_check.go, line 23 at r1 (raw file):
// Reason is only populated if IsValid = true. Reason string // InvalidLocation is only populated if IsValid = true.
ditto
pkg/geo/geos/geos.go, line 623 at r1 (raw file):
// means that self-intersecting rings forming holes are considered valid. // It returns a bool representing whether it is valid, a string giving a reason for // invalidity, an EWKB representing the location things are invalid at.
[nit] and an EWKB
pkg/sql/sem/builtins/geo_builtins.go, line 2352 at r1 (raw file):
infoBuilder{ info: `Returns whether the geometry is valid as defined by the OGC spec.`, libraryUsage: usesGEOS,
Just out of curiosity: is this field purely for documentation purposes? Is there a way we could test that this matches reality? (Obviously that would be a different PR...)
pkg/sql/sem/builtins/geo_builtins.go, line 2440 at r1 (raw file):
types.Geometry, infoBuilder{ info: "Returns a valid form of the given geometry.",
Do you need to mention the OGC spec here? Or not necessary?
There was a problem hiding this comment.
Reviewable status:
pkg/geo/geomfn/validity_check.go, line 21 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
shouldn't this be IsValid=false?
Done.
pkg/geo/geomfn/validity_check.go, line 23 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
ditto
Done.
pkg/geo/geos/geos.go, line 623 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] and an EWKB
Done.
pkg/sql/sem/builtins/geo_builtins.go, line 2352 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Just out of curiosity: is this field purely for documentation purposes? Is there a way we could test that this matches reality? (Obviously that would be a different PR...)
Yep - only docs.
Not sure about detection - requires some fancy go parsing i reckon.
pkg/sql/sem/builtins/geo_builtins.go, line 2440 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Do you need to mention the OGC spec here? Or not necessary?
Done.
Unfortunately we cannot implement ST_IsValidDetail because it returns a composite type, which we do not yet support. Release note (sql change): Implements ST_IsValid, ST_IsValidReason and ST_MakeValid operators for geometry types.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r2.
Reviewable status:
pkg/sql/sem/builtins/geo_builtins.go, line 2352 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
Yep - only docs.
Not sure about detection - requires some fancy go parsing i reckon.
Sounds good -- probably not high priority
|
bors r=rytaft |
Canceled (will resume) |
|
bors r=rytaft |
Build succeeded |
Unfortunately we cannot implement ST_IsValidDetail because it returns a
composite type, which we do not yet support.
Resolves #48960
Resolves #48961
Resolves #48963
Resolves #48964
Release note (sql change): Implements ST_IsValid, ST_IsValidReason and
ST_MakeValid operators for geometry types.