From 48b91d9c58c985a579e83c1afff158e64afcc983 Mon Sep 17 00:00:00 2001 From: James Cor Date: Wed, 14 May 2025 16:55:08 -0700 Subject: [PATCH 1/6] first pass at fifx --- sql/analyzer/fix_exec_indexes.go | 36 ++++++++++++++++++++++++++++++++ sql/rowexec/show.go | 1 + 2 files changed, 37 insertions(+) diff --git a/sql/analyzer/fix_exec_indexes.go b/sql/analyzer/fix_exec_indexes.go index 46b06a1fc1..22c6c62f1f 100644 --- a/sql/analyzer/fix_exec_indexes.go +++ b/sql/analyzer/fix_exec_indexes.go @@ -362,8 +362,20 @@ func (s *idxScope) visitChildren(n sql.Node) error { } s.children = append(s.children, newC) } + case *plan.GroupBy: + for _, c := range n.Children() { + newC, cScope, err := assignIndexesHelper(c, s) + if err != nil { + return err + } + s.childScopes = append(s.childScopes, cScope) + s.children = append(s.children, newC) + } default: for _, c := range n.Children() { + if _, ok := c.(*plan.GroupBy); ok { + print() + } newC, cScope, err := assignIndexesHelper(c, s) if err != nil { return err @@ -535,6 +547,24 @@ func (s *idxScope) visitSelf(n sql.Node) error { n.DestSch[colIdx].Default = newDef.(*sql.ColumnDefaultValue) } default: + if proj, isProj := n.(*plan.Project); isProj { + if _, isGb := proj.Child.(*plan.GroupBy); isGb { + for _, e := range proj.Expressions() { + // default nodes can't see lateral join nodes, unless we're in lateral + // join and lateral scopes are promoted to parent status + s.expressions = append(s.expressions, fixExprToScope(e, s.childScopes...)) + } + return nil + } + if _, isGb := proj.Child.(*plan.Window); isGb { + for _, e := range proj.Expressions() { + // default nodes can't see lateral join nodes, unless we're in lateral + // join and lateral scopes are promoted to parent status + s.expressions = append(s.expressions, fixExprToScope(e, s.childScopes...)) + } + return nil + } + } if ne, ok := n.(sql.Expressioner); ok { scope := append(s.parentScopes, s.childScopes...) for _, e := range ne.Expressions() { @@ -694,6 +724,12 @@ func fixExprToScope(e sql.Expression, scopes ...*idxScope) sql.Expression { // don't have the destination schema, and column references in default values are determined in the build phase) idx, _ := newScope.getIdxId(e.Id(), e.String()) + if e.String() == "ref_tbl.id" { + print() + } + if e.String() == "new.id" { + print() + } if idx >= 0 { return e.WithIndex(idx), transform.NewTree, nil } diff --git a/sql/rowexec/show.go b/sql/rowexec/show.go index f765d0c576..1824a63481 100644 --- a/sql/rowexec/show.go +++ b/sql/rowexec/show.go @@ -76,6 +76,7 @@ func (b *BaseBuilder) buildDescribeQuery(ctx *sql.Context, n *plan.DescribeQuery var rows []sql.Row if n.Format.Plan { formatString := sql.Describe(n.Child, n.Format) + formatString = strings.Replace(formatString, "\r", "", -1) for _, l := range strings.Split(formatString, "\n") { if strings.TrimSpace(l) != "" { rows = append(rows, sql.NewRow(l)) From c1974a0ad5a1b7803996d12b012c6295bd0d040f Mon Sep 17 00:00:00 2001 From: James Cor Date: Thu, 15 May 2025 12:47:02 -0700 Subject: [PATCH 2/6] maybe? --- sql/analyzer/fix_exec_indexes.go | 98 ++++++++++++++++++++++++-------- sql/analyzer/inserts.go | 13 ++++- 2 files changed, 84 insertions(+), 27 deletions(-) diff --git a/sql/analyzer/fix_exec_indexes.go b/sql/analyzer/fix_exec_indexes.go index 22c6c62f1f..1073a987e9 100644 --- a/sql/analyzer/fix_exec_indexes.go +++ b/sql/analyzer/fix_exec_indexes.go @@ -161,7 +161,33 @@ type idxScope struct { children []sql.Node expressions []sql.Expression checks sql.CheckConstraints - triggerScope bool + + triggerScope bool + insertSourceScope bool +} + +func (s *idxScope) inTrigger() bool { + if s == nil { + return false + } + for _, ps := range s.parentScopes { + if ps.inTrigger() { + return true + } + } + return s.triggerScope +} + +func (s *idxScope) inInsertSource() bool { + if s == nil { + return false + } + for _, ps := range s.parentScopes { + if ps.inInsertSource() { + return true + } + } + return s.insertSourceScope } func (s *idxScope) addSchema(sch sql.Schema) { @@ -273,6 +299,9 @@ func (s *idxScope) copy() *idxScope { parentScopes: parentCopy, columns: varsCopy, ids: idsCopy, + + triggerScope: s.triggerScope, + insertSourceScope: s.insertSourceScope, } } @@ -339,10 +368,42 @@ func (s *idxScope) visitChildren(n sql.Node) error { // keep only the first union scope to avoid double counting s.childScopes = append(s.childScopes, keepScope) case *plan.InsertInto: - newSrc, _, err := assignIndexesHelper(n.Source, s) + // TODO: special case into sources when in triggers with groupby/window functions + var newSrc sql.Node + var err error + + // TODO: do something cleaner + //var isInTrigger bool + //for _, ps := range s.parentScopes { + // if ps.triggerScope { + // isInTrigger = true + // break + // } + //} + //if isInTrigger { + // if proj, isProj := n.Source.(*plan.Project); isProj { + // switch proj.Child.(type) { + // case *plan.GroupBy, *plan.Window: + // subScope := s.copy() + // subScope.parentScopes = subScope.parentScopes[:0] + // newSrc, _, err = assignIndexesHelper(proj, subScope) + // default: + // newSrc, _, err = assignIndexesHelper(n.Source, s) + // } + // } else { + // newSrc, _, err = assignIndexesHelper(n.Source, s) + // } + //} else { + // newSrc, _, err = assignIndexesHelper(n.Source, s) + //} + + s.insertSourceScope = true + newSrc, _, err = assignIndexesHelper(n.Source, s) if err != nil { return err } + s.insertSourceScope = false + newDst, dScope, err := assignIndexesHelper(n.Destination, s) if err != nil { return err @@ -362,15 +423,6 @@ func (s *idxScope) visitChildren(n sql.Node) error { } s.children = append(s.children, newC) } - case *plan.GroupBy: - for _, c := range n.Children() { - newC, cScope, err := assignIndexesHelper(c, s) - if err != nil { - return err - } - s.childScopes = append(s.childScopes, cScope) - s.children = append(s.children, newC) - } default: for _, c := range n.Children() { if _, ok := c.(*plan.GroupBy); ok { @@ -547,22 +599,18 @@ func (s *idxScope) visitSelf(n sql.Node) error { n.DestSch[colIdx].Default = newDef.(*sql.ColumnDefaultValue) } default: + // TODO: this very specific pattern if proj, isProj := n.(*plan.Project); isProj { - if _, isGb := proj.Child.(*plan.GroupBy); isGb { - for _, e := range proj.Expressions() { - // default nodes can't see lateral join nodes, unless we're in lateral - // join and lateral scopes are promoted to parent status - s.expressions = append(s.expressions, fixExprToScope(e, s.childScopes...)) - } - return nil - } - if _, isGb := proj.Child.(*plan.Window); isGb { - for _, e := range proj.Expressions() { - // default nodes can't see lateral join nodes, unless we're in lateral - // join and lateral scopes are promoted to parent status - s.expressions = append(s.expressions, fixExprToScope(e, s.childScopes...)) + switch proj.Child.(type) { + case *plan.GroupBy, *plan.Window: + if s.inTrigger() && s.inInsertSource() { + for _, e := range proj.Expressions() { + // default nodes can't see lateral join nodes, unless we're in lateral + // join and lateral scopes are promoted to parent status + s.expressions = append(s.expressions, fixExprToScope(e, s.childScopes...)) + } + return nil } - return nil } } if ne, ok := n.(sql.Expressioner); ok { diff --git a/sql/analyzer/inserts.go b/sql/analyzer/inserts.go index f6b989ae5b..cf996488e6 100644 --- a/sql/analyzer/inserts.go +++ b/sql/analyzer/inserts.go @@ -31,7 +31,8 @@ import ( func resolveInsertRows(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Scope, sel RuleSelector, qFlags *sql.QueryFlags) (sql.Node, transform.TreeIdentity, error) { if _, ok := n.(*plan.TriggerExecutor); ok { return n, transform.SameTree, nil - } else if _, ok := n.(*plan.CreateProcedure); ok { + } + if _, ok := n.(*plan.CreateProcedure); ok { return n, transform.SameTree, nil } // We capture all INSERTs along the tree, such as those inside of block statements. @@ -50,7 +51,7 @@ func resolveInsertRows(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Sc source := insert.Source // TriggerExecutor has already been analyzed - if _, ok := insert.Source.(*plan.TriggerExecutor); !ok && !insert.LiteralValueSource { + if _, isTrigExec := insert.Source.(*plan.TriggerExecutor); !isTrigExec && !insert.LiteralValueSource { // Analyze the source of the insert independently if _, ok := insert.Source.(*plan.Values); ok { scope = scope.NewScope(plan.NewProject( @@ -58,6 +59,14 @@ func resolveInsertRows(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Sc plan.NewSubqueryAlias("dummy", "", insert.Source), )) } + //if proj, ok := insert.Source.(*plan.Project); ok { + // if _, ok := proj.Child.(*plan.GroupBy); ok { + // scope = &plan.Scope{} + // } + // if _, ok := proj.Child.(*plan.Window); ok { + // scope = &plan.Scope{} + // } + //} source, _, err = a.analyzeWithSelector(ctx, insert.Source, scope, SelectAllBatches, newInsertSourceSelector(sel), qFlags) if err != nil { return nil, transform.SameTree, err From 90c2189ec94534011039e3411805f5ec665ef43a Mon Sep 17 00:00:00 2001 From: James Cor Date: Thu, 15 May 2025 14:23:30 -0700 Subject: [PATCH 3/6] how about this? --- sql/analyzer/fix_exec_indexes.go | 8 ++------ sql/analyzer/inserts.go | 1 + sql/plan/scope.go | 2 ++ 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/sql/analyzer/fix_exec_indexes.go b/sql/analyzer/fix_exec_indexes.go index 1073a987e9..7f93ccfc45 100644 --- a/sql/analyzer/fix_exec_indexes.go +++ b/sql/analyzer/fix_exec_indexes.go @@ -32,6 +32,7 @@ func assignExecIndexes(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Sc if !scope.IsEmpty() { // triggers s.triggerScope = true + s.insertSourceScope = scope.InInsertSource s.addSchema(scope.Schema()) s = s.push() } @@ -605,8 +606,6 @@ func (s *idxScope) visitSelf(n sql.Node) error { case *plan.GroupBy, *plan.Window: if s.inTrigger() && s.inInsertSource() { for _, e := range proj.Expressions() { - // default nodes can't see lateral join nodes, unless we're in lateral - // join and lateral scopes are promoted to parent status s.expressions = append(s.expressions, fixExprToScope(e, s.childScopes...)) } return nil @@ -772,10 +771,7 @@ func fixExprToScope(e sql.Expression, scopes ...*idxScope) sql.Expression { // don't have the destination schema, and column references in default values are determined in the build phase) idx, _ := newScope.getIdxId(e.Id(), e.String()) - if e.String() == "ref_tbl.id" { - print() - } - if e.String() == "new.id" { + if e.String() == "modeldisambig.id_modelinfo" && e.Index() == 1 { print() } if idx >= 0 { diff --git a/sql/analyzer/inserts.go b/sql/analyzer/inserts.go index cf996488e6..d7dbe0cde1 100644 --- a/sql/analyzer/inserts.go +++ b/sql/analyzer/inserts.go @@ -67,6 +67,7 @@ func resolveInsertRows(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Sc // scope = &plan.Scope{} // } //} + scope.InInsertSource = true // TODO: use a setter? source, _, err = a.analyzeWithSelector(ctx, insert.Source, scope, SelectAllBatches, newInsertSourceSelector(sel), qFlags) if err != nil { return nil, transform.SameTree, err diff --git a/sql/plan/scope.go b/sql/plan/scope.go index 8e43c3d973..8b422d69fa 100644 --- a/sql/plan/scope.go +++ b/sql/plan/scope.go @@ -44,6 +44,8 @@ type Scope struct { inLateralJoin bool joinSiblings []sql.Node JoinTrees []string + + InInsertSource bool } func (s *Scope) SetJoin(b bool) { From d397f966fefa370a0cd62ba9155d255d8d50d6ea Mon Sep 17 00:00:00 2001 From: James Cor Date: Thu, 15 May 2025 14:27:53 -0700 Subject: [PATCH 4/6] nil check --- sql/analyzer/inserts.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sql/analyzer/inserts.go b/sql/analyzer/inserts.go index d7dbe0cde1..22022267c8 100644 --- a/sql/analyzer/inserts.go +++ b/sql/analyzer/inserts.go @@ -67,7 +67,9 @@ func resolveInsertRows(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Sc // scope = &plan.Scope{} // } //} - scope.InInsertSource = true // TODO: use a setter? + if scope != nil { + scope.InInsertSource = true // TODO: use a setter? + } source, _, err = a.analyzeWithSelector(ctx, insert.Source, scope, SelectAllBatches, newInsertSourceSelector(sel), qFlags) if err != nil { return nil, transform.SameTree, err From 4dfa4316e64ac7e78a35d637a667b12fb3749184 Mon Sep 17 00:00:00 2001 From: James Cor Date: Thu, 15 May 2025 15:33:49 -0700 Subject: [PATCH 5/6] adding tests --- enginetest/queries/trigger_queries.go | 44 +++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/enginetest/queries/trigger_queries.go b/enginetest/queries/trigger_queries.go index b13296c6d3..f817eac804 100644 --- a/enginetest/queries/trigger_queries.go +++ b/enginetest/queries/trigger_queries.go @@ -368,6 +368,50 @@ create trigger insert_into_a after insert on a for each row replace into c select d.x+2, 0 from d join b using (x) +where d.x = new.x`, + "insert into a (x,z) values (2,2)", + }, + Query: "select x, y from c order by 1", + Expected: []sql.Row{ + {4, 0}, + }, + }, + { + Name: "trigger insert projection group by index error", + SetUpScript: []string{ + "create table a (x int primary key, y int default 1, z int)", + "create table b (x int primary key)", + "create table c (x int primary key, y tinyint)", + "create table d (x int primary key)", + "insert into b values (1), (2)", + "insert into d values (1), (2)", + ` +create trigger insert_into_a +after insert on a +for each row replace into c +select max(d.x + new.x), 0 from d join b using (x) +where d.x = new.x`, + "insert into a (x,z) values (2,2)", + }, + Query: "select x, y from c order by 1", + Expected: []sql.Row{ + {4, 0}, + }, + }, + { + Name: "trigger insert projection window index error", + SetUpScript: []string{ + "create table a (x int primary key, y int default 1, z int)", + "create table b (x int primary key)", + "create table c (x int primary key, y tinyint)", + "create table d (x int primary key)", + "insert into b values (1), (2)", + "insert into d values (1), (2)", + ` +create trigger insert_into_a +after insert on a +for each row replace into c +select first_value(d.x + new.x) over (partition by (x) order by x), 0 from d join b using (x) where d.x = new.x`, "insert into a (x,z) values (2,2)", }, From 0196445ce5200c91b3387d394031234fcc9eb98d Mon Sep 17 00:00:00 2001 From: James Cor Date: Thu, 15 May 2025 16:01:07 -0700 Subject: [PATCH 6/6] clean up --- sql/analyzer/fix_exec_indexes.go | 49 ++++---------------------------- sql/analyzer/inserts.go | 12 +------- sql/plan/scope.go | 47 ++++++++++++++++-------------- 3 files changed, 32 insertions(+), 76 deletions(-) diff --git a/sql/analyzer/fix_exec_indexes.go b/sql/analyzer/fix_exec_indexes.go index 7f93ccfc45..5f6b823142 100644 --- a/sql/analyzer/fix_exec_indexes.go +++ b/sql/analyzer/fix_exec_indexes.go @@ -32,7 +32,7 @@ func assignExecIndexes(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Sc if !scope.IsEmpty() { // triggers s.triggerScope = true - s.insertSourceScope = scope.InInsertSource + s.insertSourceScope = scope.InInsertSource() s.addSchema(scope.Schema()) s = s.push() } @@ -300,9 +300,6 @@ func (s *idxScope) copy() *idxScope { parentScopes: parentCopy, columns: varsCopy, ids: idsCopy, - - triggerScope: s.triggerScope, - insertSourceScope: s.insertSourceScope, } } @@ -369,42 +366,10 @@ func (s *idxScope) visitChildren(n sql.Node) error { // keep only the first union scope to avoid double counting s.childScopes = append(s.childScopes, keepScope) case *plan.InsertInto: - // TODO: special case into sources when in triggers with groupby/window functions - var newSrc sql.Node - var err error - - // TODO: do something cleaner - //var isInTrigger bool - //for _, ps := range s.parentScopes { - // if ps.triggerScope { - // isInTrigger = true - // break - // } - //} - //if isInTrigger { - // if proj, isProj := n.Source.(*plan.Project); isProj { - // switch proj.Child.(type) { - // case *plan.GroupBy, *plan.Window: - // subScope := s.copy() - // subScope.parentScopes = subScope.parentScopes[:0] - // newSrc, _, err = assignIndexesHelper(proj, subScope) - // default: - // newSrc, _, err = assignIndexesHelper(n.Source, s) - // } - // } else { - // newSrc, _, err = assignIndexesHelper(n.Source, s) - // } - //} else { - // newSrc, _, err = assignIndexesHelper(n.Source, s) - //} - - s.insertSourceScope = true - newSrc, _, err = assignIndexesHelper(n.Source, s) + newSrc, _, err := assignIndexesHelper(n.Source, s) if err != nil { return err } - s.insertSourceScope = false - newDst, dScope, err := assignIndexesHelper(n.Destination, s) if err != nil { return err @@ -426,9 +391,6 @@ func (s *idxScope) visitChildren(n sql.Node) error { } default: for _, c := range n.Children() { - if _, ok := c.(*plan.GroupBy); ok { - print() - } newC, cScope, err := assignIndexesHelper(c, s) if err != nil { return err @@ -600,7 +562,9 @@ func (s *idxScope) visitSelf(n sql.Node) error { n.DestSch[colIdx].Default = newDef.(*sql.ColumnDefaultValue) } default: - // TODO: this very specific pattern + // Group By and Window functions already account for the new/old columns present from triggers + // This means that when indexing the Projections, we should not include the trigger scope(s), which are + // within s.parentScopes. if proj, isProj := n.(*plan.Project); isProj { switch proj.Child.(type) { case *plan.GroupBy, *plan.Window: @@ -771,9 +735,6 @@ func fixExprToScope(e sql.Expression, scopes ...*idxScope) sql.Expression { // don't have the destination schema, and column references in default values are determined in the build phase) idx, _ := newScope.getIdxId(e.Id(), e.String()) - if e.String() == "modeldisambig.id_modelinfo" && e.Index() == 1 { - print() - } if idx >= 0 { return e.WithIndex(idx), transform.NewTree, nil } diff --git a/sql/analyzer/inserts.go b/sql/analyzer/inserts.go index 22022267c8..ececfdfab1 100644 --- a/sql/analyzer/inserts.go +++ b/sql/analyzer/inserts.go @@ -59,17 +59,7 @@ func resolveInsertRows(ctx *sql.Context, a *Analyzer, n sql.Node, scope *plan.Sc plan.NewSubqueryAlias("dummy", "", insert.Source), )) } - //if proj, ok := insert.Source.(*plan.Project); ok { - // if _, ok := proj.Child.(*plan.GroupBy); ok { - // scope = &plan.Scope{} - // } - // if _, ok := proj.Child.(*plan.Window); ok { - // scope = &plan.Scope{} - // } - //} - if scope != nil { - scope.InInsertSource = true // TODO: use a setter? - } + scope.SetInInsertSource(true) source, _, err = a.analyzeWithSelector(ctx, insert.Source, scope, SelectAllBatches, newInsertSourceSelector(sel), qFlags) if err != nil { return nil, transform.SameTree, err diff --git a/sql/plan/scope.go b/sql/plan/scope.go index 8b422d69fa..c1d7c5902a 100644 --- a/sql/plan/scope.go +++ b/sql/plan/scope.go @@ -45,21 +45,7 @@ type Scope struct { joinSiblings []sql.Node JoinTrees []string - InInsertSource bool -} - -func (s *Scope) SetJoin(b bool) { - if s == nil { - return - } - s.inJoin = b -} - -func (s *Scope) SetLateralJoin(b bool) { - if s == nil { - return - } - s.inLateralJoin = b + inInsertSource bool } func (s *Scope) IsEmpty() bool { @@ -320,18 +306,37 @@ func (s *Scope) Schema() sql.Schema { return schema } -func (s *Scope) InJoin() bool { +func (s *Scope) SetJoin(b bool) { if s == nil { - return false + return } - return s.inJoin + s.inJoin = b } -func (s *Scope) InLateralJoin() bool { +func (s *Scope) SetLateralJoin(b bool) { if s == nil { - return false + return + } + s.inLateralJoin = b +} + +func (s *Scope) SetInInsertSource(b bool) { + if s == nil { + return } - return s.inLateralJoin + s.inInsertSource = b +} + +func (s *Scope) InJoin() bool { + return s != nil && s.inJoin +} + +func (s *Scope) InLateralJoin() bool { + return s != nil && s.inLateralJoin +} + +func (s *Scope) InInsertSource() bool { + return s != nil && s.inInsertSource } func (s *Scope) JoinSiblings() []sql.Node {