support SRID value in column definition #3457
Conversation
jennifersp
changed the title
| if err != nil { | ||
| return nil, err | ||
| } | ||
| GeometryType = &geometryType{sqlGeometryType: sql.GeometryType{InnerType: nil, SRID: uint32(sridVal), DefinedSRID: dSRID}} |
There was a problem hiding this comment.
Assigning this customized type to the shared variable GeometryType seems like it could cause problems or confusion in the future... the other usage still looks correct (e.g. IsColSpatialType uses this var, but the Equals methods just check to see if the types are the same), but it still seems odd to change the global variable to the last value someone converted, instead of just leaving it as a default. I bet there's a correctness issue when a new Geometry TypeInfo is re-assigned and then used by other parts in the code.
I suspect we should just return the new type here and not cache it in the GeometryType var, but let me know if I'm missing something.
| // Expect a types.Point, return a sql.Point | ||
| if val, ok := v.(types.Point); ok { | ||
| return ConvertTypesPointToSQLPoint(val), nil | ||
| sqlVal := ConvertTypesPointToSQLPoint(val) | ||
| err = ti.sqlPointType.MatchSRID(sqlVal) |
There was a problem hiding this comment.
I'm wondering if we actually need to validate the SRID here... we definitely need to validate any new data coming into the database from the caller (of course) and we need to validate any schema changes to make sure they don't invalidate the SRID matching constraint, but I'm wondering if we can be more lenient when we're loading data into and out of the storage layer interface. The impact of this validation failing is that customer won't be able to load their (already entered into the system) data any more, which could be worse than it being loaded and having inconsistent SRIDs that don't fit the constraint of the column.
Are we testing this constraint in enough other places where the data is coming into the system so that we don't need to test this again as the data goes into the storage layer? I'm not confident yet, but wanted to bring this up to consider.
jennifersp
changed the title
| @@ -272,6 +275,8 @@ func updateTableWithModifiedColumn(ctx context.Context, tbl *doltdb.Table, oldSc | |||
| val, ok := r.GetColVal(modifiedCol.Tag) | |||
| if !ok || val == nil || val == types.NullValue { | |||
| return true, fmt.Errorf("cannot change column to NOT NULL when one or more values is NULL") | |||
| } else if err = validateSpatialTypeSRID(modifiedCol, val); err != nil { | |||
There was a problem hiding this comment.
Should this validation check be inside the if !modifiedCol.IsNullable() block?
It seems like we only need to run this validation when a column type is changing, and you've got that covered in the if block right above this.
This block seems like it's specific for checking that a modified column that does not allow nulls doesn't have any existing nulls in the data. I'm not sure we need to do anything for SRID here, but let me know if I'm missing something.
Added SRID check for spatial types when converting the value/column between noms and sql values.
Added SRID details for column definition in type params when converting table info.