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.

Sign up for GitHub

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

fix: change temporal add and subtract #337

Merged
merged 1 commit into from
Feb 23, 2023
Merged

fix: change temporal add and subtract #337

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 26 additions & 4 deletions extensions/functions_datetime.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,12 @@ scalar_functions:
return: i64
-
name: "add"
description: Add an interval to a date/time type.
description: >-
Add an interval to a date/time type.

Timezone strings must be as defined by IANA timezone database (https://www.iana.org/time-zones).
Examples: "Pacific/Marquesas", "Etc/GMT+1".
If timezone is invalid an error is thrown.
impls:
- args:
- name: x
Expand All @@ -48,7 +53,10 @@ scalar_functions:
value: timestamp_tz
- name: y
value: interval_year
return: timestamp
- name: timezone
rok marked this conversation as resolved.
Show resolved Hide resolved
description: Timezone string from IANA tzdb.
value: string
return: timestamp_tz
- args:
- name: x
value: date
Expand All @@ -66,7 +74,7 @@ scalar_functions:
value: timestamp_tz
- name: y
value: interval_day
return: timestamp
return: timestamp_tz
westonpace marked this conversation as resolved.
Show resolved Hide resolved
- args:
- name: x
value: date
Expand All @@ -91,7 +99,12 @@ scalar_functions:
return: interval_year
-
name: "subtract"
description: Subtract an interval from a date/time type.
description: >-
Subtract an interval from a date/time type.

Timezone strings must be as defined by IANA timezone database (https://www.iana.org/time-zones).
Examples: "Pacific/Marquesas", "Etc/GMT+1".
If timezone is invalid an error is thrown.
impls:
- args:
- name: x
Expand All @@ -105,6 +118,15 @@ scalar_functions:
- name: y
value: interval_year
return: timestamp_tz
- args:
- name: x
value: timestamp_tz
- name: y
value: interval_year
- name: timezone
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm inclined to suggest that we just make a new variation that has timezone rather than change the existing here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason we added it is because, as @rok noted, adding a month/year interval to an instant is ill-defined without timezone information. For example, an instant may be February 28th in one timezone and March 1st in another, so to add a month you'd have to add either 28 or 31 days.

That being said, we could still keep the original one with a note stating that it assumes UTC.

Copy link
Contributor

Choose a reason for hiding this comment

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

That being said, we could still keep the original one with a note stating that it assumes UTC.

Yes, that is what I was requesting.

Copy link

Choose a reason for hiding this comment

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

If someone is adding 3 months to an instant, what makes you think that they want to do it in UTC? I don't think defaulting to UTC helps anyone.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the point is that removing the function is an unnecessary breaking change, and "3 months" is not well-defined without a local date, so it probably would have been interpreted as UTC. But heck, it's not even well-defined with a local date if the current day happens to be November 30.

IMO someone shouldn't be trying to add 3 months to an instant in the first place. It feels like adding an integer to a string and expecting concatenation. Just run the conversions explicitly.

Copy link

Choose a reason for hiding this comment

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

@jvanstraten I pretty much agree. As you may know, I am one of these extremists who believes that instant, local time, and time-with-time-zone should be kept strictly separate, and therefore I would be inclined to remove the original method.

Copy link
Member

Choose a reason for hiding this comment

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

I'm inclined to agree with @julianhyde . I'd rather only add new functions and implementations when needed and not for the sake of convenience overloads (I also agree that the convenience here is rather dubious). Otherwise our set of functions gets unwieldy.

There will be a time when we are forced to leave functions around for deprecation reasons but I don't think we are there yet.

@jacques-n are you -1 on removing the old function? Or was that simply a recommendation?

description: Timezone string from IANA tzdb.
value: string
return: timestamp_tz
- args:
- name: x
value: date
Expand Down