Skip to content

Commit

Permalink
Merge #40832
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
craig[bot] and Justin Jaffray committed Sep 19, 2019
2 parents 3500394 + 7a8fdfb commit 94ef651
Show file tree
Hide file tree
Showing 7 changed files with 52 additions and 166 deletions.
2 changes: 1 addition & 1 deletion pkg/sql/logictest/testdata/logic_test/window
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
84 changes: 0 additions & 84 deletions pkg/sql/opt/norm/testdata/rules/decorrelate
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
----
Expand Down
17 changes: 0 additions & 17 deletions pkg/sql/opt/norm/testdata/rules/prune_cols
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 0 additions & 11 deletions pkg/sql/opt/optbuilder/scope.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
}
Expand Down
63 changes: 19 additions & 44 deletions pkg/sql/opt/optbuilder/testdata/window
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
21 changes: 19 additions & 2 deletions pkg/sql/opt/optbuilder/window.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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))
Expand All @@ -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,
),
)
}
Expand All @@ -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),
),
)
}
Expand Down
20 changes: 13 additions & 7 deletions pkg/sql/sem/tree/select.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 94ef651

Please sign in to comment.