executor: fix a bug of update with outer join #7177

lysu · 2018-07-27T12:54:37Z

What have you changed? (mandatory)

fixes #7176

question

fix update table with outer join, mainly fix two question

a left join b, update a's columns, b should not change or error
a left join b, update b's columns, and b has rows matched, they will be updated
a left join b, update b's columns, and b no matched, b should not insert or any other action

more cases can be seen in new unit test

change

build the join physical plan will mark right table columns
during updated record will check the previous mark and check if all data is null is null filled by join
ignore update and check for these filled null columns

What is the type of the changes? (mandatory)

Bug fix (non-breaking change which fixes an issue)

How has this PR been tested? (mandatory)

unit tests
integration tests
manual tests?

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

no

Does this PR need to be added to the release notes? (mandatory)

Yes

fix a bug of update with outer join

Refer to a related PR or issue link (optional)

#7176

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

in testcase

This change is

lysu · 2018-07-27T12:59:42Z

executor/executor_test.go

+	tk.MustQuery("select id, k, v from t3").Check(testkit.Rows())
+
+	// test left join and right no records but update no records part.
+	tk.MustExec("update t1 left join t2 on t1.k = t2.k set t1.v = t2.v, t2.v = 3") // exchange to t2.v = 3, t1.v = t2.v..will got a bug.


exchange t1.v = t2.v, t2.v = 3 to t2.v = 3, t1.v = t2.v will be fix later - -

winoros · 2018-07-27T15:14:20Z

expression/column.go

