Merge pull request #3137 from dolthub/fulghum/ord_agg_fix

Bug fix: ordered aggregates in triggers

Author: fulghum

enginetest/queries/trigger_queries.go

@@ -31,9 +31,12 @@ var TriggerTests = []ScriptTest{
 			"create table b (x int primary key)",
 			"create trigger trig before insert on a for each row begin set new.j = (select coalesce(max(x),1) from b); update b set x = x + 1; end;",
 			"insert into b values (1)",
-			"insert into a values (1,0), (2,0), (3,0)",
 		},
 		Assertions: []ScriptTestAssertion{
+			{
+				Query:    "insert into a values (1,0), (2,0), (3,0)",
+				Expected: []sql.Row{{types.NewOkResult(3)}},
+			},
 			{
 				Query: "select * from a order by i",
 				Expected: []sql.Row{
@@ -60,9 +63,12 @@ var TriggerTests = []ScriptTest{
 			"create table a (i int, j int)",
 			"create table b (x int)",
 			"create trigger trig before insert on a for each row begin set new.j = (select count(x) from b); insert into b values (new.i + new.j); end;",
-			"insert into a values (0,0), (0,0), (0,0)",
 		},
 		Assertions: []ScriptTestAssertion{
+			{
+				Query:    "insert into a values (0,0), (0,0), (0,0)",
+				Expected: []sql.Row{{types.NewOkResult(3)}},
+			},
 			{
 				Query: "select * from a order by j",
 				Expected: []sql.Row{
@@ -3820,6 +3826,52 @@ end;
 			},
 		},
 	},
+	{
+		// https://github.com/dolthub/dolt/issues/9616
+		Name: "delete trigger using GROUP_CONCAT()",
+		SetUpScript: []string{
+			`CREATE TABLE orders (
+					id INT AUTO_INCREMENT PRIMARY KEY,
+					customer_name VARCHAR(100),
+					product VARCHAR(100),
+					order_date DATETIME DEFAULT CURRENT_TIMESTAMP
+				);`,
+			`CREATE TABLE order_audit (
+					audit_id INT AUTO_INCREMENT PRIMARY KEY,
+					customer_name VARCHAR(100),
+					all_products TEXT,
+					audit_time DATETIME DEFAULT CURRENT_TIMESTAMP
+				);`,
+			`CREATE TRIGGER before_order_delete
+					BEFORE DELETE ON orders
+					FOR EACH ROW
+					BEGIN
+					DECLARE products TEXT;
+					SELECT GROUP_CONCAT(product ORDER BY order_date SEPARATOR ', ')
+					INTO products
+					FROM orders
+					WHERE customer_name = OLD.customer_name
+					AND id != OLD.id;
+					INSERT INTO order_audit (customer_name, all_products)
+					VALUES (OLD.customer_name, products);
+					END;`,
+			`INSERT INTO orders (customer_name, product) VALUES
+					('Alice', 'Book'),
+					('Alice', 'Pen'),
+					('Bob', 'Notebook');`,
+		},
+		Assertions: []ScriptTestAssertion{
+			{
+				// Delete a row to trigger the BEFORE DELETE
+				Query:    "DELETE FROM orders WHERE customer_name = 'Alice' AND product = 'Pen';",
+				Expected: []sql.Row{{types.NewOkResult(1)}},
+			},
+			{
+				Query:    "SELECT audit_id, customer_name, all_products FROM order_audit;",
+				Expected: []sql.Row{{1, "Alice", "Book"}},
+			},
+		},
+	},
 }
 
 var TriggerCreateInSubroutineTests = []ScriptTest{

sql/analyzer/fix_exec_indexes.go

@@ -165,6 +165,7 @@ type idxScope struct {
 
 	triggerScope      bool
 	insertSourceScope bool
+	subqueryScope     bool
 }
 
 func (s *idxScope) inTrigger() bool {
@@ -296,10 +297,13 @@ func (s *idxScope) copy() *idxScope {
 		copy(idsCopy, s.ids)
 	}
 	return &idxScope{
-		lateralScopes: lateralCopy,
-		parentScopes:  parentCopy,
-		columns:       varsCopy,
-		ids:           idsCopy,
+		lateralScopes:     lateralCopy,
+		parentScopes:      parentCopy,
+		columns:           varsCopy,
+		ids:               idsCopy,
+		subqueryScope:     s.subqueryScope,
+		triggerScope:      s.triggerScope,
+		insertSourceScope: s.insertSourceScope,
 	}
 }
 
@@ -568,7 +572,7 @@ func (s *idxScope) visitSelf(n sql.Node) error {
 		if proj, isProj := n.(*plan.Project); isProj {
 			switch proj.Child.(type) {
 			case *plan.GroupBy, *plan.Window:
-				if s.inTrigger() && s.inInsertSource() {
+				if s.inTrigger() && !s.subqueryScope {
 					for _, e := range proj.Expressions() {
 						s.expressions = append(s.expressions, fixExprToScope(e, s.childScopes...))
 					}
@@ -581,18 +585,15 @@ func (s *idxScope) visitSelf(n sql.Node) error {
 			// default nodes can't see lateral join nodes, unless we're in lateral
 			// join and lateral scopes are promoted to parent status
 			for _, e := range ne.Expressions() {
-				// OrderedAggregations are special as they append results to the outer scope row
+				// OrderedAggregations are special as they append a new field to the outer scope row
 				// We need to account for this extra column in the rows when assigning indexes
 				// Example: gms/expression/function/aggregation/group_concat.go:groupConcatBuffer.Update()
-				if ordAgg, isOrdAgg := e.(sql.OrderedAggregation); isOrdAgg {
-					selExprs := ordAgg.OutputExpressions()
+				if _, isOrdAgg := e.(sql.OrderedAggregation); isOrdAgg {
 					selScope := &idxScope{}
-					for _, expr := range selExprs {
-						selScope.columns = append(selScope.columns, expr.String())
-						if gf, isGf := expr.(*expression.GetField); isGf {
-							selScope.ids = append(selScope.ids, gf.Id())
-						}
+					if idExpr, isIdExpr := e.(sql.IdExpression); isIdExpr {
+						selScope.ids = append(selScope.ids, idExpr.Id())
 					}
+					selScope.columns = append(selScope.columns, e.String())
 					scope = append(scope, selScope)
 				}
 				s.expressions = append(s.expressions, fixExprToScope(e, scope...))
@@ -620,7 +621,6 @@ func (s *idxScope) finalizeSelf(n sql.Node) (sql.Node, error) {
 		}
 
 		s.ids = columnIdsForNode(n)
-
 		s.addSchema(n.Schema())
 		var err error
 		if s.children != nil {
@@ -752,14 +752,17 @@ func fixExprToScope(e sql.Expression, scopes ...*idxScope) sql.Expression {
 			//  this error for the case of DEFAULT in a `plan.Values`, since we analyze the insert source in isolation (we
 			//  don't have the destination schema, and column references in default values are determined in the build phase)
 
+			// TODO: If we don't find a valid index for a field, we should report an error
 			idx, _ := newScope.getIdxId(e.Id(), e.String())
 			if idx >= 0 {
 				return e.WithIndex(idx), transform.NewTree, nil
 			}
 			return e, transform.SameTree, nil
 		case *plan.Subquery:
 			// this |outScope| prepends the subquery scope
-			newQ, _, err := assignIndexesHelper(e.Query, newScope.push())
+			subqueryScope := newScope.push()
+			subqueryScope.subqueryScope = true
+			newQ, _, err := assignIndexesHelper(e.Query, subqueryScope)
 			if err != nil {
 				return nil, transform.SameTree, err
 			}

sql/expression/function/aggregation/group_concat.go

@@ -306,7 +306,7 @@ func (g *groupConcatBuffer) Update(ctx *sql.Context, originalRow sql.Row) error
 		}
 	}
 
-	// Append the current value to the end of the row. We want to preserve the row's original structure for
+	// Append the current value to the end of the row. We want to preserve the row's original structure
 	// for sort ordering in the final step.
 	g.rows = append(g.rows, append(originalRow, vs))