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

tree: implement casts between box2d and geometry #52965

Merged
merged 1 commit into from
Aug 18, 2020

Conversation

otan
Copy link
Contributor

@otan otan commented Aug 18, 2020

Release note (sql change): Implement the ability to cast between box2d
and geometry types.

@otan otan requested review from sumeerbhola and a team August 18, 2020 03:34
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @otan)


pkg/sql/logictest/testdata/logic_test/geospatial_bbox, line 152 at r1 (raw file):

NULL
POINT (-1 -1)
LINESTRING (-1 -2, 4 6)

I tried this in Postgres after writing my other comment and got

POINT(-1 -1)
 POLYGON((-1 -2,-1 6,4 6,4 -2,-1 -2))

pkg/sql/sem/tree/casts.go, line 817 at r1 (raw file):

			} else {
				g, err = geo.NewGeometryFromGeomT(
					geom.NewLineStringFlat(geom.XY, []float64{d.LoX, d.LoY, d.HiX, d.HiY}),

surprising that Box2D turns into a LineString and not a Polygon.

@otan
Copy link
Contributor Author

otan commented Aug 18, 2020


pkg/sql/logictest/testdata/logic_test/geospatial_bbox, line 152 at r1 (raw file):

Previously, sumeerbhola wrote…

I tried this in Postgres after writing my other comment and got

POINT(-1 -1)
 POLYGON((-1 -2,-1 6,4 6,4 -2,-1 -2))

My mistake, every time I tried I go LineStrings but looks like it was because I did vertical / horizontal lines. good catch

Release note (sql change): Implement the ability to cast between box2d
and geometry types.
Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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


pkg/geo/bbox.go, line 204 at r2 (raw file):

			b.HiX, b.HiY,
			b.HiX, b.LoY,
			b.LoX, b.LoY,

should we orient this CCW to start with, since we'll need to change the orientation later in code.

@otan
Copy link
Contributor Author

otan commented Aug 18, 2020


pkg/geo/bbox.go, line 204 at r2 (raw file):

Previously, sumeerbhola wrote…

should we orient this CCW to start with, since we'll need to change the orientation later in code.

this is how PostGIS explicitly specified they'd do it :(

@otan
Copy link
Contributor Author

otan commented Aug 18, 2020

bors r=rytaft,sumeerbhola

@craig
Copy link
Contributor

craig bot commented Aug 18, 2020

Build succeeded:

@craig craig bot merged commit f41ff14 into cockroachdb:master Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants