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

Compare DATE column with INT value result in NULL and wrong result #17868

Closed
zhangysh1995 opened this issue Jun 9, 2020 · 7 comments · Fixed by #20961
Closed

Compare DATE column with INT value result in NULL and wrong result #17868

zhangysh1995 opened this issue Jun 9, 2020 · 7 comments · Fixed by #20961
Assignees
Labels
challenge-program help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. priority/P3 The issue has P3 priority. Assigned to backlog. severity/major sig/execution SIG execution type/bug The issue is confirmed as a bug.

Comments

@zhangysh1995
Copy link

zhangysh1995 commented Jun 9, 2020

Description

Bug Report

1. Minimal reproduce step (Required)

drop table if exists t7;
create table t7 (col0 SMALLINT, col1 VARBINARY(1), col2 DATE, col3 BIGINT, col4 BINARY(166));
insert into t7 values ('32767', '', '1000-01-03', '-0', '11101011');
create index wscjr on t7 (col2, col3);
select col2 = 1 from t7;
select col2 != 1 from t7;

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

-- MySQL 5.7
mysql> select col2 = 1 from t7;
+----------+
| col2 = 1 |
+----------+
|        0 |
+----------+
1 row in set, 1 warning (0.00 sec)

mysql> select col2 != 1 from t7;
+-----------+
| col2 != 1 |
+-----------+
|         1 |
+-----------+
1 row in set, 1 warning (0.00 sec)

3. What did you see instead (Required)

mysql>  select col2 = 1 from t7;                                                                                                                                                                                                                      +----------+                                                                                                                                                                                                                                          
| col2 = 1 |
+----------+
|     NULL |
+----------+
1 row in set, 1 warning (0.00 sec)

mysql>  select col2 != 1 from t7;
+-----------+
| col2 != 1 |
+-----------+
|      NULL |
+-----------+
1 row in set, 1 warning (0.00 sec)

4. Affected version (Required)

commit 8369ffd500f3fb235d8b584ac4298b2e59d8db55 (HEAD -> master, origin/master, origin/HEAD)
Author: Soup <ilovesoup@gmail.com>
Date:   Tue May 26 15:38:29 2020 +0800

Reproducible on master branch:

commit c9c9f873e92c8c258a52d2a125ca6eb6c781d11d (HEAD -> master, origin/master, origin/HEAD)
Author: Chengpeng Yan <41809508+Reminiscent@users.noreply.github.com>
Date:   Tue Jun 9 14:56:38 2020 +0800

5. Root Cause Analysis

SIG slack channel

#sig-exec

Score

  • 300

Mentor

@zhangysh1995 zhangysh1995 added the type/bug The issue is confirmed as a bug. label Jun 9, 2020
@djshow832 djshow832 added the sig/execution SIG execution label Jun 9, 2020
@qw4990 qw4990 self-assigned this Jun 11, 2020
@qw4990 qw4990 added severity/critical help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Aug 26, 2020
@fzhedu fzhedu added the priority/P1 The issue has P1 priority. label Aug 27, 2020
@qw4990 qw4990 added priority/P3 The issue has P3 priority. Assigned to backlog. and removed priority/P1 The issue has P1 priority. labels Aug 27, 2020
@LENSHOOD
Copy link
Contributor

/pick-up

@ti-challenge-bot
Copy link

Pick up success.

@LENSHOOD
Copy link
Contributor

The root cause of this issue is about two parts:

1. ScalarFunction.Eval(...) method takes all EvalXXX() result as "null" when there's some sort of err occurred.

// at scalar_function.go: line 326
func (sf *ScalarFunction) Eval(row chunk.Row) (d types.Datum, err error) {
	...
	switch tp, evalType := sf.GetType(), sf.GetType().EvalType(); evalType {
	...
	case types.ETDatetime, types.ETTimestamp:
		res, isNull, err = sf.EvalTime(sf.GetCtx(), row)
	...
	}

        // as long as err != nil, the returned Datum will be set to null
	if isNull || err != nil {
		d.SetNull()
		return d, err
	}
	d.SetValue(res, sf.RetType)
	return
}

In the expression select col2 = 1 from t7, constant 1 is not a valid datetime value, so cast 1 to datetime will cause "err != nil", that lead Eval() return a null Datum as cast result of 1.

Therefore, col2 = 1 become col2 = NULL, and that's explain why select col2 = 1 from t7; get NULL returned.

At Eval(), it is irresponsible to simply set null when err != nil, the more properly way is leave the decision to EvalXXX(), and call d.SetNull() only if isNull == true.

2. All evalTime() implementations return isNull as true when any error occurred.

Even if we modify Eval() to set d.SetNull() only when isNull == true, we still cannot solve the issue.

Because all evalTime() implementations return isNull as true when any error occurred, for example:

// builtin_cast.go: line 571
func (b *builtinCastIntAsTimeSig) evalTime(row chunk.Row) (res types.Time, isNull bool, err error) {
	... 
	res, err = types.ParseTimeFromNum(b.ctx.GetSessionVars().StmtCtx, val, b.tp.Tp, int8(b.tp.Decimal))
	if err != nil {
                 // isNull returned as true, but res is types.ZeroTime
		return types.ZeroTime, true, handleInvalidTimeError(b.ctx, err)
	}
	...
}

So cast 1 to datetime will always return null Datum.
There are 68 implementations of evalTime(), all share the same logic to deal with err != nil.


So far I cannot find a good solution. Any suggestions? @qw4990 @zhangysh1995

@zhangysh1995
Copy link
Author

zhangysh1995 commented Sep 15, 2020

@LENSHOOD I found a related description in MySQL 8 documentation:

As a convenience, MySQL automatically converts a date to a number if the date is used in numeric context and vice versa

It seems it should do a DATE->INT conversion, which will call this function:

func (b *builtinCastTimeAsIntSig) evalInt(row chunk.Row) (res int64, isNull bool, err error)

Which will further call EvalTime on a column:

func (col *Column) EvalTime(ctx sessionctx.Context, row chunk.Row) (types.Time, bool, error) 

At last the value is converted to a number:

func (b *builtinCastTimeAsIntSig) evalInt(row chunk.Row) (res int64, isNull bool, err error) {
       // extract the value
	val, isNull, err := b.args[0].EvalTime(b.ctx, row)
	if isNull || err != nil {
		return res, isNull, err
	}
       // not sure what is done here
	sc := b.ctx.GetSessionVars().StmtCtx
	t, err := val.RoundFrac(sc, types.DefaultFsp)
	if err != nil {
		return res, false, err
	}
       // convert to a numer
	res, err = t.ToNumber().ToInt()
	return res, false, err
}

As long as DATE value is valid, it will not return isNull = true to the caller function, then it should solve at least the comparison between a date and an integer.

@ti-challenge-bot
Copy link

@LENSHOOD You did not submit PR within 7 days, so give up automatically.

@ti-challenge-bot ti-challenge-bot bot removed the picked label Sep 19, 2020
@LENSHOOD
Copy link
Contributor

@LENSHOOD I found a related description in MySQL 8 documentation:

As a convenience, MySQL automatically converts a date to a number if the date is used in numeric context and vice versa

It seems it should do a DATE->INT conversion, which will call this function:

func (b *builtinCastTimeAsIntSig) evalInt(row chunk.Row) (res int64, isNull bool, err error)

Which will further call EvalTime on a column:

func (col *Column) EvalTime(ctx sessionctx.Context, row chunk.Row) (types.Time, bool, error) 

At last the value is converted to a number:

func (b *builtinCastTimeAsIntSig) evalInt(row chunk.Row) (res int64, isNull bool, err error) {
       // extract the value
	val, isNull, err := b.args[0].EvalTime(b.ctx, row)
	if isNull || err != nil {
		return res, isNull, err
	}
       // not sure what is done here
	sc := b.ctx.GetSessionVars().StmtCtx
	t, err := val.RoundFrac(sc, types.DefaultFsp)
	if err != nil {
		return res, false, err
	}
       // convert to a numer
	res, err = t.ToNumber().ToInt()
	return res, false, err
}

As long as DATE value is valid, it will not return isNull = true to the caller function, then it should solve at least the comparison between a date and an integer.

In TiDB, the comparison logic is transfer constant (at this case num "1") to the column type ("date"), so when planner generate logic plan from AST, it will choose related transfer function by comparison pair, we can see this logic at 'foldConstant()' function.

Therefore, planner will transfer int to date rather than date to int, but the num 1 is not a valid date num...

@ti-srebot
Copy link
Contributor

Please edit this comment or add a new comment to complete the following information

Not a bug

  1. Remove the 'type/bug' label
  2. Add notes to indicate why it is not a bug

Duplicate bug

  1. Add the 'type/duplicate' label
  2. Add the link to the original bug

Bug

Note: Make Sure that 'component', and 'severity' labels are added
Example for how to fill out the template: #20100

1. Root Cause Analysis (RCA) (optional)

2. Symptom (optional)

3. All Trigger Conditions (optional)

4. Workaround (optional)

5. Affected versions

6. Fixed versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
challenge-program help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. priority/P3 The issue has P3 priority. Assigned to backlog. 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.

7 participants