From 7a8fdfbbbeb1e1500d1076583f4f134109317140 Mon Sep 17 00:00:00 2001 From: Justin Jaffray Date: Tue, 20 Aug 2019 11:00:56 -0400 Subject: [PATCH] opt,sql: disallow column references in ROWS/RANGE 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. --- pkg/sql/logictest/testdata/logic_test/window | 2 +- pkg/sql/opt/norm/testdata/rules/decorrelate | 84 -------------------- pkg/sql/opt/norm/testdata/rules/prune_cols | 17 ---- pkg/sql/opt/optbuilder/scope.go | 11 --- pkg/sql/opt/optbuilder/testdata/window | 63 +++++---------- pkg/sql/opt/optbuilder/window.go | 21 ++++- pkg/sql/sem/tree/select.go | 20 +++-- 7 files changed, 52 insertions(+), 166 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/window b/pkg/sql/logictest/testdata/logic_test/window index 4625a5c29838..c2e644a6a4e5 100644 --- a/pkg/sql/logictest/testdata/logic_test/window +++ b/pkg/sql/logictest/testdata/logic_test/window @@ -2849,7 +2849,7 @@ iPad {"Microsoft Lumia","HTC One",Nexus,iPhone,"HP Elite","Lenovo Th Kindle Fire {"Microsoft Lumia","HTC One",Nexus,iPhone,"HP Elite","Lenovo Thinkpad","Sony VAIO",Dell,iPad,"Kindle Fire"} Samsung {"Microsoft Lumia","HTC One",Nexus,iPhone,"HP Elite","Lenovo Thinkpad","Sony VAIO",Dell,iPad,"Kindle Fire",Samsung} -statement error GROUPS offset cannot contain variables +statement error GROUPS mode requires an ORDER BY clause SELECT avg(price) OVER (GROUPS group_id PRECEDING) FROM products statement error GROUPS mode requires an ORDER BY clause diff --git a/pkg/sql/opt/norm/testdata/rules/decorrelate b/pkg/sql/opt/norm/testdata/rules/decorrelate index 515930be7189..6184c785ff5e 100644 --- a/pkg/sql/opt/norm/testdata/rules/decorrelate +++ b/pkg/sql/opt/norm/testdata/rules/decorrelate @@ -610,90 +610,6 @@ project └── windows └── row-number [type=int] -norm expect=TryDecorrelateWindow -SELECT - * -FROM - uv, - LATERAL ( - SELECT - row_number() OVER ( - PARTITION BY - s - ORDER BY - f - RANGE - BETWEEN u::FLOAT PRECEDING AND UNBOUNDED FOLLOWING - ), - i - FROM - a - ) ----- -project - ├── columns: u:1(int!null) v:2(int) row_number:8(int) i:4(int) - ├── fd: (1)-->(2) - └── window partition=(1,6) ordering=+5 opt(1,2,6) - ├── columns: u:1(int!null) v:2(int) i:4(int) f:5(float) s:6(string) row_number:8(int) - ├── fd: (1)-->(2) - ├── inner-join (hash) - │ ├── columns: u:1(int!null) v:2(int) i:4(int) f:5(float) s:6(string) - │ ├── fd: (1)-->(2) - │ ├── scan uv - │ │ ├── columns: u:1(int!null) v:2(int) - │ │ ├── key: (1) - │ │ └── fd: (1)-->(2) - │ ├── scan a - │ │ └── columns: i:4(int) f:5(float) s:6(string) - │ └── filters (true) - └── windows - └── windows-item: range from offset to unbounded [type=int, outer=(1)] - └── window-from-offset [type=int] - ├── row-number [type=int] - └── u::FLOAT8 [type=float] - -norm expect=TryDecorrelateWindow -SELECT - * -FROM - uv, - LATERAL ( - SELECT - row_number() OVER ( - PARTITION BY - s - ORDER BY - f - RANGE - BETWEEN UNBOUNDED PRECEDING AND u::FLOAT FOLLOWING - ), - i - FROM - a - ) ----- -project - ├── columns: u:1(int!null) v:2(int) row_number:8(int) i:4(int) - ├── fd: (1)-->(2) - └── window partition=(1,6) ordering=+5 opt(1,2,6) - ├── columns: u:1(int!null) v:2(int) i:4(int) f:5(float) s:6(string) row_number:8(int) - ├── fd: (1)-->(2) - ├── inner-join (hash) - │ ├── columns: u:1(int!null) v:2(int) i:4(int) f:5(float) s:6(string) - │ ├── fd: (1)-->(2) - │ ├── scan uv - │ │ ├── columns: u:1(int!null) v:2(int) - │ │ ├── key: (1) - │ │ └── fd: (1)-->(2) - │ ├── scan a - │ │ └── columns: i:4(int) f:5(float) s:6(string) - │ └── filters (true) - └── windows - └── windows-item: range from unbounded to offset [type=int, outer=(1)] - └── window-to-offset [type=int] - ├── row-number [type=int] - └── u::FLOAT8 [type=float] - norm expect=TryDecorrelateWindow SELECT * FROM uv, LATERAL (SELECT avg(f) FILTER (WHERE u = 3) OVER (), i FROM a) ---- diff --git a/pkg/sql/opt/norm/testdata/rules/prune_cols b/pkg/sql/opt/norm/testdata/rules/prune_cols index 6ff5c862ee63..9f227e8dd3c1 100644 --- a/pkg/sql/opt/norm/testdata/rules/prune_cols +++ b/pkg/sql/opt/norm/testdata/rules/prune_cols @@ -1657,23 +1657,6 @@ project └── ntile [type=int, outer=(2)] └── variable: t.public.a.i [type=int] -# Ensure OFFSET expressions don't get pruned. -norm -SELECT ntile(i) OVER (ORDER BY i RANGE BETWEEN f::INT PRECEDING AND CURRENT ROW) FROM a ----- -project - ├── columns: ntile:5(int) - └── window partition=() ordering=+2 - ├── columns: i:2(int) f:3(float) ntile:5(int) - ├── scan a - │ └── columns: i:2(int) f:3(float) - └── windows - └── windows-item: range from offset to current-row [type=int, outer=(2,3)] - └── window-from-offset [type=int] - ├── ntile [type=int] - │ └── variable: i [type=int] - └── f::INT8 [type=int] - # Ensure filter cols don't get pruned. norm SELECT diff --git a/pkg/sql/opt/optbuilder/scope.go b/pkg/sql/opt/optbuilder/scope.go index b0b098c4efc8..af2b962c2d96 100644 --- a/pkg/sql/opt/optbuilder/scope.go +++ b/pkg/sql/opt/optbuilder/scope.go @@ -1140,7 +1140,6 @@ func (s *scope) replaceWindowFn(f *tree.FuncExpr, def *tree.FunctionDefinition) var ( errOrderByIndexInWindow = pgerror.New(pgcode.FeatureNotSupported, "ORDER BY INDEX in window definition is not supported") - errVarOffsetGroups = pgerror.New(pgcode.Syntax, "GROUPS offset cannot contain variables") ) // analyzeWindowFrame performs semantic analysis of offset expressions of @@ -1175,16 +1174,6 @@ func analyzeWindowFrame(s *scope, windowDef *tree.WindowDef) error { } } case tree.GROUPS: - if startBound != nil && startBound.HasOffset() { - if tree.ContainsVars(startBound.OffsetExpr) { - return errVarOffsetGroups - } - } - if endBound != nil && endBound.HasOffset() { - if tree.ContainsVars(endBound.OffsetExpr) { - return errVarOffsetGroups - } - } if len(windowDef.OrderBy) == 0 { return pgerror.Newf(pgcode.Windowing, "GROUPS mode requires an ORDER BY clause") } diff --git a/pkg/sql/opt/optbuilder/testdata/window b/pkg/sql/opt/optbuilder/testdata/window index cd11585710ee..ed55851015e7 100644 --- a/pkg/sql/opt/optbuilder/testdata/window +++ b/pkg/sql/opt/optbuilder/testdata/window @@ -942,20 +942,7 @@ project build SELECT avg(k) OVER (ORDER BY v RANGE BETWEEN k - 10 PRECEDING AND CURRENT ROW) FROM kv ---- -project - ├── columns: avg:8(decimal) - └── window partition=() ordering=+2 - ├── columns: k:1(int!null) v:2(int) w:3(int) f:4(float) d:5(decimal) s:6(string) b:7(bool) avg:8(decimal) - ├── scan kv - │ └── columns: k:1(int!null) v:2(int) w:3(int) f:4(float) d:5(decimal) s:6(string) b:7(bool) - └── windows - └── windows-item: range from offset to current-row [type=decimal] - └── window-from-offset [type=decimal] - ├── avg [type=decimal] - │ └── variable: k [type=int] - └── minus [type=int] - ├── variable: k [type=int] - └── const: 10 [type=int] +error (42P10): argument of RANGE must not contain variables build SELECT avg(k) OVER (ORDER BY v RANGE BETWEEN UNBOUNDED PRECEDING AND 10 FOLLOWING) FROM kv @@ -1061,35 +1048,7 @@ SELECT FROM kv outer_table ---- -project - ├── columns: avg:16(decimal) - └── window partition=(3) - ├── columns: outer_table.k:1(int!null) outer_table.v:2(int) outer_table.w:3(int) outer_table.f:4(float) outer_table.d:5(decimal) outer_table.s:6(string) outer_table.b:7(bool) avg:16(decimal) - ├── scan outer_table - │ └── columns: outer_table.k:1(int!null) outer_table.v:2(int) outer_table.w:3(int) outer_table.f:4(float) outer_table.d:5(decimal) outer_table.s:6(string) outer_table.b:7(bool) - └── windows - └── windows-item: rows from offset to offset [type=decimal] - └── window-to-offset [type=decimal] - ├── window-from-offset [type=decimal] - │ ├── avg [type=decimal] - │ │ └── variable: outer_table.v [type=int] - │ └── subquery [type=int] - │ └── max1-row - │ ├── columns: count_rows:15(int) - │ └── scalar-group-by - │ ├── columns: count_rows:15(int) - │ ├── project - │ │ └── select - │ │ ├── columns: inner_table.k:8(int!null) inner_table.v:9(int) inner_table.w:10(int) inner_table.f:11(float) inner_table.d:12(decimal) inner_table.s:13(string) inner_table.b:14(bool) - │ │ ├── scan inner_table - │ │ │ └── columns: inner_table.k:8(int!null) inner_table.v:9(int) inner_table.w:10(int) inner_table.f:11(float) inner_table.d:12(decimal) inner_table.s:13(string) inner_table.b:14(bool) - │ │ └── filters - │ │ └── eq [type=bool] - │ │ ├── variable: inner_table.k [type=int] - │ │ └── variable: outer_table.v [type=int] - │ └── aggregations - │ └── count-rows [type=int] - └── const: 1 [type=int] +error (42P10): argument of ROWS must not contain variables build SELECT @@ -1555,7 +1514,7 @@ error (42P20): cannot copy window "w" because it has a frame clause build SELECT rank() OVER (ORDER BY k GROUPS k PRECEDING) FROM kv ---- -error (42601): GROUPS offset cannot contain variables +error (42P10): argument of GROUPS must not contain variables build SELECT @@ -1579,3 +1538,19 @@ project ├── windows-item: range from unbounded to current-row exclude ties [type=int] │ └── rank [type=int] └── rank [type=int] + +exec-ddl +CREATE TABLE table1 (col5 CHAR, col8 INT2); +---- + +build +SELECT + min(tab_536191.col5) OVER ( + ROWS BETWEEN tab_536191.col8 FOLLOWING AND 1 FOLLOWING + ) +FROM + table1 AS tab_536191 +GROUP BY + tab_536191.col8, tab_536191.col5 +---- +error (42P10): argument of ROWS must not contain variables diff --git a/pkg/sql/opt/optbuilder/window.go b/pkg/sql/opt/optbuilder/window.go index 715b557d2067..d2671b88bd60 100644 --- a/pkg/sql/opt/optbuilder/window.go +++ b/pkg/sql/opt/optbuilder/window.go @@ -16,6 +16,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/opt" "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" "github.com/cockroachdb/cockroach/pkg/sql/opt/props/physical" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/errors" @@ -126,6 +128,7 @@ func (b *Builder) buildWindow(outScope *scope, inScope *scope) { b.constructProjectForScope(outScope, argScope) outScope.expr = argScope.expr + var referencedCols opt.ColSet // frames accumulates the set of distinct window frames we're computing over // so that we can group functions over the same partition and ordering. frames := make([]memo.WindowExpr, 0, len(inScope.windows)) @@ -142,7 +145,9 @@ func (b *Builder) buildWindow(outScope *scope, inScope *scope) { b.buildScalar( w.WindowDef.Frame.Bounds.StartBound.OffsetExpr.(tree.TypedExpr), inScope, - nil, nil, nil, + nil, + nil, + &referencedCols, ), ) } @@ -153,7 +158,19 @@ func (b *Builder) buildWindow(outScope *scope, inScope *scope) { b.buildScalar( w.WindowDef.Frame.Bounds.EndBound.OffsetExpr.(tree.TypedExpr), inScope, - nil, nil, nil, + nil, + nil, + &referencedCols, + ), + ) + } + + if !referencedCols.Empty() { + panic( + pgerror.Newf( + pgcode.InvalidColumnReference, + "argument of %s must not contain variables", + tree.WindowModeName(windowFrames[i].Mode), ), ) } diff --git a/pkg/sql/sem/tree/select.go b/pkg/sql/sem/tree/select.go index 9230980b6b59..c9695f6c015e 100644 --- a/pkg/sql/sem/tree/select.go +++ b/pkg/sql/sem/tree/select.go @@ -923,18 +923,24 @@ func (node WindowFrameExclusion) Format(ctx *FmtCtx) { } } -// Format implements the NodeFormatter interface. -func (node *WindowFrame) Format(ctx *FmtCtx) { - switch node.Mode { +// WindowModeName returns the name of the window frame mode. +func WindowModeName(mode WindowFrameMode) string { + switch mode { case RANGE: - ctx.WriteString("RANGE ") + return "RANGE" case ROWS: - ctx.WriteString("ROWS ") + return "ROWS" case GROUPS: - ctx.WriteString("GROUPS ") + return "GROUPS" default: - panic(errors.AssertionFailedf("unhandled case: %d", log.Safe(node.Mode))) + panic(errors.AssertionFailedf("unhandled case: %d", log.Safe(mode))) } +} + +// Format implements the NodeFormatter interface. +func (node *WindowFrame) Format(ctx *FmtCtx) { + ctx.WriteString(WindowModeName(node.Mode)) + ctx.WriteByte(' ') if node.Bounds.EndBound != nil { ctx.WriteString("BETWEEN ") ctx.FormatNode(node.Bounds.StartBound)