opt,sql: disallow column references in ROWS/RANGE#40832
opt,sql: disallow column references in ROWS/RANGE#40832craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
RaduBerinde
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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:
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?
4fbb5a1 to
27ca53c
Compare
justinj
left a comment
There was a problem hiding this comment.
Reviewable status:
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
GROUPSmode, no? I believe we had a PR that fixed it forGROUPSmode, maybe we need to combine with that?[nit]: I'd also customize the message for a particular mode (i.e.
ROWS cannot contain variablesandRANGE cannot contain variables). Also, I thought we attempted our best to match even the error message of Postgres, so maybe useargument 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
left a comment
There was a problem hiding this comment.
Reviewed 5 of 5 files at r3.
Reviewable status:
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
left a comment
There was a problem hiding this comment.
Reviewed 1 of 4 files at r1, 1 of 2 files at r2, 5 of 5 files at r3.
Reviewable status:
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
left a comment
There was a problem hiding this comment.
Reviewable status:
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
left a comment
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
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>
Build succeeded |
Fixes #38286.
This is a departure from the behaviour of the heuristic planner but is
actually the same behaviour as Postgres:
Our behaviour now:
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.