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

bug fix: flag of builtin 'IFNULL' 's result is not consistent with mysql #4158

Merged
merged 29 commits into from Aug 21, 2017

Conversation

Projects
None yet
5 participants
@spongedu
Contributor

spongedu commented Aug 12, 2017

The flag out 'IFNULL' 's return seems not consistent with mysql under some circumstances.
Here are my test. my mysql server version is 5.7.17.
I have a table like this:
table_schema

in mysql:

mysql> select ifnull(a,c) from t;
Field 1: ifnull(a,c)
Catalog: def
Database: Table:
Org_table: ``
Type: STRING
Collation: utf8_general_ci (33)
Length: 33
Max_length: 0
Decimals: 31
Flags:

0 rows in set (0.00 sec)

in tidb:

tidb> select ifnull(a,c) from t;
Field 1: ifnull(a,c)
Catalog: def
Database: Table:
Org_table: ``
Type: STRING
Collation: utf8_general_ci (33)
Length: 21
Max_length: 0
Decimals: 31
Flags: BINARY

0 rows in set (0.00 sec)

The 'Banary' Flag is not expected. Another case:

in mysql:

mysql> select ifnull(b,g) from t;
Field 1: ifnull(b,g)
Catalog: def
Database: Table:
Org_table: ``
Type: VAR_STRING
Collation: utf8_general_ci (33)
Length: 66
Max_length: 0
Decimals: 31
Flags: NOT_NULL

in tidb:

tidb> select ifnull(b,g) from t;
Field 1: ifnull(b,g)
Catalog: def
Database: Table:
Org_table: ``
Type: VAR_STRING
Collation: utf8_general_ci (33)
Length: 4294967294
Max_length: 0
Decimals: 31
Flags: BINARY

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason
Member

zz-jason commented Aug 13, 2017

@XuHuaiyu PTAL

Show outdated Hide outdated util/types/field_type.go Outdated
Show outdated Hide outdated expression/builtin_control.go Outdated
Show outdated Hide outdated expression/builtin_control.go Outdated
Show outdated Hide outdated plan/typeinfer_test.go Outdated

spongedu added some commits Aug 14, 2017

1. Add funtion 'AggTypeClass4FieldType' to adjust FieldType' Charset,…
… Collate and Flag at same time if Binary String met 2. add more typeinfer tests
Show outdated Hide outdated util/types/field_type.go Outdated
@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Aug 17, 2017

Contributor

hi, any update in this PR~ @spongedu

Contributor

XuHuaiyu commented Aug 17, 2017

hi, any update in this PR~ @spongedu

@bailaohe

This comment has been minimized.

Show comment
Hide comment
@bailaohe

bailaohe Aug 17, 2017

Contributor

Why VarString is usually with decimals 31. Cannot find any reference.

When Charset of String type is utf8_general_ci. Flen should be 3*strlen(tp). TIDB handles this wrong at present.

Contributor

bailaohe commented Aug 17, 2017

Why VarString is usually with decimals 31. Cannot find any reference.

When Charset of String type is utf8_general_ci. Flen should be 3*strlen(tp). TIDB handles this wrong at present.

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Aug 18, 2017

Contributor

@bailaohe

  1. ported from MySQL, you can find it from the source code of MySQL.
  2. yeah, it's a known issue, we set the length to be character length right now.
Contributor

XuHuaiyu commented Aug 18, 2017

@bailaohe

  1. ported from MySQL, you can find it from the source code of MySQL.
  2. yeah, it's a known issue, we set the length to be character length right now.
Show outdated Hide outdated expression/builtin_control.go Outdated
@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Aug 21, 2017

Contributor

rest LGTM
PTAL @zz-jason

Contributor

XuHuaiyu commented Aug 21, 2017

rest LGTM
PTAL @zz-jason

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Aug 21, 2017

Contributor

CI failed. @spongedu

Contributor

XuHuaiyu commented Aug 21, 2017

CI failed. @spongedu

Show outdated Hide outdated expression/builtin_control.go Outdated
@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Aug 21, 2017

Member

@spongedu plz resolve conflicts.

Member

zz-jason commented Aug 21, 2017

@spongedu plz resolve conflicts.

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Aug 21, 2017

Contributor

LGTM
please resolve the conflicts.

PTAL @zz-jason

Contributor

XuHuaiyu commented Aug 21, 2017

LGTM
please resolve the conflicts.

PTAL @zz-jason

@zz-jason

LGTM

@zz-jason zz-jason merged commit 2df9456 into pingcap:master Aug 21, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@spongedu spongedu deleted the spongedu:fix_builtin_ifnull branch Aug 21, 2017

dbjoa added a commit to cloud-pi/tidb that referenced this pull request Aug 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment