sql: respect IntervalStyle for parsing #67210

otan · 2021-07-05T03:02:00Z

See individual commits for details.

cockroach-teamcity · 2021-07-05T03:02:08Z

This change is

otan · 2021-07-05T09:53:53Z

thanks for the tip-off @mneverov

rafiss

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

pkg/cli/sql_util.go, line 446 at r2 (raw file):

	// This should really be the same as the session's IntervalStyle
	// but that only affect for negative intervals in the magnitude

nit: should it say "the only effect for negative intervals is in the magnitude of days"?

had trouble reading this comment

pkg/sql/logictest/testdata/logic_test/interval, line 463 at r3 (raw file):

-10 years -22 months 1 day 01:02:03   -11 years -10 mons 1 day +01:02:03
-10 years 22 months -1 day 01:02:03   -8 years -2 mons -1 days +01:02:03
-10 years 22 months -1 day -01:02:03  -8 years -2 mons -1 days -01:02:03

i noticed in PG the + sign is very slightly different for the first two rows. i think it puts the + on the first non-negative one to make it more clear which ones are not negative?

 -10 years 22 months 1 day 01:02:03   | -8 years -2 mons +1 day 01:02:03
 -10 years -22 months 1 day 01:02:03  | -11 years -10 mons +1 day 01:02:03
 -10 years 22 months -1 day 01:02:03  | -8 years -2 mons -1 days +01:02:03
 -10 years 22 months -1 day -01:02:03 | -8 years -2 mons -1 days -01:02:03

