opt,sql: disallow column references in ROWS/RANGE #40832

justinj · 2019-09-17T16:00:07Z

This is a departure from the behaviour of the heuristic planner but is
actually the same behaviour as Postgres:

d=# create table x (a int primary key);
CREATE TABLE
d=# select rank() over (order by a range between a preceding and unbounded following) from x;
ERROR:  argument of RANGE must not contain variables
LINE 1: select rank() over (order by a range between a preceding and...

Our behaviour now:

root@127.0.0.1:60679/movr> CREATE TABLE x (a INT8 PRIMARY KEY)
CREATE TABLE

root@127.0.0.1:60679/movr> SELECT
    rank() OVER (ORDER BY a RANGE BETWEEN a PRECEDING AND UNBOUNDED FOLLOWING)
FROM
    x
pq: ROWS or RANGE cannot contain variables

Release justification: Category 2: Fixes a bug and brings behaviour more
in-line with Postgres.

Release note (sql change): column references are no longer allowed in
ROWS/RANGE clauses in window functions.

cockroach-teamcity · 2019-09-17T16:00:20Z

This change is

RaduBerinde

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @justinj, @RaduBerinde, @rytaft, and @yuzefovich)

pkg/sql/opt/norm/testdata/rules/prune_cols, line 1660 at r1 (raw file):

                └── variable: t.public.a.i [type=int]

# Ensure OFFSET expressions don't get pruned.

This condition is no longer tested by this.. We should either remove or replace with one that does.

pkg/sql/opt/optbuilder/testdata/window, line 1548 at r1 (raw file):

build
SELECT
	min(tab_536191.col5) OVER (

[nit] remove tabs

yuzefovich

Thanks for looking into this! I know this issue has been popping up a lot in the nightly SQLSmith runs.

Reviewed 2 of 4 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @justinj and @rytaft)

pkg/sql/opt/optbuilder/window.go, line 172 at r2 (raw file):

				pgerror.Newf(
					pgcode.InvalidColumnReference,
					"ROWS or RANGE cannot contain variables",

I think it should be the same for GROUPS mode, no? I believe we had a PR that fixed it for GROUPS mode, maybe we need to combine with that?

[nit]: I'd also customize the message for a particular mode (i.e. ROWS cannot contain variables and RANGE cannot contain variables). Also, I thought we attempted our best to match even the error message of Postgres, so maybe use argument of RANGE must not contain variables?

justinj

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @justinj, @rytaft, and @yuzefovich)

pkg/sql/opt/norm/testdata/rules/prune_cols, line 1660 at r1 (raw file):

Previously, RaduBerinde wrote…

This condition is no longer tested by this.. We should either remove or replace with one that does.

Done, removed since I don't think it's relevant any more.

pkg/sql/opt/optbuilder/window.go, line 172 at r2 (raw file):

Previously, yuzefovich wrote…

I think it should be the same for GROUPS mode, no? I believe we had a PR that fixed it for GROUPS mode, maybe we need to combine with that?

[nit]: I'd also customize the message for a particular mode (i.e. ROWS cannot contain variables and RANGE cannot contain variables). Also, I thought we attempted our best to match even the error message of Postgres, so maybe use argument of RANGE must not contain variables?

Ah yeah, good catch! Those can be unified.

I made the change here, but personally I don't think it's very important that our error messages match Postgres, and I'm not aware of any guidelines that dictate that? I think it's important that the error codes match, but the messages should be purely for human consumption.

yuzefovich

Reviewed 5 of 5 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @justinj and @rytaft)

