From 5d9de8e36c8b78411f588ab9bb280a4d881ead53 Mon Sep 17 00:00:00 2001 From: Barakat Date: Thu, 18 Jan 2024 22:35:55 -0600 Subject: [PATCH 1/7] Add test that fails when: * An update query contains a left join where only some rows join * The update query contains a where clause on the join table and/or joins a table with default values --- enginetest/queries/script_queries.go | 42 ++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/enginetest/queries/script_queries.go b/enginetest/queries/script_queries.go index 8930a68104..b6ce66b13e 100644 --- a/enginetest/queries/script_queries.go +++ b/enginetest/queries/script_queries.go @@ -5167,6 +5167,48 @@ CREATE TABLE tab3 ( }, }, }, + { + Name: "update with left join with some missing rows", + SetUpScript: []string{ + `create table joinparent ( + id int not null auto_increment, + name varchar(128) not null, + archived int default 0 not null, + archived_at datetime null, + primary key (id) + );`, + `insert into joinparent (name) values + ('first'), + ('second'), + ('third'), + ('fourth'), + ('fifth');`, + `create index joinparent_archived on joinparent (archived, archived_at);`, + `create table joinchild ( + id int not null auto_increment, + name varchar(128) not null, + parent_id int not null, + archived int default 0 not null, + archived_at datetime null, + primary key (id), + constraint joinchild_parent unique (parent_id, id, archived));`, + `insert into joinchild (name, parent_id) values + ('first', 4), + ('second', 3), + ('third', 2);`, + }, + Assertions: []ScriptTestAssertion{ + { + Query: `update joinparent as jp + left join joinchild as jc on jc.parent_id = jp.id + set jp.archived = jp.id, jp.archived_at = now(), + jc.archived = jc.id, jc.archived_at = now() + order by jp.name + limit 100`, + Expected: []sql.Row{{types.OkResult{RowsAffected: 8, Info: plan.UpdateInfo{Matched: 10, Updated: 8}}}}, + }, + }, + }, } var SpatialScriptTests = []ScriptTest{ From e7e0a1a41d84382a37872843eee5a59f0ddf28c6 Mon Sep 17 00:00:00 2001 From: Barakat Date: Thu, 18 Jan 2024 22:37:24 -0600 Subject: [PATCH 2/7] Fix update queries with left joins failing to ignore null rows when `plan.JoinNode` is wrapped by other nodes --- sql/rowexec/update.go | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/sql/rowexec/update.go b/sql/rowexec/update.go index fb79f10c71..54a1cb3813 100644 --- a/sql/rowexec/update.go +++ b/sql/rowexec/update.go @@ -17,7 +17,6 @@ package rowexec import ( "errors" "fmt" - "github.com/dolthub/go-mysql-server/sql" "github.com/dolthub/go-mysql-server/sql/plan" ) @@ -256,9 +255,36 @@ func (u *updateJoinIter) Next(ctx *sql.Context) (sql.Row, error) { } } +func toJoinNode(node sql.Node) *plan.JoinNode { + switch n := node.(type) { + case *plan.JoinNode: + return n + case *plan.TopN: + return toJoinNode(n.Child) + case *plan.Filter: + return toJoinNode(n.Child) + case *plan.Project: + return toJoinNode(n.Child) + case *plan.Limit: + return toJoinNode(n.Child) + case *plan.Offset: + return toJoinNode(n.Child) + case *plan.Sort: + return toJoinNode(n.Child) + case *plan.Distinct: + return toJoinNode(n.Child) + case *plan.Having: + return toJoinNode(n.Child) + case *plan.Window: + return toJoinNode(n.Child) + default: + return nil + } +} + func isRightOrLeftJoin(node sql.Node) bool { - jn, ok := node.(*plan.JoinNode) - if !ok { + jn := toJoinNode(node) + if jn == nil { return false } return jn.JoinType().IsLeftOuter() @@ -269,8 +295,8 @@ func isRightOrLeftJoin(node sql.Node) bool { // the left or right side of the join (given the direction). A row of all nils that does not pass condition 1 must not // be part of the update operation. This is follows the logic as established in the joinIter. func (u *updateJoinIter) shouldUpdateDirectionalJoin(ctx *sql.Context, joinRow, tableRow sql.Row) (bool, error) { - jn := u.joinNode.(*plan.JoinNode) - if !jn.JoinType().IsLeftOuter() { + jn := toJoinNode(u.joinNode) + if jn == nil || !jn.JoinType().IsLeftOuter() { return true, fmt.Errorf("expected left join") } From 5b1849d731264aec40ca3c59ef0ddc57b43c4cec Mon Sep 17 00:00:00 2001 From: Barakat Date: Thu, 18 Jan 2024 23:04:37 -0600 Subject: [PATCH 3/7] Updated test to trigger indexed table access --- enginetest/queries/script_queries.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/enginetest/queries/script_queries.go b/enginetest/queries/script_queries.go index b6ce66b13e..5fd7c0083a 100644 --- a/enginetest/queries/script_queries.go +++ b/enginetest/queries/script_queries.go @@ -5200,9 +5200,10 @@ CREATE TABLE tab3 ( Assertions: []ScriptTestAssertion{ { Query: `update joinparent as jp - left join joinchild as jc on jc.parent_id = jp.id + left join joinchild as jc on jc.parent_id = jp.id set jp.archived = jp.id, jp.archived_at = now(), jc.archived = jc.id, jc.archived_at = now() + where jp.id > 0 and jp.name != "never" order by jp.name limit 100`, Expected: []sql.Row{{types.OkResult{RowsAffected: 8, Info: plan.UpdateInfo{Matched: 10, Updated: 8}}}}, From 1668c8200e9187ed31335b2617c4acc7e4d8f6cb Mon Sep 17 00:00:00 2001 From: Barakat Date: Thu, 18 Jan 2024 23:05:01 -0600 Subject: [PATCH 4/7] Prevent errors when joins are backed by indexed table access --- sql/rowexec/update.go | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/sql/rowexec/update.go b/sql/rowexec/update.go index 54a1cb3813..80bdc6556c 100644 --- a/sql/rowexec/update.go +++ b/sql/rowexec/update.go @@ -282,6 +282,20 @@ func toJoinNode(node sql.Node) *plan.JoinNode { } } +func isIndexedAccess(node sql.Node) bool { + switch n := node.(type) { + case *plan.Filter: + return isIndexedAccess(n.Child) + case *plan.TableAlias: + return isIndexedAccess(n.Child) + case *plan.JoinNode: + return isIndexedAccess(n.Left()) + case *plan.IndexedTableAccess: + return true + } + return false +} + func isRightOrLeftJoin(node sql.Node) bool { jn := toJoinNode(node) if jn == nil { @@ -305,7 +319,7 @@ func (u *updateJoinIter) shouldUpdateDirectionalJoin(ctx *sql.Context, joinRow, if err != nil { return true, err } - if v, ok := val.(bool); ok && v { + if v, ok := val.(bool); ok && v && !isIndexedAccess(jn) { return true, nil } From 8423ec53f77f46524496c9893ff359b7ddc32f0e Mon Sep 17 00:00:00 2001 From: Barakat Date: Thu, 18 Jan 2024 23:16:17 -0600 Subject: [PATCH 5/7] Add test query for testing `plan.Sort` --- enginetest/queries/script_queries.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/enginetest/queries/script_queries.go b/enginetest/queries/script_queries.go index 5fd7c0083a..6d7a9ad653 100644 --- a/enginetest/queries/script_queries.go +++ b/enginetest/queries/script_queries.go @@ -5208,6 +5208,16 @@ CREATE TABLE tab3 ( limit 100`, Expected: []sql.Row{{types.OkResult{RowsAffected: 8, Info: plan.UpdateInfo{Matched: 10, Updated: 8}}}}, }, + // do without limit to use `plan.Sort` instead of `plan.TopN` + { + Query: `update joinparent as jp + left join joinchild as jc on jc.parent_id = jp.id + set jp.archived = 0, jp.archived_at = null, + jc.archived = 0, jc.archived_at = null + where jp.id > 0 and jp.name != "never" + order by jp.name`, + Expected: []sql.Row{{types.OkResult{RowsAffected: 8, Info: plan.UpdateInfo{Matched: 10, Updated: 8}}}}, + }, }, }, } From 8ec08f6ea3145bfc21698cfdbc1c592385c41d25 Mon Sep 17 00:00:00 2001 From: Max Hoffman Date: Tue, 23 Jan 2024 11:45:23 -0800 Subject: [PATCH 6/7] fix bad merge --- enginetest/queries/script_queries.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/enginetest/queries/script_queries.go b/enginetest/queries/script_queries.go index 2bf9da8a44..aa3f122a09 100644 --- a/enginetest/queries/script_queries.go +++ b/enginetest/queries/script_queries.go @@ -5217,6 +5217,8 @@ CREATE TABLE tab3 ( where jp.id > 0 and jp.name != "never" order by jp.name`, Expected: []sql.Row{{types.OkResult{RowsAffected: 8, Info: plan.UpdateInfo{Matched: 10, Updated: 8}}}}, + }, + }, }, { Name: "count distinct decimals", From 43e8afd49ab7f30bac038f3d547ee6503afac604 Mon Sep 17 00:00:00 2001 From: Max Hoffman Date: Tue, 23 Jan 2024 11:47:47 -0800 Subject: [PATCH 7/7] fmt --- sql/rowexec/update.go | 1 + 1 file changed, 1 insertion(+) diff --git a/sql/rowexec/update.go b/sql/rowexec/update.go index 80bdc6556c..dc494a116d 100644 --- a/sql/rowexec/update.go +++ b/sql/rowexec/update.go @@ -17,6 +17,7 @@ package rowexec import ( "errors" "fmt" + "github.com/dolthub/go-mysql-server/sql" "github.com/dolthub/go-mysql-server/sql/plan" )