-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
expression: add pushdown flags of regexp functions to tikv #37893
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
/cc @windtalker |
expression/expression.go
Outdated
@@ -979,7 +979,7 @@ func scalarExprSupportedByTiKV(sf *ScalarFunction) bool { | |||
ast.Length, ast.BitLength, ast.Concat, ast.ConcatWS, ast.Replace, ast.ASCII, ast.Hex, | |||
ast.Reverse, ast.LTrim, ast.RTrim, ast.Strcmp, ast.Space, ast.Elt, ast.Field, | |||
InternalFuncFromBinary, InternalFuncToBinary, ast.Mid, ast.Substring, ast.Substr, ast.CharLength, | |||
ast.Right, /* ast.Left */ | |||
ast.Right /* ast.Left */, ast.RegexpLike, ast.RegexpSubstr, ast.RegexpInStr, ast.RegexpReplace, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Regexp should also be pushed to TiKV
- As discussed offline, for
Regexp/RegexpLike
, do not push down to TiKV if the collation is binary - Should add some tests
related tikv pr: tikv/tikv#13480 |
@@ -1026,6 +1027,12 @@ func scalarExprSupportedByTiKV(sf *ScalarFunction) bool { | |||
case tipb.ScalarFuncSig_RandWithSeedFirstGen: | |||
return true | |||
} | |||
case ast.Regexp, ast.RegexpLike: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be all Regexp
functions?
some unit tests and mysql tests failed because of new supported function, please fix them you may need to raise a pull request in https://github.com/pingcap/tidb-test for the broken mysql-test
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix the broken tests.
rest LGTM
@@ -7598,3 +7598,45 @@ func TestCastJSONTimeDuration(t *testing.T) { | |||
fmt.Sprintf("6 %s %s 12:12:12 12:12:12 TIME", nowDate, nowDate), | |||
)) | |||
} | |||
|
|||
func TestRegexpPushdown(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you have extra time, you can refactor this test to make the test case separate from test code, like other tests. Which will make it easy to regenerate tests result.
expression/expression.go
Outdated
@@ -1026,6 +1027,12 @@ func scalarExprSupportedByTiKV(sf *ScalarFunction) bool { | |||
case tipb.ScalarFuncSig_RandWithSeedFirstGen: | |||
return true | |||
} | |||
case ast.Regexp, ast.RegexpLike: | |||
_, collation := sf.Function.CharsetAndCollation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not check both charset and collation like func (re *regexpBaseFuncSig) isBinaryCollation()
?
" └─TableFullScan_5 10000.00 cop[tikv] table:reg keep order:false, stats:pseudo")) | ||
|
||
tk.MustExec("drop table if exists regbin") | ||
tk.MustExec("create table regbin(a varchar(20) null,b varchar(20) null,rep varchar(20) null) charset=binary collate=binary;") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created a table with binary charset, but no tests?
/run-mysql-test tidb-test=pr/1974 |
/run-mysql-test tidb-test=pr/1974 |
/run-unit-test |
/run-mysql-test tidb-test=pr/1974 |
/run-unit-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/merge |
This pull request has been accepted and is ready to merge. Commit hash: cbff7b4
|
/run-mysql-test tidb-test=pr/1974 |
3 similar comments
/run-mysql-test tidb-test=pr/1974 |
/run-mysql-test tidb-test=pr/1974 |
/run-mysql-test tidb-test=pr/1974 |
/run-mysql-test tidb-test=pr/1974 |
/run-mysql-test tidb-test=pr/1974 |
TiDB MergeCI notify🔴 Bad News! New failing [1] after this pr merged.
|
What problem does this PR solve?
Issue Number: close #37894
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.