From 5f3d080dcf755e4bdcbb4ae683f7df00b5da6733 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Mon, 20 Apr 2020 10:42:46 -0700 Subject: [PATCH] opt: fix in-place modification of window definition in optbuilder 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. --- pkg/sql/logictest/testdata/logic_test/views | 21 +++++++++++++ pkg/sql/opt/optbuilder/scope.go | 35 ++++++++++++++------- pkg/sql/sem/tree/select.go | 4 +-- 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/views b/pkg/sql/logictest/testdata/logic_test/views index 1f11fde770ae..626a81e9a767 100644 --- a/pkg/sql/logictest/testdata/logic_test/views +++ b/pkg/sql/logictest/testdata/logic_test/views @@ -537,3 +537,24 @@ query T SELECT create_statement FROM [SHOW CREATE w3] ---- CREATE VIEW w3 (x) AS (WITH t AS (SELECT x FROM test.public.w) SELECT x FROM t) + +# 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 diff --git a/pkg/sql/opt/optbuilder/scope.go b/pkg/sql/opt/optbuilder/scope.go index 61fdae90a5be..c7759a47d4bf 100644 --- a/pkg/sql/opt/optbuilder/scope.go +++ b/pkg/sql/opt/optbuilder/scope.go @@ -1026,7 +1026,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 (...) @@ -1035,14 +1035,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 } } @@ -1061,9 +1063,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 { @@ -1088,17 +1093,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) diff --git a/pkg/sql/sem/tree/select.go b/pkg/sql/sem/tree/select.go index f59776581ac8..fb29fbeaf290 100644 --- a/pkg/sql/sem/tree/select.go +++ b/pkg/sql/sem/tree/select.go @@ -819,13 +819,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)