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.

Sign up for GitHub

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

Jump to bottom

executor: fix uint type overflow on generated column not compatible with mysql | tidb-test=pr/2088 #40157

Merged
merged 19 commits into from
Apr 19, 2023
Merged

executor: fix uint type overflow on generated column not compatible with mysql | tidb-test=pr/2088 #40157

merged 19 commits into from
Apr 19, 2023

Conversation

jiyfhust
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #40066

Problem Summary:

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

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

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

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

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Dec 24, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • mjonss
  • wjhuang2016

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 24, 2022
@jiyfhust
Copy link
Contributor Author

/cc @xiongjiwei

@jiyfhust jiyfhust changed the title sig: fix uint type overflow on generated column not compatible with mysql executor: fix uint type overflow on generated column not compatible with mysql Dec 24, 2022
@xiongjiwei
Copy link
Contributor

/cc @wjhuang2016

wjhuang2016
wjhuang2016 reviewed Dec 26, 2022
View reviewed changes
executor/write_test.go Outdated
@@ -4299,3 +4299,19 @@ func TestIssueInsertPrefixIndexForNonUTF8Collation(t *testing.T) {
tk.MustExec("insert into t3 select 'abc '")
tk.MustGetErrCode("insert into t3 select 'abc d'", 1062)
}

func TestIssue40066(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add more tests, for example, type char, decimal

table/column.go Outdated
@@ -691,7 +691,9 @@ func FillVirtualColumnValue(virtualRetTypes []*types.FieldType, virtualColumnInd
}
// Because the expression might return different type from
// the generated column, we should wrap a CAST on the result.
sctx.GetSessionVars().StmtCtx.InFillVirtualColumnValue = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting it here may cause a data race. FillVirtualColumnValue() can be called concurrently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @wjhuang2016 , Can you give me some advice on how to fix it? The core reason is the "ShouldClipToZero"，but i have no idea how to solve it properly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid there is not a clean way to do it. But we can add a particular check after CastValue() in FillVirtualColumnValue. We can check its type and value, then clip it to 0 if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wjhuang2016 i'm not very sure how to check it as you mentioned. After CastValue()，it should be a valiad destination type value already，how can judge it properly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can know its type before CastValue(), and validate its value after CastValue(). If it doesn't obey the ShouldClipToZero, then ClipToZero.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 27, 2022
@jiyfhust jiyfhust requested review from wjhuang2016 and removed request for xiongjiwei December 27, 2022 07:35
@mjonss
Copy link
Contributor

mjonss commented Dec 27, 2022

@jiyfhust, please re-merge with master and resolve conflicts, now that #40076 was merged.

@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 28, 2022
mjonss
mjonss reviewed Jan 2, 2023
View reviewed changes
Copy link
Contributor

@mjonss mjonss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will bring up the question about how to avoid the data race with regard to changing the sctx.GetSessionVars().StmtCtx.InFillVirtualColumnValue in our team catchup.

Otherwise LGTM.

executor/writetest/write_test.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2023
@ti-chi-bot
Copy link
Member

@jiyfhust: PR needs rebase.

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.

@jiyfhust
Copy link
Contributor Author

@wjhuang2016 @mjonss how about change this pr only solve the compatible error and warnings message? The select CastValue seems a little complex, an other pr to solve it specially?

@ti-chi-bot ti-chi-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 Jan 11, 2023
@jiyfhust
Copy link
Contributor Author

/cc @wjhuang2016 @mjonss

@tiancaiamao
Copy link
Contributor

/test check-dev

@tiancaiamao
Copy link
Contributor

/test mysql-test

@tiancaiamao
Copy link
Contributor

/test unit-test

@tiancaiamao
Copy link
Contributor

/retest

@tiancaiamao
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: a86c7e7

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 19, 2023
@tiancaiamao
Copy link
Contributor

/retest

@ti-chi-bot ti-chi-bot merged commit 875e34d into pingcap:master Apr 19, 2023
@ti-chi-bot ti-chi-bot mentioned this pull request Apr 19, 2023
Open
12 tasks
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-6.1: #43218.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Apr 19, 2023
@jiyfhust @ti-chi-bot
This is an automated cherry-pick of pingcap#40157
c02c8b6 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Apr 19, 2023
Closed
12 tasks
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-6.5: #43219.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Apr 19, 2023
@jiyfhust @ti-chi-bot
executor: fix uint type overflow on generated column not compatible w…
60aeb16 
…ith mysql (pingcap#40157)

close pingcap#40066
@ti-chi-bot ti-chi-bot mentioned this pull request Apr 19, 2023
Closed
12 tasks
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-4.0: #43220.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Apr 19, 2023
@jiyfhust @ti-chi-bot
This is an automated cherry-pick of pingcap#40157
1294f25 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Apr 19, 2023
Closed
12 tasks
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-5.0: #43221.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Apr 19, 2023
@jiyfhust @ti-chi-bot
This is an automated cherry-pick of pingcap#40157
be3a4c3 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Apr 19, 2023
Closed
12 tasks
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-5.3: #43222.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Apr 19, 2023
@jiyfhust @ti-chi-bot
This is an automated cherry-pick of pingcap#40157
fc851b4 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Apr 19, 2023
Open
12 tasks
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-5.4: #43223.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Apr 19, 2023
@jiyfhust @ti-chi-bot
This is an automated cherry-pick of pingcap#40157
371dca8 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@jiyfhust jiyfhust deleted the generated-column-compatible branch April 20, 2023 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjhuang2016 wjhuang2016 wjhuang2016 approved these changes

@mjonss mjonss mjonss approved these changes

Assignees
No one assigned
Labels
needs-cherry-pick-release-4.0 needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.3 Type: Need cherry pick to release-5.3 needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. needs-cherry-pick-release-6.1 Should cherry pick this PR to release-6.1 branch. needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. ok-to-test Indicates a PR is ready to be tested. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

generated column is not compatible with mysql
7 participants
@jiyfhust @ti-chi-bot @xiongjiwei @mjonss @wjhuang2016 @bb7133 @tiancaiamao