expression/builtin_time: fix wrong precision for maketime #28068

w169q169 · 2021-09-15T13:25:33Z

What problem does this PR solve?

Issue Number: close #27771

Problem Summary:

What is changed and how it works?

What's Changed:

How it Works:

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2021-09-15T13:25:34Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

w169q169 · 2021-09-16T00:50:27Z

/cc @wjhuang2016

w169q169 · 2021-09-16T01:12:36Z

/run-check_dev_2

expression/builtin_time.go

wjhuang2016

_

wjhuang2016

you can use setDecimalAndFlenForTime after #28252 gets merged.

wjhuang2016 · 2021-09-22T09:08:48Z

expression/builtin_time.go

-		decimal = args[2].GetType().Decimal
-		if decimal > 6 || decimal == types.UnspecifiedLength {
-			decimal = 6
+	case types.ETString:


bool resolve_type(THD *) override { set_data_type_time(MY_MIN(args[2]->decimals, DATETIME_MAX_DECIMALS)); return false; }

I think this can work, we don't need getExpressionFsp().

But be careful with UnspecifiedLength, which is 31 in MySQL but -1 in TiDB

thanks for your reply, I will continue after your works finished

If the type is ETSTRING, then the decimal should be -1?

Can we set the decimal be UnspecifiedLength, if the type is ETSTRING ?

sre-bot · 2021-09-27T01:59:29Z

Please follow PR Title Format:

pkg [, pkg2, pkg3]: what is changed

Or if the count of mainly changed packages are more than 3, use

*: what is changed

After you have format title, you can leave a comment /run-check_title to recheck it

wjhuang2016 · 2021-09-28T04:03:31Z

expression/builtin_time.go

 	switch tp {
+	case types.ETString:
+		decimal = 6
 	case types.ETInt:


If the type is ETINT, then the args[2].GetType().Decimal should be 0.

So the case ETint can be merged to default.

wjhuang2016 · 2021-09-28T04:04:26Z

expression/builtin_time.go

-		decimal = args[2].GetType().Decimal
-		if decimal > 6 || decimal == types.UnspecifiedLength {
-			decimal = 6
+	case types.ETString:


If the type is ETSTRING, then the decimal should be -1?

wjhuang2016 · 2021-09-28T04:04:47Z

expression/builtin_time.go

 	default:
+		decimal = args[2].GetType().Decimal


Can we use this to handle all the cases?

I add mores unit tests in expression/typeinfer_test.go to cover all the cases . It seems to be good.

For ETString, the decimal shoule be 31? Then it will be truncated to 6,

I think set UnspecifiedLength for ETSTRING is more clear, because decimal is 31 in MySQL but -1 in TiDB.Decimal use 31 in ETSTRING is not same as common rules in TIDB.

Anyway, it's covered by L5836. We can set decimal = args[2].GetType().Decimal for ETString.

Set decimal = args[2].GetType().Decimal for ETString can't cover the situation that the arg is varchar type({maketime(c_int_d, c_int_d, c_char) test in expression/typeinfer_test.go).
In that case, the decimal is zero not the UnspecifiedLength, and we will get zero decimal that is wrong result(expect decimal is 6).
So we have to specialize UnspecifiedLength in decimal for type ETSTRING.

For literal "string", the decimal is 31.

mysql> select "asd"; Field 1: `asd` Catalog: `def` Database: `` Table: `` Org_table: `` Type: VAR_STRING Collation: utf8mb4_bin (46) Length: 12 Max_length: 3 Decimals: 31 Flags: NOT_NULL +-----+ | asd | +-----+ | asd | +-----+ 1 row in set (0.01 sec)

Could you find out why it's 0 for the string column?

@wjhuang2016

MySQL [test]> select 'asd'; Field 1: `asd` Catalog: `def` Database: `` Table: `` Org_table: `` Type: VAR_STRING Collation: utf8mb3_general_ci (33) Length: 9 Max_length: 3 Decimals: 31 Flags: NOT_NULL +-----+ | asd | +-----+ | asd | +-----+ 1 row in set (0.001 sec)

MySQL [test]> select a from t_test; Field 1: `a` Catalog: `def` Database: `test` Table: `t_test` Org_table: `t_test` Type: VAR_STRING Collation: utf8mb3_general_ci (33) Length: 75 Max_length: 0 Decimals: 0 Flags: 0 rows in set (0.002 sec)

type decimal

mysql_string(literal) 31

mysql_varstring 0

mysql and tidb look same from the client side, but it has different implement for decimals in types.

for mysql
mysql change 31(DECIMAL_NOT_SPECIFIED) to 0 while sending column meta info (only for varstring, not for literal string).

https://github.com/mysql/mysql-server/blob/3290a66c89eb1625a7058e0ef732432b6952b435/sql/field.cc#L2061-L2063

type decimal(inner) decimal(column-meta)

mysql_string(literal) 31(DECIMAL_NOT_SPECIFIED) 31

mysql_varstring 31(DECIMAL_NOT_SPECIFIED) 0

for tidb
tidb change -1(types.UnspecifiedLength) to 0 while sending column meta info(only for literal string, not for var string)

tidb/server/driver_tidb.go

Lines 409 to 413 in 5b5020d

if fld.Column.Decimal == types.UnspecifiedLength {

if fld.Column.Tp == mysql.TypeDuration {

ci.Decimal = uint8(types.DefaultFsp)

} else {

ci.Decimal = mysql.NotFixedDec

type decimal(inner) decimal(column-meta)

mysql_string(literal) -1(UnspecifiedLength) 31(NotFixedDec)

mysql_varstring 0 0

in the end
according to the above reasons, use min to check decimal is right for mysql but will get wrong result in tidb.

https://github.com/mysql/mysql-server/blob/3290a66c89eb1625a7058e0ef732432b6952b435/sql/item_timefunc.h#L1557-L1563

ti-chi-bot requested a review from wjhuang2016 September 16, 2021 00:50

wjhuang2016 reviewed Sep 17, 2021

View reviewed changes

expression/builtin_time.go Outdated Show resolved Hide resolved

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2021

wjhuang2016 reviewed Sep 22, 2021

View reviewed changes

w169q169 force-pushed the issue-27771 branch from fe87238 to cf5b468 Compare September 22, 2021 06:21

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 22, 2021

wjhuang2016 reviewed Sep 22, 2021

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2021

w169q169 force-pushed the issue-27771 branch from f3b6c57 to 6146a8c Compare September 27, 2021 01:59

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2021

ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 27, 2021

w169q169 requested a review from wjhuang2016 September 27, 2021 02:27

wjhuang2016 reviewed Sep 28, 2021

View reviewed changes

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 28, 2021

expression/builtin_time: fix wrong precision for maketime

b953255

w169q169 added 9 commits September 29, 2021 09:43

fix tests

ea3b2b0

fix tests

7152e9d

fix for review

c106482

fix for unit test

4b53382

fix for review

7530534

expression/buildin_time: fix unit test

1eda5d7

expression/buildin_time: fix unit test

30f060d

expression/buildin_time: fix for review

b8c065a

expression/buildin_time: fix for review

4773a87

w169q169 force-pushed the issue-27771 branch from 2ef4f04 to 4773a87 Compare September 29, 2021 01:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression/builtin_time: fix wrong precision for maketime #28068

expression/builtin_time: fix wrong precision for maketime #28068

w169q169 commented Sep 15, 2021 •

edited

Loading

ti-chi-bot commented Sep 15, 2021

w169q169 commented Sep 16, 2021

w169q169 commented Sep 16, 2021

wjhuang2016 left a comment •

edited

Loading

wjhuang2016 left a comment

wjhuang2016 Sep 22, 2021

wjhuang2016 Sep 22, 2021

w169q169 Sep 23, 2021 •

edited

Loading

wjhuang2016 Sep 28, 2021

w169q169 Sep 28, 2021

sre-bot commented Sep 27, 2021

wjhuang2016 Sep 28, 2021

wjhuang2016 Oct 28, 2021

wjhuang2016 Sep 28, 2021

wjhuang2016 Sep 28, 2021

w169q169 Sep 28, 2021 •

edited

Loading

wjhuang2016 Sep 29, 2021

w169q169 Sep 29, 2021

wjhuang2016 Sep 29, 2021

w169q169 Sep 30, 2021 •

edited

Loading

wjhuang2016 Sep 30, 2021

w169q169 Oct 27, 2021

type	decimal(inner)	decimal(column-meta)
mysql_string(literal)	31(DECIMAL_NOT_SPECIFIED)	31
mysql_varstring	31(DECIMAL_NOT_SPECIFIED)	0

	if fld.Column.Decimal == types.UnspecifiedLength {
	if fld.Column.Tp == mysql.TypeDuration {
	ci.Decimal = uint8(types.DefaultFsp)
	} else {
	ci.Decimal = mysql.NotFixedDec

type	decimal(inner)	decimal(column-meta)
mysql_string(literal)	-1(UnspecifiedLength)	31(NotFixedDec)
mysql_varstring	0	0

expression/builtin_time: fix wrong precision for maketime #28068

Are you sure you want to change the base?

expression/builtin_time: fix wrong precision for maketime #28068

Conversation

w169q169 commented Sep 15, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 15, 2021

w169q169 commented Sep 16, 2021

w169q169 commented Sep 16, 2021

wjhuang2016 left a comment • edited Loading

Choose a reason for hiding this comment

wjhuang2016 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w169q169 Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sre-bot commented Sep 27, 2021

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

w169q169 Sep 28, 2021 • 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

w169q169 Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w169q169 commented Sep 15, 2021 •

edited

Loading

wjhuang2016 left a comment •

edited

Loading

w169q169 Sep 23, 2021 •

edited

Loading

w169q169 Sep 28, 2021 •

edited

Loading

w169q169 Sep 30, 2021 •

edited

Loading