Skip to content
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

concat(ifnull(time(3)) returns different results from MySQL #29498

Closed
espresso98 opened this issue Nov 5, 2021 · 12 comments · Fixed by #30588
Closed

concat(ifnull(time(3)) returns different results from MySQL #29498

espresso98 opened this issue Nov 5, 2021 · 12 comments · Fixed by #30588
Assignees
Labels
affects-5.0 This bug affects 5.0.x versions. affects-5.1 This bug affects 5.1.x versions. affects-5.2 This bug affects 5.2.x versions. affects-5.3 This bug affects 5.3.x versions. severity/major sig/execution SIG execution type/bug The issue is confirmed as a bug.

Comments

@espresso98
Copy link
Collaborator

espresso98 commented Nov 5, 2021

Bug Report

SET TIMESTAMP doesn't take effect.

1. Minimal reproduce step (Required)

DROP TABLE IF EXISTS t1;
CREATE TABLE t1 (t3 TIME(3), d DATE);
INSERT INTO t1 VALUES ('00:00:00.567', '2002-01-01');
SELECT CONCAT(IFNULL(t3, d)) AS col1 FROM t1;

2. What did you expect to see? (Required)

+-------------------------+
| col1                    |
+-------------------------+
| 2019-03-11 00:00:00.567 |
+-------------------------+

3. What did you see instead (Required)

+----------------+
| col1           |
+----------------+
| 2021-11-04 00: |
+----------------+

4. What is your TiDB version? (Required)

+-------------------------+--------------------------------------------------------------------------+
| Variable_name           | Value                                                                    |
+-------------------------+--------------------------------------------------------------------------+
| innodb_version          | 5.6.25                                                                   |
| protocol_version        | 10                                                                       |
| tidb_analyze_version    | 2                                                                        |
| tidb_row_format_version | 2                                                                        |
| tls_version             | TLSv1,TLSv1.1,TLSv1.2                                                    |
| version                 | 5.7.25-TiDB-v5.2.2                                                       |
| version_comment         | TiDB Server (Apache License 2.0) Community Edition, MySQL 5.7 compatible |
| version_compile_machine | x86_64                                                                   |
| version_compile_os      | osx10.8                                                                  |
+-------------------------+--------------------------------------------------------------------------+
@espresso98 espresso98 added the type/bug The issue is confirmed as a bug. label Nov 5, 2021
@morgo morgo changed the title SET TIMESTAMP=UNIX_TIMESTAMP() doesn't function in TiDB concat(ifnull(time(3)) returns different results from MySQL Nov 18, 2021
@morgo
Copy link
Contributor

morgo commented Nov 18, 2021

I took a look at this. The minimal testcase is:

DROP TABLE IF EXISTS t1;
CREATE TABLE t1 (t3 TIME(3), d DATE);
INSERT INTO t1 VALUES ('00:00:00.567', '2002-01-01');
SELECT CONCAT(IFNULL(t3, d)) AS col1 FROM t1;

Because IFNULL returns t3 (since t3 is not null), which (not accounting for type conversion) this should be:

SELECT CONCAT(t3) AS col1 FROM t1;

Which (not accounting for type conversion) becomes:

SELECT t3 AS col1 FROM t1;

So the specific problem is type conversion.

TiDB gets it right if the query is just SELECT IFNULL(t3, d) AS col1 FROM t1; (it should be datetime), but gets it wrong once there are 2 conversions.

@mjonss
Copy link
Contributor

mjonss commented Dec 1, 2021

I had a related comment here.

@fuzhe1989
Copy link
Contributor

/assign @XuHuaiyu

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Dec 3, 2021

The Length should be 23 instead of 14

tidb> SELECT IFNULL(t3, d) AS col1 FROM t1;
Field   1:  `col1`
Catalog:    `def`
Database:   ``
Table:      ``
Org_table:  ``
Type:       DATETIME
Collation:  utf8mb4_bin (46)
Length:     14
Max_length: 23
Decimals:   3
Flags:


+-------------------------+
| col1                    |
+-------------------------+
| 2021-12-03 00:00:00.567 |
+-------------------------+
1 row in set (0.00 sec)

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Dec 3, 2021

We need to add a check after line 145

resultEvalType := resultFieldType.EvalType()
if resultEvalType == types.ETInt {

Like this:

	if resultEvalType == types.ETDatetime {
		// the `+1` is the length of the decimal point
		resultFieldType.Flen = mysql.MaxDatetimeWidthNoFsp + mathutil.Max(lhs.Decimal, rhs.Decimal) + 1
	}

@bestwoody
Copy link
Contributor

bestwoody commented Dec 9, 2021

As @XuHuaiyu said, “The Length should be 23 instead of 14”. Operating Date op with Time op will result in Datetime result. However, TiDB uses max(Time.length, Date.length) not Datetime.legnth to derive length info which led truncated result. PR

@bestwoody
Copy link
Contributor

PR: #30588

@XuHuaiyu
Copy link
Contributor

/label affects-5.0

@ti-chi-bot ti-chi-bot added the affects-5.0 This bug affects 5.0.x versions. label Dec 16, 2021
@bestwoody
Copy link
Contributor

/label affects-5.1
/label affects-5.2
/label affects-5.3

@ti-chi-bot ti-chi-bot added affects-5.1 This bug affects 5.1.x versions. affects-5.2 This bug affects 5.2.x versions. affects-5.3 This bug affects 5.3.x versions. labels Dec 16, 2021
@yibin87
Copy link
Contributor

yibin87 commented Dec 16, 2021

Reproduced it on local dev machine.

@bestwoody
Copy link
Contributor

bestwoody commented Dec 17, 2021

this PR will also solve cases of "case when" and "union". However, since every func has its own flen compute logic, other funcs may has similar problems too. But some func is difficult to change since it may change the behavior, such as: issue#26485

@github-actions
Copy link

Please check whether the issue should be labeled with 'affects-x.y' or 'fixes-x.y.z', and then remove 'needs-more-info' label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-5.0 This bug affects 5.0.x versions. affects-5.1 This bug affects 5.1.x versions. affects-5.2 This bug affects 5.2.x versions. affects-5.3 This bug affects 5.3.x versions. severity/major sig/execution SIG execution type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants