sql: normalize age and timestamptz intervals like postgres #56667

otan · 2020-11-13T20:27:46Z

sql: normalize age and timestamptz intervals like postgres

Release note (sql change, bug fix): Previously, timestamp/timestamptz - timestamp/timestamptz operators would normalize the interval into
months, days, H:M:S (in older versions, this may be just H:M:S).

This could result in an incorrect result:

> select '2020-01-01'::timestamptz - '2018-01-01';
     ?column?
-------------------
  2 years 10 days

This has now been fixed such that it is only normalized into
days/H:M:S. This is now more postgres compatible.

Release note (sql change, bug fix): Previously, the age builtin would
incorrectly normalize months and days based on 30 days a month (in older
versions this may be just H:M:S).

This can give an incorrect result, e.g.

> select age('2020-01-01'::timestamptz, '2018-01-01');
     ?column?
-------------------
  2 years 10 days

This is not the most accurate it could be as age can use the given
timestamptz arguments to be more accurate.
This is now more postgres compatible.

Revert "sql: age returns normalized intervals"

This reverts commit 88a5d94.

Release note: None

cockroach-teamcity · 2020-11-13T20:27:55Z

This change is

solongordon

I think our release note needs to call out that the previous behavior could produce incorrect results for timestamp subtraction. For instance like this:

> select '2020-01-01'::timestamptz - '2018-01-01';
     ?column?
-------------------
  2 years 10 days

It seems hard to spin that as just a "sql change" and not a bug fix.

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

pkg/sql/logictest/testdata/logic_test/timestamp, line 487 at r2 (raw file):

29 days 05:43:04                  -29 days -05:43:04                    29 days 05:43:04    -29 days -05:43:04    29 days 05:43:04    -29 days -05:43:04
-1 mons -1 days -17:16:56         1 mon 1 day 17:16:56                  -29 days -17:16:56  29 days 17:16:56      -29 days -18:16:56  29 days 18:16:56
-18:16:56                         18:16:56                              -18:16:56           18:16:56              -18:16:56           18:16:56

@jordanlewis also proposed having a randomized test which asserts that adding and subtracting the same interval from a timestamp results in the same timestamp. Do you think that would also be valuable here or is this logic test sufficient? Though now that I'm saying this I'm remembering there might be counterexamples of this invariant in Postgres?

pkg/sql/sem/builtins/builtins.go, line 2116 at r2 (raw file):

			Info: "Calculates the interval between `val` and the current time, normalized into years, months and days." + `

			Note this may not be an accurate time span since years and months are normalized from days, and years and months are out of context. Use ` + "`timestamptz - now()`" + ` for day rounding which maintains this accuracy.`,

Did you mean now() - timestamptz? Also "rounding" seems like a confusing term here.

otan · 2020-11-13T21:34:08Z

pkg/sql/logictest/testdata/logic_test/timestamp, line 487 at r2 (raw file):

Previously, solongordon (Solon) wrote…

@jordanlewis also proposed having a randomized test which asserts that adding and subtracting the same interval from a timestamp results in the same timestamp. Do you think that would also be valuable here or is this logic test sufficient? Though now that I'm saying this I'm remembering there might be counterexamples of this invariant in Postgres?

This does not work for age. The other randomized tests testing invariants may be useful (smells like metamorphic tests), I think it's worth filing a separate issue such we that we have those. Sounds like we can generalise these kinds of invariants.

This reverts commit 88a5d94. Release note: None

solongordon

Reviewed 1 of 7 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @otan)

