util: Adjust binary collate behavior to do pattern matching in bytes #51113

yibin87 · 2024-02-18T08:37:13Z

What problem does this PR solve?

Issue Number: close #50393

Problem Summary:

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

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

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Signed-off-by: yibin <huyibin@pingcap.com>

tiprow · 2024-02-18T08:37:28Z

Hi @yibin87. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: yibin <huyibin@pingcap.com>

codecov · 2024-02-18T08:55:25Z

Codecov Report

Merging #51113 (4e404b0) into master (464a126) will decrease coverage by 17.6920%.
Report is 24 commits behind head on master.
The diff coverage is 91.0256%.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #51113         +/-   ##
=================================================
- Coverage   72.3189%   54.6270%   -17.6920%     
=================================================
  Files          1467       1576        +109     
  Lines        361663     605400     +243737     
=================================================
+ Hits         261551     330712      +69161     
- Misses        80822     251623     +170801     
- Partials      19290      23065       +3775

Flag	Coverage Δ
integration	`36.8515% <83.3333%> (?)`
unit	`70.3163% <91.0256%> (-1.8625%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`53.9957% <ø> (-2.3172%)`	⬇️
parser	`∅ <ø> (∅)`
br	`50.6253% <ø> (+4.3016%)`	⬆️

yibin87 · 2024-02-18T09:56:35Z

/test check-dev2

tiprow · 2024-02-18T09:56:56Z

@yibin87: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test check-dev2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 · 2024-02-19T01:59:03Z

/test check-dev

tiprow · 2024-02-19T01:59:28Z

@yibin87: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test check-dev

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 · 2024-02-19T03:00:46Z

/cc @windtalker @xzhangxian1008

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 · 2024-02-19T05:40:00Z

/cc @xiongjiwei

ti-chi-bot · 2024-02-19T05:40:02Z

@yibin87: GitHub didn't allow me to request PR reviews from the following users: xiongjiwei.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @xiongjiwei

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

yibin87 · 2024-02-19T06:26:10Z

/cc @YangKeao

YangKeao

Rest LGTM.

BTW, I have checked that the behavior of TiKV is correct 👍 . It decodes the character according to the charset.

pkg/util/collate/bin.go

pkg/util/stringutil/string_util.go

Signed-off-by: yibin <huyibin@pingcap.com>

YangKeao

LGTM

windtalker · 2024-02-20T00:23:54Z

pkg/util/collate/collate.go

@@ -400,8 +400,8 @@ func init() {
 	newCollatorMap = make(map[string]Collator)
 	newCollatorIDMap = make(map[int]Collator)

-	newCollatorMap["binary"] = &binCollator{}
-	newCollatorIDMap[CollationName2ID("binary")] = &binCollator{}
+	newCollatorMap["binary"] = &binPureCollator{}


How about rename binPureCollator to binaryCollator since in TiDB/MySQL binary and xxx_bin is different collators, and this collator is for binary so there is no need to use a name like binPure

windtalker · 2024-02-20T00:43:16Z

pkg/util/stringutil/string_util.go

 func CompilePatternBytes(pattern string, escape byte) (patChars, patTypes []byte) {
 	patWeights, patTypes := CompilePatternInner(pattern, escape)
 	patChars = []byte(string(patWeights))

 	return patChars, patTypes
 }

-// CompilePattern is a adapter for `CompilePatternInner`, `pattern` can be any unicode string.
+// CompilePatternPureBytes is used for binary strings.
+func CompilePatternPureBytes(pattern string, escape byte) (patChars, patTypes []byte) {


How about rename
CompilePatternPureBytes to CompilePatternBinary
and
CompilePatternInnerBytes to CompilePatternInnerBinary?

windtalker · 2024-02-20T01:12:58Z

pkg/util/stringutil/string_util.go

+// DoMatchInner matches the string with patChars and patTypes.
+// The algorithm has linear time complexity.
+// https://research.swtch.com/glob
+func DoMatchInner(lenPatWeights int, lenChars int, patTypes []byte, matcher func(a, b int) bool) bool {


If DoMatchInner is not used by other package, we can make it as a private function?

windtalker · 2024-02-20T02:19:14Z

And please double check if TiFlash has the same issue

ti-chi-bot · 2024-02-20T02:33:46Z

[LGTM Timeline notifier]

Timeline:

2024-02-19 09:50:17.272062459 +0000 UTC m=+264306.019685569: ☑️ agreed by YangKeao.
2024-02-20 02:33:45.275741967 +0000 UTC m=+324514.023365077: ☑️ agreed by xiongjiwei.

tiprow · 2024-02-20T02:50:21Z

@yibin87: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
tiprow_fast_test	`44d0ef8`	link	true	`/test tiprow_fast_test`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

xzhangxian1008

LGTM

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 · 2024-02-21T03:07:35Z

And please double check if TiFlash has the same issue

The same issue detected in TiFlash, filed a issue to track it: pingcap/tiflash#8776

windtalker

LGTM

ti-chi-bot · 2024-02-21T07:59:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: windtalker, xiongjiwei, xzhangxian1008, YangKeao

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [YangKeao,windtalker,xiongjiwei]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Add new pure binary collate to do pattern matching

0819267

Signed-off-by: yibin <huyibin@pingcap.com>

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 18, 2024

update utest

f314ea9

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 added 2 commits February 18, 2024 18:13

Little refact

ace2778

Signed-off-by: yibin <huyibin@pingcap.com>

Fix build error

83afc57

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 added 2 commits February 19, 2024 10:09

Little refact

3eec5eb

Signed-off-by: yibin <huyibin@pingcap.com>

Add unit test

01d914c

Signed-off-by: yibin <huyibin@pingcap.com>

ti-chi-bot bot requested review from windtalker and xzhangxian1008 February 19, 2024 03:00

Update bazel file

7425ae9

Signed-off-by: yibin <huyibin@pingcap.com>

ti-chi-bot bot requested a review from YangKeao February 19, 2024 06:26

YangKeao reviewed Feb 19, 2024

View reviewed changes

pkg/util/collate/bin.go Outdated Show resolved Hide resolved

pkg/util/stringutil/string_util.go Outdated Show resolved Hide resolved

pkg/util/stringutil/string_util.go Show resolved Hide resolved

Address comments

44d0ef8

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 requested a review from YangKeao February 19, 2024 08:13

YangKeao approved these changes Feb 19, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Feb 19, 2024

windtalker reviewed Feb 20, 2024

View reviewed changes

xiongjiwei approved these changes Feb 20, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Feb 20, 2024

xzhangxian1008 approved these changes Feb 20, 2024

View reviewed changes

Address comments

4e404b0

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 changed the title ~~util: Add new pure binary collate to do pattern matching in bytes~~ util: Adjust binary collate behavior to do pattern matching in bytes Feb 21, 2024

yibin87 requested a review from windtalker February 21, 2024 04:33

windtalker approved these changes Feb 21, 2024

View reviewed changes

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Feb 21, 2024

ti-chi-bot bot merged commit 2041d49 into pingcap:master Feb 21, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: Adjust binary collate behavior to do pattern matching in bytes #51113

util: Adjust binary collate behavior to do pattern matching in bytes #51113

yibin87 commented Feb 18, 2024

tiprow bot commented Feb 18, 2024

codecov bot commented Feb 18, 2024 •

edited

Loading

yibin87 commented Feb 18, 2024

tiprow bot commented Feb 18, 2024

yibin87 commented Feb 19, 2024

tiprow bot commented Feb 19, 2024

yibin87 commented Feb 19, 2024

yibin87 commented Feb 19, 2024

ti-chi-bot bot commented Feb 19, 2024

yibin87 commented Feb 19, 2024

YangKeao left a comment

YangKeao left a comment

windtalker Feb 20, 2024

yibin87 Feb 21, 2024

windtalker Feb 20, 2024

yibin87 Feb 21, 2024

windtalker Feb 20, 2024

yibin87 Feb 21, 2024

windtalker commented Feb 20, 2024

ti-chi-bot bot commented Feb 20, 2024

tiprow bot commented Feb 20, 2024

xzhangxian1008 left a comment

yibin87 commented Feb 21, 2024

windtalker left a comment

ti-chi-bot bot commented Feb 21, 2024

util: Adjust binary collate behavior to do pattern matching in bytes #51113

util: Adjust binary collate behavior to do pattern matching in bytes #51113

Conversation

yibin87 commented Feb 18, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Feb 18, 2024

codecov bot commented Feb 18, 2024 • edited Loading

Codecov Report

yibin87 commented Feb 18, 2024

tiprow bot commented Feb 18, 2024

yibin87 commented Feb 19, 2024

tiprow bot commented Feb 19, 2024

yibin87 commented Feb 19, 2024

yibin87 commented Feb 19, 2024

ti-chi-bot bot commented Feb 19, 2024

yibin87 commented Feb 19, 2024

YangKeao left a comment

Choose a reason for hiding this comment

YangKeao left a comment

Choose a reason for hiding this comment

windtalker Feb 20, 2024

Choose a reason for hiding this comment

yibin87 Feb 21, 2024

Choose a reason for hiding this comment

windtalker Feb 20, 2024

Choose a reason for hiding this comment

yibin87 Feb 21, 2024

Choose a reason for hiding this comment

windtalker Feb 20, 2024

Choose a reason for hiding this comment

yibin87 Feb 21, 2024

Choose a reason for hiding this comment

windtalker commented Feb 20, 2024

ti-chi-bot bot commented Feb 20, 2024

[LGTM Timeline notifier]

tiprow bot commented Feb 20, 2024

xzhangxian1008 left a comment

Choose a reason for hiding this comment

yibin87 commented Feb 21, 2024

windtalker left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Feb 21, 2024

codecov bot commented Feb 18, 2024 •

edited

Loading