New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
executor: truncate the trailing spaces for "CHAR[(M)]" types #3878
Conversation
executor/write.go
Outdated
@@ -873,11 +871,27 @@ func (e *InsertValues) getRowsSelect(cols []*table.Column) ([][]types.Datum, err | |||
return rows, nil | |||
} | |||
|
|||
func (e *InsertValues) truncateTrailingSpaces(v *types.Datum) { | |||
if v.Kind() == types.KindNull { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it suitable for all kinds of datum to truncate tail spaces, except KindNull?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, only char[(m)] type with non-null values
executor/write.go
Outdated
@@ -873,11 +871,27 @@ func (e *InsertValues) getRowsSelect(cols []*table.Column) ([][]types.Datum, err | |||
return rows, nil | |||
} | |||
|
|||
func (e *InsertValues) truncateTrailingSpaces(v *types.Datum) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a comment for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM |
@@ -809,6 +809,11 @@ func (s *testSuite) TestStringBuiltin(c *C) { | |||
tk.MustExec(`insert into t values(1, 1.1, "2017-01-01 12:01:01", "12:01:01", "abcdef", 0b10101)`) | |||
result := tk.MustQuery("select length(a), length(b), length(c), length(d), length(e), length(f), length(null) from t") | |||
result.Check(testkit.Rows("1 3 19 8 6 2 <nil>")) | |||
tk.MustExec("drop table if exists t") | |||
tk.MustExec("create table t(a char(20))") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the issue #3660 , the column uses b char (10) binary not null)
. But the test does not use binary charset? Are they the same scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is another issue. mysql use binary as a flag of that column, while tidb use binary flag to indicate the type of that column is binary
@zz-jason Please resolve conflict. |
@XuHuaiyu PTAL |
table/column.go
Outdated
@@ -135,7 +136,7 @@ func CastValues(ctx context.Context, rec []types.Datum, cols []*Column, ignoreEr | |||
} | |||
} | |||
rec[c.Offset] = converted | |||
if c.Tp == mysql.TypeString && !mysql.HasBinaryFlag(c.Flag) { | |||
if c.Tp == mysql.TypeString && c.Collate != charset.CollationBin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if c.Tp == mysql.TypeString && types.IsBinaryStr(c.Tp)
fix the implementation of IsBinaryStr()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, @XuHuaiyu PTAL again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fix #3660