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 partial support for datetime() function #600

Merged
merged 3 commits into from
Jan 5, 2025

Conversation

PThorpe92
Copy link
Contributor

@PThorpe92 PThorpe92 commented Jan 3, 2025

This PR adds the datetime function, with all the support currently that date/time have for modifiers, and julianday function, as well as some additional modifiers for date/time/datetime.

There are a couple considerations here, I left a couple comments but essentially there is going to have to be some more work done to track the state of the expression during the application of modifiers, to handle a bunch of edge-cases like re-applying the same timezone modifier to itself, or converting an integer automatically assumed to be julianday, into epoch, or ceiling/floor which will determine relative addition of time in cases like

2024-01-31 +1 month = 2024-03-02

which was painful enough to get working to begin with.

I couldn't get the julianday_converter library to get the exact same float precision as sqlite, so function is included that matches their output, for some reason floating point math + .floor() would give the correct result. They seem to 'round' to 8 decimal places, and I was able to get this to work with the same output as sqlite, except in cases like 2234.5, in which case we return 2234.5000000 because of the fmt precision:

pub fn exec_julianday(time_value: &OwnedValue) -> Result<String> {
    let dt = parse_naive_date_time(time_value);
    match dt {
        // if we did something heinous like: parse::<f64>().unwrap().to_string()
        // that would solve the precision issue, but dear lord...
        Some(dt) => Ok(format!("{:.1$}", to_julian_day_exact(&dt), 8)),
        None => Ok(String::new()),
    }
}

Suggestions would be appreciated on the float precision issue.

@PThorpe92 PThorpe92 marked this pull request as draft January 3, 2025 19:09
@PThorpe92 PThorpe92 force-pushed the time branch 2 times, most recently from 2c93c59 to 878bad6 Compare January 3, 2025 23:38
@PThorpe92 PThorpe92 marked this pull request as ready for review January 3, 2025 23:39
@penberg penberg changed the title datetime progress Add partial support for datetime() function Jan 4, 2025
Copy link
Contributor

@sonhmai sonhmai left a comment

Choose a reason for hiding this comment

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

thanks!

Comment on lines +229 to +252
### Date and Time Modifiers
| Modifier | Status| Comment |
|----------------|-------|---------------------------------|
| Days | Yes | |
| Hours | Yes | |
| Minutes | Yes | |
| Seconds | Yes | |
| Months | Yes | |
| Years | Yes | |
| TimeOffset | Yes | |
| DateOffset | Yes | |
| DateTimeOffset | Yes | |
| Ceiling | No | |
| Floor | No | |
| StartOfMonth | Yes | |
| StartOfYear | Yes | |
| StartOfDay | Yes | |
| Weekday(N) | Yes | |
| Auto | No | |
| UnixEpoch | No | |
| JulianDay | No | |
| Localtime |Partial| requires fixes to avoid double conversions.|
| Utc |Partial| requires fixes to avoid double conversions.|
| Subsec | Yes | |
Copy link
Contributor

Choose a reason for hiding this comment

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

great! adding the time modifiers makes it very clear 🥇

let mut start_reg = 0;
match args {
Some(args) if args.len() > 1 => {
crate::bail_parse_error!("epoch function with > 1 arguments. Modifiers are not yet supported.");
crate::bail_parse_error!("epoch or julianday function with > 1 arguments. Modifiers are not yet supported.");
Copy link
Contributor

Choose a reason for hiding this comment

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

added #602 for supporting modifiers for these functions later.

@penberg penberg merged commit ba28999 into tursodatabase:main Jan 5, 2025
36 checks passed
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.

3 participants