-
Notifications
You must be signed in to change notification settings - Fork 41
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
Adding documentation for SQL Server working copy #369
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the CRS definition stuff is wrong, but the rest is looking great.
|
||
#### Approximated types | ||
|
||
There is one type that Sno supports that has no SQL Server equivalent - the `interval`. This type is "approximated" as a string (or more precisely, as an `NVARCHAR`) in the SQL Server working copy while keeping its original type in the Sno dataset. Sno creates a column of type `NVARCHAR` in the appropriate place in the database table, and fills it with with the intervals formatted as [ISO8601 durations](https://en.wikipedia.org/wiki/ISO_8601#Durations). When the working copy is committed, Sno converts the contents of those columns back to `interval`, instead of their actual type, `NVARCHAR`. Since the change from `interval` to `NVARCHAR` is just a limitation of the working copy, this apparent change in type will not show up as a change in the commit log, nor will it show up as an uncommitted change if you run `sno status` to see what local changes you have made but not yet committed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If you attempt to commit an invalid ISO8601 duration value in a Sno interval column, then you get an error?
NULL
in Sno ends up asNULL
in the DB text field?- Empty strings and
NULL
in the DB field both end up asNULL
in Sno?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah some schema validation exists but is only half done - I'll add some more for intervals. It can take a while for me to get to these things since nobody in practise is checking out data with intervals into working copies that don't support it and then committing schema violating changes.
Yet. I'll write another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within reason doesn’t really matter what the behaviour is as long as it’s documented.
As we add more patterns to our repertoire we’ll need to think less and less hopefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've expanded the schema validation to validate dates, times, timestamps and intervals, according to the V2 spec.
Sno -> SQL Server
NULL -> NULL
"P1D" -> "P1D" (1 day)
SQL Server -> Sno
NULL -> NULL
"P1D" -> "P1D"
"", "abc", etc -> In column '{col.name}' value {repr(value)} is not an ISO 8601 duration ie PxYxMxDTxHxMxS
I could add more detail here? Or I could even write a separate document that explains approximated types more fully, and link to it from all 3 working copy documents (since they all have approximated types)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linking to a central page with details on the various types makes sense to me.
eg:
Time Interval
A time interval is the intervening time between two time points, as modelled by an ISO 8601 Duration.
Natively supported in: PostgreSQL (
interval
)
Approximated (mapped?) in: GeoPackage; MS SQL Server... some text to describe the approximation and how it works ...
|
||
#### CRS definitions | ||
|
||
Sno lets you define arbitrary CRS definitions and attach them to your dataset. By contrast, SQL Server comes pre-installed with hundreds of the standard EPSG & ESRI coordinate reference system definitions. However, these cannot be modified, and custom CRS cannot be added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT SQL Server is naive to CRS, with no CRS definitions or ability to transform coordinates between them. SRIDs are just reference numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not quite true but I'm not 100% on the details. Here are two counterpoints I do know:
- There is a table sys.spatial_reference_systems, which stores the supported CRS definitions. Now, this doesn't necessarily mean that SQL server actually cares about those definitions, but...
- A bunch of Geography type instance methods use them. For instance, STArea:
https://docs.microsoft.com/sql/t-sql/spatial-geography/starea-geography-data-type
But also STDistance, and things like intersection and union - which also require that the types they are unioning or intersecting have matching SRIDs.
But looking into it more, seems like this mostly doesn't matter to us yet as we are using Geometry types? It matters that Geometries have matching SRIDs, but it doesn't seem to matter what they are - eg the area is always given in the same units as the geometries themlselves, which exist on an abstract flat plane. So, basically you are right about SRIDs as long as we are talking about Geometries and not Geographies.
I'll rewrite it a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s get it right for the code we have and we can expand further for Geography soon.
Description
Documentation to describe the SQL Server support being added in #362
Related links:
#362