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 mssql's DateTime2 type via chrono #1681

Closed

Conversation

milgner
Copy link

@milgner milgner commented Feb 6, 2022

There was some previous conversation about it in #1251 but I didn't find any code in the current version of the repository to get it working. So I wrote my own version, based on the implementation from the go-mssqldb library.

Unfortunately, In order to add it to have a full integration test for type casting, I had to add chrono to the dev-dependencies in Cargo.toml. And since one cannot have features with the same name as a dependency, I had to rename the feature from chrono to with_chrono.

Any suggestions for improvement welcome.

@milgner milgner force-pushed the feature/mssql-chrono-datetime2 branch from 89836be to dfe3a12 Compare February 6, 2022 14:58
@milgner
Copy link
Author

milgner commented Feb 6, 2022

Simplified the code to not require lazy_static, added working tests and fixed a bug with scale>5 along the way. Let me know if you'd like to change anything.

@milgner milgner changed the title WIP: support DateTime2 type via chrono Support DateTime2 type via chrono Feb 6, 2022
@milgner milgner changed the title Support DateTime2 type via chrono Support mssql's DateTime2 type via chrono Feb 6, 2022
@milgner milgner force-pushed the feature/mssql-chrono-datetime2 branch 2 times, most recently from 39fbcf0 to b2a260c Compare February 6, 2022 15:38
@milgner
Copy link
Author

milgner commented Feb 13, 2022

I further improved the implementation by decoding datetime2 as chrono::NaiveDateTime instead and adding support for datetimeoffset, too, which is treated as chrono::DateTime with proper offset information. Suggestions for additional improvement welcome.

@Dessix
Copy link

Dessix commented Feb 26, 2022

@milgner It appears that the only flaw here is a lack of a cargo fmt run before all checks would pass; Can we try to get this merge-ready before it drifts too far from master?

Additionally, whether this is done in this PR or otherwise, enabling the chrono types in /sqlx-core/src/any/types may now be viable, and would allow use of the any feature with chrono.

@milgner
Copy link
Author

milgner commented Feb 26, 2022

@milgner It appears that the only flaw here is a lack of a cargo fmt run before all checks would pass; Can we try to get this merge-ready before it drifts too far from master?

Of course! I haven't had a chance this week to check on this but I'll gladly run it through cargo fmt and rebase it!

Additionally, whether this is done in this PR or otherwise, enabling the chrono types in /sqlx-core/src/any/types may now be viable, and would allow use of the any feature with chrono.

Not sure what the implications might be here. I think I'll try creating a separate branch and create a second PR for that.

@milgner milgner force-pushed the feature/mssql-chrono-datetime2 branch from cee7e0c to 6b44b23 Compare February 26, 2022 09:21
@zibebe
Copy link
Contributor

zibebe commented Aug 27, 2022

Any possibilities to get this merged?

@milgner
Copy link
Author

milgner commented Aug 29, 2022

I see that there are merge conflicts again. I can quickly resolve those if desired. But a short 👍 or 👎 would be appreciated so I don't have to resolve them anew every second week 😉

@zibebe
Copy link
Contributor

zibebe commented Aug 29, 2022

I really hope you get some 👍 for this. With #2073 it's the only thing preventing me from using sqlx for mssql.

@@ -121,7 +121,7 @@ mssql = ["sqlx-core/mssql", "sqlx-macros/mssql"]
# types
bigdecimal = ["sqlx-core/bigdecimal", "sqlx-macros/bigdecimal"]
decimal = ["sqlx-core/decimal", "sqlx-macros/decimal"]
chrono = ["sqlx-core/chrono", "sqlx-macros/chrono"]
with_chrono = ["sqlx-core/chrono", "sqlx-macros/chrono"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why rename the feature? This is a breaking change.

@abonander
Copy link
Collaborator

Thank you for taking the time to open this PR. However, we're now in the process of splitting out the MSSQL driver for our SQLx Pro offering and it will be rewritten from scratch to fix some longstanding issues, so we will not be accepting your contribution at this time.

We'll make sure that the new MSSQL driver supports the datetime types via chrono and time

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.

4 participants