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

Added lubridate specifics for Oracle #267

Closed
wants to merge 2 commits into from
Closed

Added lubridate specifics for Oracle #267

wants to merge 2 commits into from

Conversation

rlh1994
Copy link

@rlh1994 rlh1994 commented Mar 18, 2019

I've supplied the basic changes needed to fully support oracle with the lubridate functions currently supported across all languages.

Main differences are:

  • Forced cast to timestamp for dates to get hour/minute/second - this shouldn't introduce much if any overhead to queries where it already is a timestamp.
  • Today is based on sysdate as CURRENT_DATE has a time in oracle (note this will be the current date of the system the database is running on, not the date of the NLS settings of the database, this can be changed if it is different to the other SQL variants)
  • I've implemented as.Date via an explicit call to Date rather than via a cast - there's 2 reasons for this. The first is that the database optimiser can't identify casts as dates at run time so it might not e.g. identify it should filter on a partition before a join, rather than after. Secondly, the cast to date is NLS setting dependant in Oracle databases; at least in my experience most users will not actually be aware of what this is so the conversion will fail. The method implemented will work for YYYY-MM-DD formats and as as.Date tries YYYY-MM-DD by default this is consistent with the base R method. I do still have the code for the full(ish) version of as.Date/as_date that includes the format argument specific to an Oracle translation however did not include it as the base backend does not support this argument.
  • I've also included support for yday and wday as they were listed explicitly in the backend so assumed they would be supported soon and Oracle has a very easy to_char function to extract them.

@@ -45,9 +45,26 @@ sql_translate_env.Oracle <- function(con) {
as.integer64 = sql_cast("NUMBER(19)"),
as.numeric = sql_cast("NUMBER"),
as.double = sql_cast("NUMBER"),
#as.Date = sql_cast("DATE"),
# Oracle date casting is NLS dependant, the following will work for the default YYYY-MM-DD
as.Date = function(x) sql_expr("DATE " !!x),
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look like valid syntax. Can you please double check?

Copy link
Author

Choose a reason for hiding this comment

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

If you meant the R syntax you're right, not sure why I did it that way, it'll be corrected in my commit.
The SQL syntax is the ANSI SQL string literal. At least in Oracle SQL (I can't speak for others) using cast is dependant on the NLS settings, which means unless you know this and format your date string in advance then you can't be certain it will work. the ANSI uses the YYYY-MM-DD format (which matches the default lubridate format string, at least in my localisation?). I thought this would be the safest way to catch most dates short of adding in and translating the format options?
The last script of the introduction section here shows this example https://oracle-base.com/articles/misc/oracle-dates-timestamps-and-intervals


# https://modern-sql.com/feature/extract
yday = function(x) sql_expr((TO_NUMBER(TO_CHAR(!!x, "DDD")))),
wday = function(x) build_sql("(MOD(1 + TRUNC(", !!x, ") - TRUNC(", !!x, ", 'IW'),7) +1 )"),
Copy link
Member

Choose a reason for hiding this comment

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

If you're using build_sql() you don't need !!

Copy link
Author

Choose a reason for hiding this comment

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

Wasn't aware thanks, corrected in next commit

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.

2 participants