Skip to content

Change conversion for difftime to INTERVAL, not TIME#151

Merged
krlmlr merged 3 commits into
mainfrom
timeinterval 
May 1, 2024

Hidden character warning

The head ref may contain hidden characters: "timeinterval\u00a0"
Merged

Change conversion for difftime to INTERVAL, not TIME#151
krlmlr merged 3 commits into
mainfrom
timeinterval 

Conversation

@hannes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@krlmlr krlmlr deleted the timeinterval  branch May 1, 2024 14:41
@krlmlr
Copy link
Copy Markdown
Collaborator

krlmlr commented May 1, 2024

Thanks, part of the CRAN release just submitted.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants