fix: change temporal add and subtract #337

rok · 2022-09-19T17:15:23Z

BREAKING CHANGE: change return type of temporal add and adds add/subtract with timezone.

jvanstraten · 2022-09-20T10:35:11Z

Just to be clear, the return type changes are merely fixes for rather obvious copy-paste mistakes, but are nevertheless breaking changes.

I failed to consider that the addition of the timezone argument is also a breaking change. Maybe that should be in yet another PR, just so we can be clearer about what the PRs actually change, i.e. something like "feat: add timezone argument to timezone_tz +/- interval_year function" and "fix: correct return types in datetime add function". I don't know if that's helpful or just adds clutter, though.

jvanstraten

If we don't split this up this looks good to me, but requires an SMC vote according to the proposed governance document. @jacques-n @cpcloud @westonpace

jacques-n · 2022-09-22T16:00:18Z

extensions/functions_datetime.yaml

@@ -104,6 +117,9 @@ scalar_functions:
            value: timestamp_tz
          - name: y
            value: interval_year
+          - name: timezone


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

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.

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.

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.

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.

@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.

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?

extensions/functions_datetime.yaml

CLAassistant · 2022-10-06T23:46:19Z

All committers have signed the CLA.

CLAassistant · 2022-10-06T23:47:11Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

rok · 2022-10-18T21:41:52Z

Pushed the feedback suggestions and rebased. @jvanstraten

github-actions · 2023-01-14T01:02:26Z

ACTION NEEDED

Substrait follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

westonpace · 2023-02-16T15:11:27Z

Why is the timezone not required when adding a day? If a time zone has DST that might mean adding 23 or 25 hours correct?

rok · 2023-02-16T15:35:04Z

Why is the timezone not required when adding a day? If a time zone has DST that might mean adding 23 or 25 hours correct?

Forgive me, it's been a while since I looked at this. I vaguely recall (and perhaps incorrectly) that the day interval is a calendrical unit as opposed to a physical one so want to subtract calendar days not physical units.

westonpace · 2023-02-17T00:53:20Z

Forgive me, it's been a while since I looked at this. I vaguely recall (and perhaps incorrectly) that the day interval is a calendrical unit as opposed to a physical one so want to subtract calendar days not physical units.

Hmm, refreshing my memory I do see this in the spec for interval_day:

Usually stored as separate integers for various components, but only the total number of microseconds is significant, i.e. 1d 0s is considered equal to 0d 86400s.

I suppose that means that we, indeed, do not need a time zone.

westonpace · 2023-02-17T00:54:28Z

extensions/functions_datetime.yaml

@@ -48,7 +53,11 @@ scalar_functions:
            value: timestamp_tz
          - name: y
            value: interval_year
-        return: timestamp
+          - name: timezone
+            constant: true


Remove. Pending ML discussion on constant arguments in scalar functions.

extensions/functions_datetime.yaml

rok · 2023-02-17T13:09:00Z

Thanks for the review @westonpace. I think I addressed all the points.

westonpace · 2023-02-23T19:19:51Z

Thanks for figuring this out.

rok mentioned this pull request Sep 19, 2022

feat: add temporal functions #272

Merged

jvanstraten previously approved these changes Sep 20, 2022

View reviewed changes

jacques-n reviewed Sep 22, 2022

View reviewed changes

rok dismissed jvanstraten’s stale review via 154d027 October 18, 2022 21:27

rok force-pushed the change_temporal_add_subtract branch from 36f4950 to 154d027 Compare October 18, 2022 21:27

rok force-pushed the change_temporal_add_subtract branch from 5d24096 to d82471f Compare October 19, 2022 10:16

rok force-pushed the change_temporal_add_subtract branch 2 times, most recently from e896538 to 329004a Compare November 30, 2022 18:09

rok force-pushed the change_temporal_add_subtract branch from 329004a to ac80d82 Compare January 14, 2023 01:02

rok changed the title ~~feat: change temporal add and subtract~~ fix: change temporal add and subtract Jan 16, 2023

rok requested review from jacques-n, julianhyde and jvanstraten and removed request for jacques-n and julianhyde January 16, 2023 16:05

rok force-pushed the change_temporal_add_subtract branch from ac80d82 to 34d2300 Compare January 16, 2023 20:42

rok force-pushed the change_temporal_add_subtract branch from 34d2300 to d823789 Compare February 13, 2023 10:22

rok force-pushed the change_temporal_add_subtract branch from d823789 to c7714d1 Compare February 16, 2023 15:57

westonpace reviewed Feb 17, 2023

View reviewed changes

rok force-pushed the change_temporal_add_subtract branch from c7714d1 to 634f8db Compare February 17, 2023 13:03

rok force-pushed the change_temporal_add_subtract branch from 634f8db to 1e3e4d7 Compare February 17, 2023 17:53

rok requested review from westonpace and removed request for jvanstraten February 17, 2023 19:10

feat: change temporal add and subtract

080adfc

rok force-pushed the change_temporal_add_subtract branch from 1e3e4d7 to 080adfc Compare February 23, 2023 12:12

westonpace approved these changes Feb 23, 2023

View reviewed changes

westonpace merged commit 1b184cc into substrait-io:main Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: change temporal add and subtract #337

fix: change temporal add and subtract #337

rok commented Sep 19, 2022 •

edited

Loading

jvanstraten commented Sep 20, 2022

jvanstraten left a comment

jacques-n Sep 22, 2022

jvanstraten Sep 26, 2022

jacques-n Sep 27, 2022

julianhyde Sep 27, 2022

jvanstraten Sep 27, 2022

julianhyde Sep 27, 2022

westonpace Feb 16, 2023

CLAassistant commented Oct 6, 2022 •

edited

Loading

CLAassistant commented Oct 6, 2022

rok commented Oct 18, 2022

github-actions bot commented Jan 14, 2023

westonpace commented Feb 16, 2023

rok commented Feb 16, 2023

westonpace commented Feb 17, 2023

westonpace Feb 17, 2023

rok Feb 17, 2023

rok commented Feb 17, 2023

westonpace commented Feb 23, 2023

fix: change temporal add and subtract #337

fix: change temporal add and subtract #337

Conversation

rok commented Sep 19, 2022 • edited Loading

jvanstraten commented Sep 20, 2022

jvanstraten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Oct 6, 2022 • edited Loading

CLAassistant commented Oct 6, 2022

rok commented Oct 18, 2022

github-actions bot commented Jan 14, 2023

westonpace commented Feb 16, 2023

rok commented Feb 16, 2023

westonpace commented Feb 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rok commented Feb 17, 2023

westonpace commented Feb 23, 2023

rok commented Sep 19, 2022 •

edited

Loading

CLAassistant commented Oct 6, 2022 •

edited

Loading