Add support for datetimeoffset data type #26
Conversation
|
Uh, oh, it's not clear to me why the builds are failing..(?) 😓 |
|
This looks pretty reasonable from a brief look over. I'll look at it in more detail when I have more time. For testing you just need to set an environment variable with info for a test database to run against. The CI is failing because my build script is bad for PR's, you can ignore that. |
|
Thank you, getting the tests to run was easier than I thought. I think I was a little overwhelmed when I started working on this 😅 I'll add some tests and push my changes once I've done so. |
|
It turned out that adding a test for I've pushed a new commit with a round-trip test that passes on my machine. |
src/Database/ODBC/Internal.hs
Outdated
| @@ -605,6 +607,40 @@ getData dbc stmt i col = | |||
| (fmap fromIntegral (odbc_TIME_STRUCT_hour datePtr)) <*> | |||
| (fmap fromIntegral (odbc_TIME_STRUCT_minute datePtr)) <*> | |||
| (fmap fromIntegral (odbc_TIME_STRUCT_second datePtr)))) | |||
| | colType == sql_ss_timestampoffset -> | |||
| withCallocBytes | |||
| 20 | |||
There was a problem hiding this comment.
Shouldn't this be 10 bytes? Why 20?
There was a problem hiding this comment.
I'm not sure, but I have a theory. I'd like to air it here; please let me know if this sounds completely bonkers.
I couldn't find anything in the documentation that states the size of the TIMESTAMPOFFSET_STRUCT, so I added the sizes of the constituent elements of the struct together. 3 SQLSMALLINT + 5 SQLUSMALLINT + 1 SQLUINTEGER. I used the conversion table in https://docs.microsoft.com/en-us/sql/odbc/reference/appendixes/c-data-types to translate those to C types, and from there I guessed at the size of each, based on https://en.wikipedia.org/wiki/C_data_types. I guessed that short int and unsigned short int would both be 2 bytes (the minimum size) and unsigned long int would be 4 bytes (again, the minimum size). That's 3 * 2 + 5 + 2 + 1 * 4 = 20.
I've noticed that https://docs.microsoft.com/en-us/sql/t-sql/data-types/datetimeoffset-transact-sql states that the storage size of the datetimeoffset column type is 10 bytes, but I suppose that that's because it uses a more compact data format (something like nanoseconds since some zero date, plus the time zone offset...).
At a more pragmatic level, the code works when I allocate 20 bytes. If I try to allocate 10 bytes, it doesn't work:
test\Main.hs:343:
1) Database.ODBC.SQLServer, Conversion to SQL, QuickCheck roundtrip: HS=Datetimeoffset, SQL=datetimeoffset
uncaught exception: ODBCException (UnsuccessfulReturnCode "getTypedData ty=SQLCTYPE (-2)" (-1) "[Microsoft][ODBC Driver 17 for SQL Server]Numeric value out of range[Microsoft][ODBC Driver 17 for SQL Server]Numeric value out of range{") (after 1 test)
Datetimeoffset {unDatetimeoffset = 1777-07-20 21:45:49.3091726 +0824}
test\Main.hs:343:
2) Database.ODBC.SQLServer, Conversion to SQL, QuickCheck roundtrip: HS=Maybe Datetimeoffset, SQL=datetimeoffset
uncaught exception: ODBCException (UnsuccessfulReturnCode "getTypedData ty=SQLCTYPE (-2)" (-1) "[Microsoft][ODBC Driver 17 for SQL Server]Numeric value out of range[Microsoft][ODBC Driver 17 for SQL Server]Numeric value out of range{") (after 1 test)
Datetimeoffset {unDatetimeoffset = 1777-07-20 21:45:49.3091726 +0824}
There was a problem hiding this comment.
Fair enough. 👍 If you can document that somewhere, then the code will be self-explanatory. Allocating 20 bytes isn't a big deal more than 10, I was just curious why it differed to the claim I saw in MSDN.
test\Main.hs:343:
1) Database.ODBC.SQLServer, Conversion to SQL, QuickCheck roundtrip: HS=Datetimeoffset, SQL=datetimeoffset
uncaught exception: ODBCException (UnsuccessfulReturnCode "getTypedData ty=SQLCTYPE (-2)" (-1) "[Microsoft][ODBC Driver 17 for SQL Server]Numeric value out of range[Microsoft][ODBC Driver 17 for SQL Server]Numeric value out of range{") (after 1 test)
Datetimeoffset {unDatetimeoffset = 1777-07-20 21:45:49.3091726 +0824}
Note to self, fix that error reporting. It's truncated intentionally, but out of laziness.
There was a problem hiding this comment.
Indeed, I've now added a comment in a new commit.
FWIW, I had in mind doing that all along, but I felt the need to run the above explanation by someone first, as I wasn't entirely sure that it was correct.
|
Looking very good. I'm ready to merge after I've tested this and some of my questions are clarified, tomorrow I'll test this on my local machine. I appreciate your efforts on this, very generous of you to contribute time to this. |
|
Is there anything I still need to do? |
|
This has been resolved by #48 |
This pull request addresses #25
You can, if you will, consider this pull request a request for comments. This work definitely represents the limits of my skills with C. I understand the Haskell code that I've written, but there's some of the C interop code where I had to use trial and error to get it to work.
The code seems to work against my test table on a database I've hosted on Microsoft Azure.
There's no tests in this pull request, because I couldn't figure out how to make the tests pass, even on
master. I'm not adverse to adding tests, but I will, then, need some guidance on how to run them.Some of the Haskell code I would have preferred to write in a different way, but I chose to follow the style already in use in this code base. This also means that there's some code duplication.
I'm happy to address this as well, either as updates to this pull request, or in subsequent pull requests.