pkg/util/duration/duration.go, line 166 at r4 (raw file):

	// to be too large from the math above anyway.
	for nanos < 0 {
		nanos += int64(time.Nanosecond)

It looks like this should be nanos per second, not int64(time.Nanosecond) which is 1. Could you update this and include nanoseconds in your test cases?

Release note (sql change, bug fix): Previously, `timestamp/timestamptz - timestamp/timestamptz` operators would normalize the interval into months, days, H:M:S (in older versions, this may be just H:M:S). This could result in an incorrect result: ``` > select '2020-01-01'::timestamptz - '2018-01-01'; ?column? ------------------- 2 years 10 days ``` This has now been fixed such that it is only normalized into days/H:M:S. This is now more postgres compatible. Release note (sql change, bug fix): Previously, the `age` builtin would incorrectly normalize months and days based on 30 days a month (in older versions this may be just H:M:S). This can give an incorrect result, e.g. ``` > select age('2020-01-01'::timestamptz, '2018-01-01'); ?column? ------------------- 2 years 10 days ``` This is not the most accurate it could be as `age` can use the given timestamptz arguments to be more accurate. This is now more postgres compatible.

otan · 2020-11-16T18:25:27Z

pkg/util/duration/duration.go, line 166 at r4 (raw file):

Previously, solongordon (Solon) wrote…

It looks like this should be nanos per second, not int64(time.Nanosecond) which is 1. Could you update this and include nanoseconds in your test cases?

ah the one case i wish i had go coverage tools.
nice catch, cheers!
fixed.

solongordon

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

otan · 2020-11-16T19:21:06Z

bors r=solongordon

thanks!

craig · 2020-11-16T20:41:47Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2020-11-16T21:54:00Z

Build succeeded:

GitHub CI (Cockroach)

This commit fixes an oversight of cockroachdb#56667 (which fixed the normalization of the timestamp subtraction) where we forgot to update the vectorized code path as well. The regression tests added in that PR happened to be complex enough so that the vectorized engine would fall back to wrapping a row-by-row processor, so they didn't catch this. Release note (bug fix): CockroachDB previously would not normalize `timestamp/timestamptz - timestamp/timestamptz` like Postgres does in some cases (depending on the query), and this is now fixed.

83812: sql/schemachanger: check nil table name for comment on column r=chengxiong-ruan a=chengxiong-ruan addressing this [build error](#75604 (comment)) throw pg error instead of panic on nil pointer. Release note: None. 83923: roachprod: fix target path when getting cluster artifacts. r=srosenberg a=renatolabs A bug was introduced in the computation of the local path where cluster artifacts are downloaded: instead of prefixing the path with the node ID, the list of nodes would be used instead. This results in (other than weird looking paths, such as `[1 2 3 4].logs`) only the logs for the last node being downloaded. This commit fixes the issue by using the node ID correctly. Release note: None. 83927: storage: clarify and test overlapping SST range keys in `Export` r=nicktrav a=erikgrinaker This patch clarifies that an `Export` with overlapping MVCC range tombstones across two SSTs due to `SplitMidKey` will be handled correctly both by multiplexed iteration with `NewPebbleSSTIterator()`, and by `AddSSTable`. Release note: None 83944: coexec: normalize the timestamp subtraction like postgres r=yuzefovich a=yuzefovich This commit fixes an oversight of #56667 (which fixed the normalization of the timestamp subtraction) where we forgot to update the vectorized code path as well. The regression tests added in that PR happened to be complex enough so that the vectorized engine would fall back to wrapping a row-by-row processor, so they didn't catch this. Fixes: #83094. Release note (bug fix): CockroachDB previously would not normalize `timestamp/timestamptz - timestamp/timestamptz` like Postgres does in some cases (depending on the query), and this is now fixed. Co-authored-by: Chengxiong Ruan <[email protected]> Co-authored-by: Renato Costa <[email protected]> Co-authored-by: Erik Grinaker <[email protected]> Co-authored-by: Yahor Yuzefovich <[email protected]>

This commit fixes an oversight of cockroachdb#56667 (which fixed the normalization of the timestamp subtraction) where we forgot to update the vectorized code path as well. The regression tests added in that PR happened to be complex enough so that the vectorized engine would fall back to wrapping a row-by-row processor, so they didn't catch this. Release note (bug fix): CockroachDB previously would not normalize `timestamp/timestamptz - timestamp/timestamptz` like Postgres does in some cases (depending on the query), and this is now fixed.

otan added backport-20.1.x labels Nov 13, 2020

otan requested review from solongordon and a team November 13, 2020 20:27

otan force-pushed the fix_normalize_bug branch from 8f7f165 to 2f08992 Compare November 13, 2020 20:30

solongordon suggested changes Nov 13, 2020

View reviewed changes

otan force-pushed the fix_normalize_bug branch 5 times, most recently from 6b9eb60 to 2b462d1 Compare November 14, 2020 07:57

Revert "sql: age returns normalized intervals"

6e6e363

This reverts commit 88a5d94. Release note: None

otan force-pushed the fix_normalize_bug branch from 2b462d1 to d9d3c68 Compare November 14, 2020 07:57

solongordon suggested changes Nov 16, 2020

View reviewed changes

otan force-pushed the fix_normalize_bug branch from d9d3c68 to abfe0d5 Compare November 16, 2020 18:25

solongordon approved these changes Nov 16, 2020

View reviewed changes

otan mentioned this pull request Nov 16, 2020

release-19.2: sql: normalize age and timestamptz intervals like postgres #56748

Closed

This was referenced Nov 16, 2020

release-20.1: sql: normalize age and timestamptz intervals like postgres #56750

Closed

release-20.2: sql: normalize age and timestamptz intervals like postgres #56751

Merged

craig bot merged commit 859be50 into cockroachdb:master Nov 16, 2020

solongordon mentioned this pull request Nov 17, 2020

sql: incorrect results for timestamp subtraction #56804

Closed

rafiss added this to the 20.2 milestone Apr 22, 2021

otan mentioned this pull request May 23, 2022

sql: the conversion rules for numeric types to interval are not the same as string to interval #57876

Open

yuzefovich mentioned this pull request Jul 6, 2022

coexec: normalize the timestamp subtraction like postgres #83944

Merged

yuzefovich mentioned this pull request Jul 7, 2022

release-22.1: coexec: normalize the timestamp subtraction like postgres #83999

Merged

yuzefovich mentioned this pull request Jul 7, 2022

release-21.2: coexec: normalize the timestamp subtraction like postgres #84002

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: normalize age and timestamptz intervals like postgres #56667

sql: normalize age and timestamptz intervals like postgres #56667

otan commented Nov 13, 2020 •

edited

Loading

cockroach-teamcity commented Nov 13, 2020

solongordon left a comment

otan commented Nov 13, 2020

solongordon left a comment

otan commented Nov 16, 2020

solongordon left a comment

otan commented Nov 16, 2020

craig bot commented Nov 16, 2020

craig bot commented Nov 16, 2020

sql: normalize age and timestamptz intervals like postgres #56667

sql: normalize age and timestamptz intervals like postgres #56667

Conversation

otan commented Nov 13, 2020 • edited Loading

cockroach-teamcity commented Nov 13, 2020

solongordon left a comment

Choose a reason for hiding this comment

otan commented Nov 13, 2020

solongordon left a comment

Choose a reason for hiding this comment

otan commented Nov 16, 2020

solongordon left a comment

Choose a reason for hiding this comment

otan commented Nov 16, 2020

craig bot commented Nov 16, 2020

craig bot commented Nov 16, 2020

otan commented Nov 13, 2020 •

edited

Loading