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

tree: fix H:M:S related bugs for intervals #43924

Merged
merged 1 commit into from
Jan 22, 2020

Conversation

otan
Copy link
Contributor

@otan otan commented Jan 13, 2020

Resolves #43317.

In addition to the release note below, this change also cleans up some
of the logic deferring duration parsing from time.ParseDuration to
processing the components ourselves. The extensive unit tests, along
with additions, should cover any regressions.

Release note (sql change):

  • This change introduces support having D:H:M, D:M:S.fff or D:H:M:S.fff
    for interval parsing if the first element is a decimal or empty, e.g.
    :04:05 and 1.0:04:05 would be 04:05:00 and 1 day 04:05:00
    respectively.
  • Furthermore, we previously supported having floats in H:M:S formats,
    e.g. "1.0:2.0:3.0" -- which doesn't really make sense. This PR no longer
    allows the M field to be float.

@otan otan requested a review from a team January 13, 2020 18:22
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@otan otan requested a review from solongordon January 14, 2020 22:56
Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @solongordon)

@otan
Copy link
Contributor Author

otan commented Jan 22, 2020

bors r=solongordon

@craig
Copy link
Contributor

craig bot commented Jan 22, 2020

Build failed

@otan otan force-pushed the fix_parsing_3 branch 2 times, most recently from a97de90 to c68049b Compare January 22, 2020 19:22
In addition to the release note below, this change also cleans up some
of the logic deferring duration parsing from `time.ParseDuration` to
processing the components ourselves. The extensive unit tests, along
with additions, should cover any regressions.

Release note (sql change):
* This change introduces support having D:H:M, D:M:S.fff or D:H:M:S.fff
for interval parsing if the first element is a decimal or empty, e.g.
`:04:05` and `1.0:04:05` would be `04:05:00` and `1 day 04:05:00`
respectively.
* Furthermore, we previously supported having floats in H:M:S formats,
e.g. "1.0:2.0:3.0" -- which doesn't really make sense. This PR no longer
allows the M field to be float.
@otan
Copy link
Contributor Author

otan commented Jan 22, 2020

bors r=solongordon

looks like remaining failure is a flake on CI

craig bot pushed a commit that referenced this pull request Jan 22, 2020
43923: duration,builtins: fix interval cast involving years to match postgres r=solongordon a=otan

Refs: #43272

Release note (sql change, backward-incompatible change): Previously,
intervals could be cast to integers and floats. However, this relies on
a year being 365 days. To match `extract('epoch' from interval)`
behavior in postgres/cockroach having a year being 365.25 days, we
accordingly the cast to int and floats have years valued at 365.25 days
in seconds instead of 365.

43924: tree: fix H:M:S related bugs for intervals r=solongordon a=otan

Resolves #43317.

In addition to the release note below, this change also cleans up some
of the logic deferring duration parsing from `time.ParseDuration` to
processing the components ourselves. The extensive unit tests, along
with additions, should cover any regressions.

Release note (sql change):
* This change introduces support having D:H:M, D:M:S.fff or D:H:M:S.fff
for interval parsing if the first element is a decimal or empty, e.g.
`:04:05` and `1.0:04:05` would be `04:05:00` and `1 day 04:05:00`
respectively.
* Furthermore, we previously supported having floats in H:M:S formats,
e.g. "1.0:2.0:3.0" -- which doesn't really make sense. This PR no longer
allows the M field to be float.

44042: sql: introduce localtime and localtimestamp functionality r=solongordon a=otan

Resolves #44032.

Note: a lack of implicit casts here hurt us and make us have to return
every permutation of time(stamp) / time(stamp)tz.

Release note (sql change):
* This PR introduces `localtime`, which by default returns the current
time as the `time` data type (as opposed to `current_time`, which
returns the `timetz` data type).
* This PR introduces `localtimestamp`, which by default returns the
current timestamp as the `timestamp` data type (as opposed to
`current_timestamp`, which returns the `timestamptz` data type).

Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Jan 22, 2020

Build succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

intervals with floating point H:M:S is not parsed the same
3 participants