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

Add Sqlite support for the time crate #1865

Merged
merged 13 commits into from Jul 8, 2022
Merged

Add Sqlite support for the time crate #1865

merged 13 commits into from Jul 8, 2022

Conversation

johnbcodes
Copy link
Contributor

Based on Sqlite support for Chrono and offers similar functionality but is significantly different in the following ways:

  • DateTime doesn't support decoding from a DataType::Float.
  • Decoding from multiple text formats may not be 1:1 with Chrono support

Additionally, I updated the module documentation for type support omissions in Sqlite and corrected JSON type documentation for Mysql.

docs(sqlite): Update types module docs for JSON and Chrono
docs(mysql): Update types module docs for JSON
Copy link

@domodwyer domodwyer left a comment

Choose a reason for hiding this comment

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

Looks awesome ❤️

Proof is in the tests - nice!

sqlx-core/src/mysql/types/mod.rs Show resolved Hide resolved
@abonander
Copy link
Collaborator

All the places where you have a slice of fd!() macros and then loop through it to attempt to parse you can simplify as such:

use time::format_description::FormatItem;

// Attempts the formats in order until one successfully parses or all fail.
let formats = FormatItem::First(&[
    fd!(...),
    ...
]);

OffsetDateTime::parse(string, formats)

@johnbcodes
Copy link
Contributor Author

I've made changes to use FormatItem::First but it was not a drop-in replacement. The error handling in FormatItem::First is slightly different. Successful matches are determined by whether or not the entire format description matches the given input and not the other way around. [year]-[month]-[day] matches 2022-06-13 12:11:10 but the remaining input generates an error.

The solution is breaking up the format descriptions into common smaller pieces and using manual composition with other FormatItem variants. The current macro syntax doesn't support optional components in the description so it has to be manual. The upside is that I think the updated code ends up being more efficient.

I'm less confident about how I handled the variable naming and composition with slices.

@johnbcodes johnbcodes marked this pull request as draft June 15, 2022 12:35
@johnbcodes
Copy link
Contributor Author

Converted to a draft after benchmarking my assumptions and found them to be wanting. I'm going to spend some time looking at performance and then present findings and changes

@johnbcodes
Copy link
Contributor Author

I've made significant performance improvements over the original change and initial attempt to work with FormatItem::First. These changes are were made based on benchmarking several approaches and summarized in the README here.

@johnbcodes johnbcodes marked this pull request as ready for review June 20, 2022 19:16
@abonander abonander merged commit cfef70a into launchbadge:main Jul 8, 2022
@johnbcodes johnbcodes deleted the sqlite-add-time-crate-support branch July 18, 2022 21:39
@billy1624
Copy link
Contributor

Thanks! @johnbcodes for the hard work!! I wonder when will this be released?

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.

None yet

4 participants