expression: length(binary(m)) should return 'm' instead of the concrete string length #10671
Conversation
shenli
added
contribution
Codecov Report
@@ Coverage Diff @@
## master #10671 +/- ##
===============================================
- Coverage 79.5906% 78.366% -1.2246%
===============================================
Files 415 414 -1
Lines 88131 87654 -477
===============================================
- Hits 70144 68691 -1453
- Misses 12801 13832 +1031
+ Partials 5186 5131 -55 |
expression/column.go
Outdated
| valLen := len([]rune(val)) | ||
| // See #3644. | ||
| // Binary type column value will changed after modify the column length. | ||
| if (ctx.GetSessionVars().StmtCtx.PadCharToFullLength || (mysql.HasBinaryFlag(uint(col.GetType().Tp)) && col.GetType().Charset == charset.CharsetBin)) && col.GetType().Tp == mysql.TypeString { |
There was a problem hiding this comment.
how about using types.IsBinaryStr()?
miraclesu
force-pushed
the
fix/binary_column_length_after_modify
branch
from
a4d8376
to
299273b
Compare
|
@zz-jason I've added types.IsBinaryStr and used it. |
@miraclesu There is already a function named 83 // IsBinaryStr returns a boolean indicating
84 // whether the field type is a binary string type.
85 func IsBinaryStr(ft *FieldType) bool {
86 if ft.Collate == charset.CollationBin && IsString(ft.Tp) {
87 return true
88 }
89 return false
90 }You can see it in |
| valLen := len([]rune(val)) | ||
| // See #3644. | ||
| // Binary type column value will changed after modify the column length. | ||
| if (ctx.GetSessionVars().StmtCtx.PadCharToFullLength && col.GetType().Tp == mysql.TypeString) || col.GetType().IsBinaryStr() { |
There was a problem hiding this comment.
We store CHAR[N] in TiKV by removing the trailing spaces. But for BINARY[N], we add trailing spaces to N bytes.
When modifying a column from BINARY[X] to BINARY[Y], the underlying data should be regenerated. I think this is the correct way to fix this kind of issue.
@zimulala How do you think?
There was a problem hiding this comment.
@zz-jason
Yes. But I think we can do this first and add a TODO. Wait until the "modify column" can handle the problem of regenerating the data and then remove the logic.
There was a problem hiding this comment.
Okay. I think we'd better file an issue about this bug.
There was a problem hiding this comment.
@zimulala Since these kinds of DDL operations are not working correctly at present. I think a workaround is to forbid these operations. Do not allow modifying a binary(x) column to a binary(y) column if x is not equal to y.
zz-jason
requested review from
zimulala
and removed request for
winoros and
XuHuaiyu
|
@miraclesu friendly ping, any update? |
|
@miraclesu, please update your pull request. |
1 similar comment
|
@miraclesu, please update your pull request. |
What problem does this PR solve?
Fixes #3644.
What is changed and how it works?
Right-padded with the pad value to the specified length.
Check List
Tests
Code changes
N/A
Side effects
N/A
Related changes