@@ -140,6 +140,12 @@ func (col *CorrelatedColumn) ResolveIndices(_ *Schema) Expression {
 func (col *CorrelatedColumn) resolveIndices(_ *Schema) {
 }

+// ColumnIdentifier represents a identifier of column.
+type ColumnIdentifier struct {


Can you wait for this merged?
#7157

@winoros should #7157 cherry-pick to 2.0.6~?

The answer might be no for now.

winoros · 2018-07-27T15:17:56Z

expression/schema.go

@@ -37,7 +37,8 @@ type Schema struct {
 	Columns []*Column
 	Keys    []KeyInfo
 	// TblID2Handle stores the tables' handle column information if we need handle in execution phase.
-	TblID2Handle map[int64][]*Column
+	TblID2Handle  map[int64][]*Column
+	ReplenishCols map[ColumnIdentifier]struct{}


Can this set maintained by INSERT or UPDATE instead of in schema?

jackysp · 2018-07-30T07:21:22Z

executor/write.go

 		// Rebase auto increment id if the field is changed.
-		if mysql.HasAutoIncrementFlag(col.Flag) {
+		if mysql.HasAutoIncrementFlag(col.Flag) && !(checkReplenish && oldDataAllNull) {


I think it is acceptable, but the root cause is not this. It seems MySQL sets the auto-id and checks NULL for it just before writing the row. It is much later than TiDB. Maybe someday we should refactor TiDB's implement.

XuHuaiyu · 2018-07-30T07:23:10Z

If this PR is ready to be reviewed,
DNM can be removed. ^_^

lysu · 2018-07-31T03:34:46Z

PTAL @XuHuaiyu @winoros thx~

XuHuaiyu · 2018-08-01T03:14:29Z

executor/update.go

@@ -173,6 +179,15 @@ func (e *UpdateExec) composeNewRow(rowIdx int, oldRow []types.Datum) ([]types.Da
 	return newRowData, nil
 }

+func allNullDatums(datums []types.Datum, colRange ColumnIndexRange) bool {


s/ allNullDatums/ isAllNull

XuHuaiyu · 2018-08-01T03:16:22Z

executor/builder.go

+	switch p := selectPlan.(type) {
+	case *plan.PhysicalHashJoin:
+		joinType = p.JoinType
+		break


we do not need break in golang

XuHuaiyu · 2018-08-01T03:40:41Z

executor/executor_test.go

+	tk.MustExec("insert into t1 values (1, 0)")
+	tk.MustExec("insert into t4 values (3, 3)")
+
+	// test auto_increment & none-null column in right table of update left join.


We may need @lilin90 's help here...
For all the comments in this function.

-> test the case that the table with auto_increment or none-null columns as the right table of left join

XuHuaiyu · 2018-08-01T04:02:50Z

executor/update.go

+	newRowsData  [][]types.Datum // The new values to be set.
+	fetched      bool
+	cursor       int
+	replenishTbl map[string]ColumnIndexRange


comment for this attribute

XuHuaiyu · 2018-08-01T04:06:20Z

executor/builder.go

+	start, end int
+}
+
+// resolveReplenishTbl resolve sub-join's replenish column info.


We need a more detailed comment for this function.

coocood · 2018-08-01T05:46:55Z

executor/update.go

@@ -163,6 +165,10 @@ func (e *UpdateExec) handleErr(colName model.CIStr, rowIdx int, err error) error
 func (e *UpdateExec) composeNewRow(rowIdx int, oldRow []types.Datum) ([]types.Datum, error) {
 	newRowData := types.CopyRow(oldRow)
 	for _, assign := range e.OrderedList {
+		colRange, isReplenish := e.replenishTbl[assign.Col.DBName.O+"-"+assign.Col.TblName.O]


Is it an optimization?
Can we remove this?

mum..no..it's a bugfix not optimize, remove this will cause https://github.com/pingcap/tidb/pull/7177/files#diff-6d9a596d0d155b4342a598ba359efc8bR2985 failure

coocood · 2018-08-01T05:50:12Z

executor/write.go

@@ -53,14 +53,15 @@ const (
 //     4. lastInsertID (uint64) : the lastInsertID should be set by the newData.
 //     5. err (error) : error in the update.
 func updateRecord(ctx sessionctx.Context, h int64, oldData, newData []types.Datum, modified []bool, t table.Table,
-	onDup bool) (bool, bool, int64, uint64, error) {
+	onDup, checkReplenish bool) (bool, bool, int64, uint64, error) {


Can we move the check out of updateRecord?
This method is too complex already.

coocood · 2018-08-01T05:59:40Z

executor/update.go

+	newRowsData  [][]types.Datum // The new values to be set.
+	fetched      bool
+	cursor       int
+	replenishTbl map[string]ColumnIndexRange


We can use *expression.Column as the key of the map.

lilin90

Pls update comments description.

lilin90 · 2018-08-01T07:46:47Z

executor/builder.go

+}
+
+// Len implements sort.Interface#Less.
+// let ranges first sort by `start` increasing order, and sort by `end` decreasing order if `start` are equal.


first sort by -> first sorted by

and sort by -> and then sorted by

lilin90 · 2018-08-01T07:49:00Z

executor/builder.go

+
+// Len implements sort.Interface#Less.
+// let ranges first sort by `start` increasing order, and sort by `end` decreasing order if `start` are equal.
+// so in foldDuplicate can fold duplicate range by this order.


Please delete the extra "in".

lilin90 · 2018-08-01T07:49:29Z

executor/builder.go

+}
+
+// foldDuplicate removes the duplicate ranges.
+// c must be sorted use sort.Sort.


use -> using

lilin90 · 2018-08-01T07:49:57Z

executor/builder.go

+	return ranges
+}
+
+// findColRange find the range hit by given column index.


find -> finds

lilin90 · 2018-08-01T07:50:39Z

executor/builder.go

+}
+
+// findColRange find the range hit by given column index.
+// c must be sorted use sort.Sort.


use -> using

lilin90 · 2018-08-01T11:19:51Z

executor/executor_test.go

+	tk.MustQuery("select k, v from t1").Check(testkit.Rows("1 <nil>"))
+	tk.MustQuery("select k, v from t2").Check(testkit.Rows())
+
+	// test right join and left no records but update no records part.


-> test right join and the case that the left table has no matching record but has updated the left table columns

lilin90 · 2018-08-01T11:31:52Z

executor/executor_test.go

+	tk.MustQuery("select k, v from t1").Check(testkit.Rows("1 0"))
+	tk.MustQuery("select k, v from t2").Check(testkit.Rows())
+
+	// test left + right join update


-> test the case of right join and left join at the same time

lilin90 · 2018-08-01T11:33:16Z

executor/executor_test.go

+	tk.MustQuery("select k, v from t2").Check(testkit.Rows())
+	tk.MustQuery("select k, v from t4").Check(testkit.Rows("3 4"))
+
+	// test left join and update right data.


-> test normal left join and the case that the right table has matching rows

lilin90 · 2018-08-01T11:40:01Z

executor/executor_test.go

+	tk.MustExec("update t1 left join t2 on t1.k = t2.k set t2.v = 11")
+	tk.MustQuery("select k, v from t2").Check(testkit.Rows("1 11"))
+
+	// test join same table multiple times, update right record and not insert no exists record.


-> test the case of continuously joining the same table and updating the unmatching records

lilin90 · 2018-08-01T11:42:06Z

executor/executor_test.go

+	tk.MustQuery("select k, v from t1").Check(testkit.Rows("1 111"))
+	tk.MustQuery("select k, v from t2").Check(testkit.Rows("1 11"))
+
+	// test left join and left all null record's update.


-> test the left join case that the left table has records but all records are null

thanks very much, ^ ^ I will address them.

lysu · 2018-08-01T11:58:03Z

executor/analyze.go

@@ -72,7 +72,7 @@ func (e *AnalyzeExec) Next(ctx context.Context, chk *chunk.Chunk) error {
 		result := <-resultCh
 		if result.Err != nil {
 			err = result.Err
-			if errors.Trace(err) == analyzeWorkerPanic {


CGO_ENABLED=0 revive -formatter friendly -config revive.toml $(go list ./...| grep -vE "vendor") ✘ error-naming error var analyzeWorkerPanic should have name of the form errFoo /home/robi/Code/go/src/github.com/pingcap/tidb/executor/analyze.go:119:5 ✘ 1 problem (1 error, 0 warnings) Errors: 1 error-naming

lysu · 2018-08-01T12:25:30Z

/run-all-tests

winoros

rest lgtm

winoros · 2018-08-01T12:46:31Z

executor/builder.go

+			cols2Handle = append(cols2Handle, Columns2HandleEntry{offset, end, handleCol.Index})
+		}
+	}
+	sort.Sort(cols2Handle)


Do we need to sort them?

for later find at https://github.com/pingcap/tidb/pull/7177/files/d67a05e0fcf141855ede1651256e10a85f41cf0f#diff-badfcd30d7596a08cd207b8e6ae778e6R1284 we need sorted, and for-loop TblID2Handle doesn't make sure order o o?

winoros

lgtm

zz-jason · 2018-08-01T13:00:39Z

executor/builder.go

+// Columns2HandleEntry represents an mapper from column index to handle index.
+type Columns2HandleEntry struct {
+	start, end int
+	handleIdx  int


How about:

s/handleIndex/handleOrdinal/

s/Columns2HandleEntry/cols2Handle/

s/Columns2Handle/cols2HandleSlice/

We can use the word "ordinal" to replace "index" to avoid misunderstanding.

zz-jason · 2018-08-01T13:08:59Z

executor/builder.go

 	}
 	return updateExec
 }

+// Columns2HandleEntry represents an mapper from column index to handle index.


how about:

// Columns2HandleEntry maps a consecutive columns to a handle column. type Columns2HandleEntry struct { start, end int32 // Represent the ordinal range [start, end) of the consecutive columns. handleOrdinal int32 // Represents the ordinal of the handle column. }

And it seems that start is always equal to handleIdx, do we need to store handleIdx?

handleOrdinal may equal to start or end o.o?

if table without the primary key, the handle will be end
if table with the primary key, the handle will be primary definition index, e.g. create table t5(v int, k int, primary key(k)), will be start=1, end=3, handle=2

If so, I think to use getTableOffset to decide the begin offset of a table is wrong:

199 func getTableOffset(schema *expression.Schema, handleCol *expression.Column) int { 200 for i, col := range schema.Columns { 201 if col.DBName.L == handleCol.DBName.L && col.TblName.L == handleCol.TblName.L { 202 return i 203 } 204 } 205 panic("Couldn't get column information when do update/delete") 206 }

from this three logic:

https://github.com/lysu/tidb/blob/44a2ad7f0f65e5c5ea3dab0f20a34fe5a357e58a/plan/logical_plan_builder.go#L1817

https://github.com/lysu/tidb/blob/44a2ad7f0f65e5c5ea3dab0f20a34fe5a357e58a/plan/point_get_plan.go#L260

https://github.com/lysu/tidb/blob/44a2ad7f0f65e5c5ea3dab0f20a34fe5a357e58a/plan/find_best_task.go#L312

make sure handle will in [start, end].....

for getTableOffset..it will works if schema.Columns is order by columns group that grouped by table... e.g. |t1.x, t1.y| t2.z, t2,u|....

this logic is same to https://github.com/lysu/tidb/blob/e5922f4ff012e748bb51bd7e854fe02a9813fd93/executor/update.go#L45 update#exec

zz-jason · 2018-08-01T13:21:28Z

executor/builder.go

+	if biggerOne == 0 {
+		return 0, false
+	}
+	if c[biggerOne-1].start <= colIndex && colIndex < c[biggerOne-1].end {


I think here c[biggerOne-1].start <= colIndex is guaranteed to be true, we only need to check colIndex < c[biggerOne-1].end

zz-jason · 2018-08-01T13:21:44Z

executor/builder.go

+
+// findHandle finds the range hit by given column index.
+// c must be sorted using sort.Sort.
+func (c Columns2Handle) findHandle(colIndex int) (int, bool) {


how about s/colIndex/colOrdinal/ or s/colIndex/ordinal/ ?

zz-jason · 2018-08-01T13:25:15Z

executor/builder.go

+}
+
+// findHandle finds the range hit by given column index.
+// c must be sorted using sort.Sort.


How about simplify the comment to "findHandle finds the ordinal of the corresponding handle column."? Since the slice is sorted when created, there is no need to specify the sorted preconditions.

zz-jason · 2018-08-01T13:42:04Z

executor/update.go

@@ -62,6 +63,11 @@ func (e *UpdateExec) exec(schema *expression.Schema) ([]types.Datum, error) {
 		for _, col := range cols {
 			offset := getTableOffset(schema, col)
 			end := offset + len(tbl.WritableCols())
+			handleDatum := row[col.Index]
+			// handleDatum is nil only when outer join fill no matched columns.


How about wrapping a function to check whether we can update the current record:

// canNotUpdate checks the handle of a record to decide whether that record // can not be updated. The handle is NULL only when it is the inner side of an // outer join: the outer row can not match any inner rows, and in this scenario // the inner handle field is filled with a NULL value. // // This fixes: https://github.com/pingcap/tidb/issues/7176. func (e *UpdateExec) canNotUpdate(handle types.Datum) bool { return handle.IsNull() }

zz-jason · 2018-08-01T13:52:21Z

executor/builder.go

+// Len implements sort.Interface#Less.
+// let ranges first sorted by `start` increasing order, and then sorted by `end` increasing order if `start` are equal.
+func (c Columns2Handle) Less(i, j int) bool {
+	if c[i].start == c[j].start {


This situation should never happen?

I'm not very sure about this...so add this protection.... 23333

lysu · 2018-08-01T15:22:45Z

/run-all-tests

zz-jason · 2018-08-01T15:50:54Z

executor/builder.go

+}
+
+// Len implements sort.Interface#Less.
+// let ranges first sorted by `start` increasing order, and then sorted by `end` increasing order if `start` are equal.


the comments need to be updated 😄

zz-jason · 2018-08-01T15:51:38Z

executor/builder.go

 	}
 	return updateExec
 }

+// cols2Handle represents an mapper from column index to handle index.
+type cols2Handle struct {
+	start, end    int // Represent the ordinal range [start, end) of the consecutive columns.


how about int32?

why? you mean combine two fields into one int32?

ok, I know fixed...

zz-jason

LGTM

zz-jason · 2018-08-02T03:04:47Z

@XuHuaiyu PTAL

XuHuaiyu · 2018-08-02T02:55:14Z

executor/builder.go

+	c[i], c[j] = c[j], c[i]
+}
+
+// Len implements sort.Interface#Less.


s/ Len/ Less

XuHuaiyu · 2018-08-02T02:55:30Z

executor/builder.go

+	return len(c)
+}
+
+// Len implements sort.Interface#Swap.


XuHuaiyu · 2018-08-02T02:56:03Z

executor/builder.go

+
+// findHandle finds the ordinal of the corresponding handle column.
+func (c cols2HandleSlice) findHandle(ordinal int32) (int32, bool) {
+	if c == nil || len(c) == 0 {


if c == nil,
c.findHandle will panic?

receiver is slice(point), so it's ok

XuHuaiyu · 2018-08-02T03:03:02Z

executor/builder.go

+
+// buildColumns2Handle build columns to handle mapping.
+func buildColumns2Handle(schema *expression.Schema, tblID2Table map[int64]table.Table) cols2HandleSlice {
+	if len(schema.TblID2Handle) < 2 {


We need a comment for this check.

Any test case covers this?

old case tested this...for update single table update t set x =1 where y = 1

...but I had added one in executor_test TestUpdateJoin~

XuHuaiyu · 2018-08-02T03:06:00Z

executor/builder.go

+	if c == nil || len(c) == 0 {
+		return 0, false
+	}
+	biggerOne := sort.Search(len(c), func(i int) bool { return c[i].start > ordinal })


We need a comment for biggerOne.

XuHuaiyu · 2018-08-02T03:08:50Z

executor/builder.go

+	if biggerOne == 0 {
+		return 0, false
+	}
+	if ordinal < c[biggerOne-1].end {


Will this always be true if we reach here?

for now ...it's true...previous maybe no...- - fixed~

XuHuaiyu · 2018-08-02T03:09:05Z

executor/builder.go

+	return 0, false
+}
+
+// buildColumns2Handle build columns to handle mapping.


s/ build/ builds

XuHuaiyu · 2018-08-02T03:09:48Z

executor/update.go

+	newRowsData    [][]types.Datum // The new values to be set.
+	fetched        bool
+	cursor         int
+	columns2Handle cols2HandleSlice


we need a comment for this attribute.

XuHuaiyu

rest LGTM

XuHuaiyu · 2018-08-02T05:04:26Z

executor/builder.go

 	}
 	return updateExec
 }

+// cols2Handle represents an mapper from column index to handle index.
+type cols2Handle struct {
+	start, end    int32 // Represent the ordinal range [start, end) of the consecutive columns.


Put the comment on the top line of the attribute, since it's a complete sentence

…ecord

…null.

XuHuaiyu · 2018-08-02T05:18:46Z

/run-all-tests

XuHuaiyu

LGTM

zz-jason

LGTM

lysu added type/bugfix This PR fixes a bug. sig/execution SIG execution labels Jul 27, 2018

lysu commented Jul 27, 2018

View reviewed changes

lysu added status/all tests passed labels Jul 27, 2018

winoros reviewed Jul 27, 2018

View reviewed changes

jackysp reviewed Jul 30, 2018

View reviewed changes

lysu removed the status/DNM label Jul 30, 2018

lysu requested review from coocood, XuHuaiyu and zz-jason July 31, 2018 07:27

lysu added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Jul 31, 2018

XuHuaiyu reviewed Aug 1, 2018

View reviewed changes

coocood reviewed Aug 1, 2018

View reviewed changes

lilin90 reviewed Aug 1, 2018

View reviewed changes

lysu commented Aug 1, 2018

View reviewed changes

winoros reviewed Aug 1, 2018

View reviewed changes

winoros previously approved these changes Aug 1, 2018

View reviewed changes

zz-jason reviewed Aug 1, 2018

View reviewed changes

lysu dismissed winoros’s stale review via c321fe1 August 1, 2018 14:18

zz-jason reviewed Aug 1, 2018

View reviewed changes

zz-jason previously approved these changes Aug 2, 2018

View reviewed changes

lysu added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 2, 2018

XuHuaiyu reviewed Aug 2, 2018

View reviewed changes

lysu dismissed zz-jason’s stale review via e7d664d August 2, 2018 03:40

XuHuaiyu reviewed Aug 2, 2018

View reviewed changes

lysu added 17 commits August 2, 2018 13:15

executor: fix a bug of update with outer join

4688629

executor: modify updateExec instead of schema.

cf5d42f

executor: use new column identify.

eee7bc5

executor: use new column identify.

36dabb0

executor: use table alias and fix update reference column bug.

be85e6e

executor: optimize no match mark index & address comments.

3dfef7a

executor: address comments.

ea7b387

executor: address comments/handle update no match fill out of updateR…

588bd6d

…ecord

executor: address comments/use handle column is nil to detect filled …

9d1e212

…null.

executor: skip buildColumns2Handle if there are no join.

9907e7d

executor: address comment.

b0782e9

executor: address comment.

cae1651

executor: address comment.

7c0f2dc

executor: address comment.

17cb430

executor: address comment.

b935b3b

executor: address comment.

de5d250

executor: address comment.

a43eed8

XuHuaiyu approved these changes Aug 2, 2018

View reviewed changes

zz-jason approved these changes Aug 2, 2018

View reviewed changes

zz-jason merged commit 67e724a into pingcap:master Aug 2, 2018

lysu mentioned this pull request Aug 31, 2018

executor: fix update join result when join table order changed #7571

Merged

lysu deleted the dev/fix_outer_join_update branch September 27, 2018 04:14

executor: fix a bug of update with outer join #7177

executor: fix a bug of update with outer join #7177

Conversation

lysu commented Jul 27, 2018 • edited Loading

What have you changed? (mandatory)

question

change

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuHuaiyu commented Jul 30, 2018

lysu commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilin90 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu commented Aug 1, 2018

winoros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winoros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu commented Jul 27, 2018 •

edited

Loading

lysu Aug 1, 2018 •

edited

Loading

lysu Aug 1, 2018 •

edited

Loading