planner: reject invalid conversion from like to = #11320

wjhuang2016 · 2019-07-18T11:05:14Z

What problem does this PR solve?

What is changed and how it works?

Reject invalid conversion from like to =
0 like "a" should not convert to 0 = "a",
if so, it will further become 0 = 0 and return true
After this pr, t.col like "a" can't use indexscan when col isn't a string.
If the col isn't a string, the sql maybe wrong and is ok not to use indexsacn.

Check List

Tests

Integration test

Code changes

Side effects

Related changes

codecov · 2019-07-18T11:10:55Z

Codecov Report

Merging #11320 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11320   +/-   ##
===========================================
  Coverage   81.3711%   81.3711%           
===========================================
  Files           424        424           
  Lines         90730      90730           
===========================================
  Hits          73828      73828           
  Misses        11589      11589           
  Partials       5313       5313

XuHuaiyu · 2019-07-19T02:53:04Z

planner/core/expression_rewriter.go

@@ -1237,7 +1237,7 @@ func (er *expressionRewriter) patternLikeToExpression(v *ast.PatternLikeExpr) {
 		}
 		if !isNull {
 			patValue, patTypes := stringutil.CompilePattern(patString, v.Escape)
-			if stringutil.IsExactMatch(patTypes) {
+			if stringutil.IsExactMatch(patTypes) && er.ctxStack[l-2].GetType().EvalType().IsStringKind() {


Is it seems that we should not call IsStringKind() but check .EvalType() == ETString here?

Is it seems that we should not call IsStringKind() but check .EvalType() == ETString here?

It seems that IsStringKind() is enough, If the val's type is String, they will be compared as string.

tidb/expression/builtin_compare.go

Lines 958 to 978 in 5aef053

func getBaseCmpType(lhs, rhs types.EvalType, lft, rft *types.FieldType) types.EvalType {

if lft.Tp == mysql.TypeUnspecified || rft.Tp == mysql.TypeUnspecified {

if lft.Tp == rft.Tp {

return types.ETString

}

if lft.Tp == mysql.TypeUnspecified {

lhs = rhs

} else {

rhs = lhs

}

}

if lhs.IsStringKind() && rhs.IsStringKind() {

return types.ETString

} else if (lhs == types.ETInt || lft.Hybrid()) && (rhs == types.ETInt || rft.Hybrid()) {

return types.ETInt

} else if ((lhs == types.ETInt || lft.Hybrid()) || lhs == types.ETDecimal) &&

((rhs == types.ETInt || rft.Hybrid()) || rhs == types.ETDecimal) {

return types.ETDecimal

}

return types.ETReal

}

IsStringKind contains ETDatetime, etc.

func (et EvalType) IsStringKind() bool { return et == ETString || et == ETDatetime || et == ETTimestamp || et == ETDuration || et == ETJson }

If the val's type is String, they will be compared as string

Noop, getBaseCmpType gets a basic comparison type, the final comparison type may change later.

Though not related to this patch, looks like I found another compatibility issue:
In MySQL:

mysql> create table tbl(id int primary key, a date); Query OK, 0 rows affected (0.03 sec) mysql> set sql_mode = ''; Query OK, 0 rows affected, 1 warning (0.00 sec) mysql> insert tbl values(0, '0000-00-00'); Query OK, 1 row affected (0.02 sec) mysql> select a like 'a' from tbl; +------------+ | a like 'a' | +------------+ | 0 | +------------+ 1 row in set, 1 warning (0.00 sec) mysql> show warnings; +---------+------+---------------------------------------------------+ | Level | Code | Message | +---------+------+---------------------------------------------------+ | Warning | 1292 | Incorrect date value: 'a' for column 'a' at row 1 | +---------+------+---------------------------------------------------+

while in TiDB (even with this patch):

mysql> select a like 'a' from tbl; +------------+ | a like 'a' | +------------+ | NULL | +------------+ 1 row in set, 1 warning (0.00 sec) mysql> show warnings; +---------+------+-------------------------------+ | Level | Code | Message | +---------+------+-------------------------------+ | Warning | 1292 | Incorrect datetime value: 'a' | +---------+------+-------------------------------+

@wjhuang2016 are you interested to fix this as well?

IsStringKind contains ETDatetime, etc.

func (et EvalType) IsStringKind() bool { return et == ETString || et == ETDatetime || et == ETTimestamp || et == ETDuration || et == ETJson }

If the val's type is String, they will be compared as string

Noop, getBaseCmpType gets a basic comparison type, the final comparison type may change later.

I got it. If the lhs is a datatime, the rhs may be converted to a datatime.

foreyes

LGTM

eurekaka · 2019-07-23T08:47:24Z

planner/core/expression_rewriter.go

@@ -1237,7 +1237,7 @@ func (er *expressionRewriter) patternLikeToExpression(v *ast.PatternLikeExpr) {
 		}
 		if !isNull {
 			patValue, patTypes := stringutil.CompilePattern(patString, v.Escape)
-			if stringutil.IsExactMatch(patTypes) {
+			if stringutil.IsExactMatch(patTypes) && er.ctxStack[l-2].GetType().EvalType().IsStringKind() {


Though not related to this patch, looks like I found another compatibility issue:
In MySQL:

mysql> create table tbl(id int primary key, a date); Query OK, 0 rows affected (0.03 sec) mysql> set sql_mode = ''; Query OK, 0 rows affected, 1 warning (0.00 sec) mysql> insert tbl values(0, '0000-00-00'); Query OK, 1 row affected (0.02 sec) mysql> select a like 'a' from tbl; +------------+ | a like 'a' | +------------+ | 0 | +------------+ 1 row in set, 1 warning (0.00 sec) mysql> show warnings; +---------+------+---------------------------------------------------+ | Level | Code | Message | +---------+------+---------------------------------------------------+ | Warning | 1292 | Incorrect date value: 'a' for column 'a' at row 1 | +---------+------+---------------------------------------------------+

while in TiDB (even with this patch):

mysql> select a like 'a' from tbl; +------------+ | a like 'a' | +------------+ | NULL | +------------+ 1 row in set, 1 warning (0.00 sec) mysql> show warnings; +---------+------+-------------------------------+ | Level | Code | Message | +---------+------+-------------------------------+ | Warning | 1292 | Incorrect datetime value: 'a' | +---------+------+-------------------------------+

@wjhuang2016 are you interested to fix this as well?

eurekaka · 2019-07-23T08:49:47Z

planner/core/physical_plan_test.go

@@ -1170,11 +1170,10 @@ func (s *testPlanSuite) TestRefine(c *C) {
 			sql:  `select a from t where c_str like 123`,
 			best: "IndexReader(Index(t.c_d_e_str)[[\"123\",\"123\"]])->Projection",
 		},
-		// c is type int which will be added cast to specified type when building function signature,
-		// and rewrite predicate like to predicate '='  when exact match , index still can be used.
+		// c is type int which will be added cast to specified type when building function signature, no index can be used.


We can simply remove this test now.

@eurekaka Yeah, I will fix it.

wjhuang2016 · 2019-07-23T10:43:42Z

@eurekaka, after I changed the IsStringKind() to == ETSTRING as @XuHuaiyu suggested,
select a like 'a' from tbl; return 0 and without warnings;
Besides, insert tbl values(0, '0000-00-00'); will exec suceessfully without set sql_mode = ''; in TiDB

eurekaka · 2019-07-24T02:45:56Z

@wjhuang2016 OK, can you file GitHub issues for those 2 compatibility problems found? Then for this PR, we can focus on the correction of the query result.

eurekaka

LGTM

sre-bot · 2019-07-24T06:15:07Z

cherry pick to release-2.1 failed

sre-bot · 2019-07-24T06:18:58Z

cherry pick to release-3.0 in PR #11411

sre-bot · 2020-04-07T10:22:12Z

It seems that, not for sure, we failed to cherry-pick this commit to release-2.1. Please comment '/run-cherry-picker' to try to trigger the cherry-picker if we did fail to cherry-pick this commit before. @wjhuang2016 PTAL.

planner: reject invalid conversion from like to =

c3d195c

zz-jason added sig/planner SIG: Planner contribution This PR is from a community contributor. labels Jul 18, 2019

zz-jason requested a review from eurekaka July 18, 2019 11:34

XuHuaiyu reviewed Jul 19, 2019

View reviewed changes

XuHuaiyu added type/bugfix This PR fixes a bug. needs-cherry-pick-2.1 labels Jul 19, 2019

eurekaka requested a review from foreyes July 23, 2019 08:30

foreyes reviewed Jul 23, 2019

View reviewed changes

foreyes added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 23, 2019

eurekaka reviewed Jul 23, 2019

View reviewed changes

planner: reject invalid conversion from like to =

ce119fe

wjhuang2016 force-pushed the fix_like branch from 34f8670 to ce119fe Compare July 23, 2019 09:37

eurekaka approved these changes Jul 24, 2019

View reviewed changes

Merge branch 'master' into fix_like

a7692a1

eurekaka added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 24, 2019

Merge branch 'master' into fix_like

14557f1

zz-jason merged commit 9b52134 into pingcap:master Jul 24, 2019

sre-bot mentioned this pull request Jul 24, 2019

planner: reject invalid conversion from like to = (#11320) #11411

Merged

zz-jason pushed a commit that referenced this pull request Jul 24, 2019

planner: reject invalid conversion from like to = (#11320) (#11411)

68acdce

wjhuang2016 deleted the fix_like branch November 17, 2022 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: reject invalid conversion from like to = #11320

planner: reject invalid conversion from like to = #11320

wjhuang2016 commented Jul 18, 2019

codecov bot commented Jul 18, 2019 •

edited

Loading

XuHuaiyu Jul 19, 2019

wjhuang2016 Jul 19, 2019

XuHuaiyu Jul 23, 2019

eurekaka Jul 23, 2019

wjhuang2016 Jul 23, 2019

foreyes left a comment

eurekaka Jul 23, 2019

eurekaka Jul 23, 2019

wjhuang2016 Jul 23, 2019

wjhuang2016 commented Jul 23, 2019

eurekaka commented Jul 24, 2019

eurekaka left a comment

sre-bot commented Jul 24, 2019

sre-bot commented Jul 24, 2019

sre-bot commented Apr 7, 2020

	func getBaseCmpType(lhs, rhs types.EvalType, lft, rft *types.FieldType) types.EvalType {
	if lft.Tp == mysql.TypeUnspecified \|\| rft.Tp == mysql.TypeUnspecified {
	if lft.Tp == rft.Tp {
	return types.ETString
	}
	if lft.Tp == mysql.TypeUnspecified {
	lhs = rhs
	} else {
	rhs = lhs
	}
	}
	if lhs.IsStringKind() && rhs.IsStringKind() {
	return types.ETString
	} else if (lhs == types.ETInt \|\| lft.Hybrid()) && (rhs == types.ETInt \|\| rft.Hybrid()) {
	return types.ETInt
	} else if ((lhs == types.ETInt \|\| lft.Hybrid()) \|\| lhs == types.ETDecimal) &&
	((rhs == types.ETInt \|\| rft.Hybrid()) \|\| rhs == types.ETDecimal) {
	return types.ETDecimal
	}
	return types.ETReal
	}

planner: reject invalid conversion from like to = #11320

planner: reject invalid conversion from like to = #11320

Conversation

wjhuang2016 commented Jul 18, 2019

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented Jul 18, 2019 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

foreyes left a comment

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

wjhuang2016 commented Jul 23, 2019

eurekaka commented Jul 24, 2019

eurekaka left a comment

Choose a reason for hiding this comment

sre-bot commented Jul 24, 2019

sre-bot commented Jul 24, 2019

sre-bot commented Apr 7, 2020

codecov bot commented Jul 18, 2019 •

edited

Loading