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

Change conversion for difftime to INTERVAL, not TIME #151

Merged
merged 3 commits into from May 1, 2024
Merged

Conversation

hannes
Copy link
Member

@hannes hannes commented Apr 30, 2024

This PR changes the DuckDB representation of difftime R objects from TIME to INTERVAL. The reasoning here is that R's difftime can represent way larger time differences than single days, which is what TIME is for. This goes wrong if the difftime is longer than one day and one tries for example casting the value to a string.

Here is an example that demonstrates the problem

con <- DBI::dbConnect(duckdb::duckdb())
duckdb::duckdb_register(con, "difftime", data.frame(difftime=structure(710, units = "days", class = "difftime")))
print(DBI::dbGetQuery(con, 'select difftime, difftime::varchar difftime_string from difftime'))

Currently, this returns random garbage for the string

       difftime difftime_string
1 61344000 secs  \xd2\034:00:00

and triggers an assertion when built in debug mode.

With this PR, it returns

       difftime difftime_string
1 61344000 secs        710 days

@hannes hannes requested a review from krlmlr April 30, 2024 11:49
@krlmlr
Copy link
Collaborator

krlmlr commented Apr 30, 2024

Thanks, looks good. Let me sleep over the implications.

I wonder what we should return to R land if the database computes a TIME value. The following seems to work, although I don't understand why. OTOH, looks like we can't create fractional intervals from SQL?

con <- DBI::dbConnect(duckdb::duckdb())
print(DBI::dbGetQuery(con, "select TIME '01:02:03' as t"))
#>           t
#> 1 3723 secs
print(DBI::dbGetQuery(con, "select INTERVAL 1 HOUR as t"))
#>           t
#> 1 3600 secs
print(DBI::dbGetQuery(con, "select INTERVAL 1.2 HOUR as t"))
#> Error: {"exception_type":"Parser","exception_message":"syntax error at or near \"1.2\"","position":"16","error_subtype":"SYNTAX_ERROR"}

Created on 2024-04-30 with reprex v2.1.0

@hannes
Copy link
Member Author

hannes commented May 1, 2024

we've always returned difftime for TIME because AFAIK R does not have another suitable time type.

Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Looks good, just one question.

src/types.cpp Outdated
dtime_t RTimeSecondsType::Convert(double val) {
return dtime_t(int64_t(val * Interval::MICROS_PER_SEC));
interval_t RIntervalSecondsType::Convert(double val) {
return Interval::FromMicro(int64_t(val * Interval::MICROS_PER_SEC));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is consistent with the old implementation, but when converting datetimes I think we're rounding. Should we change this as part of the change here and further below?

Suggested change
return Interval::FromMicro(int64_t(val * Interval::MICROS_PER_SEC));
return Interval::FromMicro(int64_t(round(val * Interval::MICROS_PER_SEC)));

@krlmlr
Copy link
Collaborator

krlmlr commented May 1, 2024

How do I construct a literal interval of 1.5 hours?

I'll try to formulate issues upstream.

@krlmlr
Copy link
Collaborator

krlmlr commented May 1, 2024

No more objections after reading the docs. Tests are failing locally, need to investigate.

@krlmlr krlmlr merged commit ac19ecb into main May 1, 2024
22 of 23 checks passed
@krlmlr krlmlr deleted the timeinterval  branch May 1, 2024 14:41
@krlmlr
Copy link
Collaborator

krlmlr commented May 1, 2024

Thanks, part of the CRAN release just submitted.

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

2 participants