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 datediff() and dateadd() for hours/minutes/seconds for Spark, Oracle, PostGres, RedShift #347

Conversation

TomWhite-MedStar
Copy link
Contributor

Added translations and test cases for Spark datediff() for non-day intervals.
Solves issue #345

@TomWhite-MedStar TomWhite-MedStar changed the title Added datediff calculation in spark Added datediff() and dateadd() for hours/minutes/seconds for Spark, Oracle, PostGres, RedShift Oct 2, 2023
@schuemie
Copy link
Member

schuemie commented Oct 6, 2023

Thanks! Two questions:

  • Do we still need rules for other dialects? (E.g. Snowflake)
  • Just curious what your thoughts are for rules that don't actually tanslate (e.g. redshift,"DATEADD(day,@days,@date)","DATEADD(day,@days,@date)"

@TomWhite-MedStar
Copy link
Contributor Author

I don't have access to test environments for the other languages. Who can be engaged to make the edits for those?

I am ambivalent about rules that don't translate. One option is to keep them there so that authors know they were considered, unless it risks getting the parser confused.

@schuemie
Copy link
Member

Let's work on the other dialects at the hack-a-thon.

I'm leaning towards not including rules that don't translate. There are many things that don't need translation. If we add all those the set of rules will become massive. It is also a bit of an open set. For example, we don't translate SELECT. Should we add non-translation rules for that?

@schuemie
Copy link
Member

Sorry, this one slipped of my radar. Would you mind removing the rules that don't translate?

@TomWhite-MedStar
Copy link
Contributor Author

@schuemie , I removed the translations that didn't do anything.
I also added translations for negative sub-day time intervals to support the work on OHDSI/Atlas#2886

@TomWhite-MedStar
Copy link
Contributor Author

@schuemie - Note, I only added time-based translations for hour, minute and second. I did not change the default translations for days. This should keep the default behavior (e.g. if add days to a datetime value, the logic will effectively strip off the time portion first). If users truly want 24 hour periods, as opposed to calendar days, they can add 24 * N hours, instead of N days.

@schuemie schuemie merged commit 2cf1d91 into OHDSI:develop Mar 19, 2024
schuemie added a commit that referenced this pull request Mar 20, 2024
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