Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

geo: SpatialObjectToWKT() omits empty shapes in collections #53632

Closed
erikgrinaker opened this issue Aug 29, 2020 · 3 comments · Fixed by #53647
Closed

geo: SpatialObjectToWKT() omits empty shapes in collections #53632

erikgrinaker opened this issue Aug 29, 2020 · 3 comments · Fixed by #53647
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-community Originated from the community

Comments

@erikgrinaker
Copy link
Contributor

Describe the problem

While implementing ST_ForceCollection, I have a test case which generates a geometry collection with an empty point. However, calling geo.SpatialObjectToWKT omits the empty point from the output.

To Reproduce

func TestEmptyPointWKT(t *testing.T) {
	gc := geom.NewGeometryCollection().SetSRID(int(geopb.DefaultGeographySRID))
	gc.MustPush(geom.NewPointEmpty(geom.XY))
	g, err := geo.MakeGeographyFromGeomT(gc)
	require.NoError(t, err)
	wkt, err := geo.SpatialObjectToWKT(g.SpatialObject(), 0)
	require.NoError(t, err)
	require.EqualValues(t, "GEOMETRYCOLLECTION (POINT EMPTY)", wkt)
}

This yields a test failure:

Not equal: 
expected: string("GEOMETRYCOLLECTION (POINT EMPTY)")
actual  : geopb.WKT("GEOMETRYCOLLECTION EMPTY")

PostGIS can represent this just fine:

postgis=# select st_astext(st_forcecollection('POINT EMPTY'));
            st_astext            
---------------------------------
 GEOMETRYCOLLECTION(POINT EMPTY)
(1 row)

Environment:

  • CockroachDB version: master at 8b91062f93
  • Server OS: macOS
  • Client app: cockroach demo

CC @otan

@blathers-crl
Copy link

blathers-crl bot commented Aug 29, 2020

Hello, I am Blathers. I am here to help you get the issue triaged.

Hoot - a bug! Though bugs are the bane of my existence, rest assured the wretched thing will get the best of care here.

I have CC'd a few people who may be able to assist you:

  • @mattcrdb (member of the technical support engineering team)

If we have not gotten back to your issue within a few business days, you can try the following:

  • Join our community slack channel and ask on #cockroachdb.
  • Try find someone from here if you know they worked closely on the area and CC them.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-community Originated from the community X-blathers-oncall labels Aug 29, 2020
@erikgrinaker erikgrinaker changed the title geo: SpatialObjectToWKT() omitting empty points geo: SpatialObjectToWKT() omits empty points Aug 29, 2020
@otan
Copy link
Contributor

otan commented Aug 29, 2020

It's this if condition in go-geom -- https://github.com/twpayne/go-geom/blob/master/encoding/wkt/encode.go#L96 -- it should be changed to NumGeoms() == 0.

Feel free to change it, if not I'll get to it on Monday.

@erikgrinaker erikgrinaker changed the title geo: SpatialObjectToWKT() omits empty points geo: SpatialObjectToWKT() omits empty shapes in collections Aug 29, 2020
@erikgrinaker
Copy link
Contributor Author

Opened a go-geom PR here: twpayne/go-geom#184

@craig craig bot closed this as completed in 410616f Aug 31, 2020
craig bot pushed a commit that referenced this issue Aug 31, 2020
53556: builtins: add in unimplemented spatial builtins r=sumeerbhola a=otan

This adds telemetry to remaining unused functions, as well as displaying
the issue number when someone attempts to use an unimplemented function.

Also changed the wording of the error message to be a little more
friendly.

Release justification: low risk, high benefit changes

Release note: None

53600: build: fix TESTS env variable in roachtest nightly script r=jlinder a=adityamaru

AWS TC nightly fails with `unbound variable TESTS`. This
change initializes it outside the if condition.

Release justification: non-production code changes

53604: telemetry: fix the integer quantization r=irfansharif a=knz

The code was previously quantizing 100 to 10, and only
101 and higher to 100. This patch fixes it.

Additionally, this code ensures that negative values also get
quantized. This change does not impact any caller for now (there is no
caller that expects negative value) but will be used in an upcoming
setting telemetry change.

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

Release note: None

53643: builtins: implement ST_ForceCollection r=otan a=erikgrinaker

Some test cases deviate from PostGIS due to a bug in the WKT encoder (see #53632). The behavior of `ST_ForceCollection` is correct, but the WKT output does not accurately represent the resulting geometry. Not sure if we should wait for the upstream fix or merge this as-is. 

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

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

Closes #48934.

53656: roachtest: ignore version upgrade no inbound stream connection error r=irfansharif a=asubiotto

Release justification: non-production code change

This error is fixed in 20.2, but may occur on earlier versions because version
upgrade is a mixed-version test.

Release note: None (testing change)

Closes #53393 

Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Co-authored-by: Aditya Maru <adityamaru@gmail.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Co-authored-by: Erik Grinaker <erik@grinaker.org>
Co-authored-by: Alfonso Subiotto Marques <alfonso@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-community Originated from the community
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants