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

support SRID for spatial type column definition #1018

Merged
merged 24 commits into from
Jun 1, 2022
Merged

Conversation

jennifersp
Copy link
Contributor

@jennifersp jennifersp commented May 17, 2022

Added SRID value syntax functionality for column definition.
Added tests cover CREATE TABLE, ALTER TABLE ADD/MODIFY COLUMN, and INSERT statements.
Fixes dolthub/dolt#3425

@jennifersp jennifersp changed the title [WIP] support SRID for spatial type column definition support SRID for spatial type column definition May 19, 2022
@jennifersp jennifersp requested a review from fulghum May 19, 2022 23:35
Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Very nice work with these changes Jennifer! Just some minor comments/questions. This is looking very close. 👍

sql/expression/function/wkb.go Outdated Show resolved Hide resolved
sql/expression/function/wkt_test.go Outdated Show resolved Hide resolved
sql/geometry.go Outdated
return Geometry{Inner: inner}, nil
case Geometry:
if err := t.MatchSRID(inner.Inner); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an edge case where Geometry could have its SRID set to something, but its InnerType member could have its SRID set to something else? Probably worth checking to see if we can trigger that condition and considering any guards against that. For example... in MatchSRID, you might want to pass in the Geometry object, check its SRID, then recurse on the InnerType and check its SRID .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! I think it's safe to call MatchSRID with Geometry object itself and recursively check SRID on its inner object. But, Geometry object does not have its own SRID member variable because it can only be either of Point, Linestring or Polygon objects, which they have SRID defined.

sql/geometry.go Outdated Show resolved Hide resolved
sql/linestring.go Outdated Show resolved Hide resolved
sql/parse/parse.go Show resolved Hide resolved
sql/point.go Outdated Show resolved Hide resolved
sql/polygon.go Outdated Show resolved Hide resolved
sql/type.go Show resolved Hide resolved
sql/errors.go Show resolved Hide resolved
@jennifersp jennifersp requested a review from fulghum May 25, 2022 21:43
Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Nice work! Code looks great. Just a few very minor comments. 🎉

sql/geometry_test.go Outdated Show resolved Hide resolved
sql/geometry_test.go Outdated Show resolved Hide resolved
sql/linestring.go Outdated Show resolved Hide resolved
sql/linestring.go Outdated Show resolved Hide resolved
@jennifersp jennifersp merged commit b0eba0d into main Jun 1, 2022
@jennifersp jennifersp deleted the jennifer/srid branch June 1, 2022 01:38
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.

Cannot create spatial table with CRS
3 participants