pkg/sql/sem/tree/interval_test.go, line 159 at r2 (raw file):

			}

			for style := range duration.IntervalStyle_value {

did this test get moved?

pkg/sql/sem/tree/interval_test.go, line 235 at r2 (raw file):

		}

		for style := range duration.IntervalStyle_value {

did this test get moved?

pkg/sql/sem/tree/pretty_test.go, line 169 at r1 (raw file):

	defer log.Scope(t).Close(t)
	tests := map[string]string{
		// Verify that INTERVAL is maintained.

nit: is this comment incorrect now?

otan

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

pkg/sql/logictest/testdata/logic_test/interval, line 463 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i noticed in PG the + sign is very slightly different for the first two rows. i think it puts the + on the first non-negative one to make it more clear which ones are not negative?
 -10 years 22 months 1 day 01:02:03   | -8 years -2 mons +1 day 01:02:03
 -10 years -22 months 1 day 01:02:03  | -11 years -10 mons +1 day 01:02:03
 -10 years 22 months -1 day 01:02:03  | -8 years -2 mons -1 days +01:02:03
 -10 years 22 months -1 day -01:02:03 | -8 years -2 mons -1 days -01:02:03

i've tacked on a commit to fix this bug.

pkg/sql/sem/tree/interval_test.go, line 159 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

did this test get moved?

it just got indented, the test now works over all styles

pkg/sql/sem/tree/interval_test.go, line 235 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

did this test get moved?

it just got indented, the test now works over all styles

rafiss

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

pkg/sql/sem/tree/interval_test.go, line 159 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

it just got indented, the test now works over all styles

i see.. i was just viewing the diff for the 3rd commit, and it shows that for style := range duration.IntervalStyle_value was removed from TestValidSQLIntervalSyntax and TestInvalidSQLIntervalSyntax. that's what i was asking about. (i do see that the range over all styles was added to other tests though.)

otan · 2021-07-08T21:03:29Z

pkg/sql/sem/tree/interval_test.go, line 159 at r2 (raw file):

TestValidSQLIntervalSyntax

that one is still here

TestInvalidSQLIntervalSyntax

ah I remember! it always errored (output is never set), so the logic after the continue never got hit. i removed it.

rafiss

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

pkg/sql/sem/tree/interval_test.go, line 159 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

TestValidSQLIntervalSyntax

that one is still here

TestInvalidSQLIntervalSyntax

ah I remember! it always errored (output is never set), so the logic after the continue never got hit. i removed it.

it looks like TestValidSQLIntervalSyntax doesn't have it though :P -- look at lines 183 and 194 of the latest revision where it hardcodes IntervalStyle_POSTGRES.

But maybe the test should just be renamed if those test cases can't be parsed when using an intervalstyle other than Postgres?

otan

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

pkg/sql/sem/tree/interval_test.go, line 159 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

it looks like TestValidSQLIntervalSyntax doesn't have it though :P -- look at lines 183 and 194 of the latest revision where it hardcodes IntervalStyle_POSTGRES.

But maybe the test should just be renamed if those test cases can't be parsed when using an intervalstyle other than Postgres?

ah, yes.
woops, unremoved and also fixed a subtle bug that caught. nice catch, don't trust my tired self

rafiss

lgtm!

Reviewed 3 of 4 files at r1, 18 of 24 files at r2, 10 of 10 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

otan · 2021-07-13T04:26:24Z

thanks!

bors r=rafiss

craig · 2021-07-13T05:54:05Z

Build failed:

GitHub CI (Cockroach)

otan · 2021-07-13T06:10:14Z

bors r=rafiss

…

On Tue, 13 Jul 2021, 3:54 pm craig[bot], ***@***.***> wrote: Build failed: - GitHub CI (Cockroach) <https://teamcity.cockroachdb.com/viewLog.html?buildId=3175208&buildTypeId=Cockroach_UnitTests> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#67210 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA32FQYWX3URZ6KZKIRHHVTTXPIIRANCNFSM47Z25SGQ> .

otan · 2021-07-13T06:10:50Z

bors r-

…

On Tue, 13 Jul 2021, 4:09 pm Oliver Tan, ***@***.***> wrote: bors r=rafiss On Tue, 13 Jul 2021, 3:54 pm craig[bot], ***@***.***> wrote: > Build failed: > > - GitHub CI (Cockroach) > <https://teamcity.cockroachdb.com/viewLog.html?buildId=3175208&buildTypeId=Cockroach_UnitTests> > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#67210 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AA32FQYWX3URZ6KZKIRHHVTTXPIIRANCNFSM47Z25SGQ> > . >

craig · 2021-07-13T06:10:51Z

Canceled.

We currently evaluate intervals in the parsing stage, as opposed to the execution case. It is needed to be done in the execution stage to obey IntervalStyle. This commit changes the parsing of `INTERVAL x` to be a cast `x::INTERVAL` to allow for this. Note this should be `CastPrepend`, but that is a heavy handed fix. Release note: None

otan · 2021-07-13T06:37:55Z

bors r=rafiss

craig · 2021-07-13T06:47:29Z

Canceled.

otan · 2021-07-13T06:47:45Z

worst rebase conflict ever!

hopefully this will do

bors r=rafiss

67210: sql: respect IntervalStyle for parsing r=rafiss a=otan See individual commits for details. Co-authored-by: Oliver Tan <otan@cockroachlabs.com>

craig · 2021-07-13T07:56:39Z

Build failed:

GitHub CI (Cockroach)

This commit plumbs IntervalStyle to ParseDInterval and associated friends. This is in preparation for respecting IntervalStyle later on. Release note: None

Release note (sql change): When parsing intervals, IntervalStyle is now taken into account. In particular, IntervalStyle = 'sql_standard' will make all interval fields negative if there is a negative symbol at the front, e.g. `-3 years 1 day` would be `-(3 years 1 day)` in sql_standard and `-3 days, 1 day` in postgres DateStyle.

Release note (bug fix): Postgres style intervals now print a `+` sign for day units if the year/month unit preceding was negative (e.g. `-1 year -2 months 2 days` will now print as `-1 year -2 months +2 days`). Release note (bug fix): SQL Standard intervals will omit the day value if the day value is 0

otan · 2021-07-13T08:04:11Z

bors r=rafiss

otan · 2021-07-13T08:46:06Z

bors r-

craig · 2021-07-13T08:46:08Z

Canceled.

otan · 2021-07-13T08:46:12Z

bors r=rafiss

flake on CI

craig · 2021-07-13T09:26:47Z

Build succeeded:

GitHub CI (Cockroach)

otan force-pushed the intervalstyle_fix branch 2 times, most recently from dcc03f4 to 03ddddd Compare July 5, 2021 09:53

otan changed the title ~~[WIP] sql: respect IntervalStyle for parsing~~ sql: respect IntervalStyle for parsing Jul 5, 2021

otan requested review from rafiss and a team July 5, 2021 09:53

otan marked this pull request as ready for review July 5, 2021 09:53

otan requested a review from a team as a code owner July 5, 2021 09:53

otan force-pushed the intervalstyle_fix branch 3 times, most recently from 7e181a4 to a0d5114 Compare July 5, 2021 21:28

rafiss requested changes Jul 7, 2021

View reviewed changes

otan force-pushed the intervalstyle_fix branch from a0d5114 to cb153a0 Compare July 7, 2021 22:28

otan commented Jul 7, 2021

View reviewed changes

rafiss reviewed Jul 8, 2021

View reviewed changes

otan commented Jul 8, 2021

View reviewed changes

otan force-pushed the intervalstyle_fix branch 3 times, most recently from c0e34c2 to cce8226 Compare July 9, 2021 05:21

otan requested a review from rafiss July 12, 2021 23:47

otan force-pushed the intervalstyle_fix branch from cce8226 to 8057ef7 Compare July 13, 2021 00:26

rafiss approved these changes Jul 13, 2021

View reviewed changes

otan force-pushed the intervalstyle_fix branch from 8057ef7 to 9ba6ac6 Compare July 13, 2021 06:37

otan force-pushed the intervalstyle_fix branch from 9ba6ac6 to f94d0f6 Compare July 13, 2021 06:47

craig bot pushed a commit that referenced this pull request Jul 13, 2021

Merge #67210

bf83c58

67210: sql: respect IntervalStyle for parsing r=rafiss a=otan See individual commits for details. Co-authored-by: Oliver Tan <otan@cockroachlabs.com>

otan added 3 commits July 13, 2021 18:03

sql: plumb IntervalStyle to ParseDInterval

511d6a5

This commit plumbs IntervalStyle to ParseDInterval and associated friends. This is in preparation for respecting IntervalStyle later on. Release note: None

otan force-pushed the intervalstyle_fix branch from f94d0f6 to 19ecc2c Compare July 13, 2021 08:04

craig bot merged commit b3b850e into cockroachdb:master Jul 13, 2021

knz mentioned this pull request Jul 14, 2021

cli,sql: mismatch between CLI code and output of SHOW LAST QUERY STATISTICS #67618

Closed

jseldess mentioned this pull request Sep 8, 2021

sql: respect IntervalStyle for parsing cockroachdb/docs#11365

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: respect IntervalStyle for parsing #67210

sql: respect IntervalStyle for parsing #67210

otan commented Jul 5, 2021

cockroach-teamcity commented Jul 5, 2021

otan commented Jul 5, 2021

rafiss left a comment

otan left a comment

rafiss left a comment

otan commented Jul 8, 2021

rafiss left a comment

otan left a comment

rafiss left a comment

otan commented Jul 13, 2021

craig bot commented Jul 13, 2021

otan commented Jul 13, 2021 via email

otan commented Jul 13, 2021 via email

craig bot commented Jul 13, 2021

otan commented Jul 13, 2021

craig bot commented Jul 13, 2021

otan commented Jul 13, 2021

craig bot commented Jul 13, 2021

otan commented Jul 13, 2021

otan commented Jul 13, 2021

craig bot commented Jul 13, 2021

otan commented Jul 13, 2021

craig bot commented Jul 13, 2021

sql: respect IntervalStyle for parsing #67210

sql: respect IntervalStyle for parsing #67210

Conversation

otan commented Jul 5, 2021

cockroach-teamcity commented Jul 5, 2021

otan commented Jul 5, 2021

rafiss left a comment

Choose a reason for hiding this comment

otan left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

otan commented Jul 8, 2021

rafiss left a comment

Choose a reason for hiding this comment

otan left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

otan commented Jul 13, 2021

craig bot commented Jul 13, 2021

otan commented Jul 13, 2021 via email

otan commented Jul 13, 2021 via email

craig bot commented Jul 13, 2021

otan commented Jul 13, 2021

craig bot commented Jul 13, 2021

otan commented Jul 13, 2021

craig bot commented Jul 13, 2021

otan commented Jul 13, 2021

otan commented Jul 13, 2021

craig bot commented Jul 13, 2021

otan commented Jul 13, 2021

craig bot commented Jul 13, 2021