util: refine chunk.SwapColumn to rebuild the column reference #7841
Conversation
util/chunk/chunk.go
Outdated
| // If there exists columns refer to the column to be swapped, we need to | ||
| // re-build the reference. | ||
| refColsIdx := make([]int, 0, len(c.columns)-colIdx) | ||
| for i := colIdx + 1; i < len(c.columns); i++ { |
There was a problem hiding this comment.
we also need to check the column between 0 and colIdx-1
|
please add some tests |
|
@winoros |
|
/run-all-tests |
| // If there exists columns refer to the column to be swapped, we need to | ||
| // re-build the reference. | ||
| refColsIdx := make([]int, 0, len(c.columns)) | ||
| for i := colIdx; i < len(c.columns); i++ { |
There was a problem hiding this comment.
Why don't check 0 - colIdx?
What if column[0] refer to column[1]? Or this situation doesn't happen?
There was a problem hiding this comment.
@crazycs520
I updated the code.
column[i] refers to column[j] will never happen now where i < j.
util/chunk/chunk_test.go
Outdated
| chk2.SwapColumn(2, chk2, 0) | ||
| c.Assert(chk1.columns[0] == chk1.columns[1], check.IsTrue) | ||
| c.Assert(chk1.columns[0] == chk2.columns[0], check.IsFalse) | ||
| c.Assert(chk2.columns[0] == chk2.columns[1], check.IsTrue) |
There was a problem hiding this comment.
Delete redundant code.
How about this:
checkRef := func () {
c.Assert(chk1.columns[0] == chk1.columns[1], check.IsTrue)
c.Assert(chk1.columns[0] == chk2.columns[0], check.IsFalse)
c.Assert(chk2.columns[0] == chk2.columns[1], check.IsTrue)
}
checkRef();
chk1.SwapColumn(0, chk2, 0)
checkRef();
chk1.SwapColumn(0, chk2, 1)
checkRef();
chk2.SwapColumn(1, chk2, 0)
checkRef();
chk2.SwapColumn(1, chk2, 1)
checkRef();
chk2.SwapColumn(1, chk2, 2)
checkRef();
chk2.SwapColumn(2, chk2, 0)
checkRef();
|
|
/run-all-tests |
eurekaka
added
status/LGT2
|
/run-all-tests |
What problem does this PR solve?
If there exist some columns refer to one column in a chunk, we need to rebuild
the reference when call chunk.SwapColumn.
Take an image as an example,
If we call chk2.Reset(), the access to chk1 may cause unexpected panic.
What is changed and how it works?
Check whether exist columns refer to the column to be swapped, if so, record
the index of these columns and rebuild the reference after swapping.
Check List
Tests
To be added.
Code changes
Side effects
none
Related changes
Need to cherry-pick to release 2.0, release 2.1.