Skip to content

Commit

Permalink
opt: fix in-place modification of window definition in optbuilder
Browse files Browse the repository at this point in the history
The optbuilder code which handles window functions inadvertently modifies the
`WindowDef` in place. This leads to loss of qualification in the PARTITION BY
and ORDER BY columns (which get replaced with `*scopeColumn`s). This is a
problem for views where the query stored in the descriptor could be invalid
without the qualification.

This change fixes this by making copies as necessary.

Fixes #47704.

Release note (bug fix): fixed case where PARTITION BY and ORDER BY columns in
window specifications were losing qualifications when used inside views.
  • Loading branch information
RaduBerinde committed Apr 20, 2020
1 parent 826ac6b commit 953964b
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 13 deletions.
21 changes: 21 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/views
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,27 @@ SELECT * FROM v1
2 2 true
3 3 true

# Regression test for #47704: the columns inside PARITION BY and ORDER BY were
# losing their qualification.
statement ok
CREATE TABLE a47704 (foo UUID);
CREATE TABLE b47704 (foo UUID)

statement ok
CREATE VIEW v47704 AS
SELECT first_value(a47704.foo) OVER (PARTITION BY a47704.foo ORDER BY a47704.foo)
FROM a47704 JOIN b47704 ON a47704.foo = b47704.foo

# Verify that the descriptor did not "lose" the column qualification inside
# PARITION BY and ORDER BY.
query T
SELECT create_statement FROM [ SHOW CREATE VIEW v47704 ]
----
CREATE VIEW v47704 (first_value) AS SELECT first_value(a47704.foo) OVER (PARTITION BY a47704.foo ORDER BY a47704.foo) FROM test.public.a47704 JOIN test.public.b47704 ON a47704.foo = b47704.foo

statement ok
SELECT * FROM v47704

subtest create_or_replace

user root
Expand Down
35 changes: 24 additions & 11 deletions pkg/sql/opt/optbuilder/scope.go
Original file line number Diff line number Diff line change
Expand Up @@ -1087,7 +1087,7 @@ func (s *scope) lookupWindowDef(name tree.Name) *tree.WindowDef {
panic(pgerror.Newf(pgcode.UndefinedObject, "window %q does not exist", name))
}

func (s *scope) constructWindowDef(def tree.WindowDef) *tree.WindowDef {
func (s *scope) constructWindowDef(def tree.WindowDef) tree.WindowDef {
switch {
case def.RefName != "":
// SELECT rank() OVER (w) FROM t WINDOW w AS (...)
Expand All @@ -1096,14 +1096,16 @@ func (s *scope) constructWindowDef(def tree.WindowDef) *tree.WindowDef {
if err != nil {
panic(err)
}
return &result
return result

case def.Name != "":
// SELECT rank() OVER w FROM t WINDOW w AS (...)
// Note the lack of parens around w, compared to the first case.
// We use the referenced window specification directly, without modification.
return s.lookupWindowDef(def.Name)
return *s.lookupWindowDef(def.Name)

default:
return &def
return def
}
}

Expand All @@ -1122,9 +1124,12 @@ func (s *scope) replaceWindowFn(f *tree.FuncExpr, def *tree.FunctionDefinition)
s.builder.semaCtx.Properties.Require("window",
tree.RejectNestedWindowFunctions)

f.WindowDef = s.constructWindowDef(*f.WindowDef)
// Make a copy of f so we can modify the WindowDef.
fCopy := *f
newWindowDef := s.constructWindowDef(*f.WindowDef)
fCopy.WindowDef = &newWindowDef

expr := f.Walk(s)
expr := fCopy.Walk(s)

typedFunc, err := tree.TypeCheck(expr, s.builder.semaCtx, types.Any)
if err != nil {
Expand All @@ -1149,17 +1154,25 @@ func (s *scope) replaceWindowFn(f *tree.FuncExpr, def *tree.FunctionDefinition)
)
s.builder.semaCtx.Properties.Derived.InWindowFunc = true

for i, e := range f.WindowDef.Partitions {
oldPartitions := f.WindowDef.Partitions
f.WindowDef.Partitions = make(tree.Exprs, len(oldPartitions))
for i, e := range oldPartitions {
typedExpr := s.resolveType(e, types.Any)
f.WindowDef.Partitions[i] = typedExpr
}
for i, e := range f.WindowDef.OrderBy {
if e.OrderType != tree.OrderByColumn {

oldOrderBy := f.WindowDef.OrderBy
f.WindowDef.OrderBy = make(tree.OrderBy, len(oldOrderBy))
for i := range oldOrderBy {
ord := *oldOrderBy[i]
if ord.OrderType != tree.OrderByColumn {
panic(errOrderByIndexInWindow)
}
typedExpr := s.resolveType(e.Expr, types.Any)
f.WindowDef.OrderBy[i].Expr = typedExpr
typedExpr := s.resolveType(ord.Expr, types.Any)
ord.Expr = typedExpr
f.WindowDef.OrderBy[i] = &ord
}

if f.WindowDef.Frame != nil {
if err := analyzeWindowFrame(s, f.WindowDef); err != nil {
panic(err)
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/sem/tree/select.go
Original file line number Diff line number Diff line change
Expand Up @@ -801,13 +801,13 @@ const (
// OverrideWindowDef implements the logic to have a base window definition which
// then gets augmented by a different window definition.
func OverrideWindowDef(base *WindowDef, override WindowDef) (WindowDef, error) {
// referencedSpec.Partitions is always used.
// base.Partitions is always used.
if len(override.Partitions) > 0 {
return WindowDef{}, pgerror.Newf(pgcode.Windowing, "cannot override PARTITION BY clause of window %q", base.Name)
}
override.Partitions = base.Partitions

// referencedSpec.OrderBy is used if set.
// base.OrderBy is used if set.
if len(base.OrderBy) > 0 {
if len(override.OrderBy) > 0 {
return WindowDef{}, pgerror.Newf(pgcode.Windowing, "cannot override ORDER BY clause of window %q", base.Name)
Expand Down

0 comments on commit 953964b

Please sign in to comment.