expression: fix tikv crash when `bool like cast(bit as char)` #57484

lcwangchao · 2024-11-19T03:21:53Z

What problem does this PR solve?

Issue Number: close #56494

What changed and how does it work?

When casting the bit type to a non binary string, this PR first convert bits to binary string first. This can ensure HandleBinaryLiteral can be used in the following code and convert binary to the specified collation with right behavior.

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.

fix tikv crash when `bool like cast(bit as char)`

tiprow · 2024-11-19T03:22:08Z

Hi @lcwangchao. 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-sigs/prow repository.

codecov · 2024-11-19T03:55:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.3407%. Comparing base (a22b842) to head (561c9d1).
Report is 516 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57484        +/-   ##
================================================
+ Coverage   72.8488%   73.3407%   +0.4918%     
================================================
  Files          1672       1676         +4     
  Lines        462666     470868      +8202     
================================================
+ Hits         337047     345338      +8291     
+ Misses       104821     104711       -110     
- Partials      20798      20819        +21

Flag	Coverage Δ
integration	`43.5485% <100.0000%> (?)`
unit	`72.4417% <100.0000%> (+0.2200%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.7673% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`43.3494% <ø> (-1.7732%)`	⬇️

lcwangchao · 2024-11-19T05:17:23Z

/retest

tiprow · 2024-11-19T05:17:44Z

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

In response to this:

/retest

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-sigs/prow repository.

lcwangchao · 2024-11-19T06:05:55Z

/retest

tiprow · 2024-11-19T06:06:17Z

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

In response to this:

/retest

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-sigs/prow repository.

pkg/expression/builtin_convert_charset.go

qw4990

LGTM for the optimizer part.

ti-chi-bot · 2024-11-20T02:11:55Z

[LGTM Timeline notifier]

Timeline:

2024-11-20 02:02:59.991861831 +0000 UTC m=+1012942.182730827: ☑️ agreed by Defined2014.
2024-11-20 02:11:54.169677522 +0000 UTC m=+1013476.360546519: ☑️ agreed by qw4990.

windtalker · 2024-11-20T02:10:12Z

pkg/expression/builtin_cast.go

-		sig.setPbCode(tipb.ScalarFuncSig_CastStringAsString)
-		return sig, nil
+	if ft := args[0].GetType(ctx.GetEvalCtx()); ft.Hybrid() {
+		castBitAsUnBinary := ft.GetType() == mysql.TypeBit && c.tp.GetCharset() != charset.CharsetBin


For other type that store binary string like Blob, will TiDB meet the same problem?

No, only the "hybrid" types will address this issue. Their "EvalType" may return ETInt, but when converting them to strings, we should be recognized as bytes instead.

I did not handle other hybrid types like enum and set in this PR, because in most time their should have a legal encoding and another reason is to introduce less logic change in this fix.

windtalker · 2024-11-20T02:18:14Z

pkg/expression/expr_to_pb_test.go

+	castTp := types.NewFieldType(mysql.TypeString)
+	castTp.SetCharset(charset.CharsetBin)
+	castTp.SetCollate(charset.CollationBin)
+	castByteAsStringFunc, err := NewFunction(mock.NewContext(), ast.Cast, castTp, byteColumn)


This function can not be push down to TiKV because from_binary can not be push down to TiKV?

no, I think it's the issue: #51877 The tidb have some special logic for function conv. When converting a BIT type, it uses conv(cast(bit as char)) which uses cast to cast bit to string, but conv should also handles the string as a binary number instead of string...

See:

tidb/pkg/expression/builtin_math.go

Lines 1281 to 1290 in 9530fdc

if x.FuncName.L == ast.Cast {

arg0 := x.GetArgs()[0]

if arg0.GetType(ctx).Hybrid() || IsBinaryLiteral(arg0) {

str, isNull, err = arg0.EvalString(ctx, row)

if isNull || err != nil {

return str, isNull, err

}

d := types.NewStringDatum(str)

str = d.GetBinaryLiteral().ToBitLiteralString(true)

}

I added a comment in this test

This test is not related with this issue, but we should update it to set to casted type as bin collations to make the test pass.

YangKeao

LGTM

tiprow · 2024-11-20T02:24:22Z

@lcwangchao: 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
fast_test_tiprow	`df5cace`	link	true	`/test fast_test_tiprow`

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-sigs/prow repository. I understand the commands that are listed here.

ti-chi-bot · 2025-02-07T03:35:13Z

In response to a cherrypick label: new pull request created to branch release-7.5: #59290.
But this PR has conflicts, please resolve them!

ti-chi-bot · 2025-02-07T03:36:51Z

In response to a cherrypick label: new pull request created to branch release-8.1: #59291.
But this PR has conflicts, please resolve them!

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

lcwangchao · 2025-02-07T07:39:15Z

/cherry-pick release-7.5

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2025-02-07T07:39:59Z

@lcwangchao: new pull request created to branch release-7.5: #59314.
But this PR has conflicts, please resolve them!

In response to this:

/cherry-pick release-7.5

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 ti-community-infra/tichi repository.

lcwangchao · 2025-02-07T07:47:23Z

/cherry-pick release-8.1

ti-chi-bot · 2025-02-07T07:48:05Z

@lcwangchao: new pull request created to branch release-8.1: #59315.
But this PR has conflicts, please resolve them!

In response to this:

/cherry-pick release-8.1

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 ti-community-infra/tichi repository.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

lcwangchao · 2025-02-07T07:59:47Z

/cherry-pick release-7.1

ti-chi-bot · 2025-02-07T08:00:35Z

@lcwangchao: new pull request created to branch release-7.1: #59316.
But this PR has conflicts, please resolve them!

In response to this:

/cherry-pick release-7.1

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 ti-community-infra/tichi repository.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

lcwangchao · 2025-02-07T09:24:45Z

/cherry-pick release-8.1

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2025-02-07T09:27:47Z

@lcwangchao: new pull request created to branch release-8.1: #59321.
But this PR has conflicts, please resolve them!

In response to this:

/cherry-pick release-8.1

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 ti-community-infra/tichi repository.

lcwangchao · 2025-02-07T09:45:06Z

/cherry-pick release-6.5

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2025-02-07T09:45:49Z

@lcwangchao: new pull request created to branch release-6.5: #59324.
But this PR has conflicts, please resolve them!

In response to this:

/cherry-pick release-6.5

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 ti-community-infra/tichi repository.

#59324) close #56494

#59316) close #56494

#59321) close #56494

#59314) close #56494

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 19, 2024

lcwangchao force-pushed the fix_56494 branch from 031c6bb to 6af72e2 Compare November 19, 2024 03:34

expression: fix tikv crash when bool like cast(bit as char)

4ed280e

lcwangchao force-pushed the fix_56494 branch from 6af72e2 to 4ed280e Compare November 19, 2024 04:52

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 19, 2024

add realtikv test

48abcf7

lcwangchao force-pushed the fix_56494 branch from 4108a75 to 48abcf7 Compare November 19, 2024 06:13

fix some tests

df5cace

ti-chi-bot bot added the sig/planner SIG: Planner label Nov 19, 2024

lcwangchao commented Nov 20, 2024

View reviewed changes

pkg/expression/builtin_convert_charset.go Show resolved Hide resolved

lcwangchao requested review from Defined2014 and YangKeao November 20, 2024 01:55

Defined2014 approved these changes Nov 20, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 20, 2024

qw4990 approved these changes Nov 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 Nov 20, 2024

windtalker reviewed Nov 20, 2024

View reviewed changes

YangKeao approved these changes Nov 20, 2024

View reviewed changes

add some comment

561c9d1

ti-chi-bot mentioned this pull request Feb 7, 2025

expression: fix tikv crash when bool like cast(bit as char) (#57484) #59290

Closed

13 tasks

lcwangchao added the needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. label Feb 7, 2025

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Feb 7, 2025

This is an automated cherry-pick of pingcap#57484

1d2e773

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Feb 7, 2025

expression: fix tikv crash when bool like cast(bit as char) (#57484) #59291

Closed

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Feb 7, 2025

This is an automated cherry-pick of pingcap#57484

49933d8

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Feb 7, 2025

expression: fix tikv crash when bool like cast(bit as char) (#57484) #59314

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Feb 7, 2025

This is an automated cherry-pick of pingcap#57484

ed0ed5d

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Feb 7, 2025

expression: fix tikv crash when bool like cast(bit as char) (#57484) #59315

Closed

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Feb 7, 2025

This is an automated cherry-pick of pingcap#57484

dcff133

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Feb 7, 2025

expression: fix tikv crash when bool like cast(bit as char) (#57484) #59316

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Feb 7, 2025

This is an automated cherry-pick of pingcap#57484

7b7a39a

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Feb 7, 2025

expression: fix tikv crash when bool like cast(bit as char) (#57484) #59321

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Feb 7, 2025

This is an automated cherry-pick of pingcap#57484

8f86331

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Feb 7, 2025

expression: fix tikv crash when bool like cast(bit as char) (#57484) #59324

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Feb 8, 2025

expression: fix tikv crash when bool like cast(bit as char) (#57484) (

0bfb501

#59324) close #56494

ti-chi-bot bot pushed a commit that referenced this pull request Feb 10, 2025

expression: fix tikv crash when bool like cast(bit as char) (#57484) (

6562afb

#59316) close #56494

ti-chi-bot bot pushed a commit that referenced this pull request Feb 10, 2025

expression: fix tikv crash when bool like cast(bit as char) (#57484) (

d91b100

#59321) close #56494

ti-chi-bot bot pushed a commit that referenced this pull request Feb 10, 2025

expression: fix tikv crash when bool like cast(bit as char) (#57484) (

d299c4f

#59314) close #56494

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: fix tikv crash when `bool like cast(bit as char)` #57484

expression: fix tikv crash when `bool like cast(bit as char)` #57484

lcwangchao commented Nov 19, 2024

tiprow bot commented Nov 19, 2024

codecov bot commented Nov 19, 2024 •

edited

Loading

lcwangchao commented Nov 19, 2024

tiprow bot commented Nov 19, 2024

lcwangchao commented Nov 19, 2024

tiprow bot commented Nov 19, 2024

qw4990 left a comment

ti-chi-bot bot commented Nov 20, 2024

windtalker Nov 20, 2024

lcwangchao Nov 20, 2024 •

edited

Loading

windtalker Nov 20, 2024

lcwangchao Nov 20, 2024 •

edited

Loading

lcwangchao Nov 20, 2024

lcwangchao Feb 7, 2025

YangKeao left a comment

tiprow bot commented Nov 20, 2024

ti-chi-bot commented Feb 7, 2025

ti-chi-bot commented Feb 7, 2025

lcwangchao commented Feb 7, 2025

ti-chi-bot commented Feb 7, 2025

lcwangchao commented Feb 7, 2025

ti-chi-bot commented Feb 7, 2025

lcwangchao commented Feb 7, 2025

ti-chi-bot commented Feb 7, 2025

lcwangchao commented Feb 7, 2025

ti-chi-bot commented Feb 7, 2025

lcwangchao commented Feb 7, 2025

ti-chi-bot commented Feb 7, 2025

	if x.FuncName.L == ast.Cast {
	arg0 := x.GetArgs()[0]
	if arg0.GetType(ctx).Hybrid() \|\| IsBinaryLiteral(arg0) {
	str, isNull, err = arg0.EvalString(ctx, row)
	if isNull \|\| err != nil {
	return str, isNull, err
	}
	d := types.NewStringDatum(str)
	str = d.GetBinaryLiteral().ToBitLiteralString(true)
	}

expression: fix tikv crash when bool like cast(bit as char) #57484

expression: fix tikv crash when bool like cast(bit as char) #57484

Conversation

lcwangchao commented Nov 19, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Nov 19, 2024

codecov bot commented Nov 19, 2024 • edited Loading

Codecov Report

lcwangchao commented Nov 19, 2024

tiprow bot commented Nov 19, 2024

lcwangchao commented Nov 19, 2024

tiprow bot commented Nov 19, 2024

qw4990 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 20, 2024

[LGTM Timeline notifier]

windtalker Nov 20, 2024

Choose a reason for hiding this comment

lcwangchao Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

windtalker Nov 20, 2024

Choose a reason for hiding this comment

lcwangchao Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

lcwangchao Nov 20, 2024

Choose a reason for hiding this comment

lcwangchao Feb 7, 2025

Choose a reason for hiding this comment

YangKeao left a comment

Choose a reason for hiding this comment

tiprow bot commented Nov 20, 2024

ti-chi-bot commented Feb 7, 2025

ti-chi-bot commented Feb 7, 2025

lcwangchao commented Feb 7, 2025

ti-chi-bot commented Feb 7, 2025

lcwangchao commented Feb 7, 2025

ti-chi-bot commented Feb 7, 2025

lcwangchao commented Feb 7, 2025

ti-chi-bot commented Feb 7, 2025

lcwangchao commented Feb 7, 2025

ti-chi-bot commented Feb 7, 2025

lcwangchao commented Feb 7, 2025

ti-chi-bot commented Feb 7, 2025

expression: fix tikv crash when `bool like cast(bit as char)` #57484

expression: fix tikv crash when `bool like cast(bit as char)` #57484

codecov bot commented Nov 19, 2024 •

edited

Loading

lcwangchao Nov 20, 2024 •

edited

Loading

lcwangchao Nov 20, 2024 •

edited

Loading