pkg/sql/opt/optbuilder/scope.go, line 1185 at r3 (raw file):

		if endBound != nil && endBound.HasOffset() {
			if tree.ContainsVars(endBound.OffsetExpr) {
				return errVarOffsetGroups

[nit]: this error might no longer be used I think.

pkg/sql/opt/optbuilder/window.go, line 172 at r2 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Ah yeah, good catch! Those can be unified.

I made the change here, but personally I don't think it's very important that our error messages match Postgres, and I'm not aware of any guidelines that dictate that? I think it's important that the error codes match, but the messages should be purely for human consumption.

Thanks.

The "trying to match error message" is the impression I got when I was working on window functions in 2018. The error code was "best effort" and the error message was "nice to have." Personally, I also don't feel strongly about it.

rytaft

Reviewed 1 of 4 files at r1, 1 of 2 files at r2, 5 of 5 files at r3.
Reviewable status: complete! 2 of 0 LGTMs obtained (and 1 stale) (waiting on @justinj)

pkg/sql/sem/tree/select.go, line 927 at r3 (raw file):

// ModeName returns the name of the window frame mode.
func ModeName(mode WindowFrameMode) string {

[nit] I'd name this WindowModeName or something like that to make it clear it relates to window functions

yuzefovich

Reviewable status: complete! 0 of 0 LGTMs obtained (and 3 stale) (waiting on @justinj, @rytaft, and @yuzefovich)

pkg/sql/sem/tree/select.go, line 927 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] I'd name this WindowModeName or something like that to make it clear it relates to window functions

+1

Fixes cockroachdb#38286. This is a departure from the behaviour of the heuristic planner but is actually the same behaviour as Postgres: ``` d=# create table x (a int primary key); CREATE TABLE d=# select rank() over (order by a range between a preceding and unbounded following) from x; ERROR: argument of RANGE must not contain variables LINE 1: select rank() over (order by a range between a preceding and... ``` Our behaviour now: ``` root@127.0.0.1:60679/movr> CREATE TABLE x (a INT8 PRIMARY KEY) CREATE TABLE root@127.0.0.1:60679/movr> SELECT rank() OVER (ORDER BY a RANGE BETWEEN a PRECEDING AND UNBOUNDED FOLLOWING) FROM x pq: ROWS or RANGE cannot contain variables ``` Release justification: Category 2: Fixes a bug and brings behaviour more in-line with Postgres. Release note (sql change): column references are no longer allowed in ROWS/RANGE clauses in window functions.

justinj

TFTRs!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 3 stale) (waiting on @justinj, @rytaft, and @yuzefovich)

pkg/sql/opt/optbuilder/scope.go, line 1185 at r3 (raw file):

Previously, yuzefovich wrote…

[nit]: this error might no longer be used I think.

Done.

pkg/sql/sem/tree/select.go, line 927 at r3 (raw file):

Previously, yuzefovich wrote…

+1

Done.

40832: opt,sql: disallow column references in ROWS/RANGE r=justinj a=justinj Fixes #38286. This is a departure from the behaviour of the heuristic planner but is actually the same behaviour as Postgres: ``` d=# create table x (a int primary key); CREATE TABLE d=# select rank() over (order by a range between a preceding and unbounded following) from x; ERROR: argument of RANGE must not contain variables LINE 1: select rank() over (order by a range between a preceding and... ``` Our behaviour now: ``` root@127.0.0.1:60679/movr> CREATE TABLE x (a INT8 PRIMARY KEY) CREATE TABLE root@127.0.0.1:60679/movr> SELECT rank() OVER (ORDER BY a RANGE BETWEEN a PRECEDING AND UNBOUNDED FOLLOWING) FROM x pq: ROWS or RANGE cannot contain variables ``` Release justification: Category 2: Fixes a bug and brings behaviour more in-line with Postgres. Release note (sql change): column references are no longer allowed in ROWS/RANGE clauses in window functions. Co-authored-by: Justin Jaffray <justin@cockroachlabs.com>

craig · 2019-09-19T15:46:34Z

Build succeeded

GitHub CI (Cockroach)

justinj requested review from rytaft, yuzefovich and RaduBerinde September 17, 2019 16:00

justinj requested a review from a team as a code owner September 17, 2019 16:00

RaduBerinde approved these changes Sep 17, 2019

View reviewed changes

justinj force-pushed the window-fix branch from f6994e2 to 79d0a24 Compare September 17, 2019 17:01

yuzefovich reviewed Sep 17, 2019

View reviewed changes

justinj force-pushed the window-fix branch 3 times, most recently from 4fbb5a1 to 27ca53c Compare September 18, 2019 14:38

justinj commented Sep 18, 2019

View reviewed changes

yuzefovich approved these changes Sep 18, 2019

View reviewed changes

rytaft approved these changes Sep 18, 2019

View reviewed changes

justinj force-pushed the window-fix branch from 27ca53c to e7c190f Compare September 18, 2019 16:46

yuzefovich reviewed Sep 18, 2019

View reviewed changes

justinj force-pushed the window-fix branch from e7c190f to 7a8fdfb Compare September 18, 2019 16:57

justinj commented Sep 19, 2019

View reviewed changes

craig bot merged commit 7a8fdfb into cockroachdb:master Sep 19, 2019

jseldess mentioned this pull request Oct 1, 2019

opt,sql: disallow column references in ROWS/RANGE cockroachdb/docs#5514

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opt,sql: disallow column references in ROWS/RANGE #40832

opt,sql: disallow column references in ROWS/RANGE #40832

justinj commented Sep 17, 2019

cockroach-teamcity commented Sep 17, 2019

RaduBerinde left a comment

yuzefovich left a comment

justinj left a comment

yuzefovich left a comment

rytaft left a comment

yuzefovich left a comment

justinj left a comment

craig bot commented Sep 19, 2019

opt,sql: disallow column references in ROWS/RANGE #40832

opt,sql: disallow column references in ROWS/RANGE #40832

Conversation

justinj commented Sep 17, 2019

cockroach-teamcity commented Sep 17, 2019

RaduBerinde left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

justinj left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

justinj left a comment

Choose a reason for hiding this comment

craig bot commented Sep 19, 2019

Build succeeded